diff options
author | Mike Bayer <mike_mp@zzzcomputing.com> | 2013-07-26 00:01:04 -0400 |
---|---|---|
committer | Mike Bayer <mike_mp@zzzcomputing.com> | 2013-07-26 00:01:04 -0400 |
commit | 550141b14c8e165218cd32c27d91541eeee86d2a (patch) | |
tree | 1a87bc7934aab81f4b1b84c9100e73b56d45d09a | |
parent | e60c16c7e6c2494b623553f41694c1ebde4d65d8 (diff) | |
download | sqlalchemy-550141b14c8e165218cd32c27d91541eeee86d2a.tar.gz |
- 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.
[ticket:2789]
-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 |