Includes the response in the error message when the response bod… (#449)

Tyler-Murphy · stephenplusplus · commit 1714769022dc · 2019-07-24T12:41:36.000-04:00
* Includes the response in the error message when the response body can't be parsed as JSON.

* Adds ApiError to the possible types for the `err` property of `BodyResponseCallback`, since `handleResp` sometimes makes and attaches `ApiError`s.

* Attaches the HTTP response to the errors when possible so that it's possible to see the raw response body.
diff --git a/core/common/src/util.ts b/core/common/src/util.ts
@@ -300,7 +300,7 @@ export class PartialFailureError extends Error {
 }
 
 export interface BodyResponseCallback {
-  (err: Error | null, body?: ResponseBody, res?: r.Response): void;
+  (err: Error | ApiError | null, body?: ResponseBody, res?: r.Response): void;
 }
 
 export interface MakeRequestConfig {
@@ -367,6 +367,10 @@ export class Util {
       parsedResp.resp.body = parsedResp.body;
     }
 
+    if (parsedResp.err && resp) {
+      parsedResp.err.response = resp;
+    }
+
     callback(parsedResp.err, parsedResp.body, parsedResp.resp);
   }
 
@@ -404,7 +408,7 @@ export class Util {
    * @param {?error} parsedHttpRespMessage.err - An error detected.
    * @param {object} parsedHttpRespMessage.body - The original body value provided
    *     will try to be JSON.parse'd. If it's successful, the parsed value will
-   * be returned here, otherwise the original value.
+   * be returned here, otherwise the original value and an error will be returned.
    */
   parseHttpRespBody(body: ResponseBody) {
     const parsedHttpRespBody: ParsedHttpResponseBody = {
@@ -415,7 +419,9 @@ export class Util {
       try {
         parsedHttpRespBody.body = JSON.parse(body);
       } catch (err) {
-        parsedHttpRespBody.err = new ApiError('Cannot parse JSON response');
+        parsedHttpRespBody.err = new ApiError(
+          `Cannot parse response as JSON: ${body}`
+        );
       }
     }
 
diff --git a/core/common/test/util.ts b/core/common/test/util.ts
@@ -377,6 +377,45 @@ describe('common/util', () => {
       stub('parseHttpRespBody', () => done()); // Will throw.
       util.handleResp(null, null, null, done);
     });
+
+    it('should surface the response body in the error message when it cannot be JSON-parsed', done => {
+      const unparseableBody = '<html>There was some error</html>';
+
+      util.handleResp(null, null, unparseableBody, err => {
+        if (err === null) {
+          assert.fail('there should be an error');
+        } else {
+          assert.ok(err.message.includes(unparseableBody));
+        }
+
+        done();
+      });
+    });
+
+    it('should surface the response body as an error property when it cannot be JSON-parsed', done => {
+      const unparseableBody = '<html>There was some error</html>';
+
+      util.handleResp(
+        null,
+        {body: unparseableBody} as r.Response,
+        unparseableBody,
+        err => {
+          if (err === null) {
+            assert.fail('there should be an error');
+          } else {
+            const response = (err! as ApiError).response;
+
+            if (!response) {
+              assert.fail('there should be a response property on the error');
+            } else {
+              assert.strictEqual(response.body, unparseableBody);
+            }
+          }
+
+          done();
+        }
+      );
+    });
   });
 
   describe('parseHttpRespMessage', () => {

Original file line number	Diff line number	Diff line change
`@@ -300,7 +300,7 @@ export class PartialFailureError extends Error {`
`300`	`300`	`}`
`301`	`301`
`302`	`302`	`export interface BodyResponseCallback {`
`303`		`- (err: Error \| null, body?: ResponseBody, res?: r.Response): void;`
	`303`	`+ (err: Error \| ApiError \| null, body?: ResponseBody, res?: r.Response): void;`
`304`	`304`	`}`
`305`	`305`
`306`	`306`	`export interface MakeRequestConfig {`
`@@ -367,6 +367,10 @@ export class Util {`
`367`	`367`	`parsedResp.resp.body = parsedResp.body;`
`368`	`368`	`}`
`369`	`369`
	`370`	`+ if (parsedResp.err && resp) {`
	`371`	`+ parsedResp.err.response = resp;`
	`372`	`+ }`
	`373`	`+`
`370`	`374`	`callback(parsedResp.err, parsedResp.body, parsedResp.resp);`
`371`	`375`	`}`
`372`	`376`
`@@ -404,7 +408,7 @@ export class Util {`
`404`	`408`	`* @param {?error} parsedHttpRespMessage.err - An error detected.`
`405`	`409`	`* @param {object} parsedHttpRespMessage.body - The original body value provided`
`406`	`410`	`* will try to be JSON.parse'd. If it's successful, the parsed value will`
`407`		`- * be returned here, otherwise the original value.`
	`411`	`+ * be returned here, otherwise the original value and an error will be returned.`
`408`	`412`	`*/`
`409`	`413`	`parseHttpRespBody(body: ResponseBody) {`
`410`	`414`	`const parsedHttpRespBody: ParsedHttpResponseBody = {`
`@@ -415,7 +419,9 @@ export class Util {`
`415`	`419`	`try {`
`416`	`420`	`parsedHttpRespBody.body = JSON.parse(body);`
`417`	`421`	`} catch (err) {`
`418`		`- parsedHttpRespBody.err = new ApiError('Cannot parse JSON response');`
	`422`	`+ parsedHttpRespBody.err = new ApiError(`
	`423`	+ `Cannot parse response as JSON: ${body}`
	`424`	`+ );`
`419`	`425`	`}`
`420`	`426`	`}`
`421`	`427`