Merge ~pappacena/launchpad:storm_select_related into launchpad:master

Proposed by Thiago F. Pappacena on 2020-01-22
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)
Reviewer Review Type Date Requested Status
Launchpad code reviewers 2020-01-22 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 `PackageUploadLog.objects.all().select_related('reviewer')`. I couldn't find an easy way to do it on Storm ORM without manually joining the table and setting the condition. Way more verbose than Django requires.

The idea here is to implement something similar to what we have in Django:
`resultset = store.find(PackageUploadLog).select_related(PackageUploadLog.reviewer)` would join both tables with a left join, fill the store with all objects in a way that, when doing `[p.reviwer for p in resultset]` after, it would fetch from local store and no extra query would be needed.

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.

To post a comment you must log in.
Colin Watson (cjwatson) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/database/schema/security.cfg b/database/schema/security.cfg
2index 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
35diff --git a/lib/lp/services/database/configure.zcml b/lib/lp/services/database/configure.zcml
36index 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>
104diff --git a/lib/lp/services/database/select_related.py b/lib/lp/services/database/select_related.py
105new file mode 100644
106index 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
255diff --git a/lib/lp/services/database/tests/test_select_related.py b/lib/lp/services/database/tests/test_select_related.py
256new file mode 100644
257index 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
319diff --git a/lib/lp/soyuz/browser/queue.py b/lib/lp/soyuz/browser/queue.py
320index 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
414diff --git a/lib/lp/soyuz/browser/tests/test_queue.py b/lib/lp/soyuz/browser/tests/test_queue.py
415index 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):
488diff --git a/lib/lp/soyuz/configure.zcml b/lib/lp/soyuz/configure.zcml
489index 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"/>
518diff --git a/lib/lp/soyuz/interfaces/queue.py b/lib/lp/soyuz/interfaces/queue.py
519index 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
587diff --git a/lib/lp/soyuz/model/queue.py b/lib/lp/soyuz/model/queue.py
588index 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."""
754diff --git a/lib/lp/soyuz/stories/soyuz/xx-queue-pages.txt b/lib/lp/soyuz/stories/soyuz/xx-queue-pages.txt
755index 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 ========
805diff --git a/lib/lp/soyuz/templates/distroseries-queue.pt b/lib/lp/soyuz/templates/distroseries-queue.pt
806index 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>
826diff --git a/lib/lp/soyuz/tests/test_packageupload.py b/lib/lp/soyuz/tests/test_packageupload.py
827index 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()

Subscribers

People subscribed via source and target branches