- 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

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

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)

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

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):

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