launchpad-reviewers team mailing list archive

[Tom Wardill <tom.wardill@xxxxxxxxxxxxx>]

Tom Wardill has proposed merging lp:~twom/launchpad/allow-none-in-snapbuild-metadata into lp:launchpad.

Commit message:
Move store_upload_metadata to a property for better None handling

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~twom/launchpad/allow-none-in-snapbuild-metadata/+merge/361086

SnapStoreUploadJob was setting parameters in a dict that was None. 
Move the handling of store_upload_metadata to a property in SnapBuild, backed by a private database attribute.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~twom/launchpad/allow-none-in-snapbuild-metadata into lp:launchpad.

=== modified file 'lib/lp/snappy/model/snapbuild.py'
--- lib/lp/snappy/model/snapbuild.py	2018-12-18 14:23:22 +0000
+++ lib/lp/snappy/model/snapbuild.py	2018-12-18 17:17:47 +0000
@@ -175,7 +175,9 @@
 
     failure_count = Int(name='failure_count', allow_none=False)
 
-    store_upload_metadata = JSON('store_upload_json_data', allow_none=True)
+    # Private variable that is used by the `store_upload_metadata` to
+    # set as an empty dict rather than None, to handle pre-existing data.
+    _store_upload_metadata = JSON('store_upload_json_data', allow_none=True)
 
     def __init__(self, build_farm_job, requester, snap, archive,
                  distro_arch_series, pocket, channels, processor, virtualized,
@@ -192,12 +194,22 @@
         self.processor = processor
         self.virtualized = virtualized
         self.date_created = date_created
-        self.store_upload_metadata = store_upload_metadata or {}
+        self._store_upload_metadata = store_upload_metadata or {}
         if build_request is not None:
             self.build_request_id = build_request.id
         self.status = BuildStatus.NEEDSBUILD
 
     @property
+    def store_upload_metadata(self):
+        if self._store_upload_metadata is None:
+            self._store_upload_metadata = {}
+        return self._store_upload_metadata
+
+    @store_upload_metadata.setter
+    def store_upload_metadata(self, value):
+        self._store_upload_metadata = value
+
+    @property
     def build_request(self):
         """See `ISnapBuild`."""
         if self.build_request_id is not None:

=== modified file 'lib/lp/snappy/model/snapbuildjob.py'
--- lib/lp/snappy/model/snapbuildjob.py	2018-12-18 14:23:22 +0000
+++ lib/lp/snappy/model/snapbuildjob.py	2018-12-18 17:17:47 +0000
@@ -216,7 +216,7 @@
         """See `ISnapStoreUploadJob`."""
         intermediate = {}
         intermediate.update(self.metadata)
-        intermediate.update(self.snapbuild.store_upload_metadata or {})
+        intermediate.update(self.snapbuild.store_upload_metadata)
         return intermediate
 
     @property

=== modified file 'lib/lp/snappy/tests/test_snapbuild.py'
--- lib/lp/snappy/tests/test_snapbuild.py	2018-12-18 14:23:22 +0000
+++ lib/lp/snappy/tests/test_snapbuild.py	2018-12-18 17:17:47 +0000
@@ -780,8 +780,9 @@
     def test_store_upload_metadata_existing_as_none(self):
         db_build = self.factory.makeSnapBuild(requester=self.person)
         unsecure_db_build = removeSecurityProxy(db_build)
-        unsecure_db_build.store_upload_metadata = None
+        unsecure_db_build._store_upload_metadata = None
         store = IStore(SnapBuild)
         store.flush()
         loaded_build = store.find(SnapBuild, id=unsecure_db_build.id).one()
-        self.assertIsNone(loaded_build.store_upload_metadata)
+        self.assertIsNone(loaded_build._store_upload_metadata)
+        self.assertIsNotNone(loaded_build.store_upload_metadata)

=== modified file 'lib/lp/snappy/tests/test_snapbuildjob.py'
--- lib/lp/snappy/tests/test_snapbuildjob.py	2018-12-18 14:23:22 +0000
+++ lib/lp/snappy/tests/test_snapbuildjob.py	2018-12-18 17:17:47 +0000
@@ -685,10 +685,28 @@
     def test_with_snapbuild_metadata_as_none(self):
         db_build = self.factory.makeSnapBuild()
         unsecure_db_build = removeSecurityProxy(db_build)
-        unsecure_db_build.store_upload_metadata = None
+        unsecure_db_build._store_upload_metadata = None
         store = IStore(SnapBuild)
         store.flush()
         loaded_build = store.find(SnapBuild, id=unsecure_db_build.id).one()
 
         job = SnapStoreUploadJob.create(loaded_build)
         self.assertEqual({}, job.store_metadata)
+
+    def test_with_snapbuild_metadata_as_none_set_status(self):
+        db_build = self.factory.makeSnapBuild()
+        unsecure_db_build = removeSecurityProxy(db_build)
+        unsecure_db_build._store_upload_metadata = None
+        store = IStore(SnapBuild)
+        store.flush()
+        loaded_build = store.find(SnapBuild, id=unsecure_db_build.id).one()
+
+        job = SnapStoreUploadJob.create(loaded_build)
+        job.status_url = 'http://example.org'
+        store.flush()
+
+        loaded_build = store.find(SnapBuild, id=unsecure_db_build.id).one()
+        self.assertEqual(
+            'http://example.org',
+            loaded_build.store_upload_metadata['status_url']
+            )