fix: return json error response body as object (#238)

Error response bodies are currently returned as strings, even when the content is JSON.
This is a bug - the SDK should return the result as an object. This commit adds logic
to return the result as an object, converting it if necessary.

It also updates the error object to use the "result" field for the response body, in
order to be consistent with the success path. The "body" field is still set for
backward-compatibility but is marked as deprecated.

Additionally, for both success and error responses, if the content is JSON but the
result cannot be parsed as an object, an exception will be raised, alerting the user
that the service returned a body with an invalid format.

Signed-off-by: Dustin Popp <dpopp07@gmail.com>

etc/ibm-cloud-sdk-core.api.md

@@ -309,6 +309,9 @@ export function isFileWithMetadata(obj: any): obj is FileWithMetadata;
 // @public
 export function isHTML(text: string): boolean;
 
+// @public
+export function isJsonMimeType(mimeType: string): boolean;
+
 // @public
 export class JwtTokenManager extends TokenManager {
     constructor(options: JwtTokenManagerOptions);

lib/helper.ts

@@ -1,5 +1,5 @@
 /**
- * (C) Copyright IBM Corp. 2014, 2022.
+ * (C) Copyright IBM Corp. 2014, 2023.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -340,3 +340,14 @@ export function constructServiceUrl(
 
   return formattedUrl;
 }
+
+/**
+ * Returns true if and only if "mimeType" is a "JSON-like" mime type
+ * (e.g. "application/json; charset=utf-8").
+ * @param mimeType - the mimeType string
+ * @returns true if "mimeType" represents a JSON media type and false otherwise
+ */
+export function isJsonMimeType(mimeType: string) {
+  logger.debug(`Determining if the mime type '${mimeType}' specifies JSON content.`);
+  return !!mimeType && /^application\/json(\s*;.*)?$/i.test(mimeType);
+}

lib/request-wrapper.ts

@@ -30,6 +30,7 @@ import {
   isEmptyObject,
   isFileData,
   isFileWithMetadata,
+  isJsonMimeType,
   stripTrailingSlash,
 } from './helper';
 import logger from './logger';
@@ -266,7 +267,7 @@ export class RequestWrapper {
 
         // the other sdks use the interface `result` for the body
         // eslint-disable-next-line @typescript-eslint/dot-notation
-        res['result'] = res.data;
+        res['result'] = ensureJSONResponseBodyIsObject(res);
         delete res.data;
 
         // return another promise that resolves with 'res' to be handled in generated code
@@ -306,20 +307,19 @@ export class RequestWrapper {
 
       error.message = parseServiceErrorMessage(axiosError.data) || axiosError.statusText;
 
-      // some services bury the useful error message within 'data'
-      // adding it to the error under the key 'body' as a string or object
+      // attach the error response body to the error
       let errorBody;
       try {
-        // try/catch to handle objects with circular references
+        // try/catch to detect objects with circular references
         errorBody = JSON.stringify(axiosError.data);
       } catch (e) {
-        // ignore the error, use the object, and tack on a warning
+        logger.warn('Error field `result` contains circular reference(s)');
+        logger.debug(`Failed to stringify error response body: ${e}`);
         errorBody = axiosError.data;
-        errorBody.warning = 'Body contains circular reference';
-        logger.error(`Failed to stringify axiosError: ${e}`);
       }
 
-      error.body = errorBody;
+      error.result = ensureJSONResponseBodyIsObject(axiosError);
+      error.body = errorBody; // ** deprecated **
 
       // attach headers to error object
       error.headers = axiosError.headers;
@@ -545,3 +545,36 @@ function parseServiceErrorMessage(response: any): string | undefined {
   logger.info(`Parsing service error message: ${message}`);
   return message;
 }
+
+/**
+ * Check response for a JSON content type and a string-formatted body. If those
+ * conditions are met, we want to return an object for the body to the user. If
+ * the JSON string coming from the service is invalid, we want to raise an
+ * exception.
+ *
+ * @param response - incoming response object
+ * @returns response body - as either an object or a string
+ * @throws error - if the content is meant as JSON but is malformed
+ */
+function ensureJSONResponseBodyIsObject(response: any): any | string {
+  if (typeof response.data !== 'string' || !isJsonMimeType(response.headers['content-type'])) {
+    return response.data;
+  }
+
+  // If the content is supposed to be JSON but axios gave us a string, it is most
+  // likely due to the fact that the service sent malformed JSON, which is an error.
+  //
+  // We'll try to parse the string and return a proper object to the user but if
+  // it fails, we'll log an error and raise an exception.
+
+  let dataAsObject = response.data;
+  try {
+    dataAsObject = JSON.parse(response.data);
+  } catch (e) {
+    logger.error('Response body was supposed to have JSON content but JSON parsing failed.');
+    logger.error(`Malformed JSON string: ${response.data}`);
+    throw e;
+  }
+
+  return dataAsObject;
+}

test/unit/is-json-mime-type.test.js

@@ -0,0 +1,80 @@
+/**
+ * (C) Copyright IBM Corp. 2023.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+const { isJsonMimeType } = require('../../dist/lib/helper');
+const logger = require('../../dist/lib/logger').default;
+
+const debugLogSpy = jest.spyOn(logger, 'debug').mockImplementation(() => {});
+
+describe('isJsonMimeType()', () => {
+  afterEach(() => {
+    debugLogSpy.mockClear();
+  });
+
+  it('should return `false` for `undefined`', async () => {
+    expect(isJsonMimeType(undefined)).toBe(false);
+    expect(debugLogSpy.mock.calls[0][0]).toBe(
+      "Determining if the mime type 'undefined' specifies JSON content."
+    );
+  });
+
+  it('should return `false` for `null`', async () => {
+    expect(isJsonMimeType(null)).toBe(false);
+    expect(debugLogSpy.mock.calls[0][0]).toBe(
+      "Determining if the mime type 'null' specifies JSON content."
+    );
+  });
+
+  it('should return `false` for empty-string', async () => {
+    expect(isJsonMimeType('')).toBe(false);
+    expect(debugLogSpy.mock.calls[0][0]).toBe(
+      "Determining if the mime type '' specifies JSON content."
+    );
+  });
+
+  it('should return `false` for non-JSON mimetype', async () => {
+    expect(isJsonMimeType('application/octect-stream')).toBe(false);
+    expect(isJsonMimeType('text/plain')).toBe(false);
+    expect(isJsonMimeType('multipart/form-data; charset=utf-8')).toBe(false);
+    expect(debugLogSpy.mock.calls[0][0]).toBe(
+      "Determining if the mime type 'application/octect-stream' specifies JSON content."
+    );
+    expect(debugLogSpy.mock.calls[1][0]).toBe(
+      "Determining if the mime type 'text/plain' specifies JSON content."
+    );
+    expect(debugLogSpy.mock.calls[2][0]).toBe(
+      "Determining if the mime type 'multipart/form-data; charset=utf-8' specifies JSON content."
+    );
+  });
+
+  it('should return `true` for a JSON mimetype', async () => {
+    expect(isJsonMimeType('application/json')).toBe(true);
+    expect(isJsonMimeType('application/json;charset=utf-8')).toBe(true);
+    expect(debugLogSpy.mock.calls[0][0]).toBe(
+      "Determining if the mime type 'application/json' specifies JSON content."
+    );
+    expect(debugLogSpy.mock.calls[1][0]).toBe(
+      "Determining if the mime type 'application/json;charset=utf-8' specifies JSON content."
+    );
+  });
+
+  it('should return `true` for a JSON mimetype including optional whitespace', async () => {
+    expect(isJsonMimeType('application/json  ; charset=utf-8')).toBe(true);
+    expect(debugLogSpy.mock.calls[0][0]).toBe(
+      "Determining if the mime type 'application/json  ; charset=utf-8' specifies JSON content."
+    );
+  });
+});