Commits

Mike Bayer  committed d22d63b

- [bug] Lazy loads emitted within flush events
such as before_flush(), before_update(),
etc. will now function as they would
within non-event code, regarding consideration
of the PK/FK values used in the lazy-emitted
query. Previously,
special flags would be established that
would cause lazy loads to load related items
based on the "previous" value of the
parent PK/FK values specifically when called
upon within a flush; the signal to load
in this way is now localized to where the
unit of work actually needs to load that
way. Note that the UOW does
sometimes load these collections before
the before_update() event is called,
so the usage of "passive_updates" or not
can affect whether or not a collection will
represent the "old" or "new" data, when
accessed within a flush event, based
on when the lazy load was emitted.
The change is backwards incompatible in
the exceedingly small chance that
user event code depended on the old
behavior. [ticket:2350]

  • Participants
  • Parent commits f3425ad

Comments (0)

Files changed (5)

     would fail to function subsequent to this
     warning in any case.  [ticket:2405]
 
+  - [bug] Lazy loads emitted within flush events
+    such as before_flush(), before_update(),
+    etc. will now function as they would
+    within non-event code, regarding consideration
+    of the PK/FK values used in the lazy-emitted
+    query.   Previously,
+    special flags would be established that
+    would cause lazy loads to load related items
+    based on the "previous" value of the
+    parent PK/FK values specifically when called
+    upon within a flush; the signal to load
+    in this way is now localized to where the
+    unit of work actually needs to load that
+    way.  Note that the UOW does
+    sometimes load these collections before
+    the before_update() event is called,
+    so the usage of "passive_updates" or not
+    can affect whether or not a collection will
+    represent the "old" or "new" data, when
+    accessed within a flush event, based
+    on when the lazy load was emitted.
+    The change is backwards incompatible in
+    the exceedingly small chance that
+    user event code depended on the old
+    behavior. [ticket:2350]
+
   - [feature] Query now "auto correlates" by
     default in the same way as select() does.
     Previously, a Query used as a subquery

File lib/sqlalchemy/orm/attributes.py

 canonical=16
 )
 
+LOAD_AGAINST_COMMITTED = util.symbol("LOAD_AGAINST_COMMITTED",
+"""callables should use committed values as primary/foreign keys during a load""",
+canonical=32
+)
+
 # pre-packaged sets of flags used as inputs
 PASSIVE_OFF = util.symbol("PASSIVE_OFF",
     "Callables can be emitted in all cases.",

File lib/sqlalchemy/orm/strategies.py

 
     def lazy_clause(self, state, reverse_direction=False,
                                 alias_secondary=False,
-                                adapt_source=None):
+                                adapt_source=None,
+                                passive=None):
         if state is None:
             return self._lazy_none_clause(
                                         reverse_direction,
         else:
             mapper = self.parent_property.parent
 
-        o = state.obj() # strong ref
+        o = state.obj()  # strong ref
         dict_ = attributes.instance_dict(o)
 
         # use the "committed state" only if we're in a flush
         # for this state.
 
-        sess = _state_session(state)
-        if sess is not None and sess._flushing:
+        if passive and passive & attributes.LOAD_AGAINST_COMMITTED:
             def visit_bindparam(bindparam):
                 if bindparam._identifying_key in bind_to_col:
                     bindparam.callable = \
                                 traverse(criterion)
 
         criterion = visitors.cloned_traverse(
-                                criterion, {}, {'bindparam':visit_bindparam})
+                                criterion, {}, {'bindparam': visit_bindparam})
 
         if adapt_source:
             criterion = adapt_source(criterion)
                 not passive & attributes.RELATED_OBJECT_OK:
                 return attributes.PASSIVE_NO_RESULT
 
-        return self._emit_lazyload(session, state, ident_key)
+        return self._emit_lazyload(session, state, ident_key, passive)
 
     def _get_ident_for_use_get(self, session, state, passive):
         instance_mapper = state.manager.mapper
 
-        if session._flushing:
+        if passive & attributes.LOAD_AGAINST_COMMITTED:
             get_attr = instance_mapper._get_committed_state_attr_by_column
         else:
             get_attr = instance_mapper._get_state_attr_by_column
             for pk in self.mapper.primary_key
         ]
 
-    def _emit_lazyload(self, session, state, ident_key):
+    def _emit_lazyload(self, session, state, ident_key, passive):
         q = session.query(self.mapper)._adapt_all_clauses()
 
         q = q._with_invoke_all_eagers(False)
                         not isinstance(rev.strategy, LazyLoader):
                 q = q.options(EagerLazyOption((rev.key,), lazy='select'))
 
-        lazy_clause = self.lazy_clause(state)
+        lazy_clause = self.lazy_clause(state, passive=passive)
 
         if pending:
             bind_values = sql_util.bind_values(lazy_clause)

File lib/sqlalchemy/orm/unitofwork.py

                 and passive & attributes.SQL_OK:
                 impl = state.manager[key].impl
                 history = impl.get_history(state, state.dict,
-                                    attributes.PASSIVE_OFF)
+                                    attributes.PASSIVE_OFF |
+                                    attributes.LOAD_AGAINST_COMMITTED)
                 if history and impl.uses_objects:
                     state_history = history.as_state()
                 else:
             impl = state.manager[key].impl
             # TODO: store the history as (state, object) tuples
             # so we don't have to keep converting here
-            history = impl.get_history(state, state.dict, passive)
+            history = impl.get_history(state, state.dict, passive |
+                                attributes.LOAD_AGAINST_COMMITTED)
             if history and impl.uses_objects:
                 state_history = history.as_state()
             else:
                 state_history = history
-            self.attributes[hashkey] = (history, state_history, passive)
+            self.attributes[hashkey] = (history, state_history,
+                    passive)
 
         return state_history
 

File test/orm/test_unitofworkv2.py

             ),
         )
 
+class LoadersUsingCommittedTest(UOWTest):
+        """Test that events which occur within a flush()
+        get the same attribute loading behavior as on the outside
+        of the flush, and that the unit of work itself uses the
+        "committed" version of primary/foreign key attributes
+        when loading a collection for historical purposes (this typically
+        has importance for when primary key values change).
+
+        """
+
+        def _mapper_setup(self, passive_updates=True):
+            users, Address, addresses, User = (self.tables.users,
+                                    self.classes.Address,
+                                    self.tables.addresses,
+                                    self.classes.User)
+
+            mapper(User, users, properties={
+                'addresses': relationship(Address,
+                    order_by=addresses.c.email_address,
+                    passive_updates=passive_updates,
+                    backref='user')
+            })
+            mapper(Address, addresses)
+            return create_session(autocommit=False)
+
+        def test_before_update_m2o(self):
+            """Expect normal many to one attribute load behavior
+            (should not get committed value)
+            from within public 'before_update' event"""
+            sess = self._mapper_setup()
+
+            Address, User = self.classes.Address, self.classes.User
+
+            def before_update(mapper, connection, target):
+                # if get committed is used to find target.user, then
+                # it will be still be u1 instead of u2
+                assert target.user.id == target.user_id == u2.id
+            from sqlalchemy import event
+            event.listen(Address, 'before_update', before_update)
+
+            a1 = Address(email_address='a1')
+            u1 = User(name='u1', addresses=[a1])
+            sess.add(u1)
+
+            u2 = User(name='u2')
+            sess.add(u2)
+            sess.commit()
+
+            sess.expunge_all()
+            # lookup an address and move it to the other user
+            a1 = sess.query(Address).get(a1.id)
+
+            # move address to another user's fk
+            assert a1.user_id == u1.id
+            a1.user_id = u2.id
+
+            sess.flush()
+
+        def test_before_update_o2m_passive(self):
+            """Expect normal one to many attribute load behavior
+            (should not get committed value)
+            from within public 'before_update' event"""
+            self._test_before_update_o2m(True)
+
+        def test_before_update_o2m_notpassive(self):
+            """Expect normal one to many attribute load behavior
+            (should not get committed value)
+            from within public 'before_update' event with
+            passive_updates=False
+
+            """
+            self._test_before_update_o2m(False)
+
+        def _test_before_update_o2m(self, passive_updates):
+            sess = self._mapper_setup(passive_updates=passive_updates)
+
+            Address, User = self.classes.Address, self.classes.User
+
+            class AvoidReferencialError(Exception):
+                """the test here would require ON UPDATE CASCADE on FKs
+                for the flush to fully succeed; this exception is used
+                to cancel the flush before we get that far.
+
+                """
+
+            def before_update(mapper, connection, target):
+                if passive_updates:
+                    # we shouldn't be using committed value.
+                    # so, having switched target's primary key,
+                    # we expect no related items in the collection
+                    # since we are using passive_updates
+                    # this is a behavior change since #2350
+                    assert 'addresses' not in target.__dict__
+                    eq_(target.addresses, [])
+                else:
+                    # in contrast with passive_updates=True,
+                    # here we expect the orm to have looked up the addresses
+                    # with the committed value (it needs to in order to
+                    # update the foreign keys).  So we expect addresses
+                    # collection to move with the user,
+                    # (just like they will be after the update)
+
+                    # collection is already loaded
+                    assert 'addresses' in target.__dict__
+                    eq_([a.id for a in target.addresses],
+                            [a.id for a in [a1, a2]])
+                raise AvoidReferencialError()
+            from sqlalchemy import event
+            event.listen(User, 'before_update', before_update)
+
+            a1 = Address(email_address='jack1')
+            a2 = Address(email_address='jack2')
+            u1 = User(id=1, name='jack', addresses=[a1, a2])
+            sess.add(u1)
+            sess.commit()
+
+            sess.expunge_all()
+            u1 = sess.query(User).get(u1.id)
+            u1.id = 2
+            try:
+                sess.flush()
+            except AvoidReferencialError:
+                pass