Closed marksparkza closed 3 years ago
The approach described in the blog is not ideal.
The change introduced by commit 9c145f3 violates Important Point Number 2 in this SQLAlchemy Session FAQ, to wit:
- Make sure you have a clear notion of where transactions begin and end, and keep transactions short, meaning, they end at the series of a sequence of operations, instead of being held open indefinitely.
In the blog approach, there is no clear notion of where or when transactions begin and end. Also, the Flask app.after_request
hook may be executed more than once for a given user-initiated request; in the Admin UI, at least, it is reached first with a request status of 302, and then with a 200. It is not clear whether it is appropriate to assume that a commit should be done in either or both of these cases. The blog approach furthermore does not consider:
While it is a good idea to associate the session lifecycle with the request lifecycle (which we were already doing by calling session.remove()
in the app.teardown_request
hook) transaction scope should be managed separately and be clearly associated with the DB updates it encloses - e.g. using a context manager with
block.
In the case of FastAPI, the blog approach did not work - the commit at the end of the request lifecycle happened in a different session to that in which the object was updated and flushed. This seems to be because the model updates and dependency-injected commit were being done in different threads, while the SQLAlchemy scoped_session
is by default thread local. The thread local session works in Flask because each request is handled by a single worker thread.
We should perhaps consider defining distinct mechanisms for handling session and transaction scope specific to each of the ODP application types - API, Flask app, executable script. There might be a case for defining a custom scope tied to the FastAPI request.
This blog post provides a good example of how this can be done.