keep_session=True breaks on SQLAlchemy 1.4

[jvanasco]

BUG/PROBLEM REPORT (OR OTHER COMMON ISSUE)

Under SQLAlchemy 1.4, keep_session=True is broken.

A SQLAlchemy Session is registered to transaction via zope.sqlalchemy, with keep_session set to True.

Editing an object in the session and calling .flush will cause SQLAlchemy to invoke a commit on the actual session - not the unit of work - and trigger the before_commit hook in zope.sqlalchemy, which will then fail.

Relavent LOC from stack trace:

• https://github.com/sqlalchemy/sqlalchemy/blob/master/lib/sqlalchemy/orm/session.py#L3360
• https://github.com/zopefoundation/zope.sqlalchemy/blob/master/src/zope/sqlalchemy/datamanager.py#L300-L304

What I did:

reproducible test case below

What I expect to happen:

The session should flush as in SQLAlchemy 1.3, not commit.

What actually happened:

SQLAlchemy invoked a commit on the transaction.

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

• Python3.8
• zope.sqlalchemy==1.3
• sqlalchemy==1.4

from zope.sqlalchemy import register
import transaction

from sqlalchemy import (
    create_engine,
    Column,
    Integer,
    String,
)
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy.orm import relationship, sessionmaker

engine = create_engine("sqlite:///:memory:")
Base = declarative_base()

class User(Base):
    __tablename__ = "user"
    id = Column(Integer, primary_key=True)
    user_name = Column(String(50))

    def __repr__(self):
        return f"<User(id={self.id}, user_name='{self.user_name}')>"

Base.metadata.create_all(engine)

Session = sessionmaker(bind=engine)

# this breaks in 1.4
# session/transaction setup
session = Session()
register(session, keep_session=True)

# add a user
user = User(user_name="User")
session.add(user)
session.flush(objects=[user])
transaction.commit()
transaction.begin()

# edit them
user.user_name = "User2"

# this raises an AssertionError under SQLAlchemy 1.4
session.flush(objects=[user])

[jvanasco]

I had some time to followup, and while I do not have a solution, I believe this issue is tied to an implementation detail in the test suite.

In the KeepSession test (https://github.com/zopefoundation/zope.sqlalchemy/blob/master/src/zope/sqlalchemy/tests.py#L656-L676), the logic does the following:

1. Within a first transaction, add a user
2. Outside any transaction, query the user
3. Within a second transaction, edit the user
4. Outside any transaction, check the object

The problem in the above - the query in Step2 avoids the bug and reloads the user into the Session's identity map. As-is, the test is only checking if the session is still around on Step 4.

I suggest the following change in the test, or added as an alternate test:

1. Outside any transaction, create a user
2. Within a first transaction, add the user
3. Within a second transaction, edit the user
4. Outside any transaction, check the object

The edited code is as follows (and hereby freely contributed under the project's license):

    def testKeepSessionAlt(self):
        session = KeepSession()

        try:
            user = User(id=1, firstname="foo", lastname="bar")

            # if the keep_session works correctly, this transaction will not close
            # the session after commit
            with transaction.manager:
                session.add(user)
                session.flush(objects=[user,])

            # if the keep_session works correctly, this transaction will not close
            # the session after commit
            with transaction.manager:
                user.firstname = "super"
                session.flush()

            # make sure the session is still attached to user
            self.assertEqual(user.firstname, "super")

        finally:
            # KeepSession does not rollback on transaction abort
            session.rollback()

[sallner]

As with 1.4.7 this problem has been solved in SQLAlchemy directly. Do you still see a need for the testcase to be updated, @jvanasco? Also see https://github.com/sqlalchemy/sqlalchemy/issues/6233 for reference.

[icemac]

See https://github.com/zopefoundation/zope.sqlalchemy/commit/8a2cb11846e330b5bedc1023027475a4b7bc5030 where using SQLAlchemy < 1.4.7 gets prohibited.

[jvanasco]

@sallner I'm checking with the rest of the sqlalchemy dev group to see if they feel that ticket solved it, or if this working is more of an edge case.

i do think the suggested testcase edits offer better coverage and should be adopted.

[jvanasco]

ok, the lead says it's possibly the same regression

[sallner]

So do we need to implement the testcase or can we close the issue? @jvanasco

[jvanasco]

I think this is safe to close. I think it's safest to add this test to the suite and guard against future issues, but this has not been an issue the past few releases of either project.

[icemac]

@jvanasco Thank you for your explanation, so closing this issue now.