launchpad-reviewers team mailing list archive

[Abel Deuring <abel.deuring@xxxxxxxxxxxxx>]

Abel Deuring has proposed merging lp:~adeuring/launchpad/bug-739052-7 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~adeuring/launchpad/bug-739052-7/+merge/72917

Trying to use StormRangeFactory on a view that times out, I noticed three problems.

The first occurs in lazr.batchnavigator; version 1.2.9 fixes it. (See https://code.launchpad.net/~adeuring/lazr.batchnavigator/slicing-error-for-too-short-last-backwards-batch/+merge/72745 for details)

So, this LP branch changes version.cfg to require this version.

The second problem: A batch of a resultset is specified by the batch size, a memo value and a flag "forwards", where the memo value indicates where the batch starts (forward batch) or where it ends (backward batch). Such a batch is retrieved by calling IRangeFactory.getSlice().

When a call getSlice(size, memo="something", forwards=False) returns less elements than requested, this means that the start of the result set has been reached. In this case, lazr.batchnavigator.z3batching.batch._Batch.sliced_list() issues another call

   getSlice(number_of_missing_elements, "something", forwards=True)

so that a view can display the expected number of elements.

This second call failed in StormRangeFactory: The first call of getSlice() reverted (correctly) the sort order -- but the second call did not revert the sort order again: The method getSlice() simply assumed that the result set was in the "regular sort state", not in the reverted...

The fix is simple: The method should simply not make any assumptions about how the resultset is currently ordered. The StromRangeFactory.__init__() now stores the regular and reverted sort order in two attributes.

The test of this fix uncovered the third problem: ShadowedList.reverse() did not work when the instance was created with identical lists as the the parameters values and shadow_values. I chose a simple fix: ShadowedList.__init__() always uses a copy of shadow_values.

test: ./bin/test canonical -vvt canonical.launchpad.webapp.tests.test_batching

no lint
-- 
https://code.launchpad.net/~adeuring/launchpad/bug-739052-7/+merge/72917
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/bug-739052-7 into lp:launchpad.

=== modified file 'lib/canonical/launchpad/webapp/batching.py'
--- lib/canonical/launchpad/webapp/batching.py	2011-08-17 15:41:55 +0000
+++ lib/canonical/launchpad/webapp/batching.py	2011-08-25 15:44:35 +0000
@@ -195,7 +195,10 @@
             raise ValueError(
                 "values and shadow_values must have the same length.")
         self.values = values
-        self.shadow_values = shadow_values
+        # Store always a copy: values and shadow_values may be identical,
+        # and if this is the case, the two reverse() calls in the method
+        # reverse() below will cancel each other.
+        self.shadow_values = shadow_values[:]
 
     def __len__(self):
         """See `list`."""
@@ -271,6 +274,11 @@
         else:
             self.plain_resultset = resultset
         self.error_cb = error_cb
+        self.forward_sort_order = self.getOrderBy()
+        if self.forward_sort_order is Undef:
+            raise StormRangeFactoryError(
+                'StormRangeFactory requires a sorted result set.')
+        self.backward_sort_order = self.reverseSortOrder()
 
     def getOrderBy(self):
         """Return the order_by expressions of the result set."""
@@ -283,9 +291,6 @@
         if not zope_isinstance(row, tuple):
             row = (row, )
         sort_expressions = self.getOrderBy()
-        if sort_expressions is Undef:
-            raise StormRangeFactoryError(
-                'StormRangeFactory requires a sorted result set.')
         for expression in sort_expressions:
             if zope_isinstance(expression, Desc):
                 expression = expression.expr
@@ -531,8 +536,11 @@
 
     def getSlice(self, size, endpoint_memo='', forwards=True):
         """See `IRangeFactory`."""
-        if not forwards:
-            self.resultset.order_by(*self.reverseSortOrder())
+        if forwards:
+            self.resultset.order_by(*self.forward_sort_order)
+        else:
+            self.resultset.order_by(*self.backward_sort_order)
+
         parsed_memo = self.parseMemo(endpoint_memo)
         # Note that lazr.batchnavigator calls len(slice), so we can't
         # return the plain result set.

=== modified file 'lib/canonical/launchpad/webapp/tests/test_batching.py'
--- lib/canonical/launchpad/webapp/tests/test_batching.py	2011-08-17 15:41:55 +0000
+++ lib/canonical/launchpad/webapp/tests/test_batching.py	2011-08-25 15:44:35 +0000
@@ -66,6 +66,17 @@
         range_factory = StormRangeFactory(resultset)
         self.assertTrue(verifyObject(IRangeFactory, range_factory))
 
+    def test_StormRangeFactory_needs__ordered_result_set(self):
+        # If a result set is not ordered, it cannot be used with a
+        # StormRangeFactory.
+        resultset = self.makeStormResultSet()
+        resultset.order_by()
+        exception = self.assertRaises(
+            StormRangeFactoryError, StormRangeFactory, resultset)
+        self.assertEqual(
+            "StormRangeFactory requires a sorted result set.",
+            str(exception))
+
     def test_getOrderValuesFor__one_sort_column(self):
         # StormRangeFactory.getOrderValuesFor() returns the values
         # of the fields used in order_by expresssions for a given
@@ -110,19 +121,6 @@
                 'StormRangeFactory only supports sorting by PropertyColumn, '
                 'not by <storm.expr.SQL object at'))
 
-    def test_getOrderValuesFor__unordered_result_set(self):
-        # If a result set is not ordered, it cannot be used with a
-        # StormRangeFactory.
-        resultset = self.makeStormResultSet()
-        resultset.order_by()
-        range_factory = StormRangeFactory(resultset)
-        exception = self.assertRaises(
-            StormRangeFactoryError, range_factory.getOrderValuesFor,
-            resultset[0])
-        self.assertEqual(
-            "StormRangeFactory requires a sorted result set.",
-            str(exception))
-
     def test_getOrderValuesFor__decorated_result_set(self):
         # getOrderValuesFor() knows how to retrieve SQL sort values
         # from DecoratedResultSets.
@@ -389,7 +387,8 @@
         # ORDER BY expressions which either are all instances of
         # PropertyColumn, or are all instances of Desc(PropertyColumn).
         # memos are the related limit values.
-        range_factory = StormRangeFactory(None, self.logError)
+        resultset = self.makeStormResultSet()
+        range_factory = StormRangeFactory(resultset, self.logError)
         order_by = [
             Person.id, Person.datecreated, Person.name, Person.displayname]
         limits = [1, datetime(2011, 07, 25, 0, 0, 0), 'foo', 'bar']
@@ -410,7 +409,8 @@
     def test_lessThanOrGreaterThanExpression__asc(self):
         # beforeOrAfterExpression() returns an expression
         # (col1, col2,..) > (memo1, memo2...) for ascending sort order.
-        range_factory = StormRangeFactory(None, self.logError)
+        resultset = self.makeStormResultSet()
+        range_factory = StormRangeFactory(resultset, self.logError)
         expressions = [Person.id, Person.name]
         limits = [1, 'foo']
         limit_expression = range_factory.lessThanOrGreaterThanExpression(
@@ -422,7 +422,8 @@
     def test_lessThanOrGreaterThanExpression__desc(self):
         # beforeOrAfterExpression() returns an expression
         # (col1, col2,..) < (memo1, memo2...) for descending sort order.
-        range_factory = StormRangeFactory(None, self.logError)
+        resultset = self.makeStormResultSet()
+        range_factory = StormRangeFactory(resultset, self.logError)
         expressions = [Desc(Person.id), Desc(Person.name)]
         limits = [1, 'foo']
         limit_expression = range_factory.lessThanOrGreaterThanExpression(
@@ -432,7 +433,8 @@
             compile(limit_expression))
 
     def test_equalsExpressionsFromLimits(self):
-        range_factory = StormRangeFactory(None, self.logError)
+        resultset = self.makeStormResultSet()
+        range_factory = StormRangeFactory(resultset, self.logError)
         order_by = [
             Person.id, Person.datecreated, Desc(Person.name),
             Desc(Person.displayname)]
@@ -609,6 +611,21 @@
         sliced_result = range_factory.getSlice(3)
         self.assertIsInstance(sliced_result, ShadowedList)
 
+    def test_getSlice__backwards_then_forwards(self):
+        # A slice can be retrieved in both directions from one factory.
+        resultset = self.makeStormResultSet()
+        resultset.order_by(Person.id)
+        all_results = list(resultset)
+        memo = simplejson.dumps([all_results[2].id])
+        range_factory = StormRangeFactory(resultset)
+        backward_slice = range_factory.getSlice(
+            size=2, endpoint_memo=memo, forwards=False)
+        backward_slice.reverse()
+        self.assertEqual(all_results[:2], list(backward_slice))
+        forward_slice = range_factory.getSlice(
+            size=2, endpoint_memo=memo, forwards=True)
+        self.assertEqual(all_results[3:5], list(forward_slice))
+
     def makeStringSequence(self, sequence):
         return [str(elem) for elem in sequence]
 
@@ -687,3 +704,14 @@
         self.assertEqual(last1, shadow_list[0])
         self.assertEqual(first2, shadow_list.shadow_values[-1])
         self.assertEqual(last2, shadow_list.shadow_values[0])
+
+    def test_ShadowedList__reverse__values_and_shadow_values_identical(self):
+        # ShadowList.reverse() works also when passed the same
+        # sequence as values and as shadow_values.
+        list_ = range(3)
+        shadow_list = ShadowedList(list_, list_)
+        shadow_list.reverse()
+        self.assertEqual(0, shadow_list[-1])
+        self.assertEqual(2, shadow_list[0])
+        self.assertEqual(0, shadow_list.shadow_values[-1])
+        self.assertEqual(2, shadow_list.shadow_values[0])

=== modified file 'versions.cfg'
--- versions.cfg	2011-08-22 18:25:06 +0000
+++ versions.cfg	2011-08-25 15:44:35 +0000
@@ -30,7 +30,7 @@
 launchpadlib = 1.9.9
 lazr.amqp = 0.1
 lazr.authentication = 0.1.1
-lazr.batchnavigator = 1.2.8
+lazr.batchnavigator = 1.2.9
 lazr.config = 1.1.3
 lazr.delegates = 1.2.0
 lazr.enum = 1.1.3