db: upgrade flask-sqlalchemy #97
Conversation
pytest_invenio/fixtures.py
Outdated
| @@ -490,7 +490,7 @@ def db(database): | |||
| transaction = connection.begin() | |||
|
|
|||
| options = dict(bind=connection, binds={}) | |||
| session = database.create_scoped_session(options=options) | |||
| session = database._make_scoped_session(options=options) | |||
There was a problem hiding this comment.
Comment in an offline chat: we should not use a private method.
Explanation: the method create_scoped_session in flask-sqlalchemy v2 was not private. In v3, it has been replaced by _make_scoped_session, a private method.
Here a relevant issue explaining possible solutions: jeancochrane/pytest-flask-sqlalchemy#63
- use the private method
- copy/paste the implementation of the private method and use it directly
There was a problem hiding this comment.
I would go with using the private function directly. That way, if they remove it we have a clear failure that we will need to fix. If we copy the code, then internal changes in v3, e.g how the sessions are working, might be quite more difficult to be identified and fixed.
There was a problem hiding this comment.
what about https://docs.sqlalchemy.org/en/20/orm/contextual.html#contextual-thread-local-sessions ? is it useful for us in this case?
There was a problem hiding this comment.
Yes that is the solution 2, copy/paste the internal implementation
There was a problem hiding this comment.
then I would follow the solution 2 since it is exposed as the way of creating the scoped sessions in their documentation, I would expect the public functions exposed in the docs would be more stable than a private method, and I don't see why we would need to go and fix the method each time it is changed internally, rather than relying on something meant to be used, which should keep the backwards compatibility within the minor release range
There was a problem hiding this comment.
this is the solution from davidism to solve the rollback problem for a fixture. i am not sure if that would be enough for our case, but it seams the more stable approach. is it necessary that we use the session somehow in hour fixture function? if that is necessary then the solution would be not usable, but otherwise we should think about that.
There was a problem hiding this comment.
@utnapischtim the suggested change is not working, weird errors. I replaced the code with the classic way of creating a scoped session with SQLAlchemy.
There was a problem hiding this comment.
@ntarocco i tested it too, and i had also weird errors. so no good solution for this use case as you said
ntarocco
force-pushed
the
db-flask-sqlalchemy
branch
3 times, most recently
from
5b1eb35 to
8dd787c
Compare
* replaces the previous public method `create_scoped_session` from flask-sqlalchemy with the direct way of creating a new session with SQLAlchemy. Related GitHub issue: jeancochrane/pytest-flask-sqlalchemy#63
ntarocco
force-pushed
the
db-flask-sqlalchemy
branch
from
8dd787c to
d1f2629
Compare
No description provided.