Review encapsulation of SQLAlchemy and Psycopg2 DBALs

Working on issue #4251 (closed) revealed, that the SQLAlchemy and/or Psycopg2 DBALs may not be optimally used within the project. For example cursors or sessions my be abused in a way that goes against best practices, which can cause troubles like those in #4251 (closed), because sessions/transaction can get stuck.

It would be wise to review current implementation.

assigned to @Jan_Mach

By Jan Mach on 2024-12-26T14:06:53

added C-Dev-Core Done 0% P4 S-New T-Task labels

changed milestone to %2.7

Back Relation 'relates' from: 4251

Relation 'relates' to: 7619

Relation 'relates' to: 7642

Relation Relates - Bug 4251 User data access through SQLAlchemy keeps a transaction open

By Jan Mach on 2020-01-24T14:32:51

changed milestone to %2.8

assigned to @rajmund_hruska and unassigned @Jan_Mach

By Rajmund Hruška on 2024-12-26T14:20:02

This issue stems from #4251 (closed), where a workaround for a specific transaction handling bug was found and recommended. Based on the videocall discussion a more streamlined approach is required, possibly by managing the transaction lifetime by limiting the life of a Session object. Following the official documentation [1], the upcoming version of SQLAlchemy (v1.4), currently in beta, includes a @Python@'s context manager based workflow with Session as a transaction manager and Engine as some form of a connection manager. Based on [1], the best approach would be to keep a reference to Engine, instantiate a Session from it using a context manager and perform the data-focused actions inside that context. This is perhaps better documented by a code excerpt from [1]:

# create session and add objects
with Session(engine) as session, session.begin():
    session.add(some_object)
    session.add(some_other_object)
# inner context calls session.commit(), if there were no exceptions
# outer context calls session.close()

The current version 1.3 does not support the context manager approach while supporting the @Engine@/Session distinction, so a similar solution is possible [2], but the user (programmer) must both keep a reference to sessionmaker and carefully manage the lifetime of the Session object, what is error prone. It should nevertheless be a feasible approach.

Waiting for the next version might make sense here, although it is of note that the minimum required version of Python is bumped to 3.6 there [3] (not an issue for us as we are currently using 3.7.3, but might cause problems elsewhere). The migration document also promises various performance improvements in the next version.

The required changes to the code are non-trivial for both versions and the migration to v1.4 also seems to require some rewrites. I advise to do both at the same time and postpone this issue until after the SQLAlchemy v1.4 release.

[1] https://docs.sqlalchemy.org/en/14/orm/session_basics.html [2] https://docs.sqlalchemy.org/en/13/orm/session_basics.html [3] https://docs.sqlalchemy.org/en/14/changelog/migration_14.html

By Radko Krkoš on 2020-12-21T15:24:21

As for the flask-sqlalchemy, it is just a simple adaptor [1] and seems to take the sessionmaker route [2]. It also seems to manage the lifetime of the session subobject. Although it is not obvious from the example code [3] (why would an object's attribute lifetime be different from the object itself?), the documentation refers to the session as scoped and declares [4]:

You have to commit the session, but you don’t have to remove it at the end of the request, Flask-SQLAlchemy does that for you.

So the way of flask-sqlalchemy should be a viable approach to correctly access the database and if not, it is a bug somewhere in the flask-sqlalchemy@SQLAlchemy@`psycopg2` stack and should be treated as such.

This is all based on code review and study of the documentation, not tested hard evidence.

[1] https://github.com/pallets/flask-sqlalchemy/blob/master/src/flask_sqlalchemy/model.py
[2] https://flask-sqlalchemy.palletsprojects.com/en/2.x/api/#flask_sqlalchemy.SQLAlchemy
[3] https://flask-sqlalchemy.palletsprojects.com/en/2.x/quickstart/#a-minimal-application
[4] https://flask-sqlalchemy.palletsprojects.com/en/2.x/quickstart/#road-to-enlightenment

By Radko Krkoš on 2020-12-22T06:57:22

Attachment: test_flask_sqlalchemy.py

By Radko Krkoš on 2021-01-04T11:51:41

Further investigating the flask-sqlalchemy, a simple testing script was prepared (attached), to determine the session management behaviour. The output, especially section "Commit - transaction start", shows that commit() in fact does not immediately start another transaction and that is deferred until the next query. Therefore, a viable approach using flask-sqlalchemy is:

from flask import Flask
from flask_sqlalchemy import SQLAlchemy

app = Flask(__name__)
db = SQLAlchemy(app)


# New transaction
db.session.query(...)
db.session.commit()

# New transaction
db.session.query(...)
db.session.commit()

...

as opposed to rollback() that is required using the current architecture based on pure SQLAlchemy.

Add file test_flask_sqlalchemy.py.

By Radko Krkoš on 2021-01-04T11:51:41

Consensus on 2021-01-11 meeting was that the correct way most probably is switching to flask-sqlalchemy, which hides some sqlalchemy idiosyncracies. We would than be able to remove weird rollback introduced in #4251 (closed).

By Pavel Kácha on 2021-01-13T13:25:49

changed milestone to %2.9

I tried using flask_sqlalchemy to get rid of that rollback in 0d12bd68. Although the code is executable and all tests passed, when I check the running queries I see that the query from #4251 (closed) is still running. So the original problem is not solved yet.

From my understanding, I used the flask_sqlalchemy in the same way as in the example posted in https://gitlab.cesnet.cz/709/migrace/homeproj/mentat-project-05/mentat-ng/-/issues/6205#note_199730 so it should be working. That is not the case though.

By Rajmund Hruška on 2021-10-07T13:03:43

added S-Feedback label and removed S-New label

Rajmund Hruska wrote in https://gitlab.cesnet.cz/709/migrace/homeproj/mentat-project-05/mentat-ng/-/issues/6205#note_199733:

From my understanding, I used the flask_sqlalchemy in the same way as in the example posted in https://gitlab.cesnet.cz/709/migrace/homeproj/mentat-project-05/mentat-ng/-/issues/6205#note_199730 so it should be working.

Not quite. You need to signal an end of transaction to the ORM somehow. In https://gitlab.cesnet.cz/709/migrace/homeproj/mentat-project-05/mentat-ng/-/issues/6205#note_199730, it is done by calling db.session.commit(). In whois.py, after being done with the loop of queries (somewhere about where previously the storage.session.rollback() was), the storage.session.commit() method should be called. That should close the running transaction, without it, the transaction will remain open. The symptoms are the same as for #4251 (closed), but the cause is not a deficiency in the new ORM (as was with the prior one), but in the way it is used, see https://gitlab.cesnet.cz/709/migrace/homeproj/mentat-project-05/mentat-ng/-/issues/6205#note_199729 and original the source [4,#note-6] for more details.

I did not run any code now, so take all this with a grain of salt. Is my reasoning understandable?

By Radko Krkoš on 2021-10-08T06:49:35

As was discussed on the meeting, with regards also to #4569, access to SQLAlchemy's internals is needed even in the flask_sqlalchemy. Based on the documentation [1], this might work:

from flask_sqlalchemy import BaseQuery

[1] https://flask-sqlalchemy.palletsprojects.com/en/2.x/customizing/#query-class

By Radko Krkoš on 2021-10-08T08:36:12

Radko Krkoš wrote in https://gitlab.cesnet.cz/709/migrace/homeproj/mentat-project-05/mentat-ng/-/issues/6205#note_199735:

As was discussed on the meeting, with regards also to #4569, access to SQLAlchemy's internals is needed even in the flask_sqlalchemy. Based on the documentation [1], this might work:

[...]

[1] https://flask-sqlalchemy.palletsprojects.com/en/2.x/customizing/#query-class

I found out that Query can be found at flask_sqlalchemy.orm.Query, so I used that class.

I can't fully remove sqlalchemy module because sqlalchemy.exc.OperationalError is used and exc module is "not present in the flask_sqlalchemy module":https://github.com/pallets/flask-sqlalchemy/issues/764.

In 7c1dce70 I also added the RetryingQuery back, which I removed in the previous commit just for test purposes.

As it was discussed during the meeting, I will check if flask_sqlalchemy can be used at other places where insert/delete queries are used.

By Rajmund Hruška on 2021-10-08T09:03:06

added Done 50% S-In Progress labels and removed Done 0% S-Feedback labels

Rajmund Hruska wrote in https://gitlab.cesnet.cz/709/migrace/homeproj/mentat-project-05/mentat-ng/-/issues/6205#note_199737:

I can't fully remove sqlalchemy module because sqlalchemy.exc.OperationalError is used and exc module is "not present in the flask_sqlalchemy module":https://github.com/pallets/flask-sqlalchemy/issues/764.

I have found:

flask_sqlalchemy.orm.exc.sa_exc.OperationalError

Not thoroughly tested.

Also, there seems to be proper support for model definition:
https://flask-sqlalchemy.palletsprojects.com/en/2.x/models/

Lastly, I would advise to redo the patches from scratch, the commit history is getting hard to understand.

By Radko Krkoš on 2021-10-08T09:35:48

Radko Krkoš wrote in https://gitlab.cesnet.cz/709/migrace/homeproj/mentat-project-05/mentat-ng/-/issues/6205#note_199738:

Rajmund Hruska wrote in https://gitlab.cesnet.cz/709/migrace/homeproj/mentat-project-05/mentat-ng/-/issues/6205#note_199737:

I can't fully remove sqlalchemy module because sqlalchemy.exc.OperationalError is used and exc module is "not present in the flask_sqlalchemy module":https://github.com/pallets/flask-sqlalchemy/issues/764.

I have found:

[...]

Not thoroughly tested.

I tried running the following code:

import flask_sqlalchemy
import sqlalchemy

def func():
    raise sqlalchemy.exc.OperationalError('Test', orig=None, params=None)

def func2():
    try:
        func()
    except flask_sqlalchemy.orm.exc.sa_exc.OperationalError:
        print('caught')

func2()

After executing the code, 'caught' was printed on the screen so I assume it is possible to use flask_sqlalchemy.orm.exc.sa_exc.OperationalError instead of sqlalchemy.exc.OperationalError.

Also, there seems to be proper support for model definition:
https://flask-sqlalchemy.palletsprojects.com/en/2.x/models/

I will look into that in more detail.

Lastly, I would advise to redo the patches from scratch, the commit history is getting hard to understand.

I will create a new branch.

By Rajmund Hruška on 2021-10-11T14:08:26