Merge "Skip on slice assignment to self"

3 files changed, 45 insertions, 4 deletions

diff --git a/doc/build/changelog/unreleased_13/4990.rst b/doc/build/changelog/unreleased_13/4990.rst
new file mode 100644
index 000000000..702403dd5
--- /dev/null
+++ b/doc/build/changelog/unreleased_13/4990.rst
@@ -0,0 +1,11 @@
+.. change::
+    :tags: bug, orm, py3k
+    :tickets: 4990
+
+    Fixed issue where when assigning a collection to itself as a slice, the
+    mutation operation would fail as it would first erase the assigned
+    collection inadvertently.   As an assignment that does not change  the
+    contents should not generate events, the operation is now a no-op. Note
+    that the fix only applies to Python 3; in Python 2, the ``__setitem__``
+    hook isn't called in this case; ``__setslice__`` is used instead which
+    recreates the list item-by-item in all cases.
\ No newline at end of file
diff --git a/lib/sqlalchemy/orm/collections.py b/lib/sqlalchemy/orm/collections.py
index 4b096c152..5008f5727 100644
--- a/lib/sqlalchemy/orm/collections.py
+++ b/lib/sqlalchemy/orm/collections.py
@@ -1201,6 +1201,8 @@ def _list_decorators():
                     stop += len(self)
 
                 if step == 1:
+                    if value is self:
+                        return
                     for i in range(start, stop, step):
                         if len(self) > start:
                             del self[start]
diff --git a/test/orm/test_collection.py b/test/orm/test_collection.py
index 34b7fc5fe..03b6c8758 100644
--- a/test/orm/test_collection.py
+++ b/test/orm/test_collection.py
@@ -1,3 +1,4 @@
+import contextlib
 from operator import and_
 
 from sqlalchemy import event
@@ -30,6 +31,15 @@ class Canary(object):
         self.data = set()
         self.added = set()
         self.removed = set()
+        self.dupe_check = True
+
+    @contextlib.contextmanager
+    def defer_dupe_check(self):
+        self.dupe_check = False
+        try:
+            yield
+        finally:
+            self.dupe_check = True
 
     def listen(self, attr):
         event.listen(attr, "append", self.append)
@@ -37,15 +47,17 @@ class Canary(object):
         event.listen(attr, "set", self.set)
 
     def append(self, obj, value, initiator):
-        assert value not in self.added
+        if self.dupe_check:
+            assert value not in self.added
+            self.added.add(value)
         self.data.add(value)
-        self.added.add(value)
         return value
 
     def remove(self, obj, value, initiator):
-        assert value not in self.removed
+        if self.dupe_check:
+            assert value not in self.removed
+            self.removed.add(value)
         self.data.remove(value)
-        self.removed.add(value)
 
     def set(self, obj, value, oldvalue, initiator):
         if isinstance(value, str):
@@ -299,6 +311,22 @@ class CollectionsTest(fixtures.ORMTest):
             control[:] = values
             assert_eq()
 
+            # test slice assignment where we slice assign to self,
+            # currently a no-op, issue #4990
+            # note that in py2k, the bug does not exist but it recreates
+            # the collection which breaks our fixtures here
+            with canary.defer_dupe_check():
+                direct[:] = direct
+                control[:] = control
+            assert_eq()
+
+            # we dont handle assignment of self to slices, as this
+            # implies duplicate entries.  behavior here is not well defined
+            # and perhaps should emit a warning
+            # direct[0:1] = list(direct)
+            # control[0:1] = list(control)
+            # assert_eq()
+
             # test slice assignment where
             # slice size goes over the number of items
             values = [creator(), creator()]