Merge lp:~lifeless/launchpad/bug-618367 into lp:launchpad
- bug-618367
- Merge into devel
Proposed by
Robert Collins
Status: | Merged |
---|---|
Approved by: | Robert Collins |
Approved revision: | no longer in the source branch. |
Merged at revision: | 11483 |
Proposed branch: | lp:~lifeless/launchpad/bug-618367 |
Merge into: | lp:launchpad |
Diff against target: |
419 lines (+246/-42) 5 files modified
lib/lp/blueprints/browser/tests/test_views.py (+78/-5) lib/lp/blueprints/model/specification.py (+78/-0) lib/lp/registry/model/distribution.py (+9/-9) lib/lp/registry/model/person.py (+72/-19) lib/lp/registry/model/product.py (+9/-9) |
To merge this branch: | bzr merge lp:~lifeless/launchpad/bug-618367 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Michael Nelson (community) | code | Approve | |
Review via email: mp+34162@code.launchpad.net |
Commit message
Preload is_valid_person for +assignments on Product and Distribution.
Description of the change
Preload is_valid_person for +assignments on Product and Distribution, should fix bug 618367, according to the observed oopses. It will probably still be slow, but 6 seconds faster.
To post a comment you must log in.
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'lib/lp/blueprints/browser/tests/test_views.py' | |||
2 | --- lib/lp/blueprints/browser/tests/test_views.py 2010-08-20 20:31:18 +0000 | |||
3 | +++ lib/lp/blueprints/browser/tests/test_views.py 2010-08-31 19:38:45 +0000 | |||
4 | @@ -9,19 +9,92 @@ | |||
5 | 9 | import os | 9 | import os |
6 | 10 | import unittest | 10 | import unittest |
7 | 11 | 11 | ||
8 | 12 | from storm.store import Store | ||
9 | 13 | from testtools.matchers import LessThan | ||
10 | 14 | |||
11 | 12 | from canonical.launchpad.testing.systemdocs import ( | 15 | from canonical.launchpad.testing.systemdocs import ( |
12 | 13 | LayeredDocFileSuite, | 16 | LayeredDocFileSuite, |
13 | 14 | setUp, | 17 | setUp, |
14 | 15 | tearDown, | 18 | tearDown, |
15 | 16 | ) | 19 | ) |
16 | 20 | from canonical.launchpad.webapp import canonical_url | ||
17 | 17 | from canonical.testing import DatabaseFunctionalLayer | 21 | from canonical.testing import DatabaseFunctionalLayer |
22 | 18 | 22 | from lp.testing import ( | |
23 | 19 | 23 | login, | |
24 | 20 | here = os.path.dirname(os.path.realpath(__file__)) | 24 | logout, |
25 | 21 | 25 | TestCaseWithFactory, | |
26 | 26 | ) | ||
27 | 27 | from lp.testing.matchers import HasQueryCount | ||
28 | 28 | from lp.testing.sampledata import ADMIN_EMAIL | ||
29 | 29 | from lp.testing._webservice import QueryCollector | ||
30 | 30 | |||
31 | 31 | |||
32 | 32 | class TestAssignments(TestCaseWithFactory): | ||
33 | 33 | |||
34 | 34 | layer = DatabaseFunctionalLayer | ||
35 | 35 | |||
36 | 36 | def invalidate_and_render(self, browser, dbobj, url): | ||
37 | 37 | # Ensure caches have been flushed. | ||
38 | 38 | store = Store.of(dbobj) | ||
39 | 39 | store.flush() | ||
40 | 40 | store.invalidate() | ||
41 | 41 | browser.open(url) | ||
42 | 42 | |||
43 | 43 | def check_query_counts_scaling_with_unique_people(self, | ||
44 | 44 | target, targettype): | ||
45 | 45 | """Check that a particular hasSpecifications target scales well. | ||
46 | 46 | |||
47 | 47 | :param target: A spec target like a product. | ||
48 | 48 | :param targettype: The parameter to pass to makeSpecification to | ||
49 | 49 | associate the target. e.g. 'product'. | ||
50 | 50 | """ | ||
51 | 51 | query_baseline = 40 | ||
52 | 52 | people = [] | ||
53 | 53 | for _ in range(10): | ||
54 | 54 | people.append(self.factory.makePerson()) | ||
55 | 55 | specs = [] | ||
56 | 56 | for _ in range(10): | ||
57 | 57 | specs.append(self.factory.makeSpecification( | ||
58 | 58 | **{targettype:target})) | ||
59 | 59 | collector = QueryCollector() | ||
60 | 60 | collector.register() | ||
61 | 61 | self.addCleanup(collector.unregister) | ||
62 | 62 | viewer = self.factory.makePerson(password="test") | ||
63 | 63 | browser = self.getUserBrowser(user=viewer) | ||
64 | 64 | url = canonical_url(target) + "/+assignments" | ||
65 | 65 | # Seed the cookie cache and any other cross-request state we may gain | ||
66 | 66 | # in future. See canonical.launchpad.webapp.serssion: _get_secret. | ||
67 | 67 | browser.open(url) | ||
68 | 68 | self.invalidate_and_render(browser, target, url) | ||
69 | 69 | # Set a baseline | ||
70 | 70 | self.assertThat(collector, HasQueryCount(LessThan(query_baseline))) | ||
71 | 71 | no_assignees_count = collector.count | ||
72 | 72 | # Assign many unique people, which shouldn't change the page queries. | ||
73 | 73 | # Due to storm bug 619017 additional queries can be triggered when | ||
74 | 74 | # revalidating people, so we allow -some- fuzz. | ||
75 | 75 | login(ADMIN_EMAIL) | ||
76 | 76 | for person, spec in zip(people, specs): | ||
77 | 77 | spec.assignee = person | ||
78 | 78 | logout() | ||
79 | 79 | self.invalidate_and_render(browser, target, url) | ||
80 | 80 | self.assertThat( | ||
81 | 81 | collector, | ||
82 | 82 | HasQueryCount(LessThan(no_assignees_count + 5))) | ||
83 | 83 | |||
84 | 84 | def test_product_query_counts_scale_below_unique_people(self): | ||
85 | 85 | self.check_query_counts_scaling_with_unique_people( | ||
86 | 86 | self.factory.makeProduct(), | ||
87 | 87 | 'product') | ||
88 | 88 | |||
89 | 89 | def test_distro_query_counts_scale_below_unique_people(self): | ||
90 | 90 | self.check_query_counts_scaling_with_unique_people( | ||
91 | 91 | self.factory.makeDistribution(), | ||
92 | 92 | 'distribution') | ||
93 | 93 | |||
94 | 22 | 94 | ||
95 | 23 | def test_suite(): | 95 | def test_suite(): |
97 | 24 | suite = unittest.TestSuite() | 96 | suite = unittest.TestLoader().loadTestsFromName(__name__) |
98 | 97 | here = os.path.dirname(os.path.realpath(__file__)) | ||
99 | 25 | testsdir = os.path.abspath(here) | 98 | testsdir = os.path.abspath(here) |
100 | 26 | 99 | ||
101 | 27 | # Add tests using default setup/teardown | 100 | # Add tests using default setup/teardown |
102 | 28 | 101 | ||
103 | === modified file 'lib/lp/blueprints/model/specification.py' | |||
104 | --- lib/lp/blueprints/model/specification.py 2010-08-20 20:31:18 +0000 | |||
105 | +++ lib/lp/blueprints/model/specification.py 2010-08-31 19:38:45 +0000 | |||
106 | @@ -24,6 +24,14 @@ | |||
107 | 24 | SQLRelatedJoin, | 24 | SQLRelatedJoin, |
108 | 25 | StringCol, | 25 | StringCol, |
109 | 26 | ) | 26 | ) |
110 | 27 | from storm.expr import ( | ||
111 | 28 | LeftJoin, | ||
112 | 29 | ) | ||
113 | 30 | from storm.locals import ( | ||
114 | 31 | ClassAlias, | ||
115 | 32 | Desc, | ||
116 | 33 | SQL, | ||
117 | 34 | ) | ||
118 | 27 | from storm.store import Store | 35 | from storm.store import Store |
119 | 28 | from zope.event import notify | 36 | from zope.event import notify |
120 | 29 | from zope.interface import implements | 37 | from zope.interface import implements |
121 | @@ -40,6 +48,9 @@ | |||
122 | 40 | SQLBase, | 48 | SQLBase, |
123 | 41 | sqlvalues, | 49 | sqlvalues, |
124 | 42 | ) | 50 | ) |
125 | 51 | from canonical.launchpad.components.decoratedresultset import ( | ||
126 | 52 | DecoratedResultSet, | ||
127 | 53 | ) | ||
128 | 43 | from canonical.launchpad.helpers import ( | 54 | from canonical.launchpad.helpers import ( |
129 | 44 | get_contact_email_addresses, | 55 | get_contact_email_addresses, |
130 | 45 | shortlist, | 56 | shortlist, |
131 | @@ -680,6 +691,73 @@ | |||
132 | 680 | # this should be implemented by the actual context class | 691 | # this should be implemented by the actual context class |
133 | 681 | raise NotImplementedError | 692 | raise NotImplementedError |
134 | 682 | 693 | ||
135 | 694 | def _specification_sort(self, sort): | ||
136 | 695 | """Return the storm sort order for 'specifications'. | ||
137 | 696 | |||
138 | 697 | :param sort: As per HasSpecificationsMixin.specifications. | ||
139 | 698 | """ | ||
140 | 699 | # sort by priority descending, by default | ||
141 | 700 | if sort is None or sort == SpecificationSort.PRIORITY: | ||
142 | 701 | return ( | ||
143 | 702 | Desc(Specification.priority), Specification.definition_status, | ||
144 | 703 | Specification.name) | ||
145 | 704 | elif sort == SpecificationSort.DATE: | ||
146 | 705 | return (Desc(Specification.datecreated), Specification.id) | ||
147 | 706 | |||
148 | 707 | def _preload_specifications_people(self, query): | ||
149 | 708 | """Perform eager loading of people and their validity for query. | ||
150 | 709 | |||
151 | 710 | :param query: a string query generated in the 'specifications' | ||
152 | 711 | method. | ||
153 | 712 | :return: A DecoratedResultSet with Person precaching setup. | ||
154 | 713 | """ | ||
155 | 714 | # Circular import. | ||
156 | 715 | from lp.registry.model.person import Person | ||
157 | 716 | assignee = ClassAlias(Person, "assignee") | ||
158 | 717 | approver = ClassAlias(Person, "approver") | ||
159 | 718 | drafter = ClassAlias(Person, "drafter") | ||
160 | 719 | origin = [Specification, | ||
161 | 720 | LeftJoin(assignee, Specification.assignee==assignee.id), | ||
162 | 721 | LeftJoin(approver, Specification.approver==approver.id), | ||
163 | 722 | LeftJoin(drafter, Specification.drafter==drafter.id), | ||
164 | 723 | ] | ||
165 | 724 | columns = [Specification, assignee, approver, drafter] | ||
166 | 725 | for alias in (assignee, approver, drafter): | ||
167 | 726 | validity_info = Person._validity_queries(person_table=alias) | ||
168 | 727 | origin.extend(validity_info["joins"]) | ||
169 | 728 | columns.extend(validity_info["tables"]) | ||
170 | 729 | # We only need one decorators list: all the decorators will be | ||
171 | 730 | # bound the same, and having a single list lets us more easily call | ||
172 | 731 | # the right one. | ||
173 | 732 | decorators = validity_info["decorators"] | ||
174 | 733 | columns = tuple(columns) | ||
175 | 734 | results = Store.of(self).using(*origin).find( | ||
176 | 735 | columns, | ||
177 | 736 | SQL(query), | ||
178 | 737 | ) | ||
179 | 738 | def cache_person(person, row, start_index): | ||
180 | 739 | """apply caching decorators to person.""" | ||
181 | 740 | index = start_index | ||
182 | 741 | for decorator in decorators: | ||
183 | 742 | column = row[index] | ||
184 | 743 | index += 1 | ||
185 | 744 | decorator(person, column) | ||
186 | 745 | return index | ||
187 | 746 | def cache_validity(row): | ||
188 | 747 | # Assignee | ||
189 | 748 | person = row[1] | ||
190 | 749 | # After drafter | ||
191 | 750 | index = 4 | ||
192 | 751 | index = cache_person(person, row, index) | ||
193 | 752 | # approver | ||
194 | 753 | person = row[2] | ||
195 | 754 | index = cache_person(person, row, index) | ||
196 | 755 | # drafter | ||
197 | 756 | person = row[3] | ||
198 | 757 | index = cache_person(person, row, index) | ||
199 | 758 | return row[0] | ||
200 | 759 | return DecoratedResultSet(results, cache_validity) | ||
201 | 760 | |||
202 | 683 | @property | 761 | @property |
203 | 684 | def valid_specifications(self): | 762 | def valid_specifications(self): |
204 | 685 | """See IHasSpecifications.""" | 763 | """See IHasSpecifications.""" |
205 | 686 | 764 | ||
206 | === modified file 'lib/lp/registry/model/distribution.py' | |||
207 | --- lib/lp/registry/model/distribution.py 2010-08-24 15:29:01 +0000 | |||
208 | +++ lib/lp/registry/model/distribution.py 2010-08-31 19:38:45 +0000 | |||
209 | @@ -810,13 +810,7 @@ | |||
210 | 810 | # defaults for informationalness: we don't have to do anything | 810 | # defaults for informationalness: we don't have to do anything |
211 | 811 | # because the default if nothing is said is ANY | 811 | # because the default if nothing is said is ANY |
212 | 812 | 812 | ||
220 | 813 | # sort by priority descending, by default | 813 | order = self._specification_sort(sort) |
214 | 814 | if sort is None or sort == SpecificationSort.PRIORITY: | ||
215 | 815 | order = ( | ||
216 | 816 | ['-priority', 'Specification.definition_status', | ||
217 | 817 | 'Specification.name']) | ||
218 | 818 | elif sort == SpecificationSort.DATE: | ||
219 | 819 | order = ['-Specification.datecreated', 'Specification.id'] | ||
221 | 820 | 814 | ||
222 | 821 | # figure out what set of specifications we are interested in. for | 815 | # figure out what set of specifications we are interested in. for |
223 | 822 | # distributions, we need to be able to filter on the basis of: | 816 | # distributions, we need to be able to filter on the basis of: |
224 | @@ -859,9 +853,15 @@ | |||
225 | 859 | query += ' AND Specification.fti @@ ftq(%s) ' % quote( | 853 | query += ' AND Specification.fti @@ ftq(%s) ' % quote( |
226 | 860 | constraint) | 854 | constraint) |
227 | 861 | 855 | ||
228 | 862 | results = Specification.select(query, orderBy=order, limit=quantity) | ||
229 | 863 | if prejoin_people: | 856 | if prejoin_people: |
231 | 864 | results = results.prejoin(['assignee', 'approver', 'drafter']) | 857 | results = self._preload_specifications_people(query) |
232 | 858 | else: | ||
233 | 859 | results = Store.of(self).find( | ||
234 | 860 | Specification, | ||
235 | 861 | SQL(query)) | ||
236 | 862 | results.order_by(order) | ||
237 | 863 | if quantity is not None: | ||
238 | 864 | results = results[:quantity] | ||
239 | 865 | return results | 865 | return results |
240 | 866 | 866 | ||
241 | 867 | def getSpecification(self, name): | 867 | def getSpecification(self, name): |
242 | 868 | 868 | ||
243 | === modified file 'lib/lp/registry/model/person.py' | |||
244 | --- lib/lp/registry/model/person.py 2010-08-23 16:51:11 +0000 | |||
245 | +++ lib/lp/registry/model/person.py 2010-08-31 19:38:45 +0000 | |||
246 | @@ -1172,6 +1172,7 @@ | |||
247 | 1172 | @cachedproperty('_is_valid_person_cached') | 1172 | @cachedproperty('_is_valid_person_cached') |
248 | 1173 | def is_valid_person(self): | 1173 | def is_valid_person(self): |
249 | 1174 | """See `IPerson`.""" | 1174 | """See `IPerson`.""" |
250 | 1175 | # This is prepopulated by various queries in and out of person.py. | ||
251 | 1175 | if self.is_team: | 1176 | if self.is_team: |
252 | 1176 | return False | 1177 | return False |
253 | 1177 | try: | 1178 | try: |
254 | @@ -1577,6 +1578,67 @@ | |||
255 | 1577 | need_location=True, need_archive=True, need_preferred_email=True, | 1578 | need_location=True, need_archive=True, need_preferred_email=True, |
256 | 1578 | need_validity=True) | 1579 | need_validity=True) |
257 | 1579 | 1580 | ||
258 | 1581 | @staticmethod | ||
259 | 1582 | def _validity_queries(person_table=None): | ||
260 | 1583 | """Return storm expressions and a decorator function for validity. | ||
261 | 1584 | |||
262 | 1585 | Preloading validity implies preloading preferred email addresses. | ||
263 | 1586 | |||
264 | 1587 | :param person_table: The person table to join to. Only supply if | ||
265 | 1588 | ClassAliases are in use. | ||
266 | 1589 | :return: A dict with four keys joins, tables, conditions, decorators | ||
267 | 1590 | |||
268 | 1591 | * joins are additional joins to use. e.g. [LeftJoin,LeftJoin] | ||
269 | 1592 | * tables are tables to use e.g. [EmailAddress, Account] | ||
270 | 1593 | * decorators are callbacks to call for each row. Each decorator takes | ||
271 | 1594 | (Person, column) where column is the column in the result set for that | ||
272 | 1595 | decorators type. | ||
273 | 1596 | """ | ||
274 | 1597 | if person_table is None: | ||
275 | 1598 | person_table = Person | ||
276 | 1599 | email_table = EmailAddress | ||
277 | 1600 | account_table = Account | ||
278 | 1601 | else: | ||
279 | 1602 | email_table = ClassAlias(EmailAddress) | ||
280 | 1603 | account_table = ClassAlias(Account) | ||
281 | 1604 | origins = [] | ||
282 | 1605 | columns = [] | ||
283 | 1606 | decorators = [] | ||
284 | 1607 | # Teams don't have email, so a left join | ||
285 | 1608 | origins.append( | ||
286 | 1609 | LeftJoin(email_table, And( | ||
287 | 1610 | email_table.personID == person_table.id, | ||
288 | 1611 | email_table.status == EmailAddressStatus.PREFERRED))) | ||
289 | 1612 | columns.append(email_table) | ||
290 | 1613 | origins.append( | ||
291 | 1614 | LeftJoin(account_table, And( | ||
292 | 1615 | person_table.accountID == account_table.id, | ||
293 | 1616 | account_table.status == AccountStatus.ACTIVE))) | ||
294 | 1617 | columns.append(account_table) | ||
295 | 1618 | def handleemail(person, column): | ||
296 | 1619 | #-- preferred email caching | ||
297 | 1620 | if not person: | ||
298 | 1621 | return | ||
299 | 1622 | email = column | ||
300 | 1623 | cache_property(person, '_preferredemail_cached', email) | ||
301 | 1624 | decorators.append(handleemail) | ||
302 | 1625 | def handleaccount(person, column): | ||
303 | 1626 | #-- validity caching | ||
304 | 1627 | if not person: | ||
305 | 1628 | return | ||
306 | 1629 | # valid if: | ||
307 | 1630 | valid = ( | ||
308 | 1631 | # -- valid account found | ||
309 | 1632 | column is not None | ||
310 | 1633 | # -- preferred email found | ||
311 | 1634 | and person.preferredemail is not None) | ||
312 | 1635 | cache_property(person, '_is_valid_person_cached', valid) | ||
313 | 1636 | decorators.append(handleaccount) | ||
314 | 1637 | return dict( | ||
315 | 1638 | joins=origins, | ||
316 | 1639 | tables=columns, | ||
317 | 1640 | decorators=decorators) | ||
318 | 1641 | |||
319 | 1580 | def _all_members(self, need_karma=False, need_ubuntu_coc=False, | 1642 | def _all_members(self, need_karma=False, need_ubuntu_coc=False, |
320 | 1581 | need_location=False, need_archive=False, need_preferred_email=False, | 1643 | need_location=False, need_archive=False, need_preferred_email=False, |
321 | 1582 | need_validity=False): | 1644 | need_validity=False): |
322 | @@ -1606,6 +1668,7 @@ | |||
323 | 1606 | # But not the team itself. | 1668 | # But not the team itself. |
324 | 1607 | TeamParticipation.person != self.id) | 1669 | TeamParticipation.person != self.id) |
325 | 1608 | columns = [Person] | 1670 | columns = [Person] |
326 | 1671 | decorators = [] | ||
327 | 1609 | if need_karma: | 1672 | if need_karma: |
328 | 1610 | # New people have no karmatotalcache rows. | 1673 | # New people have no karmatotalcache rows. |
329 | 1611 | origin.append( | 1674 | origin.append( |
330 | @@ -1636,7 +1699,7 @@ | |||
331 | 1636 | Archive.owner == Person.id, Archive), | 1699 | Archive.owner == Person.id, Archive), |
332 | 1637 | Archive.purpose == ArchivePurpose.PPA))) | 1700 | Archive.purpose == ArchivePurpose.PPA))) |
333 | 1638 | # checking validity requires having a preferred email. | 1701 | # checking validity requires having a preferred email. |
335 | 1639 | if need_preferred_email or need_validity: | 1702 | if need_preferred_email and not need_validity: |
336 | 1640 | # Teams don't have email, so a left join | 1703 | # Teams don't have email, so a left join |
337 | 1641 | origin.append( | 1704 | origin.append( |
338 | 1642 | LeftJoin(EmailAddress, EmailAddress.person == Person.id)) | 1705 | LeftJoin(EmailAddress, EmailAddress.person == Person.id)) |
339 | @@ -1645,14 +1708,10 @@ | |||
340 | 1645 | Or(EmailAddress.status == None, | 1708 | Or(EmailAddress.status == None, |
341 | 1646 | EmailAddress.status == EmailAddressStatus.PREFERRED)) | 1709 | EmailAddress.status == EmailAddressStatus.PREFERRED)) |
342 | 1647 | if need_validity: | 1710 | if need_validity: |
351 | 1648 | # May find teams (teams are not valid people) | 1711 | valid_stuff = Person._validity_queries() |
352 | 1649 | origin.append( | 1712 | origin.extend(valid_stuff["joins"]) |
353 | 1650 | LeftJoin(Account, Person.account == Account.id)) | 1713 | columns.extend(valid_stuff["tables"]) |
354 | 1651 | columns.append(Account) | 1714 | decorators.extend(valid_stuff["decorators"]) |
347 | 1652 | conditions = And(conditions, | ||
348 | 1653 | Or( | ||
349 | 1654 | Account.status == None, | ||
350 | 1655 | Account.status == AccountStatus.ACTIVE)) | ||
355 | 1656 | if len(columns) == 1: | 1715 | if len(columns) == 1: |
356 | 1657 | columns = columns[0] | 1716 | columns = columns[0] |
357 | 1658 | # Return a simple ResultSet | 1717 | # Return a simple ResultSet |
358 | @@ -1689,20 +1748,14 @@ | |||
359 | 1689 | index += 1 | 1748 | index += 1 |
360 | 1690 | cache_property(result, '_archive_cached', archive) | 1749 | cache_property(result, '_archive_cached', archive) |
361 | 1691 | #-- preferred email caching | 1750 | #-- preferred email caching |
363 | 1692 | if need_preferred_email: | 1751 | if need_preferred_email and not need_validity: |
364 | 1693 | email = row[index] | 1752 | email = row[index] |
365 | 1694 | index += 1 | 1753 | index += 1 |
366 | 1695 | cache_property(result, '_preferredemail_cached', email) | 1754 | cache_property(result, '_preferredemail_cached', email) |
375 | 1696 | #-- validity caching | 1755 | for decorator in decorators: |
376 | 1697 | if need_validity: | 1756 | column = row[index] |
369 | 1698 | # valid if: | ||
370 | 1699 | valid = ( | ||
371 | 1700 | # -- valid account found | ||
372 | 1701 | row[index] is not None | ||
373 | 1702 | # -- preferred email found | ||
374 | 1703 | and result.preferredemail is not None) | ||
377 | 1704 | index += 1 | 1757 | index += 1 |
379 | 1705 | cache_property(result, '_is_valid_person_cached', valid) | 1758 | decorator(result, column) |
380 | 1706 | return result | 1759 | return result |
381 | 1707 | return DecoratedResultSet(raw_result, | 1760 | return DecoratedResultSet(raw_result, |
382 | 1708 | result_decorator=prepopulate_person) | 1761 | result_decorator=prepopulate_person) |
383 | 1709 | 1762 | ||
384 | === modified file 'lib/lp/registry/model/product.py' | |||
385 | --- lib/lp/registry/model/product.py 2010-08-23 18:03:26 +0000 | |||
386 | +++ lib/lp/registry/model/product.py 2010-08-31 19:38:45 +0000 | |||
387 | @@ -1033,13 +1033,7 @@ | |||
388 | 1033 | # defaults for informationalness: we don't have to do anything | 1033 | # defaults for informationalness: we don't have to do anything |
389 | 1034 | # because the default if nothing is said is ANY | 1034 | # because the default if nothing is said is ANY |
390 | 1035 | 1035 | ||
398 | 1036 | # sort by priority descending, by default | 1036 | order = self._specification_sort(sort) |
392 | 1037 | if sort is None or sort == SpecificationSort.PRIORITY: | ||
393 | 1038 | order = ( | ||
394 | 1039 | ['-priority', 'Specification.definition_status', | ||
395 | 1040 | 'Specification.name']) | ||
396 | 1041 | elif sort == SpecificationSort.DATE: | ||
397 | 1042 | order = ['-Specification.datecreated', 'Specification.id'] | ||
399 | 1043 | 1037 | ||
400 | 1044 | # figure out what set of specifications we are interested in. for | 1038 | # figure out what set of specifications we are interested in. for |
401 | 1045 | # products, we need to be able to filter on the basis of: | 1039 | # products, we need to be able to filter on the basis of: |
402 | @@ -1082,9 +1076,15 @@ | |||
403 | 1082 | query += ' AND Specification.fti @@ ftq(%s) ' % quote( | 1076 | query += ' AND Specification.fti @@ ftq(%s) ' % quote( |
404 | 1083 | constraint) | 1077 | constraint) |
405 | 1084 | 1078 | ||
406 | 1085 | results = Specification.select(query, orderBy=order, limit=quantity) | ||
407 | 1086 | if prejoin_people: | 1079 | if prejoin_people: |
409 | 1087 | results = results.prejoin(['assignee', 'approver', 'drafter']) | 1080 | results = self._preload_specifications_people(query) |
410 | 1081 | else: | ||
411 | 1082 | results = Store.of(self).find( | ||
412 | 1083 | Specification, | ||
413 | 1084 | SQL(query)) | ||
414 | 1085 | results.order_by(order) | ||
415 | 1086 | if quantity is not None: | ||
416 | 1087 | results = results[:quantity] | ||
417 | 1088 | return results | 1088 | return results |
418 | 1089 | 1089 | ||
419 | 1090 | def getSpecification(self, name): | 1090 | def getSpecification(self, name): |
Looks great Robert.
I've got a few questions below, more for my own understanding. And you said you'd update _validity_queries() to return a dict or namedtuple.
Thanks!
> === modified file 'lib/lp/ blueprints/ browser/ tests/test_ views.py' blueprints/ browser/ tests/test_ views.py 2010-08-20 20:31:18 +0000 blueprints/ browser/ tests/test_ views.py 2010-08-31 07:46:51 +0000 launchpad. testing. systemdocs import ( uite, launchpad. webapp import canonical_url nalLayer dirname( os.path. realpath( __file_ _)) tory, sampledata import ADMIN_EMAIL _webservice import QueryCollector
> --- lib/lp/
> +++ lib/lp/
> @@ -9,19 +9,92 @@
> import os
> import unittest
>
> +from storm.store import Store
> +from testtools.matchers import LessThan
> +
> from canonical.
> LayeredDocFileS
> setUp,
> tearDown,
> )
> +from canonical.
> from canonical.testing import DatabaseFunctio
> -
> -
> -here = os.path.
> -
> +from lp.testing import (
> + login,
> + logout,
> + TestCaseWithFac
> + )
> +from lp.testing.matchers import HasQueryCount
> +from lp.testing.
> +from lp.testing.
Should this be exported from lp.testing. __init_ _.py instead of importing from
_webservice?
> + (TestCaseWithFa ctory): nalLayer and_render( self, browser, dbobj, url): counts_ scaling_ with_unique_ people( self,
> +
> +class TestAssignments
> +
> + layer = DatabaseFunctio
> +
> + def invalidate_
> + # Ensure caches have been flushed.
> + store = Store.of(dbobj)
> + store.flush()
> + store.invalidate()
> + browser.open(url)
> +
> + def check_query_
> + target, targettype):
> + """Check that a particular hasSpecifications target scales well.
> +
> + :param target: A spec target like a product.
> + :param targettype: The parameter to pass to makeSpecification to
> + associate the target. e.g. 'product'.
> + """
> + query_baseline = 40
> + people = []
> + for _ in range(10):
Up to you, but JFYI, our style guide says:
Single character names should be avoided for both variables and methods. Even
list comprehensions and generator expressions should not use them.
> + people. append( self.factory. makePerson( ))
> + specs = []
> + for _ in range(10):
ditto.
> + specs.append( self.factory. makeSpecificati on( target} )) register( ) (collector. unregister) makePerson( password= "test") wser(user= viewer) url(target) + "/+assignments" launchpad. webapp. serssion: _get_secret. _and_render( browser, target, url) (collector, HasQueryCount( LessThan( query_baseline) ))
> + **{targettype:
> + collector = QueryCollector()
> + collector.
> + self.addCleanup
> + viewer = self.factory.
> + browser = self.getUserBro
> + url = canonical_
> + # Seed the cookie cache and any other cross-request state we may gain
> + # in future. See canonical.
> + browser.open(url)
> + self.invalidate
> + # Set a baseline
> + self.assertThat
> + no_assignees_count = collector.count
> + # Assign many unique people, which shouldn't change the page queries.
> + # Due to storm bug 619017 additional...