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
1=== modified file 'lib/canonical/launchpad/components/decoratedresultset.py'
2--- lib/canonical/launchpad/components/decoratedresultset.py 2010-08-20 20:31:18 +0000
3+++ lib/canonical/launchpad/components/decoratedresultset.py 2010-08-24 07:53:46 +0000
4@@ -9,7 +9,6 @@
5 ]
6
7 from lazr.delegates import delegates
8-from storm.expr import Column
9 from storm.zope.interfaces import IResultSet
10 from zope.security.proxy import removeSecurityProxy
11
12@@ -31,9 +30,6 @@
13
14 This behaviour is required for other classes as well (Distribution,
15 DistroArchSeries), hence a generalised solution.
16-
17- This class also fixes a bug currently in Storm's ResultSet.count
18- method (see below)
19 """
20 delegates(IResultSet, context='result_set')
21
22
23=== modified file 'lib/lp/archivepublisher/utils.py'
24--- lib/lp/archivepublisher/utils.py 2010-08-20 20:31:18 +0000
25+++ lib/lp/archivepublisher/utils.py 2010-08-24 07:53:46 +0000
26@@ -109,7 +109,8 @@
27 end = start + chunk_size
28
29 # The reason why we listify the sliced ResultSet is because we
30- # cannot very it's size using 'count' (see bug #217644). However,
31+ # cannot very it's size using 'count' (see bug #217644 and note
32+ # that it was fixed in storm but not SQLObjectResultSet). However,
33 # It's not exactly a problem considering non-empty set will be
34 # iterated anyway.
35 batch = list(self.input[start:end])
36
37=== modified file 'lib/lp/hardwaredb/model/hwdb.py'
38--- lib/lp/hardwaredb/model/hwdb.py 2010-08-20 20:31:18 +0000
39+++ lib/lp/hardwaredb/model/hwdb.py 2010-08-24 07:53:46 +0000
40@@ -64,9 +64,6 @@
41 SQLBase,
42 sqlvalues,
43 )
44-from canonical.launchpad.components.decoratedresultset import (
45- DecoratedResultSet,
46- )
47 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
48 from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
49 from canonical.launchpad.validators.name import valid_name
50@@ -347,13 +344,7 @@
51 # DISTINCT clause.
52 result_set.config(distinct=True)
53 result_set.order_by(HWSubmission.id)
54- # The Storm implementation of ResultSet.count() is incorrect if
55- # the select query uses the distinct directive (see bug #217644).
56- # DecoratedResultSet solves this problem by modifying the query
57- # to count only the records appearing in a subquery.
58- # We don't actually need to transform the results, which is why
59- # the second argument is a no-op.
60- return DecoratedResultSet(result_set, lambda result: result)
61+ return result_set
62
63 def _submissionsSubmitterSelects(
64 self, target_column, bus, vendor_id, product_id, driver_name,
65
66=== modified file 'lib/lp/registry/browser/distribution.py'
67--- lib/lp/registry/browser/distribution.py 2010-08-20 20:31:18 +0000
68+++ lib/lp/registry/browser/distribution.py 2010-08-24 07:53:46 +0000
69@@ -474,18 +474,7 @@
70 """See `AbstractPackageSearchView`."""
71
72 if self.search_by_binary_name:
73- non_exact_matches = self.context.searchBinaryPackages(self.text)
74-
75- # XXX Michael Nelson 20090605 bug=217644
76- # We are only using a decorated resultset here to conveniently
77- # get around the storm bug whereby count returns the count
78- # of non-distinct results, even though this result set
79- # is configured for distinct results.
80- def dummy_func(result):
81- return result
82- non_exact_matches = DecoratedResultSet(
83- non_exact_matches, dummy_func)
84-
85+ return self.context.searchBinaryPackages(self.text)
86 else:
87 non_exact_matches = self.context.searchSourcePackageCaches(
88 self.text)
89
90=== modified file 'lib/lp/registry/model/distroseries.py'
91--- lib/lp/registry/model/distroseries.py 2010-08-22 18:31:30 +0000
92+++ lib/lp/registry/model/distroseries.py 2010-08-24 07:53:46 +0000
93@@ -343,7 +343,7 @@
94 @cachedproperty
95 def _all_packagings(self):
96 """Get an unordered list of all packagings.
97-
98+
99 :return: A ResultSet which can be decorated or tuned further. Use
100 DistroSeries._packaging_row_to_packaging to extract the
101 packaging objects out.
102@@ -353,7 +353,7 @@
103 # Packaging object.
104 # NB: precaching objects like this method tries to do has a very poor
105 # hit rate with storm - many queries will still be executed; consider
106- # ripping this out and instead allowing explicit inclusion of things
107+ # ripping this out and instead allowing explicit inclusion of things
108 # like Person._all_members does - returning a cached object graph.
109 # -- RBC 20100810
110 # Avoid circular import failures.
111@@ -1809,11 +1809,7 @@
112 DistroSeries.hide_all_translations == False,
113 DistroSeries.id == POTemplate.distroseriesID)
114 result_set = result_set.config(distinct=True)
115- # XXX: henninge 2009-02-11 bug=217644: Convert to sequence right here
116- # because ResultSet reports a wrong count() when using DISTINCT. Also
117- # ResultSet does not implement __len__(), which would make it more
118- # like a sequence.
119- return list(result_set)
120+ return result_set
121
122 def findByName(self, name):
123 """See `IDistroSeriesSet`."""
124
125=== modified file 'lib/lp/registry/vocabularies.py'
126--- lib/lp/registry/vocabularies.py 2010-08-22 03:09:51 +0000
127+++ lib/lp/registry/vocabularies.py 2010-08-24 07:53:46 +0000
128@@ -95,9 +95,6 @@
129 SQLBase,
130 sqlvalues,
131 )
132-from canonical.launchpad.components.decoratedresultset import (
133- DecoratedResultSet,
134- )
135 from canonical.launchpad.database.account import Account
136 from canonical.launchpad.database.emailaddress import EmailAddress
137 from canonical.launchpad.database.stormsugar import StartsWith
138@@ -648,10 +645,7 @@
139 else:
140 result.order_by(Person.displayname, Person.name)
141 result.config(limit=self.LIMIT)
142- # XXX: BradCrittenden 2009-04-24 bug=217644: Wrap the results to
143- # ensure the .count() method works until the Storm bug is fixed and
144- # integrated.
145- return DecoratedResultSet(result)
146+ return result
147
148 def search(self, text):
149 """Return people/teams whose fti or email address match :text:."""
150@@ -727,10 +721,7 @@
151 result.config(distinct=True)
152 result.order_by(Person.displayname, Person.name)
153 result.config(limit=self.LIMIT)
154- # XXX: BradCrittenden 2009-04-24 bug=217644: Wrap the results to
155- # ensure the .count() method works until the Storm bug is fixed and
156- # integrated.
157- return DecoratedResultSet(result)
158+ return result
159
160
161 class ValidPersonVocabulary(ValidPersonOrTeamVocabulary):
162
163=== modified file 'lib/lp/soyuz/doc/package-diff.txt'
164--- lib/lp/soyuz/doc/package-diff.txt 2010-05-13 12:04:56 +0000
165+++ lib/lp/soyuz/doc/package-diff.txt 2010-08-24 07:53:46 +0000
166@@ -451,12 +451,7 @@
167 >>> packagediff_set.getPendingDiffs().count()
168 7
169
170-XXX cprov 20070530: storm doesn't go well with limited count()s
171-See bug #217644. For now we have to listify the results and used
172-the list length.
173-
174- >>> r = packagediff_set.getPendingDiffs(limit=2)
175- >>> len(list(r))
176+ >>> packagediff_set.getPendingDiffs(limit=2).count()
177 2
178
179 All package diffs targeting a set of source package releases can also