launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-store-upload-error-debug into lp:launchpad.

Commit message:
Include the full response from unscanned-upload as an error_detail variable in BadUploadResponse OOPSes.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-store-upload-error-debug/+merge/322715

I'm hoping to get something I can use to figure out bug 1683819.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-store-upload-error-debug into lp:launchpad.

=== modified file 'lib/lp/snappy/interfaces/snapbuildjob.py'
--- lib/lp/snappy/interfaces/snapbuildjob.py	2017-04-03 14:43:22 +0000
+++ lib/lp/snappy/interfaces/snapbuildjob.py	2017-04-18 21:59:36 +0000
@@ -57,6 +57,9 @@
     error_message = TextLine(
         title=_("Error message"), required=False, readonly=True)
 
+    error_detail = TextLine(
+        title=_("Error message detail"), required=False, readonly=True)
+
     store_url = TextLine(
         title=_("The URL on the store corresponding to this build"),
         required=False, readonly=True)

=== modified file 'lib/lp/snappy/interfaces/snapstoreclient.py'
--- lib/lp/snappy/interfaces/snapstoreclient.py	2016-12-08 17:39:19 +0000
+++ lib/lp/snappy/interfaces/snapstoreclient.py	2017-04-18 21:59:36 +0000
@@ -1,4 +1,4 @@
-# Copyright 2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2016-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Interface for communication with the snap store."""
@@ -17,6 +17,7 @@
     'NeedsRefreshResponse',
     'ReleaseFailedResponse',
     'ScanFailedResponse',
+    'SnapStoreError',
     'UnauthorizedUploadResponse',
     'UploadNotScannedYetResponse',
     ]
@@ -27,48 +28,56 @@
 from zope.interface import Interface
 
 
+class SnapStoreError(Exception):
+
+    def __init__(self, message="", detail=None):
+        super(SnapStoreError, self).__init__(message)
+        self.message = message
+        self.detail = detail
+
+
 @error_status(httplib.INTERNAL_SERVER_ERROR)
-class BadRequestPackageUploadResponse(Exception):
-    pass
-
-
-class BadUploadResponse(Exception):
-    pass
-
-
-class BadRefreshResponse(Exception):
-    pass
-
-
-class NeedsRefreshResponse(Exception):
-    pass
-
-
-class UnauthorizedUploadResponse(Exception):
-    pass
-
-
-class BadScanStatusResponse(Exception):
-    pass
-
-
-class UploadNotScannedYetResponse(Exception):
-    pass
-
-
-class ScanFailedResponse(Exception):
-    pass
-
-
-class BadSearchResponse(Exception):
-    pass
-
-
-class BadReleaseResponse(Exception):
-    pass
-
-
-class ReleaseFailedResponse(Exception):
+class BadRequestPackageUploadResponse(SnapStoreError):
+    pass
+
+
+class BadUploadResponse(SnapStoreError):
+    pass
+
+
+class BadRefreshResponse(SnapStoreError):
+    pass
+
+
+class NeedsRefreshResponse(SnapStoreError):
+    pass
+
+
+class UnauthorizedUploadResponse(SnapStoreError):
+    pass
+
+
+class BadScanStatusResponse(SnapStoreError):
+    pass
+
+
+class UploadNotScannedYetResponse(SnapStoreError):
+    pass
+
+
+class ScanFailedResponse(SnapStoreError):
+    pass
+
+
+class BadSearchResponse(SnapStoreError):
+    pass
+
+
+class BadReleaseResponse(SnapStoreError):
+    pass
+
+
+class ReleaseFailedResponse(SnapStoreError):
     pass
 
 

=== modified file 'lib/lp/snappy/model/snapbuildjob.py'
--- lib/lp/snappy/model/snapbuildjob.py	2017-04-03 14:43:22 +0000
+++ lib/lp/snappy/model/snapbuildjob.py	2017-04-18 21:59:36 +0000
@@ -61,6 +61,7 @@
     ISnapStoreClient,
     ReleaseFailedResponse,
     ScanFailedResponse,
+    SnapStoreError,
     UnauthorizedUploadResponse,
     UploadNotScannedYetResponse,
     )
@@ -165,7 +166,7 @@
         return oops_vars
 
 
-class ManualReview(Exception):
+class ManualReview(SnapStoreError):
     pass
 
 
@@ -216,6 +217,16 @@
         self.metadata["error_message"] = message
 
     @property
+    def error_detail(self):
+        """See `ISnapStoreUploadJob`."""
+        return self.metadata.get("error_detail")
+
+    @error_detail.setter
+    def error_detail(self, detail):
+        """See `ISnapStoreUploadJob`."""
+        self.metadata["error_detail"] = detail
+
+    @property
     def store_url(self):
         """See `ISnapStoreUploadJob`."""
         return self.metadata.get("store_url")
@@ -262,6 +273,12 @@
     def resume(self, *args, **kwargs):
         self._do_lifecycle(self.job.resume, *args, **kwargs)
 
+    def getOopsVars(self):
+        """See `IRunnableJob`."""
+        oops_vars = super(SnapStoreUploadJob, self).getOopsVars()
+        oops_vars.append(('error_detail', self.error_detail))
+        return oops_vars
+
     def run(self):
         """See `IRunnableJob`."""
         client = getUtility(ISnapStoreClient)
@@ -282,6 +299,7 @@
             raise
         except Exception as e:
             self.error_message = str(e)
+            self.error_detail = getattr(e, "detail", None)
             if isinstance(e, UnauthorizedUploadResponse):
                 mailer = SnapBuildMailer.forUnauthorizedUpload(self.snapbuild)
                 mailer.sendAll()

=== modified file 'lib/lp/snappy/model/snapstoreclient.py'
--- lib/lp/snappy/model/snapstoreclient.py	2017-02-27 12:01:08 +0000
+++ lib/lp/snappy/model/snapstoreclient.py	2017-04-18 21:59:36 +0000
@@ -179,13 +179,22 @@
             try:
                 response = urlfetch(
                     unscanned_upload_url, method="POST", data=encoder,
-                    headers={"Content-Type": encoder.content_type})
+                    headers={
+                        "Content-Type": encoder.content_type,
+                        "Accept": "application/json",
+                        })
                 response_data = response.json()
                 if not response_data.get("successful", False):
                     raise BadUploadResponse(response.text)
                 return {"upload_id": response_data["upload_id"]}
             except requests.HTTPError as e:
-                raise BadUploadResponse(e.args[0])
+                if e.response is not None:
+                    detail = e.response.content
+                    if isinstance(detail, bytes):
+                        detail = detail.decode("UTF-8", errors="replace")
+                else:
+                    detail = None
+                raise BadUploadResponse(e.args[0], detail=detail)
         finally:
             lfa.close()
 

=== modified file 'lib/lp/snappy/tests/test_snapbuildjob.py'
--- lib/lp/snappy/tests/test_snapbuildjob.py	2017-04-03 14:43:22 +0000
+++ lib/lp/snappy/tests/test_snapbuildjob.py	2017-04-18 21:59:36 +0000
@@ -239,7 +239,8 @@
         self.assertContentEqual([], snapbuild.store_upload_jobs)
         job = SnapStoreUploadJob.create(snapbuild)
         client = FakeSnapStoreClient()
-        client.upload.failure = BadUploadResponse("Failed to upload")
+        client.upload.failure = BadUploadResponse(
+            "Failed to upload", detail="The proxy exploded.\n")
         self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
         with dbuser(config.ISnapStoreUploadJobSource.dbuser):
             JobRunner([job]).runAll()
@@ -277,6 +278,8 @@
             build_url, footer)
         self.assertWebhookDeliveries(
             snapbuild, ["Pending", "Failed to upload"])
+        self.assertIn(
+            ("error_detail", "The proxy exploded.\n"), job.getOopsVars())
 
     def test_run_scan_pending_retries(self):
         # A run that finds that the store has not yet finished scanning the

=== modified file 'lib/lp/snappy/tests/test_snapstoreclient.py'
--- lib/lp/snappy/tests/test_snapstoreclient.py	2017-02-27 12:01:08 +0000
+++ lib/lp/snappy/tests/test_snapstoreclient.py	2017-04-18 21:59:36 +0000
@@ -48,6 +48,7 @@
     BadRequestPackageUploadResponse,
     BadScanStatusResponse,
     BadSearchResponse,
+    BadUploadResponse,
     ISnapStoreClient,
     ReleaseFailedResponse,
     ScanFailedResponse,
@@ -445,6 +446,25 @@
             store_secrets["discharge"],
             snapbuild.snap.store_secrets["discharge"])
 
+    def test_upload_file_error(self):
+        @urlmatch(path=r".*/unscanned-upload/$")
+        def unscanned_upload_handler(url, request):
+            return {
+                "status_code": 502,
+                "reason": "Proxy Error",
+                "content": b"The proxy exploded.\n",
+                }
+
+        store_secrets = self._make_store_secrets()
+        snapbuild = self.makeUploadableSnapBuild(store_secrets=store_secrets)
+        transaction.commit()
+        with dbuser(config.ISnapStoreUploadJobSource.dbuser):
+            with HTTMock(unscanned_upload_handler):
+                err = self.assertRaises(
+                    BadUploadResponse, self.client.upload, snapbuild)
+                self.assertEqual("502 Server Error: Proxy Error", str(err))
+                self.assertEqual(b"The proxy exploded.\n", err.detail)
+
     def test_refresh_discharge_macaroon(self):
         store_secrets = self._make_store_secrets()
         snap = self.factory.makeSnap(