Fix request error handling (#210)

* throw if error response is received * Fix error handling Ensures that non-2xx response codes return an error * Add underlying server message to response error
coinbase · Jan 18, 2018 · ba94d6e · ba94d6e
1 parent 50ef51b
commit ba94d6e
Show file tree

Hide file tree

Showing 4 changed files with 66 additions and 17 deletions.
diff --git a/lib/clients/authenticated.js b/lib/clients/authenticated.js
@@ -139,9 +139,7 @@ class AuthenticatedClient extends PublicClient {
     const p = function deleteNext() {
       return new Promise((resolve, reject) => {
         this.delete(['orders'], opts, (err, response, data) => {
-          if (err || data === null) {
-            err = err || new Error('Could not delete all orders');
-            err.response = response;
+          if (err) {
             reject(err);
           } else {
             resolve([response, data]);

diff --git a/lib/clients/public.js b/lib/clients/public.js
@@ -60,17 +60,30 @@ class PublicClient {
       } catch (e) {
         data = null;
       }
-      if (typeof callback === 'function') {
-        callback(err, response, data);
-        return;
-      }
+
       if (err) {
         err.response = response;
-        reject(err);
-      }
-      if (data === null) {
+      } else if (response.statusCode > 299) {
+        err = new Error(
+          `HTTP ${response.statusCode} Error: ${data && data.message}`
+        );
+        err.response = response;
+        err.data = data;
+      } else if (data === null) {
         err = new Error('Response could not be parsed as JSON');
         err.response = response;
+      }
+
+      if (typeof callback === 'function') {
+        if (err) {
+          callback(err);
+        } else {
+          callback(null, response, data);
+        }
+        return;
+      }
+
+      if (err) {
         reject(err);
       } else {
         resolve(data);

diff --git a/tests/authenticated.spec.js b/tests/authenticated.spec.js
@@ -405,24 +405,29 @@ suite('AuthenticatedClient', () => {
       nock(EXCHANGE_API_URL)
         .delete('/orders')
         .times(2)
-        .reply(404, null);
+        .reply(400, { message: 'some error' });
 
-      let cbTest = new Promise((resolve, reject) => {
+      const cbtest = new Promise((resolve, reject) => {
         authClient.cancelAllOrders(err => {
           if (err) {
+            assert.equal(err.response.statusCode, 400);
+            assert.equal(err.data.message, 'some error');
             resolve();
           } else {
             reject();
           }
         });
       });
 
-      return cbTest
-        .then(() => {
-          return authClient.cancelAllOrders();
-        })
+      const promisetest = authClient
+        .cancelAllOrders()
         .then(() => assert.fail('should have thrown an error'))
-        .catch(err => assert(err));
+        .catch(err => {
+          assert.equal(err.response.statusCode, 400);
+          assert.equal(err.data.message, 'some error');
+        });
+
+      return Promise.all([cbtest, promisetest]);
     });
   });
 

diff --git a/tests/public_client.spec.js b/tests/public_client.spec.js
@@ -27,6 +27,39 @@ suite('PublicClient', () => {
     assert.equal(client.productID, 'LTC-USD');
   });
 
+  suite('.request()', () => {
+    test('handles errors', () => {
+      nock(EXCHANGE_API_URL)
+        .get('/some/path')
+        .times(2)
+        .reply(400, { message: 'some error' });
+
+      const cbtest = new Promise((resolve, reject) => {
+        publicClient.request('get', ['some', 'path'], {}, err => {
+          if (err) {
+            assert.equal(err.message, 'HTTP 400 Error: some error');
+            assert.equal(err.response.statusCode, 400);
+            assert.equal(err.data.message, 'some error');
+            resolve();
+          } else {
+            reject();
+          }
+        });
+      });
+
+      const promisetest = publicClient
+        .request('get', ['some', 'path'])
+        .then(() => assert.fail('should have thrown an error'))
+        .catch(err => {
+          assert.equal(err.message, 'HTTP 400 Error: some error');
+          assert.equal(err.response.statusCode, 400);
+          assert(err.data.message, 'some error');
+        });
+
+      return Promise.all([cbtest, promisetest]);
+    });
+  });
+
   test('.getProductOrderBook()', () => {
     nock(EXCHANGE_API_URL)
       .get('/products/LTC-USD/book?level=3')