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

Backend: investigate a simpler solution for SQLAlchemy session management in FastAPI #22

Open
benoit74 opened this issue Sep 5, 2023 · 2 comments
Labels
enhancement New feature or request investigation_needed Expertise needed to clearly define the issue stale
Milestone

Comments

@benoit74
Copy link
Collaborator

benoit74 commented Sep 5, 2023

In every API method, we need an SQLAlchemy session to interact with the database.

FastAPI does not like the @dbsession decorator we usually use because it makes it consider that we should pass args and kwargs query parameter.

Currently the backend is hence using dependency injection to inject the SQLAlchemy session. The drawback is that there is no more autocommit, we have to commit manually inside the API methods. And should we forget to commit, the transaction is rolled-back silently because the injected dependency is cleaned-up after the HTTP Response is sent AND it is cleaned-up with a GeneratorExit exception, which cause a transaction rollback. More details are provided in FastAPI documentation e.g. here.

The other solution mentionned in FastAPI documentation would be to use an ASGI Middleware.
It works, but is not easier / cleaner than the dependency injection approach:

  • the SQLAchemy session has to be injected inside the request scope, which makes the code a bit weird
  • we cannot benefit from exception handler inside the middleware, looks like the response has already been sent or at least too much prepared once the exception handler is triggered. For instance we cannot have an exception handler managing all IntegrityError exceptions, which is kind of an issue

We have to investigate what could be a cleaner solution, maybe my ASGI / decorator experience is too limited or I missed an obvious solution. The solution would benefit other projects like openzim/nautilus-web (and next ones, or Zimfarm once we would have migrated it to FastAPI).

@benoit74 benoit74 added enhancement New feature or request investigation_needed Expertise needed to clearly define the issue labels Sep 5, 2023
@benoit74 benoit74 added this to the v1 milestone Nov 2, 2023
@benoit74 benoit74 modified the milestones: v1, v2 Dec 1, 2023
@benoit74
Copy link
Collaborator Author

benoit74 commented Dec 1, 2023

Moving this to v2 since we have very few endpoints + they are all readonly so we do not really mind.

Copy link

stale bot commented Jan 31, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

@stale stale bot added the stale label Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request investigation_needed Expertise needed to clearly define the issue stale
Projects
None yet
Development

No branches or pull requests

1 participant