Don't run postfetch_post_update for a DELETE

Fixed 1.2 regression caused by :ticket:`3472` where the handling of an
"updated_at" style column within the context of a post-update operation
would also occur for a row that is to be deleted following the update,
meaning both that a column with a Python-side value generator would show
the now-deleted value that was emitted for the UPDATE before the DELETE
(which was not the previous behavor), as well as that a SQL- emitted value
generator would have the attribute expired, meaning the previous value
would be unreachable due to the row having been deleted and the object
detached from the session.The "postfetch" logic that was added as part of
:ticket:`3472` is now skipped entirely for an object that ultimately is to
be deleted.

Fixes: #4327
Change-Id: Ieac845348979df296bcf7e785c0353bdc6074220

diff --git a/doc/build/changelog/unreleased_12/4327.rst b/doc/build/changelog/unreleased_12/4327.rst
new file mode 100644 (file)
index 0000000..2a10c55
--- /dev/null
+++ b/doc/build/changelog/unreleased_12/4327.rst
@@ -0,0 +1,15 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 4327
+
+    Fixed 1.2 regression caused by :ticket:`3472` where the handling of an
+    "updated_at" style column within the context of a post-update operation
+    would also occur for a row that is to be deleted following the update,
+    meaning both that a column with a Python-side value generator would show
+    the now-deleted value that was emitted for the UPDATE before the DELETE
+    (which was not the previous behavor), as well as that a SQL- emitted value
+    generator would have the attribute expired, meaning the previous value
+    would be unreachable due to the row having been deleted and the object
+    detached from the session.The "postfetch" logic that was added as part of
+    :ticket:`3472` is now skipped entirely for an object that ultimately is to
+    be deleted.

diff --git a/lib/sqlalchemy/orm/persistence.py b/lib/sqlalchemy/orm/persistence.py
index a48bf9bf7ee31c617f9080721454e52e45b8bce1..95e26d83c174e506a628b5f36cd9dadbb8e95f53 100644 (file)
--- a/lib/sqlalchemy/orm/persistence.py
+++ b/lib/sqlalchemy/orm/persistence.py
@@ -1136,6 +1136,9 @@ def _finalize_insert_update_commands(base_mapper, uowtransaction, states):
 
 def _postfetch_post_update(mapper, uowtransaction, table,
                            state, dict_, result, params):
+    if uowtransaction.is_deleted(state):
+        return
+
     prefetch_cols = result.context.compiled.prefetch
     postfetch_cols = result.context.compiled.postfetch
 

diff --git a/test/orm/test_cycles.py b/test/orm/test_cycles.py
index 7528d9696f18ecd4c8e2bd8066c37ba500021b0a..6e7702ad84502df713664cffd2424e316ead55f5 100644 (file)
--- a/test/orm/test_cycles.py
+++ b/test/orm/test_cycles.py
@@ -16,6 +16,7 @@ from sqlalchemy.testing import eq_, is_
 from sqlalchemy.testing.assertsql import RegexSQL, CompiledSQL, AllOf
 from sqlalchemy.testing import fixtures
 from itertools import count
+from sqlalchemy import select
 
 
 class SelfReferentialTest(fixtures.MappedTest):
@@ -1330,9 +1331,9 @@ class PostUpdateBatchingTest(fixtures.MappedTest):
         )
 
 
-class PostUpdateOnUpdateTest(fixtures.DeclarativeMappedTest):
+from sqlalchemy import bindparam
 
-    counter = count()
+class PostUpdateOnUpdateTest(fixtures.DeclarativeMappedTest):
 
     @classmethod
     def setup_classes(cls):
@@ -1341,11 +1342,19 @@ class PostUpdateOnUpdateTest(fixtures.DeclarativeMappedTest):
         class A(Base):
             __tablename__ = 'a'
             id = Column(Integer, primary_key=True)
+            data = Column(Integer)
             favorite_b_id = Column(ForeignKey('b.id', name="favorite_b_fk"))
             bs = relationship("B", primaryjoin="A.id == B.a_id")
             favorite_b = relationship(
                 "B", primaryjoin="A.favorite_b_id == B.id", post_update=True)
             updated = Column(Integer, onupdate=lambda: next(cls.counter))
+            updated_db = Column(
+                Integer,
+                onupdate=bindparam(
+                    key='foo',
+                    callable_=lambda: next(cls.db_counter)
+                )
+            )
 
         class B(Base):
             __tablename__ = 'b'
@@ -1355,6 +1364,7 @@ class PostUpdateOnUpdateTest(fixtures.DeclarativeMappedTest):
     def setup(self):
         super(PostUpdateOnUpdateTest, self).setup()
         PostUpdateOnUpdateTest.counter = count()
+        PostUpdateOnUpdateTest.db_counter = count()
 
     def test_update_defaults(self):
         A, B = self.classes("A", "B")
@@ -1369,12 +1379,14 @@ class PostUpdateOnUpdateTest(fixtures.DeclarativeMappedTest):
         s.flush()
 
         eq_(a1.updated, 0)
+        eq_(a1.updated_db, 0)
 
     def test_update_defaults_refresh_flush_event(self):
         A, B = self.classes("A", "B")
 
         canary = mock.Mock()
-        event.listen(A, "refresh_flush", canary)
+        event.listen(A, "refresh_flush", canary.refresh_flush)
+        event.listen(A, "expire", canary.expire)
 
         s = Session()
         a1 = A()
@@ -1386,10 +1398,157 @@ class PostUpdateOnUpdateTest(fixtures.DeclarativeMappedTest):
         s.flush()
 
         eq_(a1.updated, 0)
+        eq_(a1.updated_db, 0)
+        eq_(
+            canary.mock_calls,
+            [
+                mock.call.refresh_flush(a1, mock.ANY, ['updated']),
+                mock.call.expire(a1, ['updated_db']),
+            ]
+        )
+
+    def test_update_defaults_refresh_flush_event_no_postupdate(self):
+        # run the same test as test_update_defaults_refresh_flush_event
+        # but don't actually use any postupdate functionality
+        A, = self.classes("A")
+
+        canary = mock.Mock()
+        event.listen(A, "refresh_flush", canary.refresh_flush)
+        event.listen(A, "expire", canary.expire)
+
+        s = Session()
+        a1 = A()
+
+        s.add(a1)
+        s.flush()
+
+        eq_(a1.updated, None)
+        eq_(a1.updated_db, None)
+
+        # now run a normal UPDATE
+        a1.data = 5
+        s.flush()
+
+        # now they are updated
+        eq_(a1.updated, 0)
+        eq_(a1.updated_db, 0)
+        eq_(
+            canary.mock_calls,
+            [
+                mock.call.refresh_flush(a1, mock.ANY, ['updated']),
+                mock.call.expire(a1, ['updated_db']),
+            ]
+        )
+
+    def test_update_defaults_dont_expire_on_delete(self):
+        A, B = self.classes("A", "B")
+
+        canary = mock.Mock()
+        event.listen(A, "refresh_flush", canary.refresh_flush)
+        event.listen(A, "expire", canary.expire)
+
+        s = Session()
+        a1 = A()
+        b1 = B()
+
+        a1.bs.append(b1)
+        a1.favorite_b = b1
+        s.add(a1)
+        s.flush()
+
+        eq_(
+            canary.mock_calls,
+            [
+                mock.call.refresh_flush(a1, mock.ANY, ['updated']),
+                mock.call.expire(a1, ['updated_db']),
+            ]
+        )
+
+        # ensure that we load this value here, we want to see that it
+        # stays the same and isn't expired below.
+        eq_(a1.updated, 0)
+        eq_(a1.updated_db, 0)
+
+        s.delete(a1)
+        s.flush()
+
+        assert a1 not in s
+
+        # both the python-side default and the server side default
+        # *did* get bumped for the UPDATE, however the row was then
+        # deleted, show what the values were *before* the UPDATE.
+        eq_(a1.updated, 0)
+        eq_(a1.updated_db, 0)
+        eq_(
+            canary.mock_calls,
+            [
+                # previous flush
+                mock.call.refresh_flush(a1, mock.ANY, ['updated']),
+                mock.call.expire(a1, ['updated_db']),
+
+                # nothing happened
+            ]
+        )
+
+        eq_(next(self.counter), 2)
+
+    def test_update_defaults_dont_expire_on_delete_no_postupdate(self):
+        # run the same test as
+        # test_update_defaults_dont_expire_on_delete_no_postupdate
+        # but don't actually use any postupdate functionality
+        A, = self.classes("A")
+
+        canary = mock.Mock()
+        event.listen(A, "refresh_flush", canary.refresh_flush)
+        event.listen(A, "expire", canary.expire)
+
+        s = Session()
+        a1 = A()
+
+        s.add(a1)
+        s.flush()
+
+        eq_(a1.updated, None)
+        eq_(a1.updated_db, None)
+
+        a1.data = 5
+        s.flush()
+
+        eq_(a1.updated, 0)
+        eq_(a1.updated_db, 0)
+
+        eq_(
+            canary.mock_calls,
+            [
+                mock.call.refresh_flush(a1, mock.ANY, ['updated']),
+                mock.call.expire(a1, ['updated_db']),
+            ]
+        )
+
+        # ensure that we load this value here, we want to see that it
+        # stays the same and isn't expired below.
+        eq_(a1.updated, 0)
+        eq_(a1.updated_db, 0)
+
+        s.delete(a1)
+        s.flush()
+
+        assert a1 not in s
+
+        # no UPDATE was emitted here, so they stay at zero.  but the
+        # server-side default wasn't expired either even though the row
+        # was deleted.
+        eq_(a1.updated, 0)
+        eq_(a1.updated_db, 0)
         eq_(
             canary.mock_calls,
             [
-                mock.call(a1, mock.ANY, ['updated'])
+                # previous flush
+                mock.call.refresh_flush(a1, mock.ANY, ['updated']),
+                mock.call.expire(a1, ['updated_db']),
+
+
+                # nothing called for this flush
             ]
         )
 
@@ -1403,8 +1562,13 @@ class PostUpdateOnUpdateTest(fixtures.DeclarativeMappedTest):
         a1.bs.append(b1)
         a1.favorite_b = b1
         a1.updated = 5
+        a1.updated_db = 7
         s.add(a1)
         s.flush()
 
+        # doesn't require DB access
+        s.expunge(a1)
+
         eq_(a1.updated, 5)
+        eq_(a1.updated_db, 7)