Merge ~ilasc/launchpad:add-get-snapbuild-by-revision into launchpad:master

[Ioana Lasc]

Add getBuildByStoreRevision to Snap

This needs the new column "_store_upload_revision" to be added to SnapBuild and populated (prerequisite branch: populate-store-upload-revision).

[Colin Watson (cjwatson)]

As I noted in comments on the prerequisite MP, it seems likely that you'll need to adjust garbo tests here.

review: Needs Fixing

5d86f07... by Ioana Lasc on 2021-09-06

Merge branch 'populate-store-upload-revision' into add-get-snapbuild-by-revision

df6797e... by Ioana Lasc on 2021-09-07

Address code review comments

[Colin Watson (cjwatson)]

review: Approve

54adb9f... by Ioana Lasc on 2021-09-13

Remove MP number from comment

diff --git a/lib/lp/scripts/tests/test_garbo.py b/lib/lp/scripts/tests/test_garbo.py
index 3a43045..5fb7f77 100644
--- a/lib/lp/scripts/tests/test_garbo.py
+++ b/lib/lp/scripts/tests/test_garbo.py
@@ -2013,52 +2013,34 @@ class TestGarbo(FakeAdapterMixin, TestCaseWithFactory):
         with dbuser(config.ISnapStoreUploadJobSource.dbuser):
             run_isolated_jobs([job])
 
-        # this mimics what we have in the DB right now:
-        # uploaded snaps that do not have the new DB column
-        # _store_upload_revision populated yet
-        populator = PopulateSnapBuildStoreRevision(None)
-        filter = populator.findSnapBuilds()
+        # The _store_upload_revision now gets populated when
+        # uploading the build as part of the
+        # `SnapStoreUploadJob.store_revision` property setter.
+        # Assert that upload job above populated _store_upload_revision
+        # for build1:
         build1 = removeSecurityProxy(build1)
-        self.assertEqual(1, filter.count())
-        self.assertEqual(build1, filter.one())
-        self.assertEqual(build1._store_upload_revision, None)
-
-        # run the garbo job and verify _store_upload_revision
-        # is not populated with the value assigned to the build during upload
-        self.runDaily()
-        switch_dbuser('testadmin')
         self.assertEqual(build1._store_upload_revision, 1)
 
-        # Tests that of all builds for the same snap only those that have
-        # been uploaded to the store will get
-        # their new _store_upload_revision DB column updated
-        build2 = self.factory.makeSnapBuild(
-            snap=snap1,
-            status=BuildStatus.FULLYBUILT)
-        build3 = self.factory.makeSnapBuild(
-            snap=snap1,
-            status=BuildStatus.FULLYBUILT)
-        job = getUtility(ISnapStoreUploadJobSource).create(build2)
-        client = FakeSnapStoreClient()
-        client.upload.result = (
-            "http://sca.example/dev/api/snaps/1/builds/2/status")
-        client.checkStatus.result = (
-            "http://sca.example/dev/click-apps/1/rev/2/", 1)
-        self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
-        with dbuser(config.ISnapStoreUploadJobSource.dbuser):
-            run_isolated_jobs([job])
+        # and that the populator job doesn't pick up build1 anymore.
+        populator = PopulateSnapBuildStoreRevision(None)
+        filter = populator.findSnapBuilds()
+        self.assertEqual(0, filter.count())
+
+        # We manually simulate here the situation
+        # where a job to upload snap to the store has run in the past
+        # without populating the new column (prior to MP 407781).
+        build1._store_upload_revision = None
 
+        # in this case the populator garbo job should pick up build1
+        # to update the _store_upload_revision for it:
         populator = PopulateSnapBuildStoreRevision(None)
         filter = populator.findSnapBuilds()
         self.assertEqual(1, filter.count())
-        self.assertEqual(build2, filter.one())
+        self.assertEqual(build1, filter.one())
 
         self.runDaily()
         switch_dbuser('testadmin')
-        build2 = removeSecurityProxy(build2)
-        self.assertEqual(build2._store_upload_revision, 1)
-        build3 = removeSecurityProxy(build3)
-        self.assertIsNone(build3._store_upload_revision)
+        self.assertEqual(build1._store_upload_revision, 1)
 
 
 class TestGarboTasks(TestCaseWithFactory):
diff --git a/lib/lp/snappy/interfaces/snap.py b/lib/lp/snappy/interfaces/snap.py
index 2445553..088af8c 100644
--- a/lib/lp/snappy/interfaces/snap.py
+++ b/lib/lp/snappy/interfaces/snap.py
@@ -529,6 +529,21 @@ class ISnapView(Interface):
 
     @call_with(user=REQUEST_USER)
     @operation_parameters(
+        store_upload_revision=Int(title="Store revision",
+                                  required=True))
+    @export_read_operation()
+    @operation_for_version("devel")
+    def getBuildByStoreRevision(store_upload_revision, user=None):
+        """Returns the build (if any) of that snap recipe
+            that has the given store_upload_revision.
+
+        :param store_upload_revision: The revision assigned by the store.
+        :param user: The `IPerson` requesting this information.
+        :return: An 'ISnapBuild' or None.
+        """
+
+    @call_with(user=REQUEST_USER)
+    @operation_parameters(
         request_ids=List(
             title=_("A list of snap build request IDs."), value_type=Int(),
             required=False),
diff --git a/lib/lp/snappy/model/snap.py b/lib/lp/snappy/model/snap.py
index 24aa2be..e68182e 100644
--- a/lib/lp/snappy/model/snap.py
+++ b/lib/lp/snappy/model/snap.py
@@ -1111,6 +1111,20 @@ class Snap(Storm, WebhookTargetMixin):
 
         return result
 
+    def getBuildByStoreRevision(self, store_upload_revision, user=None):
+        build = Store.of(self).find(
+            SnapBuild,
+            SnapBuild.snap == self,
+            SnapBuild._store_upload_revision == store_upload_revision,
+            SnapBuild.archive == Archive.id,
+            Archive._enabled,
+            get_enabled_archive_filter(
+                user,
+                include_public=True,
+                include_subscribed=True)
+        ).one()
+        return build
+
     @property
     def builds(self):
         """See `ISnap`."""
diff --git a/lib/lp/snappy/model/snapbuildjob.py b/lib/lp/snappy/model/snapbuildjob.py
index 9634f62..a8fc49f 100644
--- a/lib/lp/snappy/model/snapbuildjob.py
+++ b/lib/lp/snappy/model/snapbuildjob.py
@@ -263,6 +263,7 @@ class SnapStoreUploadJob(SnapBuildJobDerived):
         if self.snapbuild.store_upload_metadata is None:
             self.snapbuild.store_upload_metadata = {}
         self.snapbuild.store_upload_metadata["store_revision"] = revision
+        self.snapbuild._store_upload_revision = revision
 
     @property
     def status_url(self):
diff --git a/lib/lp/snappy/tests/test_snap.py b/lib/lp/snappy/tests/test_snap.py
index 4d9c92f..adb8cfa 100644
--- a/lib/lp/snappy/tests/test_snap.py
+++ b/lib/lp/snappy/tests/test_snap.py
@@ -125,6 +125,7 @@ from lp.snappy.interfaces.snapbase import (
 from lp.snappy.interfaces.snapbuild import (
     ISnapBuild,
     ISnapBuildSet,
+    SnapBuildStoreUploadStatus,
     )
 from lp.snappy.interfaces.snapbuildjob import ISnapStoreUploadJobSource
 from lp.snappy.interfaces.snapjob import ISnapRequestBuildsJobSource
@@ -137,6 +138,10 @@ from lp.snappy.model.snap import (
 from lp.snappy.model.snapbuild import SnapFile
 from lp.snappy.model.snapbuildjob import SnapBuildJob
 from lp.snappy.model.snapjob import SnapJob
+from lp.snappy.tests.test_snapbuildjob import (
+    FakeSnapStoreClient,
+    run_isolated_jobs,
+    )
 from lp.testing import (
     admin_logged_in,
     ANONYMOUS,
@@ -1079,6 +1084,50 @@ class TestSnap(TestCaseWithFactory):
             snap.destroySelf()
         self.assertFalse(getUtility(ISnapSet).exists(owner, "condemned"))
 
+    def test_getBuildByStoreRevision(self):
+        snap1 = self.factory.makeSnap()
+        build = self.factory.makeSnapBuild(
+            snap=snap1,
+            status=BuildStatus.FULLYBUILT)
+
+        # There is no build with revision 5 for snap1
+        self.assertIsNone(snap1.getBuildByStoreRevision(5))
+
+        # Upload build1 and check we return it by version 1
+        job = getUtility(ISnapStoreUploadJobSource).create(build)
+        client = FakeSnapStoreClient()
+        client.upload.result = (
+            "http://sca.example/dev/api/snaps/1/builds/1/status")
+        client.checkStatus.result = (
+            "http://sca.example/dev/click-apps/1/rev/1/", 1)
+        self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
+        with dbuser(config.ISnapStoreUploadJobSource.dbuser):
+            run_isolated_jobs([job])
+        self.assertEqual(
+            SnapBuildStoreUploadStatus.UPLOADED, build.store_upload_status)
+        self.assertEqual(build.store_upload_revision, 1)
+        self.assertEqual(snap1.getBuildByStoreRevision(1), build)
+
+        # build & upload again, check revision
+        # and that we return the second build for revision 2
+        build2 = self.factory.makeSnapBuild(
+            snap=snap1,
+            status=BuildStatus.FULLYBUILT)
+        job = getUtility(ISnapStoreUploadJobSource).create(build2)
+        client = FakeSnapStoreClient()
+        client.upload.result = (
+            "http://sca.example/dev/api/snaps/1/builds/2/status")
+        client.checkStatus.result = (
+            "http://sca.example/dev/click-apps/1/rev/2/", 2)
+        self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
+        with dbuser(config.ISnapStoreUploadJobSource.dbuser):
+            run_isolated_jobs([job])
+        self.assertEqual(
+            SnapBuildStoreUploadStatus.UPLOADED, build2.store_upload_status)
+        self.assertEqual(build2.store_upload_revision, 2)
+        self.assertEqual(snap1.getBuildByStoreRevision(2), build2)
+        self.assertEqual(snap1.getBuildByStoreRevision(1), build)
+
     def test_getBuildSummariesForSnapBuildIds(self):
         snap1 = self.factory.makeSnap()
         snap2 = self.factory.makeSnap()
diff --git a/lib/lp/snappy/tests/test_snapbuildjob.py b/lib/lp/snappy/tests/test_snapbuildjob.py
index 3acefa2..5b56264 100644
--- a/lib/lp/snappy/tests/test_snapbuildjob.py
+++ b/lib/lp/snappy/tests/test_snapbuildjob.py
@@ -178,6 +178,8 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
         self.assertContentEqual([job], snapbuild.store_upload_jobs)
         self.assertEqual(self.store_url, job.store_url)
         self.assertEqual(1, job.store_revision)
+        self.assertEqual(1,
+                         removeSecurityProxy(snapbuild)._store_upload_revision)
         self.assertIsNone(job.error_message)
         self.assertEqual([], pop_notifications())
         self.assertWebhookDeliveries(
@@ -199,6 +201,8 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
         self.assertContentEqual([job], snapbuild.store_upload_jobs)
         self.assertIsNone(job.store_url)
         self.assertIsNone(job.store_revision)
+        self.assertIsNone(
+            removeSecurityProxy(snapbuild)._store_upload_revision)
         self.assertEqual("An upload failure", job.error_message)
         self.assertEqual([], pop_notifications())
         self.assertWebhookDeliveries(
@@ -225,6 +229,8 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
         self.assertContentEqual([job], snapbuild.store_upload_jobs)
         self.assertIsNone(job.store_url)
         self.assertIsNone(job.store_revision)
+        self.assertIsNone(
+            removeSecurityProxy(snapbuild)._store_upload_revision)
         self.assertEqual("Authorization failed.", job.error_message)
         [notification] = pop_notifications()
         self.assertEqual(
@@ -272,6 +278,8 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
         self.assertContentEqual([job], snapbuild.store_upload_jobs)
         self.assertIsNone(job.store_url)
         self.assertIsNone(job.store_revision)
+        self.assertIsNone(
+            removeSecurityProxy(snapbuild)._store_upload_revision)
         self.assertIsNone(job.error_message)
         self.assertEqual([], pop_notifications())
         self.assertEqual(JobStatus.WAITING, job.job.status)
@@ -290,6 +298,8 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
         self.assertContentEqual([job], snapbuild.store_upload_jobs)
         self.assertEqual(self.store_url, job.store_url)
         self.assertEqual(1, job.store_revision)
+        self.assertEqual(1,
+                         removeSecurityProxy(snapbuild)._store_upload_revision)
         self.assertIsNone(job.error_message)
         self.assertEqual([], pop_notifications())
         self.assertEqual(JobStatus.COMPLETED, job.job.status)
@@ -317,6 +327,8 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
         self.assertContentEqual([job], snapbuild.store_upload_jobs)
         self.assertIsNone(job.store_url)
         self.assertIsNone(job.store_revision)
+        self.assertIsNone(
+            removeSecurityProxy(snapbuild)._store_upload_revision)
         self.assertEqual("SSO melted.", job.error_message)
         [notification] = pop_notifications()
         self.assertEqual(
@@ -369,6 +381,8 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
         self.assertContentEqual([job], snapbuild.store_upload_jobs)
         self.assertIsNone(job.store_url)
         self.assertIsNone(job.store_revision)
+        self.assertIsNone(
+            removeSecurityProxy(snapbuild)._store_upload_revision)
         self.assertEqual("Failed to upload", job.error_message)
         [notification] = pop_notifications()
         self.assertEqual(
@@ -419,6 +433,8 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
         self.assertContentEqual([job], snapbuild.store_upload_jobs)
         self.assertIsNone(job.store_url)
         self.assertIsNone(job.store_revision)
+        self.assertIsNone(
+            removeSecurityProxy(snapbuild)._store_upload_revision)
         self.assertIsNone(job.error_message)
         self.assertEqual([], pop_notifications())
         self.assertEqual(JobStatus.WAITING, job.job.status)
@@ -437,6 +453,8 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
         self.assertContentEqual([job], snapbuild.store_upload_jobs)
         self.assertEqual(self.store_url, job.store_url)
         self.assertEqual(1, job.store_revision)
+        self.assertEqual(1,
+                         removeSecurityProxy(snapbuild)._store_upload_revision)
         self.assertIsNone(job.error_message)
         self.assertEqual([], pop_notifications())
         self.assertEqual(JobStatus.COMPLETED, job.job.status)
@@ -468,6 +486,8 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
         self.assertContentEqual([job], snapbuild.store_upload_jobs)
         self.assertIsNone(job.store_url)
         self.assertIsNone(job.store_revision)
+        self.assertIsNone(
+            removeSecurityProxy(snapbuild)._store_upload_revision)
         self.assertEqual(
             "Scan failed.\nConfinement not allowed.", job.error_message)
         self.assertEqual([
@@ -519,6 +539,8 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
         self.assertContentEqual([job], snapbuild.store_upload_jobs)
         self.assertIsNone(job.store_url)
         self.assertIsNone(job.store_revision)
+        self.assertIsNone(
+            removeSecurityProxy(snapbuild)._store_upload_revision)
         self.assertIsNone(job.error_message)
         self.assertEqual([], pop_notifications())
         self.assertWebhookDeliveries(
@@ -542,6 +564,8 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
         self.assertContentEqual([job], snapbuild.store_upload_jobs)
         self.assertEqual(self.store_url, job.store_url)
         self.assertEqual(1, job.store_revision)
+        self.assertEqual(1,
+                         removeSecurityProxy(snapbuild)._store_upload_revision)
         self.assertIsNone(job.error_message)
         self.assertEqual([], pop_notifications())
         self.assertWebhookDeliveries(
@@ -603,6 +627,8 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
         # Check we uploaded as expected
         self.assertEqual(self.store_url, job.store_url)
         self.assertEqual(1, job.store_revision)
+        self.assertEqual(1,
+                         removeSecurityProxy(snapbuild)._store_upload_revision)
         self.assertEqual(timedelta(seconds=60), job.retry_delay)
         self.assertEqual(1, len(client.upload.calls))
         self.assertIsNone(job.error_message)