launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-962912 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #962912 in Launchpad itself: "StormRangeFactory evaluates DecoratedResultSet slices twice"
  https://bugs.launchpad.net/launchpad/+bug/962912

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-962912/+merge/99252

This branch prevents StormRangeFactory from evaluating a DecoratedResultSet's primary query twice (once for the base values, once for the decorated). I extended DRS to optionally return both the base and the decorated values, and made SRF use that.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-962912/+merge/99252
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-962912 into lp:launchpad.

=== modified file 'lib/lp/services/database/decoratedresultset.py'
--- lib/lp/services/database/decoratedresultset.py	2011-12-18 14:29:19 +0000
+++ lib/lp/services/database/decoratedresultset.py	2012-03-26 04:42:22 +0000
@@ -39,7 +39,7 @@
     delegates(IResultSet, context='result_set')
 
     def __init__(self, result_set, result_decorator=None, pre_iter_hook=None,
-                 slice_info=False):
+                 slice_info=False, return_both=False):
         """
         Wrap `result_set` in a decorator.
 
@@ -55,23 +55,30 @@
         :param slice_info: If True pass information about the slice parameters
             to the result_decorator and pre_iter_hook. any() and similar
             methods will cause None to be supplied.
+        :param return_both: If True return both the plain and decorated
+            values as a tuple.
         """
         self.result_set = result_set
         self.result_decorator = result_decorator
         self.pre_iter_hook = pre_iter_hook
         self.slice_info = slice_info
+        self.return_both = return_both
 
     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
+            decorated = None
         else:
             if self.result_decorator is None:
-                return result
+                decorated = result
             elif self.slice_info:
-                return self.result_decorator(result, row_index)
+                decorated = self.result_decorator(result, row_index)
             else:
-                return self.result_decorator(result)
+                decorated = self.result_decorator(result)
+        if self.return_both:
+            return (result, decorated)
+        else:
+            return decorated
 
     def copy(self, *args, **kwargs):
         """See `IResultSet`.
@@ -81,13 +88,17 @@
         new_result_set = self.result_set.copy(*args, **kwargs)
         return DecoratedResultSet(
             new_result_set, self.result_decorator, self.pre_iter_hook,
-            self.slice_info)
+            self.slice_info, self.return_both)
 
     def config(self, *args, **kwargs):
         """See `IResultSet`.
 
         :return: The decorated result set.after updating the config.
         """
+        return_both = kwargs.pop('return_both', None)
+        if return_both is not None:
+            self.return_both = return_both
+
         self.result_set.config(*args, **kwargs)
         return self
 
@@ -173,7 +184,7 @@
         new_result_set = self.result_set.order_by(*args, **kwargs)
         return DecoratedResultSet(
             new_result_set, self.result_decorator, self.pre_iter_hook,
-            self.slice_info)
+            self.slice_info, self.return_both)
 
     def get_plain_result_set(self):
         """Return the plain Storm result set."""
@@ -195,4 +206,4 @@
             new_result_set = self.result_set.find(*args, **kwargs)
         return DecoratedResultSet(
             new_result_set, self.result_decorator, self.pre_iter_hook,
-            self.slice_info)
+            self.slice_info, self.return_both)

=== modified file 'lib/lp/services/database/doc/decoratedresultset.txt'
--- lib/lp/services/database/doc/decoratedresultset.txt	2011-12-18 17:15:11 +0000
+++ lib/lp/services/database/doc/decoratedresultset.txt	2012-03-26 04:42:22 +0000
@@ -62,6 +62,20 @@
     Dist name is: gentoo
     Dist name is: guadalinex
 
+It's also possible to ask the set to return both the normal and
+decorated results:
+
+    >>> decorated_result_set.config(return_both=True)
+    <lp.services.database.decoratedresultset.DecoratedResultSet object at ...>
+    >>> for dist in decorated_result_set:
+    ...     print dist
+    (<Distribution 'Debian' (debian)>, u'Dist name is: debian')
+    (<Distribution 'Gentoo' (gentoo)>, u'Dist name is: gentoo')
+    ...
+    (<Distribution 'ubuntutest' (ubuntutest)>, u'Dist name is: ubuntutest')
+    >>> decorated_result_set.first()
+    (<Distribution 'Debian' (debian)>, u'Dist name is: debian')
+
 Some methods of the DecoratedResultSet are not actually decorated and
 just work like normally:
 

=== removed file 'lib/lp/services/database/tests/test_decoratedresultset.py'
--- lib/lp/services/database/tests/test_decoratedresultset.py	2011-12-28 17:03:06 +0000
+++ lib/lp/services/database/tests/test_decoratedresultset.py	1970-01-01 00:00:00 +0000
@@ -1,20 +0,0 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-"""Test harness for running the decoratedresultset.txt test.
-
-All the non-documentation-worthy tests for the DecoratedResultSet class.
-"""
-
-__metaclass__ = type
-
-__all__ = []
-
-import unittest
-
-from lp.testing.layers import LaunchpadZopelessLayer
-from lp.testing.systemdocs import LayeredDocFileSuite
-
-
-def test_suite():
-    return LayeredDocFileSuite('decoratedresultset.txt', layer=LaunchpadZopelessLayer)

=== modified file 'lib/lp/services/webapp/batching.py'
--- lib/lp/services/webapp/batching.py	2012-03-21 22:11:31 +0000
+++ lib/lp/services/webapp/batching.py	2012-03-26 04:42:22 +0000
@@ -552,6 +552,17 @@
         result = ProxyFactory(naked_result)
         return result.config(limit=size)
 
+    def _get_shadowed_list(self, result):
+        if zope_isinstance(result, DecoratedResultSet):
+            items = list(result.copy().config(return_both=True))
+            if items:
+                shadow_result, real_result = map(list, zip(*items))
+            else:
+                shadow_result = real_result = []
+        else:
+            shadow_result = real_result = list(result)
+        return ShadowedList(real_result, shadow_result)
+
     def getSlice(self, size, endpoint_memo='', forwards=True):
         """See `IRangeFactory`."""
         if self.empty_resultset:
@@ -568,21 +579,12 @@
             result = self.resultset.config(limit=size)
         else:
             result = self.getSliceFromMemo(size, parsed_memo)
-        real_result = list(result)
-        if zope_isinstance(result, DecoratedResultSet):
-            shadow_result = list(result.get_plain_result_set())
-        else:
-            shadow_result = real_result
-        return ShadowedList(real_result, shadow_result)
+        return self._get_shadowed_list(result)
 
     def getSliceByIndex(self, start, end):
         """See `IRangeFactory."""
         sliced = self.resultset[start:end]
-        if zope_isinstance(sliced, DecoratedResultSet):
-            return ShadowedList(
-                list(sliced), list(sliced.get_plain_result_set()))
-        sliced = list(sliced)
-        return ShadowedList(sliced, sliced)
+        return self._get_shadowed_list(sliced)
 
     @cachedproperty
     def rough_length(self):