Merge lp:~lifeless/launchpad/bug-736008 into lp:launchpad
- bug-736008
- Merge into devel
Proposed by
Robert Collins
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Robert Collins | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | 12681 | ||||
Proposed branch: | lp:~lifeless/launchpad/bug-736008 | ||||
Merge into: | lp:launchpad | ||||
Diff against target: |
421 lines (+169/-55) 3 files modified
lib/lp/code/model/branchcollection.py (+166/-52) lib/lp/code/model/tests/test_branchcollection.py (+2/-2) versions.cfg (+1/-1) |
||||
To merge this branch: | bzr merge lp:~lifeless/launchpad/bug-736008 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Steve Kowalik (community) | Approve | ||
Review via email: mp+55028@code.launchpad.net |
Commit message
[r=stevenk][bug=736008] Another optimisation attempt for Product:+code-index branch merge proposal counting
Description of the change
Take 3 at fixing this bug: separate clauses that should apply to both source and target branches and those for just the source branch. Then use with to tell sql we're looking for common constraints on both source and target. This lowers the reported cost from 50K to 20K even for launchpad, on qastaging.
There is a new storm needed as part of this - I hadn't made select statements usable in WITH clauses before.
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/code/model/branchcollection.py' | |||
2 | --- lib/lp/code/model/branchcollection.py 2011-03-28 20:54:50 +0000 | |||
3 | +++ lib/lp/code/model/branchcollection.py 2011-03-28 22:49:01 +0000 | |||
4 | @@ -19,7 +19,9 @@ | |||
5 | 19 | LeftJoin, | 19 | LeftJoin, |
6 | 20 | Or, | 20 | Or, |
7 | 21 | Select, | 21 | Select, |
8 | 22 | SQL, | ||
9 | 22 | Union, | 23 | Union, |
10 | 24 | With, | ||
11 | 23 | ) | 25 | ) |
12 | 24 | from storm.info import ClassAlias | 26 | from storm.info import ClassAlias |
13 | 25 | from zope.component import getUtility | 27 | from zope.component import getUtility |
14 | @@ -81,7 +83,8 @@ | |||
15 | 81 | implements(IBranchCollection) | 83 | implements(IBranchCollection) |
16 | 82 | 84 | ||
17 | 83 | def __init__(self, store=None, branch_filter_expressions=None, | 85 | def __init__(self, store=None, branch_filter_expressions=None, |
19 | 84 | tables=None, exclude_from_search=None): | 86 | tables=None, exclude_from_search=None, |
20 | 87 | asymmetric_filter_expressions=None, asymmetric_tables=None): | ||
21 | 85 | """Construct a `GenericBranchCollection`. | 88 | """Construct a `GenericBranchCollection`. |
22 | 86 | 89 | ||
23 | 87 | :param store: The store to look in for branches. If not specified, | 90 | :param store: The store to look in for branches. If not specified, |
24 | @@ -93,14 +96,25 @@ | |||
25 | 93 | :param tables: A dict of Storm tables to the Join expression. If an | 96 | :param tables: A dict of Storm tables to the Join expression. If an |
26 | 94 | expression in branch_filter_expressions refers to a table, then | 97 | expression in branch_filter_expressions refers to a table, then |
27 | 95 | that table *must* be in this list. | 98 | that table *must* be in this list. |
28 | 99 | :param asymmetric_filter_expressions: As per branch_filter_expressions | ||
29 | 100 | but only applies to one side of reflexive joins. | ||
30 | 101 | :param asymmetric_tables: As per tables, for | ||
31 | 102 | asymmetric_filter_expressions. | ||
32 | 96 | """ | 103 | """ |
33 | 97 | self._store = store | 104 | self._store = store |
34 | 98 | if branch_filter_expressions is None: | 105 | if branch_filter_expressions is None: |
35 | 99 | branch_filter_expressions = [] | 106 | branch_filter_expressions = [] |
37 | 100 | self._branch_filter_expressions = branch_filter_expressions | 107 | self._branch_filter_expressions = list(branch_filter_expressions) |
38 | 101 | if tables is None: | 108 | if tables is None: |
39 | 102 | tables = {} | 109 | tables = {} |
40 | 103 | self._tables = tables | 110 | self._tables = tables |
41 | 111 | if asymmetric_filter_expressions is None: | ||
42 | 112 | asymmetric_filter_expressions = [] | ||
43 | 113 | self._asymmetric_filter_expressions = list( | ||
44 | 114 | asymmetric_filter_expressions) | ||
45 | 115 | if asymmetric_tables is None: | ||
46 | 116 | asymmetric_tables = {} | ||
47 | 117 | self._asymmetric_tables = asymmetric_tables | ||
48 | 104 | if exclude_from_search is None: | 118 | if exclude_from_search is None: |
49 | 105 | exclude_from_search = [] | 119 | exclude_from_search = [] |
50 | 106 | self._exclude_from_search = exclude_from_search | 120 | self._exclude_from_search = exclude_from_search |
51 | @@ -132,25 +146,42 @@ | |||
52 | 132 | return self._store | 146 | return self._store |
53 | 133 | 147 | ||
54 | 134 | def _filterBy(self, expressions, table=None, join=None, | 148 | def _filterBy(self, expressions, table=None, join=None, |
57 | 135 | exclude_from_search=None): | 149 | exclude_from_search=None, symmetric=True): |
58 | 136 | """Return a subset of this collection, filtered by 'expressions'.""" | 150 | """Return a subset of this collection, filtered by 'expressions'. |
59 | 151 | |||
60 | 152 | :param symmetric: If True this filter will apply to both sides of merge | ||
61 | 153 | proposal lookups and any other lookups that join Branch back onto | ||
62 | 154 | Branch. | ||
63 | 155 | """ | ||
64 | 137 | # NOTE: JonathanLange 2009-02-17: We might be able to avoid the need | 156 | # NOTE: JonathanLange 2009-02-17: We might be able to avoid the need |
65 | 138 | # for explicit 'tables' by harnessing Storm's table inference system. | 157 | # for explicit 'tables' by harnessing Storm's table inference system. |
66 | 139 | # See http://paste.ubuntu.com/118711/ for one way to do that. | 158 | # See http://paste.ubuntu.com/118711/ for one way to do that. |
67 | 140 | tables = self._tables.copy() | ||
68 | 141 | if table is not None: | 159 | if table is not None: |
69 | 142 | if join is None: | 160 | if join is None: |
70 | 143 | raise InvalidFilter("Cannot specify a table without a join.") | 161 | raise InvalidFilter("Cannot specify a table without a join.") |
72 | 144 | tables[table] = join | 162 | if expressions is None: |
73 | 163 | expressions = [] | ||
74 | 164 | tables = self._tables.copy() | ||
75 | 165 | asymmetric_tables = self._asymmetric_tables.copy() | ||
76 | 166 | if symmetric: | ||
77 | 167 | if table is not None: | ||
78 | 168 | tables[table] = join | ||
79 | 169 | symmetric_expr = self._branch_filter_expressions + expressions | ||
80 | 170 | asymmetric_expr = list(self._asymmetric_filter_expressions) | ||
81 | 171 | else: | ||
82 | 172 | if table is not None: | ||
83 | 173 | asymmetric_tables[table] = join | ||
84 | 174 | symmetric_expr = list(self._branch_filter_expressions) | ||
85 | 175 | asymmetric_expr = self._asymmetric_filter_expressions + expressions | ||
86 | 145 | if exclude_from_search is None: | 176 | if exclude_from_search is None: |
87 | 146 | exclude_from_search = [] | 177 | exclude_from_search = [] |
88 | 147 | if expressions is None: | ||
89 | 148 | expressions = [] | ||
90 | 149 | return self.__class__( | 178 | return self.__class__( |
91 | 150 | self.store, | 179 | self.store, |
93 | 151 | self._branch_filter_expressions + expressions, | 180 | symmetric_expr, |
94 | 152 | tables, | 181 | tables, |
96 | 153 | self._exclude_from_search + exclude_from_search) | 182 | self._exclude_from_search + exclude_from_search, |
97 | 183 | asymmetric_expr, | ||
98 | 184 | asymmetric_tables) | ||
99 | 154 | 185 | ||
100 | 155 | def _getBranchIdQuery(self): | 186 | def _getBranchIdQuery(self): |
101 | 156 | """Return a Storm 'Select' for the branch IDs in this collection.""" | 187 | """Return a Storm 'Select' for the branch IDs in this collection.""" |
102 | @@ -160,16 +191,28 @@ | |||
103 | 160 | 191 | ||
104 | 161 | def _getBranchExpressions(self): | 192 | def _getBranchExpressions(self): |
105 | 162 | """Return the where expressions for this collection.""" | 193 | """Return the where expressions for this collection.""" |
108 | 163 | return (self._branch_filter_expressions | 194 | return (self._branch_filter_expressions + |
109 | 164 | + self._getBranchVisibilityExpression()) | 195 | self._asymmetric_filter_expressions + |
110 | 196 | self._getBranchVisibilityExpression()) | ||
111 | 165 | 197 | ||
112 | 166 | def _getBranchVisibilityExpression(self, branch_class=None): | 198 | def _getBranchVisibilityExpression(self, branch_class=None): |
113 | 167 | """Return the where clauses for visibility.""" | 199 | """Return the where clauses for visibility.""" |
114 | 168 | return [] | 200 | return [] |
115 | 169 | 201 | ||
116 | 202 | def _getCandidateBranchesWith(self): | ||
117 | 203 | """Return WITH clauses defining candidate branches. | ||
118 | 204 | |||
119 | 205 | These are defined in terms of scope_branches which should be separately | ||
120 | 206 | calculated. | ||
121 | 207 | """ | ||
122 | 208 | return [ | ||
123 | 209 | With("candidate_branches", SQL("SELECT id from scope_branches"))] | ||
124 | 210 | |||
125 | 170 | def getBranches(self, eager_load=False): | 211 | def getBranches(self, eager_load=False): |
126 | 171 | """See `IBranchCollection`.""" | 212 | """See `IBranchCollection`.""" |
128 | 172 | tables = [Branch] + self._tables.values() | 213 | all_tables = set( |
129 | 214 | self._tables.values() + self._asymmetric_tables.values()) | ||
130 | 215 | tables = [Branch] + list(all_tables) | ||
131 | 173 | expressions = self._getBranchExpressions() | 216 | expressions = self._getBranchExpressions() |
132 | 174 | resultset = self.store.using(*tables).find(Branch, *expressions) | 217 | resultset = self.store.using(*tables).find(Branch, *expressions) |
133 | 175 | if not eager_load: | 218 | if not eager_load: |
134 | @@ -221,15 +264,25 @@ | |||
135 | 221 | def getMergeProposals(self, statuses=None, for_branches=None, | 264 | def getMergeProposals(self, statuses=None, for_branches=None, |
136 | 222 | target_branch=None, merged_revnos=None): | 265 | target_branch=None, merged_revnos=None): |
137 | 223 | """See `IBranchCollection`.""" | 266 | """See `IBranchCollection`.""" |
147 | 224 | Target = ClassAlias(Branch, "target") | 267 | # teams = SQL("teams as (SELECT team from teamparticipation where person=%s)" % sqlvalues |
148 | 225 | tables = [Branch] + self._tables.values() + [ | 268 | scope_tables = [Branch] + self._tables.values() |
149 | 226 | Join(BranchMergeProposal, And( | 269 | scope_expressions = self._branch_filter_expressions |
150 | 227 | Branch.id==BranchMergeProposal.source_branchID, | 270 | select = self.store.using(*scope_tables).find( |
151 | 228 | *self._branch_filter_expressions)), | 271 | (Branch.id, Branch.private, Branch.ownerID), *scope_expressions) |
152 | 229 | Join(Target, Target.id==BranchMergeProposal.target_branchID) | 272 | branches_query = select._get_select() |
153 | 230 | ] | 273 | with_expr = [With("scope_branches", branches_query) |
154 | 231 | expressions = self._getBranchVisibilityExpression() | 274 | ] + self._getCandidateBranchesWith() |
155 | 232 | expressions.extend(self._getBranchVisibilityExpression(Target)) | 275 | expressions = [SQL(""" |
156 | 276 | source_branch IN (SELECT id FROM candidate_branches) AND | ||
157 | 277 | target_branch IN (SELECT id FROM candidate_branches)""")] | ||
158 | 278 | tables = [BranchMergeProposal] | ||
159 | 279 | if self._asymmetric_filter_expressions: | ||
160 | 280 | # Need to filter on Branch beyond the with constraints. | ||
161 | 281 | expressions += self._asymmetric_filter_expressions | ||
162 | 282 | expressions.append( | ||
163 | 283 | BranchMergeProposal.source_branchID == Branch.id) | ||
164 | 284 | tables.append(Branch) | ||
165 | 285 | tables.extend(self._asymmetric_tables.values()) | ||
166 | 233 | if for_branches is not None: | 286 | if for_branches is not None: |
167 | 234 | branch_ids = [branch.id for branch in for_branches] | 287 | branch_ids = [branch.id for branch in for_branches] |
168 | 235 | expressions.append( | 288 | expressions.append( |
169 | @@ -243,7 +296,8 @@ | |||
170 | 243 | if statuses is not None: | 296 | if statuses is not None: |
171 | 244 | expressions.append( | 297 | expressions.append( |
172 | 245 | BranchMergeProposal.queue_status.is_in(statuses)) | 298 | BranchMergeProposal.queue_status.is_in(statuses)) |
174 | 246 | return self.store.using(*tables).find(BranchMergeProposal, *expressions) | 299 | return self.store.with_(with_expr).using(*tables).find( |
175 | 300 | BranchMergeProposal, *expressions) | ||
176 | 247 | 301 | ||
177 | 248 | def getMergeProposalsForPerson(self, person, status=None): | 302 | def getMergeProposalsForPerson(self, person, status=None): |
178 | 249 | """See `IBranchCollection`.""" | 303 | """See `IBranchCollection`.""" |
179 | @@ -420,7 +474,7 @@ | |||
180 | 420 | 474 | ||
181 | 421 | def ownedBy(self, person): | 475 | def ownedBy(self, person): |
182 | 422 | """See `IBranchCollection`.""" | 476 | """See `IBranchCollection`.""" |
184 | 423 | return self._filterBy([Branch.owner == person]) | 477 | return self._filterBy([Branch.owner == person], symmetric=False) |
185 | 424 | 478 | ||
186 | 425 | def ownedByTeamMember(self, person): | 479 | def ownedByTeamMember(self, person): |
187 | 426 | """See `IBranchCollection`.""" | 480 | """See `IBranchCollection`.""" |
188 | @@ -429,11 +483,11 @@ | |||
189 | 429 | where=TeamParticipation.personID==person.id) | 483 | where=TeamParticipation.personID==person.id) |
190 | 430 | filter = [In(Branch.ownerID, subquery)] | 484 | filter = [In(Branch.ownerID, subquery)] |
191 | 431 | 485 | ||
193 | 432 | return self._filterBy(filter) | 486 | return self._filterBy(filter, symmetric=False) |
194 | 433 | 487 | ||
195 | 434 | def registeredBy(self, person): | 488 | def registeredBy(self, person): |
196 | 435 | """See `IBranchCollection`.""" | 489 | """See `IBranchCollection`.""" |
198 | 436 | return self._filterBy([Branch.registrant == person]) | 490 | return self._filterBy([Branch.registrant == person], symmetric=False) |
199 | 437 | 491 | ||
200 | 438 | def relatedTo(self, person): | 492 | def relatedTo(self, person): |
201 | 439 | """See `IBranchCollection`.""" | 493 | """See `IBranchCollection`.""" |
202 | @@ -444,7 +498,8 @@ | |||
203 | 444 | Select(Branch.id, Branch.registrant == person), | 498 | Select(Branch.id, Branch.registrant == person), |
204 | 445 | Select(Branch.id, | 499 | Select(Branch.id, |
205 | 446 | And(BranchSubscription.person == person, | 500 | And(BranchSubscription.person == person, |
207 | 447 | BranchSubscription.branch == Branch.id))))]) | 501 | BranchSubscription.branch == Branch.id))))], |
208 | 502 | symmetric=False) | ||
209 | 448 | 503 | ||
210 | 449 | def _getExactMatch(self, search_term): | 504 | def _getExactMatch(self, search_term): |
211 | 450 | """Return the exact branch that 'search_term' matches, or None.""" | 505 | """Return the exact branch that 'search_term' matches, or None.""" |
212 | @@ -510,7 +565,8 @@ | |||
213 | 510 | [BranchSubscription.person == person], | 565 | [BranchSubscription.person == person], |
214 | 511 | table=BranchSubscription, | 566 | table=BranchSubscription, |
215 | 512 | join=Join(BranchSubscription, | 567 | join=Join(BranchSubscription, |
217 | 513 | BranchSubscription.branch == Branch.id)) | 568 | BranchSubscription.branch == Branch.id), |
218 | 569 | symmetric=False) | ||
219 | 514 | 570 | ||
220 | 515 | def targetedBy(self, person, since=None): | 571 | def targetedBy(self, person, since=None): |
221 | 516 | """See `IBranchCollection`.""" | 572 | """See `IBranchCollection`.""" |
222 | @@ -521,7 +577,8 @@ | |||
223 | 521 | clauses, | 577 | clauses, |
224 | 522 | table=BranchMergeProposal, | 578 | table=BranchMergeProposal, |
225 | 523 | join=Join(BranchMergeProposal, | 579 | join=Join(BranchMergeProposal, |
227 | 524 | BranchMergeProposal.target_branch == Branch.id)) | 580 | BranchMergeProposal.target_branch == Branch.id), |
228 | 581 | symmetric=False) | ||
229 | 525 | 582 | ||
230 | 526 | def visibleByUser(self, person): | 583 | def visibleByUser(self, person): |
231 | 527 | """See `IBranchCollection`.""" | 584 | """See `IBranchCollection`.""" |
232 | @@ -531,74 +588,104 @@ | |||
233 | 531 | if person is None: | 588 | if person is None: |
234 | 532 | return AnonymousBranchCollection( | 589 | return AnonymousBranchCollection( |
235 | 533 | self._store, self._branch_filter_expressions, | 590 | self._store, self._branch_filter_expressions, |
237 | 534 | self._tables, self._exclude_from_search) | 591 | self._tables, self._exclude_from_search, |
238 | 592 | self._asymmetric_filter_expressions, self._asymmetric_tables) | ||
239 | 535 | return VisibleBranchCollection( | 593 | return VisibleBranchCollection( |
240 | 536 | person, self._store, self._branch_filter_expressions, | 594 | person, self._store, self._branch_filter_expressions, |
242 | 537 | self._tables, self._exclude_from_search) | 595 | self._tables, self._exclude_from_search, |
243 | 596 | self._asymmetric_filter_expressions, self._asymmetric_tables) | ||
244 | 538 | 597 | ||
245 | 539 | def withBranchType(self, *branch_types): | 598 | def withBranchType(self, *branch_types): |
247 | 540 | return self._filterBy([Branch.branch_type.is_in(branch_types)]) | 599 | return self._filterBy([Branch.branch_type.is_in(branch_types)], |
248 | 600 | symmetric=False) | ||
249 | 541 | 601 | ||
250 | 542 | def withLifecycleStatus(self, *statuses): | 602 | def withLifecycleStatus(self, *statuses): |
251 | 543 | """See `IBranchCollection`.""" | 603 | """See `IBranchCollection`.""" |
253 | 544 | return self._filterBy([Branch.lifecycle_status.is_in(statuses)]) | 604 | return self._filterBy([Branch.lifecycle_status.is_in(statuses)], |
254 | 605 | symmetric=False) | ||
255 | 545 | 606 | ||
256 | 546 | def modifiedSince(self, epoch): | 607 | def modifiedSince(self, epoch): |
257 | 547 | """See `IBranchCollection`.""" | 608 | """See `IBranchCollection`.""" |
259 | 548 | return self._filterBy([Branch.date_last_modified > epoch]) | 609 | return self._filterBy([Branch.date_last_modified > epoch], |
260 | 610 | symmetric=False) | ||
261 | 549 | 611 | ||
262 | 550 | def scannedSince(self, epoch): | 612 | def scannedSince(self, epoch): |
263 | 551 | """See `IBranchCollection`.""" | 613 | """See `IBranchCollection`.""" |
265 | 552 | return self._filterBy([Branch.last_scanned > epoch]) | 614 | return self._filterBy([Branch.last_scanned > epoch], symmetric=False) |
266 | 553 | 615 | ||
267 | 554 | 616 | ||
268 | 555 | class AnonymousBranchCollection(GenericBranchCollection): | 617 | class AnonymousBranchCollection(GenericBranchCollection): |
269 | 556 | """Branch collection that only shows public branches.""" | 618 | """Branch collection that only shows public branches.""" |
270 | 557 | 619 | ||
271 | 558 | def __init__(self, store=None, branch_filter_expressions=None, | ||
272 | 559 | tables=None, exclude_from_search=None): | ||
273 | 560 | super(AnonymousBranchCollection, self).__init__( | ||
274 | 561 | store=store, | ||
275 | 562 | branch_filter_expressions=list(branch_filter_expressions), | ||
276 | 563 | tables=tables, exclude_from_search=exclude_from_search) | ||
277 | 564 | |||
278 | 565 | def _getBranchVisibilityExpression(self, branch_class=Branch): | 620 | def _getBranchVisibilityExpression(self, branch_class=Branch): |
279 | 566 | """Return the where clauses for visibility.""" | 621 | """Return the where clauses for visibility.""" |
280 | 567 | return [branch_class.private == False] | 622 | return [branch_class.private == False] |
281 | 568 | 623 | ||
282 | 624 | def _getCandidateBranchesWith(self): | ||
283 | 625 | """Return WITH clauses defining candidate branches. | ||
284 | 626 | |||
285 | 627 | These are defined in terms of scope_branches which should be separately | ||
286 | 628 | calculated. | ||
287 | 629 | """ | ||
288 | 630 | # Anonymous users get public branches only. | ||
289 | 631 | return [ | ||
290 | 632 | With("candidate_branches", | ||
291 | 633 | SQL("select id from scope_branches where not private")) | ||
292 | 634 | ] | ||
293 | 635 | |||
294 | 569 | 636 | ||
295 | 570 | class VisibleBranchCollection(GenericBranchCollection): | 637 | class VisibleBranchCollection(GenericBranchCollection): |
296 | 571 | """A branch collection that has special logic for visibility.""" | 638 | """A branch collection that has special logic for visibility.""" |
297 | 572 | 639 | ||
298 | 573 | def __init__(self, user, store=None, branch_filter_expressions=None, | 640 | def __init__(self, user, store=None, branch_filter_expressions=None, |
300 | 574 | tables=None, exclude_from_search=None): | 641 | tables=None, exclude_from_search=None, |
301 | 642 | asymmetric_filter_expressions=None, asymmetric_tables=None): | ||
302 | 575 | super(VisibleBranchCollection, self).__init__( | 643 | super(VisibleBranchCollection, self).__init__( |
303 | 576 | store=store, branch_filter_expressions=branch_filter_expressions, | 644 | store=store, branch_filter_expressions=branch_filter_expressions, |
305 | 577 | tables=tables, exclude_from_search=exclude_from_search) | 645 | tables=tables, exclude_from_search=exclude_from_search, |
306 | 646 | asymmetric_filter_expressions=asymmetric_filter_expressions, | ||
307 | 647 | asymmetric_tables=asymmetric_tables) | ||
308 | 578 | self._user = user | 648 | self._user = user |
309 | 579 | self._private_branch_ids = self._getPrivateBranchSubQuery() | 649 | self._private_branch_ids = self._getPrivateBranchSubQuery() |
310 | 580 | 650 | ||
311 | 581 | def _filterBy(self, expressions, table=None, join=None, | 651 | def _filterBy(self, expressions, table=None, join=None, |
314 | 582 | exclude_from_search=None): | 652 | exclude_from_search=None, symmetric=True): |
315 | 583 | """Return a subset of this collection, filtered by 'expressions'.""" | 653 | """Return a subset of this collection, filtered by 'expressions'. |
316 | 654 | |||
317 | 655 | :param symmetric: If True this filter will apply to both sides of merge | ||
318 | 656 | proposal lookups and any other lookups that join Branch back onto | ||
319 | 657 | Branch. | ||
320 | 658 | """ | ||
321 | 584 | # NOTE: JonathanLange 2009-02-17: We might be able to avoid the need | 659 | # NOTE: JonathanLange 2009-02-17: We might be able to avoid the need |
322 | 585 | # for explicit 'tables' by harnessing Storm's table inference system. | 660 | # for explicit 'tables' by harnessing Storm's table inference system. |
323 | 586 | # See http://paste.ubuntu.com/118711/ for one way to do that. | 661 | # See http://paste.ubuntu.com/118711/ for one way to do that. |
324 | 587 | tables = self._tables.copy() | ||
325 | 588 | if table is not None: | 662 | if table is not None: |
326 | 589 | if join is None: | 663 | if join is None: |
327 | 590 | raise InvalidFilter("Cannot specify a table without a join.") | 664 | raise InvalidFilter("Cannot specify a table without a join.") |
329 | 591 | tables[table] = join | 665 | if expressions is None: |
330 | 666 | expressions = [] | ||
331 | 667 | tables = self._tables.copy() | ||
332 | 668 | asymmetric_tables = self._asymmetric_tables.copy() | ||
333 | 669 | if symmetric: | ||
334 | 670 | if table is not None: | ||
335 | 671 | tables[table] = join | ||
336 | 672 | symmetric_expr = self._branch_filter_expressions + expressions | ||
337 | 673 | asymmetric_expr = list(self._asymmetric_filter_expressions) | ||
338 | 674 | else: | ||
339 | 675 | if table is not None: | ||
340 | 676 | asymmetric_tables[table] = join | ||
341 | 677 | symmetric_expr = list(self._branch_filter_expressions) | ||
342 | 678 | asymmetric_expr = self._asymmetric_filter_expressions + expressions | ||
343 | 592 | if exclude_from_search is None: | 679 | if exclude_from_search is None: |
344 | 593 | exclude_from_search = [] | 680 | exclude_from_search = [] |
345 | 594 | if expressions is None: | ||
346 | 595 | expressions = [] | ||
347 | 596 | return self.__class__( | 681 | return self.__class__( |
348 | 597 | self._user, | 682 | self._user, |
349 | 598 | self.store, | 683 | self.store, |
351 | 599 | self._branch_filter_expressions + expressions, | 684 | symmetric_expr, |
352 | 600 | tables, | 685 | tables, |
354 | 601 | self._exclude_from_search + exclude_from_search) | 686 | self._exclude_from_search + exclude_from_search, |
355 | 687 | asymmetric_expr, | ||
356 | 688 | asymmetric_tables) | ||
357 | 602 | 689 | ||
358 | 603 | def _getPrivateBranchSubQuery(self): | 690 | def _getPrivateBranchSubQuery(self): |
359 | 604 | """Return a subquery to get the private branches the user can see. | 691 | """Return a subquery to get the private branches the user can see. |
360 | @@ -649,6 +736,33 @@ | |||
361 | 649 | branch_class.id.is_in(self._private_branch_ids)) | 736 | branch_class.id.is_in(self._private_branch_ids)) |
362 | 650 | return [public_or_private] | 737 | return [public_or_private] |
363 | 651 | 738 | ||
364 | 739 | def _getCandidateBranchesWith(self): | ||
365 | 740 | """Return WITH clauses defining candidate branches. | ||
366 | 741 | |||
367 | 742 | These are defined in terms of scope_branches which should be separately | ||
368 | 743 | calculated. | ||
369 | 744 | """ | ||
370 | 745 | person = self._user | ||
371 | 746 | if person is None: | ||
372 | 747 | # Really an anonymous sitation | ||
373 | 748 | return [ | ||
374 | 749 | With("candidate_branches", | ||
375 | 750 | SQL("select id from scope_branches where not private")) | ||
376 | 751 | ] | ||
377 | 752 | return [ | ||
378 | 753 | With("teams", self.store.find(TeamParticipation.teamID, | ||
379 | 754 | TeamParticipation.personID == person.id)._get_select()), | ||
380 | 755 | With("private_branches", SQL(""" | ||
381 | 756 | SELECT scope_branches.id FROM scope_branches WHERE | ||
382 | 757 | scope_branches.private AND ((scope_branches.owner in (select team from teams) OR | ||
383 | 758 | EXISTS(SELECT true from BranchSubscription, teams WHERE | ||
384 | 759 | branchsubscription.branch = scope_branches.id AND | ||
385 | 760 | branchsubscription.person = teams.team)))""")), | ||
386 | 761 | With("candidate_branches", SQL(""" | ||
387 | 762 | (SELECT id FROM private_branches) UNION | ||
388 | 763 | (select id FROM scope_branches WHERE not private)""")) | ||
389 | 764 | ] | ||
390 | 765 | |||
391 | 652 | def visibleByUser(self, person): | 766 | def visibleByUser(self, person): |
392 | 653 | """See `IBranchCollection`.""" | 767 | """See `IBranchCollection`.""" |
393 | 654 | if person == self._user: | 768 | if person == self._user: |
394 | 655 | 769 | ||
395 | === modified file 'lib/lp/code/model/tests/test_branchcollection.py' | |||
396 | --- lib/lp/code/model/tests/test_branchcollection.py 2011-03-07 12:43:40 +0000 | |||
397 | +++ lib/lp/code/model/tests/test_branchcollection.py 2011-03-28 22:49:01 +0000 | |||
398 | @@ -746,8 +746,8 @@ | |||
399 | 746 | 746 | ||
400 | 747 | def test_just_owned_branch_merge_proposals(self): | 747 | def test_just_owned_branch_merge_proposals(self): |
401 | 748 | # If the collection only includes branches owned by a person, the | 748 | # If the collection only includes branches owned by a person, the |
404 | 749 | # getMergeProposals() will only return merge proposals for branches | 749 | # getMergeProposals() will only return merge proposals for source |
405 | 750 | # that are owned by that person. | 750 | # branches that are owned by that person. |
406 | 751 | person = self.factory.makePerson() | 751 | person = self.factory.makePerson() |
407 | 752 | product = self.factory.makeProduct() | 752 | product = self.factory.makeProduct() |
408 | 753 | branch1 = self.factory.makeProductBranch( | 753 | branch1 = self.factory.makeProductBranch( |
409 | 754 | 754 | ||
410 | === modified file 'versions.cfg' | |||
411 | --- versions.cfg 2011-03-23 16:28:51 +0000 | |||
412 | +++ versions.cfg 2011-03-28 22:49:01 +0000 | |||
413 | @@ -72,7 +72,7 @@ | |||
414 | 72 | Sphinx = 1.0.7 | 72 | Sphinx = 1.0.7 |
415 | 73 | soupmatchers = 0.1r53 | 73 | soupmatchers = 0.1r53 |
416 | 74 | sourcecodegen = 0.6.9 | 74 | sourcecodegen = 0.6.9 |
418 | 75 | storm = 0.18.0.99-lpwithnodatetime-r389 | 75 | storm = 0.18.0.99-lpwithnodatetime-r391 |
419 | 76 | testtools = 0.9.8 | 76 | testtools = 0.9.8 |
420 | 77 | transaction = 1.0.0 | 77 | transaction = 1.0.0 |
421 | 78 | Twisted = 10.2.0-4395fix-1-4909 | 78 | Twisted = 10.2.0-4395fix-1-4909 |
After discussing my concerns with Robert on IRC, I'm happy with this branch.