Merge ~pappacena/launchpad:storm_select_related into launchpad:master
- Git
- lp:~pappacena/launchpad
- storm_select_related
- Merge into master
Status: | Needs review |
---|---|
Proposed branch: | ~pappacena/launchpad:storm_select_related |
Merge into: | launchpad:master |
Diff against target: |
902 lines (+525/-20) 12 files modified
database/schema/security.cfg (+4/-1) lib/lp/services/database/configure.zcml (+60/-0) lib/lp/services/database/select_related.py (+145/-0) lib/lp/services/database/tests/test_select_related.py (+57/-0) lib/lp/soyuz/browser/queue.py (+28/-4) lib/lp/soyuz/browser/tests/test_queue.py (+39/-4) lib/lp/soyuz/configure.zcml (+7/-1) lib/lp/soyuz/interfaces/queue.py (+32/-1) lib/lp/soyuz/model/queue.py (+79/-5) lib/lp/soyuz/stories/soyuz/xx-queue-pages.txt (+25/-1) lib/lp/soyuz/templates/distroseries-queue.pt (+10/-0) lib/lp/soyuz/tests/test_packageupload.py (+39/-3) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Launchpad code reviewers | Pending | ||
Review via email: mp+377968@code.launchpad.net |
Commit message
[DONT MERGE] Adding an easy way to select related objects on Storm (discussion only)
Description of the change
I'm creating this MP in order to have a place to discuss an initial idea. Please don't merge it, since it's probably a better idea to propose this change on Storm, if others would think this could be a good idea.
One problem we have today in LP is that, in order to prefetch related objects (reviewer Person from PackageUploadLog, for example), we are first fetching the list of the first objects (PackageUploadLog), getting the related IDs and issuing another query to fetch the second class of objects (reviewers/Person).
This way is obviously better than doing one query per PackageUploadLog, but it's still one query worst than it would be if we just LEFT JOIN PackageUploadLog with Person in this simple scenario (ignoring the other complexities of Person entity here).
Django has an easy way to solve this with something like `PackageUploadL
The idea here is to implement something similar to what we have in Django:
`resultset = store.find(
This branch already works and doesn't introduce regressions on LP (although the code is terribly ugly and not well tested; I would need to work the implementation details with Storm maintainers - if any!)
Implementing something similar to Django's `prefetch_related` (to fetch multiple objects) could be a future possibility.
Unmerged commits
- 661606c... by Thiago F. Pappacena
-
comments
- f32f75a... by Thiago F. Pappacena
-
doing the queries on the resultset
- 6f96dc5... by Thiago F. Pappacena
-
checkpoint
- 768adab... by Thiago F. Pappacena
-
kind of working with left join, bug failing test
- 81c973d... by Thiago F. Pappacena
-
working for flat joins but only when there are objects
Preview Diff
1 | diff --git a/database/schema/security.cfg b/database/schema/security.cfg |
2 | index 06da0d4..2844a30 100644 |
3 | --- a/database/schema/security.cfg |
4 | +++ b/database/schema/security.cfg |
5 | @@ -1,4 +1,4 @@ |
6 | -# Copyright 2009-2019 Canonical Ltd. This software is licensed under the |
7 | +# Copyright 2009-2020 Canonical Ltd. This software is licensed under the |
8 | # GNU Affero General Public License version 3 (see the file LICENSE). |
9 | # |
10 | # Possible permissions: SELECT, INSERT, UPDATE, EXECUTE |
11 | @@ -1080,6 +1080,7 @@ public.packagesetgroup = SELECT, INSERT |
12 | public.packagesetinclusion = SELECT, INSERT |
13 | public.packagesetsources = SELECT, INSERT |
14 | public.packageupload = SELECT, INSERT, UPDATE |
15 | +public.packageuploadlog = SELECT |
16 | public.packageuploadsource = SELECT |
17 | public.packageuploadbuild = SELECT |
18 | public.packageuploadcustom = SELECT, INSERT |
19 | @@ -1242,6 +1243,7 @@ public.ociprojectname = SELECT, INSERT, UPDATE |
20 | public.ociprojectseries = SELECT, INSERT, UPDATE, DELETE |
21 | public.openididentifier = SELECT |
22 | public.packageupload = SELECT, INSERT, UPDATE |
23 | +public.packageuploadlog = SELECT, INSERT |
24 | public.packageuploadbuild = SELECT, INSERT, UPDATE |
25 | public.packageuploadcustom = SELECT, INSERT, UPDATE |
26 | public.packageuploadsource = SELECT, INSERT, UPDATE |
27 | @@ -2293,6 +2295,7 @@ public.packagediff = SELECT, UPDATE |
28 | public.packageset = SELECT, UPDATE |
29 | public.packagesetgroup = SELECT, UPDATE |
30 | public.packageupload = SELECT, UPDATE |
31 | +public.packageuploadlog = SELECT, UPDATE |
32 | public.packaging = SELECT, UPDATE |
33 | public.person = SELECT, UPDATE |
34 | public.personlanguage = SELECT, UPDATE |
35 | diff --git a/lib/lp/services/database/configure.zcml b/lib/lp/services/database/configure.zcml |
36 | index 3795b28..d21caea 100644 |
37 | --- a/lib/lp/services/database/configure.zcml |
38 | +++ b/lib/lp/services/database/configure.zcml |
39 | @@ -17,4 +17,64 @@ |
40 | <allow attributes="__getslice__ get_plain_result_set" /> |
41 | </class> |
42 | |
43 | + <class class="lp.services.database.select_related.ResultSet"> |
44 | + <allow attributes=" |
45 | + __class__ |
46 | + __contains__ |
47 | + __delattr__ |
48 | + __dict__ |
49 | + __doc__ |
50 | + __format__ |
51 | + __getattribute__ |
52 | + __getitem__ |
53 | + __getslice__ |
54 | + __hash__ |
55 | + __implemented__ |
56 | + __init__ |
57 | + __iter__ |
58 | + __module__ |
59 | + __new__ |
60 | + __providedBy__ |
61 | + __provides__ |
62 | + __reduce__ |
63 | + __reduce_ex__ |
64 | + __repr__ |
65 | + __setattr__ |
66 | + __sizeof__ |
67 | + __str__ |
68 | + __subclasshook__ |
69 | + __weakref__ |
70 | + _aggregate |
71 | + _any |
72 | + _get_select |
73 | + _load_objects |
74 | + _set_expr |
75 | + any |
76 | + avg |
77 | + cached |
78 | + config |
79 | + copy |
80 | + count |
81 | + difference |
82 | + find |
83 | + first |
84 | + get_select_expr |
85 | + group_by |
86 | + having |
87 | + intersection |
88 | + is_empty |
89 | + last |
90 | + max |
91 | + min |
92 | + one |
93 | + order_by |
94 | + remove |
95 | + set |
96 | + sum |
97 | + union |
98 | + values |
99 | + select_related" |
100 | + /> |
101 | + </class> |
102 | + |
103 | </configure> |
104 | diff --git a/lib/lp/services/database/select_related.py b/lib/lp/services/database/select_related.py |
105 | new file mode 100644 |
106 | index 0000000..eabf546 |
107 | --- /dev/null |
108 | +++ b/lib/lp/services/database/select_related.py |
109 | @@ -0,0 +1,145 @@ |
110 | +from __future__ import absolute_import, print_function, unicode_literals |
111 | + |
112 | +__metaclass__ = type |
113 | + |
114 | +from storm import Undef |
115 | +from storm.expr import LeftJoin, compare_columns |
116 | +from storm.info import get_cls_info, ClassAlias |
117 | +from storm.properties import PropertyColumn |
118 | +from storm.store import ( |
119 | + ResultSet as StormResultSet, FindSpec as StormFindSpec, Store) |
120 | + |
121 | + |
122 | +class FindSpec(StormFindSpec): |
123 | + def __init__(self, cls_spec, *args, **kwargs): |
124 | + super(FindSpec, self).__init__(cls_spec, *args, **kwargs) |
125 | + self.cls_spec = cls_spec |
126 | + self._to_select_related = {} |
127 | + |
128 | + @staticmethod |
129 | + def tuplify(obj): |
130 | + """Transform a given object in a tuple, if it's not yet one""" |
131 | + if not isinstance(obj, tuple): |
132 | + return (obj, ) |
133 | + return obj |
134 | + |
135 | + def add_select_related(self, reference, cls_alias): |
136 | + self._to_select_related[reference] = cls_alias |
137 | + |
138 | + def filter_values_for_related(self, values, reference): |
139 | + """From the given row, filter only the values refering to the given |
140 | + reference |
141 | + |
142 | + In summary, this method returns a tuple in the same format that |
143 | + would be returned by selecting a row for only the given Reference |
144 | + attribute (or None, for the main cls_spec given) |
145 | + """ |
146 | + # If called with store.find(Column), do not try to filter anything |
147 | + skip_filter = isinstance(self.cls_spec, PropertyColumn) |
148 | + # If no select_related was asked, do not try to filter also |
149 | + skip_filter = skip_filter or not len(self._to_select_related) |
150 | + if skip_filter: |
151 | + return values |
152 | + |
153 | + cols, tables = self.get_columns_and_tables() |
154 | + if reference is None: |
155 | + table_alias = self.tuplify(self.cls_spec) |
156 | + else: |
157 | + table_alias = [self._to_select_related[reference]] |
158 | + |
159 | + target_cols = set([i for i in cols if i.table in table_alias]) |
160 | + return tuple( |
161 | + [value for col, value in zip(cols, values) |
162 | + if col in target_cols]) |
163 | + |
164 | + def load_related_objects(self, store, result, values): |
165 | + """Load into the store the objects fetched through |
166 | + ResultSet.select_related (stored here as self._to_select_related) |
167 | + """ |
168 | + for ref, cls_alias in self._to_select_related.items(): |
169 | + cls_info = get_cls_info(ref._relation.remote_cls) |
170 | + ref_values = self.filter_values_for_related(values, ref) |
171 | + try: |
172 | + store._load_object(cls_info, result, ref_values) |
173 | + except Exception, e: |
174 | + import ipdb; ipdb.set_trace() |
175 | + |
176 | + def load_objects(self, store, result, values): |
177 | + """Load into the store the objects from this FindSpec, and also the |
178 | + """ |
179 | + self.load_related_objects(store, result, values) |
180 | + cls_values = self.filter_values_for_related(values, None) |
181 | + result = super(FindSpec, self).load_objects( |
182 | + store, result, cls_values) |
183 | + |
184 | + # If the cls_spec contains spec of select_related things, exclude |
185 | + # them from the final result |
186 | + related_slice = len(self._to_select_related) |
187 | + if related_slice: |
188 | + result = result[:-related_slice] |
189 | + |
190 | + # If it only got back a tuple because of select_related, returns |
191 | + # the only object remaining (rather then a tuple) |
192 | + if len(self.cls_spec) - 1 == related_slice: |
193 | + return result[0] |
194 | + else: |
195 | + return result |
196 | + return result |
197 | + |
198 | + |
199 | +class ResultSet(StormResultSet): |
200 | + def __init__(self, *args, **kwargs): |
201 | + super(ResultSet, self).__init__(*args, **kwargs) |
202 | + self._to_select_related = [] |
203 | + |
204 | + def _get_left_join_condition(self, rel, cls_alias): |
205 | + # Rebuild the foreign key condition using cls_alias instead of the |
206 | + # original class name |
207 | + remote_alias_key = tuple( |
208 | + getattr(cls_alias, r.name) for r in rel.remote_key) |
209 | + return compare_columns(rel.local_key, remote_alias_key) |
210 | + |
211 | + def select_related(self, *args): |
212 | + if len(args) == 0: |
213 | + return self |
214 | + rs = self.copy() |
215 | + |
216 | + cls_spec = self._find_spec.cls_spec |
217 | + if not isinstance(cls_spec, tuple): |
218 | + cls_spec = (cls_spec,) |
219 | + cls_spec = list(cls_spec) |
220 | + |
221 | + if rs._tables == Undef: |
222 | + rs._tables = [i for i in cls_spec] |
223 | + |
224 | + cls_alias_per_ref = {} |
225 | + for ref in args: |
226 | + rel = ref._relation |
227 | + cls_alias = ClassAlias(rel.remote_cls) |
228 | + left_condition = self._get_left_join_condition(rel, cls_alias) |
229 | + rs._tables.append(LeftJoin(cls_alias, left_condition)) |
230 | + cls_spec.append(cls_alias) |
231 | + cls_alias_per_ref[ref] = cls_alias |
232 | + |
233 | + # Rebuild find spec |
234 | + rs._find_spec = FindSpec(tuple(cls_spec)) |
235 | + rs._find_spec._to_select_related = cls_alias_per_ref |
236 | + return rs |
237 | + |
238 | + |
239 | +# Replace resultset by mine |
240 | +Store._result_set_factory = ResultSet |
241 | + |
242 | +# Replace FindSpec by mine on module level |
243 | +import storm.store |
244 | +storm.store.FindSpec = FindSpec |
245 | + |
246 | +# Replace the references from storm.Store that uses FindSpec |
247 | +original_find = Store.find |
248 | + |
249 | +def _find(self, cls_spec, *args, **kwargs): |
250 | + resultset = original_find(self, cls_spec, *args, **kwargs) |
251 | + resultset._find_spec = FindSpec(cls_spec) |
252 | + return resultset |
253 | + |
254 | +Store.find = _find |
255 | diff --git a/lib/lp/services/database/tests/test_select_related.py b/lib/lp/services/database/tests/test_select_related.py |
256 | new file mode 100644 |
257 | index 0000000..55860f9 |
258 | --- /dev/null |
259 | +++ b/lib/lp/services/database/tests/test_select_related.py |
260 | @@ -0,0 +1,57 @@ |
261 | +from lp.bugs.model.bug import Bug |
262 | +from lp.services.database.select_related import ResultSet |
263 | +from lp.testing import TestCaseWithFactory, StormStatementRecorder, login |
264 | +from lp.testing.layers import DatabaseFunctionalLayer |
265 | +from lp.testing.matchers import HasQueryCount |
266 | +from lp.testing.sampledata import ADMIN_EMAIL |
267 | +from storm.store import Store |
268 | +from testtools.matchers import Equals |
269 | +from zope.security.proxy import removeSecurityProxy |
270 | + |
271 | + |
272 | +class TestLoaders(TestCaseWithFactory): |
273 | + layer = DatabaseFunctionalLayer |
274 | + |
275 | + def test_select_related_object_does_left_join(self): |
276 | + # we have 11 bugs of the same owner |
277 | + duplicate = self.factory.makeBug() |
278 | + store = Store.of(duplicate) |
279 | + bug1 = self.factory.makeBug() # this bug doesnt have dup |
280 | + for i in range(10): |
281 | + b = removeSecurityProxy(self.factory.makeBug(owner=bug1.owner)) |
282 | + # XXX: Test with left join |
283 | + b.duplicateof = duplicate |
284 | + store.add(duplicate) |
285 | + |
286 | + # and 2 other random bugs |
287 | + self.factory.makeBug() |
288 | + self.factory.makeBug() |
289 | + |
290 | + store.flush() |
291 | + store.commit() |
292 | + |
293 | + bugs_filter = Bug.owner == bug1.owner |
294 | + store.invalidate() |
295 | + # fetch bug1 again outside recorder, to do the match agains it bellow |
296 | + bug1.owner |
297 | + with StormStatementRecorder() as recorder: |
298 | + rs = store.find(Bug, bugs_filter) |
299 | + bugs = rs.select_related( |
300 | + Bug.owner, Bug.who_made_private, Bug.duplicateof) |
301 | + |
302 | + # One query for the count |
303 | + self.assertEqual(11, bugs.count()) |
304 | + |
305 | + # This should trigger only one query to database |
306 | + # (even if we are fetching related objects) |
307 | + for bug in bugs: |
308 | + bug = removeSecurityProxy(bug) |
309 | + self.assertIsNotNone(bug.date_last_message) |
310 | + self.assertEqual(bug.owner.name, bug1.owner.name) |
311 | + self.assertIsNone(bug.who_made_private) |
312 | + if bug.id == bug1.id: |
313 | + self.assertIsNone(bug.duplicateof) |
314 | + else: |
315 | + self.assertIsNotNone(bug.duplicateof) |
316 | + |
317 | + self.assertThat(recorder, HasQueryCount(Equals(2))) |
318 | \ No newline at end of file |
319 | diff --git a/lib/lp/soyuz/browser/queue.py b/lib/lp/soyuz/browser/queue.py |
320 | index a476e1d..a036f11 100644 |
321 | --- a/lib/lp/soyuz/browser/queue.py |
322 | +++ b/lib/lp/soyuz/browser/queue.py |
323 | @@ -1,4 +1,4 @@ |
324 | -# Copyright 2009-2017 Canonical Ltd. This software is licensed under the |
325 | +# Copyright 2009-2020 Canonical Ltd. This software is licensed under the |
326 | # GNU Affero General Public License version 3 (see the file LICENSE). |
327 | |
328 | """Browser views for package queue.""" |
329 | @@ -10,6 +10,7 @@ __all__ = [ |
330 | 'QueueItemsView', |
331 | ] |
332 | |
333 | +from collections import defaultdict |
334 | from operator import attrgetter |
335 | |
336 | from lazr.delegates import delegate_to |
337 | @@ -68,6 +69,7 @@ from lp.soyuz.model.files import ( |
338 | from lp.soyuz.model.packagecopyjob import PackageCopyJob |
339 | from lp.soyuz.model.queue import ( |
340 | PackageUploadBuild, |
341 | + PackageUploadLog, |
342 | PackageUploadSource, |
343 | ) |
344 | from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease |
345 | @@ -207,7 +209,23 @@ class QueueItemsView(LaunchpadView): |
346 | jobs = load_related(Job, package_copy_jobs, ['job_id']) |
347 | person_ids.extend(map(attrgetter('requester_id'), jobs)) |
348 | list(getUtility(IPersonSet).getPrecachedPersonsFromIDs( |
349 | - person_ids, need_validity=True, need_icon=True)) |
350 | + person_ids, need_validity=True)) |
351 | + |
352 | + def getPreloadedLogsDict(self, uploads): |
353 | + """Returns a dict of preloaded PackageUploadLog objects by |
354 | + package_upload_id, to be used on preloading CompletePackageUpload |
355 | + objects""" |
356 | + logs = load_referencing( |
357 | + PackageUploadLog, uploads, ['package_upload_id']) |
358 | + logs.sort(key=attrgetter("date_created"), reverse=True) |
359 | + list(getUtility(IPersonSet).getPrecachedPersonsFromIDs( |
360 | + [log.reviewer_id for log in logs], |
361 | + need_validity=True |
362 | + )) |
363 | + logs_dict = defaultdict(list) |
364 | + for log in logs: |
365 | + logs_dict[log.package_upload_id].append(log) |
366 | + return logs_dict |
367 | |
368 | def decoratedQueueBatch(self): |
369 | """Return the current batch, converted to decorated objects. |
370 | @@ -227,6 +245,9 @@ class QueueItemsView(LaunchpadView): |
371 | pubs = load_referencing( |
372 | PackageUploadBuild, uploads, ['packageuploadID']) |
373 | |
374 | + # preload logs info |
375 | + logs_dict = self.getPreloadedLogsDict(uploads) |
376 | + |
377 | source_sprs = load_related( |
378 | SourcePackageRelease, puses, ['sourcepackagereleaseID']) |
379 | bpbs = load_related(BinaryPackageBuild, pubs, ['buildID']) |
380 | @@ -264,7 +285,8 @@ class QueueItemsView(LaunchpadView): |
381 | |
382 | return [ |
383 | CompletePackageUpload( |
384 | - item, build_upload_files, source_upload_files, package_sets) |
385 | + item, build_upload_files, source_upload_files, package_sets, |
386 | + logs_dict[item.id]) |
387 | for item in uploads] |
388 | |
389 | def is_new(self, binarypackagerelease): |
390 | @@ -493,13 +515,14 @@ class CompletePackageUpload: |
391 | date_created = None |
392 | sources = None |
393 | builds = None |
394 | + logs = None |
395 | customfiles = None |
396 | contains_source = None |
397 | contains_build = None |
398 | sourcepackagerelease = None |
399 | |
400 | def __init__(self, packageupload, build_upload_files, |
401 | - source_upload_files, package_sets): |
402 | + source_upload_files, package_sets, logs=None): |
403 | self.pocket = packageupload.pocket |
404 | self.date_created = packageupload.date_created |
405 | self.context = packageupload |
406 | @@ -507,6 +530,7 @@ class CompletePackageUpload: |
407 | self.contains_source = len(self.sources) > 0 |
408 | self.builds = list(packageupload.builds) |
409 | self.contains_build = len(self.builds) > 0 |
410 | + self.logs = list(logs) if logs is not None else None |
411 | self.customfiles = list(packageupload.customfiles) |
412 | |
413 | # Create a dictionary of binary files keyed by |
414 | diff --git a/lib/lp/soyuz/browser/tests/test_queue.py b/lib/lp/soyuz/browser/tests/test_queue.py |
415 | index c2d15f2..0990845 100644 |
416 | --- a/lib/lp/soyuz/browser/tests/test_queue.py |
417 | +++ b/lib/lp/soyuz/browser/tests/test_queue.py |
418 | @@ -1,4 +1,4 @@ |
419 | -# Copyright 2010-2018 Canonical Ltd. This software is licensed under the |
420 | +# Copyright 2010-2020 Canonical Ltd. This software is licensed under the |
421 | # GNU Affero General Public License version 3 (see the file LICENSE). |
422 | |
423 | """Unit tests for QueueItemsView.""" |
424 | @@ -33,6 +33,7 @@ from lp.testing import ( |
425 | login_person, |
426 | logout, |
427 | person_logged_in, |
428 | + record_two_runs, |
429 | StormStatementRecorder, |
430 | TestCaseWithFactory, |
431 | ) |
432 | @@ -342,10 +343,10 @@ class TestQueueItemsView(TestCaseWithFactory): |
433 | |
434 | layer = LaunchpadFunctionalLayer |
435 | |
436 | - def makeView(self, distroseries, user): |
437 | + def makeView(self, distroseries, user, form=None): |
438 | """Create a queue view.""" |
439 | return create_initialized_view( |
440 | - distroseries, name='+queue', principal=user) |
441 | + distroseries, name='+queue', principal=user, form=form) |
442 | |
443 | def test_view_renders_source_upload(self): |
444 | login(ADMIN_EMAIL) |
445 | @@ -437,7 +438,41 @@ class TestQueueItemsView(TestCaseWithFactory): |
446 | with StormStatementRecorder() as recorder: |
447 | view = self.makeView(distroseries, queue_admin) |
448 | view() |
449 | - self.assertThat(recorder, HasQueryCount(Equals(56))) |
450 | + self.assertThat(recorder, HasQueryCount(Equals(57))) |
451 | + |
452 | + def test_package_upload_with_logs_query_count(self): |
453 | + login(ADMIN_EMAIL) |
454 | + uploads = [] |
455 | + distroseries = self.factory.makeDistroSeries() |
456 | + |
457 | + for i in range(50): |
458 | + uploads.append(self.factory.makeSourcePackageUpload(distroseries)) |
459 | + queue_admin = self.factory.makeArchiveAdmin(distroseries.main_archive) |
460 | + |
461 | + def reject_some_package(): |
462 | + for upload in uploads: |
463 | + if len(upload.logs) == 0: |
464 | + person = self.factory.makePerson() |
465 | + upload.rejectFromQueue(person) |
466 | + break |
467 | + |
468 | + def run_view(): |
469 | + Store.of(uploads[0]).invalidate() |
470 | + with person_logged_in(queue_admin): |
471 | + form = { |
472 | + "queue_state": PackageUploadStatus.REJECTED.value} |
473 | + view = self.makeView(distroseries, queue_admin, form=form) |
474 | + view() |
475 | + |
476 | + recorder1, recorder2 = record_two_runs( |
477 | + run_view, reject_some_package, 1, 10) |
478 | + # XXX: The query count should be the same, but there is one query |
479 | + # fetching PersonLocation that only happens in the first run (no |
480 | + # matter how many times we create anything), and it's not related at |
481 | + # all to the logs. |
482 | + # self.assertThat(recorder2, HasQueryCount.byEquality(recorder1)) |
483 | + self.assertThat(recorder1, HasQueryCount(Equals(37))) |
484 | + self.assertThat(recorder2, HasQueryCount(Equals(36))) |
485 | |
486 | |
487 | class TestCompletePackageUpload(TestCaseWithFactory): |
488 | diff --git a/lib/lp/soyuz/configure.zcml b/lib/lp/soyuz/configure.zcml |
489 | index 0e35ccb..f159d27 100644 |
490 | --- a/lib/lp/soyuz/configure.zcml |
491 | +++ b/lib/lp/soyuz/configure.zcml |
492 | @@ -1,4 +1,4 @@ |
493 | -<!-- Copyright 2009-2019 Canonical Ltd. This software is licensed under the |
494 | +<!-- Copyright 2009-2020 Canonical Ltd. This software is licensed under the |
495 | GNU Affero General Public License version 3 (see the file LICENSE). |
496 | --> |
497 | |
498 | @@ -151,6 +151,7 @@ |
499 | displayarchs |
500 | displayversion |
501 | isPPA |
502 | + logs |
503 | package_copy_job |
504 | package_copy_job_id |
505 | package_name |
506 | @@ -183,6 +184,11 @@ |
507 | set_attributes="status distroseries pocket changesfile archive"/> |
508 | </class> |
509 | <class |
510 | + class="lp.soyuz.model.queue.PackageUploadLog"> |
511 | + <allow |
512 | + interface="lp.soyuz.interfaces.queue.IPackageUploadLog"/> |
513 | + </class> |
514 | + <class |
515 | class="lp.soyuz.model.queue.PackageUploadSource"> |
516 | <allow |
517 | interface="lp.soyuz.interfaces.queue.IPackageUploadSource"/> |
518 | diff --git a/lib/lp/soyuz/interfaces/queue.py b/lib/lp/soyuz/interfaces/queue.py |
519 | index c57da73..6f440b3 100644 |
520 | --- a/lib/lp/soyuz/interfaces/queue.py |
521 | +++ b/lib/lp/soyuz/interfaces/queue.py |
522 | @@ -1,4 +1,4 @@ |
523 | -# Copyright 2009-2016 Canonical Ltd. This software is licensed under the |
524 | +# Copyright 2009-2020 Canonical Ltd. This software is licensed under the |
525 | # GNU Affero General Public License version 3 (see the file LICENSE). |
526 | |
527 | """Queue interfaces.""" |
528 | @@ -11,6 +11,7 @@ __all__ = [ |
529 | 'IHasQueueItems', |
530 | 'IPackageUploadQueue', |
531 | 'IPackageUpload', |
532 | + 'IPackageUploadLog', |
533 | 'IPackageUploadBuild', |
534 | 'IPackageUploadSource', |
535 | 'IPackageUploadCustom', |
536 | @@ -37,6 +38,7 @@ from lazr.restful.declarations import ( |
537 | REQUEST_USER, |
538 | ) |
539 | from lazr.restful.fields import Reference |
540 | +from twisted.words.im.interfaces import IPerson |
541 | from zope.interface import ( |
542 | Attribute, |
543 | Interface, |
544 | @@ -149,6 +151,8 @@ class IPackageUpload(Interface): |
545 | title=_('Date created'), |
546 | description=_("The date this package upload was done."))) |
547 | |
548 | + logs = Attribute(_("The change log of this PackageUpload.")) |
549 | + |
550 | changesfile = Attribute("The librarian alias for the changes file " |
551 | "associated with this upload") |
552 | changes_file_url = exported( |
553 | @@ -711,6 +715,33 @@ class IPackageUploadCustom(Interface): |
554 | """ |
555 | |
556 | |
557 | +class IPackageUploadLog(Interface): |
558 | + id = Int(title=_('ID'), required=True, readonly=True) |
559 | + |
560 | + package_upload = Reference( |
561 | + IPackageUpload, |
562 | + title=_("Original package upload."), required=True, readonly=True) |
563 | + |
564 | + date_created = Datetime( |
565 | + title=_("When this action happened."), required=True, readonly=True) |
566 | + |
567 | + reviewer = Reference( |
568 | + IPerson, title=_("Who did this action."), |
569 | + required=True, readonly=True) |
570 | + |
571 | + old_status = Choice( |
572 | + vocabulary=PackageUploadStatus, description=_("Old status."), |
573 | + required=True, readonly=True) |
574 | + |
575 | + new_status = Choice( |
576 | + vocabulary=PackageUploadStatus, description=_("New status."), |
577 | + required=True, readonly=True) |
578 | + |
579 | + comment = TextLine( |
580 | + title=_("User's comment about this change."), |
581 | + required=False, readonly=True) |
582 | + |
583 | + |
584 | class IPackageUploadSet(Interface): |
585 | """Represents a set of IPackageUploads""" |
586 | |
587 | diff --git a/lib/lp/soyuz/model/queue.py b/lib/lp/soyuz/model/queue.py |
588 | index 0fd1799..d394e9e 100644 |
589 | --- a/lib/lp/soyuz/model/queue.py |
590 | +++ b/lib/lp/soyuz/model/queue.py |
591 | @@ -1,18 +1,21 @@ |
592 | -# Copyright 2009-2018 Canonical Ltd. This software is licensed under the |
593 | +# Copyright 2009-2020 Canonical Ltd. This software is licensed under the |
594 | # GNU Affero General Public License version 3 (see the file LICENSE). |
595 | |
596 | __metaclass__ = type |
597 | __all__ = [ |
598 | - 'PackageUploadQueue', |
599 | 'PackageUpload', |
600 | 'PackageUploadBuild', |
601 | - 'PackageUploadSource', |
602 | 'PackageUploadCustom', |
603 | + 'PackageUploadLog', |
604 | + 'PackageUploadLog', |
605 | + 'PackageUploadQueue', |
606 | 'PackageUploadSet', |
607 | + 'PackageUploadSource', |
608 | ] |
609 | |
610 | from itertools import chain |
611 | |
612 | +import pytz |
613 | from sqlobject import ( |
614 | ForeignKey, |
615 | SQLMultipleJoin, |
616 | @@ -29,6 +32,7 @@ from storm.locals import ( |
617 | SQL, |
618 | Unicode, |
619 | ) |
620 | +from storm.properties import DateTime |
621 | from storm.store import ( |
622 | EmptyResultSet, |
623 | Store, |
624 | @@ -49,7 +53,10 @@ from lp.services.database.bulk import ( |
625 | from lp.services.database.constants import UTC_NOW |
626 | from lp.services.database.datetimecol import UtcDateTimeCol |
627 | from lp.services.database.decoratedresultset import DecoratedResultSet |
628 | -from lp.services.database.enumcol import EnumCol |
629 | +from lp.services.database.enumcol import ( |
630 | + DBEnum, |
631 | + EnumCol, |
632 | + ) |
633 | from lp.services.database.interfaces import ( |
634 | IMasterStore, |
635 | IStore, |
636 | @@ -58,6 +65,7 @@ from lp.services.database.sqlbase import ( |
637 | SQLBase, |
638 | sqlvalues, |
639 | ) |
640 | +from lp.services.database.stormbase import StormBase |
641 | from lp.services.database.stormexpr import ( |
642 | Array, |
643 | ArrayContains, |
644 | @@ -97,6 +105,7 @@ from lp.soyuz.interfaces.queue import ( |
645 | IPackageUpload, |
646 | IPackageUploadBuild, |
647 | IPackageUploadCustom, |
648 | + IPackageUploadLog, |
649 | IPackageUploadQueue, |
650 | IPackageUploadSet, |
651 | IPackageUploadSource, |
652 | @@ -106,7 +115,7 @@ from lp.soyuz.interfaces.queue import ( |
653 | QueueInconsistentStateError, |
654 | QueueSourceAcceptError, |
655 | QueueStateWriteProtectedError, |
656 | - ) |
657 | + IPackageUploadLog) |
658 | from lp.soyuz.interfaces.section import ISectionSet |
659 | from lp.soyuz.mail.packageupload import PackageUploadMailer |
660 | from lp.soyuz.model.binarypackagename import BinaryPackageName |
661 | @@ -203,6 +212,26 @@ class PackageUpload(SQLBase): |
662 | if self.package_copy_job: |
663 | self.addSearchableNames([self.package_copy_job.package_name]) |
664 | self.addSearchableVersions([self.package_copy_job.package_version]) |
665 | + self._logs = None |
666 | + |
667 | + @property |
668 | + def logs(self): |
669 | + if self._logs is None: |
670 | + logs = Store.of(self).find( |
671 | + PackageUploadLog, |
672 | + PackageUploadLog.package_upload == self) |
673 | + self._logs = list(logs.order_by(Desc('date_created'))) |
674 | + return self._logs |
675 | + |
676 | + def _addLog(self, reviewer, new_status, comment=None): |
677 | + self._logs = None # clear local cache |
678 | + return Store.of(self).add(PackageUploadLog( |
679 | + package_upload=self, |
680 | + reviewer=reviewer, |
681 | + old_status=self.status, |
682 | + new_status=new_status, |
683 | + comment=comment |
684 | + )) |
685 | |
686 | @cachedproperty |
687 | def sources(self): |
688 | @@ -571,6 +600,10 @@ class PackageUpload(SQLBase): |
689 | |
690 | def acceptFromQueue(self, user=None): |
691 | """See `IPackageUpload`.""" |
692 | + # XXX: Only tests are not passing user here. We should adjust the |
693 | + # tests and always create the log entries after |
694 | + if user is not None: |
695 | + self._addLog(user, PackageUploadStatus.ACCEPTED, None) |
696 | if self.package_copy_job is None: |
697 | self._acceptNonSyncFromQueue() |
698 | else: |
699 | @@ -581,6 +614,7 @@ class PackageUpload(SQLBase): |
700 | |
701 | def rejectFromQueue(self, user, comment=None): |
702 | """See `IPackageUpload`.""" |
703 | + self._addLog(user, PackageUploadStatus.REJECTED, comment) |
704 | self.setRejected() |
705 | if self.package_copy_job is not None: |
706 | # Circular imports :( |
707 | @@ -1153,6 +1187,46 @@ def get_properties_for_binary(bpr): |
708 | } |
709 | |
710 | |
711 | +@implementer(IPackageUploadLog) |
712 | +class PackageUploadLog(StormBase): |
713 | + """Tracking of status changes for a given package upload""" |
714 | + |
715 | + __storm_table__ = "PackageUploadLog" |
716 | + |
717 | + id = Int(primary=True) |
718 | + |
719 | + package_upload_id = Int(name='package_upload') |
720 | + package_upload = Reference(package_upload_id, PackageUpload.id) |
721 | + |
722 | + date_created = DateTime(tzinfo=pytz.UTC, allow_none=False, |
723 | + default=UTC_NOW) |
724 | + |
725 | + reviewer_id = Int(name='reviewer', allow_none=False) |
726 | + reviewer = Reference(reviewer_id, 'Person.id') |
727 | + |
728 | + old_status = DBEnum(enum=PackageUploadStatus, allow_none=False) |
729 | + |
730 | + new_status = DBEnum(enum=PackageUploadStatus, allow_none=False) |
731 | + |
732 | + comment = Unicode(allow_none=True) |
733 | + |
734 | + def __init__(self, package_upload, reviewer, old_status, new_status, |
735 | + comment=None, date_created=None): |
736 | + self.package_upload = package_upload |
737 | + self.reviewer = reviewer |
738 | + self.old_status = old_status |
739 | + self.new_status = new_status |
740 | + if comment is not None: |
741 | + self.comment = comment |
742 | + if date_created is not None: |
743 | + self.date_created = date_created |
744 | + |
745 | + def __repr__(self): |
746 | + return ( |
747 | + "<{self.__class__.__name__} ~{self.reviewer.name} " |
748 | + "changed {self.package_upload} to {self.new_status} " |
749 | + "{self.date_created}>").format(self=self) |
750 | + |
751 | @implementer(IPackageUploadBuild) |
752 | class PackageUploadBuild(SQLBase): |
753 | """A Queue item's related builds.""" |
754 | diff --git a/lib/lp/soyuz/stories/soyuz/xx-queue-pages.txt b/lib/lp/soyuz/stories/soyuz/xx-queue-pages.txt |
755 | index 7417a7b..8dcd71a 100644 |
756 | --- a/lib/lp/soyuz/stories/soyuz/xx-queue-pages.txt |
757 | +++ b/lib/lp/soyuz/stories/soyuz/xx-queue-pages.txt |
758 | @@ -510,7 +510,7 @@ values: |
759 | >>> filelist = find_tags_by_class(anon_browser.contents, 'queue-2') |
760 | >>> for row in filelist: |
761 | ... print(extract_text(row)) |
762 | - pmount_1.0-1_all.deb (18 bytes) NEW 0.1-1 restricted admin extra |
763 | + pmount_1.0-1_all.deb (18 bytes) NEW 0.1-1 restricted admin extra Accepted a moment ago by Sample Person |
764 | |
765 | 'netapplet' has gone straight to the 'done' queue because it's a single |
766 | source upload, and we can see its overridden values there: |
767 | @@ -546,6 +546,15 @@ Rejecting 'alsa-utils' source: |
768 | netapplet...ddtp... - Release 2006-... |
769 | netapplet...dist... - Release 2006-... |
770 | |
771 | + >>> upload_manager_browser.getControl( |
772 | + ... name="queue_state", index=0).displayValue=['Rejected'] |
773 | + >>> upload_manager_browser.getControl("Update").click() |
774 | + >>> logs = find_tags_by_class( |
775 | + ... upload_manager_browser.contents, "log-content") |
776 | + >>> for log in logs: |
777 | + ... print(extract_text(log)) |
778 | + Rejected...a moment ago...by Sample Person...Foo |
779 | + |
780 | One rejection email is generated: |
781 | |
782 | >>> run_package_upload_notification_jobs() |
783 | @@ -599,6 +608,21 @@ button will be disabled. |
784 | >>> upload_manager_browser.getControl(name="Reject").disabled |
785 | True |
786 | |
787 | +Accepting alsa again, and check that the package upload log has more rows |
788 | + |
789 | + >>> upload_manager_browser.getControl(name="QUEUE_ID").value = ['4'] |
790 | + >>> upload_manager_browser.getControl(name="Accept").click() |
791 | + >>> upload_manager_browser.getControl( |
792 | + ... name="queue_state", index=0).displayValue=['Accepted'] |
793 | + >>> upload_manager_browser.getControl("Update").click() |
794 | + >>> pkg_content = first_tag_by_class(upload_manager_browser.contents, |
795 | + ... "queue-4") |
796 | + >>> logs = find_tags_by_class(str(pkg_content), "log-content") |
797 | + >>> for log in logs: |
798 | + ... print(extract_text(log)) |
799 | + Accepted...a moment ago...by Sample Person... |
800 | + Rejected...a moment ago...by Sample Person...Foo |
801 | + |
802 | |
803 | Clean up |
804 | ======== |
805 | diff --git a/lib/lp/soyuz/templates/distroseries-queue.pt b/lib/lp/soyuz/templates/distroseries-queue.pt |
806 | index 59b77d5..39629ce 100644 |
807 | --- a/lib/lp/soyuz/templates/distroseries-queue.pt |
808 | +++ b/lib/lp/soyuz/templates/distroseries-queue.pt |
809 | @@ -149,6 +149,16 @@ |
810 | </tbody> |
811 | <tbody tal:attributes="class string:${filelist_class}"> |
812 | <metal:filelist use-macro="template/macros/package-filelist"/> |
813 | + <tr class="log-content" tal:repeat="log packageupload/logs"> |
814 | + <td colspan="2" style="border: 0"></td> |
815 | + <td colspan="8" style="border: 0"> |
816 | + <span tal:content="log/new_status"></span> |
817 | + <span tal:attributes="title log/date_created/fmt:datetime" |
818 | + tal:content="log/date_created/fmt:displaydate" /> |
819 | + by <span tal:content="structure log/reviewer/fmt:link" /> |
820 | + <p tal:condition="log/comment" tal:content="log/comment" /> |
821 | + </td> |
822 | + </tr> |
823 | </tbody> |
824 | </tal:block> |
825 | </tal:batch> |
826 | diff --git a/lib/lp/soyuz/tests/test_packageupload.py b/lib/lp/soyuz/tests/test_packageupload.py |
827 | index cb4d038..a2eb80f 100644 |
828 | --- a/lib/lp/soyuz/tests/test_packageupload.py |
829 | +++ b/lib/lp/soyuz/tests/test_packageupload.py |
830 | @@ -16,7 +16,10 @@ from lazr.restfulclient.errors import ( |
831 | BadRequest, |
832 | Unauthorized, |
833 | ) |
834 | -from testtools.matchers import Equals |
835 | +from testtools.matchers import ( |
836 | + Equals, |
837 | + MatchesStructure, |
838 | + ) |
839 | import transaction |
840 | from zope.component import ( |
841 | getUtility, |
842 | @@ -91,6 +94,27 @@ class PackageUploadTestCase(TestCaseWithFactory): |
843 | if os.path.exists(config.personalpackagearchive.root): |
844 | shutil.rmtree(config.personalpackagearchive.root) |
845 | |
846 | + def test_add_log_entry(self): |
847 | + upload = self.factory.makePackageUpload( |
848 | + status=PackageUploadStatus.UNAPPROVED) |
849 | + upload = removeSecurityProxy(upload) |
850 | + self.assertEqual(0, len(upload.logs)) |
851 | + |
852 | + person = self.factory.makePerson(name='lpusername') |
853 | + |
854 | + upload._addLog(person, PackageUploadStatus.REJECTED, 'just because') |
855 | + |
856 | + log = upload.logs[0] |
857 | + self.assertThat(log, MatchesStructure.byEquality( |
858 | + reviewer=person, old_status=PackageUploadStatus.UNAPPROVED, |
859 | + new_status=PackageUploadStatus.REJECTED, comment='just because')) |
860 | + |
861 | + expected_repr = ( |
862 | + "<PackageUploadLog ~lpusername " |
863 | + "changed {self.package_upload} to Rejected " |
864 | + "{self.date_created}>").format(self=log) |
865 | + self.assertEqual(str(expected_repr), repr(log)) |
866 | + |
867 | def test_realiseUpload_for_overridden_component_archive(self): |
868 | # If the component of an upload is overridden to 'Partner' for |
869 | # example, then the new publishing record should be for the |
870 | @@ -358,8 +382,14 @@ class PackageUploadTestCase(TestCaseWithFactory): |
871 | |
872 | # Accepting one of them works. (Since it's a single source upload, |
873 | # it goes straight to DONE.) |
874 | - upload_one.acceptFromQueue() |
875 | + person = self.factory.makePerson() |
876 | + upload_one.acceptFromQueue(person) |
877 | self.assertEqual("DONE", upload_one.status.name) |
878 | + |
879 | + log = upload_one.logs[0] |
880 | + self.assertThat(log, MatchesStructure.byEquality( |
881 | + reviewer=person, old_status=PackageUploadStatus.UNAPPROVED, |
882 | + new_status=PackageUploadStatus.ACCEPTED, comment=None)) |
883 | transaction.commit() |
884 | |
885 | # Trying to accept the second fails. |
886 | @@ -368,9 +398,15 @@ class PackageUploadTestCase(TestCaseWithFactory): |
887 | self.assertEqual("UNAPPROVED", upload_two.status.name) |
888 | |
889 | # Rejecting the second upload works. |
890 | - upload_two.rejectFromQueue(self.factory.makePerson()) |
891 | + upload_two.rejectFromQueue(person, 'Because yes') |
892 | self.assertEqual("REJECTED", upload_two.status.name) |
893 | |
894 | + self.assertEqual(1, len(upload_two.logs)) |
895 | + log = upload_two.logs[0] |
896 | + self.assertThat(log, MatchesStructure.byEquality( |
897 | + reviewer=person, old_status=PackageUploadStatus.UNAPPROVED, |
898 | + new_status=PackageUploadStatus.REJECTED, comment='Because yes')) |
899 | + |
900 | def test_rejectFromQueue_source_sends_email(self): |
901 | # Rejecting a source package sends an email to the uploader. |
902 | self.test_publisher.prepareBreezyAutotest() |
I think I see where you're going with this, and in principle it seems like a reasonable thing to add. This is almost a worst case for a third-party extension to Storm though; you're having to contort things a bit to cope with being in a subclass, and I'd find it a *lot* easier to review as an MP against lp:storm itself. (The people more or less active in maintaining Storm at the moment seem to be me and Simon Poirier.)
I'd also suggest talking with Free Ekanayaka, who's still around (though not working on Storm much) and you should be able to get hold of him. It was a long time ago and I'm not quite sure where it went, but https:/ /bazaar. launchpad. net/~free. ekanayaka/ storm/eager- loading/ revision/ 361 seems like it's trying to do some of the same things that you're trying to do, and to my mind looks rather cleaner and better-integrated with Storm's reference mechanisms; so it would be worth asking him whether there were any fundamental obstacles there. I haven't reviewed either approach in a lot of fine detail, though, and both would need lots of testing.