Merge lp:~bryce/launchpad/lp-548824 into lp:launchpad

[Bryce Harrington]

[r=gmb][ui=none][bug=548824] Add a getProcessedData() API call to retrieve the metadata about the uploaded blob.

Adds a getProcessedData() API call to retrieve the metadata about the uploaded blob.

This should enable apport to do further testing on blobs it's uploaded.

[Bryce Harrington (bryce)]

This passes ec2 test without issue, however I'm unsure about if I've done the tests correctly. Grahm, I'd appreciate it if you could give this a really deep review, and suggest how to do the tests better. (For instance, should it examine the data items inside the metadata itself?)

[Graham Binns (gmb)]

This looks fine, but you should add a test to lib/canonical/launchpad/pagetests/webservice/xx-temporary-blob-storage.txt to cover calling the method from the API. Other than that this is fine.

review: Needs Fixing (code)

[Bryce Harrington (bryce)]

I'm finally quite happy with this.

It turned out that calling .start() and .complete() on the job wasn't sufficient... it needed job.run() in order to actually parse out the metadata. Now it works swimmingly!

In rummaging around in the apport job internals I found there was some additional interesting metadata about the job itself, which I found interesting for debugging, but may or may not be of any interest for testing purposes. I opted to add a getJobStatus() routine to expose this info for apport's tests to use. Please review. If it looks like either something apport won't care about, or that it is something we don't want to expose, it's easily excised from this change.

[Graham Binns (gmb)]

> I'm finally quite happy with this.
>
> It turned out that calling .start() and .complete() on the job wasn't
> sufficient... it needed job.run() in order to actually parse out the metadata.
> Now it works swimmingly!
>

Hah. Shows what my memory's like, doesn't it?

> In rummaging around in the apport job internals I found there was some
> additional interesting metadata about the job itself, which I found
> interesting for debugging, but may or may not be of any interest for testing
> purposes. I opted to add a getJobStatus() routine to expose this info for
> apport's tests to use. Please review. If it looks like either something
> apport won't care about, or that it is something we don't want to expose, it's
> easily excised from this change.

I don't think we should expose this; it should be enough for people to be able to query to see whether the job has completed yet. Also, if we were going to expose this metadata we'd probably want to do it at the IJob level, since there are several systems in Launchpad (e.g. diff generation for merge proposals) which use it. Nice idea, though :).

I'm happy with the rest of the branch. r=me with the getJobStatus() code removed. Good work!

review: Approve (code)

=== modified file 'lib/canonical/launchpad/database/temporaryblobstorage.py'
--- lib/canonical/launchpad/database/temporaryblobstorage.py	2010-02-26 17:35:01 +0000
+++ lib/canonical/launchpad/database/temporaryblobstorage.py	2010-05-28 15:37:41 +0000
@@ -58,18 +58,34 @@
         finally:
             self.file_alias.close()
 
-    def hasBeenProcessed(self):
-        """See `ITemporaryBlobStorage`."""
+    @property
+    def _apport_job(self):
         # Imported here to avoid circular imports
         from lp.bugs.interfaces.apportjob import IProcessApportBlobJobSource
         try:
             job_for_blob = getUtility(
                 IProcessApportBlobJobSource).getByBlobUUID(self.uuid)
         except SQLObjectNotFound:
+            return None
+
+        return job_for_blob
+
+    def hasBeenProcessed(self):
+        """See `ITemporaryBlobStorage`."""
+        job_for_blob = self._apport_job
+        if not job_for_blob:
             return False
-
         return (job_for_blob.job.status == JobStatus.COMPLETED)
 
+    def getProcessedData(self):
+        """See `ITemporaryBlobStorage`."""
+        job_for_blob = self._apport_job
+        if not job_for_blob:
+            return None
+        if 'processed_data' not in job_for_blob.metadata:
+            return {}
+        
+        return job_for_blob.metadata['processed_data']
 
 class TemporaryStorageManager:
     """A tool to create temporary BLOB's in Launchpad."""

=== modified file 'lib/canonical/launchpad/interfaces/temporaryblobstorage.py'
--- lib/canonical/launchpad/interfaces/temporaryblobstorage.py	2010-02-26 17:35:01 +0000
+++ lib/canonical/launchpad/interfaces/temporaryblobstorage.py	2010-05-28 15:37:41 +0000
@@ -49,6 +49,10 @@
     def hasBeenProcessed():
         """Return True if this blob has been processed."""
 
+    @export_read_operation()
+    def getProcessedData():
+        """Returns a dict containing the processed blob data."""
+
 
 class ITemporaryStorageManager(Interface):
     """A tool to create temporary blobs."""

=== modified file 'lib/canonical/launchpad/pagetests/webservice/xx-temporary-blob-storage.txt'
--- lib/canonical/launchpad/pagetests/webservice/xx-temporary-blob-storage.txt	2010-02-26 17:35:01 +0000
+++ lib/canonical/launchpad/pagetests/webservice/xx-temporary-blob-storage.txt	2010-05-28 15:37:41 +0000
@@ -13,11 +13,23 @@
 set.
 
     >>> login('[e-mail]')
+    >>> import os
     >>> from zope.component import getUtility
+    >>> from canonical.config import config
     >>> from canonical.launchpad.interfaces.temporaryblobstorage import (
     ...     ITemporaryStorageManager)
 
-    >>> blob_token = getUtility(ITemporaryStorageManager).new("some data")
+    >>> testfiles = os.path.join(config.root,
+    ...     'lib/lp/bugs/tests/testfiles')
+    >>> blob_file = open(
+    ...     os.path.join(testfiles, 'extra_filebug_data.msg'))
+    >>> blob_data = blob_file.read()
+    >>> print blob_data[307:373]
+    --boundary
+    Content-disposition: attachment; filename='attachment1'
+
+    >>> blob_token = getUtility(ITemporaryStorageManager).new(blob_data)
+
     >>> logout()
 
     >>> temporary_blobs = webservice.get('/temporary-blobs').jsonBody()
@@ -48,7 +60,6 @@
     >>> blob['token'] == blob_token
     True
 
-
 Checking whether a blob has been processed
 ------------------------------------------
 
@@ -65,6 +76,13 @@
     ...     blob['self_link'], 'hasBeenProcessed').jsonBody()
     False
 
+However, since the blob has not been processed there will be no
+job processed data at this point.
+
+    >>> print webservice.named_get(
+    ...     blob['self_link'], 'getProcessedData').jsonBody()
+    None
+
 Once the blob has been processed, its hasBeenProcessed() method will
 return True.
 
@@ -74,8 +92,24 @@
     ...     getUtility(ITemporaryStorageManager).fetch(blob_token))
     >>> job.job.start()
     >>> job.job.complete()
+    >>> job.run()
     >>> logout()
 
     >>> print webservice.named_get(
     ...     blob['self_link'], 'hasBeenProcessed').jsonBody()
     True
+
+And now the blob's parsed-out metadata is now accessible.
+
+    >>> metadata = webservice.named_get(
+    ...     blob['self_link'], 'getProcessedData').jsonBody()
+
+    >>> print metadata['extra_description']
+    This should be added to the description.
+
+    >>> print len(metadata['comments'])
+    2
+
+    >>> attachment = metadata['attachments'][0]
+    >>> print attachment['description']
+    attachment1

=== modified file 'lib/lp/bugs/tests/test_apportjob.py'
--- lib/lp/bugs/tests/test_apportjob.py	2010-04-13 13:13:45 +0000
+++ lib/lp/bugs/tests/test_apportjob.py	2010-05-28 15:37:41 +0000
@@ -338,6 +338,31 @@
         view.publishTraverse(view.request, blob_uuid)
         return view
 
+    def test_blob_has_been_processed(self):
+        # Using the TemporaryBlobStorageAddView to upload a new BLOB
+        # will show blob as being processed
+        blob_uuid = self._create_blob_and_job_using_storeblob()
+        blob = getUtility(ITemporaryStorageManager).fetch(blob_uuid)
+
+        self.assertFalse(
+            blob.hasBeenProcessed(),
+            "BLOB should not be processed, but indicates it has.")
+
+    def test_blob_get_processed_data(self):
+        # Using the TemporaryBlobStorageAddView to upload a new BLOB
+        # should indicate there two attachments were processed.
+        blob_uuid = self._create_blob_and_job_using_storeblob()
+        blob = getUtility(ITemporaryStorageManager).fetch(blob_uuid)
+        job = getUtility(IProcessApportBlobJobSource).getByBlobUUID(blob_uuid)
+        job.job.start()
+        job.job.complete()
+        job.run()
+        blob_meta = blob.getProcessedData()
+
+        self.assertEqual(
+            len(blob_meta['attachments']), 2,
+            "BLOB metadata: %s" %(str(blob_meta)))
+
     def test_adding_blob_adds_job(self):
         # Using the TemporaryBlobStorageAddView to upload a new BLOB
         # will add a new ProcessApportBlobJob for that BLOB.