From c57101bf4954e53bb3b31934720b52885adc0707 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Fri, 21 May 2010 17:44:56 -0400 Subject: [PATCH] - Fixed regression introduced in 0.6.0 unit of work refactor that broke updates for bi-directional relationship() with post_update=True. [ticket:1807] --- CHANGES | 4 ++ lib/sqlalchemy/orm/dependency.py | 75 ++++++++++++++++---------------- test/orm/test_cycles.py | 53 ++++++++++++++++++++-- 3 files changed, 92 insertions(+), 40 deletions(-) diff --git a/CHANGES b/CHANGES index 3a41e098c7..17ede848de 100644 --- a/CHANGES +++ b/CHANGES @@ -9,6 +9,10 @@ CHANGES - Fixed regression introduced in 0.6.0 involving improper history accounting on mutable attributes. [ticket:1782] + - Fixed regression introduced in 0.6.0 unit of work refactor + that broke updates for bi-directional relationship() + with post_update=True. [ticket:1807] + - session.merge() will not expire attributes on the returned instance if that instance is "pending". [ticket:1789] diff --git a/lib/sqlalchemy/orm/dependency.py b/lib/sqlalchemy/orm/dependency.py index 9f1b78f4ad..b2c3d1fb9d 100644 --- a/lib/sqlalchemy/orm/dependency.py +++ b/lib/sqlalchemy/orm/dependency.py @@ -52,16 +52,10 @@ class DependencyProcessor(object): the aggreagte. """ - if self.post_update and self._check_reverse(uow): - return - uow.register_preprocessor(self, True) def per_property_flush_actions(self, uow): - if self.post_update and self._check_reverse(uow): - return - after_save = unitofwork.ProcessAll(uow, self, False, True) before_delete = unitofwork.ProcessAll(uow, self, True, True) @@ -258,27 +252,33 @@ class DependencyProcessor(object): clearkeys, uowcommit): raise NotImplementedError() - def _check_reverse(self, uow): - """return True if a comparable dependency processor has - already set up on the "reverse" side of a relationship. - - """ - for p in self.prop._reverse_property: - if not p.viewonly and p._dependency_processor and \ - uow.has_dep(p._dependency_processor): - return True - else: - return False + def _get_reversed_processed_set(self, uow): + if not self.prop._reverse_property: + return None - def _post_update(self, state, uowcommit, related): + process_key = tuple(sorted( + [self.key] + + [p.key for p in self.prop._reverse_property] + )) + return uow.memo( + ('reverse_key', process_key), + set + ) + + def _post_update(self, state, uowcommit, related, processed): + if processed is not None and state in processed: + return for x in related: if x is not None: uowcommit.issue_post_update( state, [r for l, r in self.prop.synchronize_pairs] ) + if processed is not None: + processed.add(state) + break - + def _pks_changed(self, uowcommit, state): raise NotImplementedError() @@ -426,6 +426,9 @@ class OneToManyDP(DependencyProcessor): # key to the parent set to NULL this phase can be called # safely for any cascade but is unnecessary if delete cascade # is on. + if self.post_update: + processed = self._get_reversed_processed_set(uowcommit) + if self.post_update or not self.passive_deletes == 'all': children_added = uowcommit.memo(('children_added', self), set) @@ -446,7 +449,7 @@ class OneToManyDP(DependencyProcessor): self._post_update( child, uowcommit, - [state]) + [state], processed) if self.post_update or not self.cascade.delete: for child in set(history.unchanged).\ difference(children_added): @@ -459,13 +462,16 @@ class OneToManyDP(DependencyProcessor): self._post_update( child, uowcommit, - [state]) + [state], processed) # technically, we can even remove each child from the # collection here too. but this would be a somewhat # inconsistent behavior since it wouldn't happen if the old # parent wasn't deleted but child was moved. def process_saves(self, uowcommit, states): + if self.post_update: + processed = self._get_reversed_processed_set(uowcommit) + for state in states: history = uowcommit.get_attribute_history(state, self.key, passive=True) if history: @@ -475,7 +481,9 @@ class OneToManyDP(DependencyProcessor): self._post_update( child, uowcommit, - [state]) + [state], + processed + ) for child in history.deleted: if not self.cascade.delete_orphan and \ @@ -622,6 +630,9 @@ class ManyToOneDP(DependencyProcessor): if self.post_update and \ not self.cascade.delete_orphan and \ not self.passive_deletes == 'all': + + processed = self._get_reversed_processed_set(uowcommit) + # post_update means we have to update our # row to not reference the child object # before we can DELETE the row @@ -636,9 +647,12 @@ class ManyToOneDP(DependencyProcessor): self._post_update( state, uowcommit, - history.sum()) + history.sum(), processed) def process_saves(self, uowcommit, states): + if self.post_update: + processed = self._get_reversed_processed_set(uowcommit) + for state in states: history = uowcommit.get_attribute_history(state, self.key, passive=True) if history: @@ -648,7 +662,7 @@ class ManyToOneDP(DependencyProcessor): if self.post_update: self._post_update( state, - uowcommit, history.sum()) + uowcommit, history.sum(), processed) def _synchronize(self, state, child, associationrow, clearkeys, uowcommit): if state is None or (not self.post_update and uowcommit.is_deleted(state)): @@ -850,19 +864,6 @@ class ManyToManyDP(DependencyProcessor): uowcommit.register_object( attributes.instance_state(c), isdelete=True) - def _get_reversed_processed_set(self, uow): - if not self.prop._reverse_property: - return None - - process_key = tuple(sorted( - [self.key] + - [p.key for p in self.prop._reverse_property] - )) - return uow.memo( - ('reverse_key', process_key), - set - ) - def process_deletes(self, uowcommit, states): secondary_delete = [] secondary_insert = [] diff --git a/test/orm/test_cycles.py b/test/orm/test_cycles.py index aca5fa9dbd..0327e8a9a1 100644 --- a/test/orm/test_cycles.py +++ b/test/orm/test_cycles.py @@ -8,7 +8,8 @@ T1/T2. from sqlalchemy.test import testing from sqlalchemy import Integer, String, ForeignKey from sqlalchemy.test.schema import Table, Column -from sqlalchemy.orm import mapper, relationship, backref, create_session +from sqlalchemy.orm import mapper, relationship, backref, \ + create_session, sessionmaker from sqlalchemy.test.testing import eq_ from sqlalchemy.test.assertsql import RegexSQL, ExactSQL, CompiledSQL, AllOf from test.orm import _base @@ -532,10 +533,10 @@ class OneToManyManyToOneTest(_base.MappedTest): @classmethod def setup_classes(cls): - class Person(_base.BasicEntity): + class Person(_base.ComparableEntity): pass - class Ball(_base.BasicEntity): + class Ball(_base.ComparableEntity): pass @testing.resolve_artifact_names @@ -614,6 +615,52 @@ class OneToManyManyToOneTest(_base.MappedTest): ExactSQL("DELETE FROM person WHERE person.id = :id", lambda ctx:[{'id': p.id}]) ) + @testing.resolve_artifact_names + def test_post_update_backref(self): + """test bidirectional post_update.""" + + mapper(Ball, ball) + mapper(Person, person, properties=dict( + balls=relationship(Ball, + primaryjoin=ball.c.person_id == person.c.id, + remote_side=ball.c.person_id, post_update=True, + backref=backref('person', post_update=True) + ), + favorite=relationship(Ball, + primaryjoin=person.c.favorite_ball_id == ball.c.id, + remote_side=person.c.favorite_ball_id) + + )) + + sess = sessionmaker()() + p1 = Person(data='p1') + p2 = Person(data='p2') + p3 = Person(data='p3') + + b1 = Ball(data='b1') + + b1.person = p1 + sess.add_all([p1, p2, p3]) + sess.commit() + + # switch here. the post_update + # on ball.person can't get tripped up + # by the fact that there's a "reverse" prop. + b1.person = p2 + sess.commit() + eq_( + p2, b1.person + ) + + # do it the other way + p3.balls.append(b1) + sess.commit() + eq_( + p3, b1.person + ) + + + @testing.resolve_artifact_names def test_post_update_o2m(self): """A cycle between two rows, with a post_update on the one-to-many""" -- 2.47.2