From: Mike Bayer Date: Thu, 23 Aug 2018 15:55:13 +0000 (-0400) Subject: Don't run postfetch_post_update for a DELETE X-Git-Tag: rel_1_3_0b1~96 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=1b5393db36a6c4353d41c7065b29a377d7c3b9b2;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git 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 index 0000000000..2a10c55c83 --- /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 a48bf9bf7e..95e26d83c1 100644 --- 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 7528d9696f..6e7702ad84 100644 --- 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)