[RFC] SQLAlchemyUserDatabase with engine injection instead of session injection

[francoposa]

Hi!

Huge fan of fastapi-users.

This library in particular is a huge help to spin up with it quickly, since I prefer to use Postgres + SQLAlchemy backend for my database repository implementations.

However, I find this library hard to use in the standard way I like, which is to perform all dependency injection on startup, instantiating the database engine and injecting it into the repository implementation (whose role is played here by SQLAlchemyUserDatabase) only once, in the main file (example here). The repository instance then uses the engine to instantiate the sessions as needed, encapsulating the repo functionality behind a domain-layer interface which is completely agnostic of DB implementation details - no need for the consumer of the interface to know that it's using SQLAlchemy or any other infrastructure details.

However, the API of the SQLAlchemyUserDatabase class in this library only can take a session, meaning the session must be instantiated outside of the class, then the class must be instantiated each time the request handler is called. I have found it very difficult (maybe impossible?) to maintain proper clean/hexagonal/layered architecture this way, as all callers of the repository methods must supply a database session, which is not something business logic code should be concerned with.

Would you be open to introducing something like this into this library? It's not a major change, and I could of course maintain a fork for my own purposes, but I hope it could be better for everyone to include the option in this upstream library that the vast majority of people would use by default.

The changes I have here are of course not compatible with the existing API of SQLAlchemyUserDatabase, but are just a proof of concept - I figured I'd wait to go for compatibility after getting your opinion on the matter.

The implementations could be made compatible with the addition of the option to provide engine or session to the class init, then handling things accordingly within the session helper method, like:

def session(self) -> AsyncSession:
    if self._session:  # was provided on class instantiation instead of an engine
        return self._session
    # otherwise the session was not provided on class init, so we create one from the engine
    return sessionmaker(self.engine, class_=AsyncSession)()

Alternatively, the version with engine injected could be a completely separate class with this slightly different implementation.

[frankie567]

When I first implemented this DB adapter, I used an approach where we pass the engine instead of a session. However, this approach doesn't really fit in the FastAPI and SQLAlchemy philosophy.

Indeed, the good thing about the current approach is that the approach doesn't have to know anything about how a session is created. In a FastAPI context, this session is instantiated once when we start to handle the request and closed (or rollback) at the end of the session. This is achieved very easily by using dependency injection, as showed here in the documentation: https://fastapi-users.github.io/fastapi-users/configuration/databases/sqlalchemy/#create-the-database-adapter-dependency

async def get_async_session() -> AsyncGenerator[AsyncSession, None]:
    async with async_session_maker() as session:
        yield session

This dependency callable is absolutely key here, and nothing stops you from having something more complex to instantiate it. Typically, it can be overridden during tests so each test can run on a test database and rollback at the end of the test for the next one. Said another way, the database adapter shouldn't be responsible for creating session, this is something that should be done at application level.

The key takeaway here is that the FastAPI Users database adapter is included in the request session, not apart.

I hope that clarifies things a bit. From my point of view, I don't really feel the need of changing this approach right now. But I'm open to discuss it further.

[francoposa]

Thanks for the response!

Everything you said is fair, it seems like just two different approaches to the abstractions.

I prefer to have architecture a bit more like:

--------------------------------------------------------
Application layer
- UserHandler
  - injected on startup with fully-instantiated domain-layer constructs: UserService (aka UserManager)
  - does very little: determines if the HTTP/GRPC request is structured properly
  - parses reqeusts into domain objects (via adapters), then calls into UserService with the objects
  - handles domain-layer errors from UserService and applies HTTP/GRPC error codes
  - never does anything with a database without going through domain interfaces
  - no concept of sessions/transactions
--------------------------------------------------------
Domain layer
- UserService/UserManager
  - instantiated on started, injected with *implementations* of Repository interfaces
  - handles business/domain logic, e.g. password length requirements, which information is required for user signup, etc.
  - never does anything directly with a database without going through Repository interface implementations
  - ultimately, does not need to know if the Repository implementation hits an HTTP API, a database, or a file, as long as interface is satisfied and transactionality expectations are satisfied
  - If session/transaction management is needed, it works through a UnitOfWork abstraction - again, no need to know database/repository specifics
- IUserRepository interface definition
  - leaves implementation up to Infrastructure layer
  - if interface methods like update_user do not take a unitofwork argument, they are assumed to be a single transaction
  - if interface methods take a unitofwork argument, they can be chained together with unitofwork management done by the UserService 
--------------------------------------------------------
Infrastructure layer
- Implementation of IUserRepository for a given datastore, like PGSQLUserRepository
  - instantiated on startup with a UnitofWorkManager abstraction, which wraps the Postgres engine
  - handles all database details - again, callers of this implementation of the IUserRepository interface do not need to be concerned about which backing store is behind it.

In my experience, frameworks lean into overly-coupled patterns such as starting a session/transaction in the HTTP handler layer without even using a UnitOfWork abstraction, simply because it offers fast time-to-market and allows the framework to demonstrate value to developers quickly. However this level of coupling leads to tightly-would ball-of-mud apps over time as the codebase grows and ossifies.
I really like that generally FastAPI allows you to trivially opt out of these approaches because it is so composable and modular, even if its documentation tends to recommend the more-coupled go-fast approach. In contrast, frameworks like Django make this borderline impossible.

All in all if you don't agree, I'll figure out another way, no worries. Besides forking, another approach I have considered is to keep this library as-is and just write a UserRepository wrapper around it and use a UnitOfWork abstraction to pass in the session.

I appreciate your work on this project, without it I wouldn't have all this functionality I want with only a 40-line diff.

Thanks!

[frankie567]

I totally get what you mean and theoretically, I would tend to agree about an architecture like this.

However, in my opinion, the purpose of frameworks and libraries like FastAPI Users is actually to provide easy-to-use, easy to understand and fast time-to-market abstractions. Yes, it makes your code quite opinionated and tightly coupled to those assumptions; and I agree with you that it makes things difficult if one day we want to change the framework, the database engine or whatever. But, who really does this? From my experience, that's really rare. The "bad" abstraction that blocks a project usually comes from the end-developer, not the framework, the ORM or related libraries.

For the specific case of FastAPI Users, one of my main frustration right now is that it's already quite difficult to setup and make understand all its blocks and pieces. If we add even more abstraction that's not common in FastAPI projects, I think we'll lost lot of people there.

That said, I completely understand that you want to apply this approach, and that's totally fine. FastAPI is indeed very good at giving you choices about how to do things.

I think you can achieve something like you want by Implementing a custom database adapter working directly with your infrastructure layer, instead of a SQLAlchemy session or even engine. In other words, it's just a layer between FastAPI Users and your database layer.

[francoposa]

Thanks, I appreciate the input!