From: Mike Bayer Date: Thu, 27 Oct 2016 13:51:50 +0000 (-0400) Subject: Restore object to the identity_map upon delete() unconditionally X-Git-Tag: rel_1_1_3~1^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e56a9d85acd165cfea08f9dc7e9d747054ce6fdb;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Restore object to the identity_map upon delete() unconditionally Fixed regression caused by :ticket:`2677` whereby calling :meth:`.Session.delete` on an object that was already flushed as deleted in that session would fail to set up the object in the identity map (or reject the object), causing flush errors as the object were in a state not accommodated by the unit of work. The pre-1.1 behavior in this case has been restored, which is that the object is put back into the identity map so that the DELETE statement will be attempted again, which emits a warning that the number of expected rows was not matched (unless the row were restored outside of the session). Change-Id: I9a8871f82cb1ebe67a7ad54d888d5ee835a9a40a Fixes: #3839 --- diff --git a/doc/build/changelog/changelog_11.rst b/doc/build/changelog/changelog_11.rst index 70db53d2f1..dd8b1b45ec 100644 --- a/doc/build/changelog/changelog_11.rst +++ b/doc/build/changelog/changelog_11.rst @@ -21,6 +21,21 @@ .. changelog:: :version: 1.1.3 + .. change:: + :tags: bug, orm + :tickets: 3839 + + Fixed regression caused by :ticket:`2677` whereby calling + :meth:`.Session.delete` on an object that was already flushed as + deleted in that session would fail to set up the object in the + identity map (or reject the object), causing flush errors as the + object were in a state not accommodated by the unit of work. + The pre-1.1 behavior in this case has been restored, which is that + the object is put back into the identity map so that the DELETE + statement will be attempted again, which emits a warning that the number + of expected rows was not matched (unless the row were restored outside + of the session). + .. change:: :tags: bug, postgresql :tickets: 3835 diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py index b492cbb7f3..b39ba1465a 100644 --- a/lib/sqlalchemy/orm/session.py +++ b/lib/sqlalchemy/orm/session.py @@ -1726,8 +1726,9 @@ class Session(_SessionClassMethods): if state in self._deleted: return + self.identity_map.add(state) + if to_attach: - self.identity_map.add(state) self._after_attach(state, obj) if head: @@ -2196,7 +2197,8 @@ class Session(_SessionClassMethods): for state in proc: is_orphan = ( _state_mapper(state)._is_orphan(state) and state.has_identity) - flush_context.register_object(state, isdelete=is_orphan) + _reg = flush_context.register_object(state, isdelete=is_orphan) + assert _reg, "Failed to add object to the flush context!" processed.add(state) # put all remaining deletes into the flush context. @@ -2205,7 +2207,8 @@ class Session(_SessionClassMethods): else: proc = deleted.difference(processed) for state in proc: - flush_context.register_object(state, isdelete=True) + _reg = flush_context.register_object(state, isdelete=True) + assert _reg, "Failed to add object to the flush context!" if not flush_context.has_work: return diff --git a/lib/sqlalchemy/orm/unitofwork.py b/lib/sqlalchemy/orm/unitofwork.py index f3e39d9b5a..de4842d6fa 100644 --- a/lib/sqlalchemy/orm/unitofwork.py +++ b/lib/sqlalchemy/orm/unitofwork.py @@ -242,6 +242,9 @@ class UOWTransaction(object): listonly=False, cancel_delete=False, operation=None, prop=None): if not self.session._contains_state(state): + # this condition is normal when objects are registered + # as part of a relationship cascade operation. it should + # not occur for the top-level register from Session.flush(). if not state.deleted and operation is not None: util.warn("Object of type %s not in session, %s operation " "along '%s' will not proceed" % diff --git a/test/orm/test_session.py b/test/orm/test_session.py index caeb08530b..24666d084c 100644 --- a/test/orm/test_session.py +++ b/test/orm/test_session.py @@ -1,5 +1,5 @@ from sqlalchemy.testing import eq_, assert_raises, \ - assert_raises_message + assert_raises_message, assertions from sqlalchemy.testing.util import gc_collect from sqlalchemy.testing import pickleable from sqlalchemy.util import pickle @@ -347,6 +347,37 @@ class SessionStateTest(_fixtures.FixtureTest): eq_(sess.query(User).count(), 1) + def test_deleted_adds_to_imap_unconditionally(self): + users, User = self.tables.users, self.classes.User + + mapper(User, users) + + sess = Session() + u1 = User(name='u1') + sess.add(u1) + sess.commit() + + sess.delete(u1) + sess.flush() + + # object is not in session + assert u1 not in sess + + # but it *is* attached + assert u1._sa_instance_state.session_id == sess.hash_key + + # mark as deleted again + sess.delete(u1) + + # in the session again + assert u1 in sess + + # commit proceeds w/ warning + with assertions.expect_warnings( + "DELETE statement on table 'users' " + r"expected to delete 1 row\(s\); 0 were matched."): + sess.commit() + def test_autoflush_expressions(self): """test that an expression which is dependent on object state is evaluated after the session autoflushes. This is the lambda diff --git a/test/orm/test_unitofworkv2.py b/test/orm/test_unitofworkv2.py index ae8454f6f3..b2fefe6167 100644 --- a/test/orm/test_unitofworkv2.py +++ b/test/orm/test_unitofworkv2.py @@ -1566,6 +1566,26 @@ class BasicStaleChecksTest(fixtures.MappedTest): sess.flush ) + @testing.requires.sane_multi_rowcount + def test_delete_twice(self): + Parent, Child = self._fixture() + sess = Session() + p1 = Parent(id=1, data=2, child=None) + sess.add(p1) + sess.commit() + + sess.delete(p1) + sess.flush() + + sess.delete(p1) + + assert_raises_message( + exc.SAWarning, + "DELETE statement on table 'parent' expected to " + "delete 1 row\(s\); 0 were matched.", + sess.commit + ) + @testing.requires.sane_multi_rowcount def test_delete_multi_missing_warning(self): Parent, Child = self._fixture()