From 3fc11aa9d12717092e31ac6907c442370acccbef Mon Sep 17 00:00:00 2001 From: Dustin Popp Date: Tue, 21 Feb 2023 14:51:15 -0600 Subject: [PATCH] fix: return json error response body as object 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 malformatted body. Signed-off-by: Dustin Popp --- etc/ibm-cloud-sdk-core.api.md | 3 + lib/helper.ts | 13 +- lib/request-wrapper.ts | 53 ++++++-- test/unit/is-json-mime-type.test.js | 56 +++++++++ test/unit/request-wrapper.test.js | 189 ++++++++++++++++++++++------ 5 files changed, 267 insertions(+), 47 deletions(-) create mode 100644 test/unit/is-json-mime-type.test.js diff --git a/etc/ibm-cloud-sdk-core.api.md b/etc/ibm-cloud-sdk-core.api.md index deb59eefe..282a80381 100644 --- a/etc/ibm-cloud-sdk-core.api.md +++ b/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); diff --git a/lib/helper.ts b/lib/helper.ts index ead3bff49..67e87af89 100644 --- a/lib/helper.ts +++ b/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(;.*)?$/i.test(mimeType); +} diff --git a/lib/request-wrapper.ts b/lib/request-wrapper.ts index d8325b763..e9e57a5e3 100644 --- a/lib/request-wrapper.ts +++ b/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,17 @@ 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 - let errorBody; + // attach the error response body to the error try { - // try/catch to handle objects with circular references - errorBody = JSON.stringify(axiosError.data); + // try/catch to detect objects with circular references + JSON.stringify(axiosError.data); } catch (e) { - // ignore the error, use the object, and tack on a warning - errorBody = axiosError.data; - errorBody.warning = 'Body contains circular reference'; - logger.error(`Failed to stringify axiosError: ${e}`); + logger.warn('Error field `result` contains circular reference(s)'); + logger.debug(`Failed to stringify error response body: ${e}`); } - error.body = errorBody; + error.result = ensureJSONResponseBodyIsObject(axiosError); + error.body = error.result; // ** deprecated ** // attach headers to error object error.headers = axiosError.headers; @@ -545,3 +543,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; +} diff --git a/test/unit/is-json-mime-type.test.js b/test/unit/is-json-mime-type.test.js new file mode 100644 index 000000000..6eaf2f7fa --- /dev/null +++ b/test/unit/is-json-mime-type.test.js @@ -0,0 +1,56 @@ +/** + * (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.'); + }); +}); diff --git a/test/unit/request-wrapper.test.js b/test/unit/request-wrapper.test.js index ec3dfdfc1..a0e9c0307 100644 --- a/test/unit/request-wrapper.test.js +++ b/test/unit/request-wrapper.test.js @@ -56,6 +56,10 @@ const { RequestWrapper } = require('../../dist/lib/request-wrapper'); const requestWrapperInstance = new RequestWrapper(); +const warnLogSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {}); +const errorLogSpy = jest.spyOn(logger, 'error').mockImplementation(() => {}); +const debugLogSpy = jest.spyOn(logger, 'debug').mockImplementation(() => {}); + describe('axios', () => { let env; beforeEach(() => { @@ -620,6 +624,56 @@ describe('sendRequest', () => { requestWrapperInstance.compressRequestData = false; }); + it('should convert string response body to object when content is JSON', async () => { + const parameters = { + defaultOptions: { + body: 'post=body', + formData: '', + qs: {}, + method: 'POST', + url: 'https://example.ibm.com/v1/environments', + headers: {}, + responseType: 'json', + }, + }; + + axiosResolveValue.data = '{"key": "value"}'; + axiosResolveValue.headers = { 'content-type': 'application/json' }; + mockAxiosInstance.mockResolvedValue(axiosResolveValue); + + const res = await requestWrapperInstance.sendRequest(parameters); + expect(typeof res.result).toEqual('object'); + expect(res.result).toEqual({ key: 'value' }); + }); + + it('should raise exception when response body content is invalid json', async () => { + const parameters = { + defaultOptions: { + body: 'post=body', + formData: '', + qs: {}, + method: 'POST', + url: 'https://example.ibm.com/v1/environments', + headers: {}, + responseType: 'json', + }, + }; + + axiosResolveValue.data = '{"key": "value"'; + axiosResolveValue.headers = { 'content-type': 'application/json; charset=utf8' }; + mockAxiosInstance.mockResolvedValue(axiosResolveValue); + + await expect(requestWrapperInstance.sendRequest(parameters)).rejects.toThrow( + 'Unexpected end of JSON input' + ); + expect(errorLogSpy).toHaveBeenCalledTimes(2); + expect(errorLogSpy.mock.calls[0][0]).toBe( + 'Response body was supposed to have JSON content but JSON parsing failed.' + ); + expect(errorLogSpy.mock.calls[1][0]).toBe('Malformed JSON string: {"key": "value"'); + errorLogSpy.mockClear(); + }); + // Need to rewrite this to test instantiation with userOptions // it('should keep parameters in options that are not explicitly set in requestwrapper', async => { @@ -654,6 +708,12 @@ describe('sendRequest', () => { }); describe('formatError', () => { + afterEach(() => { + warnLogSpy.mockClear(); + errorLogSpy.mockClear(); + debugLogSpy.mockClear(); + }); + const basicAxiosError = { response: { config: 'large object', @@ -682,20 +742,34 @@ describe('formatError', () => { code: 'SOME_STATUS_KEY', }; + const axiosErrorCopy = makeCopy(basicAxiosError); + it('should get the message from errors[0].message', () => { const error = requestWrapperInstance.formatError(basicAxiosError); expect(error instanceof Error).toBe(true); expect(error.name).toBe('Not Found'); expect(error.code).toBe(404); expect(error.message).toBe('First error: no model found.'); - expect(typeof error.body).toBe('string'); + expect(typeof error.body).toBe('object'); + expect(typeof error.result).toBe('object'); expect(error.headers).toEqual(basicAxiosError.response.headers); }); - it('should get the message from error', () => { - delete basicAxiosError.response.data.errors; + it('should return error with object "result" if content is JSON', () => { const error = requestWrapperInstance.formatError(basicAxiosError); expect(error instanceof Error).toBe(true); + expect(error.statusText).toBe('Not Found'); + expect(error.status).toBe(404); + expect(error.message).toBe('First error: no model found.'); + expect(typeof error.result).toBe('object'); + expect(error.body).toEqual(error.result); + expect(error.result).toEqual(basicAxiosError.response.data); + }); + + it('should get the message from error', () => { + delete axiosErrorCopy.response.data.errors; + const error = requestWrapperInstance.formatError(axiosErrorCopy); + expect(error instanceof Error).toBe(true); expect(error.name).toBe('Not Found'); expect(error.code).toBe(404); expect(error.message).toBe('Model not found.'); @@ -703,8 +777,8 @@ describe('formatError', () => { }); it('should get the message from message', () => { - delete basicAxiosError.response.data.error; - const error = requestWrapperInstance.formatError(basicAxiosError); + delete axiosErrorCopy.response.data.error; + const error = requestWrapperInstance.formatError(axiosErrorCopy); expect(error instanceof Error).toBe(true); expect(error.name).toBe('Not Found'); expect(error.code).toBe(404); @@ -713,48 +787,44 @@ describe('formatError', () => { }); it('should get the message from errorMessage', () => { - delete basicAxiosError.response.data.message; - const error = requestWrapperInstance.formatError(basicAxiosError); + delete axiosErrorCopy.response.data.message; + const error = requestWrapperInstance.formatError(axiosErrorCopy); expect(error instanceof Error).toBe(true); expect(error.name).toBe('Not Found'); expect(error.code).toBe(404); expect(error.message).toBe('There is just no finding this model.'); - expect(error.body).toBe('{"errorMessage":"There is just no finding this model.","code":404}'); expect(error.headers).toEqual(basicAxiosError.response.headers); }); it('should get error from status text when not found', () => { - delete basicAxiosError.response.data.errorMessage; - const error = requestWrapperInstance.formatError(basicAxiosError); + delete axiosErrorCopy.response.data.errorMessage; + const error = requestWrapperInstance.formatError(axiosErrorCopy); expect(error instanceof Error).toBe(true); expect(error.message).toBe('Not Found'); }); it('check the unauthenticated thing - 401', () => { - basicAxiosError.response.status = 401; - const error = requestWrapperInstance.formatError(basicAxiosError); + axiosErrorCopy.response.status = 401; + const error = requestWrapperInstance.formatError(axiosErrorCopy); expect(error instanceof Error).toBe(true); expect(error.message).toBe('Access is denied due to invalid credentials.'); }); it('check the unauthenticated thing - 403', () => { - basicAxiosError.response.status = 403; - const error = requestWrapperInstance.formatError(basicAxiosError); + axiosErrorCopy.response.status = 403; + const error = requestWrapperInstance.formatError(axiosErrorCopy); expect(error instanceof Error).toBe(true); expect(error.message).toBe('Access is denied due to invalid credentials.'); }); it('check the unauthenticated thing - iam', () => { - basicAxiosError.response.status = 400; - basicAxiosError.response.data.context = { + axiosErrorCopy.response.status = 400; + axiosErrorCopy.response.data.context = { url: 'https://iam.cloud.ibm.com/identity/token', }; - const error = requestWrapperInstance.formatError(basicAxiosError); + const error = requestWrapperInstance.formatError(axiosErrorCopy); expect(error instanceof Error).toBe(true); expect(error.message).toBe('Access is denied due to invalid credentials.'); - - // clean up - delete basicAxiosError.response.data.context; }); it('check error with circular ref in data', () => { @@ -763,20 +833,27 @@ describe('formatError', () => { b: 'c', }, }; - basicAxiosError.response.data = { + axiosErrorCopy.response.data = { error: otherObject, }; // create a circular reference - basicAxiosError.response.data.error.a.newKey = otherObject; - const error = requestWrapperInstance.formatError(basicAxiosError); + axiosErrorCopy.response.data.error.a.newKey = otherObject; + const error = requestWrapperInstance.formatError(axiosErrorCopy); expect(error instanceof Error).toBe(true); expect(typeof error.body).toBe('object'); expect(error.message).toBe('Not Found'); + + expect(warnLogSpy).toHaveBeenCalled(); + expect(warnLogSpy.mock.calls[0][0]).toBe('Error field `result` contains circular reference(s)'); + expect(debugLogSpy).toHaveBeenCalled(); + expect(debugLogSpy.mock.calls[0][0]).toMatch( + 'Failed to stringify error response body: TypeError: Converting circular structure to JSON' + ); }); it('check the request flow', () => { - delete basicAxiosError.response; - const error = requestWrapperInstance.formatError(basicAxiosError); + delete axiosErrorCopy.response; + const error = requestWrapperInstance.formatError(axiosErrorCopy); expect(error instanceof Error).toBe(true); expect(error.message).toBe('error in building the request'); expect(error.statusText).toBe('SOME_STATUS_KEY'); @@ -787,11 +864,11 @@ describe('formatError', () => { // save the original message const originalMessage = basicAxiosError.message; - basicAxiosError.message = 'request has self signed certificate'; - const error = requestWrapperInstance.formatError(basicAxiosError); + axiosErrorCopy.message = 'request has self signed certificate'; + const error = requestWrapperInstance.formatError(axiosErrorCopy); // put the original message back in, before expectations in case they fail - basicAxiosError.message = originalMessage; + axiosErrorCopy.message = originalMessage; expect(error instanceof Error).toBe(true); expect(error.message).toMatch(/SSL certificate is not valid/); @@ -801,22 +878,62 @@ describe('formatError', () => { // save the original code const originalCode = basicAxiosError.code; - basicAxiosError.code = 'DEPTH_ZERO_SELF_SIGNED_CERT'; - const error = requestWrapperInstance.formatError(basicAxiosError); + axiosErrorCopy.code = 'DEPTH_ZERO_SELF_SIGNED_CERT'; + const error = requestWrapperInstance.formatError(axiosErrorCopy); // put the original message back in, before expectations in case they fail - basicAxiosError.code = originalCode; + axiosErrorCopy.code = originalCode; expect(error instanceof Error).toBe(true); expect(error.message).toMatch(/SSL certificate is not valid/); }); it('check the message flow', () => { - delete basicAxiosError.request; - const error = requestWrapperInstance.formatError(basicAxiosError); + delete axiosErrorCopy.request; + const error = requestWrapperInstance.formatError(axiosErrorCopy); expect(error instanceof Error).toBe(true); expect(error.message).toBe('error in building the request'); }); + + it('should convert string response bodies to objects if content is JSON', () => { + const newAxiosError = makeCopy(basicAxiosError); + newAxiosError.response.data = '{ "errorMessage": "some error" }'; + expect(newAxiosError.response.headers['content-type']).toBe('application/json'); + + const error = requestWrapperInstance.formatError(newAxiosError); + expect(error instanceof Error).toBe(true); + expect(typeof error.result).toBe('object'); + expect(error.result.errorMessage).toBeDefined(); + expect(error.result.errorMessage).toBe('some error'); + }); + + it('should return error response body as string if content is not JSON', () => { + const newAxiosError = makeCopy(basicAxiosError); + newAxiosError.response.data = '{ "errorMessage": "some error" }'; + newAxiosError.response.headers['content-type'] = 'text/plain'; + + const error = requestWrapperInstance.formatError(newAxiosError); + expect(error instanceof Error).toBe(true); + expect(typeof error.result).toBe('string'); + expect(error.result).toBe('{ "errorMessage": "some error" }'); + }); + + it('should raise an exception if content is JSON and error response body is malformed', () => { + const newAxiosError = makeCopy(basicAxiosError); + newAxiosError.response.data = '{ "errorMessage": "some error"'; + expect(newAxiosError.response.headers['content-type']).toBe('application/json'); + + expect(() => { + requestWrapperInstance.formatError(newAxiosError); + }).toThrow('Unexpected end of JSON input'); + expect(errorLogSpy).toHaveBeenCalledTimes(2); + expect(errorLogSpy.mock.calls[0][0]).toBe( + 'Response body was supposed to have JSON content but JSON parsing failed.' + ); + expect(errorLogSpy.mock.calls[1][0]).toBe( + 'Malformed JSON string: { "errorMessage": "some error"' + ); + }); }); describe('getHttpClient', () => { @@ -828,8 +945,6 @@ describe('getHttpClient', () => { describe('gzipRequestBody', () => { const gzipSpy = jest.spyOn(zlib, 'gzipSync'); - const errorLogSpy = jest.spyOn(logger, 'error').mockImplementation(() => {}); - const debugLogSpy = jest.spyOn(logger, 'debug').mockImplementation(() => {}); afterEach(() => { gzipSpy.mockClear(); @@ -1022,3 +1137,7 @@ describe('gzipRequestBody', () => { expect(requestWrapperInstance.raxConfig).toBeUndefined(); }); }); + +function makeCopy(obj) { + return JSON.parse(JSON.stringify(obj)); +}