launchpad-reviewers team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

Brad Crittenden has proposed merging lp:~bac/lazr.batchnavigator/bug-826839 into lp:lazr.batchnavigator.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #826839 in Launchpad itself: "DataError: OFFSET must not be negative"
  https://bugs.launchpad.net/launchpad/+bug/826839

For more details, see:
https://code.launchpad.net/~bac/lazr.batchnavigator/bug-826839/+merge/72247

Negative slice indexing into Storm/Postgresql empty result sets causes an exception to be raised.  In that instance, prevent the index from being negative.
-- 
https://code.launchpad.net/~bac/lazr.batchnavigator/bug-826839/+merge/72247
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/lazr.batchnavigator/bug-826839 into lp:lazr.batchnavigator.

=== modified file 'src/lazr/batchnavigator/_batchnavigator.py'
--- src/lazr/batchnavigator/_batchnavigator.py	2011-08-17 14:34:35 +0000
+++ src/lazr/batchnavigator/_batchnavigator.py	2011-08-19 18:57:23 +0000
@@ -363,7 +363,8 @@
                     self.results = IFiniteSequence(self.results)
                 total_length = len(self.results)
                 offset = total_length
-                offset_plus_size = offset + offset_plus_size
+                # Storm raises an exception for a negative initial offset.
+                offset_plus_size = max(offset + offset_plus_size, 0)
             result = list(self.results[offset_plus_size:offset])
             result.reverse()
             return result

=== modified file 'src/lazr/batchnavigator/tests/test_batchnavigator.py'
--- src/lazr/batchnavigator/tests/test_batchnavigator.py	2011-08-17 14:34:35 +0000
+++ src/lazr/batchnavigator/tests/test_batchnavigator.py	2011-08-19 18:57:23 +0000
@@ -66,6 +66,35 @@
     return BatchNavigator(collection, request, range_factory=range_factory)
 
 
+class PickyListLikeCollection:
+    """Collection that really hates slices with negative start values.
+
+    Some database-backed collections, e.g. Postgres via Storm, throw
+    exceptions if a slice is used that starts with a negative value.  When
+    batch navigator is going backwards it is easy to pass such a slice.  This
+    collection is used to ensure it is noticed if that happens.
+    """
+    def __init__(self, collection):
+        self._collection = collection
+    def __iter__(self):
+        return iter(self._collection)
+    def __getitem__(self, index):
+        if (isinstance(index, slice) and
+            index.start is not None and index.start < 0):
+            raise RuntimeError
+        return self._collection.__getitem__(index)
+    def __len__(self):
+        return self._collection.__len__()
+
+
+def empty_batchnav(start=None, batch=None, memo=None, direction=None):
+    collection = PickyListLikeCollection([])
+    request = batchnav_request(start=start, batch=batch, memo=memo,
+        direction=direction)
+    range_factory = RecordingFactory(collection)
+    return BatchNavigator(collection, request, range_factory=range_factory)
+
+
 class EqualsQuery(Equals):
 
     def __init__(self, start=None, batch=None, direction=None, memo=None):
@@ -80,7 +109,8 @@
         results = [1, 2, 3, 4]
         range_factory = ListRangeFactory(results)
         request = batchnav_request()
-        batchnav = BatchNavigator(results, request, range_factory=range_factory)
+        batchnav = BatchNavigator(
+            results, request, range_factory=range_factory)
         self.assertEqual(batchnav.batch.range_factory, range_factory)
 
     def test_without_memo_direction_gets_non_factory_batch(self):
@@ -167,6 +197,16 @@
             batchnav.nextBatchURL(),
             Equals(SERVER_URL + '?batch=3&memo=end%26%2F2&start=9'))
 
+    def test_empty_collection(self):
+        batchnav = empty_batchnav(
+            start=2, batch=2)
+        self.assertEqual([], list(batchnav.currentBatch()))
+
+    def test_empty_collection_backwards(self):
+        batchnav = empty_batchnav(
+            start=2, batch=2, direction='backwards')
+        self.assertEqual([], list(batchnav.currentBatch()))
+
 
 class TestListRangeFactory(testtools.TestCase):
 
@@ -221,42 +261,70 @@
     def test_getslice_next_end(self):
         # at the end, crickets...
         results = [0, 1, 2, 3, 4, 5]
-        slice = ListRangeFactory(results).getSlice(3, '6')
-        self.assertEqual([], slice)
+        _slice = ListRangeFactory(results).getSlice(3, '6')
+        self.assertEqual([], _slice)
 
     def test_getslice_before_start(self):
         # at the end, crickets...
         results = [0, 1, 2, 3, 4, 5]
-        slice = list(ListRangeFactory(results).getSlice(3, '0', forwards=False))
-        self.assertEqual([], slice)
+        _slice = list(ListRangeFactory(results).getSlice(3, '0', forwards=False))
+        self.assertEqual([], _slice)
 
     def test_getslice_before_end(self):
         results = [0, 1, 2, 3, 4, 5]
-        slice = list(ListRangeFactory(results).getSlice(3, '6', forwards=False))
-        self.assertEqual([5, 4, 3], slice)
+        _slice = list(ListRangeFactory(results).getSlice(3, '6', forwards=False))
+        self.assertEqual([5, 4, 3], _slice)
 
     def test_getslice_next(self):
         # The slice returned starts where indicated but continues on.
         results = [0, 1, 2, 3, 4, 5]
-        slice = ListRangeFactory(results).getSlice(3, '3')
-        self.assertEqual([3, 4, 5], slice)
+        _slice = ListRangeFactory(results).getSlice(3, '3')
+        self.assertEqual([3, 4, 5], _slice)
 
     def test_getslice_before_middle(self):
         # Going backwards does not include the anchor (because going forwards
         # includes it)
         results = [0, 1, 2, 3, 4, 5]
-        slice = list(ListRangeFactory(results).getSlice(3, '3', forwards=False))
-        self.assertEqual([2, 1, 0], slice)
+        _slice = list(ListRangeFactory(results).getSlice(3, '3', forwards=False))
+        self.assertEqual([2, 1, 0], _slice)
 
     def test_getSliceByIndex(self):
         # getSliceByIndex returns the slice of the result limited by
         # the indices start, end.
         results = [0, 1, 2, 3, 4, 5]
-        slice = ListRangeFactory(results).getSliceByIndex(2, 5)
-        self.assertEqual([2, 3, 4], slice)
+        _slice = ListRangeFactory(results).getSliceByIndex(2, 5)
+        self.assertEqual([2, 3, 4], _slice)
 
     def test_getSliceByIndex__no_result_set(self):
         # If no result set is present, getSliceByIndex() returns an
         # empty list.
-        slice = ListRangeFactory(None).getSliceByIndex(2, 5)
-        self.assertEqual([], slice)
+        _slice = ListRangeFactory(None).getSliceByIndex(2, 5)
+        self.assertEqual([], _slice)
+
+    def test_picky_collection_ok(self):
+        p = PickyListLikeCollection(range(5))
+        self.assertEqual(range(5)[0:2], p[0:2])
+
+    def test_picky_collection_bad(self):
+        p = PickyListLikeCollection([])
+        # It would be nice to demonstrate p[-10:2] but it cannot be done
+        # directly since assertRaises needs individual parameters.  The
+        # following is the equivalent.
+        self.assertRaises(RuntimeError,
+                          p.__getitem__, slice(-10, 2, None))
+
+    def test_getSlice_empty_result_set_forwards(self):
+        results = PickyListLikeCollection([])
+        _slice = ListRangeFactory(results).getSlice(5, forwards=True)
+        self.assertEqual([], _slice)
+
+    def test_getSlice_empty_result_set_backwards(self):
+        results = PickyListLikeCollection([])
+        _slice = ListRangeFactory(results).getSlice(5, forwards=False)
+        self.assertEqual([], _slice)
+
+    def test_getSliceByIndex_empty_result_set(self):
+        results = PickyListLikeCollection([])
+        self.assertRaises(
+            RuntimeError,
+            ListRangeFactory(results).getSliceByIndex, -1, 1)