launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:stormify-branchjob into launchpad:master.

Commit message:
Convert BranchJob to Storm

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/381010
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:stormify-branchjob into launchpad:master.

diff --git a/lib/lp/code/model/branch.py b/lib/lp/code/model/branch.py
index cd9d69f..31f9b63 100644
--- a/lib/lp/code/model/branch.py
+++ b/lib/lp/code/model/branch.py
@@ -1262,7 +1262,7 @@ class Branch(SQLBase, WebhookTargetMixin, BzrIdentityMixin):
         jobs = Store.of(self).find(
             BranchJob,
             BranchJob.branch == self,
-            Job.id == BranchJob.jobID,
+            Job.id == BranchJob.job_id,
             Job._status.is_in([JobStatus.WAITING, JobStatus.RUNNING]),
             BranchJob.job_type == BranchJobType.SCAN_BRANCH)
         pending_scan_job = not jobs.is_empty()
@@ -1431,7 +1431,7 @@ class Branch(SQLBase, WebhookTargetMixin, BzrIdentityMixin):
         # Remove BranchJobs.
         store = Store.of(self)
         affected_jobs = Select(
-            [BranchJob.jobID],
+            [BranchJob.job_id],
             And(BranchJob.job == Job.id, BranchJob.branch == self))
         store.find(Job, Job.id.is_in(affected_jobs)).remove()
 
@@ -1497,7 +1497,7 @@ class Branch(SQLBase, WebhookTargetMixin, BzrIdentityMixin):
         jobs = store.find(
             BranchJob,
             BranchJob.branch == self,
-            Job.id == BranchJob.jobID,
+            Job.id == BranchJob.job_id,
             Job._status != JobStatus.COMPLETED,
             Job._status != JobStatus.FAILED,
             BranchJob.job_type == BranchJobType.UPGRADE_BRANCH)
diff --git a/lib/lp/code/model/branchjob.py b/lib/lp/code/model/branchjob.py
index 8cf99c9..01a1f66 100644
--- a/lib/lp/code/model/branchjob.py
+++ b/lib/lp/code/model/branchjob.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __all__ = [
@@ -38,18 +38,17 @@ from lazr.enum import (
     DBEnumeratedType,
     DBItem,
     )
-import simplejson
-from sqlobject import (
-    ForeignKey,
-    SQLObjectNotFound,
-    StringCol,
-    )
 from storm.exceptions import LostObjectError
 from storm.expr import (
     And,
     SQL,
     )
-from storm.locals import Store
+from storm.locals import (
+    Int,
+    JSON,
+    Reference,
+    Store,
+    )
 import transaction
 from zope.component import getUtility
 from zope.interface import (
@@ -57,6 +56,7 @@ from zope.interface import (
     provider,
     )
 
+from lp.app.errors import NotFoundError
 from lp.code.bzr import (
     branch_revision_history,
     get_branch_formats,
@@ -100,7 +100,7 @@ from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.productseries import IProductSeriesSet
 from lp.scripts.helpers import TransactionFreeOperation
 from lp.services.config import config
-from lp.services.database.enumcol import EnumCol
+from lp.services.database.enumcol import DBEnum
 from lp.services.database.interfaces import (
     IMasterStore,
     IStore,
@@ -110,7 +110,7 @@ from lp.services.database.locking import (
     LockType,
     try_advisory_lock,
     )
-from lp.services.database.sqlbase import SQLBase
+from lp.services.database.stormbase import StormBase
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import (
     EnumeratedSubclass,
@@ -197,22 +197,22 @@ class BranchJobType(DBEnumeratedType):
 
 
 @implementer(IBranchJob)
-class BranchJob(SQLBase):
+class BranchJob(StormBase):
     """Base class for jobs related to branches."""
 
-    _table = 'BranchJob'
+    __storm_table__ = 'BranchJob'
 
-    job = ForeignKey(foreignKey='Job', notNull=True)
+    id = Int(primary=True)
 
-    branch = ForeignKey(foreignKey='Branch')
+    job_id = Int(name='job', allow_none=False)
+    job = Reference(job_id, 'Job.id')
 
-    job_type = EnumCol(enum=BranchJobType, notNull=True)
+    branch_id = Int(name='branch', allow_none=True)
+    branch = Reference(branch_id, 'Branch.id')
 
-    _json_data = StringCol(dbName='json_data')
+    job_type = DBEnum(name='job_type', enum=BranchJobType, allow_none=False)
 
-    @property
-    def metadata(self):
-        return simplejson.loads(self._json_data)
+    metadata = JSON('json_data', allow_none=True)
 
     def __init__(self, branch, job_type, metadata, **job_args):
         """Constructor.
@@ -225,14 +225,15 @@ class BranchJob(SQLBase):
         :param metadata: The type-specific variables, as a JSON-compatible
             dict.
         """
-        json_data = simplejson.dumps(metadata)
-        SQLBase.__init__(
-            self, job=Job(**job_args), branch=branch, job_type=job_type,
-            _json_data=json_data)
+        super(BranchJob, self).__init__()
+        self.job = Job(**job_args)
+        self.branch = branch
+        self.job_type = job_type
+        self.metadata = metadata
 
     def destroySelf(self):
         """See `IBranchJob`."""
-        SQLBase.destroySelf(self)
+        Store.of(self).remove(self)
         self.job.destroySelf()
 
     def makeDerived(self):
@@ -281,11 +282,11 @@ class BranchJobDerived(BaseRunnableJob):
     def get(cls, key):
         """Return the instance of this class whose key is supplied.
 
-        :raises: SQLObjectNotFound
+        :raises: NotFoundError
         """
         instance = IStore(BranchJob).get(BranchJob, key)
         if instance is None or instance.job_type != cls.class_job_type:
-            raise SQLObjectNotFound(
+            raise NotFoundError(
                 'No occurrence of %s has key %s' % (cls.__name__, key))
         return cls(instance)
 
@@ -968,7 +969,7 @@ class RosettaUploadJob(BranchJobDerived):
         """See `IRosettaUploadJobSource`."""
         store = IMasterStore(BranchJob)
         match = And(
-            Job.id == BranchJob.jobID,
+            Job.id == BranchJob.job_id,
             BranchJob.branch == branch,
             BranchJob.job_type == BranchJobType.ROSETTA_UPLOAD,
             Job._status != JobStatus.COMPLETED,
diff --git a/lib/lp/code/model/tests/test_branchjob.py b/lib/lp/code/model/tests/test_branchjob.py
index 0c4a893..c919fce 100644
--- a/lib/lp/code/model/tests/test_branchjob.py
+++ b/lib/lp/code/model/tests/test_branchjob.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for BranchJobs."""
@@ -18,7 +18,6 @@ from breezy.revision import NULL_REVISION
 from breezy.transport import get_transport
 from fixtures import MockPatch
 import pytz
-from sqlobject import SQLObjectNotFound
 from storm.locals import Store
 import transaction
 from zope.component import getUtility
@@ -65,7 +64,10 @@ from lp.codehosting.vfs import branch_id_to_path
 from lp.scripts.helpers import TransactionFreeOperation
 from lp.services.config import config
 from lp.services.database.constants import UTC_NOW
-from lp.services.database.interfaces import IMasterStore
+from lp.services.database.interfaces import (
+    IMasterStore,
+    IStore,
+    )
 from lp.services.features.testing import FeatureFixture
 from lp.services.identity.interfaces.emailaddress import EmailAddressStatus
 from lp.services.job.interfaces.job import JobStatus
@@ -116,7 +118,9 @@ class TestBranchJob(TestCaseWithFactory):
         branch_job = BranchJob(branch, BranchJobType.STATIC_DIFF, {})
         job_id = branch_job.job.id
         branch_job.destroySelf()
-        self.assertRaises(SQLObjectNotFound, BranchJob.get, job_id)
+        self.assertIsNone(
+            IStore(BranchJob).find(
+                BranchJob, BranchJob.job_id == job_id).one())
 
 
 class TestBranchJobDerived(TestCaseWithFactory):
@@ -394,8 +398,6 @@ class TestRevisionMailJob(TestCaseWithFactory):
         branch = self.factory.makeAnyBranch()
         job = RevisionMailJob.create(
             branch, 0, 'from@xxxxxxxxxxx', 'body', 'subject')
-        job.job.sync()
-        job.context.sync()
         self.assertEqual([job], list(RevisionMailJob.iterReady()))
 
     def test_iterReady_excludes_unready_jobs(self):
@@ -949,10 +951,7 @@ class TestRosettaUploadJob(TestCaseWithFactory):
         dummy.destroySelf()
 
         # Now create the RosettaUploadJob.
-        job = RosettaUploadJob.create(self.branch, NULL_REVISION)
-        job.job.sync()
-        job.context.sync()
-        return job
+        return RosettaUploadJob.create(self.branch, NULL_REVISION)
 
     def _commitFilesToTree(self, files, commit_message=None):
         """Add files to the tree.
diff --git a/lib/lp/code/scripts/tests/test_scan_branches.py b/lib/lp/code/scripts/tests/test_scan_branches.py
index 4f5783f..6c0143b 100644
--- a/lib/lp/code/scripts/tests/test_scan_branches.py
+++ b/lib/lp/code/scripts/tests/test_scan_branches.py
@@ -1,6 +1,6 @@
 #! /usr/bin/python
 #
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test the scan_branches script."""
@@ -76,7 +76,7 @@ class TestScanBranches(TestCaseWithFactory):
         store = Store.of(db_branch)
         result = store.find(
             BranchJob,
-            BranchJob.jobID == Job.id,
+            BranchJob.job_id == Job.id,
             Job._status == JobStatus.WAITING,
             BranchJob.job_type == BranchJobType.REVISION_MAIL,
             BranchJob.branch == db_branch)