From 42337be1ccf8cd46942c5da2c69323fb491360d4 Mon Sep 17 00:00:00 2001 From: Shazron Abdullah <36107+shazron@users.noreply.github.com> Date: Tue, 26 Mar 2024 12:23:26 +0800 Subject: [PATCH] fix: ACNA-2890 - fix error handling in library (#144) --- lib/AdobeState.js | 84 +++++++++++++++++++++++------------------ test/AdobeState.test.js | 18 ++++----- 2 files changed, 57 insertions(+), 45 deletions(-) diff --git a/lib/AdobeState.js b/lib/AdobeState.js index 7c4b707..b8eb8dc 100644 --- a/lib/AdobeState.js +++ b/lib/AdobeState.js @@ -72,37 +72,41 @@ 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) + } + + return handleResponse(response, params) +} + +/** + * Handle a network response. + * + * @param {Response} response a fetch Response + * @param {object} params the params to the network call + * @returns {void} + */ +async function handleResponse (response, params) { + if (response.ok) { + return response + } + + const copyParams = cloneDeep(params) + switch (response.status) { + case 404: + // no exception on 404 + return response + 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} body: ${await response.text()}`], sdkDetails: copyParams })) } - return response } /** @@ -293,7 +297,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() @@ -357,7 +361,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) { @@ -373,8 +377,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 + } } /** @@ -395,7 +403,7 @@ class AdobeState { const promise = this.fetchRetry.exponentialBackoff(this.createRequestUrl(), requestOptions) const response = await _wrap(promise, {}) - return response !== null + return (response.status !== 404) } /** @@ -416,7 +424,7 @@ class AdobeState { const promise = this.fetchRetry.exponentialBackoff(this.createRequestUrl(), requestOptions) const response = await _wrap(promise, {}) - return response !== null + return (response.status !== 404) } /** @@ -437,7 +445,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 a959ffd..4fe285a 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 () => { @@ -263,9 +264,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 +319,7 @@ describe('deleteAll', () => { }) }) -describe('any', () => { +describe('stats()', () => { let store beforeEach(async () => { @@ -342,7 +342,7 @@ describe('any', () => { }) }) -describe('stats()', () => { +describe('any', () => { let store beforeEach(async () => {