Merge ~jugmac00/launchpad:expose-whether-copies-in-upload-queues-contain-binaries into launchpad:master

[Jürgen Gmach]

expose whether copies in upload queues contain binaries

[Colin Watson (cjwatson)]

This is a partial review because I ran out of time before my long weekend, but I'll give you what I've got.

I think this may have the problem that, if a webservice request needs to fetch a collection of uploads and render their exported properties (of which `contains_build` is one), then this will incur an extra `PackageCopyJob` query for each such upload. I ran out of time trying to construct a test that would prove this, although I at least verified that it seems to be the case by stepping through some relevant bits of code. My partial attempt was https://paste.ubuntu.com/p/sRbZSXs3zN/ - perhaps you can make something of that.

A fix will probably involve adding some more preloading to `lp.soyuz.model.queue.prefill_packageupload_caches`; I suggest comparing with `lp.soyuz.browser.queue.QueueItemsView.loadPackageCopyJobs` (called from the immediately-following `decoratedQueueBatch`), which does something similar for the web UI case; you may not need quite as much preloading as that, and if you can manage to construct a failing test along the lines of what I wrote then you can be guided by that.

However, `PackageUpload.copy_source_archive` already has a similar problem (apparently my fault in 2013), so this branch isn't making things any worse. As a result, I think the above would be good to fix but shouldn't be a blocker.

However however: I think we need to consider whether it's OK to put this in `contains_build`, or whether we need an additional property of some kind. The main client tool that consumes this is https://git.launchpad.net/ubuntu-archive-tools/tree/queue. If we need to change it too then that's OK, but we should work out whether this change will be backwards-compatible. I'm happy to help further with this once I'm back.

[Colin Watson (cjwatson)]

After some more debugging (I'd forgotten that the test suite normally sets the batch size to 5, so the batches being retrieved by `getPackageUploads` were the same size on each run and so didn't show any query scaling problems), here's a test diff that exhibits the problem: https://paste.ubuntu.com/p/3gy3mm8pW2/

62261be... by Jürgen Gmach on 2021-10-26

preload related PackageCopyJob data

... in order to avoid extra queries.

[Jürgen Gmach (jugmac00)]

Thank you for your excellent feedback (and preparation).

MP is ready for review.

[Colin Watson (cjwatson)]

review: Approve

f350ab9... by Jürgen Gmach on 2021-10-29

Use the dedicated `include_binaries` property

diff --git a/lib/lp/soyuz/model/queue.py b/lib/lp/soyuz/model/queue.py
index 29b5a2d..8dac27a 100644
--- a/lib/lp/soyuz/model/queue.py
+++ b/lib/lp/soyuz/model/queue.py
@@ -651,7 +651,14 @@ class PackageUpload(SQLBase):
     @cachedproperty
     def contains_build(self):
         """See `IPackageUpload`."""
-        return bool(self.builds)
+        if self.builds:
+            return True
+        else:
+            # handle case when PackageUpload is a copy
+            copy_job = self.concrete_package_copy_job
+            if copy_job is not None:
+                return copy_job.include_binaries
+        return False
 
     @cachedproperty
     def contains_copy(self):
@@ -1621,8 +1628,10 @@ class PackageUploadSet:
 
 def prefill_packageupload_caches(uploads, puses, pubs, pucs, logs):
     # Circular imports.
+    from lp.registry.model.distribution import Distribution
     from lp.soyuz.model.archive import Archive
     from lp.soyuz.model.binarypackagebuild import BinaryPackageBuild
+    from lp.soyuz.model.packagecopyjob import PackageCopyJob
     from lp.soyuz.model.publishing import SourcePackagePublishingHistory
     from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
 
@@ -1684,3 +1693,11 @@ def prefill_packageupload_caches(uploads, puses, pubs, pucs, logs):
         spr_cache.published_archives.append(publication.archive)
     for diff in diffs:
         get_property_cache(diff.to_source).package_diffs.append(diff)
+
+    package_copy_jobs = load_related(
+        PackageCopyJob, uploads, ['package_copy_job_id']
+    )
+    archives = load_related(
+        Archive, package_copy_jobs, ['source_archive_id']
+    )
+    load_related(Distribution, archives, ['distributionID'])
diff --git a/lib/lp/soyuz/tests/test_packageupload.py b/lib/lp/soyuz/tests/test_packageupload.py
index 9bd9a66..12d8c1f 100644
--- a/lib/lp/soyuz/tests/test_packageupload.py
+++ b/lib/lp/soyuz/tests/test_packageupload.py
@@ -666,6 +666,13 @@ class TestPackageUploadWithPackageCopyJob(TestCaseWithFactory):
         pu, pcj = self.makeUploadWithPackageCopyJob()
         self.assertIs(None, pu.sourcepackagerelease)
 
+    def test_upload_contains_build_works_with_copy_jobs(self):
+        pu = self.factory.makeCopyJobPackageUpload()
+        self.assertFalse(pu.contains_build)
+
+        pu = self.factory.makeCopyJobPackageUpload(include_binaries=True)
+        self.assertTrue(pu.contains_build)
+
 
 class TestPackageUploadCustom(TestCase):
     """Unit tests for `PackageUploadCustom`."""
@@ -1474,13 +1481,18 @@ class TestPackageUploadWebservice(TestCaseWithFactory):
         self.assertEndsWith(ws_upload.copy_source_archive_link, archive_url)
 
     def test_getPackageUploads_query_count(self):
+        self.pushConfig("launchpad", default_batch_size=20)
         person = self.makeQueueAdmin([self.universe])
         ws_distroseries = self.load(self.distroseries, person)
+
+        def make_uploads():
+            self.makeBinaryPackageUpload(person, component=self.universe)
+            self.makeCopyJobPackageUpload(person)
+
         recorder1, recorder2 = record_two_runs(
-            ws_distroseries.getPackageUploads,
-            lambda: self.makeBinaryPackageUpload(
-                person, component=self.universe),
-            5)
+            ws_distroseries.getPackageUploads, make_uploads, 5
+        )
+
         self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
 
     def test_api_package_upload_log(self):