launchpad-reviewers team mailing list archive

[Abel Deuring <abel.deuring@xxxxxxxxxxxxx>]

Abel Deuring has proposed merging lp:~adeuring/launchpad/bug-739052-10 into lp:launchpad with lp:~adeuring/launchpad/bug-739052-9 as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

This branch fixes a serious problem of StormRangeFactory (hopefully the last one...): It did not work with empty result sets.

We have two "flavours" of empty result sets: Regular Storm ResultSet instances which have no rows, and instances of EmptyResultSet. (The latter can be returned by model code when it is obvious that a query would not return any rows, to avoid a DB query.)

Both variants have of course the "problem" that expressions like resultset[0] raise an IndexError; moreover, EmptyResultSet has no sort order that is usable by StormRangeFactory. This branch makes StormRangeFactory usable for these empty result sets.

test: ./bin/test canonical -vvt test_batching

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

=== modified file 'lib/canonical/launchpad/webapp/batching.py'
--- lib/canonical/launchpad/webapp/batching.py	2011-09-22 10:12:25 +0000
+++ lib/canonical/launchpad/webapp/batching.py	2011-09-22 10:12:26 +0000
@@ -23,6 +23,7 @@
     SQL,
     )
 from storm.properties import PropertyColumn
+from storm.store import EmptyResultSet
 from storm.zope.interfaces import IResultSet
 from zope.component import (
     adapts,
@@ -294,11 +295,16 @@
         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()
+        if not self.empty_resultset:
+            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()
+
+    @property
+    def empty_resultset(self):
+        return zope_isinstance(self.plain_resultset, EmptyResultSet)
 
     def getOrderBy(self):
         """Return the order_by expressions of the result set."""
@@ -334,6 +340,8 @@
     def getEndpointMemos(self, batch):
         """See `IRangeFactory`."""
         plain_slice = batch.sliced_list.shadow_values
+        if len(plain_slice) == 0:
+            return ('', '')
         lower = self.getOrderValuesFor(plain_slice[0])
         upper = self.getOrderValuesFor(plain_slice[batch.trueSize - 1])
         return (
@@ -549,6 +557,8 @@
 
     def getSlice(self, size, endpoint_memo='', forwards=True):
         """See `IRangeFactory`."""
+        if self.empty_resultset:
+            return ShadowedList([], [])
         if forwards:
             self.resultset.order_by(*self.forward_sort_order)
         else:
@@ -585,6 +595,8 @@
         # in the result set, so let's use them. Moreover, for SELECT
         # DISTINCT queries, each column used for sorting must appear
         # in the result.
+        if self.empty_resultset:
+            return 0
         columns = [plain_expression(column) for column in self.getOrderBy()]
         select = removeSecurityProxy(self.plain_resultset).get_select_expr(
             *columns)

=== modified file 'lib/canonical/launchpad/webapp/tests/test_batching.py'
--- lib/canonical/launchpad/webapp/tests/test_batching.py	2011-09-22 10:12:25 +0000
+++ lib/canonical/launchpad/webapp/tests/test_batching.py	2011-09-22 10:12:26 +0000
@@ -12,6 +12,7 @@
     compile,
     Desc,
     )
+from storm.store import EmptyResultSet
 from testtools.matchers import (
     LessThan,
     Not,
@@ -785,3 +786,45 @@
         self.assertIsInstance(sliced, ShadowedList)
         self.assertEqual(all_results[2:4], list(sliced))
         self.assertEqual(all_undecorated_results[2:4], sliced.shadow_values)
+
+    def test_StormRangeFactory__EmptyResultSet(self):
+        # It is possible to create StormRangeFactory instances for
+        # EmptyResultSets,
+        resultset = EmptyResultSet()
+        range_factory = StormRangeFactory(resultset)
+        # rough_length is always zero.
+        self.assertEqual(0, range_factory.rough_length)
+        # getSlice() and getSliceByIndex() return empty ShadowedLists.
+        sliced = range_factory.getSlice(1)
+        self.assertEqual([], sliced.values)
+        self.assertEqual([], sliced.shadow_values)
+        sliced = range_factory.getSliceByIndex(0, 1)
+        self.assertEqual([], sliced.values)
+        self.assertEqual([], sliced.shadow_values)
+        # The endpoint memos are empty strings.
+        request = LaunchpadTestRequest()
+        batchnav = BatchNavigator(
+            resultset, request, size=3, range_factory=range_factory)
+        first, last = range_factory.getEndpointMemos(batchnav.batch)
+        self.assertEqual('', first)
+        self.assertEqual('', last)
+
+    def test_StormRangeFactory__empty_real_resultset(self):
+        # StormRangeFactory works with empty regular result sets,
+        resultset = self.factory.makeProduct().open_bugtasks
+        self.assertEqual(0, resultset.count())
+        range_factory = StormRangeFactory(resultset)
+        # getSlice() and getSliceByIndex() return empty ShadowedLists.
+        sliced = range_factory.getSlice(1)
+        self.assertEqual([], sliced.values)
+        self.assertEqual([], sliced.shadow_values)
+        sliced = range_factory.getSliceByIndex(0, 1)
+        self.assertEqual([], sliced.values)
+        self.assertEqual([], sliced.shadow_values)
+        # The endpoint memos are empty strings.
+        request = LaunchpadTestRequest()
+        batchnav = BatchNavigator(
+            resultset, request, size=3, range_factory=range_factory)
+        first, last = range_factory.getEndpointMemos(batchnav.batch)
+        self.assertEqual('', first)
+        self.assertEqual('', last)