crate / sqlalchemy-cratedb

SQLAlchemy dialect for CrateDB.
https://cratedb.com/docs/sqlalchemy-cratedb/
Apache License 2.0
3 stars 2 forks source link

SQLAlchemy 1.4: On ORM DELETE operations, `rowcount=-1` is wrong #107

Closed amotl closed 1 year ago

amotl commented 1 year ago

Hi there,

@thdud1744 and @AndreyKuchko reported a problem when migrating from SA13 to SA14. I've created two gists which approach and narrow down the problem in a reproducible way.

The first example ^1 runs a few insert and delete operations using the SQLAlchemy APIs in all different ways of:

The problem is only present on SQLAlchemy 1.4 in ORM-only mode, so the second example ^2 focuses on this. The program works well on SQLAlchemy 1.3 and 2.0, where it correctly returns the number of deleted records (2). However, on SQLAlchemy 1.4, it returns -1 there.

With kind regards, Andreas.

amotl commented 1 year ago

Apparently, while SQLAlchemy 1.3 does not do it ^1, on SQLAlchemy 1.4, sqlalchemy.orm.query.Query.delete starts invoking result.close() before accessing result.rowcount ^2. SQLAlchemy 2.0 also does it this way ^3, the new sqlalchemy.engine.cursor.CursorResult apparently can return the rowcount properly, while the previously used sqlalchemy.engine.cursor.LegacyCursorResult can't.

amotl commented 1 year ago

SQLAlchemy 2.0 will standardize the production of SELECT statements across both Core and ORM by making direct use of the Select object within the ORM, removing the need for there to be a separate Query object.

-- SQLAlchemy » Querying (2.0 style)

Because 2.0-style querying is already supported by SQLAlchemy 1.4, and .rowcount is properly populated when using it, I think we will not try to fix this for SQLAlchemy 1.4 [^1].

If you still need to work around the problem on SQLAlchemy 1.4, you would need to make sqlalchemy.engine.cursor.LegacyCursorResult.close a no-op. Please note this not in any way an official recommendation, it is merely only what I've figured out when poking around the code of SQLAlchemy 1.4.

from unittest import mock

with mock.patch("sqlalchemy.engine.cursor.LegacyCursorResult.close"):
    # your code

Again, the recommendation is to use 2.0-style querying, and NOT applying this workaround.

[^1]: Please raise your voice if you think differently.