diff options
| -rw-r--r-- | doc/build/changelog/changelog_09.rst | 11 | ||||
| -rw-r--r-- | lib/sqlalchemy/ext/orderinglist.py | 7 | ||||
| -rw-r--r-- | lib/sqlalchemy/orm/collections.py | 10 | ||||
| -rw-r--r-- | test/ext/test_orderinglist.py | 22 | ||||
| -rw-r--r-- | test/orm/test_collection.py | 17 |
5 files changed, 65 insertions, 2 deletions
diff --git a/doc/build/changelog/changelog_09.rst b/doc/build/changelog/changelog_09.rst index 44a2add71..accd827f8 100644 --- a/doc/build/changelog/changelog_09.rst +++ b/doc/build/changelog/changelog_09.rst @@ -14,6 +14,17 @@ :version: 0.9.8 .. change:: + :tags: bug, ext + :verions: 1.0.0 + :tickets: 3191 + + Fixed bug in ordering list where the order of items would be + thrown off during a collection replace event, if the + reorder_on_append flag were set to True. The fix ensures that the + ordering list only impacts the list that is explicitly associated + with the object. + + .. change:: :tags: bug, sql :versions: 1.0.0 :tickets: 3180 diff --git a/lib/sqlalchemy/ext/orderinglist.py b/lib/sqlalchemy/ext/orderinglist.py index 67fda44c4..61155731c 100644 --- a/lib/sqlalchemy/ext/orderinglist.py +++ b/lib/sqlalchemy/ext/orderinglist.py @@ -119,7 +119,7 @@ start numbering at 1 or some other integer, provide ``count_from=1``. """ -from ..orm.collections import collection +from ..orm.collections import collection, collection_adapter from .. import util __all__ = ['ordering_list'] @@ -319,7 +319,10 @@ class OrderingList(list): def remove(self, entity): super(OrderingList, self).remove(entity) - self._reorder() + + adapter = collection_adapter(self) + if adapter and adapter._referenced_by_owner: + self._reorder() def pop(self, index=-1): entity = super(OrderingList, self).pop(index) diff --git a/lib/sqlalchemy/orm/collections.py b/lib/sqlalchemy/orm/collections.py index 1fc0873bd..c2754d58f 100644 --- a/lib/sqlalchemy/orm/collections.py +++ b/lib/sqlalchemy/orm/collections.py @@ -589,6 +589,16 @@ class CollectionAdapter(object): "The entity collection being adapted." return self._data() + @property + def _referenced_by_owner(self): + """return True if the owner state still refers to this collection. + + This will return False within a bulk replace operation, + where this collection is the one being replaced. + + """ + return self.owner_state.dict[self._key] is self._data() + @util.memoized_property def attr(self): return self.owner_state.manager[self._key].impl diff --git a/test/ext/test_orderinglist.py b/test/ext/test_orderinglist.py index 3223c8048..0eba137e7 100644 --- a/test/ext/test_orderinglist.py +++ b/test/ext/test_orderinglist.py @@ -349,6 +349,28 @@ class OrderingListTest(fixtures.TestBase): self.assert_(srt.bullets[1].text == 'new 2') self.assert_(srt.bullets[2].text == '3') + def test_replace_two(self): + """test #3191""" + + self._setup(ordering_list('position', reorder_on_append=True)) + + s1 = Slide('Slide #1') + + b1, b2, b3, b4 = Bullet('1'), Bullet('2'), Bullet('3'), Bullet('4') + s1.bullets = [b1, b2, b3] + + eq_( + [b.position for b in s1.bullets], + [0, 1, 2] + ) + + s1.bullets = [b4, b2, b1] + eq_( + [b.position for b in s1.bullets], + [0, 1, 2] + ) + + def test_funky_ordering(self): class Pos(object): def __init__(self): diff --git a/test/orm/test_collection.py b/test/orm/test_collection.py index f94c742b3..82331b9af 100644 --- a/test/orm/test_collection.py +++ b/test/orm/test_collection.py @@ -2191,6 +2191,23 @@ class InstrumentationTest(fixtures.ORMTest): f1.attr = l2 eq_(canary, [adapter_1, f1.attr._sa_adapter, None]) + def test_referenced_by_owner(self): + + class Foo(object): + pass + + instrumentation.register_class(Foo) + attributes.register_attribute( + Foo, 'attr', uselist=True, useobject=True) + + f1 = Foo() + f1.attr.append(3) + + adapter = collections.collection_adapter(f1.attr) + assert adapter._referenced_by_owner + + f1.attr = [] + assert not adapter._referenced_by_owner |