From 5ba31b5a8768e1ca1a08a82144ec0e726d8357c1 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Fri, 9 Apr 2021 11:09:16 -0400 Subject: [PATCH] Dont return outer transaction for _subtrans flag Fixed critical regression where the :class:`_orm.Session` could fail to "autobegin" a new transaction when a flush occurred without an existing transaction in place, implicitly placing the :class:`_orm.Session` into legacy autocommit mode which commit the transaction. The :class:`_orm.Session` now has a check that will prevent this condition from occurring, in addition to repairing the flush issue. Additionally, scaled back part of the change made as part of :ticket:`5226` which can run autoflush during an unexpire operation, to not actually do this in the case of a :class:`_orm.Session` using legacy :paramref:`_orm.Session.autocommit` mode, as this incurs a commit within a refresh operation. Fixes: #6233 Change-Id: Ia980e62a090e39e3e2a7fb77c95832ae784cc9a5 --- doc/build/changelog/unreleased_14/6233.rst | 16 ++++++ lib/sqlalchemy/orm/loading.py | 4 +- lib/sqlalchemy/orm/session.py | 14 ++++-- test/orm/test_session.py | 58 ++++++++++++++++++++++ 4 files changed, 88 insertions(+), 4 deletions(-) create mode 100644 doc/build/changelog/unreleased_14/6233.rst diff --git a/doc/build/changelog/unreleased_14/6233.rst b/doc/build/changelog/unreleased_14/6233.rst new file mode 100644 index 0000000000..80552e9b32 --- /dev/null +++ b/doc/build/changelog/unreleased_14/6233.rst @@ -0,0 +1,16 @@ +.. change:: + :tags: bug, regression, orm + :tickets: 6233 + + Fixed critical regression where the :class:`_orm.Session` could fail to + "autobegin" a new transaction when a flush occurred without an existing + transaction in place, implicitly placing the :class:`_orm.Session` into + legacy autocommit mode which commit the transaction. The + :class:`_orm.Session` now has a check that will prevent this condition from + occurring, in addition to repairing the flush issue. + + Additionally, scaled back part of the change made as part of :ticket:`5226` + which can run autoflush during an unexpire operation, to not actually + do this in the case of a :class:`_orm.Session` using legacy + :paramref:`_orm.Session.autocommit` mode, as this incurs a commit within + a refresh operation. \ No newline at end of file diff --git a/lib/sqlalchemy/orm/loading.py b/lib/sqlalchemy/orm/loading.py index 0dc92daf5b..12acfc5b78 100644 --- a/lib/sqlalchemy/orm/loading.py +++ b/lib/sqlalchemy/orm/loading.py @@ -1343,7 +1343,9 @@ def load_scalar_attributes(mapper, state, attribute_names, passive): result = False - no_autoflush = bool(passive & attributes.NO_AUTOFLUSH) + no_autoflush = ( + bool(passive & attributes.NO_AUTOFLUSH) or state.session.autocommit + ) # in the case of inheritance, particularly concrete and abstract # concrete inheritance, the class manager might have some keys diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py index 0562569bff..d08abb7880 100644 --- a/lib/sqlalchemy/orm/session.py +++ b/lib/sqlalchemy/orm/session.py @@ -1140,8 +1140,7 @@ class Session(_SessionClassMethods): if autocommit: if future: raise sa_exc.ArgumentError( - "Cannot use autocommit mode with future=True. " - "use the autobegin flag." + "Cannot use autocommit mode with future=True." ) self.autocommit = True else: @@ -1308,7 +1307,7 @@ class Session(_SessionClassMethods): "Session objects." ) if self._autobegin(): - if not subtransactions and not nested: + if not subtransactions and not nested and not _subtrans: return self._transaction if self._transaction is not None: @@ -1321,9 +1320,18 @@ class Session(_SessionClassMethods): raise sa_exc.InvalidRequestError( "A transaction is already begun on this Session." ) + elif not self.autocommit: + # outermost transaction. must be a not nested and not + # a subtransaction + assert not nested and not _subtrans and not subtransactions + trans = SessionTransaction(self) + assert self._transaction is trans else: + # legacy autocommit mode + assert not self.future trans = SessionTransaction(self, nested=nested) assert self._transaction is trans + return self._transaction # needed for __enter__/__exit__ hook def begin_nested(self): diff --git a/test/orm/test_session.py b/test/orm/test_session.py index 280bb99b8b..89d61c3ea1 100644 --- a/test/orm/test_session.py +++ b/test/orm/test_session.py @@ -151,6 +151,64 @@ class TransScopingTest(_fixtures.FixtureTest): s.commit() is_(s._transaction, None) + def test_autobegin_within_flush(self): + """test :ticket:`6233`""" + + s = Session(testing.db) + + User, users = self.classes.User, self.tables.users + + mapper(User, users) + s.add(User(name="u1")) + s.commit() + + u1 = s.query(User).first() + + s.commit() + + u1.name = "newname" + + s.flush() + + eq_(s.connection().scalar(select(User.name)), "newname") + assert s.in_transaction() + s.rollback() + assert not s.in_transaction() + eq_(s.connection().scalar(select(User.name)), "u1") + + def test_no_autoflush_or_commit_in_expire_w_autocommit(self): + """test second part of :ticket:`6233`. + + Here we test that the "autoflush on unexpire" feature added + in :ticket:`5226` is turned off for a legacy autocommit session. + + """ + + s = Session( + testing.db, autocommit=True, expire_on_commit=True, autoflush=True + ) + + User, users = self.classes.User, self.tables.users + + mapper(User, users) + + u1 = User(name="u1") + s.add(u1) + s.flush() # this commits + + u1.name = "u2" # this does not commit + + assert "id" not in u1.__dict__ + u1.id # this unexpires + + # never expired + eq_(u1.__dict__["name"], "u2") + + eq_(u1.name, "u2") + + # still in dirty collection + assert u1 in s.dirty + def test_autobegin_begin_method(self): s = Session(testing.db) -- 2.47.2