Merge lp:~lifeless/launchpad/prejoin into lp:launchpad
- prejoin
- Merge into devel
Proposed by
Robert Collins
Status: | Merged |
---|---|
Approved by: | Robert Collins |
Approved revision: | no longer in the source branch. |
Merged at revision: | 11956 |
Proposed branch: | lp:~lifeless/launchpad/prejoin |
Merge into: | lp:launchpad |
Diff against target: |
455 lines (+30/-259) 7 files modified
lib/lp/code/model/branch.py (+6/-3) lib/lp/registry/model/product.py (+4/-2) lib/lp/services/database/configure.zcml (+0/-4) lib/lp/services/database/prejoin.py (+0/-118) lib/lp/services/database/tests/test_prejoin.py (+0/-118) lib/lp/translations/model/potemplate.py (+7/-3) lib/lp/translations/model/translationgroup.py (+13/-11) |
To merge this branch: | bzr merge lp:~lifeless/launchpad/prejoin |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Jonathan Lange (community) | Approve | ||
Review via email: mp+39720@code.launchpad.net |
Commit message
Replace lp.services.
Description of the change
Delete unused code.
To post a comment you must log in.
Revision history for this message
Robert Collins (lifeless) wrote : | # |
Bah, my grep was not powerful enough. Theres at least 'a' user. Will investigate.
Revision history for this message
Robert Collins (lifeless) wrote : | # |
The 'extra' logic was used in one call site - to return a slice. We can, if we want, add it to DRS eventually - but for now lambda x:x[slice(0,3)] should do the job.
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'lib/lp/code/model/branch.py' |
2 | --- lib/lp/code/model/branch.py 2010-11-18 16:34:35 +0000 |
3 | +++ lib/lp/code/model/branch.py 2010-11-19 22:50:20 +0000 |
4 | @@ -10,6 +10,7 @@ |
5 | ] |
6 | |
7 | from datetime import datetime |
8 | +import operator |
9 | import simplejson |
10 | |
11 | from bzrlib import urlutils |
12 | @@ -58,6 +59,9 @@ |
13 | sqlvalues, |
14 | ) |
15 | from canonical.launchpad import _ |
16 | +from canonical.launchpad.components.decoratedresultset import ( |
17 | + DecoratedResultSet, |
18 | + ) |
19 | from canonical.launchpad.interfaces.launchpad import ( |
20 | ILaunchpadCelebrities, |
21 | IPrivacy, |
22 | @@ -129,7 +133,6 @@ |
23 | validate_person, |
24 | validate_public_person, |
25 | ) |
26 | -from lp.services.database.prejoin import prejoin |
27 | from lp.services.job.interfaces.job import JobStatus |
28 | from lp.services.job.model.job import Job |
29 | from lp.services.mail.notificationrecipientset import NotificationRecipientSet |
30 | @@ -283,7 +286,7 @@ |
31 | Revision.id == BranchRevision.revision_id, |
32 | BranchRevision.sequence != None) |
33 | result = result.order_by(Desc(BranchRevision.sequence)) |
34 | - return prejoin(result, return_slice=slice(0, 1)) |
35 | + return DecoratedResultSet(result, operator.itemgetter(0)) |
36 | |
37 | subscriptions = SQLMultipleJoin( |
38 | 'BranchSubscription', joinColumn='branch', orderBy='id') |
39 | @@ -593,7 +596,7 @@ |
40 | Revision.revision_date > timestamp) |
41 | result = result.order_by(Desc(BranchRevision.sequence)) |
42 | # Return BranchRevision but prejoin Revision as well. |
43 | - return prejoin(result, slice(0, 1)) |
44 | + return DecoratedResultSet(result, operator.itemgetter(0)) |
45 | |
46 | def canBeDeleted(self): |
47 | """See `IBranch`.""" |
48 | |
49 | === modified file 'lib/lp/registry/model/product.py' |
50 | --- lib/lp/registry/model/product.py 2010-11-08 01:08:15 +0000 |
51 | +++ lib/lp/registry/model/product.py 2010-11-19 22:50:20 +0000 |
52 | @@ -55,6 +55,9 @@ |
53 | SQLBase, |
54 | sqlvalues, |
55 | ) |
56 | +from canonical.launchpad.components.decoratedresultset import ( |
57 | + DecoratedResultSet, |
58 | + ) |
59 | from canonical.launchpad.interfaces.launchpad import ( |
60 | IHasIcon, |
61 | IHasLogo, |
62 | @@ -157,7 +160,6 @@ |
63 | from lp.registry.model.structuralsubscription import ( |
64 | StructuralSubscriptionTargetMixin, |
65 | ) |
66 | -from lp.services.database.prejoin import prejoin |
67 | from lp.services.propertycache import ( |
68 | cachedproperty, |
69 | get_property_cache, |
70 | @@ -1576,7 +1578,7 @@ |
71 | |
72 | # We only want Product - the other tables are just to populate |
73 | # the cache. |
74 | - return prejoin(results) |
75 | + return DecoratedResultSet(results, operator.itemgetter(0)) |
76 | |
77 | def featuredTranslatables(self, maximumproducts=8): |
78 | """See `IProductSet`""" |
79 | |
80 | === modified file 'lib/lp/services/database/configure.zcml' |
81 | --- lib/lp/services/database/configure.zcml 2010-03-11 15:57:43 +0000 |
82 | +++ lib/lp/services/database/configure.zcml 2010-11-19 22:50:20 +0000 |
83 | @@ -8,10 +8,6 @@ |
84 | xmlns:i18n="http://namespaces.zope.org/i18n" |
85 | i18n_domain="launchpad"> |
86 | |
87 | - <class class="lp.services.database.prejoin.PrejoinResultSet"> |
88 | - <allow interface="storm.zope.interfaces.IResultSet" /> |
89 | - <allow attributes="__getslice__" /> |
90 | - </class> |
91 | <class class="storm.references.BoundIndirectReferenceSet"> |
92 | <allow attributes="add remove clear" /> |
93 | </class> |
94 | |
95 | === removed file 'lib/lp/services/database/prejoin.py' |
96 | --- lib/lp/services/database/prejoin.py 2010-09-07 04:53:35 +0000 |
97 | +++ lib/lp/services/database/prejoin.py 1970-01-01 00:00:00 +0000 |
98 | @@ -1,118 +0,0 @@ |
99 | -# Copyright 2009 Canonical Ltd. This software is licensed under the |
100 | -# GNU Affero General Public License version 3 (see the file LICENSE). |
101 | - |
102 | -"""Prejoin for Storm. |
103 | - |
104 | -XXX bug=632150 This is duplicated with DecoratedResultSet. please use that. |
105 | -RBC 20100907. |
106 | -""" |
107 | - |
108 | -__metaclass__ = type |
109 | -__all__ = ['prejoin'] |
110 | - |
111 | -from lazr.delegates import delegates |
112 | -from storm.zope.interfaces import IResultSet |
113 | - |
114 | - |
115 | -class PrejoinResultSet: |
116 | - """Prejoin support. |
117 | - |
118 | - Wrap a ResultSet, trimming unwanted items. The unwanted items |
119 | - are still pulled from the database and populate the Storm caches. |
120 | - |
121 | - This is a temporary solution, as as count() performs an unnecessary |
122 | - join making it slower and __contains__() is not implemented at all. |
123 | - The preferred solution is support in Storm core, so we can just do |
124 | - something like: |
125 | - |
126 | - # Select Product, but prejoin the owner as well. |
127 | - >>> join = store.find((Product, Person), Product.name == name) |
128 | - >>> results = prejoin(join, slice(0, 1)) |
129 | - """ |
130 | - delegates(IResultSet, context='result_set') |
131 | - |
132 | - def __init__(self, result_set, return_slice=slice(0, 1)): |
133 | - self.result_set = result_set |
134 | - self.return_slice = return_slice |
135 | - |
136 | - def _chain(self, result_set): |
137 | - return PrejoinResultSet(result_set, self.return_slice) |
138 | - |
139 | - def _chomp(self, row): |
140 | - if row is None: |
141 | - return None |
142 | - elems = row[self.return_slice] |
143 | - if len(elems) == 1: |
144 | - return elems[0] |
145 | - else: |
146 | - return elems |
147 | - |
148 | - def copy(self): |
149 | - """See `IResultSet`.""" |
150 | - return self._chain(self.result_set.copy()) |
151 | - |
152 | - def config(self, distinct=None, offset=None, limit=None): |
153 | - """See `IResultSet`.""" |
154 | - self.result_set.config(distinct, offset, limit) |
155 | - return self |
156 | - |
157 | - def order_by(self, *args): |
158 | - """See `IResultSet`.""" |
159 | - return self._chain(self.result_set.order_by(*args)) |
160 | - |
161 | - def difference(self, *args, **kw): |
162 | - """See `IResultSet`.""" |
163 | - raise NotImplementedError("difference") |
164 | - |
165 | - def group_by(self, *args, **kw): |
166 | - """See `IResultSet`.""" |
167 | - raise NotImplementedError("group_by") |
168 | - |
169 | - def having(self, *args, **kw): |
170 | - """See `IResultSet`.""" |
171 | - raise NotImplementedError("having") |
172 | - |
173 | - def intersection(self, *args, **kw): |
174 | - """See `IResultSet`.""" |
175 | - raise NotImplementedError("intersection") |
176 | - |
177 | - def union(self, *args, **kw): |
178 | - """See `IResultSet`.""" |
179 | - raise NotImplementedError("union") |
180 | - |
181 | - def __iter__(self): |
182 | - """See `IResultSet`.""" |
183 | - return (self._chomp(row) for row in self.result_set) |
184 | - |
185 | - def __getitem__(self, index): |
186 | - """See `IResultSet`.""" |
187 | - if isinstance(index, slice): |
188 | - return self._chain(self.result_set[index]) |
189 | - else: |
190 | - return self._chomp(self.result_set[index]) |
191 | - |
192 | - def __contains__(self, item): |
193 | - """See `IResultSet`.""" |
194 | - raise NotImplementedError("__contains__") |
195 | - |
196 | - def any(self): |
197 | - """See `IResultSet`.""" |
198 | - return self._chomp(self.result_set.any()) |
199 | - |
200 | - def first(self): |
201 | - """See `IResultSet`.""" |
202 | - return self._chomp(self.result_set.first()) |
203 | - |
204 | - def last(self): |
205 | - """See `IResultSet`.""" |
206 | - return self._chomp(self.result_set.last()) |
207 | - |
208 | - def one(self): |
209 | - """See `IResultSet`.""" |
210 | - return self._chomp(self.result_set.one()) |
211 | - |
212 | - def cached(self): |
213 | - """See `IResultSet`.""" |
214 | - raise NotImplementedError("cached") |
215 | - |
216 | -prejoin = PrejoinResultSet |
217 | |
218 | === removed file 'lib/lp/services/database/tests/test_prejoin.py' |
219 | --- lib/lp/services/database/tests/test_prejoin.py 2010-10-20 20:51:26 +0000 |
220 | +++ lib/lp/services/database/tests/test_prejoin.py 1970-01-01 00:00:00 +0000 |
221 | @@ -1,118 +0,0 @@ |
222 | -# Copyright 2009 Canonical Ltd. This software is licensed under the |
223 | -# GNU Affero General Public License version 3 (see the file LICENSE). |
224 | - |
225 | -"""prejoin module tests.""" |
226 | - |
227 | -__metaclass__ = type |
228 | -__all__ = [] |
229 | - |
230 | -import unittest |
231 | - |
232 | -from canonical.launchpad.interfaces.lpstorm import IMasterStore |
233 | -from canonical.testing.layers import LaunchpadZopelessLayer |
234 | -from lp.registry.model.person import Person |
235 | -from lp.registry.model.product import Product |
236 | -from lp.services.database.prejoin import ( |
237 | - prejoin, |
238 | - PrejoinResultSet, |
239 | - ) |
240 | -from lp.testing import TestCase |
241 | - |
242 | - |
243 | -class PrejoinTestCase(TestCase): |
244 | - layer = LaunchpadZopelessLayer |
245 | - |
246 | - def setUp(self): |
247 | - super(PrejoinTestCase, self).setUp() |
248 | - self.store = IMasterStore(Product) |
249 | - |
250 | - # All products |
251 | - self.standard_result = self.store.find(Product).order_by(Product.name) |
252 | - |
253 | - # All products, with owner and preferred email address prejoined. |
254 | - # Note that because some Product owners have multiple email |
255 | - # addresses, this query returns more results. prejoin needs |
256 | - # to hide this from callsites. |
257 | - self.unwrapped_result = self.store.find( |
258 | - (Product, Person), |
259 | - Product._ownerID == Person.id).order_by(Product.name) |
260 | - self.prejoin_result = prejoin(self.unwrapped_result) |
261 | - |
262 | - def verify(self, prejoined, normal): |
263 | - # Ensure our prejoined result really is a PrejoinResultSet. |
264 | - # One of our methods might not be sticky, and we have ended up |
265 | - # with a normal Storm ResultSet. |
266 | - self.assertTrue( |
267 | - isinstance(prejoined, PrejoinResultSet), |
268 | - "Expected a PrejoinResultSet, got %s" % repr(prejoined)) |
269 | - |
270 | - # Confirm the two result sets return identical results. |
271 | - self.assertEqual(list(normal), list(prejoined)) |
272 | - |
273 | - def test_count(self): |
274 | - self.assertEqual( |
275 | - self.standard_result.count(), |
276 | - self.prejoin_result.count()) |
277 | - |
278 | - def test_copy(self): |
279 | - copy = self.prejoin_result.copy() |
280 | - self.verify(copy, self.standard_result) |
281 | - |
282 | - def test_config(self): |
283 | - self.prejoin_result.config(offset=3, limit=2) |
284 | - self.standard_result.config(offset=3, limit=2) |
285 | - self.verify(self.prejoin_result, self.standard_result) |
286 | - |
287 | - def test_config_returnvalue(self): |
288 | - prejoin_rv = self.prejoin_result.config(offset=3, limit=2) |
289 | - standard_rv = self.standard_result.config(offset=3, limit=2) |
290 | - self.verify(prejoin_rv, standard_rv) |
291 | - |
292 | - def test_order_by(self): |
293 | - self.verify( |
294 | - self.prejoin_result.order_by(Product.id), |
295 | - self.standard_result.order_by(Product.id)) |
296 | - |
297 | - def test_slice(self): |
298 | - self.verify( |
299 | - self.prejoin_result[4:6], |
300 | - self.standard_result[4:6]) |
301 | - |
302 | - def test_any(self): |
303 | - self.assertIn(self.prejoin_result.any(), self.standard_result) |
304 | - |
305 | - def test_first(self): |
306 | - self.assertEqual( |
307 | - self.standard_result.first(), self.prejoin_result.first()) |
308 | - |
309 | - def test_one(self): |
310 | - standard_result = self.store.find(Product, Product.name == 'firefox') |
311 | - prejoin_result = prejoin(self.store.find( |
312 | - (Product, Person), |
313 | - Person.id == Product._ownerID, |
314 | - Product.name == 'firefox')) |
315 | - self.assertEqual(standard_result.one(), prejoin_result.one()) |
316 | - |
317 | - def test_one_empty(self): |
318 | - # For an empty result (None), one returns None, too. |
319 | - name = "none-existent-name" |
320 | - prejoin_result = prejoin(self.store.find( |
321 | - (Product, Person), |
322 | - Person.id == Product._ownerID, |
323 | - Product.name == name)) |
324 | - self.assertIs(None, prejoin_result.one()) |
325 | - |
326 | - def test_cache_populated(self): |
327 | - # Load a row. |
328 | - product = self.prejoin_result.first() |
329 | - |
330 | - # Destroy our data, without telling Storm. This way we can |
331 | - # tell if we are accessing an object from the cache, or if it |
332 | - # was retrieved from the database. |
333 | - self.store.execute("UPDATE Person SET displayname='foo'") |
334 | - |
335 | - self.failIfEqual('foo', product.owner.displayname) |
336 | - |
337 | - |
338 | -def test_suite(): |
339 | - return unittest.TestLoader().loadTestsFromName(__name__) |
340 | |
341 | === modified file 'lib/lp/translations/model/potemplate.py' |
342 | --- lib/lp/translations/model/potemplate.py 2010-11-11 06:16:33 +0000 |
343 | +++ lib/lp/translations/model/potemplate.py 2010-11-19 22:50:20 +0000 |
344 | @@ -17,6 +17,7 @@ |
345 | |
346 | import datetime |
347 | import logging |
348 | +import operator |
349 | import os |
350 | import re |
351 | |
352 | @@ -56,6 +57,9 @@ |
353 | sqlvalues, |
354 | ) |
355 | from canonical.launchpad import helpers |
356 | +from canonical.launchpad.components.decoratedresultset import ( |
357 | + DecoratedResultSet, |
358 | + ) |
359 | from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities |
360 | from canonical.launchpad.interfaces.lpstorm import ( |
361 | IMasterStore, |
362 | @@ -65,7 +69,6 @@ |
363 | from lp.registry.interfaces.person import validate_public_person |
364 | from lp.registry.model.sourcepackagename import SourcePackageName |
365 | from lp.services.database.collection import Collection |
366 | -from lp.services.database.prejoin import prejoin |
367 | from lp.services.propertycache import cachedproperty |
368 | from lp.services.worlddata.model.language import Language |
369 | from lp.translations.enums import RosettaImportStatus |
370 | @@ -1066,10 +1069,11 @@ |
371 | else: |
372 | store = Store.of(self.distroseries) |
373 | if do_prejoin: |
374 | - query = prejoin(store.find( |
375 | + query = DecoratedResultSet(store.find( |
376 | (POTemplate, SourcePackageName), |
377 | (POTemplate.sourcepackagenameID == |
378 | - SourcePackageName.id), condition)) |
379 | + SourcePackageName.id), condition), |
380 | + operator.itemgetter(0)) |
381 | else: |
382 | query = store.find(POTemplate, condition) |
383 | |
384 | |
385 | === modified file 'lib/lp/translations/model/translationgroup.py' |
386 | --- lib/lp/translations/model/translationgroup.py 2010-09-03 10:59:24 +0000 |
387 | +++ lib/lp/translations/model/translationgroup.py 2010-11-19 22:50:20 +0000 |
388 | @@ -9,6 +9,8 @@ |
389 | 'TranslationGroupSet', |
390 | ] |
391 | |
392 | +import operator |
393 | + |
394 | from sqlobject import ( |
395 | ForeignKey, |
396 | SQLMultipleJoin, |
397 | @@ -26,6 +28,9 @@ |
398 | from canonical.database.constants import DEFAULT |
399 | from canonical.database.datetimecol import UtcDateTimeCol |
400 | from canonical.database.sqlbase import SQLBase |
401 | +from canonical.launchpad.components.decoratedresultset import ( |
402 | + DecoratedResultSet, |
403 | + ) |
404 | from canonical.launchpad.database.librarian import ( |
405 | LibraryFileAlias, |
406 | LibraryFileContent, |
407 | @@ -41,7 +46,6 @@ |
408 | ) |
409 | from lp.registry.model.projectgroup import ProjectGroup |
410 | from lp.registry.model.teammembership import TeamParticipation |
411 | -from lp.services.database.prejoin import prejoin |
412 | from lp.services.worlddata.model.language import Language |
413 | from lp.translations.interfaces.translationgroup import ( |
414 | ITranslationGroup, |
415 | @@ -171,10 +175,9 @@ |
416 | Translator.translationgroup == self, |
417 | Language.id == Translator.languageID, |
418 | Person.id == Translator.translatorID) |
419 | - |
420 | - return prejoin( |
421 | - translator_data.order_by(Language.englishname), |
422 | - slice(0, 3)) |
423 | + translator_data = translator_data.order_by(Language.englishname) |
424 | + mapper = lambda row:row[slice(0,3)] |
425 | + return DecoratedResultSet(translator_data, mapper) |
426 | |
427 | def fetchProjectsForDisplay(self): |
428 | """See `ITranslationGroup`.""" |
429 | @@ -218,10 +221,9 @@ |
430 | project_data = ISlaveStore(ProjectGroup).using(*using).find( |
431 | tables, |
432 | ProjectGroup.translationgroupID == self.id, |
433 | - ProjectGroup.active == True) |
434 | + ProjectGroup.active == True).order_by(ProjectGroup.displayname) |
435 | |
436 | - return prejoin( |
437 | - project_data.order_by(ProjectGroup.displayname), slice(0, 1)) |
438 | + return DecoratedResultSet(project_data, operator.itemgetter(0)) |
439 | |
440 | def fetchDistrosForDisplay(self): |
441 | """See `ITranslationGroup`.""" |
442 | @@ -239,10 +241,10 @@ |
443 | LibraryFileContent, |
444 | ) |
445 | distro_data = ISlaveStore(Distribution).using(*using).find( |
446 | - tables, Distribution.translationgroupID == self.id) |
447 | + tables, Distribution.translationgroupID == self.id).order_by( |
448 | + Distribution.displayname) |
449 | |
450 | - return prejoin( |
451 | - distro_data.order_by(Distribution.displayname), slice(0, 1)) |
452 | + return DecoratedResultSet(distro_data, operator.itemgetter(0)) |
453 | |
454 | |
455 | class TranslationGroupSet: |
I guess this isn't actually used anywhere, so my one qualm – that this is not a duplicate of DecoratedResultSet but instead a near-duplicate with extra logic – is not valid.