cqlengine: remove the negative indices slicing support in ModelQuerySet

aboudreault · aboudreault · commit 3d18ff6f8f48 · 2017-12-08T08:25:54.000-05:00
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
@@ -15,6 +15,7 @@ Other
 -----
 * Cassandra 2.0 support removal (PYTHON-716)
 * cqlengine: disallow Counter create, save operations (PYTHON-497)
+* cqlengine: remove the negative indices slicing support in ModelQuerySet (PYTHON-875)
 
 3.12.0
 ======
diff --git a/cassandra/cqlengine/query.py b/cassandra/cqlengine/query.py
@@ -562,13 +562,15 @@ def __getitem__(self, s):
         if isinstance(s, slice):
             start = s.start if s.start else 0
 
-            # calculate the amount of results that need to be loaded
-            end = s.stop
-            if start < 0 or s.stop is None or s.stop < 0:
-                end = self.count()
+            # no support for negative indices
+            if start < 0 or (s.stop is not None and s.stop < 0):
+                raise ValueError("QuerySet slice indices must be positive")
 
             try:
-                self._fill_result_cache_to_idx(end)
+                if s.stop:
+                    self._fill_result_cache_to_idx(s.stop)
+                else:
+                    self._fill_result_cache()
             except StopIteration:
                 pass
 
@@ -579,10 +581,9 @@ def __getitem__(self, s):
             except (ValueError, TypeError):
                 raise TypeError('QuerySet indices must be integers')
 
-            # Using negative indexing is costly since we have to execute a count()
+            # no support for negative indices
             if s < 0:
-                num_results = self.count()
-                s += num_results
+                raise ValueError("QuerySet indices must be positive")
 
             try:
                 self._fill_result_cache_to_idx(s)
diff --git a/docs/cqlengine/upgrade_guide.rst b/docs/cqlengine/upgrade_guide.rst
@@ -64,6 +64,11 @@ Upgrade Guide 3.x to 4.x
     v.rating_counter += 1
     v.update()
 
+* ModelQuerySet support for negative slice indices has been removed due to the
+  inefficiency and performance cost. The following code now raises an error::
+
+    Automobile.objects.all()[-10:]
+
 Upgrade Guide cqlengine to cassandra.cqlengine
 ==============================================
 
diff --git a/tests/integration/cqlengine/query/test_queryset.py b/tests/integration/cqlengine/query/test_queryset.py
@@ -653,11 +653,10 @@ def test_array_indexing_works_properly(self):
             assert q[i].attempt_id == expected_order[i]
 
     @execute_count(1)
-    def test_negative_indexing_works_properly(self):
+    def test_negative_indexing_raises_error(self):
         q = TestModel.objects(test_id=0).order_by('attempt_id')
-        expected_order = [0, 1, 2, 3]
-        assert q[-1].attempt_id == expected_order[-1]
-        assert q[-2].attempt_id == expected_order[-2]
+        with self.assertRaises(ValueError):
+            q[-1].attempt_id
 
     @execute_count(1)
     def test_slicing_works_properly(self):
@@ -670,25 +669,33 @@ def test_slicing_works_properly(self):
         for model, expect in zip(q[0:3:2], expected_order[0:3:2]):
             self.assertEqual(model.attempt_id, expect)
 
+        for model, expect in zip(q[0:], expected_order):
+            self.assertEqual(model.attempt_id, expect)
+
     @execute_count(1)
-    def test_negative_slicing(self):
+    def test_negative_slicing_raises_error(self):
         q = TestModel.objects(test_id=0).order_by('attempt_id')
-        expected_order = [0, 1, 2, 3]
 
-        for model, expect in zip(q[-3:], expected_order[-3:]):
-            self.assertEqual(model.attempt_id, expect)
+        with self.assertRaises(ValueError):
+            q[-1:]
 
-        for model, expect in zip(q[:-1], expected_order[:-1]):
-            self.assertEqual(model.attempt_id, expect)
+        with self.assertRaises(ValueError):
+            q[:-1]
 
-        for model, expect in zip(q[1:-1], expected_order[1:-1]):
-            self.assertEqual(model.attempt_id, expect)
+        with self.assertRaises(ValueError):
+            q[1:-1]
 
-        for model, expect in zip(q[-3:-1], expected_order[-3:-1]):
-            self.assertEqual(model.attempt_id, expect)
+        with self.assertRaises(ValueError):
+            q[-1:-1]
 
-        for model, expect in zip(q[-3:-1:2], expected_order[-3:-1:2]):
-            self.assertEqual(model.attempt_id, expect)
+        with self.assertRaises(ValueError):
+            q[-1:1]
+
+        with self.assertRaises(ValueError):
+            q[-1:4:2]
+
+        with self.assertRaises(ValueError):
+            q[4:-1:2]
 
 
 class TestQuerySetValidation(BaseQuerySetUsage):
