Launchpad itself

Merge lp:~michael.nelson/launchpad/remove-bug-217644-workarounds into lp:launchpad

  1. remove-bug-217644-workarounds
  2. Merge into devel
Proposed by Michael Nelson
Status: Merged
Approved by: Aaron Bentley
Approved revision: no longer in the source branch.
Merged at revision: 11421
Proposed branch: lp:~michael.nelson/launchpad/remove-bug-217644-workarounds
Merge into: lp:launchpad
Diff against target: 179 lines (+10/-51)
7 files modified
lib/canonical/launchpad/components/decoratedresultset.py (+0/-4)
lib/lp/archivepublisher/utils.py (+2/-1)
lib/lp/hardwaredb/model/hwdb.py (+1/-10)
lib/lp/registry/browser/distribution.py (+1/-12)
lib/lp/registry/model/distroseries.py (+3/-7)
lib/lp/registry/vocabularies.py (+2/-11)
lib/lp/soyuz/doc/package-diff.txt (+1/-6)
To merge this branch: bzr merge lp:~michael.nelson/launchpad/remove-bug-217644-workarounds
Reviewer Review Type Date Requested Status
Aaron Bentley (community) Approve
Review via email: mp+33397@code.launchpad.net

Commit message

Remove workarounds for fixed storm bug 217644.

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Aaron Bentley (abentley) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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-24 07:53:46 +0000
@@ -9,7 +9,6 @@
9 ]9 ]
1010
11from lazr.delegates import delegates11from lazr.delegates import delegates
12from storm.expr import Column
13from storm.zope.interfaces import IResultSet12from storm.zope.interfaces import IResultSet
14from zope.security.proxy import removeSecurityProxy13from zope.security.proxy import removeSecurityProxy
1514
@@ -31,9 +30,6 @@
3130
32 This behaviour is required for other classes as well (Distribution,31 This behaviour is required for other classes as well (Distribution,
33 DistroArchSeries), hence a generalised solution.32 DistroArchSeries), hence a generalised solution.
34
35 This class also fixes a bug currently in Storm's ResultSet.count
36 method (see below)
37 """33 """
38 delegates(IResultSet, context='result_set')34 delegates(IResultSet, context='result_set')
3935
4036
=== 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-24 07:53:46 +0000
@@ -109,7 +109,8 @@
109 end = start + chunk_size109 end = start + chunk_size
110110
111 # The reason why we listify the sliced ResultSet is because we111 # The reason why we listify the sliced ResultSet is because we
112 # cannot very it's size using 'count' (see bug #217644). However,112 # cannot very it's size using 'count' (see bug #217644 and note
113 # that it was fixed in storm but not SQLObjectResultSet). However,
113 # It's not exactly a problem considering non-empty set will be114 # It's not exactly a problem considering non-empty set will be
114 # iterated anyway.115 # iterated anyway.
115 batch = list(self.input[start:end])116 batch = list(self.input[start:end])
116117
=== 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-24 07:53:46 +0000
@@ -64,9 +64,6 @@
64 SQLBase,64 SQLBase,
65 sqlvalues,65 sqlvalues,
66 )66 )
67from canonical.launchpad.components.decoratedresultset import (
68 DecoratedResultSet,
69 )
70from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities67from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
71from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet68from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
72from canonical.launchpad.validators.name import valid_name69from canonical.launchpad.validators.name import valid_name
@@ -347,13 +344,7 @@
347 # DISTINCT clause.344 # DISTINCT clause.
348 result_set.config(distinct=True)345 result_set.config(distinct=True)
349 result_set.order_by(HWSubmission.id)346 result_set.order_by(HWSubmission.id)
350 # The Storm implementation of ResultSet.count() is incorrect if347 return result_set
351 # the select query uses the distinct directive (see bug #217644).
352 # DecoratedResultSet solves this problem by modifying the query
353 # to count only the records appearing in a subquery.
354 # We don't actually need to transform the results, which is why
355 # the second argument is a no-op.
356 return DecoratedResultSet(result_set, lambda result: result)
357348
358 def _submissionsSubmitterSelects(349 def _submissionsSubmitterSelects(
359 self, target_column, bus, vendor_id, product_id, driver_name,350 self, target_column, bus, vendor_id, product_id, driver_name,
360351
=== 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-24 07:53:46 +0000
@@ -474,18 +474,7 @@
474 """See `AbstractPackageSearchView`."""474 """See `AbstractPackageSearchView`."""
475475
476 if self.search_by_binary_name:476 if self.search_by_binary_name:
477 non_exact_matches = self.context.searchBinaryPackages(self.text)477 return self.context.searchBinaryPackages(self.text)
478
479 # XXX Michael Nelson 20090605 bug=217644
480 # We are only using a decorated resultset here to conveniently
481 # get around the storm bug whereby count returns the count
482 # of non-distinct results, even though this result set
483 # is configured for distinct results.
484 def dummy_func(result):
485 return result
486 non_exact_matches = DecoratedResultSet(
487 non_exact_matches, dummy_func)
488
489 else:478 else:
490 non_exact_matches = self.context.searchSourcePackageCaches(479 non_exact_matches = self.context.searchSourcePackageCaches(
491 self.text)480 self.text)
492481
=== 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-24 07:53:46 +0000
@@ -343,7 +343,7 @@
343 @cachedproperty343 @cachedproperty
344 def _all_packagings(self):344 def _all_packagings(self):
345 """Get an unordered list of all packagings.345 """Get an unordered list of all packagings.
346 346
347 :return: A ResultSet which can be decorated or tuned further. Use347 :return: A ResultSet which can be decorated or tuned further. Use
348 DistroSeries._packaging_row_to_packaging to extract the348 DistroSeries._packaging_row_to_packaging to extract the
349 packaging objects out.349 packaging objects out.
@@ -353,7 +353,7 @@
353 # Packaging object.353 # Packaging object.
354 # NB: precaching objects like this method tries to do has a very poor354 # NB: precaching objects like this method tries to do has a very poor
355 # hit rate with storm - many queries will still be executed; consider355 # hit rate with storm - many queries will still be executed; consider
356 # ripping this out and instead allowing explicit inclusion of things 356 # ripping this out and instead allowing explicit inclusion of things
357 # like Person._all_members does - returning a cached object graph.357 # like Person._all_members does - returning a cached object graph.
358 # -- RBC 20100810358 # -- RBC 20100810
359 # Avoid circular import failures.359 # Avoid circular import failures.
@@ -1809,11 +1809,7 @@
1809 DistroSeries.hide_all_translations == False,1809 DistroSeries.hide_all_translations == False,
1810 DistroSeries.id == POTemplate.distroseriesID)1810 DistroSeries.id == POTemplate.distroseriesID)
1811 result_set = result_set.config(distinct=True)1811 result_set = result_set.config(distinct=True)
1812 # XXX: henninge 2009-02-11 bug=217644: Convert to sequence right here1812 return result_set
1813 # because ResultSet reports a wrong count() when using DISTINCT. Also
1814 # ResultSet does not implement __len__(), which would make it more
1815 # like a sequence.
1816 return list(result_set)
18171813
1818 def findByName(self, name):1814 def findByName(self, name):
1819 """See `IDistroSeriesSet`."""1815 """See `IDistroSeriesSet`."""
18201816
=== 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-24 07:53:46 +0000
@@ -95,9 +95,6 @@
95 SQLBase,95 SQLBase,
96 sqlvalues,96 sqlvalues,
97 )97 )
98from canonical.launchpad.components.decoratedresultset import (
99 DecoratedResultSet,
100 )
101from canonical.launchpad.database.account import Account98from canonical.launchpad.database.account import Account
102from canonical.launchpad.database.emailaddress import EmailAddress99from canonical.launchpad.database.emailaddress import EmailAddress
103from canonical.launchpad.database.stormsugar import StartsWith100from canonical.launchpad.database.stormsugar import StartsWith
@@ -648,10 +645,7 @@
648 else:645 else:
649 result.order_by(Person.displayname, Person.name)646 result.order_by(Person.displayname, Person.name)
650 result.config(limit=self.LIMIT)647 result.config(limit=self.LIMIT)
651 # XXX: BradCrittenden 2009-04-24 bug=217644: Wrap the results to648 return result
652 # ensure the .count() method works until the Storm bug is fixed and
653 # integrated.
654 return DecoratedResultSet(result)
655649
656 def search(self, text):650 def search(self, text):
657 """Return people/teams whose fti or email address match :text:."""651 """Return people/teams whose fti or email address match :text:."""
@@ -727,10 +721,7 @@
727 result.config(distinct=True)721 result.config(distinct=True)
728 result.order_by(Person.displayname, Person.name)722 result.order_by(Person.displayname, Person.name)
729 result.config(limit=self.LIMIT)723 result.config(limit=self.LIMIT)
730 # XXX: BradCrittenden 2009-04-24 bug=217644: Wrap the results to724 return result
731 # ensure the .count() method works until the Storm bug is fixed and
732 # integrated.
733 return DecoratedResultSet(result)
734725
735726
736class ValidPersonVocabulary(ValidPersonOrTeamVocabulary):727class ValidPersonVocabulary(ValidPersonOrTeamVocabulary):
737728
=== 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-24 07:53:46 +0000
@@ -451,12 +451,7 @@
451 >>> packagediff_set.getPendingDiffs().count()451 >>> packagediff_set.getPendingDiffs().count()
452 7452 7
453453
454XXX cprov 20070530: storm doesn't go well with limited count()s454 >>> packagediff_set.getPendingDiffs(limit=2).count()
455See bug #217644. For now we have to listify the results and used
456the list length.
457
458 >>> r = packagediff_set.getPendingDiffs(limit=2)
459 >>> len(list(r))
460 2455 2
461456
462All package diffs targeting a set of source package releases can also457All package diffs targeting a set of source package releases can also