From: Mike Bayer Date: Sun, 8 May 2016 06:21:57 +0000 (-0400) Subject: Use the "committed" values when extracting many-to-one lazyload value X-Git-Tag: rel_1_1_0b1~13 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c99fc44e170be61696206872701ff75e4c8a3711;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Use the "committed" values when extracting many-to-one lazyload value The scalar object set() method calls upon the lazy loader to get at the "old" value of the attriute, however doesn't ensure that the "committed" value of the foreign key attributes is used. If the user has manipulated these attributes and they themselves have pending, non committed changes, we get the "new" value which these attributes would have set up if they were flushed. "old" vs "new" value is always about how the value has changed since the load, so we always have to use the DB-persisted values for everything when looking for "old". Change-Id: I82bdc40ad0cf033c3a98f3361776cf3161542cd6 Fixes: #3708 --- diff --git a/doc/build/changelog/changelog_11.rst b/doc/build/changelog/changelog_11.rst index b57336c4af..e1c8a654d7 100644 --- a/doc/build/changelog/changelog_11.rst +++ b/doc/build/changelog/changelog_11.rst @@ -21,6 +21,24 @@ .. changelog:: :version: 1.1.0b1 + .. change:: + :tags: bug, orm + :tickets: 3708 + + Fixed an issue where a many-to-one change of an object from one + parent to another could work inconsistently when combined with + an un-flushed modication of the foreign key attribute. The attribute + move now considers the database-committed value of the foreign key + in order to locate the "previous" parent of the object being + moved. This allows events to fire off correctly including + backref events. Previously, these events would not always fire. + Applications which may have relied on the previously broken + behavior may be affected. + + .. seealso:: + + :ref:`change_3708` + .. change:: :tags: feature, sql :tickets: 3049 diff --git a/doc/build/changelog/migration_11.rst b/doc/build/changelog/migration_11.rst index 8b8075a29c..c60b17af5d 100644 --- a/doc/build/changelog/migration_11.rst +++ b/doc/build/changelog/migration_11.rst @@ -678,6 +678,54 @@ would have to be compared during the merge. :ticket:`3601` +.. _change_3708: + +Fix involving many-to-one object moves with user-initiated foriegn key manipulations +------------------------------------------------------------------------------------ + +A bug has been fixed involving the mechanics of replacing a many-to-one +reference to an object with another object. During the attribute operation, +the location of the object tha was previouly referred to now makes use of the +database-committed foreign key value, rather than the current foreign key +value. The main effect of the fix is that a backref event towards a collection +will fire off more accurately when a many-to-one change is made, even if the +foreign key attribute was manually moved to the new value beforehand. Assume a +mapping of the classes ``Parent`` and ``SomeClass``, where ``SomeClass.parent`` +refers to ``Parent`` and ``Parent.items`` refers to the collection of +``SomeClass`` objects:: + + some_object = SomeClass() + session.add(some_object) + some_object.parent_id = some_parent.id + some_object.parent = some_parent + +Above, we've made a pending object ``some_object``, manipulated its foreign key +towards ``Parent`` to refer to it, *then* we actually set up the relationship. +Before the bug fix, the backref would not have fired off:: + + # before the fix + assert some_object not in some_parent.items + +The fix now is that when we seek to locate the previous value of +``some_object.parent``, we disregard the parent id that's been manually set, +and we look for the database-committed value. In this case, it's None because +the object is pending, so the event system logs ``some_object.parent`` +as a net change:: + + # after the fix, backref fired off for some_object.parent = some_parent + assert some_object in some_parent.items + +While it is discouraged to manipulate foreign key attributes that are managed +by relationships, there is limited support for this use case. Applications +that manipulate foreign keys in order to allow loads to proceed will often make +use of the :meth:`.Session.enable_relationship_loading` and +:attr:`.RelationshipProperty.load_on_pending` features, which cause +relationships to emit lazy loads based on in-memory foreign key values that +aren't persisted. Whether or not these features are in use, this behavioral +improvement will now be apparent. + +:ticket:`3708` + .. _change_3662: Improvements to the Query.correlate method with polymoprhic entities diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py index 7239d41f29..e01c135871 100644 --- a/lib/sqlalchemy/orm/attributes.py +++ b/lib/sqlalchemy/orm/attributes.py @@ -788,9 +788,13 @@ class ScalarObjectAttributeImpl(ScalarAttributeImpl): """ if self.dispatch._active_history: old = self.get( - state, dict_, passive=PASSIVE_ONLY_PERSISTENT | NO_AUTOFLUSH) + state, dict_, + passive=PASSIVE_ONLY_PERSISTENT | + NO_AUTOFLUSH | LOAD_AGAINST_COMMITTED) else: - old = self.get(state, dict_, passive=PASSIVE_NO_FETCH ^ INIT_OK) + old = self.get( + state, dict_, passive=PASSIVE_NO_FETCH ^ INIT_OK | + LOAD_AGAINST_COMMITTED) if check_old is not None and \ old is not PASSIVE_NO_RESULT and \ diff --git a/test/orm/test_load_on_fks.py b/test/orm/test_load_on_fks.py index 471c8665a1..efb709ff2f 100644 --- a/test/orm/test_load_on_fks.py +++ b/test/orm/test_load_on_fks.py @@ -97,7 +97,7 @@ class LoadOnFKsTest(AssertsExecutionResults, fixtures.TestBase): sess.rollback() Base.metadata.drop_all(engine) - def test_load_on_pending_disallows_backref_event(self): + def test_load_on_pending_allows_backref_event(self): Child.parent.property.load_on_pending = True sess.autoflush = False c3 = Child() @@ -105,23 +105,30 @@ class LoadOnFKsTest(AssertsExecutionResults, fixtures.TestBase): c3.parent_id = p1.id c3.parent = p1 - # a side effect of load-on-pending with no autoflush. - # a change to the backref event handler to check - # collection membership before assuming "old == new so return" - # would fix this - but this is wasteful and autoflush - # should be turned on. - assert c3 not in p1.children + # backref fired off when c3.parent was set, + # because the "old" value was None. + # change as of [ticket:3708] + assert c3 in p1.children - def test_enable_rel_loading_disallows_backref_event(self): + def test_enable_rel_loading_allows_backref_event(self): sess.autoflush = False c3 = Child() sess.enable_relationship_loading(c3) c3.parent_id = p1.id c3.parent = p1 - # c3.parent is already acting like a "load" here, - # so backref events don't work - assert c3 not in p1.children + # backref fired off when c3.parent was set, + # because the "old" value was None + # change as of [ticket:3708] + assert c3 in p1.children + + def test_m2o_history_on_persistent_allows_backref_event(self): + c3 = Child() + sess.add(c3) + c3.parent_id = p1.id + c3.parent = p1 + + assert c3 in p1.children def test_load_on_persistent_allows_backref_event(self): Child.parent.property.load_on_pending = True @@ -132,15 +139,16 @@ class LoadOnFKsTest(AssertsExecutionResults, fixtures.TestBase): assert c3 in p1.children - def test_enable_rel_loading_on_persistent_disallows_backref_event(self): + def test_enable_rel_loading_on_persistent_allows_backref_event(self): c3 = Child() sess.enable_relationship_loading(c3) c3.parent_id = p1.id c3.parent = p1 - # c3.parent is already acting like a "load" here, - # so backref events don't work - assert c3 not in p1.children + # backref fired off when c3.parent was set, + # because the "old" value was None + # change as of [ticket:3708] + assert c3 in p1.children def test_no_load_on_pending_allows_backref_event(self): # users who stick with the program and don't use