From dc85dca0a898640ad6b09bca4ef981caca1c1555 Mon Sep 17 00:00:00 2001 From: Vitaly Tsaplin Date: Wed, 13 Mar 2024 23:22:50 +0100 Subject: [PATCH 1/8] fix: do not reveal unexpected errors --- actions/AuthAction.js | 88 ++++++++++++------------ actions/FirefallClient.js | 19 ++--- actions/FirefallClient.test.js | 16 ++--- actions/GenericAction.js | 40 +++++++---- actions/ImsClient.js | 5 +- actions/Network.js | 31 +++++++-- actions/target/index.js | 8 +-- app.config.yaml | 4 +- e2e/WebApp.test.js | 2 +- web-src/src/components/GenerateButton.js | 1 + web-src/src/helpers/NetworkHelper.js | 28 ++++++-- web-src/src/services/CsvParserService.js | 4 +- web-src/src/services/FirefallService.js | 9 ++- web-src/src/services/TargetService.js | 4 +- 14 files changed, 151 insertions(+), 108 deletions(-) diff --git a/actions/AuthAction.js b/actions/AuthAction.js index 57c57c3b..ded51b33 100644 --- a/actions/AuthAction.js +++ b/actions/AuthAction.js @@ -12,58 +12,58 @@ const { Core } = require('@adobe/aio-sdk'); const QueryStringAddon = require('wretch/addons/queryString'); const { ImsClient } = require('./ImsClient.js'); -const { wretchRetry } = require('./Network.js'); +const { wretch } = require('./Network.js'); const logger = Core.Logger('AuthAction'); async function isValidToken(endpoint, clientId, token) { - return wretchRetry(`${endpoint}/ims/validate_token/v1`) - .addon(QueryStringAddon).query({ - client_id: clientId, - type: 'access_token', - }) - .headers({ - Authorization: `Bearer ${token}`, - 'Content-Type': 'application/json', - }) - .get() - .json() - .then((json) => { - return json.valid; - }) - .catch((error) => { - logger.error(error); - return false; - }); + try { + const response = await wretch(`${endpoint}/ims/validate_token/v1`) + .addon(QueryStringAddon).query({ + client_id: clientId, + type: 'access_token', + }) + .headers({ + Authorization: `Bearer ${token}`, + 'Content-Type': 'application/json', + }) + .get() + .json(); + const { valid } = response; + return valid; + } catch (error) { + logger.error(error); + return false; + } } async function checkForProductContext(endpoint, clientId, org, token, productContext) { - return wretchRetry(`${endpoint}/ims/profile/v1`) - .addon(QueryStringAddon).query({ - client_id: clientId, - }) - .headers({ - Authorization: `Bearer ${token}`, - 'Content-Type': 'application/json', - }) - .get() - .json() - .then(async (json) => { - if (Array.isArray(json.projectedProductContext)) { - const filteredProductContext = json.projectedProductContext - .filter((obj) => obj.prodCtx.serviceCode === productContext); + try { + const response = await wretch(`${endpoint}/ims/profile/v1`) + .addon(QueryStringAddon).query({ + client_id: clientId, + }) + .headers({ + Authorization: `Bearer ${token}`, + 'Content-Type': 'application/json', + }) + .get() + .json(); - // For each entry in filteredProductContext check that - // there is at least one entry where imsOrg matches the owningEntity property - // otherwise, if no match, the user is not authorized - return filteredProductContext.some((obj) => obj.prodCtx.owningEntity === org); - } - return false; - }) - .catch((error) => { - logger.error(error); - return false; - }); + if (Array.isArray(response.projectedProductContext)) { + const filteredProductContext = response.projectedProductContext + .filter((obj) => obj.prodCtx.serviceCode === productContext); + + // For each entry in filteredProductContext check that + // there is at least one entry where imsOrg matches the owningEntity property + // otherwise, if no match, the user is not authorized + return filteredProductContext.some((obj) => obj.prodCtx.owningEntity === org); + } + return false; + } catch (error) { + logger.error(error); + return false; + } } function asAuthAction(action) { diff --git a/actions/FirefallClient.js b/actions/FirefallClient.js index ae22b39d..e9588252 100644 --- a/actions/FirefallClient.js +++ b/actions/FirefallClient.js @@ -10,20 +10,21 @@ * governing permissions and limitations under the License. */ const { Core } = require('@adobe/aio-sdk'); -const { wretchRetry } = require('./Network.js'); +const { wretch, NetworkError } = require('./Network.js'); const logger = Core.Logger('FirefallAction'); -const FIREFALL_ERROR_CODES = { +const ERROR_CODES = { defaultCompletion: 'An error occurred while generating results', defaultFeedback: 'An error occurred while sending feedback', 400: "The response was filtered due to the prompt triggering Generative AI's content management policy. Please modify your prompt and retry.", + 408: "Generative AI's request timed out. Please try again.", 429: "Generative AI's Rate limit exceeded. Please wait one minute and try again.", }; -function firefallErrorMessage(defaultMessage, errorStatus) { - const errorString = FIREFALL_ERROR_CODES[errorStatus] ?? defaultMessage; - return `IS-ERROR: ${errorString} (${errorStatus}).`; +function handleError(error, defaultMessage) { + const errorMessage = ERROR_CODES[error.status] ?? defaultMessage; + return new NetworkError(400, `IS-ERROR: ${errorMessage} (${error.status}).`); } class FirefallClient { @@ -38,7 +39,7 @@ class FirefallClient { const startTime = Date.now(); try { - const response = await wretchRetry(`${this.endpoint}/v1/completions`) + const response = await wretch(`${this.endpoint}/v1/completions`) .headers({ 'x-gw-ims-org-id': this.org, 'x-api-key': this.apiKey, @@ -69,7 +70,7 @@ class FirefallClient { return response; } catch (error) { logger.error('Failed generating results:', error); - throw new Error(firefallErrorMessage(FIREFALL_ERROR_CODES.defaultCompletion, error.status)); + throw handleError(error, ERROR_CODES.defaultCompletion); } } @@ -77,7 +78,7 @@ class FirefallClient { const startTime = Date.now(); try { - const response = await wretchRetry(`${this.endpoint}/v1/feedback`) + const response = await wretch(`${this.endpoint}/v1/feedback`) .headers({ Authorization: `Bearer ${this.accessToken}`, 'x-api-key': this.apiKey, @@ -99,7 +100,7 @@ class FirefallClient { return response; } catch (error) { logger.error('Failed sending feedback:', error); - throw new Error(firefallErrorMessage(FIREFALL_ERROR_CODES.defaultFeedback, error.status)); + throw handleError(error, ERROR_CODES.defaultFeedback); } } } diff --git a/actions/FirefallClient.test.js b/actions/FirefallClient.test.js index 84f33975..7a293d16 100644 --- a/actions/FirefallClient.test.js +++ b/actions/FirefallClient.test.js @@ -9,9 +9,8 @@ * OF ANY KIND, either express or implied. See the License for the specific language * governing permissions and limitations under the License. */ -import { WretchError } from 'wretch'; import { FirefallClient } from './FirefallClient.js'; -import { wretchRetry } from './Network.js'; +import { NetworkError, wretch } from './Network.js'; // Mock the 'Network.js' module jest.mock('./Network.js'); @@ -30,21 +29,18 @@ let firefall; let error; function createFirefallClient() { - const client = new FirefallClient('endpoint', 'apiKey', 'org', 'accessToken'); - return client; + return new FirefallClient('endpoint', 'apiKey', 'org', 'accessToken'); } -function createWretchError(status) { - const wretchError = new WretchError('Internal Server Error'); - wretchError.status = status; - return wretchError; +function createNetworkError(status) { + return new NetworkError(status, 'Internal Server Error'); } beforeEach(() => { jest.clearAllMocks(); firefall = createFirefallClient(); - error = createWretchError(500); - wretchRetry.mockImplementation(() => ({ + error = createNetworkError(500); + wretch.mockImplementation(() => ({ headers: jest.fn().mockReturnThis(), post: jest.fn().mockReturnThis(), json: jest.fn().mockRejectedValue(error), diff --git a/actions/GenericAction.js b/actions/GenericAction.js index 87ee474f..72c9819c 100644 --- a/actions/GenericAction.js +++ b/actions/GenericAction.js @@ -9,22 +9,36 @@ * OF ANY KIND, either express or implied. See the License for the specific language * governing permissions and limitations under the License. */ + +const { NetworkError } = require('./Network.js'); + +function createResponse(status, body) { + return { + headers: { 'Content-Type': 'application/json' }, + statusCode: status, + body, + }; +} + +function createSuccessResponse(body) { + return createResponse(200, body); +} + +function createErrorResponse(status, message) { + return createResponse(status, { error: message }); +} + function asGenericAction(action) { - return async function (params) { + return async (params) => { try { - return { - headers: { 'Content-Type': 'application/json' }, - statusCode: 200, - body: await action(params), - }; + return createSuccessResponse(await action(params)); } catch (e) { - return { - headers: { 'Content-Type': 'application/json' }, - statusCode: e.status ?? 500, - body: { - error: e.message ?? 'Internal Server Error', - }, - }; + if (e instanceof NetworkError) { + console.error(`Network error: ${e.message} (${e.status})`); + return createErrorResponse(e.status, e.message); + } + console.error(`Internal error: ${e.message}`); + return createErrorResponse(500, e.message ?? 'Internal Server Error'); } }; } diff --git a/actions/ImsClient.js b/actions/ImsClient.js index 83d94d1c..78c58905 100644 --- a/actions/ImsClient.js +++ b/actions/ImsClient.js @@ -10,8 +10,7 @@ * governing permissions and limitations under the License. */ const FormUrlAddon = require('wretch/addons/formUrl'); -const { wretchRetry } = require('./Network.js'); -// const FormDataAddon = require('wretch/addons/formData'); +const { wretch } = require('./Network.js'); class ImsClient { constructor(endpoint, clientId, clientSecret, permAuthCode) { @@ -22,7 +21,7 @@ class ImsClient { } async getServiceToken() { - const json = await wretchRetry(`${this.endpoint}/ims/token/v1`) + const json = await wretch(`${this.endpoint}/ims/token/v1`) .addon(FormUrlAddon).formUrl({ client_id: this.clientId, client_secret: this.clientSecret, diff --git a/actions/Network.js b/actions/Network.js index 7df0e537..16a16407 100644 --- a/actions/Network.js +++ b/actions/Network.js @@ -9,15 +9,32 @@ * OF ANY KIND, either express or implied. See the License for the specific language * governing permissions and limitations under the License. */ +const AbortAddon = require('wretch/addons/abort'); + const wretch = require('wretch'); -const { retry } = require('wretch/middlewares/retry'); -function wretchRetry(url) { +const REQUEST_TIMEOUT = 5 * 1000; + +class NetworkError extends Error { + constructor(status, message) { + super(message); + this.name = 'NetworkError'; + this.status = status; + } +} + +function wretchWithOptions(url) { return wretch(url) - .middlewares([retry({ - retryOnNetworkError: true, - until: (response) => response && (response.ok || (response.status >= 400 && response.status < 500)), - })]); + .addon(AbortAddon()) + .resolve((_) => _.setTimeout(REQUEST_TIMEOUT)) + .resolve((_) => { + return _.fetchError((error) => { + if (error.name === 'AbortError') { + throw new NetworkError(408, 'Request timed out'); + } + throw new NetworkError(500, 'Network error'); + }); + }); } -module.exports = { wretchRetry }; +module.exports = { wretch: wretchWithOptions, NetworkError }; diff --git a/actions/target/index.js b/actions/target/index.js index 24552504..0ad02666 100644 --- a/actions/target/index.js +++ b/actions/target/index.js @@ -9,7 +9,7 @@ * OF ANY KIND, either express or implied. See the License for the specific language * governing permissions and limitations under the License. */ -const wretch = require('wretch'); +const { wretch } = require('../Network.js'); const { asGenericAction } = require('../GenericAction.js'); const MIN_DESCRIPTION_LENGTH = 5; @@ -18,7 +18,7 @@ async function main({ __ow_headers: headers, org, TARGET_API_KEY }) { const { authorization } = headers; const accessToken = authorization.split(' ')[1]; - const json = await wretch(`https://mc.adobe.io/${org}/target/audiences`) + const response = await wretch(`https://mc.adobe.io/${org}/target/audiences`) .headers({ 'x-api-key': TARGET_API_KEY, }) @@ -27,11 +27,11 @@ async function main({ __ow_headers: headers, org, TARGET_API_KEY }) { .get() .json(); - if (!json.audiences) { + if (!response.audiences) { throw new Error('Failed to fetch audiences'); } - return json.audiences + return response.audiences .filter((audience) => audience.name && audience.type === 'reusable') .map((audience) => ({ id: audience.id, diff --git a/app.config.yaml b/app.config.yaml index f7162f6e..1de0b2e1 100644 --- a/app.config.yaml +++ b/app.config.yaml @@ -19,7 +19,7 @@ application: IMS_SERVICE_PERM_AUTH_CODE: $IMS_SERVICE_PERM_AUTH_CODE IMS_PRODUCT_CONTEXT: $IMS_PRODUCT_CONTEXT limits: - timeout: 180000 + timeout: 60000 feedback: function: actions/feedback/index.js web: true @@ -34,7 +34,7 @@ application: IMS_SERVICE_PERM_AUTH_CODE: $IMS_SERVICE_PERM_AUTH_CODE IMS_PRODUCT_CONTEXT: $IMS_PRODUCT_CONTEXT limits: - timeout: 180000 + timeout: 60000 target: function: actions/target/index.js web: true diff --git a/e2e/WebApp.test.js b/e2e/WebApp.test.js index 028e34f8..ce05bff6 100644 --- a/e2e/WebApp.test.js +++ b/e2e/WebApp.test.js @@ -45,7 +45,7 @@ jest.mock('@adobe/exc-app/settings', () => ({ })); jest.mock('../web-src/src/helpers/NetworkHelper.js', () => ({ - wretchRetry: jest.fn().mockImplementation(() => ({ + wretch: jest.fn().mockImplementation(() => ({ get: jest.fn().mockImplementation(() => ({ json: jest.fn().mockRejectedValue(new Error('error')), })), diff --git a/web-src/src/components/GenerateButton.js b/web-src/src/components/GenerateButton.js index 3c6931b0..495fb278 100644 --- a/web-src/src/components/GenerateButton.js +++ b/web-src/src/components/GenerateButton.js @@ -39,6 +39,7 @@ export function GenerateButton() { const setIsOpenPromptEditor = useSetRecoilState(promptEditorState); const [generationInProgress, setGenerationInProgress] = useState(false); const saveResults = useSaveResults(); + const generateResults = useCallback(async () => { try { const finalPrompt = renderPrompt(prompt, parameters); diff --git a/web-src/src/helpers/NetworkHelper.js b/web-src/src/helpers/NetworkHelper.js index 8c6d03dc..f498cb93 100644 --- a/web-src/src/helpers/NetworkHelper.js +++ b/web-src/src/helpers/NetworkHelper.js @@ -10,12 +10,28 @@ * governing permissions and limitations under the License. */ import wretch from 'wretch'; -import { retry } from 'wretch/middlewares/retry'; -export function wretchRetry(url) { +function unwrapError(error) { + if (error.json?.error) { + throw new Error(error.json.error); + } + console.error(`Unexpected network error: ${error}`); + throw new Error('Unknown error. Try again later.'); +} + +function wretchWithOptions(url) { return wretch(url) - .middlewares([retry({ - retryOnNetworkError: true, - until: (response) => response && (response.ok || (response.status >= 400 && response.status < 500)), - })]); + .headers({ 'X-OW-EXTRA-LOGGING': 'on' }) + .resolve((_) => { + return _ + .badRequest((err) => unwrapError(err)) + .unauthorized((err) => unwrapError(err)) + .forbidden((err) => unwrapError(err)) + .notFound((err) => unwrapError(err)) + .timeout((err) => unwrapError(err)) + .internalError((err) => unwrapError(err)) + .fetchError((err) => unwrapError(err)); + }); } + +export { wretchWithOptions as wretch }; diff --git a/web-src/src/services/CsvParserService.js b/web-src/src/services/CsvParserService.js index 929f53a9..ea0cbc8c 100644 --- a/web-src/src/services/CsvParserService.js +++ b/web-src/src/services/CsvParserService.js @@ -10,7 +10,7 @@ * governing permissions and limitations under the License. */ import QueryStringAddon from 'wretch/addons/queryString'; -import { wretchRetry } from '../../../actions/Network.js'; +import { wretch } from '../helpers/NetworkHelper.js'; export class CsvParserService { constructor({ @@ -22,7 +22,7 @@ export class CsvParserService { } async getData(url) { - const json = await wretchRetry(this.csvParserEndpoint) + const json = await wretch(this.csvParserEndpoint) .addon(QueryStringAddon) .query({ url }) .get() diff --git a/web-src/src/services/FirefallService.js b/web-src/src/services/FirefallService.js index bed95a9a..d83166d2 100644 --- a/web-src/src/services/FirefallService.js +++ b/web-src/src/services/FirefallService.js @@ -9,7 +9,7 @@ * OF ANY KIND, either express or implied. See the License for the specific language * governing permissions and limitations under the License. */ -import { wretchRetry } from '../helpers/NetworkHelper.js'; +import { wretch } from '../helpers/NetworkHelper.js'; export class FirefallService { constructor({ @@ -29,7 +29,7 @@ export class FirefallService { async complete(prompt, temperature) { /* eslint-disable-next-line camelcase */ - const { query_id, generations } = await wretchRetry(this.completeEndpoint) + const { query_id, generations } = await wretch(this.completeEndpoint) .post({ prompt, temperature, @@ -46,14 +46,13 @@ export class FirefallService { async feedback(queryId, sentiment) { /* eslint-disable-next-line camelcase */ - const { feedback_id } = await wretchRetry(this.feedbackEndpoint) + const { feedback_id } = await wretch(this.feedbackEndpoint) .post({ queryId, sentiment, imsOrg: this.imsOrg, accessToken: this.accessToken, - }) - .json(); + }); /* eslint-disable-next-line camelcase */ return feedback_id; } diff --git a/web-src/src/services/TargetService.js b/web-src/src/services/TargetService.js index 68eb4b68..d9903f27 100644 --- a/web-src/src/services/TargetService.js +++ b/web-src/src/services/TargetService.js @@ -9,7 +9,7 @@ * OF ANY KIND, either express or implied. See the License for the specific language * governing permissions and limitations under the License. */ -import { wretchRetry } from '../../../actions/Network.js'; +import { wretch } from '../helpers/NetworkHelper.js'; export class TargetService { constructor({ @@ -26,7 +26,7 @@ export class TargetService { async getAudiences() { const url = `${this.targetEndpoint}?org=${this.imsTenant}`; - return wretchRetry(url) + return wretch(url) .auth(`Bearer ${this.accessToken}`) .accept('application/json') .get() From 4303ea9e0d3043f2027f6d8007a08b606b464d64 Mon Sep 17 00:00:00 2001 From: Vitaly Tsaplin Date: Wed, 13 Mar 2024 23:23:57 +0100 Subject: [PATCH 2/8] feat: set timeout back to normal value --- actions/Network.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actions/Network.js b/actions/Network.js index 16a16407..f87af77d 100644 --- a/actions/Network.js +++ b/actions/Network.js @@ -13,7 +13,7 @@ const AbortAddon = require('wretch/addons/abort'); const wretch = require('wretch'); -const REQUEST_TIMEOUT = 5 * 1000; +const REQUEST_TIMEOUT = 55 * 1000; class NetworkError extends Error { constructor(status, message) { From 52eef6d52689dbce29ad4b3fcf1a7756ee3de455 Mon Sep 17 00:00:00 2001 From: Vitaly Tsaplin Date: Thu, 14 Mar 2024 09:33:46 +0100 Subject: [PATCH 3/8] refactor: reaming --- actions/FirefallClient.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/actions/FirefallClient.js b/actions/FirefallClient.js index e9588252..2892166e 100644 --- a/actions/FirefallClient.js +++ b/actions/FirefallClient.js @@ -22,7 +22,7 @@ const ERROR_CODES = { 429: "Generative AI's Rate limit exceeded. Please wait one minute and try again.", }; -function handleError(error, defaultMessage) { +function toFirefallError(error, defaultMessage) { const errorMessage = ERROR_CODES[error.status] ?? defaultMessage; return new NetworkError(400, `IS-ERROR: ${errorMessage} (${error.status}).`); } @@ -70,7 +70,7 @@ class FirefallClient { return response; } catch (error) { logger.error('Failed generating results:', error); - throw handleError(error, ERROR_CODES.defaultCompletion); + throw toFirefallError(error, ERROR_CODES.defaultCompletion); } } @@ -100,7 +100,7 @@ class FirefallClient { return response; } catch (error) { logger.error('Failed sending feedback:', error); - throw handleError(error, ERROR_CODES.defaultFeedback); + throw toFirefallError(error, ERROR_CODES.defaultFeedback); } } } From 8f3173adb8d2d63b29dc41f2f2b7c5fa0d92107d Mon Sep 17 00:00:00 2001 From: Vitaly Tsaplin Date: Thu, 14 Mar 2024 12:57:28 +0100 Subject: [PATCH 4/8] refactor: better wording of error message --- actions/Network.js | 5 +++++ web-src/src/helpers/NetworkHelper.js | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/actions/Network.js b/actions/Network.js index f87af77d..054910a2 100644 --- a/actions/Network.js +++ b/actions/Network.js @@ -12,9 +12,12 @@ const AbortAddon = require('wretch/addons/abort'); const wretch = require('wretch'); +const { Core } = require('@adobe/aio-sdk'); const REQUEST_TIMEOUT = 55 * 1000; +const logger = Core.Logger('FirefallAction'); + class NetworkError extends Error { constructor(status, message) { super(message); @@ -30,8 +33,10 @@ function wretchWithOptions(url) { .resolve((_) => { return _.fetchError((error) => { if (error.name === 'AbortError') { + logger.error('Request aborted', error); throw new NetworkError(408, 'Request timed out'); } + logger.error('Network error', error); throw new NetworkError(500, 'Network error'); }); }); diff --git a/web-src/src/helpers/NetworkHelper.js b/web-src/src/helpers/NetworkHelper.js index f498cb93..bcc63238 100644 --- a/web-src/src/helpers/NetworkHelper.js +++ b/web-src/src/helpers/NetworkHelper.js @@ -15,8 +15,8 @@ function unwrapError(error) { if (error.json?.error) { throw new Error(error.json.error); } - console.error(`Unexpected network error: ${error}`); - throw new Error('Unknown error. Try again later.'); + console.error(`Unexpected error: ${error}`); + throw new Error('Oops! We\'ve encountered an unexpected error. Please try again later.'); } function wretchWithOptions(url) { From 66929be53555649a38bb5016750caa70aad64992 Mon Sep 17 00:00:00 2001 From: Vitaly Tsaplin Date: Thu, 14 Mar 2024 14:07:48 +0100 Subject: [PATCH 5/8] refactor: apply review feedback --- actions/AuthAction.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/actions/AuthAction.js b/actions/AuthAction.js index ded51b33..5c02e374 100644 --- a/actions/AuthAction.js +++ b/actions/AuthAction.js @@ -29,8 +29,7 @@ async function isValidToken(endpoint, clientId, token) { }) .get() .json(); - const { valid } = response; - return valid; + return response.valid; } catch (error) { logger.error(error); return false; From d3768a9de4490a0ff2d3daf743d38e594f417cf0 Mon Sep 17 00:00:00 2001 From: Vitaly Tsaplin Date: Fri, 15 Mar 2024 14:07:56 +0100 Subject: [PATCH 6/8] test: fix broken test --- actions/FirefallClient.js | 5 ++-- actions/FirefallClient.test.js | 47 +++++++++++++--------------------- actions/GenericAction.js | 8 +++--- actions/ImsClient.js | 2 +- actions/InternalError.js | 20 +++++++++++++++ actions/Network.js | 23 ++++++++--------- 6 files changed, 57 insertions(+), 48 deletions(-) create mode 100644 actions/InternalError.js diff --git a/actions/FirefallClient.js b/actions/FirefallClient.js index 2892166e..460c7eed 100644 --- a/actions/FirefallClient.js +++ b/actions/FirefallClient.js @@ -10,7 +10,8 @@ * governing permissions and limitations under the License. */ const { Core } = require('@adobe/aio-sdk'); -const { wretch, NetworkError } = require('./Network.js'); +const wretch = require('./Network.js'); +const InternalError = require('./InternalError.js'); const logger = Core.Logger('FirefallAction'); @@ -24,7 +25,7 @@ const ERROR_CODES = { function toFirefallError(error, defaultMessage) { const errorMessage = ERROR_CODES[error.status] ?? defaultMessage; - return new NetworkError(400, `IS-ERROR: ${errorMessage} (${error.status}).`); + return new InternalError(400, `IS-ERROR: ${errorMessage} (${error.status}).`); } class FirefallClient { diff --git a/actions/FirefallClient.test.js b/actions/FirefallClient.test.js index 7a293d16..16ac78d9 100644 --- a/actions/FirefallClient.test.js +++ b/actions/FirefallClient.test.js @@ -10,12 +10,8 @@ * governing permissions and limitations under the License. */ import { FirefallClient } from './FirefallClient.js'; -import { NetworkError, wretch } from './Network.js'; +import wretch from './Network.js'; -// Mock the 'Network.js' module -jest.mock('./Network.js'); - -// Mock the '@adobe/aio-sdk' module jest.mock('@adobe/aio-sdk', () => ({ Core: { Logger: jest.fn().mockReturnValue({ @@ -25,41 +21,34 @@ jest.mock('@adobe/aio-sdk', () => ({ }, })); -let firefall; -let error; - -function createFirefallClient() { - return new FirefallClient('endpoint', 'apiKey', 'org', 'accessToken'); -} - -function createNetworkError(status) { - return new NetworkError(status, 'Internal Server Error'); -} - -beforeEach(() => { - jest.clearAllMocks(); - firefall = createFirefallClient(); - error = createNetworkError(500); - wretch.mockImplementation(() => ({ +jest.mock('./Network.js', () => { + const wretchMock = { headers: jest.fn().mockReturnThis(), post: jest.fn().mockReturnThis(), - json: jest.fn().mockRejectedValue(error), - })); + json: jest.fn().mockResolvedValue({}), + }; + return jest.fn().mockImplementation(() => wretchMock); }); describe('FirefallClient', () => { + const sut = new FirefallClient('endpoint', 'apiKey', 'org', 'accessToken'); + + beforeEach(() => { + jest.clearAllMocks(); + }); + test('handles 400 http status in completion method', async () => { - error.status = 400; - await expect(firefall.completion('prompt')).rejects.toThrow("IS-ERROR: The response was filtered due to the prompt triggering Generative AI's content management policy. Please modify your prompt and retry. (400)."); + wretch().json.mockRejectedValue({ status: 400 }); + await expect(sut.completion('prompt')).rejects.toThrow("IS-ERROR: The response was filtered due to the prompt triggering Generative AI's content management policy. Please modify your prompt and retry. (400)."); }); test('handles 429 http status in completion method', async () => { - error.status = 429; - await expect(firefall.completion('prompt')).rejects.toThrow("IS-ERROR: Generative AI's Rate limit exceeded. Please wait one minute and try again. (429)."); + wretch().json.mockRejectedValue({ status: 429 }); + await expect(sut.completion('prompt')).rejects.toThrow("IS-ERROR: Generative AI's Rate limit exceeded. Please wait one minute and try again. (429)."); }); test('handless any http status in the feedback method', async () => { - error.status = 500; - await expect(firefall.feedback('queryId', 'sentiment')).rejects.toThrow('An error occurred while sending feedback'); + wretch().json.mockRejectedValue({ status: 500 }); + await expect(sut.feedback('queryId', 'sentiment')).rejects.toThrow('An error occurred while sending feedback'); }); }); diff --git a/actions/GenericAction.js b/actions/GenericAction.js index 72c9819c..9390ef2d 100644 --- a/actions/GenericAction.js +++ b/actions/GenericAction.js @@ -10,7 +10,7 @@ * governing permissions and limitations under the License. */ -const { NetworkError } = require('./Network.js'); +const InternalError = require('./InternalError.js'); function createResponse(status, body) { return { @@ -33,11 +33,11 @@ function asGenericAction(action) { try { return createSuccessResponse(await action(params)); } catch (e) { - if (e instanceof NetworkError) { - console.error(`Network error: ${e.message} (${e.status})`); + if (e instanceof InternalError) { + console.error(`Internal error: ${e.message} (${e.status})`); return createErrorResponse(e.status, e.message); } - console.error(`Internal error: ${e.message}`); + console.error(`Unexpected error: ${e.message}`); return createErrorResponse(500, e.message ?? 'Internal Server Error'); } }; diff --git a/actions/ImsClient.js b/actions/ImsClient.js index 78c58905..544d693a 100644 --- a/actions/ImsClient.js +++ b/actions/ImsClient.js @@ -10,7 +10,7 @@ * governing permissions and limitations under the License. */ const FormUrlAddon = require('wretch/addons/formUrl'); -const { wretch } = require('./Network.js'); +const wretch = require('./Network.js'); class ImsClient { constructor(endpoint, clientId, clientSecret, permAuthCode) { diff --git a/actions/InternalError.js b/actions/InternalError.js new file mode 100644 index 00000000..54a348d3 --- /dev/null +++ b/actions/InternalError.js @@ -0,0 +1,20 @@ +/* + * Copyright 2023 Adobe. All rights reserved. + * This file is licensed to you 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 REPRESENTATIONS + * OF ANY KIND, either express or implied. See the License for the specific language + * governing permissions and limitations under the License. + */ +class InternalError extends Error { + constructor(status, message) { + super(message); + this.name = 'InternalError'; + this.status = status; + } +} + +module.exports = InternalError; diff --git a/actions/Network.js b/actions/Network.js index 054910a2..d4720306 100644 --- a/actions/Network.js +++ b/actions/Network.js @@ -11,19 +11,18 @@ */ const AbortAddon = require('wretch/addons/abort'); -const wretch = require('wretch'); +const { wretch, WretchError } = require('wretch'); const { Core } = require('@adobe/aio-sdk'); -const REQUEST_TIMEOUT = 55 * 1000; - const logger = Core.Logger('FirefallAction'); -class NetworkError extends Error { - constructor(status, message) { - super(message); - this.name = 'NetworkError'; - this.status = status; - } +const REQUEST_TIMEOUT = 55 * 1000; + +function createWretchError(status, message) { + const error = new WretchError(); + error.status = status; + error.message = message; + return error; } function wretchWithOptions(url) { @@ -34,12 +33,12 @@ function wretchWithOptions(url) { return _.fetchError((error) => { if (error.name === 'AbortError') { logger.error('Request aborted', error); - throw new NetworkError(408, 'Request timed out'); + throw createWretchError(408, 'Request timed out'); } logger.error('Network error', error); - throw new NetworkError(500, 'Network error'); + throw createWretchError(500, 'Network error'); }); }); } -module.exports = { wretch: wretchWithOptions, NetworkError }; +module.exports.wretch = wretchWithOptions; From add5131b02c1058d2c79745a14d47dc83128185d Mon Sep 17 00:00:00 2001 From: Vitaly Tsaplin Date: Fri, 15 Mar 2024 14:13:00 +0100 Subject: [PATCH 7/8] refactor: apply code review feedback --- actions/Network.js | 6 +++--- web-src/src/helpers/NetworkHelper.js | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/actions/Network.js b/actions/Network.js index d4720306..de5db051 100644 --- a/actions/Network.js +++ b/actions/Network.js @@ -28,9 +28,9 @@ function createWretchError(status, message) { function wretchWithOptions(url) { return wretch(url) .addon(AbortAddon()) - .resolve((_) => _.setTimeout(REQUEST_TIMEOUT)) - .resolve((_) => { - return _.fetchError((error) => { + .resolve((resolver) => resolver.setTimeout(REQUEST_TIMEOUT)) + .resolve((resolver) => { + return resolver.fetchError((error) => { if (error.name === 'AbortError') { logger.error('Request aborted', error); throw createWretchError(408, 'Request timed out'); diff --git a/web-src/src/helpers/NetworkHelper.js b/web-src/src/helpers/NetworkHelper.js index bcc63238..5fe91573 100644 --- a/web-src/src/helpers/NetworkHelper.js +++ b/web-src/src/helpers/NetworkHelper.js @@ -22,8 +22,8 @@ function unwrapError(error) { function wretchWithOptions(url) { return wretch(url) .headers({ 'X-OW-EXTRA-LOGGING': 'on' }) - .resolve((_) => { - return _ + .resolve((resolver) => { + return resolver .badRequest((err) => unwrapError(err)) .unauthorized((err) => unwrapError(err)) .forbidden((err) => unwrapError(err)) From d199b356652681b008858ae71e6f520d75e0c325 Mon Sep 17 00:00:00 2001 From: Vitaly Tsaplin Date: Fri, 15 Mar 2024 15:17:36 +0100 Subject: [PATCH 8/8] fix: wrong import --- actions/AuthAction.js | 2 +- actions/Network.js | 5 +++-- actions/csv/index.js | 2 +- actions/target/index.js | 2 +- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/actions/AuthAction.js b/actions/AuthAction.js index 5c02e374..ad312431 100644 --- a/actions/AuthAction.js +++ b/actions/AuthAction.js @@ -12,7 +12,7 @@ const { Core } = require('@adobe/aio-sdk'); const QueryStringAddon = require('wretch/addons/queryString'); const { ImsClient } = require('./ImsClient.js'); -const { wretch } = require('./Network.js'); +const wretch = require('./Network.js'); const logger = Core.Logger('AuthAction'); diff --git a/actions/Network.js b/actions/Network.js index de5db051..02ef475d 100644 --- a/actions/Network.js +++ b/actions/Network.js @@ -11,7 +11,8 @@ */ const AbortAddon = require('wretch/addons/abort'); -const { wretch, WretchError } = require('wretch'); +const { WretchError } = require('wretch'); +const wretch = require('wretch'); const { Core } = require('@adobe/aio-sdk'); const logger = Core.Logger('FirefallAction'); @@ -41,4 +42,4 @@ function wretchWithOptions(url) { }); } -module.exports.wretch = wretchWithOptions; +module.exports = wretchWithOptions; diff --git a/actions/csv/index.js b/actions/csv/index.js index 36962e38..103d0aca 100644 --- a/actions/csv/index.js +++ b/actions/csv/index.js @@ -9,8 +9,8 @@ * OF ANY KIND, either express or implied. See the License for the specific language * governing permissions and limitations under the License. */ -const wretch = require('wretch'); const Papa = require('papaparse'); +const wretch = require('../Network.js'); const { asGenericAction } = require('../GenericAction.js'); async function main({ url }) { diff --git a/actions/target/index.js b/actions/target/index.js index 0ad02666..ac7a696d 100644 --- a/actions/target/index.js +++ b/actions/target/index.js @@ -9,7 +9,7 @@ * OF ANY KIND, either express or implied. See the License for the specific language * governing permissions and limitations under the License. */ -const { wretch } = require('../Network.js'); +const wretch = require('../Network.js'); const { asGenericAction } = require('../GenericAction.js'); const MIN_DESCRIPTION_LENGTH = 5;