Merge lp:~gmb/launchpad/create-blobjobs-bug-519205 into lp:launchpad/db-devel

[Graham Binns]

TemporaryBlobStorageAddView will now create a new ProcessApportBlobJob for each uploaded BLOB.

[Graham Binns (gmb)]

This branch is the next stage of moving the work for processing Apport
BLOBs out of the +filebug view and into the Jobs system.

The changes in this branch mean that when a BLOB is uploaded Launchpad
will automatically create a new ProcessApportBlobJob for it. This job
can then be polled later by +filebug to see if the BLOB has been parsed
properly.

== lib/canonical/launchpad/browser/temporaryblobstorage.py ==

 - I've updated TemporaryBlobStorageAddView so that it now creates a new
   ProcessApportBlobJob for the uploaded BLOB. I've moved the uploading
   functionality into a separate method so that it can be tested
   directly rather than having to test it via the 'Continue' action.

== lib/lp/bugs/configure.zcml ==

 - I've added ZCML for the ApportBlobJob interfaces.

== lib/lp/bugs/interfaces/apportjob.py ==

 - I've added a getByBlobUUID() method to IApportBlobJobSource so that
   we can look up Jobs by BLOB UUID. This will be used in a subsequent
   branch to poll for processing jobs being complete.

== lib/lp/bugs/model/apportjob.py ==

 - I've updated ProcessApportBlobJob.create() so that it will not create
   a new job for BLOB if there's already a job, complete or otherwise,
   in the database.
 - I've added an implementation of getByBlobUUID() to
   ProcessApportBlobJob.

== lib/lp/bugs/tests/test_apportjob.py ==

 - I've added tests for the changes to ProcessApportBlobJob and
   TemporaryBlobStorageAddView.

== lib/lp/bugs/utilities/filebugdataparser.py ==

 - I've cleaned up some lint by adding FileBugData to the __all__ clause
   of the module.

No lint.

[Eleanor Berger (intellectronica)]

(12:00:36) intellectronica: gmb: i suspect the 'blob=322819' in aaron's XXX in lib/lp/bugs/model/apportjob.py is a the result of a search&replace that went out of control?
(12:00:49) intellectronica: it's not in the current diff, but if you agree you can change it back now
(12:01:03) gmb: intellectronica: Ah, wacky fun. Yes, that'll be an over-eager %s.
(12:01:06) gmb: I'll fix it.
(12:03:14) intellectronica: gmb: in getByBlobUUID, i'd change the way you do the assert. i'd do `if jobs_for_blob.count() > 1: raise AssertionError(...)`
(12:03:47) intellectronica: which both rids you from the strange '< 2' condition, and from the assert statement that is always a bit confusing
(12:03:51) intellectronica: what do you think?
(12:04:11) gmb: intellectronica: I agree. I'll change it.
(12:07:15) intellectronica: gmb: other than that it all looks great. r=me

review: Approve (code)

[Graham Binns (gmb)]

Incremental diff of fixage:

=== modified file 'lib/lp/bugs/model/apportjob.py'
--- lib/lp/bugs/model/apportjob.py 2010-02-11 11:01:12 +0000
+++ lib/lp/bugs/model/apportjob.py 2010-02-11 12:08:02 +0000
@@ -83,7 +83,7 @@
         self.job = Job()
         self.blob = blob
         self.job_type = job_type
- # XXX AaronBentley 2009-01-29 blob=322819: This should be a
+ # XXX AaronBentley 2009-01-29 bug=322819: This should be a
         # bytestring, but the DB representation is unicode.
         self._json_data = json_data.decode('utf-8')

@@ -196,8 +196,9 @@
             raise SQLObjectNotFound(
                 "No ProcessApportBlobJob found for UUID %s" % uuid)

- assert jobs_for_blob.count() < 2, (
- "There can only be one ProcessApportBlobJob for a BLOB.")
+ if jobs_for_blob.count() > 1:
+ raise AssertionError(
+ "There can only be one ProcessApportBlobJob for a BLOB.")

         return cls(jobs_for_blob.any())

=== modified file 'lib/canonical/launchpad/browser/temporaryblobstorage.py'
--- lib/canonical/launchpad/browser/temporaryblobstorage.py	2009-09-18 13:24:29 +0000
+++ lib/canonical/launchpad/browser/temporaryblobstorage.py	2010-02-11 12:16:27 +0000
@@ -16,6 +16,8 @@
     BlobTooLarge, ITemporaryBlobStorage, ITemporaryStorageManager)
 from canonical.librarian.interfaces import UploadFailed
 
+from lp.bugs.interfaces.apportjob import IProcessApportBlobJobSource
+
 
 class TemporaryBlobStorageAddView(LaunchpadFormView):
     # XXX: gary 2009-09-18 bug=31358
@@ -40,13 +42,21 @@
     # being named like that.
     @action('Continue', name='FORM_SUBMIT')
     def continue_action(self, action, data):
+        uuid = self.store_blob(data['blob'])
+        self.request.response.setHeader('X-Launchpad-Blob-Token', uuid)
+        self.request.response.addInfoNotification(
+            'Your ticket is "%s"' % uuid)
+
+    def store_blob(self, blob):
+        """Store a blob and return its UUID."""
         try:
-            uuid = getUtility(ITemporaryStorageManager).new(data['blob'])
-            self.request.response.setHeader('X-Launchpad-Blob-Token', uuid)
-            self.request.response.addInfoNotification(
-                'Your ticket is "%s"' % uuid)
+            uuid = getUtility(ITemporaryStorageManager).new(blob)
         except BlobTooLarge:
             self.addError('Uploaded file was too large.')
         except UploadFailed:
             self.addError('File storage unavailable - try again later.')
 
+        # Create ProcessApportBlobJob for the BLOB.
+        blob = getUtility(ITemporaryStorageManager).fetch(uuid)
+        getUtility(IProcessApportBlobJobSource).create(blob)
+        return uuid

=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml	2010-02-09 16:44:17 +0000
+++ lib/lp/bugs/configure.zcml	2010-02-11 12:16:27 +0000
@@ -914,4 +914,16 @@
         <allow
             interface="lp.bugs.interfaces.bugjob.ICalculateBugHeatJobSource"/>
     </securedutility>
+
+    <!-- ProcessApportBlobJobs -->
+    <class class="lp.bugs.model.apportjob.ProcessApportBlobJob">
+        <allow interface="lp.bugs.interfaces.apportjob.IApportJob" />
+        <allow interface="lp.bugs.interfaces.apportjob.IProcessApportBlobJob"/>
+    </class>
+    <securedutility
+        component="lp.bugs.model.apportjob.ProcessApportBlobJob"
+        provides="lp.bugs.interfaces.apportjob.IProcessApportBlobJobSource">
+        <allow
+            interface="lp.bugs.interfaces.apportjob.IProcessApportBlobJobSource"/>
+    </securedutility>
 </configure>

=== modified file 'lib/lp/bugs/interfaces/apportjob.py'
--- lib/lp/bugs/interfaces/apportjob.py	2010-02-01 15:49:37 +0000
+++ lib/lp/bugs/interfaces/apportjob.py	2010-02-11 12:16:26 +0000
@@ -7,6 +7,8 @@
     'ApportJobType',
     'IApportJob',
     'IApportJobSource',
+    'IProcessApportBlobJob',
+    'IProcessApportBlobJobSource',
     ]
 
 from zope.interface import Attribute, Interface
@@ -57,6 +59,9 @@
     def create(bug):
         """Create a new IApportJob for a bug."""
 
+    def getByBlobUUID(uuid):
+        """For a given BLOB UUID, return any jobs pertaining to that BLOB."""
+
 
 class IProcessApportBlobJob(IRunnableJob):
     """A Job to process an Apport BLOB."""

=== modified file 'lib/lp/bugs/model/apportjob.py'
--- lib/lp/bugs/model/apportjob.py	2010-02-03 22:20:14 +0000
+++ lib/lp/bugs/model/apportjob.py	2010-02-11 12:16:26 +0000
@@ -83,8 +83,8 @@
         self.job = Job()
         self.blob = blob
         self.job_type = job_type
-        # XXX AaronBentley 2009-01-29 blob=322819: This should be a bytestring,
-        # but the DB representation is unicode.
+        # XXX AaronBentley 2009-01-29 bug=322819: This should be a
+        # bytestring, but the DB representation is unicode.
         self._json_data = json_data.decode('utf-8')
 
     @classmethod
@@ -163,14 +163,16 @@
     @classmethod
     def create(cls, blob):
         """See `IProcessApportBlobJobSource`."""
-        # If there's already a job for the bug, don't create a new one.
+        # If there's already a job for the BLOB, don't create a new one.
+        # We also include jobs which have been completed when checking
+        # for exisiting jobs, since a BLOB should only be processed
+        # once.
         store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
         job_for_blob = store.find(
             ApportJob,
             ApportJob.blob == blob,
             ApportJob.job_type == cls.class_job_type,
             ApportJob.job == Job.id,
-            Job.id.is_in(Job.ready_jobs)
             ).any()
 
         if job_for_blob is not None:
@@ -178,6 +180,28 @@
         else:
             return super(ProcessApportBlobJob, cls).create(blob)
 
+    @classmethod
+    def getByBlobUUID(cls, uuid):
+        """See `IApportJobSource`."""
+        store = IStore(ApportJob)
+        blob = store.get(TemporaryBlobStorage, uuid == uuid)
+        jobs_for_blob = store.find(
+            ApportJob,
+            ApportJob.blob == blob,
+            ApportJob.job_type == cls.class_job_type,
+            ApportJob.job == Job.id
+            )
+
+        if jobs_for_blob.is_empty():
+            raise SQLObjectNotFound(
+                "No ProcessApportBlobJob found for UUID %s" % uuid)
+
+        if jobs_for_blob.count() > 1:
+            raise AssertionError(
+                "There can only be one ProcessApportBlobJob for a BLOB.")
+
+        return cls(jobs_for_blob.any())
+
     def run(self):
         """See `IRunnableJob`."""
         self.blob.file_alias.open()

=== modified file 'lib/lp/bugs/tests/test_apportjob.py'
--- lib/lp/bugs/tests/test_apportjob.py	2010-02-03 22:20:14 +0000
+++ lib/lp/bugs/tests/test_apportjob.py	2010-02-11 12:16:27 +0000
@@ -13,13 +13,18 @@
 
 from canonical.config import config
 from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
-from canonical.testing import LaunchpadZopelessLayer
+from canonical.launchpad.interfaces.temporaryblobstorage import (
+    ITemporaryStorageManager)
+from canonical.launchpad.webapp.interfaces import ILaunchpadRoot
+from canonical.testing import (
+    LaunchpadFunctionalLayer, LaunchpadZopelessLayer)
 
 from lp.bugs.interfaces.apportjob import ApportJobType
 from lp.bugs.model.apportjob import (
     ApportJob, ApportJobDerived, ProcessApportBlobJob)
 from lp.bugs.utilities.filebugdataparser import FileBugDataParser
 from lp.testing import TestCaseWithFactory
+from lp.testing.views import create_initialized_view
 
 
 class ApportJobTestCase(TestCaseWithFactory):
@@ -157,6 +162,104 @@
                 "LibrarianFileAlias %s" % (
                     attachment['filename'], file_alias.id))
 
+    def test_getByBlobUUID(self):
+        # ProcessApportBlobJob.getByBlobUUID takes a BLOB UUID as a
+        # parameter and returns any jobs for that BLOB.
+        uuid = self.blob.uuid
+
+        job = ProcessApportBlobJob.create(self.blob)
+        job_from_uuid = ProcessApportBlobJob.getByBlobUUID(uuid)
+        self.assertEqual(
+            job, job_from_uuid,
+            "Job returend by getByBlobUUID() did not match original job.")
+        self.assertEqual(
+            self.blob, job_from_uuid.blob,
+            "BLOB referenced by Job returned by getByBlobUUID() did not "
+            "match original BLOB.")
+
+    def test_create_job_creates_only_one(self):
+        # ProcessApportBlobJob.create() will create only one
+        # ProcessApportBlobJob for a given BLOB, no matter how many
+        # times it is called.
+        current_jobs = list(ProcessApportBlobJob.iterReady())
+        self.assertEqual(
+            0, len(current_jobs),
+            "There should be no ProcessApportBlobJobs. Found %s" %
+            len(current_jobs))
+
+        job = ProcessApportBlobJob.create(self.blob)
+        current_jobs = list(ProcessApportBlobJob.iterReady())
+        self.assertEqual(
+            1, len(current_jobs),
+            "There should be only one ProcessApportBlobJob. Found %s" %
+            len(current_jobs))
+
+        another_job = ProcessApportBlobJob.create(self.blob)
+        current_jobs = list(ProcessApportBlobJob.iterReady())
+        self.assertEqual(
+            1, len(current_jobs),
+            "There should be only one ProcessApportBlobJob. Found %s" %
+            len(current_jobs))
+
+        # If the job is complete, it will no longer show up in the list
+        # of ready jobs. However, it won't be possible to create a new
+        # job to process the BLOB because each BLOB can only have one
+        # ProcessApportBlobJob.
+        job.job.start()
+        job.job.complete()
+        current_jobs = list(ProcessApportBlobJob.iterReady())
+        self.assertEqual(
+            0, len(current_jobs),
+            "There should be no ready ProcessApportBlobJobs. Found %s" %
+            len(current_jobs))
+
+        yet_another_job = ProcessApportBlobJob.create(self.blob)
+        current_jobs = list(ProcessApportBlobJob.iterReady())
+        self.assertEqual(
+            0, len(current_jobs),
+            "There should be no new ProcessApportBlobJobs. Found %s" %
+            len(current_jobs))
+
+        # In fact, yet_another_job will be the same job as before, since
+        # it's attached to the same BLOB.
+        self.assertEqual(job.id, yet_another_job.id, "Jobs do not match.")
+
+
+class TestTemporaryBlobStorageAddView(TestCaseWithFactory):
+    """Test case for the TemporaryBlobStorageAddView."""
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(TestTemporaryBlobStorageAddView, self).setUp()
+
+        # Create a BLOB using existing testing data.
+        testfiles = os.path.join(config.root, 'lib/lp/bugs/tests/testfiles')
+        blob_file = open(
+            os.path.join(testfiles, 'extra_filebug_data.msg'))
+        self.blob_data = blob_file.read()
+        blob_file.close()
+
+    def test_adding_blob_adds_job(self):
+        # Using the TemporaryBlobStorageAddView to upload a new BLOB
+        # will add a new ProcessApportBlobJob for that BLOB.
+        view = create_initialized_view(
+            getUtility(ILaunchpadRoot), '+storeblob')
+
+        # The view's store_blob method stores the blob in the database
+        # and returns its UUID.
+        blob_uuid = view.store_blob(self.blob_data)
+        transaction.commit()
+
+        # A new ProcessApportBlobJob will have been created for the
+        # BLOB.
+        blob = getUtility(ITemporaryStorageManager).fetch(blob_uuid)
+        job = ProcessApportBlobJob.getByBlobUUID(blob_uuid)
+
+        self.assertEqual(
+            blob, job.blob,
+            "BLOB attached to Job returned by getByBlobUUID() did not match "
+            "expected BLOB.")
 
 
 def test_suite():

=== modified file 'lib/lp/bugs/utilities/filebugdataparser.py'
--- lib/lp/bugs/utilities/filebugdataparser.py	2010-02-03 22:20:14 +0000
+++ lib/lp/bugs/utilities/filebugdataparser.py	2010-02-11 12:16:27 +0000
@@ -4,6 +4,7 @@
 
 __metaclass__ = type
 __all__ = [
+    'FileBugData',
     'FileBugDataParser',
     ]