diff options
| -rw-r--r-- | doc/build/changelog/changelog_09.rst | 25 | ||||
| -rw-r--r-- | doc/build/changelog/migration_09.rst | 73 | ||||
| -rw-r--r-- | doc/build/orm/internals.rst | 4 | ||||
| -rw-r--r-- | lib/sqlalchemy/orm/attributes.py | 135 | ||||
| -rw-r--r-- | lib/sqlalchemy/orm/dynamic.py | 7 | ||||
| -rw-r--r-- | lib/sqlalchemy/orm/events.py | 33 | ||||
| -rw-r--r-- | test/orm/test_attributes.py | 8 | ||||
| -rw-r--r-- | test/orm/test_backref_mutations.py | 11 |
8 files changed, 231 insertions, 65 deletions
diff --git a/doc/build/changelog/changelog_09.rst b/doc/build/changelog/changelog_09.rst index ba642ba26..6ba1642bd 100644 --- a/doc/build/changelog/changelog_09.rst +++ b/doc/build/changelog/changelog_09.rst @@ -89,6 +89,31 @@ phase should only be once per dialect. Also in 0.8.3. .. change:: + :tags: feature, orm + :tickets: 2789 + + The mechanism by which attribute events pass along an + :class:`.AttributeImpl` as an "initiator" token has been changed; + the object is now an event-specific object called :class:`.attributes.Event`. + Additionally, the attribute system no longer halts events based + on a matching "initiator" token; this logic has been moved to be + specific to ORM backref event handlers, which are the typical source + of the re-propagation of an attribute event onto subsequent append/set/remove + operations. End user code which emulates the behavior of backrefs + must now ensure that recursive event propagation schemes are halted, + if the scheme does not use the backref handlers. Using this new system, + backref handlers can now peform a + "two-hop" operation when an object is appended to a collection, + associated with a new many-to-one, de-associated with the previous + many-to-one, and then removed from a previous collection. Before this + change, the last step of removal from the previous collection would + not occur. + + .. seealso:: + + :ref:`migration_2789` + + .. change:: :tags: feature, sql :tickets: 722 diff --git a/doc/build/changelog/migration_09.rst b/doc/build/changelog/migration_09.rst index 424802c3d..6f4263070 100644 --- a/doc/build/changelog/migration_09.rst +++ b/doc/build/changelog/migration_09.rst @@ -124,6 +124,78 @@ to 0.9 without issue. :ticket:`2736` +.. _migration_2789: + +Backref handlers can now propagate more than one level deep +----------------------------------------------------------- + +The mechanism by which attribute events pass along their "initiator", that is +the object associated with the start of the event, has been changed; instead +of a :class:`.AttributeImpl` being passed, a new object :class:`.attributes.Event` +is passed instead; this object refers to the :class:`.AttributeImpl` as well as +to an "operation token", representing if the operation is an append, remove, +or replace operation. + +The attribute event system no longer looks at this "initiator" object in order to halt a +recursive series of attribute events. Instead, the system of preventing endless +recursion due to mutually-dependent backref handlers has been moved +to the ORM backref event handlers specifically, which now take over the role +of ensuring that a chain of mutually-dependent events (such as append to collection +A.bs, set many-to-one attribute B.a in response) doesn't go into an endless recursion +stream. The rationale here is that the backref system, given more detail and control +over event propagation, can finally allow operations more than one level deep +to occur; the typical scenario is when a collection append results in a many-to-one +replacement operation, which in turn should cause the item to be removed from a +previous collection:: + + class Parent(Base): + __tablename__ = 'parent' + + id = Column(Integer, primary_key=True) + children = relationship("Child", backref="parent") + + class Child(Base): + __tablename__ = 'child' + + id = Column(Integer, primary_key=True) + parent_id = Column(ForeignKey('parent.id')) + + p1 = Parent() + p2 = Parent() + c1 = Child() + + p1.children.append(c1) + + assert c1.parent is p1 # backref event establishes c1.parent as p1 + + p2.children.append(c1) + + assert c1.parent is p2 # backref event establishes c1.parent as p2 + assert c1 not in p1.children # second backref event removes c1 from p1.children + +Above, prior to this change, the ``c1`` object would still have been present +in ``p1.children``, even though it is also present in ``p2.children`` at the +same time; the backref handlers would have stopped at replacing ``c1.parent`` with +``p2`` instead of ``p1``. In 0.9, using the more detailed :class:`.Event` +object as well as letting the backref handlers make more detailed decisions about +these objects, the propagation can continue onto removing ``c1`` from ``p1.children`` +while maintaining a check against the propagation from going into an endless +recursive loop. + +End-user code which a. makes use of the :meth:`.AttributeEvents.set`, +:meth:`.AttributeEvents.append`, or :meth:`.AttributeEvents.remove` events, +and b. initiates further attribute modification operations as a result of these +events may need to be modified to prevent recursive loops, as the attribute system +no longer stops a chain of events from propagating endlessly in the absense of the backref +event handlers. Additionally, code which depends upon the value of the ``initiator`` +will need to be adjusted to the new API, and furthermore must be ready for the +value of ``initiator`` to change from its original value within a string of +backref-initiated events, as the backref handlers may now swap in a +new ``initiator`` value for some operations. + +:ticket:`2789` + + .. _migration_2751: Association Proxy SQL Expression Improvements and Fixes @@ -547,6 +619,7 @@ Scenarios which now work correctly include: :ticket:`1765` + Dialect Changes =============== diff --git a/doc/build/orm/internals.rst b/doc/build/orm/internals.rst index 38efdb08a..b5a491a52 100644 --- a/doc/build/orm/internals.rst +++ b/doc/build/orm/internals.rst @@ -27,6 +27,10 @@ sections, are listed here. :members: :show-inheritance: +.. autoclass:: sqlalchemy.orm.attributes.Event + :members: + :show-inheritance: + .. autoclass:: sqlalchemy.orm.interfaces._InspectionAttr :members: :show-inheritance: diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py index 13c2cf256..f4f0cc782 100644 --- a/lib/sqlalchemy/orm/attributes.py +++ b/lib/sqlalchemy/orm/attributes.py @@ -398,6 +398,59 @@ def create_proxied_attribute(descriptor): from_instance=descriptor) return Proxy +OP_REMOVE = util.symbol("REMOVE") +OP_APPEND = util.symbol("APPEND") +OP_REPLACE = util.symbol("REPLACE") + +class Event(object): + """A token propagated throughout the course of a chain of attribute + events. + + Serves as an indicator of the source of the event and also provides + a means of controlling propagation across a chain of attribute + operations. + + The :class:`.Event` object is sent as the ``initiator`` argument + when dealing with the :meth:`.AttributeEvents.append`, + :meth:`.AttributeEvents.set`, + and :meth:`.AttributeEvents.remove` events. + + The :class:`.Event` object is currently interpreted by the backref + event handlers, and is used to control the propagation of operations + across two mutually-dependent attributes. + + .. versionadded:: 0.9.0 + + """ + + impl = None + """The :class:`.AttributeImpl` which is the current event initiator. + """ + + op = None + """The symbol :attr:`.OP_APPEND`, :attr:`.OP_REMOVE` or :attr:`.OP_REPLACE`, + indicating the source operation. + + """ + + def __init__(self, attribute_impl, op): + self.impl = attribute_impl + self.op = op + self.parent_token = self.impl.parent_token + + @classmethod + def _token_gen(self, op): + @util.memoized_property + def gen(self): + return Event(self, op) + return gen + + @property + def key(self): + return self.impl.key + + def hasparent(self, state): + return self.impl.hasparent(state) class AttributeImpl(object): """internal implementation for instrumented attributes.""" @@ -683,7 +736,7 @@ class ScalarAttributeImpl(AttributeImpl): old = dict_.get(self.key, NO_VALUE) if self.dispatch.remove: - self.fire_remove_event(state, dict_, old, None) + self.fire_remove_event(state, dict_, old, self._remove_token) state._modified_event(dict_, self, old) del dict_[self.key] @@ -693,9 +746,6 @@ class ScalarAttributeImpl(AttributeImpl): def set(self, state, dict_, value, initiator, passive=PASSIVE_OFF, check_old=None, pop=False): - if initiator and initiator.parent_token is self.parent_token: - return - if self.dispatch._active_history: old = self.get(state, dict_, PASSIVE_RETURN_NEVER_SET) else: @@ -707,14 +757,17 @@ class ScalarAttributeImpl(AttributeImpl): state._modified_event(dict_, self, old) dict_[self.key] = value + _replace_token = _append_token = Event._token_gen(OP_REPLACE) + _remove_token = Event._token_gen(OP_REMOVE) + def fire_replace_event(self, state, dict_, value, previous, initiator): for fn in self.dispatch.set: - value = fn(state, value, previous, initiator or self) + value = fn(state, value, previous, initiator or self._replace_token) return value def fire_remove_event(self, state, dict_, value, initiator): for fn in self.dispatch.remove: - fn(state, value, initiator or self) + fn(state, value, initiator or self._remove_token) @property def type(self): @@ -736,7 +789,7 @@ class ScalarObjectAttributeImpl(ScalarAttributeImpl): def delete(self, state, dict_): old = self.get(state, dict_) - self.fire_remove_event(state, dict_, old, self) + self.fire_remove_event(state, dict_, old, self._remove_token) del dict_[self.key] def get_history(self, state, dict_, passive=PASSIVE_OFF): @@ -773,14 +826,7 @@ class ScalarObjectAttributeImpl(ScalarAttributeImpl): passive=PASSIVE_OFF, check_old=None, pop=False): """Set a value on the given InstanceState. - `initiator` is the ``InstrumentedAttribute`` that initiated the - ``set()`` operation and is used to control the depth of a circular - setter operation. - """ - if initiator and initiator.parent_token is self.parent_token: - return - if self.dispatch._active_history: old = self.get(state, dict_, passive=PASSIVE_ONLY_PERSISTENT) else: @@ -801,12 +847,13 @@ class ScalarObjectAttributeImpl(ScalarAttributeImpl): value = self.fire_replace_event(state, dict_, value, old, initiator) dict_[self.key] = value + def fire_remove_event(self, state, dict_, value, initiator): if self.trackparent and value is not None: self.sethasparent(instance_state(value), state, False) for fn in self.dispatch.remove: - fn(state, value, initiator or self) + fn(state, value, initiator or self._remove_token) state._modified_event(dict_, self, value) @@ -818,7 +865,7 @@ class ScalarObjectAttributeImpl(ScalarAttributeImpl): self.sethasparent(instance_state(previous), state, False) for fn in self.dispatch.set: - value = fn(state, value, previous, initiator or self) + value = fn(state, value, previous, initiator or self._replace_token) state._modified_event(dict_, self, previous) @@ -902,9 +949,12 @@ class CollectionAttributeImpl(AttributeImpl): return [(instance_state(o), o) for o in current] + _append_token = Event._token_gen(OP_APPEND) + _remove_token = Event._token_gen(OP_REMOVE) + def fire_append_event(self, state, dict_, value, initiator): for fn in self.dispatch.append: - value = fn(state, value, initiator or self) + value = fn(state, value, initiator or self._append_token) state._modified_event(dict_, self, NEVER_SET, True) @@ -921,7 +971,7 @@ class CollectionAttributeImpl(AttributeImpl): self.sethasparent(instance_state(value), state, False) for fn in self.dispatch.remove: - fn(state, value, initiator or self) + fn(state, value, initiator or self._remove_token) state._modified_event(dict_, self, NEVER_SET, True) @@ -948,8 +998,6 @@ class CollectionAttributeImpl(AttributeImpl): self.key, state, self.collection_factory) def append(self, state, dict_, value, initiator, passive=PASSIVE_OFF): - if initiator and initiator.parent_token is self.parent_token: - return collection = self.get_collection(state, dict_, passive=passive) if collection is PASSIVE_NO_RESULT: value = self.fire_append_event(state, dict_, value, initiator) @@ -960,9 +1008,6 @@ class CollectionAttributeImpl(AttributeImpl): collection.append_with_event(value, initiator) def remove(self, state, dict_, value, initiator, passive=PASSIVE_OFF): - if initiator and initiator.parent_token is self.parent_token: - return - collection = self.get_collection(state, state.dict, passive=passive) if collection is PASSIVE_NO_RESULT: self.fire_remove_event(state, dict_, value, initiator) @@ -985,14 +1030,8 @@ class CollectionAttributeImpl(AttributeImpl): passive=PASSIVE_OFF, pop=False): """Set a value on the given object. - `initiator` is the ``InstrumentedAttribute`` that initiated the - ``set()`` operation and is used to control the depth of a circular - setter operation. """ - if initiator and initiator.parent_token is self.parent_token: - return - self._set_iterable( state, dict_, value, lambda adapter, i: adapter.adapt_like_to_iterable(i)) @@ -1085,6 +1124,7 @@ def backref_listeners(attribute, key, uselist): # use easily recognizable names for stack traces parent_token = attribute.impl.parent_token + parent_impl = attribute.impl def _acceptable_key_err(child_state, initiator, child_impl): raise ValueError( @@ -1108,10 +1148,14 @@ def backref_listeners(attribute, key, uselist): old_state, old_dict = instance_state(oldchild),\ instance_dict(oldchild) impl = old_state.manager[key].impl - impl.pop(old_state, - old_dict, - state.obj(), - initiator, passive=PASSIVE_NO_FETCH) + + if initiator.impl is not impl or \ + initiator.op not in (OP_REPLACE, OP_REMOVE): + impl.pop(old_state, + old_dict, + state.obj(), + parent_impl._append_token, + passive=PASSIVE_NO_FETCH) if child is not None: child_state, child_dict = instance_state(child),\ @@ -1120,12 +1164,14 @@ def backref_listeners(attribute, key, uselist): if initiator.parent_token is not parent_token and \ initiator.parent_token is not child_impl.parent_token: _acceptable_key_err(state, initiator, child_impl) - child_impl.append( - child_state, - child_dict, - state.obj(), - initiator, - passive=PASSIVE_NO_FETCH) + elif initiator.impl is not child_impl or \ + initiator.op not in (OP_APPEND, OP_REPLACE): + child_impl.append( + child_state, + child_dict, + state.obj(), + initiator, + passive=PASSIVE_NO_FETCH) return child def emit_backref_from_collection_append_event(state, child, initiator): @@ -1139,7 +1185,9 @@ def backref_listeners(attribute, key, uselist): if initiator.parent_token is not parent_token and \ initiator.parent_token is not child_impl.parent_token: _acceptable_key_err(state, initiator, child_impl) - child_impl.append( + elif initiator.impl is not child_impl or \ + initiator.op not in (OP_APPEND, OP_REPLACE): + child_impl.append( child_state, child_dict, state.obj(), @@ -1152,10 +1200,9 @@ def backref_listeners(attribute, key, uselist): child_state, child_dict = instance_state(child),\ instance_dict(child) child_impl = child_state.manager[key].impl - # can't think of a path that would produce an initiator - # mismatch here, as it would require an existing collection - # mismatch. - child_impl.pop( + if initiator.impl is not child_impl or \ + initiator.op not in (OP_REMOVE, OP_REPLACE): + child_impl.pop( child_state, child_dict, state.obj(), diff --git a/lib/sqlalchemy/orm/dynamic.py b/lib/sqlalchemy/orm/dynamic.py index 5814b47ca..fb46713d0 100644 --- a/lib/sqlalchemy/orm/dynamic.py +++ b/lib/sqlalchemy/orm/dynamic.py @@ -78,6 +78,9 @@ class DynamicAttributeImpl(attributes.AttributeImpl): history = self._get_collection_history(state, passive) return history.added_plus_unchanged + _append_token = attributes.Event._token_gen(attributes.OP_APPEND) + _remove_token = attributes.Event._token_gen(attributes.OP_REMOVE) + def fire_append_event(self, state, dict_, value, initiator, collection_history=None): if collection_history is None: @@ -86,7 +89,7 @@ class DynamicAttributeImpl(attributes.AttributeImpl): collection_history.add_added(value) for fn in self.dispatch.append: - value = fn(state, value, initiator or self) + value = fn(state, value, initiator or self._append_token) if self.trackparent and value is not None: self.sethasparent(attributes.instance_state(value), state, True) @@ -102,7 +105,7 @@ class DynamicAttributeImpl(attributes.AttributeImpl): self.sethasparent(attributes.instance_state(value), state, False) for fn in self.dispatch.remove: - fn(state, value, initiator or self) + fn(state, value, initiator or self._remove_token) def _modified_event(self, state, dict_): diff --git a/lib/sqlalchemy/orm/events.py b/lib/sqlalchemy/orm/events.py index 14b3a770e..9a7190746 100644 --- a/lib/sqlalchemy/orm/events.py +++ b/lib/sqlalchemy/orm/events.py @@ -1561,8 +1561,15 @@ class AttributeEvents(event.Events): is registered with ``retval=True``, the listener function must return this value, or a new value which replaces it. - :param initiator: the attribute implementation object - which initiated this event. + :param initiator: An instance of :class:`.attributes.Event` + representing the initiation of the event. May be modified + from it's original value by backref handlers in order to control + chained event propagation. + + .. versionchanged:: 0.9.0 the ``initiator`` argument is now + passed as a :class:`.attributes.Event` object, and may be modified + by backref handlers within a chain of backref-linked events. + :return: if the event was registered with ``retval=True``, the given value, or a new effective value, should be returned. @@ -1575,8 +1582,15 @@ class AttributeEvents(event.Events): If the listener is registered with ``raw=True``, this will be the :class:`.InstanceState` object. :param value: the value being removed. - :param initiator: the attribute implementation object - which initiated this event. + :param initiator: An instance of :class:`.attributes.Event` + representing the initiation of the event. May be modified + from it's original value by backref handlers in order to control + chained event propagation. + + .. versionchanged:: 0.9.0 the ``initiator`` argument is now + passed as a :class:`.attributes.Event` object, and may be modified + by backref handlers within a chain of backref-linked events. + :return: No return value is defined for this event. """ @@ -1596,8 +1610,15 @@ class AttributeEvents(event.Events): the previous value of the attribute will be loaded from the database if the existing value is currently unloaded or expired. - :param initiator: the attribute implementation object - which initiated this event. + :param initiator: An instance of :class:`.attributes.Event` + representing the initiation of the event. May be modified + from it's original value by backref handlers in order to control + chained event propagation. + + .. versionchanged:: 0.9.0 the ``initiator`` argument is now + passed as a :class:`.attributes.Event` object, and may be modified + by backref handlers within a chain of backref-linked events. + :return: if the event was registered with ``retval=True``, the given value, or a new effective value, should be returned. diff --git a/test/orm/test_attributes.py b/test/orm/test_attributes.py index 4bcecb71b..f3a6f7d8e 100644 --- a/test/orm/test_attributes.py +++ b/test/orm/test_attributes.py @@ -1162,12 +1162,8 @@ class BackrefTest(fixtures.ORMTest): p2.children.append(c1) assert c1.parent is p2 - # note its still in p1.children - - # the event model currently allows only - # one level deep. without the parent_token, - # it keeps going until a ValueError is raised - # and this condition changes. - assert c1 in p1.children + # event propagates to remove as of [ticket:2789] + assert c1 not in p1.children class CyclicBackrefAssertionTest(fixtures.TestBase): """test that infinite recursion due to incorrect backref assignments diff --git a/test/orm/test_backref_mutations.py b/test/orm/test_backref_mutations.py index 925eedfa9..e9448d41c 100644 --- a/test/orm/test_backref_mutations.py +++ b/test/orm/test_backref_mutations.py @@ -75,10 +75,8 @@ class O2MCollectionTest(_fixtures.FixtureTest): # backref fires assert a1.user is u2 - # doesn't extend to the previous collection tho, - # which was already loaded. - # flushing at this point means its anyone's guess. - assert a1 in u1.addresses + # a1 removed from u1.addresses as of [ticket:2789] + assert a1 not in u1.addresses assert a1 in u2.addresses def test_collection_move_notloaded(self): @@ -699,9 +697,8 @@ class O2MStaleBackrefTest(_fixtures.FixtureTest): u1.addresses.append(a1) u2.addresses.append(a1) - # events haven't updated - # u1.addresses here. - u1.addresses.remove(a1) + # a1 removed from u1.addresses as of [ticket:2789] + assert a1 not in u1.addresses assert a1.user is u2 assert a1 in u2.addresses |