launchpad-reviewers team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

Robert Collins has proposed merging lp:~lifeless/launchpad/decoratedresultset into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)


Make it possible to get slice data out of a DecoratedResultSet. This is useful for 'Bug.indexed_messages'.
-- 
https://code.launchpad.net/~lifeless/launchpad/decoratedresultset/+merge/35507
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/decoratedresultset into lp:launchpad/devel.

=== modified file 'lib/canonical/launchpad/components/decoratedresultset.py'
--- lib/canonical/launchpad/components/decoratedresultset.py	2010-08-25 19:23:13 +0000
+++ lib/canonical/launchpad/components/decoratedresultset.py	2010-09-15 09:29:48 +0000
@@ -9,6 +9,7 @@
     ]
 
 from lazr.delegates import delegates
+from storm import Undef
 from storm.zope.interfaces import IResultSet
 from zope.security.proxy import removeSecurityProxy
 
@@ -33,25 +34,32 @@
     """
     delegates(IResultSet, context='result_set')
 
-    def __init__(self, result_set, result_decorator=None, pre_iter_hook=None):
+    def __init__(self, result_set, result_decorator=None, pre_iter_hook=None,
+        slice_info=False):
         """
         :param result_set: The original result set to be decorated.
         :param result_decorator: The method with which individual results
             will be passed through before being returned.
         :param pre_iter_hook: The method to be called (with the 'result_set')
             immediately before iteration starts.
+        :param slice_info: If True pass information about the slice parameters
+            to the result_decoratora and pre_iter_hook. any() and similar
+            methods will cause None to be supplied.
         """
         self.result_set = result_set
         self.result_decorator = result_decorator
         self.pre_iter_hook = pre_iter_hook
+        self.slice_info = slice_info
 
-    def decorate_or_none(self, result):
+    def decorate_or_none(self, result, row_index=None):
         """Decorate a result or return None if the result is itself None"""
         if result is None:
             return None
         else:
             if self.result_decorator is None:
                 return result
+            elif self.slice_info:
+                return self.result_decorator(result, row_index)
             else:
                 return self.result_decorator(result)
 
@@ -62,7 +70,8 @@
         """
         new_result_set = self.result_set.copy(*args, **kwargs)
         return DecoratedResultSet(
-            new_result_set, self.result_decorator, self.pre_iter_hook)
+            new_result_set, self.result_decorator, self.pre_iter_hook,
+            self.slice_info)
 
     def config(self, *args, **kwargs):
         """See `IResultSet`.
@@ -79,10 +88,25 @@
         """
         # Execute/evaluate the result set query.
         results = list(self.result_set.__iter__(*args, **kwargs))
+        if self.slice_info:
+            # Calculate slice data
+            start = self.result_set._offset
+            if start is Undef:
+                start = 0
+            stop = start + len(results)
+            slice_info = slice(start, stop)
         if self.pre_iter_hook is not None:
-            self.pre_iter_hook(results)
-        for value in results:
-            yield self.decorate_or_none(value)
+            if self.slice_info:
+                self.pre_iter_hook(results, slice_info)
+            else:
+                self.pre_iter_hook(results)
+        if self.slice_info:
+            start = slice_info.start
+            for offset, value in enumerate(results):
+                yield self.decorate_or_none(value, offset + start)
+        else:
+            for value in results:
+                yield self.decorate_or_none(value)
 
     def __getitem__(self, *args, **kwargs):
         """See `IResultSet`.
@@ -94,7 +118,8 @@
         naked_value = removeSecurityProxy(value)
         if IResultSet.providedBy(naked_value):
             return DecoratedResultSet(
-                value, self.result_decorator, self.pre_iter_hook)
+                value, self.result_decorator, self.pre_iter_hook,
+                self.slice_info)
         else:
             return self.decorate_or_none(value)
 
@@ -137,4 +162,5 @@
         """
         new_result_set = self.result_set.order_by(*args, **kwargs)
         return DecoratedResultSet(
-            new_result_set, self.result_decorator, self.pre_iter_hook)
+            new_result_set, self.result_decorator, self.pre_iter_hook,
+            self.slice_info)

=== modified file 'lib/canonical/launchpad/doc/decoratedresultset.txt'
--- lib/canonical/launchpad/doc/decoratedresultset.txt	2009-07-23 17:51:28 +0000
+++ lib/canonical/launchpad/doc/decoratedresultset.txt	2010-09-15 09:29:48 +0000
@@ -1,30 +1,17 @@
 = DecoratedResultSet =
 
-Within Soyuz (and possibly other areas of Launchpad?) there are a 
-number of content classes which do not actually correspond directly to
-a database table. For example, a `DistroSeriesBinaryPackage` is really
-just a representation of a `BinaryPackageName` within a certain
-DistroSeries. Nonetheless, this representation is presented (via views)
-to users as a useful reference point for obtaining particular package
-releases and/or architectures.
-
-A problem arises however when attempting to present a search result of
-such content. For example, when a user searches for all packages within
-the Ubuntu Intrepid `DistroSeries` that include the letter 'l', it is 
-actually the `DistroSeriesPackageCache` that is searched, but the
-results need to be presented back to the browser as a set of
-`DistroSeriesBinaryPackage`s. In the past this was achieved by using a
-list comprehension on the complete result set to convert each result
-into the expected DSBP. This in-memory list was then passed to the view.
-But given that views usually paginate results, they are only interested
-in 20 or so items at a time, so there is a huge waste of resources (DB
-and memory primarily, but some CPU).
-
-The purpose of the `DecoratedResultSet` is to allow such content
-classes to pass an un-evaluated result set to the view so that the view
-can add limits to the query *before* it is evaluated (batching), while
-still ensuring that when the query is evaluated the result is converted
-into the expected content class.
+Within Launchpad we often want to return related data for a ResultSet
+but not have every call site know how that data is structured - they
+should not need to know how to go about loading related Persons for
+a BugMessage, or how to calculate whether a Person is valid. Nor do
+we want to return a less capable object - that prohibits late slicing
+and sorting.
+
+DecoratedResultSet permits some preprocessing of Storm ResultSet
+objects at the time the query executes. This can be used to present
+content classes which are not backed directly in the database, to
+eager load multiple related tables and present just one in the result,
+and so on.
 
 First, we'll create the un-decorated result set of all distributions:
 
@@ -38,7 +25,8 @@
 == Creating the decorator method ==
 
 We create a decorator method that we want to be applied to any
-results obtained from our undecorated result set:
+results obtained from our undecorated result set. For instance,
+we can turn a model object into a string:
 
     >>> def result_decorator(distribution):
     ...     return "Dist name is: %s" % distribution.name
@@ -150,3 +138,47 @@
     > original value : 4
      decorated result: 8
 
+
+== Calculating row numbers ==
+
+DecoratedResultSet can inform its hooks about slice data if slice_info=True is
+passed.
+
+    >>> def pre_iter(rows, slice_info):
+    ...     print "pre iter", len(rows), slice_info.start, slice_info.stop
+    >>> def decorate(row, row_index):
+    ...     print "row", row.id, row_index
+    >>> _ = result_set.order_by(Distribution.id)
+    >>> drs = DecoratedResultSet(
+    ...     result_set, decorate, pre_iter, slice_info=True)
+
+We need enough rows to play with:
+
+    >>> drs.count()
+    7
+
+    >>> _ = list(drs[1:3])
+    pre iter 2 1 3
+    row 2 1
+    row 3 2
+
+Half open slicing is supported too:
+
+    >>> _ = list(drs[:3])
+    pre iter 3 0 3
+    row 1 0
+    row 2 1
+    row 3 2
+
+    >>> _ = list(drs[2:])
+    pre iter 5 2 7
+    row 3 2
+    row 4 3
+    row 5 4
+    row 7 5
+    row 8 6
+
+And of course empty slices:
+
+    >>> _ = list(drs[3:3])
+    pre iter 0 3 3