From d2c55af9cd4e387fdbfc94df8f20cda674fb62a4 Mon Sep 17 00:00:00 2001 From: Shazron Abdullah Date: Tue, 19 Mar 2024 23:06:02 +0800 Subject: [PATCH 1/2] fix: ACNA-2890 - fix error handling in library --- lib/AdobeState.js | 83 ++++++++++++++++++++++++----------------- test/AdobeState.test.js | 7 ++-- 2 files changed, 51 insertions(+), 39 deletions(-) diff --git a/lib/AdobeState.js b/lib/AdobeState.js index 458cdfe..a3727a2 100644 --- a/lib/AdobeState.js +++ b/lib/AdobeState.js @@ -72,39 +72,44 @@ async function _wrap (promise, params) { try { response = await promise logger.debug('response', response) - // reuse code in exception handler, for any other network exceptions - if (!response.ok) { - // no exception on 404 - if (response.status === 404) { - return null - } else { - const e = new Error(response.statusText) - e.status = response.status - e.internal = response - throw e - } - } } catch (e) { - const status = e.status || e.code - const copyParams = cloneDeep(params) - logger.debug(`got internal error with status ${status}: ${e.message} `) - switch (status) { - case 401: - return logAndThrow(new codes.ERROR_UNAUTHORIZED({ messageValues: ['underlying DB provider'], sdkDetails: copyParams })) - case 403: - return logAndThrow(new codes.ERROR_BAD_CREDENTIALS({ messageValues: ['underlying DB provider'], sdkDetails: copyParams })) - case 413: - return logAndThrow(new codes.ERROR_PAYLOAD_TOO_LARGE({ messageValues: ['underlying DB provider'], sdkDetails: copyParams })) - case 429: - return logAndThrow(new codes.ERROR_REQUEST_RATE_TOO_HIGH({ sdkDetails: copyParams })) - default: - // NOTE: we should throw a different error if its not a response error - return logAndThrow(new codes.ERROR_INTERNAL({ messageValues: [`unexpected response from provider with status: ${status}`], sdkDetails: { ...cloneDeep(params), _internal: e.internal || e.message } })) - } + logAndThrow(e) } + + handleResponse(response, params) return response } +/** + * Handle a network response. + * + * @param {Response} response a fetch Response + * @param {object} params the params to the network call + * @returns {void} + */ +function handleResponse (response, params) { + if (response.ok) { + return + } + + const copyParams = cloneDeep(params) + switch (response.status) { + case 404: + // do nothing, no exception on 404 + break + case 401: + return logAndThrow(new codes.ERROR_UNAUTHORIZED({ messageValues: ['underlying DB provider'], sdkDetails: copyParams })) + case 403: + return logAndThrow(new codes.ERROR_BAD_CREDENTIALS({ messageValues: ['underlying DB provider'], sdkDetails: copyParams })) + case 413: + return logAndThrow(new codes.ERROR_PAYLOAD_TOO_LARGE({ messageValues: ['underlying DB provider'], sdkDetails: copyParams })) + case 429: + return logAndThrow(new codes.ERROR_REQUEST_RATE_TOO_HIGH({ sdkDetails: copyParams })) + default: // 500 errors + return logAndThrow(new codes.ERROR_INTERNAL({ messageValues: [`unexpected response from provider with status: ${response.status}`], sdkDetails: copyParams })) + } +} + /** * @abstract * @class AdobeState @@ -263,7 +268,7 @@ class AdobeState { logger.debug('get', requestOptions) const promise = this.fetchRetry.exponentialBackoff(this.createRequestUrl(key), requestOptions) const response = await _wrap(promise, { key }) - if (response) { + if (response.ok) { // we only expect string values const value = await response.text() const expiration = new Date(Number(response.headers.get(HEADER_KEY_EXPIRES))).toISOString() @@ -327,7 +332,7 @@ class AdobeState { * Deletes a state key-value pair * * @param {string} key state key identifier - * @returns {Promise} key of deleted state or `null` if state does not exists + * @returns {Promise} key of deleted state or `null` if state does not exist * @memberof AdobeState */ async delete (key) { @@ -343,8 +348,12 @@ class AdobeState { logger.debug('delete', requestOptions) const promise = this.fetchRetry.exponentialBackoff(this.createRequestUrl(key), requestOptions) - const ret = await _wrap(promise, { key }) - return ret && key + const response = await _wrap(promise, { key }) + if (response.status === 404) { + return null + } else { + return key + } } /** @@ -365,7 +374,7 @@ class AdobeState { const promise = this.fetchRetry.exponentialBackoff(this.createRequestUrl(), requestOptions) const response = await _wrap(promise, {}) - return response !== null + return (response.status !== 404) } /** @@ -386,7 +395,7 @@ class AdobeState { const promise = this.fetchRetry.exponentialBackoff(this.createRequestUrl(), requestOptions) const response = await _wrap(promise, {}) - return response !== null + return (response.status !== 404) } /** @@ -407,7 +416,11 @@ class AdobeState { const promise = this.fetchRetry.exponentialBackoff(this.createRequestUrl(), requestOptions) const response = await _wrap(promise, {}) - return !!response && response.json() + if (response.status === 404) { + return false + } else { + return response.json() + } } } diff --git a/test/AdobeState.test.js b/test/AdobeState.test.js index 79efdc7..f15df73 100644 --- a/test/AdobeState.test.js +++ b/test/AdobeState.test.js @@ -263,9 +263,8 @@ describe('put', () => { const value = 'some-value' const error = new Error('some network error') - error.code = 502 mockExponentialBackoff.mockRejectedValue(error) - await expect(store.put(key, value)).rejects.toThrow('[AdobeStateLib:ERROR_INTERNAL] unexpected response from provider with status: 502') + await expect(store.put(key, value)).rejects.toThrow(error.message) }) }) @@ -319,7 +318,7 @@ describe('deleteAll', () => { }) }) -describe('any', () => { +describe('stats()', () => { let store beforeEach(async () => { @@ -342,7 +341,7 @@ describe('any', () => { }) }) -describe('stats()', () => { +describe('any', () => { let store beforeEach(async () => { From 72e7b45d5c5862e292db4864b1eff6c517a72f8e Mon Sep 17 00:00:00 2001 From: Shazron Abdullah Date: Thu, 21 Mar 2024 22:36:57 +0800 Subject: [PATCH 2/2] add AdobeStateLib:ERROR_INTERNAL returning response body contents --- lib/AdobeState.js | 13 ++++++------- test/AdobeState.test.js | 11 ++++++----- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/lib/AdobeState.js b/lib/AdobeState.js index a3727a2..3a3a1ef 100644 --- a/lib/AdobeState.js +++ b/lib/AdobeState.js @@ -76,8 +76,7 @@ async function _wrap (promise, params) { logAndThrow(e) } - handleResponse(response, params) - return response + return handleResponse(response, params) } /** @@ -87,16 +86,16 @@ async function _wrap (promise, params) { * @param {object} params the params to the network call * @returns {void} */ -function handleResponse (response, params) { +async function handleResponse (response, params) { if (response.ok) { - return + return response } const copyParams = cloneDeep(params) switch (response.status) { case 404: - // do nothing, no exception on 404 - break + // no exception on 404 + return response case 401: return logAndThrow(new codes.ERROR_UNAUTHORIZED({ messageValues: ['underlying DB provider'], sdkDetails: copyParams })) case 403: @@ -106,7 +105,7 @@ function handleResponse (response, params) { case 429: return logAndThrow(new codes.ERROR_REQUEST_RATE_TOO_HIGH({ sdkDetails: copyParams })) default: // 500 errors - return logAndThrow(new codes.ERROR_INTERNAL({ messageValues: [`unexpected response from provider with status: ${response.status}`], sdkDetails: copyParams })) + return logAndThrow(new codes.ERROR_INTERNAL({ messageValues: [`unexpected response from provider with status: ${response.status} body: ${await response.text()}`], sdkDetails: copyParams })) } } diff --git a/test/AdobeState.test.js b/test/AdobeState.test.js index f15df73..b434628 100644 --- a/test/AdobeState.test.js +++ b/test/AdobeState.test.js @@ -55,14 +55,14 @@ const wrapInFetchResponse = (body, options = {}) => { } } -const wrapInFetchError = (status) => { +const wrapInFetchError = (status, body) => { return { ok: false, headers: { get: () => 'fake req id' }, - json: async () => 'error', - text: async () => 'error', + json: async () => JSON.parse(body), + text: async () => body, status } } @@ -253,9 +253,10 @@ describe('put', () => { test('coverage: unknown server error', async () => { const key = 'some-key' const value = 'some-value' + const responseBody = 'error: this is the response body' - mockExponentialBackoff.mockResolvedValue(wrapInFetchError(500)) - await expect(store.put(key, value)).rejects.toThrow('[AdobeStateLib:ERROR_INTERNAL] unexpected response from provider with status: 500') + mockExponentialBackoff.mockResolvedValue(wrapInFetchError(500, responseBody)) + await expect(store.put(key, value)).rejects.toThrow(`[AdobeStateLib:ERROR_INTERNAL] unexpected response from provider with status: 500 body: ${responseBody}`) }) test('coverage: unknown error (fetch network failure)', async () => {