From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Tue, 19 Nov 2019 14:30:31 +0000 (-0500)
Subject: Skip on slice assignment to self
X-Git-Tag: rel_1_3_12~15
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=24517c01041aaeb0dfcbc5cb7840e0f78b55e90e;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

Skip on slice assignment to self

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.

Fixes: #4990
Change-Id: I08727880f70f4fe188de53a4dcd36746b62c7233
(cherry picked from commit 560044748a8ff5488769f8ebfa8a353a8d0115fa)
---

diff --git a/doc/build/changelog/unreleased_13/4990.rst b/doc/build/changelog/unreleased_13/4990.rst
new file mode 100644
index 0000000000..702403dd5a
--- /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 b9297e15c9..45baff2c48 100644
--- a/lib/sqlalchemy/orm/collections.py
+++ b/lib/sqlalchemy/orm/collections.py
@@ -1155,6 +1155,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 83f4f44511..3003445ebe 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
@@ -28,6 +29,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)
@@ -35,15 +45,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):
@@ -259,6 +271,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()]