From 39bb672446598545117c74226954c02eca1fff6a Mon Sep 17 00:00:00 2001 From: Andrew Bell <115623869+andybharness@users.noreply.github.com> Date: Thu, 23 Nov 2023 14:09:59 +0000 Subject: [PATCH 1/6] [FFM-10041] - Fall back to identifier if bucketBy attribute is not found What Update BucketBy logic to fall back to the target identifier if given BucketBy attribute is not found. Why Keep the SDK consistent with the behaviour of the other server SDKs. Testing Testgrid + new unit test --- src/__tests__/evaluator.test.ts | 80 +++++++++++++++++++++++++++++++++ src/__tests__/sdk_codes.test.ts | 1 + src/evaluator.ts | 20 +++++++-- src/sdk_codes.ts | 11 +++++ 4 files changed, 109 insertions(+), 3 deletions(-) create mode 100644 src/__tests__/evaluator.test.ts diff --git a/src/__tests__/evaluator.test.ts b/src/__tests__/evaluator.test.ts new file mode 100644 index 0000000..532275a --- /dev/null +++ b/src/__tests__/evaluator.test.ts @@ -0,0 +1,80 @@ +import { Evaluator } from '../evaluator'; +import { Logger } from '../log'; + +describe('Evaluator', () => { + it('should return identifier if bucketby attribute does not exist', async () => { + const logger: Logger = { + trace: jest.fn(), + debug: jest.fn(), + info: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + }; + + const mockQuery = { + getFlag: jest.fn(), + getSegment: jest.fn(), + findFlagsBySegment: jest.fn(), + }; + + const featureConfig = { + feature: 'flag', + kind: 'string', + state: 'on', + variationToTargetMap: [], + variations: [ + { identifier: 'variation1', value: 'default_on' }, + { identifier: 'variation2', value: 'wanted_value' }, + { identifier: 'variation3', value: 'default_off' }, + ], + defaultServe: { + distribution: null, + variation: 'default_on', + }, + offVariation: 'variation3', + rules: [ + { + ruleId: 'rule1', + clauses: [ + { + op: 'equal', + attribute: 'identifier', + values: ['test'], + }, + ], + serve: { + distribution: { + bucketBy: 'i_do_not_exist', + variations: [ + { variation: 'variation1', weight: 56 }, + { variation: 'variation2', weight: 1 }, // bucket 57 + { variation: 'variation3', weight: 43 }, + ], + }, + }, + }, + ], + }; + + mockQuery.getFlag.mockReturnValue(featureConfig); + + const evaluator = new Evaluator(mockQuery, logger); + + const target = { + identifier: 'test', // Test will fall back to bucketing on this (bucket 57) + name: 'test name', + attributes: { + location: 'emea', + }, + }; + + const actualVariation = await evaluator.stringVariation( + 'flag', + target, + 'fallback_value', + null, + ); + + expect(actualVariation).toEqual('wanted_value'); + }); +}); diff --git a/src/__tests__/sdk_codes.test.ts b/src/__tests__/sdk_codes.test.ts index 504c220..d6a7527 100644 --- a/src/__tests__/sdk_codes.test.ts +++ b/src/__tests__/sdk_codes.test.ts @@ -44,6 +44,7 @@ describe('SDK Codes', () => { 'warnDefaultVariationServed', ['flag', { name: 'dummy', identifier: 'dummy' }, 'default value', logger], ], + ['warnBucketByAttributeNotFound', ['dummy1', 'dummy2', logger]], ])('it should not throw when %s is called', (fn, args) => { expect(() => sdkCodes[fn](...args)).not.toThrow(); }); diff --git a/src/evaluator.ts b/src/evaluator.ts index 6d38226..86b91a1 100644 --- a/src/evaluator.ts +++ b/src/evaluator.ts @@ -23,7 +23,11 @@ import { } from './openapi'; import murmurhash from 'murmurhash'; import { ConsoleLog } from './log'; -import { debugEvalSuccess, warnDefaultVariationServed } from './sdk_codes'; +import { + debugEvalSuccess, + warnBucketByAttributeNotFound, + warnDefaultVariationServed, +} from './sdk_codes'; type Callback = ( fc: FeatureConfig, @@ -76,9 +80,19 @@ export class Evaluator { bucketBy: string, percentage: number, ): boolean { - const property = this.getAttrValue(target, bucketBy); + let property = this.getAttrValue(target, bucketBy); if (!property) { - return false; + const old_bucketBy = bucketBy; + bucketBy = 'identifier'; + property = this.getAttrValue(target, bucketBy); + if (!property) { + return false; + } + warnBucketByAttributeNotFound( + old_bucketBy, + property?.toString(), + this.log, + ); } const bucketId = this.getNormalizedNumber(property, bucketBy); return percentage > 0 && bucketId <= percentage; diff --git a/src/sdk_codes.ts b/src/sdk_codes.ts index 9211a92..43464a5 100644 --- a/src/sdk_codes.ts +++ b/src/sdk_codes.ts @@ -26,6 +26,7 @@ const sdkCodes: Record = { // SDK_EVAL_6xxx - 6000: 'Evaluation successful: ', 6001: 'Evaluation Failed, returning default variation: ', + 6002: "BucketBy attribute not found in target attributes, falling back to 'identifier':", // SDK_METRICS_7xxx 7000: 'Metrics thread started with request interval:', 7001: 'Metrics stopped', @@ -173,3 +174,13 @@ export function warnDefaultVariationServed( ), ); } + +export function warnBucketByAttributeNotFound( + bucketBy: string, + usingValue: string, + logger: Logger, +): void { + logger.warn( + getSdkErrMsg(6002, `missing=${bucketBy}, using value=${usingValue}`), + ); +} From c3653ef00e2d697a2e7bdf0f02264593953c9db4 Mon Sep 17 00:00:00 2001 From: Andrew Bell <115623869+andybharness@users.noreply.github.com> Date: Thu, 23 Nov 2023 14:14:03 +0000 Subject: [PATCH 2/6] update version numbers --- examples/getting_started/package-lock.json | 2 +- package-lock.json | 4 ++-- package.json | 2 +- src/version.ts | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/examples/getting_started/package-lock.json b/examples/getting_started/package-lock.json index fd164da..8412b2f 100644 --- a/examples/getting_started/package-lock.json +++ b/examples/getting_started/package-lock.json @@ -13,7 +13,7 @@ }, "../..": { "name": "@harnessio/ff-nodejs-server-sdk", - "version": "1.3.7", + "version": "1.3.8", "bundleDependencies": [ "axios", "eventsource", diff --git a/package-lock.json b/package-lock.json index 526b4ed..cc98ce8 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@harnessio/ff-nodejs-server-sdk", - "version": "1.3.7", + "version": "1.3.8", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "@harnessio/ff-nodejs-server-sdk", - "version": "1.3.7", + "version": "1.3.8", "bundleDependencies": [ "axios", "eventsource", diff --git a/package.json b/package.json index f3afa2b..fce5a60 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@harnessio/ff-nodejs-server-sdk", - "version": "1.3.7", + "version": "1.3.8", "description": "Feature flags SDK for NodeJS environments", "main": "dist/cjs/index.js", "module": "dist/esm/index.mjs", diff --git a/src/version.ts b/src/version.ts index 01d6001..a29a557 100644 --- a/src/version.ts +++ b/src/version.ts @@ -1 +1 @@ -export const VERSION = '1.3.7'; +export const VERSION = '1.3.8'; From 85750c1dc0c714c85c61097633c2fd8f3e6fcd98 Mon Sep 17 00:00:00 2001 From: Andrew Bell <115623869+andybharness@users.noreply.github.com> Date: Fri, 24 Nov 2023 08:57:00 +0000 Subject: [PATCH 3/6] Update src/evaluator.ts Co-authored-by: Kevin Nagurski --- src/evaluator.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/evaluator.ts b/src/evaluator.ts index 86b91a1..f5274c1 100644 --- a/src/evaluator.ts +++ b/src/evaluator.ts @@ -82,7 +82,7 @@ export class Evaluator { ): boolean { let property = this.getAttrValue(target, bucketBy); if (!property) { - const old_bucketBy = bucketBy; + const oldBucketBy = bucketBy; bucketBy = 'identifier'; property = this.getAttrValue(target, bucketBy); if (!property) { From 205456e6989588af23150346a9fa1d5a7422832e Mon Sep 17 00:00:00 2001 From: Andrew Bell <115623869+andybharness@users.noreply.github.com> Date: Fri, 24 Nov 2023 08:58:20 +0000 Subject: [PATCH 4/6] Update src/evaluator.ts Co-authored-by: Kevin Nagurski --- src/evaluator.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/evaluator.ts b/src/evaluator.ts index f5274c1..8f69b0c 100644 --- a/src/evaluator.ts +++ b/src/evaluator.ts @@ -85,9 +85,11 @@ export class Evaluator { const oldBucketBy = bucketBy; bucketBy = 'identifier'; property = this.getAttrValue(target, bucketBy); + if (!property) { return false; } + warnBucketByAttributeNotFound( old_bucketBy, property?.toString(), From 8506dbb26869655cfe55719718c45670165106f9 Mon Sep 17 00:00:00 2001 From: Andrew Bell <115623869+andybharness@users.noreply.github.com> Date: Fri, 24 Nov 2023 09:51:51 +0000 Subject: [PATCH 5/6] review comments --- examples/getting_started/package-lock.json | 2 +- package-lock.json | 4 ++-- package.json | 2 +- src/__tests__/evaluator.test.ts | 14 ++++++------ src/evaluator.ts | 26 +++++++++------------- src/version.ts | 2 +- 6 files changed, 23 insertions(+), 27 deletions(-) diff --git a/examples/getting_started/package-lock.json b/examples/getting_started/package-lock.json index 8412b2f..dc91897 100644 --- a/examples/getting_started/package-lock.json +++ b/examples/getting_started/package-lock.json @@ -13,7 +13,7 @@ }, "../..": { "name": "@harnessio/ff-nodejs-server-sdk", - "version": "1.3.8", + "version": "1.4.0", "bundleDependencies": [ "axios", "eventsource", diff --git a/package-lock.json b/package-lock.json index cc98ce8..0ecdf6f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@harnessio/ff-nodejs-server-sdk", - "version": "1.3.8", + "version": "1.4.0", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "@harnessio/ff-nodejs-server-sdk", - "version": "1.3.8", + "version": "1.4.0", "bundleDependencies": [ "axios", "eventsource", diff --git a/package.json b/package.json index fce5a60..e30715d 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@harnessio/ff-nodejs-server-sdk", - "version": "1.3.8", + "version": "1.4.0", "description": "Feature flags SDK for NodeJS environments", "main": "dist/cjs/index.js", "module": "dist/esm/index.mjs", diff --git a/src/__tests__/evaluator.test.ts b/src/__tests__/evaluator.test.ts index 532275a..8fd32a9 100644 --- a/src/__tests__/evaluator.test.ts +++ b/src/__tests__/evaluator.test.ts @@ -2,7 +2,7 @@ import { Evaluator } from '../evaluator'; import { Logger } from '../log'; describe('Evaluator', () => { - it('should return identifier if bucketby attribute does not exist', async () => { + it('should fallback to identifier if bucketby attribute does not exist', async () => { const logger: Logger = { trace: jest.fn(), debug: jest.fn(), @@ -11,13 +11,13 @@ describe('Evaluator', () => { error: jest.fn(), }; - const mockQuery = { + const mock_query = { getFlag: jest.fn(), getSegment: jest.fn(), findFlagsBySegment: jest.fn(), }; - const featureConfig = { + const feature_config = { feature: 'flag', kind: 'string', state: 'on', @@ -56,9 +56,9 @@ describe('Evaluator', () => { ], }; - mockQuery.getFlag.mockReturnValue(featureConfig); + mock_query.getFlag.mockReturnValue(feature_config); - const evaluator = new Evaluator(mockQuery, logger); + const evaluator = new Evaluator(mock_query, logger); const target = { identifier: 'test', // Test will fall back to bucketing on this (bucket 57) @@ -68,13 +68,13 @@ describe('Evaluator', () => { }, }; - const actualVariation = await evaluator.stringVariation( + const actual_variation = await evaluator.stringVariation( 'flag', target, 'fallback_value', null, ); - expect(actualVariation).toEqual('wanted_value'); + expect(actual_variation).toEqual('wanted_value'); }); }); diff --git a/src/evaluator.ts b/src/evaluator.ts index 8f69b0c..7c23c0d 100644 --- a/src/evaluator.ts +++ b/src/evaluator.ts @@ -58,8 +58,8 @@ export class Evaluator { } private getNormalizedNumberWithNormalizer( - property: Type, bucketBy: string, + property: Type, normalizer: number, ): number { const value = [bucketBy, property].join(':'); @@ -67,10 +67,10 @@ export class Evaluator { return (hash % normalizer) + 1; } - private getNormalizedNumber(property: Type, bucketBy: string): number { + private getNormalizedNumber(bucketBy: string, property: Type): number { return this.getNormalizedNumberWithNormalizer( - property, bucketBy, + property, ONE_HUNDRED, ); } @@ -80,23 +80,19 @@ export class Evaluator { bucketBy: string, percentage: number, ): boolean { - let property = this.getAttrValue(target, bucketBy); + let bb = bucketBy; + let property = this.getAttrValue(target, bb); if (!property) { - const oldBucketBy = bucketBy; - bucketBy = 'identifier'; - property = this.getAttrValue(target, bucketBy); - + bb = 'identifier'; + property = this.getAttrValue(target, bb); + if (!property) { return false; } - - warnBucketByAttributeNotFound( - old_bucketBy, - property?.toString(), - this.log, - ); + + warnBucketByAttributeNotFound(bb, property?.toString(), this.log); } - const bucketId = this.getNormalizedNumber(property, bucketBy); + const bucketId = this.getNormalizedNumber(bb, property); return percentage > 0 && bucketId <= percentage; } diff --git a/src/version.ts b/src/version.ts index a29a557..b44ee38 100644 --- a/src/version.ts +++ b/src/version.ts @@ -1 +1 @@ -export const VERSION = '1.3.8'; +export const VERSION = '1.4.0'; From 8bb25a997a83db2fd2219a8bdca1cfbace13b42c Mon Sep 17 00:00:00 2001 From: Andrew Bell <115623869+andybharness@users.noreply.github.com> Date: Fri, 24 Nov 2023 11:43:04 +0000 Subject: [PATCH 6/6] minor fix, update logger to show missing field not the one being used --- src/evaluator.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/evaluator.ts b/src/evaluator.ts index 7c23c0d..e92d8c8 100644 --- a/src/evaluator.ts +++ b/src/evaluator.ts @@ -90,7 +90,7 @@ export class Evaluator { return false; } - warnBucketByAttributeNotFound(bb, property?.toString(), this.log); + warnBucketByAttributeNotFound(bucketBy, property?.toString(), this.log); } const bucketId = this.getNormalizedNumber(bb, property); return percentage > 0 && bucketId <= percentage;