zopefoundation / zope.sqlalchemy

Integration of SQLAlchemy with transaction management
Other
32 stars 34 forks source link

ORM-enabled updates and SQLAlchemy 1.4 #67

Closed silenius closed 3 years ago

silenius commented 3 years ago

Hello,

I'm upgrading an application which is using Pyramid and thus zope.sqlalchemy for the transaction management. It looks like there is an issue with ORM-enabled updates, which is a SQLAlchemy 1.4 thing (https://docs.sqlalchemy.org/en/14/orm/session_basics.html#update-and-delete-with-arbitrary-where-clause see 2.0 style, Session.execute()).

What I did:

The underlying engine and session is configured as the following: https://github.com/silenius/amnesia/blob/sqlalchemy_1_4/amnesia/db/__init__.py

My view statement is https://github.com/silenius/amnesia/blob/sqlalchemy_1_4/amnesia/modules/content/views/move.py#L25-L41

which call https://github.com/silenius/amnesia/blob/sqlalchemy_1_4/amnesia/modules/content/resources.py#L122-L166 under the hood.

What I expect to happen:

transaction gets commited

What actually happened:

It looks like doing an UPDATE at the ORM layer does not make the transaction dirty and thus it's always rolled back

What version of Python and Zope/Addons I am using:

This is with Python 3.7.9 running on FreeBSD 13 with a PostgreSQL database and:

pyramid-retry==2.0
pyramid-tm==2.4
pyramid==2.0
psycopg2==2.8.5
zope.sqlalchemy==1.4
SQLAlchemy==1.4.20
transaction==3.0.1

I also tried with https://github.com/zopefoundation/zope.sqlalchemy/pull/66 but it doesn't make any difference

silenius commented 3 years ago

The SQLAlchemy mentions that (see "Additional Caveats for ORM-enabled updates and deletes" paragraph on https://docs.sqlalchemy.org/en/14/orm/session_basics.html#selecting-a-synchronization-strategy):

In order to intercept ORM-enabled UPDATE and DELETE operations with event handlers, use the SessionEvents.do_orm_execute() event.

and I'm wondering if zope.sqlalchemy shouldn't listen on that event too in https://github.com/zopefoundation/zope.sqlalchemy/blob/master/src/zope/sqlalchemy/datamanager.py#L377-L382 ?

silenius commented 3 years ago

... on the other hand the SQLAlchemy docs also says that:

"The ORM-enabled UPDATE and DELETE features bypass ORM unit-of-work automation in favor being able to emit a single UPDATE or DELETE statement that matches multiple rows at once without complexity."

so I'm not sure...

jvanasco commented 3 years ago

I believe this is happening because you have synchronize_session=False, although I am not 100% sure it would change the session state if synchronize_session where True and no matching objects were in the session.

Anyways, this is the expected behavior. You want to use mark_changed from the docs.

see:

https://github.com/zopefoundation/zope.sqlalchemy/blob/master/src/zope/sqlalchemy/README.rst

The basic invocation is:

>>> from zope.sqlalchemy import mark_changed
>>> mark_changed(session)

Other use-cases are described in the docs as well.

silenius commented 3 years ago

Thanks, setting synchronize_session to another value doesn't change anything, but with mark_changed it works as expected. If this is acceptable (having to call such "low level" function to do an UPDATE) for most people then we can close the issue, otherwise listening to do_orm_execute as in #68 works too

jvanasco commented 3 years ago

Well, I've always done it with mark_changed. This would be more convenient. I'm not sure if this would cause any unwanted situations. Personally, I would prefer to not have this behavior in a few high-load projects because they only invoke a commit if there are affected rows; but I would definitely benefit from this in others... so I'm the worst person to opine on your PR!

I brought the PR up with the SQLAlchemy development team to see if there are any red flags to your approach. I don't think so, but Mike, Federico or Gord might see something that I missed. The weekly virtual meeting is Thursday mornings @ 10am EST, so I'll either get you feedback before then or during the meeting.

jvanasco commented 3 years ago

no red flags to sqlalchemy devs.

silenius commented 3 years ago

fixed in b65543f