From a645032262afc41cd39a85c9f56388341346a995 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Thu, 7 Apr 2011 17:56:01 -0400 Subject: [PATCH] - Some fixes to the state handling regarding backrefs, typically when autoflush=False, where the back-referenced collection wouldn't properly handle add/removes with no net change. Thanks to Richard Murri for the test case + patch. [ticket:2123] (also in 0.6.7). --- CHANGES | 8 ++ lib/sqlalchemy/orm/attributes.py | 1 - lib/sqlalchemy/orm/state.py | 6 +- test/orm/test_attributes.py | 29 ++++--- test/orm/test_backref_mutations.py | 120 ++++++++++++++++++++++++++++- 5 files changed, 147 insertions(+), 17 deletions(-) diff --git a/CHANGES b/CHANGES index a1d49f0368..ddf6467c7b 100644 --- a/CHANGES +++ b/CHANGES @@ -46,6 +46,14 @@ CHANGES can't find the target entity. Explain that the path must be from one of the root entities. + - Some fixes to the state handling regarding + backrefs, typically when autoflush=False, where + the back-referenced collection wouldn't + properly handle add/removes with no net + change. Thanks to Richard Murri for the + test case + patch. [ticket:2123] + (also in 0.6.7). + - sql - Restored the "catchall" constructor on the base TypeEngine class, with a deprecation warning. diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py index ead62d1fee..c084db8a88 100644 --- a/lib/sqlalchemy/orm/attributes.py +++ b/lib/sqlalchemy/orm/attributes.py @@ -871,7 +871,6 @@ class CollectionAttributeImpl(AttributeImpl): state.commit(dict_, [self.key]) if self.key in state.pending: - # pending items exist. issue a modified event, # add/remove new items. state.modified_event(dict_, self, user_data, True) diff --git a/lib/sqlalchemy/orm/state.py b/lib/sqlalchemy/orm/state.py index e29c5c03ad..00582fbdbd 100644 --- a/lib/sqlalchemy/orm/state.py +++ b/lib/sqlalchemy/orm/state.py @@ -547,10 +547,12 @@ class PendingCollection(object): def append(self, value): if value in self.deleted_items: self.deleted_items.remove(value) - self.added_items.add(value) + else: + self.added_items.add(value) def remove(self, value): if value in self.added_items: self.added_items.remove(value) - self.deleted_items.add(value) + else: + self.deleted_items.add(value) diff --git a/test/orm/test_attributes.py b/test/orm/test_attributes.py index a05f5f70df..b8002a446f 100644 --- a/test/orm/test_attributes.py +++ b/test/orm/test_attributes.py @@ -1041,24 +1041,29 @@ class PendingBackrefTest(fixtures.ORMTest): ([p, p4], [p1, p2, p3], [])) assert called[0] == 1 - def test_lazy_remove(self): - global lazy_load - called[0] = 0 - lazy_load = [] - + def test_state_on_add_remove(self): b = Blog("blog 1") p = Post("post 1") p.blog = b - assert called[0] == 0 + p.blog = None - lazy_load = [p] + eq_(called[0], 0) + eq_(b.posts, []) + eq_(called[0], 1) - p.blog = None + def test_pending_combines_with_lazy(self): + global lazy_load + b = Blog("blog 1") + p = Post("post 1") p2 = Post("post 2") - p2.blog = b - assert called[0] == 0 - assert b.posts == [p2] - assert called[0] == 1 + p.blog = b + lazy_load = [p, p2] + # lazy loaded + pending get added together. + # This isn't seen often with the ORM due + # to usual practices surrounding the + # load/flush/load cycle. + eq_(b.posts, [p, p2, p]) + eq_(called[0], 1) def test_normal_load(self): global lazy_load diff --git a/test/orm/test_backref_mutations.py b/test/orm/test_backref_mutations.py index 3d13ae02db..0e11655044 100644 --- a/test/orm/test_backref_mutations.py +++ b/test/orm/test_backref_mutations.py @@ -13,7 +13,8 @@ from test.lib.testing import assert_raises, assert_raises_message from sqlalchemy import Integer, String, ForeignKey, Sequence, exc as sa_exc from test.lib.schema import Table from test.lib.schema import Column -from sqlalchemy.orm import mapper, relationship, create_session, class_mapper, backref, sessionmaker +from sqlalchemy.orm import mapper, relationship, create_session, \ + class_mapper, backref, sessionmaker, Session from sqlalchemy.orm import attributes, exc as orm_exc from test.lib import testing from test.lib.testing import eq_ @@ -238,6 +239,7 @@ class O2MCollectionTest(_fixtures.FixtureTest): assert a1 not in u1.addresses assert a1 in u2.addresses + class O2OScalarBackrefMoveTest(_fixtures.FixtureTest): run_inserts = None @@ -477,6 +479,118 @@ class O2OScalarOrphanTest(_fixtures.FixtureTest): assert sess.query(User).count() == 1 +class M2MCollectionMoveTest(_fixtures.FixtureTest): + run_inserts = None + + @classmethod + def setup_mappers(cls): + keywords, items, item_keywords, Keyword, Item = (cls.tables.keywords, + cls.tables.items, + cls.tables.item_keywords, + cls.classes.Keyword, + cls.classes.Item) + + mapper(Item, items, properties={ + 'keywords':relationship(Keyword, secondary=item_keywords, + backref='items') + }) + mapper(Keyword, keywords) + + def test_add_remove_pending_backref(self): + """test that pending doesn't add an item that's not a net add.""" + + Item, Keyword = (self.classes.Item, self.classes.Keyword) + + session = Session(autoflush=False) + + i1 = Item(description='i1') + session.add(i1) + session.commit() + + session.expire(i1, ['keywords']) + + k1= Keyword(name='k1') + k1.items.append(i1) + k1.items.remove(i1) + eq_(i1.keywords, []) + + def test_remove_add_pending_backref(self): + """test that pending doesn't remove an item that's not a net remove.""" + + Item, Keyword = (self.classes.Item, self.classes.Keyword) + + session = Session(autoflush=False) + + k1= Keyword(name='k1') + i1 = Item(description='i1', keywords=[k1]) + session.add(i1) + session.commit() + + session.expire(i1, ['keywords']) + + k1.items.remove(i1) + k1.items.append(i1) + eq_(i1.keywords, [k1]) + + def test_pending_combines_with_flushed(self): + """test the combination of unflushed pending + lazy loaded from DB.""" + + Item, Keyword = (self.classes.Item, self.classes.Keyword) + + session = Session(testing.db, autoflush=False) + + k1 = Keyword(name='k1') + k2 = Keyword(name='k2') + i1 = Item(description='i1', keywords=[k1]) + session.add(i1) + session.add(k2) + session.commit() + + k2.items.append(i1) + # the pending + # list is still here. + eq_( + set(attributes.instance_state(i1). + pending['keywords'].added_items), + set([k2]) + ) + # because autoflush is off, k2 is still + # coming in from pending + eq_(i1.keywords, [k1, k2]) + + # prove it didn't flush + eq_(session.scalar("select count(*) from item_keywords"), 1) + + # the pending collection was removed + assert 'keywords' not in attributes.\ + instance_state(i1).\ + pending + + def test_duplicate_adds(self): + Item, Keyword = (self.classes.Item, self.classes.Keyword) + + session = Session(testing.db, autoflush=False) + + k1 = Keyword(name='k1') + i1 = Item(description='i1', keywords=[k1]) + session.add(i1) + session.commit() + + k1.items.append(i1) + eq_(i1.keywords, [k1, k1]) + + session.expire(i1, ['keywords']) + k1.items.append(i1) + eq_(i1.keywords, [k1, k1]) + + session.expire(i1, ['keywords']) + k1.items.append(i1) + eq_(i1.keywords, [k1, k1]) + + eq_(k1.items, [i1, i1, i1, i1]) + session.commit() + eq_(k1.items, [i1]) + class M2MScalarMoveTest(_fixtures.FixtureTest): run_inserts = None @@ -489,7 +603,9 @@ class M2MScalarMoveTest(_fixtures.FixtureTest): cls.classes.Item) mapper(Item, items, properties={ - 'keyword':relationship(Keyword, secondary=item_keywords, uselist=False, backref=backref("item", uselist=False)) + 'keyword':relationship(Keyword, secondary=item_keywords, + uselist=False, + backref=backref("item", uselist=False)) }) mapper(Keyword, keywords) -- 2.39.5