From a59a36bbbee440a2bd43b2c5a51cb38385ee944f Mon Sep 17 00:00:00 2001 From: duyhungtnn Date: Mon, 6 Jan 2025 15:11:43 +0700 Subject: [PATCH] fix: evaluation event being reported with no user ID (#89) --- src/__tests__/assert.ts | 75 +++++++++++ src/__tests__/client.ts | 2 +- .../client_assert_get_evaluation_rq.ts | 116 ++++++++++++++++++ src/__tests__/default_bkt_evaluation.ts | 15 +++ src/assert.ts | 24 ++++ src/evaluationDetails.ts | 3 +- src/index.ts | 27 ++-- 7 files changed, 252 insertions(+), 10 deletions(-) create mode 100644 src/__tests__/assert.ts create mode 100644 src/__tests__/client_assert_get_evaluation_rq.ts create mode 100644 src/assert.ts diff --git a/src/__tests__/assert.ts b/src/__tests__/assert.ts new file mode 100644 index 0000000..acbd5c6 --- /dev/null +++ b/src/__tests__/assert.ts @@ -0,0 +1,75 @@ +import test from 'ava'; +import { User } from '../objects/user'; +import { assertGetEvaluationRequest } from '../assert'; + +const assertGetEvaluationRequestTestCases = [ + { + name: 'assertGetEvaluationRequest throws error for invalid user', + user: { id: '', data: {} }, + featureID: 'feature1', + errorMessage: 'userID is empty' + }, + { + name: 'assertGetEvaluationRequest throws error for missing featureID', + user: { id: '123', data: {} }, + featureID: '', + errorMessage: 'featureID is required' + }, + { + name: 'assertGetEvaluationRequest throws error for null user', + user: null as any, + featureID: 'feature1', + errorMessage: 'user is null or undefined' + }, + { + name: 'assertGetEvaluationRequest throws error for undefined user', + user: undefined as any, + featureID: 'feature1', + errorMessage: 'user is null or undefined' + }, + { + name: 'assertGetEvaluationRequest throws error for user with null id', + user: { id: null as any, data: {} }, + featureID: 'feature1', + errorMessage: 'userID is empty' + }, + { + name: 'assertGetEvaluationRequest throws error for user with undefined id', + user: { id: undefined as any, data: {} }, + featureID: 'feature1', + errorMessage: 'userID is empty' + }, + { + name: 'assertGetEvaluationRequest throws error for null featureID', + user: { id: '123', data: {} }, + featureID: null as any, + errorMessage: 'featureID is required' + }, + { + name: 'assertGetEvaluationRequest throws error for undefined featureID', + user: { id: '123', data: {} }, + featureID: undefined as any, + errorMessage: 'featureID is required' + } +]; + +assertGetEvaluationRequestTestCases.forEach(testCase => { + test(testCase.name, t => { + const error = t.throws(() => { + assertGetEvaluationRequest(testCase.user, testCase.featureID); + }, { instanceOf: Error }); + t.is(error.message, testCase.errorMessage); + }); +}); + +test('assertGetEvaluationRequest does not throw error for valid user and featureID', t => { + const user: User = { + id: '123', + data: {} + }; + t.notThrows(() => { + assertGetEvaluationRequest(user, 'feature1'); + }); +}); + + diff --git a/src/__tests__/client.ts b/src/__tests__/client.ts index d4b29c5..2067d1a 100644 --- a/src/__tests__/client.ts +++ b/src/__tests__/client.ts @@ -1,7 +1,7 @@ import { http, HttpResponse } from 'msw'; import { SetupServer } from 'msw/node'; import anyTest, { TestFn } from 'ava'; -import { Bucketeer, initialize } from '..'; +import { BKTClientImpl, Bucketeer, initialize } from '..'; import { Config, User } from '../bootstrap'; import { DefaultLogger } from '../logger'; import { setupServerAndListen } from './utils/setup_server'; diff --git a/src/__tests__/client_assert_get_evaluation_rq.ts b/src/__tests__/client_assert_get_evaluation_rq.ts new file mode 100644 index 0000000..cee8f06 --- /dev/null +++ b/src/__tests__/client_assert_get_evaluation_rq.ts @@ -0,0 +1,116 @@ +import anyTest, { TestFn } from 'ava'; +import { BKTClientImpl, Bucketeer, initialize } from '..'; +import { Config, User } from '../bootstrap'; +import { DefaultLogger } from '../logger'; +import { newDefaultBKTEvaluationDetails } from '../evaluationDetails'; + +const test = anyTest as TestFn<{ + bktClient: Bucketeer; + targetedUser: User; + config: Config; +}>; + +test.beforeEach((t) => { + const config = { + host: 'api.bucketeer.io', + token: 'api_key_value', + tag: 'feature_tag_value', + logger: new DefaultLogger('expected'), + }; + t.context = { + bktClient: initialize(config), + targetedUser: { id: 'user_id', data: {} }, + config: config, + }; +}); + +const testCases = [ + { + description: 'return default value when featureID is empty', + featureId: '', + user: { id: 'user_id', data: {} }, + expected: newDefaultBKTEvaluationDetails('user_id', '', 'default-test', 'DEFAULT'), + }, + { + description: 'return default value when userID is empty', + featureId: 'stringEvaluationDetails', + user: { id: '', data: {} }, + expected: newDefaultBKTEvaluationDetails('', 'stringEvaluationDetails', 'default-test', 'DEFAULT'), + }, + { + description: 'return default value when userID & featureID is empty', + featureId: '', + user: { id: '', data: {} }, + expected: newDefaultBKTEvaluationDetails('', '', 'default-test', 'DEFAULT'), + }, + // Simulate the case where the object is null when passed from JavaScript code. + { + description: 'return default value when userID is null', + featureId: 'featureId', + user: null, + expected: newDefaultBKTEvaluationDetails('', 'featureId', 'default-test', 'DEFAULT'), + }, + { + description: 'return default value when featureId is null', + featureId: null, + user: { id: 'user_id', data: {} }, + expected: newDefaultBKTEvaluationDetails('user_id', '', 'default-test', 'DEFAULT'), + }, + { + description: 'return default value when featureId & userID is null', + featureId: null, + user: null, + expected: newDefaultBKTEvaluationDetails('', '', 'default-test', 'DEFAULT'), + }, + // Simulate the case where the user.id is null when passed from JavaScript code. + { + description: 'return default value when user.id is null', + featureId: 'featureId', + user: { id: null, data: {} }, + expected: newDefaultBKTEvaluationDetails('', 'featureId', 'default-test', 'DEFAULT'), + }, + // Simulate the case where the user.id is undefined when passed from JavaScript code. + { + description: 'return default value when user.id is undefined', + featureId: 'featureId', + user: { id: undefined, data: {} }, + expected: newDefaultBKTEvaluationDetails('', 'featureId', 'default-test', 'DEFAULT'), + }, + // Simulate the case where the object is undefined when passed from JavaScript code. + { + description: 'return default value when userID is undefined', + featureId: 'featureId', + user: undefined, + expected: newDefaultBKTEvaluationDetails('', 'featureId', 'default-test', 'DEFAULT'), + }, + { + description: 'return default value when featureId is undefined', + featureId: undefined, + user: { id: 'user_id', data: {} }, + expected: newDefaultBKTEvaluationDetails('user_id', '', 'default-test', 'DEFAULT'), + }, + { + description: 'return default value when featureId & userID is undefined', + featureId: undefined, + user: undefined, + expected: newDefaultBKTEvaluationDetails('', '', 'default-test', 'DEFAULT'), + }, +]; + +for (const testCase of testCases) { + test.serial(`getEvaluation: ${testCase.description}`, async (t) => { + const client = t.context.bktClient; + const clientImpl = client as BKTClientImpl; + + t.deepEqual( + // Type cast for simulate the case where the user object is null when passed from JavaScript code. + await client.stringVariationDetails(testCase.user as User, testCase.featureId as string, 'default-test'), + testCase.expected, + ); + t.true(clientImpl.eventStore.size() == 0, 'eventStore should be empty and not contain any error events'); + }); +} + +test.afterEach.always((t) => { + t.context.bktClient.destroy(); +}); \ No newline at end of file diff --git a/src/__tests__/default_bkt_evaluation.ts b/src/__tests__/default_bkt_evaluation.ts index c70a1ad..3b8207d 100644 --- a/src/__tests__/default_bkt_evaluation.ts +++ b/src/__tests__/default_bkt_evaluation.ts @@ -33,3 +33,18 @@ newDefaultBKTEvaluationDetailsTests.forEach((defaultValue, index) => { }); }); }); + +test('newDefaultBKTEvaluationDetails should return correct reason `DEFAULT`', (t) => { + const featureId = 'test_flag'; + const userId = 'user1'; + let output = newDefaultBKTEvaluationDetails(userId, featureId, 'default true', 'DEFAULT'); + t.deepEqual(output, { + featureId: featureId, + featureVersion: 0, + userId: userId, + variationId: '', + variationName: '', + variationValue: 'default true', + reason: 'DEFAULT', + }); +}); diff --git a/src/assert.ts b/src/assert.ts new file mode 100644 index 0000000..9e6af59 --- /dev/null +++ b/src/assert.ts @@ -0,0 +1,24 @@ +import { User } from './objects/user' + +/** + * Asserts that the provided user and featureID are valid for an evaluation request. + * + * @param user - The user object to be validated. + * @param featureID - The feature ID to be validated. + * @throws Will throw an error if the user is invalid or if the featureID is not provided. + */ +function assertGetEvaluationRequest(user: User, featureID: string) { + if (!user) { + throw new Error('user is null or undefined') + } + + if (!user.id) { + throw new Error('userID is empty') + } + + if (!featureID) { + throw new Error('featureID is required') + } +} + +export { assertGetEvaluationRequest } \ No newline at end of file diff --git a/src/evaluationDetails.ts b/src/evaluationDetails.ts index 3a5275c..1a77ddf 100644 --- a/src/evaluationDetails.ts +++ b/src/evaluationDetails.ts @@ -14,6 +14,7 @@ export const newDefaultBKTEvaluationDetails = ( userId: string, featureId: string, defaultValue: T, + reason: 'DEFAULT' | 'CLIENT' = 'CLIENT' ): BKTEvaluationDetails => { return { featureId: featureId, @@ -22,6 +23,6 @@ export const newDefaultBKTEvaluationDetails = ( variationId: '', variationName: '', variationValue: defaultValue, - reason: 'CLIENT', + reason: reason, } satisfies BKTEvaluationDetails; }; diff --git a/src/index.ts b/src/index.ts index d2eda26..e35a1ef 100644 --- a/src/index.ts +++ b/src/index.ts @@ -25,6 +25,7 @@ import { StringToTypeConverter, } from './converter'; import { error } from 'console'; +import { assertGetEvaluationRequest } from './assert'; export interface BuildInfo { readonly GIT_REVISION: string; @@ -57,7 +58,6 @@ export interface Bucketeer { */ getNumberVariation(user: User, featureId: string, defaultValue: number): Promise; - /** * booleanVariation returns variation as boolean. * If a variation returned by server is not boolean, defaultValue is retured. @@ -75,13 +75,13 @@ export interface Bucketeer { ): Promise>; /** - * stringVariation returns variation as string. - * If a variation returned by server is not string, defaultValue is retured. - * @param user User information. - * @param featureId Feature flag ID to use. - * @param defaultValue The variation value that is retured if SDK fails to fetch the variation or the variation is not string. - * @returns The variation value returned from server or default value. - */ + * stringVariation returns variation as string. + * If a variation returned by server is not string, defaultValue is retured. + * @param user User information. + * @param featureId Feature flag ID to use. + * @param defaultValue The variation value that is retured if SDK fails to fetch the variation or the variation is not string. + * @returns The variation value returned from server or default value. + */ stringVariation(user: User, featureId: string, defaultValue: string): Promise; stringVariationDetails( @@ -307,6 +307,17 @@ export class BKTClientImpl implements Bucketeer { defaultValue: T, typeConverter: StringToTypeConverter, ): Promise> { + try { + assertGetEvaluationRequest(user, featureId); + } catch (error) { + this.config.logger?.error('getVariationDetails failed', error); + return newDefaultBKTEvaluationDetails( + user && user.id ? user.id : '', + featureId ?? '', + defaultValue, + 'DEFAULT'); + } + const evaluation = await this.getEvaluation(user, featureId); const variationValue = evaluation?.variationValue;