- 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.
fixes #3191

diff --git a/doc/build/changelog/changelog_09.rst b/doc/build/changelog/changelog_09.rst
index 44a2add716aa1ef1c4dc6062c4268715ade49c2c..accd827f8f933812553c3e5d9096542dfd26faad 100644 (file)
--- a/doc/build/changelog/changelog_09.rst
+++ b/doc/build/changelog/changelog_09.rst
@@ -13,6 +13,17 @@
 .. changelog::
     :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

diff --git a/lib/sqlalchemy/ext/orderinglist.py b/lib/sqlalchemy/ext/orderinglist.py
index 67fda44c40a82127840a3332906aa9c462ca4512..61155731c25d498b2895735e5315cabcf11bd376 100644 (file)
--- 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 698677a0bb1749f3e7e825b813f6152975668c28..3c17bc09e94d4ef61906bbeeb492cc52fd7329e6 100644 (file)
--- a/lib/sqlalchemy/orm/collections.py
+++ b/lib/sqlalchemy/orm/collections.py
@@ -585,6 +585,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 3223c8048b0cb67c15e5f831de07ebc8661e6e23..0eba137e7dfd6ca897226a1cdd00eb50a3fc6614 100644 (file)
--- 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 f94c742b358162a1e50873fd730947423146b1b4..82331b9af14bc10f6fbe484806723b8a0493aaae 100644 (file)
--- 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