Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use sqlalchemy.orm.session instead of databases? #308

Closed
lefnire opened this issue Aug 22, 2020 · 14 comments
Closed

Use sqlalchemy.orm.session instead of databases? #308

lefnire opened this issue Aug 22, 2020 · 14 comments
Labels
question Further information is requested

Comments

@lefnire
Copy link

lefnire commented Aug 22, 2020

Got a decent-sized project already built on SQLAlchemy. I'm using fastapi-sqlalchemy to inject a Session object into routes. Alternatively, FastAPI's documentation uses samples of SessionLocal = sessionmaker(... bind=engine) used with injection:

# Dependency
def get_db():
    db = SessionLocal()
    try:
        yield db
    finally:
        db.close()
...
@app.post("/users")
def create_user(..., db: Session = Depends(get_db)):
    ...

I figured: that's fine, I'll have fastapi-users's use of databases alongside my use of SQLAlchemy. But I'm hitting all sorts of snags in how fastapi-users loads the user model for downstream tasks, and how my app intends to work with such. Pretty vague, I'll update this ticket with specific issues as I unravel them (currently just error-mania and hard to wrap my head around). But in the meantime, I'm wondering what your thoughts are on supporting passing a sqlalchemy.orm.session object into SQLAlchemyUserDatabase instead of a databases.Database object? Something like:

# change this
database = databases.Database(DB_URL)
fastapi_users = FastAPIUsers(
    SQLAlchemyUserDatabase(UserDB, database, User.__table__), 
    [jwt_authentication], User, UserCreate, UserUpdate, UserDB,
)
# to this
engine = sqlalchemy.create_engine(DB_URL)
Sess = sqlalchemy.orm.sessionmaker(autocommit=False, autoflush=False, bind=engine)
fastapi_users = FastAPIUsers(
    SQLAlchemyUserDatabase(UserDB, Sess, User.__table__), 
    [jwt_authentication], User, UserCreate, UserUpdate, UserDB,
)
@lefnire
Copy link
Author

lefnire commented Aug 22, 2020

My issues were actually Pydantic setup. My previous schemas weren't working with the new setup, just had to migrate over; my bad. It still might be worth considering allowing sa.orm.Session instead of databases.Database though, to reduce needing two active database connections per request, easing load on the database.

@lefnire lefnire closed this as completed Aug 22, 2020
@lefnire
Copy link
Author

lefnire commented Aug 23, 2020

Ah, I found my snag. I'm using Pydantic orm_mode=True in schemas.UserDB to marshal out custom SQLAlchemy @property fields, and other non-simple SQLAlchemy bits (not just columns). Those weren't loading on the user object in the dependency callbacks (user: User = Depends(fastapi_users.get_current_user). I imagine fastapi-users is loading this dependency via databases.Database, rather than SQLAlchemy (I need to dig into code)? So I had to instead have schemas.UserDB be empty (as per the docs), and then separately-load my SQLAlchemy User model at the top of each route so I can work with it how I intend. Indeed, it's very non-ideal and causes double the database-querying. It would be better - (1) to reduce double-querying the DB, (2) consistency in projects when we're using SQLAlchemy to be able to stick to our own SQLAlchemy code - if we can use our already-existing Sessions. I'm gonna re-open.

@lefnire lefnire reopened this Aug 23, 2020
@frankie567 frankie567 added the question Further information is requested label Aug 26, 2020
@frankie567
Copy link
Member

Well, I've to admit I've not understood everything here.

Concerning the SQLAlchemy ORM issue, my advice would be to implement a custom DB adapter that uses SQLAlchemy ORM. It's pretty easy, all you have to do is extend BaseUserDatabase and implement the abstract methods: https://github.com/frankie567/fastapi-users/blob/master/fastapi_users/db/base.py

You can take inspiration from the SQLAlchemy Core / databases adapter : https://github.com/frankie567/fastapi-users/blob/master/fastapi_users/db/sqlalchemy.py

It can directly live in your project so that you can make it work just like you wish.

@lefnire
Copy link
Author

lefnire commented Aug 26, 2020

Oh fantastic, that's just what I need thanks! Yeah sorry for the jumble, the short of it is I'm super heavy on the sqlalchemy use in my app, and I realized I had to double load users (once through the Depends, and again to get my alchemy model helper properties). Not a biggie once I realized that, just took me a while. I'll make an adapter

@lefnire lefnire closed this as completed Aug 26, 2020
@sid3r
Copy link

sid3r commented Mar 6, 2021

@lefnire I'm facing the same issue, I've done heavy work relying on sqlalchemy and now I'm stuck, have you been able to use the sqlachemy.orm.session ?

@lefnire
Copy link
Author

lefnire commented Mar 6, 2021

@sid3r Frankie's right that you'll just need to write an adapter. Should be pretty minimal, but I never got around to it. I just dealt with the DB double-ping, not at a place currently where it's hurting performance so I figure I'd get to it when I get to it. That is: just use fastapi-users to auth the user, and what it returns to you you'll just throw away; re-fetch the user via User.get(fastapi_users_user.id).

In the end (I'm so sorry Frankie, don't hate me!) I moved to Cognito. I think this project is wonderful for quick up-and-running, but my project is super security-hungry, and needs all the extra auth tools (MFA, key-rotation, remember device, logout multiple, etc) so it just saves time & anxiety.

@sid3r
Copy link

sid3r commented Mar 7, 2021

@lefnire I see, the package could consider sqlalchemy and other famous ORM which developpers rely on to manage databses on FastAPI, but it's not the case here. I don't like the idea of using two DB connections for a simple thing like auth, so I'll find something else like you did. thank you for the quick reply.

@lefnire
Copy link
Author

lefnire commented Mar 7, 2021

@sid3r yeah, fastapi-users integrates well with SQLAlchemy for setting up the schema. It's just on the user-fetching bit it doesn't pull the SQLA model. I hunch they went that way for interop, and use databases for async (though the most recent SQLA supports async). Even if this wasn't an issue, and even if it supported xyz features, I feel that auth isn't one of those things I wanna mess with with - too risky / hairy. Unless you need on-prem, let the megacorps handle that part. Cognito auths client-side, returns a JWT - decode JWT in fastapi, save/fetch user from SQLA.

@frankie567
Copy link
Member

Didn't know SQLAlchemy had implemented async support. I'll consider writing a new adapter for this.

As a side note, I would like to point out that we support several ORMs already: Tortoise ORM and Ormar. If you think about other DBMS/ORM, feel free to implement an adapter for it, would be happy to help.

@sid3r
Copy link

sid3r commented Mar 7, 2021

@frankie567 Haha, Sorry I went too far in my previous comment, indeed, the package already supports many ORMs like you said. Most of devs relying on sqlalchemy use session to manage async database transactions, implementing it in the package would be a great enhancement for futur releases. thanks for the package again.

@sid3r
Copy link

sid3r commented Mar 7, 2021

@lefnire Yep, the entire web stack is going Async, the languages are merging into complete spaghetti lol, they all copy one another to stay in the market. I don't share your thoughts on Auth, I don't like my web apps to rely too much on a heavy corp like Amazon, I'm now implementing my own auth logic for FastApi, may take a day or two but it's worth the effort.

@lefnire
Copy link
Author

lefnire commented Mar 7, 2021

@sid3r in that case, and especially if you're just starting, I'm with @frankie567 on using Tortoise. I can't because of a bunch of specialized plugins I'm using (eg encrypted columns); but I would in the future.

This is personal opinion, so grain of salt, but I feel like SQLAlchemy is to Tortoise what Flask is to FastAPI: legacy.

@sid3r
Copy link

sid3r commented Mar 7, 2021

@lefnire Yea, I'm just starting, first time using FastApi so I went with the 'legacy' solutions, I don't bother redoing the models ans schemas if it's a good foundation for the rest of the project, TortoiseORM looks beautiful I'll give it a try and see. Thanks for the suggestion.

@silllli
Copy link
Contributor

silllli commented Mar 25, 2021

Didn't know SQLAlchemy had implemented async support. I'll consider writing a new adapter for this.

I would be thankful! I am also just starting out with FastAPI, but SQLAlchemy seems to be the most robust or tested solution available. Probably going for the (temporary) solution @lefnire described to use SQLAlchemy ORM aside fastapi-users[sqlalchemy].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants