launchpad-reviewers team mailing list archive

[Michael Nelson <michael.nelson@xxxxxxxxxxxxx>]

Michael Nelson has proposed merging lp:~michael.nelson/launchpad/remove-bug-217644-workarounds into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #405772 Update to Storm 0.15
  https://bugs.launchpad.net/bugs/405772


Overview
========
This branch removes some XXX's and their respective workarounds for a bug 217644 in storm that was fixed.

Pre-imp
=======

08:59 < lifeless> hi noodles775
08:59 < lifeless> noodles775: I has a question for you
09:00 < lifeless> noodles775: lib/lp/registry/browser/distribution.py
09:00 < lifeless> line 479
09:00 < lifeless> is that still relevant ?
09:07  * noodles775 looks
09:09  * noodles775 pulls a recent version of devel
09:13 < noodles775> lifeless: So the associated bug has been released, and we're using a version of storm that includes it. So I would say no. Remove it, and see (or I can do it if you're done for the day).
10:33 < lifeless> noodles775: I'm well done ;)
10:33 < lifeless> noodles775: I'd love it if you could; I found that code while starting to look at a perf issue
10:33 < noodles775> lifeless: np, I'll do it now.
10:33 < lifeless> thanks!

I've sent this off to ec2 test a few hours ago. There are a few test failures where the tests assume a list rather than an array, which I'll fix once they finish.
-- 
https://code.launchpad.net/~michael.nelson/launchpad/remove-bug-217644-workarounds/+merge/33397
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~michael.nelson/launchpad/remove-bug-217644-workarounds into lp:launchpad/devel.

=== modified file 'lib/canonical/launchpad/components/decoratedresultset.py'
--- lib/canonical/launchpad/components/decoratedresultset.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/components/decoratedresultset.py	2010-08-23 14:55:01 +0000
@@ -9,7 +9,6 @@
     ]
 
 from lazr.delegates import delegates
-from storm.expr import Column
 from storm.zope.interfaces import IResultSet
 from zope.security.proxy import removeSecurityProxy
 
@@ -31,9 +30,6 @@
 
     This behaviour is required for other classes as well (Distribution,
     DistroArchSeries), hence a generalised solution.
-
-    This class also fixes a bug currently in Storm's ResultSet.count
-    method (see below)
     """
     delegates(IResultSet, context='result_set')
 

=== modified file 'lib/lp/archivepublisher/utils.py'
--- lib/lp/archivepublisher/utils.py	2010-08-20 20:31:18 +0000
+++ lib/lp/archivepublisher/utils.py	2010-08-23 14:55:01 +0000
@@ -109,7 +109,8 @@
         end = start + chunk_size
 
         # The reason why we listify the sliced ResultSet is because we
-        # cannot very it's size using 'count' (see bug #217644). However,
+        # cannot very it's size using 'count' (see bug #217644 and note
+        # that it was fixed in storm but not SQLObjectResultSet). However,
         # It's not exactly a problem considering non-empty set will be
         # iterated anyway.
         batch = list(self.input[start:end])

=== modified file 'lib/lp/hardwaredb/model/hwdb.py'
--- lib/lp/hardwaredb/model/hwdb.py	2010-08-20 20:31:18 +0000
+++ lib/lp/hardwaredb/model/hwdb.py	2010-08-23 14:55:01 +0000
@@ -64,9 +64,6 @@
     SQLBase,
     sqlvalues,
     )
-from canonical.launchpad.components.decoratedresultset import (
-    DecoratedResultSet,
-    )
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
 from canonical.launchpad.validators.name import valid_name
@@ -347,13 +344,7 @@
         # DISTINCT clause.
         result_set.config(distinct=True)
         result_set.order_by(HWSubmission.id)
-        # The Storm implementation of ResultSet.count() is incorrect if
-        # the select query uses the distinct directive (see bug #217644).
-        # DecoratedResultSet solves this problem by modifying the query
-        # to count only the records appearing in a subquery.
-        # We don't actually need to transform the results, which is why
-        # the second argument is a no-op.
-        return DecoratedResultSet(result_set, lambda result: result)
+        return result_set
 
     def _submissionsSubmitterSelects(
         self, target_column, bus, vendor_id, product_id, driver_name,

=== modified file 'lib/lp/registry/browser/distribution.py'
--- lib/lp/registry/browser/distribution.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/browser/distribution.py	2010-08-23 14:55:01 +0000
@@ -474,18 +474,7 @@
         """See `AbstractPackageSearchView`."""
 
         if self.search_by_binary_name:
-            non_exact_matches = self.context.searchBinaryPackages(self.text)
-
-            # XXX Michael Nelson 20090605 bug=217644
-            # We are only using a decorated resultset here to conveniently
-            # get around the storm bug whereby count returns the count
-            # of non-distinct results, even though this result set
-            # is configured for distinct results.
-            def dummy_func(result):
-                return result
-            non_exact_matches = DecoratedResultSet(
-                non_exact_matches, dummy_func)
-
+            return self.context.searchBinaryPackages(self.text)
         else:
             non_exact_matches = self.context.searchSourcePackageCaches(
                 self.text)

=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py	2010-08-22 18:31:30 +0000
+++ lib/lp/registry/model/distroseries.py	2010-08-23 14:55:01 +0000
@@ -343,7 +343,7 @@
     @cachedproperty
     def _all_packagings(self):
         """Get an unordered list of all packagings.
-        
+
         :return: A ResultSet which can be decorated or tuned further. Use
             DistroSeries._packaging_row_to_packaging to extract the
             packaging objects out.
@@ -353,7 +353,7 @@
         # Packaging object.
         # NB: precaching objects like this method tries to do has a very poor
         # hit rate with storm - many queries will still be executed; consider
-        # ripping this out and instead allowing explicit inclusion of things 
+        # ripping this out and instead allowing explicit inclusion of things
         # like Person._all_members does - returning a cached object graph.
         # -- RBC 20100810
         # Avoid circular import failures.
@@ -1809,11 +1809,7 @@
             DistroSeries.hide_all_translations == False,
             DistroSeries.id == POTemplate.distroseriesID)
         result_set = result_set.config(distinct=True)
-        # XXX: henninge 2009-02-11 bug=217644: Convert to sequence right here
-        # because ResultSet reports a wrong count() when using DISTINCT. Also
-        # ResultSet does not implement __len__(), which would make it more
-        # like a sequence.
-        return list(result_set)
+        return result_set
 
     def findByName(self, name):
         """See `IDistroSeriesSet`."""

=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py	2010-08-22 03:09:51 +0000
+++ lib/lp/registry/vocabularies.py	2010-08-23 14:55:01 +0000
@@ -95,9 +95,6 @@
     SQLBase,
     sqlvalues,
     )
-from canonical.launchpad.components.decoratedresultset import (
-    DecoratedResultSet,
-    )
 from canonical.launchpad.database.account import Account
 from canonical.launchpad.database.emailaddress import EmailAddress
 from canonical.launchpad.database.stormsugar import StartsWith
@@ -624,12 +621,8 @@
             combined_result = public_result.union(private_result)
             # Eliminate default ordering.
             combined_result.order_by()
-            # XXX: BradCrittenden 2009-04-26 bug=217644: The use of Alias and
-            # is a work-around for .count() not working with the 'distinct'
-            # option.
-            subselect = Alias(combined_result._get_select(), 'Person')
             exact_match = (Person.name == text)
-            result = self.store.using(subselect).find((Person, exact_match))
+            result = combined_result(Person, exact_match)
         # XXX: BradCrittenden 2009-05-07 bug=373228: A bug in Storm prevents
         # setting the 'distinct' and 'limit' options in a single call to
         # .config().  The work-around is to split them up.  Note the limit has
@@ -648,10 +641,7 @@
         else:
             result.order_by(Person.displayname, Person.name)
         result.config(limit=self.LIMIT)
-        # XXX: BradCrittenden 2009-04-24 bug=217644: Wrap the results to
-        # ensure the .count() method works until the Storm bug is fixed and
-        # integrated.
-        return DecoratedResultSet(result)
+        return result
 
     def search(self, text):
         """Return people/teams whose fti or email address match :text:."""
@@ -727,10 +717,7 @@
         result.config(distinct=True)
         result.order_by(Person.displayname, Person.name)
         result.config(limit=self.LIMIT)
-        # XXX: BradCrittenden 2009-04-24 bug=217644: Wrap the results to
-        # ensure the .count() method works until the Storm bug is fixed and
-        # integrated.
-        return DecoratedResultSet(result)
+        return result
 
 
 class ValidPersonVocabulary(ValidPersonOrTeamVocabulary):

=== modified file 'lib/lp/soyuz/doc/package-diff.txt'
--- lib/lp/soyuz/doc/package-diff.txt	2010-05-13 12:04:56 +0000
+++ lib/lp/soyuz/doc/package-diff.txt	2010-08-23 14:55:01 +0000
@@ -451,12 +451,7 @@
     >>> packagediff_set.getPendingDiffs().count()
     7
 
-XXX cprov 20070530: storm doesn't go well with  limited count()s
-See bug #217644. For now we have to listify the results and used
-the list length.
-
-    >>> r = packagediff_set.getPendingDiffs(limit=2)
-    >>> len(list(r))
+    >>> packagediff_set.getPendingDiffs(limit=2).count()
     2
 
 All package diffs targeting a set of source package releases can also