From da5ba76aed092e1de9f2d80ab7f685c11161ddc9 Mon Sep 17 00:00:00 2001 From: Marcos Passos Date: Sun, 25 Aug 2024 17:20:23 -0600 Subject: [PATCH] Add help links and remove deprecated API (#417) --- src/channel/httpBeaconChannel.ts | 37 ++----- src/contentFetcher.ts | 38 ++++---- src/evaluator.ts | 76 +++++++-------- src/help.ts | 24 +++++ test/channel/httpBeaconChannel.test.ts | 14 ++- test/contentFetcher.test.ts | 121 ++++++++++++++--------- test/evaluator.test.ts | 129 +++++++++++++++---------- test/help.test.ts | 33 +++++++ 8 files changed, 276 insertions(+), 196 deletions(-) create mode 100644 src/help.ts create mode 100644 test/help.test.ts diff --git a/src/channel/httpBeaconChannel.ts b/src/channel/httpBeaconChannel.ts index 74bf0302..bf7e4536 100644 --- a/src/channel/httpBeaconChannel.ts +++ b/src/channel/httpBeaconChannel.ts @@ -4,6 +4,7 @@ import {Logger, NullLogger} from '../logging'; import {CidAssigner} from '../cid'; import {formatMessage} from '../error'; import {CLIENT_LIBRARY} from '../constants'; +import {Help} from '../help'; export type Configuration = { appId: string, @@ -72,38 +73,12 @@ export class HttpBeaconChannel implements DuplexChannel response.json() .then(body => { if (response.ok) { - resolve(body); - } else { - reject(new ContentError(body)); + return resolve(body); } + + this.logHelp(response.status); + + reject(new ContentError(body)); }) .catch(error => { if (!response.ok) { @@ -211,15 +212,6 @@ export class ContentFetcher { const dynamic = ContentFetcher.isDynamicContent(options); if (dynamic) { - if (options.userAgent !== undefined) { - this.logger.warn( - 'The `userAgent` option is deprecated and ' - + 'will be removed in future releases. ' - + 'Please update the part of your code calling the `fetch` method ' - + 'to use the `clientAgent` option instead.', - ); - } - if (options.clientId !== undefined) { headers['X-Client-Id'] = options.clientId; } @@ -232,10 +224,8 @@ export class ContentFetcher { headers['X-Token'] = options.userToken.toString(); } - const clientAgent = options.clientAgent ?? options.userAgent; - - if (clientAgent !== undefined) { - headers['X-Client-Agent'] = clientAgent; + if (options.clientAgent !== undefined) { + headers['X-Client-Agent'] = options.clientAgent; } if (options.context !== undefined) { @@ -264,6 +254,14 @@ export class ContentFetcher { }); } + private logHelp(statusCode: number): void { + const help = Help.forStatusCode(statusCode); + + if (help !== undefined) { + this.logger.error(help); + } + } + private static isDynamicContent(options: FetchOptions): options is DynamicContentOptions { return options.static !== true; } diff --git a/src/evaluator.ts b/src/evaluator.ts index d052c05f..61c7e383 100644 --- a/src/evaluator.ts +++ b/src/evaluator.ts @@ -5,6 +5,7 @@ import {formatMessage} from './error'; import {getLength, getLocation, Location} from './sourceLocation'; import {Logger, NullLogger} from './logging'; import type {ApiKey} from './apiKey'; +import {Help} from './help'; export type Campaign = { name?: string, @@ -37,10 +38,6 @@ export type EvaluationOptions = { clientId?: string, clientIp?: string, clientAgent?: string, - /** - * @deprecated Use `clientAgent` instead. This option will be removed in future releases. - */ - userAgent?: string, userToken?: Token|string, timeout?: number, context?: EvaluationContext, @@ -154,12 +151,12 @@ export class Evaluator { return Promise.reject(new QueryError(response)); } - const body: JsonObject = { + const payload: JsonObject = { query: query, }; if (options.context !== undefined) { - body.context = options.context; + payload.context = options.context; } return new Promise((resolve, reject) => { @@ -177,33 +174,37 @@ export class Evaluator { abortController.abort(); + this.logHelp(response.status); + reject(new EvaluationError(response)); }, options.timeout, ); } - const promise = this.fetch(body, abortController.signal, options); + const promise = this.fetch(payload, abortController.signal, options); promise.then( response => response.json() - .then(data => { + .then(body => { if (response.ok) { - return resolve(data); + return resolve(body); } - const errorResponse: ErrorResponse = data; + this.logHelp(response.status); + + const problem: ErrorResponse = body; - switch (errorResponse.type) { + switch (problem.type) { case EvaluationErrorType.INVALID_QUERY: case EvaluationErrorType.EVALUATION_FAILED: case EvaluationErrorType.TOO_COMPLEX_QUERY: - reject(new QueryError(errorResponse as QueryErrorResponse)); + reject(new QueryError(problem as QueryErrorResponse)); break; default: - reject(new EvaluationError(errorResponse)); + reject(new EvaluationError(problem)); break; } @@ -215,37 +216,26 @@ export class Evaluator { throw error; }), - ) - .catch( - error => { - if (!abortController.signal.aborted) { - reject( - new EvaluationError({ - title: formatMessage(error), - type: EvaluationErrorType.UNEXPECTED_ERROR, - detail: 'Please try again or contact Croct support if the error persists.', - status: 500, // Internal Server Error - }), - ); - } - }, - ); + ).catch( + error => { + if (!abortController.signal.aborted) { + reject( + new EvaluationError({ + title: formatMessage(error), + type: EvaluationErrorType.UNEXPECTED_ERROR, + detail: 'Please try again or contact Croct support if the error persists.', + status: 500, // Internal Server Error + }), + ); + } + }, + ); }); } private fetch(body: JsonObject, signal: AbortSignal, options: EvaluationOptions): Promise { const {appId, apiKey} = this.configuration; - const {clientId, clientIp, userToken} = options; - const clientAgent = options.clientAgent ?? options.userAgent; - - if (options.userAgent !== undefined) { - this.logger.warn( - 'The `userAgent` option is deprecated and ' - + 'will be removed in future releases. ' - + 'Please update the part of your code calling the `evaluate` method ' - + 'to use the `clientAgent` option instead.', - ); - } + const {clientId, clientIp, userToken, clientAgent} = options; const headers: Record = { 'Content-Type': 'application/json', @@ -292,6 +282,14 @@ export class Evaluator { }); } + private logHelp(statusCode: number): void { + const help = Help.forStatusCode(statusCode); + + if (help !== undefined) { + this.logger.error(help); + } + } + public toJSON(): never { // Prevent sensitive configuration from being serialized throw new Error('Unserializable value.'); diff --git a/src/help.ts b/src/help.ts new file mode 100644 index 00000000..003d8f94 --- /dev/null +++ b/src/help.ts @@ -0,0 +1,24 @@ +export namespace Help { + export function forStatusCode(statusCode: number): string|undefined { + switch (statusCode) { + case 401: + return 'The request was not authorized, most likely due to invalid credentials. ' + + 'For help, see https://croct.help/sdk/js/invalid-credentials'; + + case 403: + return 'The origin of the request is not allowed in your application settings. ' + + 'For help, see https://croct.help/sdk/js/cors'; + + case 408: + return 'The request timed out. ' + + 'For help, see https://croct.help/sdk/js/timeout'; + + case 423: + return 'The application has exceeded the monthly active users (MAU) quota. ' + + 'For help, see https://croct.help/sdk/js/mau-exceeded'; + + default: + return undefined; + } + } +} diff --git a/test/channel/httpBeaconChannel.test.ts b/test/channel/httpBeaconChannel.test.ts index 87ff2b7b..cc908868 100644 --- a/test/channel/httpBeaconChannel.test.ts +++ b/test/channel/httpBeaconChannel.test.ts @@ -5,6 +5,7 @@ import {FixedAssigner} from '../../src/cid'; import {Beacon} from '../../src/trackingEvents'; import {Token} from '../../src/token'; import {CLIENT_LIBRARY} from '../../src/constants'; +import {Help} from '../../src/help'; describe('An HTTP beacon channel', () => { beforeEach(() => { @@ -170,7 +171,7 @@ describe('An HTTP beacon channel', () => { type NonRetryableErrorScenario = { status: number, title: string, - log: string, + log?: string, }; it.each([ @@ -182,8 +183,6 @@ describe('An HTTP beacon channel', () => { { status: 401, title: 'Unauthorized request', - log: 'The request was not authorized, most likely due to invalid credentials. ' - + 'For help, see https://croct.help/sdk/js/invalid-credentials', }, { status: 402, @@ -193,17 +192,16 @@ describe('An HTTP beacon channel', () => { { status: 403, title: 'Unallowed origin', - log: 'The origin of the request is not allowed in your application settings. ' - + 'For help, see https://croct.help/sdk/js/cors', }, { status: 423, title: 'Quota exceeded', - log: 'The application has exceeded the monthly active users (MAU) quota. ' - + 'For help, see https://croct.help/sdk/js/mau-exceeded', }, ])('should report a non-retryable error if the response status is $status', async scenario => { - const {status, title, log} = scenario; + const {status, title} = scenario; + const log = scenario.log ?? Help.forStatusCode(status); + + expect(log).toBeDefined(); fetchMock.mock(endpointUrl, { status: status, diff --git a/test/contentFetcher.test.ts b/test/contentFetcher.test.ts index 1c71a3f5..09727873 100644 --- a/test/contentFetcher.test.ts +++ b/test/contentFetcher.test.ts @@ -4,8 +4,9 @@ import {EvaluationContext} from '../src/evaluator'; import {Token} from '../src/token'; import {ContentFetcher, ContentError, ContentErrorType, ErrorResponse, FetchOptions} from '../src/contentFetcher'; import {BASE_ENDPOINT_URL, CLIENT_LIBRARY} from '../src/constants'; -import {Logger} from '../src/logging'; import {ApiKey} from '../src/apiKey'; +import {Logger} from '../src/logging'; +import {Help} from '../src/help'; jest.mock( '../src/constants', @@ -251,41 +252,6 @@ describe('A content fetcher', () => { await expect(fetcher.fetch(contentId, options)).resolves.toEqual(content); }); - it('should warn when passing a userAgent option', async () => { - const logger: Logger = { - debug: jest.fn(), - info: jest.fn(), - warn: jest.fn(), - error: jest.fn(), - }; - - const fetcher = new ContentFetcher({ - appId: appId, - logger: logger, - }); - - const userAgent = 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7)'; - - const options: FetchOptions = { - userAgent: userAgent, - }; - - fetchMock.mock({ - ...requestMatcher, - headers: { - ...requestMatcher.headers, - 'X-Client-Agent': userAgent, - }, - response: content, - }); - - await expect(fetcher.fetch(contentId, options)).resolves.toEqual(content); - - expect(logger.warn).toHaveBeenCalledWith( - expect.stringContaining('The `userAgent` option is deprecated'), - ); - }); - it('should fetch dynamic content using the provided user token', async () => { const token = Token.issue(appId, 'foo', Date.now()); @@ -369,8 +335,16 @@ describe('A content fetcher', () => { }); it('should abort after reaching the timeout', async () => { + const logger: Logger = { + debug: jest.fn(), + info: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + }; + const fetcher = new ContentFetcher({ appId: appId, + logger: logger, }); fetchMock.mock({ @@ -391,16 +365,16 @@ describe('A content fetcher', () => { await expect(promise).rejects.toThrow(ContentError); - await expect(promise).rejects.toEqual(expect.objectContaining({ - response: { - title: 'Maximum timeout reached before content could be loaded.', - type: ContentErrorType.TIMEOUT, - detail: 'The content took more than 10ms to load.', - status: 408, - }, - })); + await expect(promise).rejects.toHaveProperty('response', { + title: 'Maximum timeout reached before content could be loaded.', + type: ContentErrorType.TIMEOUT, + detail: 'The content took more than 10ms to load.', + status: 408, + }); expect(fetchOptions?.signal.aborted).toBe(true); + + expect(logger.error).toHaveBeenCalledWith(Help.forStatusCode(408)); }); it('should fetch dynamic content using the provided context', async () => { @@ -505,7 +479,7 @@ describe('A content fetcher', () => { const promise = fetcher.fetch(contentId); await expect(promise).rejects.toThrow(ContentError); - await expect(promise).rejects.toEqual(expect.objectContaining({response: response})); + await expect(promise).rejects.toHaveProperty('response', response); }); it('should catch deserialization errors', async () => { @@ -531,7 +505,7 @@ describe('A content fetcher', () => { const promise = fetcher.fetch(contentId); await expect(promise).rejects.toThrow(ContentError); - await expect(promise).rejects.toEqual(expect.objectContaining({response: response})); + await expect(promise).rejects.toHaveProperty('response', response); }); it('should catch unexpected error responses', async () => { @@ -582,7 +556,60 @@ describe('A content fetcher', () => { const promise = fetcher.fetch(contentId); await expect(promise).rejects.toThrow(ContentError); - await expect(promise).rejects.toEqual(expect.objectContaining({response: response})); + await expect(promise).rejects.toHaveProperty('response', response); + }); + + type HelpScenario = { + status: number, + title: string, + }; + + it.each([ + { + status: 401, + title: 'Unauthorized request', + }, + { + status: 403, + title: 'Unallowed origin', + }, + { + status: 423, + title: 'Quota exceeded', + }, + ])('should log help messages for status code $status', async scenario => { + const logger: Logger = { + debug: jest.fn(), + info: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + }; + + const fetcher = new ContentFetcher({ + appId: appId, + logger: logger, + }); + + fetchMock.mock({ + ...requestMatcher, + response: { + status: scenario.status, + body: { + type: 'https://croct.help/api/content', + status: scenario.status, + title: scenario.title, + } satisfies ErrorResponse, + }, + }); + + const promise = fetcher.fetch(contentId); + + await expect(promise).rejects.toThrowWithMessage(ContentError, scenario.title); + + const log = Help.forStatusCode(scenario.status); + + expect(log).toBeDefined(); + expect(logger.error).toHaveBeenCalledWith(log); }); it('should not be serializable', () => { diff --git a/test/evaluator.test.ts b/test/evaluator.test.ts index 3fd95d45..8cc638ac 100644 --- a/test/evaluator.test.ts +++ b/test/evaluator.test.ts @@ -12,8 +12,9 @@ import { } from '../src/evaluator'; import {Token} from '../src/token'; import {BASE_ENDPOINT_URL, CLIENT_LIBRARY} from '../src/constants'; -import {Logger} from '../src/logging'; import {ApiKey} from '../src/apiKey'; +import {Logger} from '../src/logging'; +import {Help} from '../src/help'; jest.mock( '../src/constants', @@ -219,43 +220,6 @@ describe('An evaluator', () => { await expect(evaluator.evaluate(query, options)).resolves.toBe(result); }); - it('should warn when passing a userAgent option', async () => { - const logger: Logger = { - debug: jest.fn(), - info: jest.fn(), - warn: jest.fn(), - error: jest.fn(), - }; - - const evaluator = new Evaluator({ - appId: appId, - logger: logger, - }); - - const userAgent = 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7)'; - - const result = 'Carol'; - - fetchMock.mock({ - ...requestMatcher, - headers: { - ...requestMatcher.headers, - 'X-Client-Agent': userAgent, - }, - response: JSON.stringify(result), - }); - - const options: EvaluationOptions = { - userAgent: userAgent, - }; - - await expect(evaluator.evaluate(query, options)).resolves.toBe(result); - - expect(logger.warn).toHaveBeenCalledWith( - expect.stringContaining('The `userAgent` option is deprecated '), - ); - }); - it('should fetch using the extra options', async () => { const evaluator = new Evaluator({ appId: appId, @@ -293,8 +257,16 @@ describe('An evaluator', () => { }); it('should abort the evaluation if the timeout is reached', async () => { + const logger: Logger = { + debug: jest.fn(), + info: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + }; + const evaluator = new Evaluator({ appId: appId, + logger: logger, }); fetchMock.mock({ @@ -314,16 +286,15 @@ describe('An evaluator', () => { expect(fetchOptions?.signal).toBeDefined(); await expect(promise).rejects.toThrow(EvaluationError); - await expect(promise).rejects.toThrow(expect.objectContaining({ - response: { - title: 'Maximum evaluation timeout reached before evaluation could complete.', - type: EvaluationErrorType.TIMEOUT, - detail: 'The evaluation took more than 10ms to complete.', - status: 408, - }, - })); + await expect(promise).rejects.toHaveProperty('response', { + title: 'Maximum evaluation timeout reached before evaluation could complete.', + type: EvaluationErrorType.TIMEOUT, + detail: 'The evaluation took more than 10ms to complete.', + status: 408, + }); expect(fetchOptions?.signal.aborted).toBe(true); + expect(logger.error).toHaveBeenCalledWith(Help.forStatusCode(408)); }); it('should evaluate queries using the provided context', async () => { @@ -388,7 +359,7 @@ describe('An evaluator', () => { const promise = evaluator.evaluate(query); await expect(promise).rejects.toThrow(EvaluationError); - await expect(promise).rejects.toEqual(expect.objectContaining({response: response})); + await expect(promise).rejects.toHaveProperty('response', response); }); it.each([ @@ -434,7 +405,7 @@ describe('An evaluator', () => { const promise = evaluator.evaluate(query); await expect(promise).rejects.toThrow(QueryError); - await expect(promise).rejects.toEqual(expect.objectContaining({response: response})); + await expect(promise).rejects.toHaveProperty('response', response); }, ); @@ -470,7 +441,7 @@ describe('An evaluator', () => { const promise = evaluator.evaluate('_'.repeat(length)); await expect(promise).rejects.toThrow(QueryError); - await expect(promise).rejects.toEqual(expect.objectContaining({response: response})); + await expect(promise).rejects.toHaveProperty('response', response); }); it('should catch deserialization errors', async () => { @@ -496,7 +467,7 @@ describe('An evaluator', () => { const promise = evaluator.evaluate(query); await expect(promise).rejects.toThrow(EvaluationError); - await expect(promise).rejects.toEqual(expect.objectContaining({response: response})); + await expect(promise).rejects.toHaveProperty('response', response); }); it('should catch unexpected error responses', async () => { @@ -547,7 +518,63 @@ describe('An evaluator', () => { const promise = evaluator.evaluate(query); await expect(promise).rejects.toThrow(EvaluationError); - await expect(promise).rejects.toEqual(expect.objectContaining({response: response})); + await expect(promise).rejects.toHaveProperty('response', response); + }); + + type HelpScenario = { + status: number, + title: string, + }; + + it.each([ + { + status: 401, + title: 'Unauthorized request', + }, + { + status: 403, + title: 'Unallowed origin', + }, + { + status: 423, + title: 'Quota exceeded', + }, + ])('should log help messages for status code $status', async scenario => { + const logger: Logger = { + debug: jest.fn(), + info: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + }; + + const evaluator = new Evaluator({ + appId: appId, + logger: logger, + }); + + const response: ErrorResponse = { + title: scenario.title, + type: EvaluationErrorType.UNEXPECTED_ERROR, + status: scenario.status, + }; + + fetchMock.mock({ + ...requestMatcher, + response: { + status: scenario.status, + body: response, + }, + }); + + const promise = evaluator.evaluate(query); + + await expect(promise).rejects.toThrowWithMessage(EvaluationError, scenario.title); + + const help = Help.forStatusCode(scenario.status); + + expect(help).toBeDefined(); + + expect(logger.error).toHaveBeenCalledWith(help); }); it('should not be serializable', () => { diff --git a/test/help.test.ts b/test/help.test.ts new file mode 100644 index 00000000..2ea84f30 --- /dev/null +++ b/test/help.test.ts @@ -0,0 +1,33 @@ +import {Help} from '../src/help'; + +describe('A function to provide help for errors', () => { + type StatusCodeScenario = { + status: number, + help: string, + }; + + it.each([ + { + status: 401, + help: 'https://croct.help/sdk/js/invalid-credentials', + }, + { + status: 403, + help: 'https://croct.help/sdk/js/cors', + }, + { + status: 408, + help: 'https://croct.help/sdk/js/timeout', + }, + { + status: 423, + help: 'https://croct.help/sdk/js/mau-exceeded', + }, + ])('should provide help for status code %i', scenario => { + expect(Help.forStatusCode(scenario.status)).toContain(scenario.help); + }); + + it('should return undefined for status codes without help', () => { + expect(Help.forStatusCode(999)).toBeUndefined(); + }); +});