Merge lp:~wgrant/launchpad/bug-962912 into lp:launchpad

[William Grant]

Fix StormRangeFactory to not evaluate DecoratedResultSets twice.

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.

[Ian Booth (wallyworld)]

Yay, less queries. Awesome.

review: Approve

=== modified file 'lib/lp/registry/browser/tests/test_pillar_sharing.py'
--- lib/lp/registry/browser/tests/test_pillar_sharing.py	2012-03-22 23:21:24 +0000
+++ lib/lp/registry/browser/tests/test_pillar_sharing.py	2012-03-27 04:04:19 +0000
@@ -247,7 +247,7 @@
             view = create_view(self.pillar, name='+sharing')
             with StormStatementRecorder() as recorder:
                 view.initialize()
-            self.assertThat(recorder, HasQueryCount(LessThan(7)))
+            self.assertThat(recorder, HasQueryCount(LessThan(6)))
 
     def test_view_write_enabled_without_feature_flag(self):
         # Test that sharing_write_enabled is not set without the feature flag.

=== 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-27 04:04:19 +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,50 @@
         :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.config(return_both=return_both)
+
+    def _extract_plain_and_result(self, results):
+        """Extract the plain and normal results from a sub-result.
+
+        This gets slightly complicated when there are nested
+        DecoratedResultSets, as we have to propogate the plain result
+        all the way up.
+        """
+        if not results:
+            return [], []
+        elif (zope_isinstance(self.result_set, DecoratedResultSet)
+              and self.return_both):
+            assert self.result_set.return_both == self.return_both
+            return zip(*results)
+        else:
+            return results, results
 
     def decorate_or_none(self, result, row_index=None):
         """Decorate a result or return None if the result is itself None"""
+        # If we have a nested DecoratedResultSet we need to propogate
+        # the plain result.
+        ([plain], [result]) = self._extract_plain_and_result([result])
+
         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 (plain, decorated)
+        else:
+            return decorated
 
     def copy(self, *args, **kwargs):
         """See `IResultSet`.
@@ -81,13 +108,19 @@
         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
+            if zope_isinstance(self.result_set, DecoratedResultSet):
+                self.result_set.config(return_both=return_both)
+
         self.result_set.config(*args, **kwargs)
         return self
 
@@ -106,10 +139,11 @@
             stop = start + len(results)
             result_slice = slice(start, stop)
         if self.pre_iter_hook is not None:
+            pre_iter_rows = self._extract_plain_and_result(results)[1]
             if self.slice_info:
-                self.pre_iter_hook(results, result_slice)
+                self.pre_iter_hook(pre_iter_rows, result_slice)
             else:
-                self.pre_iter_hook(results)
+                self.pre_iter_hook(pre_iter_rows)
         if self.slice_info:
             start = result_slice.start
             for offset, value in enumerate(results):
@@ -173,7 +207,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 +229,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-27 04:04:19 +0000
@@ -62,6 +62,31 @@
     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')
+
+This works even if there are multiple levels:
+
+    >>> drs_squared = DecoratedResultSet(
+    ...     decorated_result_set, lambda x: len(x)).config(return_both=True)
+    >>> for dist in drs_squared:
+    ...     print dist
+    (<Distribution 'Debian' (debian)>, 20)
+    (<Distribution 'Gentoo' (gentoo)>, 20)
+    ...
+    (<Distribution 'ubuntutest' (ubuntutest)>, 24)
+
 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-27 04:04:19 +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):

=== modified file 'lib/lp/soyuz/browser/builder.py'
--- lib/lp/soyuz/browser/builder.py	2012-01-04 16:39:15 +0000
+++ lib/lp/soyuz/browser/builder.py	2012-03-27 04:04:19 +0000
@@ -42,7 +42,6 @@
     IBuilderSet,
     )
 from lp.buildmaster.model.buildqueue import BuildQueue
-from lp.services.database.decoratedresultset import DecoratedResultSet
 from lp.services.database.lpstorm import IStore
 from lp.services.propertycache import (
     cachedproperty,
@@ -150,22 +149,22 @@
     @cachedproperty
     def builders(self):
         """All active builders"""
-        def do_eager_load(builders):
-            # Populate builders' currentjob cachedproperty.
-            queues = IStore(BuildQueue).find(
-                BuildQueue,
-                BuildQueue.builderID.is_in(
-                    builder.id for builder in builders))
-            queue_builders = dict(
-                (queue.builderID, queue) for queue in queues)
-            for builder in builders:
-                cache = get_property_cache(builder)
-                cache.currentjob = queue_builders.get(builder.id, None)
-            # Prefetch the jobs' data.
-            BuildQueue.preloadSpecificJobData(queues)
-
-        return list(DecoratedResultSet(
-            list(self.context.getBuilders()), pre_iter_hook=do_eager_load))
+        builders = list(self.context.getBuilders())
+
+        # Populate builders' currentjob cachedproperty.
+        queues = IStore(BuildQueue).find(
+            BuildQueue,
+            BuildQueue.builderID.is_in(
+                builder.id for builder in builders))
+        queue_builders = dict(
+            (queue.builderID, queue) for queue in queues)
+        for builder in builders:
+            cache = get_property_cache(builder)
+            cache.currentjob = queue_builders.get(builder.id, None)
+        # Prefetch the jobs' data.
+        BuildQueue.preloadSpecificJobData(queues)
+
+        return builders
 
     @property
     def number_of_registered_builders(self):