Repository: dwavesystems/dwave-cloud-client

Issue:
Bubble up SAPI error messages during all stages of multipart upload
For example, if a too big upload size is requested (during multipart upload initialize), SAPI responds with:
```
{"error_code": 400, "error_msg": "The proposed upload is bigger than the maximum upload allowed of 42949672960"}
```
but the exception raised is `HTTPError('400 Client Error: Bad Request ...')`.

The expected behavior in this case is to fail with `ProblemUploadError("The proposed upload is bigger than ...")` or similar.

Discussion:

Patch:
diff --git a/dwave/cloud/client.py b/dwave/cloud/client.py
index baf8341..4744bf9 100644
--- a/dwave/cloud/client.py
+++ b/dwave/cloud/client.py
@@ -44,12 +44,12 @@ import sys
 import time
 import json
 import copy
+import queue
 import logging
-import threading
-import requests
+import inspect
 import warnings
 import operator
-import queue
+import threading
 
 import base64
 import hashlib
@@ -61,6 +61,7 @@ from functools import partial, wraps
 from collections import abc, namedtuple, OrderedDict
 from concurrent.futures import ThreadPoolExecutor
 
+import requests
 from dateutil.parser import parse as parse_datetime
 from plucky import pluck
 
@@ -1598,6 +1599,31 @@ class Client(object):
         return self._upload_problem_executor.submit(
             self._upload_problem_worker, problem=problem, problem_id=problem_id)
 
+    @staticmethod
+    def _parse_multipart_upload_response(response):
+        """Parse the JSON response body, raising appropriate exceptions."""
+
+        caller = inspect.stack()[1].function
+        logger.trace("%s response: (code=%r, text=%r)",
+                     caller, response.status_code, response.text)
+
+        if response.status_code == 401:
+            raise SolverAuthenticationError()
+
+        try:
+            msg = response.json()
+        except:
+            response.raise_for_status()
+
+        if response.status_code != 200:
+            try:
+                error_msg = msg['error_msg']
+            except KeyError:
+                response.raise_for_status()
+            raise ProblemUploadError(error_msg)
+
+        return msg
+
     @staticmethod
     @retried(_UPLOAD_REQUEST_RETRIES, backoff=_UPLOAD_RETRIES_BACKOFF)
     def _initiate_multipart_upload(session, size):
@@ -1614,14 +1640,10 @@ class Client(object):
         except requests.exceptions.Timeout:
             raise RequestTimeout
 
-        if response.status_code == 401:
-            raise SolverAuthenticationError()
-        else:
-            logger.trace("Multipart upload initiate response: %r", response.text)
-            response.raise_for_status()
+        msg = Client._parse_multipart_upload_response(response)
 
         try:
-            problem_id = response.json()['id']
+            problem_id = msg['id']
         except KeyError:
             raise InvalidAPIResponseError("problem ID missing")
 
@@ -1715,11 +1737,7 @@ class Client(object):
         except requests.exceptions.Timeout:
             raise RequestTimeout
 
-        if response.status_code == 401:
-            raise SolverAuthenticationError()
-        else:
-            logger.trace("Part upload response: %r", response.text)
-            response.raise_for_status()
+        msg = Client._parse_multipart_upload_response(response)
 
         logger.debug("Uploaded part_id=%r of problem_id=%r", part_id, problem_id)
 
@@ -1738,23 +1756,18 @@ class Client(object):
         except requests.exceptions.Timeout:
             raise RequestTimeout
 
-        if response.status_code == 401:
-            raise SolverAuthenticationError()
-        else:
-            logger.trace("Upload status response: %r", response.text)
-            response.raise_for_status()
+        msg = Client._parse_multipart_upload_response(response)
 
         try:
-            problem_status = response.json()
-            problem_status['status']
-            problem_status['parts']
+            msg['status']
+            msg['parts']
         except KeyError:
             raise InvalidAPIResponseError("'status' and/or 'parts' missing")
 
         logger.debug("Got upload status=%r for problem_id=%r",
-                     problem_status['status'], problem_id)
+                     msg['status'], problem_id)
 
-        return problem_status
+        return msg
 
     @staticmethod
     def _failsafe_get_multipart_upload_status(session, problem_id):
@@ -1780,11 +1793,7 @@ class Client(object):
         except requests.exceptions.Timeout:
             raise RequestTimeout
 
-        if response.status_code == 401:
-            raise SolverAuthenticationError()
-        else:
-            logger.trace("Combine parts response: %r", response.text)
-            response.raise_for_status()
+        msg = Client._parse_multipart_upload_response(response)
 
         logger.debug("Issued a combine command for problem_id=%r", problem_id)
 
@@ -1825,7 +1834,7 @@ class Client(object):
         # via executor initializer
         with self.create_session() as session:
             chunks = ChunkedData(problem, chunk_size=self._UPLOAD_PART_SIZE_BYTES)
-            size = len(chunks.view)
+            size = chunks.total_size
 
             if problem_id is None:
                 try:
@@ -1834,7 +1843,7 @@ class Client(object):
                     errmsg = ("Multipart upload initialization failed "
                               "with {!r}.".format(e))
                     logger.error(errmsg)
-                    raise ProblemUploadError(errmsg)
+                    raise ProblemUploadError(errmsg) from e
 
             # check problem status, so we only upload parts missing or invalid
             problem_status = \
@@ -1869,7 +1878,7 @@ class Client(object):
                     errmsg = ("Multipart upload of problem_id={!r} failed for "
                               "part_no={!r} with {!r}.".format(problem_id, part_no, e))
                     logger.error(errmsg)
-                    raise ProblemUploadError(errmsg)
+                    raise ProblemUploadError(errmsg) from e
 
             # verify all parts uploaded via status call
             # (check remote checksum matches the local one)
@@ -1904,6 +1913,6 @@ class Client(object):
                 errmsg = ("Multipart upload of problem_id={!r} failed on parts "
                           "combine with {!r}".format(problem_id, e))
                 logger.error(errmsg)
-                raise ProblemUploadError(errmsg)
+                raise ProblemUploadError(errmsg) from e
 
             return problem_id
diff --git a/dwave/cloud/events.py b/dwave/cloud/events.py
index efe1809..8e9b53d 100644
--- a/dwave/cloud/events.py
+++ b/dwave/cloud/events.py
@@ -62,6 +62,8 @@ def add_handler(name, handler):
 def dispatch_event(name, *args, **kwargs):
     """Call the complete chain of event handlers attached to event `name`."""
 
+    logger.trace("dispatch_event(%r, *%r, **%r)", name, args, kwargs)
+
     if name not in _client_event_hooks_registry:
         raise ValueError('invalid event name')
 
diff --git a/dwave/cloud/upload.py b/dwave/cloud/upload.py
index 85d603b..87bc306 100644
--- a/dwave/cloud/upload.py
+++ b/dwave/cloud/upload.py
@@ -372,12 +372,15 @@ class ChunkedData(object):
         if self.view is None:
             raise TypeError("bytes/str/IOBase-subclass data required")
 
+    @property
+    def total_size(self):
+        """Total data size, in bytes."""
+        return len(self.view)
+
     @property
     def num_chunks(self):
         """Total number of chunks."""
-
-        total_size = len(self.view)
-        return math.ceil(total_size / self.chunk_size)
+        return math.ceil(self.total_size / self.chunk_size)
 
     def __len__(self):
         return self.num_chunks


Test patch:
diff --git a/tests/test_upload.py b/tests/test_upload.py
index 6717d1e..4a62f68 100644
--- a/tests/test_upload.py
+++ b/tests/test_upload.py
@@ -316,6 +316,7 @@ class TestChunkedData(unittest.TestCase):
     def verify_chunking(self, cd, chunks_expected):
         self.assertEqual(len(cd), len(chunks_expected))
         self.assertEqual(cd.num_chunks, len(chunks_expected))
+        self.assertEqual(cd.total_size, sum(map(len, chunks_expected)))
 
         chunks_iter = [c.read() for c in cd]
         chunks_explicit = []
@@ -365,19 +366,6 @@ class TestChunkedData(unittest.TestCase):
         self.assertListEqual(chunks_expected, chunks_generated)
 
 
-@unittest.skipUnless(config, "No live server configuration available.")
-class TestMultipartUpload(unittest.TestCase):
-
-    def test_smoke_test(self):
-        data = b'123'
-        with Client(**config) as client:
-            future = client.upload_problem_encoded(data)
-            try:
-                problem_id = future.result()
-            except Exception as e:
-                self.fail(e)
-
-
 def choose_reply(key, replies, statuses=None):
     """Choose the right response based on a hashable `key` and make a mock
     response.
@@ -462,7 +450,7 @@ class TestMockedMultipartUpload(unittest.TestCase):
                         part_data[i],
                         json.dumps(sorted([
                             ('Content-MD5', _b64(part_digest[i])),
-                            ('Content-Type',     'application/octet-stream')
+                            ('Content-Type', 'application/octet-stream')
                         ]))
                     ): json.dumps({})
                     for i in parts
@@ -548,7 +536,7 @@ class TestMockedMultipartUpload(unittest.TestCase):
                         part_data[i],
                         json.dumps(sorted([
                             ('Content-MD5', _b64(part_digest[i])),
-                            ('Content-Type',     'application/octet-stream')
+                            ('Content-Type', 'application/octet-stream')
                         ]))
                     ): json.dumps({})
                     for i in parts[:1]
@@ -633,7 +621,7 @@ class TestMockedMultipartUpload(unittest.TestCase):
                         part_data[i],
                         json.dumps(sorted([
                             ('Content-MD5', _b64(part_digest[i])),
-                            ('Content-Type',     'application/octet-stream')
+                            ('Content-Type', 'application/octet-stream')
                         ]))
                     ) for i in parts
                 ]
@@ -729,7 +717,7 @@ class TestMockedMultipartUpload(unittest.TestCase):
                         part_data[i],
                         json.dumps(sorted([
                             ('Content-MD5', _b64(part_digest[i])),
-                            ('Content-Type',     'application/octet-stream')
+                            ('Content-Type', 'application/octet-stream')
                         ]))
                     ): json.dumps({})
                     for i in parts[2:]
@@ -756,3 +744,35 @@ class TestMockedMultipartUpload(unittest.TestCase):
                     self.fail(e)
 
                 self.assertEqual(returned_problem_id, upload_problem_id)
+
+
+@unittest.skipUnless(config, "No live server configuration available.")
+class TestMultipartUpload(unittest.TestCase):
+    _100gb = 100 * 2**30
+
+    def test_smoke_test(self):
+        data = b'123'
+        with Client(**config) as client:
+            future = client.upload_problem_encoded(data)
+            try:
+                problem_id = future.result()
+            except Exception as e:
+                self.fail(e)
+
+    def test_initiate_size_limit(self):
+        size = self._100gb
+
+        with Client(**config) as client:
+            with client.create_session() as session:
+                with self.assertRaisesRegex(ProblemUploadError,
+                                            'bigger than the maximum'):
+                    client._initiate_multipart_upload(session, size)
+
+    @mock.patch.object(ChunkedData, 'total_size', _100gb)
+    def test_initiate_size_limit_end_to_end(self):
+
+        with Client(**config) as client:
+            future = client.upload_problem_encoded(b'')
+            with self.assertRaisesRegex(ProblemUploadError,
+                                        'bigger than the maximum'):
+                problem_id = future.result()