diff options
| -rw-r--r-- | doc/build/changelog/changelog_11.rst | 18 | ||||
| -rw-r--r-- | doc/build/changelog/migration_11.rst | 48 | ||||
| -rw-r--r-- | lib/sqlalchemy/orm/attributes.py | 8 | ||||
| -rw-r--r-- | test/orm/test_load_on_fks.py | 38 |
4 files changed, 95 insertions, 17 deletions
diff --git a/doc/build/changelog/changelog_11.rst b/doc/build/changelog/changelog_11.rst index b57336c4a..e1c8a654d 100644 --- a/doc/build/changelog/changelog_11.rst +++ b/doc/build/changelog/changelog_11.rst @@ -22,6 +22,24 @@ :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 8b8075a29..c60b17af5 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 7239d41f2..e01c13587 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 471c8665a..efb709ff2 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 |