From af7f130ee8c124e2b0f0db473751645e9d115d72 Mon Sep 17 00:00:00 2001 From: Nicolas Zelaya Date: Fri, 3 Nov 2023 12:35:14 -0300 Subject: [PATCH 01/45] Fixing keybuilder to match spec --- src/storages/KeyBuilder.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storages/KeyBuilder.ts b/src/storages/KeyBuilder.ts index dc9581b5..c3124ae9 100644 --- a/src/storages/KeyBuilder.ts +++ b/src/storages/KeyBuilder.ts @@ -21,7 +21,7 @@ export class KeyBuilder { } buildFlagSetKey(flagSet: string) { - return `${this.prefix}.flagset.${flagSet}`; + return `${this.prefix}.flagSet.${flagSet}`; } buildSplitKey(splitName: string) { From 5912dc898434f1258030fbee85a451489a04b3ca Mon Sep 17 00:00:00 2001 From: Nicolas Zelaya Date: Fri, 3 Nov 2023 12:36:35 -0300 Subject: [PATCH 02/45] Implement getNamesByFlagSets for Redis --- src/storages/inRedis/SplitsCacheInRedis.ts | 28 ++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/src/storages/inRedis/SplitsCacheInRedis.ts b/src/storages/inRedis/SplitsCacheInRedis.ts index 310c014d..f83ee21a 100644 --- a/src/storages/inRedis/SplitsCacheInRedis.ts +++ b/src/storages/inRedis/SplitsCacheInRedis.ts @@ -10,7 +10,7 @@ import { ISet, _Set } from '../../utils/lang/sets'; /** * Discard errors for an answer of multiple operations. */ -function processPipelineAnswer(results: Array<[Error | null, string]>): string[] { +function processPipelineAnswer(results: Array<[Error | null, string]>): (string | string[])[] { return results.reduce((accum: string[], errValuePair: [Error | null, string]) => { if (errValuePair[0] === null) accum.push(errValuePair[1]); return accum; @@ -195,9 +195,29 @@ export class SplitsCacheInRedis extends AbstractSplitsCacheAsync { * or rejected if wrapper operation fails. * @todo this is a no-op method to be implemented */ - getNamesByFlagSets(): Promise> { - // eslint-disable-next-line @typescript-eslint/no-unused-vars - return new Promise(flagSets => new _Set([])); + getNamesByFlagSets(flagSets: string[]): Promise> { + const toReturn: ISet = new _Set([]); + const listOfKeys: string[] = []; + + flagSets && flagSets.forEach(flagSet => { + listOfKeys.push(this.keys.buildFlagSetKey(flagSet)); + }); + + if (listOfKeys.length > 0) { + + return this.redis.pipeline(listOfKeys.map(k => ['smembers', k])).exec() + .then(processPipelineAnswer) + .then((setsRaw) => { + this.log.error(setsRaw); + setsRaw.forEach((setContent) => { + (setContent as string[]).forEach(flagName => toReturn.add(flagName)); + }); + + return toReturn; + }); + } else { + return new Promise(() => toReturn); + } } /** From 6268360ab044e250665b89bead81b2ca58d0819c Mon Sep 17 00:00:00 2001 From: Nicolas Zelaya Date: Mon, 6 Nov 2023 09:19:35 -0300 Subject: [PATCH 03/45] Handling pipeline method properly in RedisAdapter --- src/storages/inRedis/RedisAdapter.ts | 71 +++++++++++++++++++--------- 1 file changed, 48 insertions(+), 23 deletions(-) diff --git a/src/storages/inRedis/RedisAdapter.ts b/src/storages/inRedis/RedisAdapter.ts index 7beb2988..423566f5 100644 --- a/src/storages/inRedis/RedisAdapter.ts +++ b/src/storages/inRedis/RedisAdapter.ts @@ -8,7 +8,8 @@ import { timeout } from '../../utils/promise/timeout'; const LOG_PREFIX = 'storage:redis-adapter: '; // If we ever decide to fully wrap every method, there's a Commander.getBuiltinCommands from ioredis. -const METHODS_TO_PROMISE_WRAP = ['set', 'exec', 'del', 'get', 'keys', 'sadd', 'srem', 'sismember', 'smembers', 'incr', 'rpush', 'pipeline', 'expire', 'mget', 'lrange', 'ltrim', 'hset']; +const METHODS_TO_PROMISE_WRAP = ['set', 'exec', 'del', 'get', 'keys', 'sadd', 'srem', 'sismember', 'smembers', 'incr', 'rpush', 'expire', 'mget', 'lrange', 'ltrim', 'hset']; +const METHODS_TO_PROMISE_WRAP_EXEC = ['pipeline']; // Not part of the settings since it'll vary on each storage. We should be removing storage specific logic from elsewhere. const DEFAULT_OPTIONS = { @@ -56,6 +57,7 @@ export class RedisAdapter extends ioredis { this.once('ready', () => { const commandsCount = this._notReadyCommandsQueue ? this._notReadyCommandsQueue.length : 0; this.log.info(LOG_PREFIX + `Redis connection established. Queued commands: ${commandsCount}.`); + this._notReadyCommandsQueue && this._notReadyCommandsQueue.forEach(queued => { this.log.info(LOG_PREFIX + `Executing queued ${queued.name} command.`); queued.command().then(queued.resolve).catch(queued.reject); @@ -71,49 +73,72 @@ export class RedisAdapter extends ioredis { _setTimeoutWrappers() { const instance: Record = this; - METHODS_TO_PROMISE_WRAP.forEach(method => { - const originalMethod = instance[method]; + const wrapWithQueueOrExecute = (method: string, commandWrapper: Function) => { + if (instance._notReadyCommandsQueue) { + return new Promise((resolve, reject) => { + instance._notReadyCommandsQueue.unshift({ + resolve, + reject, + command: commandWrapper, + name: method.toUpperCase() + }); + }); + } else { + return commandWrapper(); + } + }; - instance[method] = function () { + const wrapCommand = (originalMethod: Function, methodName: string) => { + return function () { const params = arguments; - function commandWrapper() { - instance.log.debug(LOG_PREFIX + `Executing ${method}.`); - // Return original method - const result = originalMethod.apply(instance, params); + const commandWrapper = () => { + instance.log.debug(`${LOG_PREFIX} Executing ${methodName}.`); + const result = originalMethod.apply(this, params); if (thenable(result)) { // For handling pending commands on disconnect, add to the set and remove once finished. // On sync commands there's no need, only thenables. instance._runningCommands.add(result); - const cleanUpRunningCommandsCb = function () { + const cleanUpRunningCommandsCb = () => { instance._runningCommands.delete(result); }; // Both success and error remove from queue. result.then(cleanUpRunningCommandsCb, cleanUpRunningCommandsCb); return timeout(instance._options.operationTimeout, result).catch(err => { - instance.log.error(LOG_PREFIX + `${method} operation threw an error or exceeded configured timeout of ${instance._options.operationTimeout}ms. Message: ${err}`); + instance.log.error(`${LOG_PREFIX}${methodName} operation threw an error or exceeded configured timeout of ${instance._options.operationTimeout}ms. Message: ${err}`); // Handling is not the adapter responsibility. throw err; }); } return result; - } + }; - if (instance._notReadyCommandsQueue) { - return new Promise((res, rej) => { - instance._notReadyCommandsQueue.unshift({ - resolve: res, - reject: rej, - command: commandWrapper, - name: method.toUpperCase() - }); - }); - } else { - return commandWrapper(); - } + return wrapWithQueueOrExecute(methodName, commandWrapper); + }; + }; + + // Wrap regular async methods to track timeouts and queue when Redis is not yet executing commands. + METHODS_TO_PROMISE_WRAP.forEach(methodName => { + const originalFn = instance[methodName]; + instance[methodName] = wrapCommand(originalFn, methodName); + }); + + // Special handling for pipeline~like methods. We need to wrap the async trigger, which is exec, but return the Pipeline right away. + METHODS_TO_PROMISE_WRAP_EXEC.forEach(methodName => { + const originalFn = instance[methodName]; + // "First level wrapper" to handle the sync execution and wrap async, queueing later if applicable. + instance[methodName] = function () { + instance.log.debug(`${LOG_PREFIX} Creating ${methodName}.`); + + const res = originalFn.apply(instance, arguments); + const originalExec = res.exec; + + res.exec = wrapCommand(originalExec, methodName + '.exec').bind(res); + + return res; }; }); } From 965df61ed2df298f27325f4d553e7d2de614f531 Mon Sep 17 00:00:00 2001 From: Nicolas Zelaya Date: Mon, 6 Nov 2023 14:39:32 -0300 Subject: [PATCH 04/45] RedisAdapter typing shadowed this --- src/storages/inRedis/RedisAdapter.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/storages/inRedis/RedisAdapter.ts b/src/storages/inRedis/RedisAdapter.ts index 423566f5..5a616c11 100644 --- a/src/storages/inRedis/RedisAdapter.ts +++ b/src/storages/inRedis/RedisAdapter.ts @@ -1,4 +1,4 @@ -import ioredis from 'ioredis'; +import ioredis, { Pipeline } from 'ioredis'; import { ILogger } from '../../logger/types'; import { merge, isString } from '../../utils/lang'; import { _Set, setToArray, ISet } from '../../utils/lang/sets'; @@ -89,12 +89,15 @@ export class RedisAdapter extends ioredis { }; const wrapCommand = (originalMethod: Function, methodName: string) => { + // The value of "this" in this function should be the instance actually executing the method. It might be the instance referred (the base one) + // or it can be the instance of a Pipeline object. return function () { const params = arguments; + const caller: (Pipeline | RedisAdapter) = this; // Need to change this crap to have TypeScript stop complaining const commandWrapper = () => { instance.log.debug(`${LOG_PREFIX} Executing ${methodName}.`); - const result = originalMethod.apply(this, params); + const result = originalMethod.apply(caller, params); if (thenable(result)) { // For handling pending commands on disconnect, add to the set and remove once finished. From 8057a7c0f8a3b26184427464a9f7a04465a16489 Mon Sep 17 00:00:00 2001 From: Nicolas Zelaya Date: Tue, 7 Nov 2023 11:13:45 -0300 Subject: [PATCH 05/45] fix after merge --- src/storages/inRedis/SplitsCacheInRedis.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storages/inRedis/SplitsCacheInRedis.ts b/src/storages/inRedis/SplitsCacheInRedis.ts index 83c06472..06f61074 100644 --- a/src/storages/inRedis/SplitsCacheInRedis.ts +++ b/src/storages/inRedis/SplitsCacheInRedis.ts @@ -5,7 +5,7 @@ import { ILogger } from '../../logger/types'; import { LOG_PREFIX } from './constants'; import { ISplit } from '../../dtos/types'; import { AbstractSplitsCacheAsync } from '../AbstractSplitsCacheAsync'; -import { ISet } from '../../utils/lang/sets'; +import { _Set, ISet } from '../../utils/lang/sets'; /** * Discard errors for an answer of multiple operations. From 03d884a5277ea88b4b1fdb19c575f53cab7e7f7d Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Thu, 23 Nov 2023 11:50:30 -0300 Subject: [PATCH 06/45] Implement SplitsCachePluggable::getNamesByFlagSets method --- src/logger/messages/warn.ts | 2 +- src/storages/inMemory/SplitsCacheInMemory.ts | 1 - .../pluggable/SplitsCachePluggable.ts | 19 +++++++++++++------ 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/logger/messages/warn.ts b/src/logger/messages/warn.ts index 8c6f60ce..fbce6805 100644 --- a/src/logger/messages/warn.ts +++ b/src/logger/messages/warn.ts @@ -24,7 +24,7 @@ export const codesWarn: [number, string][] = codesError.concat([ [c.WARN_NOT_EXISTENT_SPLIT, '%s: feature flag "%s" does not exist in this environment. Please double check what feature flags exist in the Split user interface.'], [c.WARN_LOWERCASE_TRAFFIC_TYPE, '%s: traffic_type_name should be all lowercase - converting string to lowercase.'], [c.WARN_NOT_EXISTENT_TT, '%s: traffic type "%s" does not have any corresponding feature flag in this environment, make sure you\'re tracking your events to a valid traffic type defined in the Split user interface.'], - [c.WARN_FLAGSET_NOT_CONFIGURED, '%s: : you passed %s wich is not part of the configured FlagSetsFilter, ignoring Flag Set.'], + [c.WARN_FLAGSET_NOT_CONFIGURED, '%s: you passed %s which is not part of the configured FlagSetsFilter, ignoring Flag Set.'], // initialization / settings validation [c.WARN_INTEGRATION_INVALID, c.LOG_PREFIX_SETTINGS+': %s integration item(s) at settings is invalid. %s'], [c.WARN_SPLITS_FILTER_INVALID, c.LOG_PREFIX_SETTINGS+': feature flag filter at position %s is invalid. It must be an object with a valid filter type ("bySet", "byName" or "byPrefix") and a list of "values".'], diff --git a/src/storages/inMemory/SplitsCacheInMemory.ts b/src/storages/inMemory/SplitsCacheInMemory.ts index 8cb45aef..6c5ecb36 100644 --- a/src/storages/inMemory/SplitsCacheInMemory.ts +++ b/src/storages/inMemory/SplitsCacheInMemory.ts @@ -114,7 +114,6 @@ export class SplitsCacheInMemory extends AbstractSplitsCacheSync { } }); return toReturn; - } private addToFlagSets(featureFlag: ISplit) { diff --git a/src/storages/pluggable/SplitsCachePluggable.ts b/src/storages/pluggable/SplitsCachePluggable.ts index 786fb8a5..76670951 100644 --- a/src/storages/pluggable/SplitsCachePluggable.ts +++ b/src/storages/pluggable/SplitsCachePluggable.ts @@ -5,7 +5,7 @@ import { ILogger } from '../../logger/types'; import { ISplit } from '../../dtos/types'; import { LOG_PREFIX } from './constants'; import { AbstractSplitsCacheAsync } from '../AbstractSplitsCacheAsync'; -import { ISet } from '../../utils/lang/sets'; +import { ISet, _Set } from '../../utils/lang/sets'; /** * ISplitsCacheAsync implementation for pluggable storages. @@ -158,12 +158,19 @@ export class SplitsCachePluggable extends AbstractSplitsCacheAsync { /** * Get list of split names related to a given flag set names list. * The returned promise is resolved with the list of split names, - * or rejected if wrapper operation fails. - * @todo this is a no-op method to be implemented + * or rejected if any wrapper operation fails. */ - getNamesByFlagSets(): Promise> { - this.log.error(LOG_PREFIX + 'ByFlagSet/s evaluations are not supported with pluggable storage yet.'); - return Promise.reject(); + getNamesByFlagSets(flagSets: string[]): Promise> { + return Promise.all(flagSets.map(flagSet => { + const flagSetKey = this.keys.buildFlagSetKey(flagSet); + return this.wrapper.getItems(flagSetKey); + })).then(namesByFlagSets => { + const featureFlagNames = new _Set(); + namesByFlagSets.forEach(names => { + names.forEach(name => featureFlagNames.add(name)); + }); + return featureFlagNames; + }); } /** From 5ace458517d07aa6dd2be6f5b8b13ee132d42ea6 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Thu, 23 Nov 2023 15:40:48 -0300 Subject: [PATCH 07/45] Implement AddToFlagSets and RemoveFromFlagSets in SplitsCachePluggable --- .../pluggable/SplitsCachePluggable.ts | 29 ++++++++++++++++--- src/storages/pluggable/index.ts | 2 +- src/storages/types.ts | 4 +-- src/utils/lang/sets.ts | 10 ++++++- 4 files changed, 37 insertions(+), 8 deletions(-) diff --git a/src/storages/pluggable/SplitsCachePluggable.ts b/src/storages/pluggable/SplitsCachePluggable.ts index 76670951..8aa7da58 100644 --- a/src/storages/pluggable/SplitsCachePluggable.ts +++ b/src/storages/pluggable/SplitsCachePluggable.ts @@ -5,7 +5,7 @@ import { ILogger } from '../../logger/types'; import { ISplit } from '../../dtos/types'; import { LOG_PREFIX } from './constants'; import { AbstractSplitsCacheAsync } from '../AbstractSplitsCacheAsync'; -import { ISet, _Set } from '../../utils/lang/sets'; +import { ISet, _Set, returnListDifference } from '../../utils/lang/sets'; /** * ISplitsCacheAsync implementation for pluggable storages. @@ -15,6 +15,7 @@ export class SplitsCachePluggable extends AbstractSplitsCacheAsync { private readonly log: ILogger; private readonly keys: KeyBuilder; private readonly wrapper: IPluggableStorageWrapper; + private readonly flagSetsFilter: string[]; /** * Create a SplitsCache that uses a storage wrapper. @@ -22,11 +23,12 @@ export class SplitsCachePluggable extends AbstractSplitsCacheAsync { * @param keys Key builder. * @param wrapper Adapted wrapper storage. */ - constructor(log: ILogger, keys: KeyBuilder, wrapper: IPluggableStorageWrapper) { + constructor(log: ILogger, keys: KeyBuilder, wrapper: IPluggableStorageWrapper, flagSetsFilter: string[] = []) { super(); this.log = log; this.keys = keys; this.wrapper = wrapper; + this.flagSetsFilter = flagSetsFilter; } private _decrementCounts(split: ISplit) { @@ -41,6 +43,24 @@ export class SplitsCachePluggable extends AbstractSplitsCacheAsync { return this.wrapper.incr(ttKey); } + private _updateFlagSets(name: string, flagSetsOfRemovedFlag?: string[], flagSetsOfAddedFlag?: string[]) { + const removeFromFlagSets = returnListDifference(flagSetsOfRemovedFlag, flagSetsOfAddedFlag); + + let addToFlagSets = returnListDifference(flagSetsOfAddedFlag, flagSetsOfRemovedFlag); + if (this.flagSetsFilter.length > 0) { + addToFlagSets = addToFlagSets.filter(flagSet => { + return this.flagSetsFilter.some(filterFlagSet => filterFlagSet === flagSet); + }); + } + + const items = [name]; + + return Promise.all([ + ...removeFromFlagSets.map(flagSetName => this.wrapper.removeItems(this.keys.buildFlagSetKey(flagSetName), items)), + ...addToFlagSets.map(flagSetName => this.wrapper.addItems(this.keys.buildFlagSetKey(flagSetName), items)) + ]); + } + /** * Add a given split. * The returned promise is resolved when the operation success @@ -67,7 +87,7 @@ export class SplitsCachePluggable extends AbstractSplitsCacheAsync { return this._incrementCounts(split).then(() => { if (parsedPreviousSplit) return this._decrementCounts(parsedPreviousSplit); }); - }); + }).then(() => this._updateFlagSets(name, parsedPreviousSplit && parsedPreviousSplit.sets, split.sets)); }).then(() => true); } @@ -88,8 +108,9 @@ export class SplitsCachePluggable extends AbstractSplitsCacheAsync { removeSplit(name: string) { return this.getSplit(name).then((split) => { if (split) { - this._decrementCounts(split); + return this._decrementCounts(split).then(() => this._updateFlagSets(name, split.sets)); } + }).then(() => { return this.wrapper.del(this.keys.buildSplitKey(name)); }); } diff --git a/src/storages/pluggable/index.ts b/src/storages/pluggable/index.ts index c1516f28..93442b73 100644 --- a/src/storages/pluggable/index.ts +++ b/src/storages/pluggable/index.ts @@ -105,7 +105,7 @@ export function PluggableStorage(options: PluggableStorageOptions): IStorageAsyn }); return { - splits: new SplitsCachePluggable(log, keys, wrapper), + splits: new SplitsCachePluggable(log, keys, wrapper, settings.sync.__splitFiltersValidation.groupedFilters.bySet), segments: new SegmentsCachePluggable(log, keys, wrapper), impressions: isPartialConsumer ? new ImpressionsCacheInMemory(impressionsQueueSize) : new ImpressionsCachePluggable(log, keys.buildImpressionsKey(), wrapper, metadata), impressionCounts: impressionCountsCache, diff --git a/src/storages/types.ts b/src/storages/types.ts index 30832f5e..8f45f4b2 100644 --- a/src/storages/types.ts +++ b/src/storages/types.ts @@ -44,10 +44,10 @@ export interface IPluggableStorageWrapper { * * @function del * @param {string} key Item to delete - * @returns {Promise} A promise that resolves if the operation success, whether the key existed and was removed or it didn't exist. + * @returns {Promise} A promise that resolves if the operation success, whether the key existed and was removed (resolves with true) or it didn't exist (resolves with false). * The promise rejects if the operation fails, for example, if there is a connection error. */ - del: (key: string) => Promise + del: (key: string) => Promise /** * Returns all keys matching the given prefix. * diff --git a/src/utils/lang/sets.ts b/src/utils/lang/sets.ts index f89fc29f..bda2c612 100644 --- a/src/utils/lang/sets.ts +++ b/src/utils/lang/sets.ts @@ -114,8 +114,16 @@ export const _Set = __getSetConstructor(); export function returnSetsUnion(set: ISet, set2: ISet): ISet { const result = new _Set(setToArray(set)); - set2.forEach( value => { + set2.forEach(value => { result.add(value); }); return result; } + +export function returnListDifference(list: T[] = [], list2: T[] = []): T[] { + const result = new _Set(list); + list2.forEach(item => { + result.delete(item); + }); + return setToArray(result); +} From 7eb99a2c5ebe9c7f282c326e752cbe616bc6124f Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Thu, 23 Nov 2023 16:30:09 -0300 Subject: [PATCH 08/45] Add feature flag filters validation for consumer and partial consumer modes --- src/logger/constants.ts | 1 + src/logger/messages/warn.ts | 13 ++--- .../__tests__/splitFilters.spec.ts | 51 ++++++++++--------- src/utils/settingsValidation/index.ts | 2 +- src/utils/settingsValidation/splitFilters.ts | 19 ++++--- 5 files changed, 50 insertions(+), 36 deletions(-) diff --git a/src/logger/constants.ts b/src/logger/constants.ts index 18c86f9f..961a1a98 100644 --- a/src/logger/constants.ts +++ b/src/logger/constants.ts @@ -91,6 +91,7 @@ export const WARN_NOT_EXISTENT_SPLIT = 215; export const WARN_LOWERCASE_TRAFFIC_TYPE = 216; export const WARN_NOT_EXISTENT_TT = 217; export const WARN_INTEGRATION_INVALID = 218; +export const WARN_SPLITS_FILTER_IGNORED = 219; export const WARN_SPLITS_FILTER_INVALID = 220; export const WARN_SPLITS_FILTER_EMPTY = 221; export const WARN_SDK_KEY = 222; diff --git a/src/logger/messages/warn.ts b/src/logger/messages/warn.ts index 8c6f60ce..db555e78 100644 --- a/src/logger/messages/warn.ts +++ b/src/logger/messages/warn.ts @@ -26,13 +26,14 @@ export const codesWarn: [number, string][] = codesError.concat([ [c.WARN_NOT_EXISTENT_TT, '%s: traffic type "%s" does not have any corresponding feature flag in this environment, make sure you\'re tracking your events to a valid traffic type defined in the Split user interface.'], [c.WARN_FLAGSET_NOT_CONFIGURED, '%s: : you passed %s wich is not part of the configured FlagSetsFilter, ignoring Flag Set.'], // initialization / settings validation - [c.WARN_INTEGRATION_INVALID, c.LOG_PREFIX_SETTINGS+': %s integration item(s) at settings is invalid. %s'], - [c.WARN_SPLITS_FILTER_INVALID, c.LOG_PREFIX_SETTINGS+': feature flag filter at position %s is invalid. It must be an object with a valid filter type ("bySet", "byName" or "byPrefix") and a list of "values".'], - [c.WARN_SPLITS_FILTER_EMPTY, c.LOG_PREFIX_SETTINGS+': feature flag filter configuration must be a non-empty array of filter objects.'], - [c.WARN_SDK_KEY, c.LOG_PREFIX_SETTINGS+': You already have %s. We recommend keeping only one instance of the factory at all times (Singleton pattern) and reusing it throughout your application'], + [c.WARN_INTEGRATION_INVALID, c.LOG_PREFIX_SETTINGS + ': %s integration item(s) at settings is invalid. %s'], + [c.WARN_SPLITS_FILTER_IGNORED, c.LOG_PREFIX_SETTINGS + ': feature flag filters are not applicable for Consumer modes where the SDK does not keep rollout data in sync. Filters were discarded'], + [c.WARN_SPLITS_FILTER_INVALID, c.LOG_PREFIX_SETTINGS + ': feature flag filter at position %s is invalid. It must be an object with a valid filter type ("bySet", "byName" or "byPrefix") and a list of "values".'], + [c.WARN_SPLITS_FILTER_EMPTY, c.LOG_PREFIX_SETTINGS + ': feature flag filter configuration must be a non-empty array of filter objects.'], + [c.WARN_SDK_KEY, c.LOG_PREFIX_SETTINGS + ': You already have %s. We recommend keeping only one instance of the factory at all times (Singleton pattern) and reusing it throughout your application'], [c.STREAMING_PARSING_MY_SEGMENTS_UPDATE_V2, c.LOG_PREFIX_SYNC_STREAMING + 'Fetching MySegments due to an error processing %s notification: %s'], [c.STREAMING_PARSING_SPLIT_UPDATE, c.LOG_PREFIX_SYNC_STREAMING + 'Fetching SplitChanges due to an error processing SPLIT_UPDATE notification: %s'], - [c.WARN_SPLITS_FILTER_INVALID_SET, c.LOG_PREFIX_SETTINGS+': you passed %s, flag set must adhere to the regular expressions %s. This means a flag set must start with a letter or number, be in lowercase, alphanumeric and have a max length of 50 characters. %s was discarded.'], - [c.WARN_SPLITS_FILTER_LOWERCASE_SET, c.LOG_PREFIX_SETTINGS+': flag set %s should be all lowercase - converting string to lowercase.'], + [c.WARN_SPLITS_FILTER_INVALID_SET, c.LOG_PREFIX_SETTINGS + ': you passed %s, flag set must adhere to the regular expressions %s. This means a flag set must start with a letter or number, be in lowercase, alphanumeric and have a max length of 50 characters. %s was discarded.'], + [c.WARN_SPLITS_FILTER_LOWERCASE_SET, c.LOG_PREFIX_SETTINGS + ': flag set %s should be all lowercase - converting string to lowercase.'], ]); diff --git a/src/utils/settingsValidation/__tests__/splitFilters.spec.ts b/src/utils/settingsValidation/__tests__/splitFilters.spec.ts index 5fe5dfb2..553c2604 100644 --- a/src/utils/settingsValidation/__tests__/splitFilters.spec.ts +++ b/src/utils/settingsValidation/__tests__/splitFilters.spec.ts @@ -1,11 +1,12 @@ import { loggerMock } from '../../../logger/__tests__/sdkLogger.mock'; +import { STANDALONE_MODE, CONSUMER_MODE, CONSUMER_PARTIAL_MODE, LOCALHOST_MODE, PRODUCER_MODE } from '../../constants'; // Split filter and QueryStrings examples import { splitFilters, queryStrings, groupedFilters } from '../../../__tests__/mocks/fetchSpecificSplits'; // Test target import { flagSetsAreValid, validateSplitFilters } from '../splitFilters'; -import { SETTINGS_SPLITS_FILTER, ERROR_INVALID, ERROR_EMPTY_ARRAY, ERROR_SETS_FILTER_EXCLUSIVE, WARN_SPLITS_FILTER_INVALID, WARN_SPLITS_FILTER_EMPTY, WARN_TRIMMING, WARN_SPLITS_FILTER_INVALID_SET, WARN_SPLITS_FILTER_LOWERCASE_SET, WARN_FLAGSET_NOT_CONFIGURED } from '../../../logger/constants'; +import { SETTINGS_SPLITS_FILTER, ERROR_INVALID, ERROR_EMPTY_ARRAY, ERROR_SETS_FILTER_EXCLUSIVE, WARN_SPLITS_FILTER_INVALID, WARN_SPLITS_FILTER_EMPTY, WARN_TRIMMING, WARN_SPLITS_FILTER_INVALID_SET, WARN_SPLITS_FILTER_LOWERCASE_SET, WARN_FLAGSET_NOT_CONFIGURED, WARN_SPLITS_FILTER_IGNORED } from '../../../logger/constants'; describe('validateSplitFilters', () => { @@ -28,17 +29,21 @@ describe('validateSplitFilters', () => { afterEach(() => { loggerMock.mockClear(); }); - test('Returns default output with empty values if `splitFilters` is an invalid object or `mode` is not \'standalone\'', () => { + test('Returns default output with empty values if `splitFilters` is an invalid object or `mode` is \'consumer\' or \'consumer_partial\'', () => { - expect(validateSplitFilters(loggerMock, undefined)).toEqual(defaultOutput); // splitFilters ignored if not a non-empty array - expect(validateSplitFilters(loggerMock, null)).toEqual(defaultOutput); // splitFilters ignored if not a non-empty array + expect(validateSplitFilters(loggerMock, undefined, STANDALONE_MODE)).toEqual(defaultOutput); // splitFilters ignored if not a non-empty array + expect(validateSplitFilters(loggerMock, null, STANDALONE_MODE)).toEqual(defaultOutput); // splitFilters ignored if not a non-empty array expect(loggerMock.warn).not.toBeCalled(); - expect(validateSplitFilters(loggerMock, true)).toEqual(defaultOutput); // splitFilters ignored if not a non-empty array - expect(validateSplitFilters(loggerMock, 15)).toEqual(defaultOutput); // splitFilters ignored if not a non-empty array - expect(validateSplitFilters(loggerMock, 'string')).toEqual(defaultOutput); // splitFilters ignored if not a non-empty array - expect(validateSplitFilters(loggerMock, [])).toEqual(defaultOutput); // splitFilters ignored if not a non-empty array - expect(loggerMock.warn.mock.calls).toEqual([[WARN_SPLITS_FILTER_EMPTY], [WARN_SPLITS_FILTER_EMPTY], [WARN_SPLITS_FILTER_EMPTY], [WARN_SPLITS_FILTER_EMPTY]]); + expect(validateSplitFilters(loggerMock, true, STANDALONE_MODE)).toEqual(defaultOutput); // splitFilters ignored if not a non-empty array + expect(validateSplitFilters(loggerMock, 15, STANDALONE_MODE)).toEqual(defaultOutput); // splitFilters ignored if not a non-empty array + expect(validateSplitFilters(loggerMock, 'string', STANDALONE_MODE)).toEqual(defaultOutput); // splitFilters ignored if not a non-empty array + expect(validateSplitFilters(loggerMock, [], STANDALONE_MODE)).toEqual(defaultOutput); // splitFilters ignored if not a non-empty array + + expect(validateSplitFilters(loggerMock, [{ type: 'byName', values: ['split_1'] }], CONSUMER_MODE)).toEqual(defaultOutput); // splitFilters ignored if in consumer mode + expect(validateSplitFilters(loggerMock, [{ type: 'byName', values: ['split_1'] }], CONSUMER_PARTIAL_MODE)).toEqual(defaultOutput); // splitFilters ignored if in partial consumer mode + + expect(loggerMock.warn.mock.calls).toEqual([[WARN_SPLITS_FILTER_EMPTY], [WARN_SPLITS_FILTER_EMPTY], [WARN_SPLITS_FILTER_EMPTY], [WARN_SPLITS_FILTER_EMPTY], [WARN_SPLITS_FILTER_IGNORED], [WARN_SPLITS_FILTER_IGNORED]]); expect(loggerMock.debug).not.toBeCalled(); expect(loggerMock.error).not.toBeCalled(); @@ -56,7 +61,7 @@ describe('validateSplitFilters', () => { queryString: null, groupedFilters: { bySet: [], byName: [], byPrefix: [] }, }; - expect(validateSplitFilters(loggerMock, splitFilters)).toEqual(output); // filters without values + expect(validateSplitFilters(loggerMock, splitFilters, STANDALONE_MODE)).toEqual(output); // filters without values expect(loggerMock.debug).toBeCalledWith(SETTINGS_SPLITS_FILTER, [null]); loggerMock.debug.mockClear(); @@ -66,7 +71,7 @@ describe('validateSplitFilters', () => { { type: null, values: [] }, { type: 'byName', values: [13] }); output.validFilters.push({ type: 'byName', values: [13] }); - expect(validateSplitFilters(loggerMock, splitFilters)).toEqual(output); // some filters are invalid + expect(validateSplitFilters(loggerMock, splitFilters, STANDALONE_MODE)).toEqual(output); // some filters are invalid expect(loggerMock.debug.mock.calls).toEqual([[SETTINGS_SPLITS_FILTER, [null]]]); expect(loggerMock.warn.mock.calls).toEqual([ [WARN_SPLITS_FILTER_INVALID, [4]], // invalid value of `type` property @@ -90,24 +95,24 @@ describe('validateSplitFilters', () => { queryString: queryStrings[i], groupedFilters: groupedFilters[i], }; - expect(validateSplitFilters(loggerMock, splitFilters[i])).toEqual(output); // splitFilters #${i} + expect(validateSplitFilters(loggerMock, splitFilters[i], STANDALONE_MODE)).toEqual(output); // splitFilters #${i} expect(loggerMock.debug).lastCalledWith(SETTINGS_SPLITS_FILTER, [queryStrings[i]]); } else { // tests where validateSplitFilters throws an exception - expect(() => validateSplitFilters(loggerMock, splitFilters[i])).toThrow(queryStrings[i]); + expect(() => validateSplitFilters(loggerMock, splitFilters[i], STANDALONE_MODE)).toThrow(queryStrings[i]); } } }); test('Validates flag set filters', () => { // extra spaces trimmed and sorted query output - expect(validateSplitFilters(loggerMock, splitFilters[6])).toEqual(getOutput(6)); // trim & sort + expect(validateSplitFilters(loggerMock, splitFilters[6], STANDALONE_MODE)).toEqual(getOutput(6)); // trim & sort expect(loggerMock.warn.mock.calls[0]).toEqual([WARN_TRIMMING, ['settings', 'bySet filter value', ' set_1']]); expect(loggerMock.warn.mock.calls[1]).toEqual([WARN_TRIMMING, ['settings', 'bySet filter value', 'set_3 ']]); expect(loggerMock.warn.mock.calls[2]).toEqual([WARN_TRIMMING, ['settings', 'bySet filter value', ' set_a ']]); expect(loggerMock.error.mock.calls[0]).toEqual([ERROR_SETS_FILTER_EXCLUSIVE]); - expect(validateSplitFilters(loggerMock, splitFilters[7])).toEqual(getOutput(7)); // lowercase and regexp + expect(validateSplitFilters(loggerMock, splitFilters[7], LOCALHOST_MODE)).toEqual(getOutput(7)); // lowercase and regexp expect(loggerMock.warn.mock.calls[3]).toEqual([WARN_SPLITS_FILTER_LOWERCASE_SET, ['seT_c']]); // lowercase expect(loggerMock.warn.mock.calls[4]).toEqual([WARN_SPLITS_FILTER_LOWERCASE_SET, ['set_B']]); // lowercase expect(loggerMock.warn.mock.calls[5]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set_ 1', regexp, 'set_ 1']]); // empty spaces @@ -116,7 +121,7 @@ describe('validateSplitFilters', () => { expect(loggerMock.warn.mock.calls[8]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set_1234567890_1234567890_234567890_1234567890_1234567890', regexp, 'set_1234567890_1234567890_234567890_1234567890_1234567890']]); // max of 50 characters expect(loggerMock.error.mock.calls[1]).toEqual([ERROR_SETS_FILTER_EXCLUSIVE]); - expect(validateSplitFilters(loggerMock, splitFilters[8])).toEqual(getOutput(8)); // lowercase and dedupe + expect(validateSplitFilters(loggerMock, splitFilters[8], PRODUCER_MODE)).toEqual(getOutput(8)); // lowercase and dedupe expect(loggerMock.warn.mock.calls[9]).toEqual([WARN_SPLITS_FILTER_LOWERCASE_SET, ['SET_2']]); // lowercase expect(loggerMock.warn.mock.calls[10]).toEqual([WARN_SPLITS_FILTER_LOWERCASE_SET, ['set_B']]); // lowercase expect(loggerMock.warn.mock.calls[11]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set_3!', regexp, 'set_3!']]); // special character @@ -147,21 +152,21 @@ describe('validateSplitFilters', () => { expect(loggerMock.warn.mock.calls[2]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, [longName, regexp, longName]]); // both set names invalid -> empty list & warn - expect(flagSetsAreValid(loggerMock, 'test_method', ['set*1','set*3'], flagSetsFilter)).toEqual([]); + expect(flagSetsAreValid(loggerMock, 'test_method', ['set*1', 'set*3'], flagSetsFilter)).toEqual([]); expect(loggerMock.warn.mock.calls[3]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set*1', regexp, 'set*1']]); expect(loggerMock.warn.mock.calls[4]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set*3', regexp, 'set*3']]); // only set_1 is valid => [set_1] & warn - expect(flagSetsAreValid(loggerMock, 'test_method', ['set_1','set*3'], flagSetsFilter)).toEqual(['set_1']); + expect(flagSetsAreValid(loggerMock, 'test_method', ['set_1', 'set*3'], flagSetsFilter)).toEqual(['set_1']); expect(loggerMock.warn.mock.calls[5]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set*3', regexp, 'set*3']]); // set_3 not included in configuration but set_1 included => [set_1] & warn - expect(flagSetsAreValid(loggerMock, 'test_method', ['set_1','set_3'], flagSetsFilter)).toEqual(['set_1']); + expect(flagSetsAreValid(loggerMock, 'test_method', ['set_1', 'set_3'], flagSetsFilter)).toEqual(['set_1']); expect(loggerMock.warn.mock.calls[6]).toEqual([WARN_FLAGSET_NOT_CONFIGURED, ['test_method', 'set_3']]); // set_3 not included in configuration => [] & warn expect(flagSetsAreValid(loggerMock, 'test_method', ['set_3'], flagSetsFilter)).toEqual([]); - expect(loggerMock.warn.mock.calls[7]).toEqual([WARN_FLAGSET_NOT_CONFIGURED, ['test_method','set_3']]); + expect(loggerMock.warn.mock.calls[7]).toEqual([WARN_FLAGSET_NOT_CONFIGURED, ['test_method', 'set_3']]); // empty config @@ -179,17 +184,17 @@ describe('validateSplitFilters', () => { expect(loggerMock.warn.mock.calls[10]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, [longName, regexp, longName]]); // both set names invalid -> empty list & warn - expect(flagSetsAreValid(loggerMock, 'test_method', ['set*1','set*3'], [])).toEqual([]); + expect(flagSetsAreValid(loggerMock, 'test_method', ['set*1', 'set*3'], [])).toEqual([]); expect(loggerMock.warn.mock.calls[11]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set*1', regexp, 'set*1']]); expect(loggerMock.warn.mock.calls[12]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set*3', regexp, 'set*3']]); // only set_1 is valid => [set_1] & warn - expect(flagSetsAreValid(loggerMock, 'test_method', ['set_1','set*3'], [])).toEqual(['set_1']); + expect(flagSetsAreValid(loggerMock, 'test_method', ['set_1', 'set*3'], [])).toEqual(['set_1']); expect(loggerMock.warn.mock.calls[13]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set*3', regexp, 'set*3']]); // any set should be returned if there isn't flag sets in filter expect(flagSetsAreValid(loggerMock, 'test_method', ['set_1'], [])).toEqual(['set_1']); - expect(flagSetsAreValid(loggerMock, 'test_method', ['set_1','set_2'], [])).toEqual(['set_1','set_2']); + expect(flagSetsAreValid(loggerMock, 'test_method', ['set_1', 'set_2'], [])).toEqual(['set_1', 'set_2']); expect(flagSetsAreValid(loggerMock, 'test_method', ['set_3'], [])).toEqual(['set_3']); }); diff --git a/src/utils/settingsValidation/index.ts b/src/utils/settingsValidation/index.ts index 8affa738..700ada43 100644 --- a/src/utils/settingsValidation/index.ts +++ b/src/utils/settingsValidation/index.ts @@ -202,7 +202,7 @@ export function settingsValidation(config: unknown, validationParams: ISettingsV } // validate the `splitFilters` settings and parse splits query - const splitFiltersValidation = validateSplitFilters(log, withDefaults.sync.splitFilters); + const splitFiltersValidation = validateSplitFilters(log, withDefaults.sync.splitFilters, withDefaults.mode); withDefaults.sync.splitFilters = splitFiltersValidation.validFilters; withDefaults.sync.__splitFiltersValidation = splitFiltersValidation; diff --git a/src/utils/settingsValidation/splitFilters.ts b/src/utils/settingsValidation/splitFilters.ts index 023c3fc1..a1d9e6eb 100644 --- a/src/utils/settingsValidation/splitFilters.ts +++ b/src/utils/settingsValidation/splitFilters.ts @@ -1,8 +1,9 @@ +import { CONSUMER_MODE, CONSUMER_PARTIAL_MODE } from '../constants'; import { validateSplits } from '../inputValidation/splits'; import { ISplitFiltersValidation } from '../../dtos/types'; import { SplitIO } from '../../types'; import { ILogger } from '../../logger/types'; -import { WARN_SPLITS_FILTER_EMPTY, WARN_SPLITS_FILTER_INVALID, SETTINGS_SPLITS_FILTER, LOG_PREFIX_SETTINGS, ERROR_SETS_FILTER_EXCLUSIVE, WARN_SPLITS_FILTER_LOWERCASE_SET, WARN_SPLITS_FILTER_INVALID_SET, WARN_FLAGSET_NOT_CONFIGURED } from '../../logger/constants'; +import { WARN_SPLITS_FILTER_IGNORED, WARN_SPLITS_FILTER_EMPTY, WARN_SPLITS_FILTER_INVALID, SETTINGS_SPLITS_FILTER, LOG_PREFIX_SETTINGS, ERROR_SETS_FILTER_EXCLUSIVE, WARN_SPLITS_FILTER_LOWERCASE_SET, WARN_SPLITS_FILTER_INVALID_SET, WARN_FLAGSET_NOT_CONFIGURED } from '../../logger/constants'; import { objectAssign } from '../lang/objectAssign'; import { find, uniq } from '../lang'; @@ -102,15 +103,15 @@ function queryStringBuilder(groupedFilters: Record { - if (CAPITAL_LETTERS_REGEX.test(flagSet)){ - log.warn(WARN_SPLITS_FILTER_LOWERCASE_SET,[flagSet]); + if (CAPITAL_LETTERS_REGEX.test(flagSet)) { + log.warn(WARN_SPLITS_FILTER_LOWERCASE_SET, [flagSet]); flagSet = flagSet.toLowerCase(); } return flagSet; }) .filter(flagSet => { - if (!VALID_FLAGSET_REGEX.test(flagSet)){ - log.warn(WARN_SPLITS_FILTER_INVALID_SET, [flagSet,VALID_FLAGSET_REGEX,flagSet]); + if (!VALID_FLAGSET_REGEX.test(flagSet)) { + log.warn(WARN_SPLITS_FILTER_INVALID_SET, [flagSet, VALID_FLAGSET_REGEX, flagSet]); return false; } if (typeof flagSet !== 'string') return false; @@ -128,6 +129,7 @@ function configuredFilter(validFilters: SplitIO.SplitFilter[], filterType: Split * * @param {ILogger} log logger * @param {any} maybeSplitFilters split filters configuration param provided by the user + * @param {string} mode settings mode * @returns it returns an object with the following properties: * - `validFilters`: the validated `splitFilters` configuration object defined by the user. * - `queryString`: the parsed split filter query. it is null if all filters are invalid or all values in filters are invalid. @@ -135,7 +137,7 @@ function configuredFilter(validFilters: SplitIO.SplitFilter[], filterType: Split * * @throws Error if the some of the grouped list of values per filter exceeds the max allowed length */ -export function validateSplitFilters(log: ILogger, maybeSplitFilters: any): ISplitFiltersValidation { +export function validateSplitFilters(log: ILogger, maybeSplitFilters: any, mode: string): ISplitFiltersValidation { // Validation result schema const res = { validFilters: [], @@ -145,6 +147,11 @@ export function validateSplitFilters(log: ILogger, maybeSplitFilters: any): ISpl // do nothing if `splitFilters` param is not a non-empty array or mode is not STANDALONE if (!maybeSplitFilters) return res; + // Warn depending on the mode + if (mode === CONSUMER_MODE || mode === CONSUMER_PARTIAL_MODE) { + log.warn(WARN_SPLITS_FILTER_IGNORED); + return res; + } // Check collection type if (!Array.isArray(maybeSplitFilters) || maybeSplitFilters.length === 0) { log.warn(WARN_SPLITS_FILTER_EMPTY); From b607eda8fb4a65613ae08b033ee562c266f59132 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Thu, 23 Nov 2023 21:43:01 -0300 Subject: [PATCH 09/45] Add unit tests --- src/storages/inMemory/SplitsCacheInMemory.ts | 4 +- .../pluggable/SplitsCachePluggable.ts | 6 +- .../__tests__/SplitsCachePluggable.spec.ts | 62 ++++++++++++++++++- src/storages/pluggable/index.ts | 2 +- 4 files changed, 67 insertions(+), 7 deletions(-) diff --git a/src/storages/inMemory/SplitsCacheInMemory.ts b/src/storages/inMemory/SplitsCacheInMemory.ts index 6c5ecb36..36a55fe1 100644 --- a/src/storages/inMemory/SplitsCacheInMemory.ts +++ b/src/storages/inMemory/SplitsCacheInMemory.ts @@ -16,9 +16,9 @@ export class SplitsCacheInMemory extends AbstractSplitsCacheSync { private splitsWithSegmentsCount: number = 0; private flagSetsCache: Record> = {}; - constructor(splitFiltersValidation: ISplitFiltersValidation = { queryString: null, groupedFilters: { bySet: [], byName: [], byPrefix: [] }, validFilters: [] }) { + constructor(splitFiltersValidation?: ISplitFiltersValidation) { super(); - this.flagSetsFilter = splitFiltersValidation.groupedFilters.bySet; + this.flagSetsFilter = splitFiltersValidation ? splitFiltersValidation.groupedFilters.bySet : []; } clear() { diff --git a/src/storages/pluggable/SplitsCachePluggable.ts b/src/storages/pluggable/SplitsCachePluggable.ts index 8aa7da58..8f355fb1 100644 --- a/src/storages/pluggable/SplitsCachePluggable.ts +++ b/src/storages/pluggable/SplitsCachePluggable.ts @@ -2,7 +2,7 @@ import { isFiniteNumber, isNaNNumber } from '../../utils/lang'; import { KeyBuilder } from '../KeyBuilder'; import { IPluggableStorageWrapper } from '../types'; import { ILogger } from '../../logger/types'; -import { ISplit } from '../../dtos/types'; +import { ISplit, ISplitFiltersValidation } from '../../dtos/types'; import { LOG_PREFIX } from './constants'; import { AbstractSplitsCacheAsync } from '../AbstractSplitsCacheAsync'; import { ISet, _Set, returnListDifference } from '../../utils/lang/sets'; @@ -23,12 +23,12 @@ export class SplitsCachePluggable extends AbstractSplitsCacheAsync { * @param keys Key builder. * @param wrapper Adapted wrapper storage. */ - constructor(log: ILogger, keys: KeyBuilder, wrapper: IPluggableStorageWrapper, flagSetsFilter: string[] = []) { + constructor(log: ILogger, keys: KeyBuilder, wrapper: IPluggableStorageWrapper, splitFiltersValidation?: ISplitFiltersValidation) { super(); this.log = log; this.keys = keys; this.wrapper = wrapper; - this.flagSetsFilter = flagSetsFilter; + this.flagSetsFilter = splitFiltersValidation ? splitFiltersValidation.groupedFilters.bySet : []; } private _decrementCounts(split: ISplit) { diff --git a/src/storages/pluggable/__tests__/SplitsCachePluggable.spec.ts b/src/storages/pluggable/__tests__/SplitsCachePluggable.spec.ts index 6cad1e2a..3d67e454 100644 --- a/src/storages/pluggable/__tests__/SplitsCachePluggable.spec.ts +++ b/src/storages/pluggable/__tests__/SplitsCachePluggable.spec.ts @@ -2,8 +2,9 @@ import { SplitsCachePluggable } from '../SplitsCachePluggable'; import { KeyBuilder } from '../../KeyBuilder'; import { loggerMock } from '../../../logger/__tests__/sdkLogger.mock'; import { wrapperMockFactory } from './wrapper.mock'; -import { splitWithUserTT, splitWithAccountTT } from '../../__tests__/testUtils'; +import { splitWithUserTT, splitWithAccountTT, featureFlagOne, featureFlagThree, featureFlagTwo, featureFlagWithEmptyFS, featureFlagWithoutFS } from '../../__tests__/testUtils'; import { ISplit } from '../../../dtos/types'; +import { _Set } from '../../../utils/lang/sets'; const keysBuilder = new KeyBuilder(); @@ -150,4 +151,63 @@ describe('SPLITS CACHE PLUGGABLE', () => { expect(await wrapper.getKeysByPrefix('SPLITIO')).toHaveLength(0); }); + test('flag set cache tests', async () => { + // @ts-ignore + const cache = new SplitsCachePluggable(loggerMock, keysBuilder, wrapperMockFactory(), { groupedFilters: { bySet: ['o', 'n', 'e', 'x'] } }); + const emptySet = new _Set([]); + + await cache.addSplits([ + [featureFlagOne.name, featureFlagOne], + [featureFlagTwo.name, featureFlagTwo], + [featureFlagThree.name, featureFlagThree], + ]); + await cache.addSplit(featureFlagWithEmptyFS.name, featureFlagWithEmptyFS); + + expect(await cache.getNamesByFlagSets(['o'])).toEqual(new _Set(['ff_one', 'ff_two'])); + expect(await cache.getNamesByFlagSets(['n'])).toEqual(new _Set(['ff_one'])); + expect(await cache.getNamesByFlagSets(['e'])).toEqual(new _Set(['ff_one', 'ff_three'])); + expect(await cache.getNamesByFlagSets(['t'])).toEqual(emptySet); // 't' not in filter + expect(await cache.getNamesByFlagSets(['o', 'n', 'e'])).toEqual(new _Set(['ff_one', 'ff_two', 'ff_three'])); + + await cache.addSplit(featureFlagOne.name, { ...featureFlagOne, sets: ['1'] }); + + expect(await cache.getNamesByFlagSets(['1'])).toEqual(emptySet); // '1' not in filter + expect(await cache.getNamesByFlagSets(['o'])).toEqual(new _Set(['ff_two'])); + expect(await cache.getNamesByFlagSets(['n'])).toEqual(emptySet); + + await cache.addSplit(featureFlagOne.name, { ...featureFlagOne, sets: ['x'] }); + expect(await cache.getNamesByFlagSets(['x'])).toEqual(new _Set(['ff_one'])); + expect(await cache.getNamesByFlagSets(['o', 'e', 'x'])).toEqual(new _Set(['ff_one', 'ff_two', 'ff_three'])); + + await cache.removeSplit(featureFlagOne.name); + expect(await cache.getNamesByFlagSets(['x'])).toEqual(emptySet); + + await cache.removeSplit(featureFlagOne.name); + expect(await cache.getNamesByFlagSets(['y'])).toEqual(emptySet); // 'y' not in filter + expect(await cache.getNamesByFlagSets([])).toEqual(emptySet); + + await cache.addSplit(featureFlagWithEmptyFS.name, featureFlagWithoutFS); + expect(await cache.getNamesByFlagSets([])).toEqual(emptySet); + }); + + // if FlagSets filter is not defined, it should store all FlagSets in memory. + test('flag set cache tests without filters', async () => { + const cacheWithoutFilters = new SplitsCachePluggable(loggerMock, keysBuilder, wrapperMockFactory()); + const emptySet = new _Set([]); + + await cacheWithoutFilters.addSplits([ + [featureFlagOne.name, featureFlagOne], + [featureFlagTwo.name, featureFlagTwo], + [featureFlagThree.name, featureFlagThree], + ]); + await cacheWithoutFilters.addSplit(featureFlagWithEmptyFS.name, featureFlagWithEmptyFS); + + expect(await cacheWithoutFilters.getNamesByFlagSets(['o'])).toEqual(new _Set(['ff_one', 'ff_two'])); + expect(await cacheWithoutFilters.getNamesByFlagSets(['n'])).toEqual(new _Set(['ff_one'])); + expect(await cacheWithoutFilters.getNamesByFlagSets(['e'])).toEqual(new _Set(['ff_one', 'ff_three'])); + expect(await cacheWithoutFilters.getNamesByFlagSets(['t'])).toEqual(new _Set(['ff_two', 'ff_three'])); + expect(await cacheWithoutFilters.getNamesByFlagSets(['y'])).toEqual(emptySet); + expect(await cacheWithoutFilters.getNamesByFlagSets(['o', 'n', 'e'])).toEqual(new _Set(['ff_one', 'ff_two', 'ff_three'])); + }); + }); diff --git a/src/storages/pluggable/index.ts b/src/storages/pluggable/index.ts index 93442b73..3e72d791 100644 --- a/src/storages/pluggable/index.ts +++ b/src/storages/pluggable/index.ts @@ -105,7 +105,7 @@ export function PluggableStorage(options: PluggableStorageOptions): IStorageAsyn }); return { - splits: new SplitsCachePluggable(log, keys, wrapper, settings.sync.__splitFiltersValidation.groupedFilters.bySet), + splits: new SplitsCachePluggable(log, keys, wrapper, settings.sync.__splitFiltersValidation), segments: new SegmentsCachePluggable(log, keys, wrapper), impressions: isPartialConsumer ? new ImpressionsCacheInMemory(impressionsQueueSize) : new ImpressionsCachePluggable(log, keys.buildImpressionsKey(), wrapper, metadata), impressionCounts: impressionCountsCache, From 80af8add8915e8446c2df93e6c3a6e4321025386 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Fri, 24 Nov 2023 11:43:16 -0300 Subject: [PATCH 10/45] Update wrong comment --- src/types.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/types.ts b/src/types.ts index b71f6e80..020c5909 100644 --- a/src/types.ts +++ b/src/types.ts @@ -197,8 +197,6 @@ interface ISharedSettings { * List of feature flag filters. These filters are used to fetch a subset of the feature flag definitions in your environment, in order to reduce the delay of the SDK to be ready. * This configuration is only meaningful when the SDK is working in "standalone" mode. * - * At the moment, only one type of feature flag filter is supported: by name. - * * Example: * `splitFilter: [ * { type: 'byName', values: ['my_feature_flag_1', 'my_feature_flag_2'] }, // will fetch feature flags named 'my_feature_flag_1' and 'my_feature_flag_2' From 5f4043526e9fc5c7c4c81c1cef750f469c0f9b0f Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Fri, 24 Nov 2023 14:01:33 -0300 Subject: [PATCH 11/45] Add warning when evaluating with flag sets that don't contain cached feature flags --- .../__tests__/evaluate-features.spec.ts | 109 +++++++++++------- src/evaluator/index.ts | 28 ++++- src/logger/constants.ts | 1 + src/logger/messages/warn.ts | 1 + src/sdkClient/client.ts | 2 +- src/storages/AbstractSplitsCacheAsync.ts | 2 +- src/storages/AbstractSplitsCacheSync.ts | 2 +- .../inLocalStorage/SplitsCacheInLocal.ts | 14 +-- .../__tests__/SplitsCacheInLocal.spec.ts | 44 +++---- src/storages/inMemory/SplitsCacheInMemory.ts | 15 +-- .../__tests__/SplitsCacheInMemory.spec.ts | 44 +++---- src/storages/inRedis/SplitsCacheInRedis.ts | 2 +- .../pluggable/SplitsCachePluggable.ts | 10 +- .../__tests__/SplitsCachePluggable.spec.ts | 40 +++---- src/storages/types.ts | 6 +- 15 files changed, 174 insertions(+), 146 deletions(-) diff --git a/src/evaluator/__tests__/evaluate-features.spec.ts b/src/evaluator/__tests__/evaluate-features.spec.ts index 3b81a37b..e3f1fa96 100644 --- a/src/evaluator/__tests__/evaluate-features.spec.ts +++ b/src/evaluator/__tests__/evaluate-features.spec.ts @@ -3,7 +3,7 @@ import { evaluateFeatures, evaluateFeaturesByFlagSets } from '../index'; import * as LabelsConstants from '../../utils/labels'; import { loggerMock } from '../../logger/__tests__/sdkLogger.mock'; import { _Set } from '../../utils/lang/sets'; -import { returnSetsUnion } from '../../utils/lang/sets'; +import { WARN_FLAGSET_WITHOUT_FLAGS } from '../../logger/constants'; const splitsMock = { regular: { 'changeNumber': 1487277320548, 'trafficAllocationSeed': 1667452163, 'trafficAllocation': 100, 'trafficTypeName': 'user', 'name': 'always-on', 'seed': 1684183541, 'configurations': {}, 'status': 'ACTIVE', 'killed': false, 'defaultTreatment': 'off', 'conditions': [{ 'conditionType': 'ROLLOUT', 'matcherGroup': { 'combiner': 'AND', 'matchers': [{ 'keySelector': { 'trafficType': 'user', 'attribute': '' }, 'matcherType': 'ALL_KEYS', 'negate': false, 'userDefinedSegmentMatcherData': { 'segmentName': '' }, 'unaryNumericMatcherData': { 'dataType': '', 'value': 0 }, 'whitelistMatcherData': { 'whitelist': null }, 'betweenMatcherData': { 'dataType': '', 'start': 0, 'end': 0 } }] }, 'partitions': [{ 'treatment': 'on', 'size': 100 }, { 'treatment': 'off', 'size': 0 }], 'label': 'in segment all' }] }, @@ -38,14 +38,7 @@ const mockStorage = { return splits; }, getNamesByFlagSets(flagSets) { - let toReturn = new _Set([]); - flagSets.forEach(flagset => { - const featureFlagNames = flagSetsMock[flagset]; - if (featureFlagNames) { - toReturn = returnSetsUnion(toReturn, featureFlagNames); - } - }); - return toReturn; + return flagSets.map(flagset => flagSetsMock[flagset] || new _Set()); } } }; @@ -123,7 +116,7 @@ test('EVALUATOR - Multiple evaluations at once / should return right labels, tre }); -test('EVALUATOR - Multiple evaluations at once by flag sets / should return right labels, treatments and configs if storage returns without errors.', async function () { +describe('EVALUATOR - Multiple evaluations at once by flag sets', () => { const expectedOutput = { config: { @@ -135,44 +128,76 @@ test('EVALUATOR - Multiple evaluations at once by flag sets / should return righ }, }; - const getResultsByFlagsets = (flagSets: string[]) => { + const getResultsByFlagsets = (flagSets: string[], storage = mockStorage) => { return evaluateFeaturesByFlagSets( loggerMock, 'fake-key', flagSets, null, - mockStorage, + storage, + 'method-name' ); }; - - - let multipleEvaluationAtOnceByFlagSets = await getResultsByFlagsets(['reg_and_config', 'arch_and_killed']); - - // assert evaluationWithConfig - expect(multipleEvaluationAtOnceByFlagSets['config']).toEqual(expectedOutput['config']); // If the split is retrieved successfully we should get the right evaluation result, label and config. - // @todo assert flag set not found - for input validations - - // assert regular - expect(multipleEvaluationAtOnceByFlagSets['regular']).toEqual({ ...expectedOutput['config'], config: null }); // If the split is retrieved successfully we should get the right evaluation result, label and config. If Split has no config it should have config equal null. - // assert killed - expect(multipleEvaluationAtOnceByFlagSets['killed']).toEqual({ ...expectedOutput['config'], treatment: 'off', config: null, label: LabelsConstants.SPLIT_KILLED }); - // 'If the split is retrieved but is killed, we should get the right evaluation result, label and config. - - // assert archived - expect(multipleEvaluationAtOnceByFlagSets['archived']).toEqual({ ...expectedOutput['config'], treatment: 'control', label: LabelsConstants.SPLIT_ARCHIVED, config: null }); - // If the split is retrieved but is archived, we should get the right evaluation result, label and config. - - // assert not_existent_split not in evaluation if it is not related to defined flag sets - expect(multipleEvaluationAtOnceByFlagSets['not_existent_split']).toEqual(undefined); - - multipleEvaluationAtOnceByFlagSets = await getResultsByFlagsets([]); - expect(multipleEvaluationAtOnceByFlagSets).toEqual({}); - - multipleEvaluationAtOnceByFlagSets = await getResultsByFlagsets(['reg_and_config']); - expect(multipleEvaluationAtOnceByFlagSets['config']).toEqual(expectedOutput['config']); - expect(multipleEvaluationAtOnceByFlagSets['regular']).toEqual({ ...expectedOutput['config'], config: null }); - expect(multipleEvaluationAtOnceByFlagSets['killed']).toEqual(undefined); - expect(multipleEvaluationAtOnceByFlagSets['archived']).toEqual(undefined); - + test('should return right labels, treatments and configs if storage returns without errors', async () => { + + let multipleEvaluationAtOnceByFlagSets = await getResultsByFlagsets(['reg_and_config', 'arch_and_killed']); + + // assert evaluationWithConfig + expect(multipleEvaluationAtOnceByFlagSets['config']).toEqual(expectedOutput['config']); // If the split is retrieved successfully we should get the right evaluation result, label and config. + // @todo assert flag set not found - for input validations + + // assert regular + expect(multipleEvaluationAtOnceByFlagSets['regular']).toEqual({ ...expectedOutput['config'], config: null }); // If the split is retrieved successfully we should get the right evaluation result, label and config. If Split has no config it should have config equal null. + // assert killed + expect(multipleEvaluationAtOnceByFlagSets['killed']).toEqual({ ...expectedOutput['config'], treatment: 'off', config: null, label: LabelsConstants.SPLIT_KILLED }); + // 'If the split is retrieved but is killed, we should get the right evaluation result, label and config. + + // assert archived + expect(multipleEvaluationAtOnceByFlagSets['archived']).toEqual({ ...expectedOutput['config'], treatment: 'control', label: LabelsConstants.SPLIT_ARCHIVED, config: null }); + // If the split is retrieved but is archived, we should get the right evaluation result, label and config. + + // assert not_existent_split not in evaluation if it is not related to defined flag sets + expect(multipleEvaluationAtOnceByFlagSets['not_existent_split']).toEqual(undefined); + + multipleEvaluationAtOnceByFlagSets = await getResultsByFlagsets([]); + expect(multipleEvaluationAtOnceByFlagSets).toEqual({}); + + multipleEvaluationAtOnceByFlagSets = await getResultsByFlagsets(['reg_and_config']); + expect(multipleEvaluationAtOnceByFlagSets['config']).toEqual(expectedOutput['config']); + expect(multipleEvaluationAtOnceByFlagSets['regular']).toEqual({ ...expectedOutput['config'], config: null }); + expect(multipleEvaluationAtOnceByFlagSets['killed']).toEqual(undefined); + expect(multipleEvaluationAtOnceByFlagSets['archived']).toEqual(undefined); + }); + + test('should log a warning if evaluating with flag sets that doesn\'t contain cached feature flags', async () => { + const getSplitsSpy = jest.spyOn(mockStorage.splits, 'getSplits'); + + // No flag set contains cached feature flags -> getSplits method is not called + expect(getResultsByFlagsets(['inexistent_set1', 'inexistent_set2'])).toEqual({}); + expect(getSplitsSpy).not.toHaveBeenCalled(); + expect(loggerMock.warn.mock.calls).toEqual([ + [WARN_FLAGSET_WITHOUT_FLAGS, ['method-name', 'inexistent_set1']], + [WARN_FLAGSET_WITHOUT_FLAGS, ['method-name', 'inexistent_set2']], + ]); + + // One flag set contains cached feature flags -> getSplits method is called + expect(getResultsByFlagsets(['inexistent_set3', 'reg_and_config'])).toEqual(getResultsByFlagsets(['reg_and_config'])); + expect(getSplitsSpy).toHaveBeenLastCalledWith(['regular', 'config']); + expect(loggerMock.warn).toHaveBeenLastCalledWith(WARN_FLAGSET_WITHOUT_FLAGS, ['method-name', 'inexistent_set3']); + + getSplitsSpy.mockRestore(); + loggerMock.warn.mockClear(); + + // Should support async storage too + expect(await getResultsByFlagsets(['inexistent_set1', 'inexistent_set2'], { + splits: { + getNamesByFlagSets(flagSets) { return Promise.resolve(flagSets.map(flagset => flagSetsMock[flagset] || new _Set())); } + } + })).toEqual({}); + expect(loggerMock.warn.mock.calls).toEqual([ + [WARN_FLAGSET_WITHOUT_FLAGS, ['method-name', 'inexistent_set1']], + [WARN_FLAGSET_WITHOUT_FLAGS, ['method-name', 'inexistent_set2']], + ]); + }); }); diff --git a/src/evaluator/index.ts b/src/evaluator/index.ts index 62f82e98..ffcc9321 100644 --- a/src/evaluator/index.ts +++ b/src/evaluator/index.ts @@ -7,7 +7,8 @@ import { IStorageAsync, IStorageSync } from '../storages/types'; import { IEvaluationResult } from './types'; import { SplitIO } from '../types'; import { ILogger } from '../logger/types'; -import { ISet, setToArray } from '../utils/lang/sets'; +import { ISet, setToArray, returnSetsUnion, _Set } from '../utils/lang/sets'; +import { WARN_FLAGSET_WITHOUT_FLAGS } from '../logger/constants'; const treatmentException = { treatment: CONTROL, @@ -94,8 +95,27 @@ export function evaluateFeaturesByFlagSets( flagSets: string[], attributes: SplitIO.Attributes | undefined, storage: IStorageSync | IStorageAsync, + method: string, ): MaybeThenable> { - let storedFlagNames: MaybeThenable>; + let storedFlagNames: MaybeThenable[]>; + + function evaluate( + featureFlagsByFlagSets: ISet[], + ) { + let featureFlags = new _Set(); + for (let i = 0; i < flagSets.length; i++) { + const featureFlagByFlagSet = featureFlagsByFlagSets[i]; + if (featureFlagByFlagSet.size) { + featureFlags = returnSetsUnion(featureFlags, featureFlagByFlagSet); + } else { + log.warn(WARN_FLAGSET_WITHOUT_FLAGS, [method, flagSets[i]]); + } + } + + return featureFlags.size ? + evaluateFeatures(log, key, setToArray(featureFlags), attributes, storage) : + {}; + } // get features by flag sets try { @@ -107,11 +127,11 @@ export function evaluateFeaturesByFlagSets( // evaluate related features return thenable(storedFlagNames) ? - storedFlagNames.then((splitNames) => evaluateFeatures(log, key, setToArray(splitNames), attributes, storage)) + storedFlagNames.then((storedFlagNames) => evaluate(storedFlagNames)) .catch(() => { return {}; }) : - evaluateFeatures(log, key, setToArray(storedFlagNames), attributes, storage); + evaluate(storedFlagNames); } function getEvaluation( diff --git a/src/logger/constants.ts b/src/logger/constants.ts index 961a1a98..91e601fc 100644 --- a/src/logger/constants.ts +++ b/src/logger/constants.ts @@ -100,6 +100,7 @@ export const STREAMING_PARSING_SPLIT_UPDATE = 224; export const WARN_SPLITS_FILTER_INVALID_SET = 225; export const WARN_SPLITS_FILTER_LOWERCASE_SET = 226; export const WARN_FLAGSET_NOT_CONFIGURED = 227; +export const WARN_FLAGSET_WITHOUT_FLAGS = 228; export const ERROR_ENGINE_COMBINER_IFELSEIF = 300; export const ERROR_LOGLEVEL_INVALID = 301; diff --git a/src/logger/messages/warn.ts b/src/logger/messages/warn.ts index 2dafabdf..3feca12f 100644 --- a/src/logger/messages/warn.ts +++ b/src/logger/messages/warn.ts @@ -36,4 +36,5 @@ export const codesWarn: [number, string][] = codesError.concat([ [c.STREAMING_PARSING_SPLIT_UPDATE, c.LOG_PREFIX_SYNC_STREAMING + 'Fetching SplitChanges due to an error processing SPLIT_UPDATE notification: %s'], [c.WARN_SPLITS_FILTER_INVALID_SET, c.LOG_PREFIX_SETTINGS + ': you passed %s, flag set must adhere to the regular expressions %s. This means a flag set must start with a letter or number, be in lowercase, alphanumeric and have a max length of 50 characters. %s was discarded.'], [c.WARN_SPLITS_FILTER_LOWERCASE_SET, c.LOG_PREFIX_SETTINGS + ': flag set %s should be all lowercase - converting string to lowercase.'], + [c.WARN_FLAGSET_WITHOUT_FLAGS, '%s: you passed %s flag set that does not contain cached feature flag names. Please double check what flag sets are in use in the Split user interface.'], ]); diff --git a/src/sdkClient/client.ts b/src/sdkClient/client.ts index 31aa5f74..1c10da3a 100644 --- a/src/sdkClient/client.ts +++ b/src/sdkClient/client.ts @@ -99,7 +99,7 @@ export function clientFactory(params: ISdkFactoryContext): SplitIO.IClient | Spl }; const evaluations = readinessManager.isReady() || readinessManager.isReadyFromCache() ? - evaluateFeaturesByFlagSets(log, key, flagSetNames, attributes, storage) : + evaluateFeaturesByFlagSets(log, key, flagSetNames, attributes, storage, method) : isStorageSync(settings) ? {} : Promise.resolve({}); // Promisify if async return thenable(evaluations) ? evaluations.then((res) => wrapUp(res)) : wrapUp(evaluations); diff --git a/src/storages/AbstractSplitsCacheAsync.ts b/src/storages/AbstractSplitsCacheAsync.ts index bbd33e78..9e4e136c 100644 --- a/src/storages/AbstractSplitsCacheAsync.ts +++ b/src/storages/AbstractSplitsCacheAsync.ts @@ -18,7 +18,7 @@ export abstract class AbstractSplitsCacheAsync implements ISplitsCacheAsync { abstract getChangeNumber(): Promise abstract getAll(): Promise abstract getSplitNames(): Promise - abstract getNamesByFlagSets(flagSets: string[]): Promise> + abstract getNamesByFlagSets(flagSets: string[]): Promise[]> abstract trafficTypeExists(trafficType: string): Promise abstract clear(): Promise diff --git a/src/storages/AbstractSplitsCacheSync.ts b/src/storages/AbstractSplitsCacheSync.ts index 92a29c18..841ba26e 100644 --- a/src/storages/AbstractSplitsCacheSync.ts +++ b/src/storages/AbstractSplitsCacheSync.ts @@ -79,7 +79,7 @@ export abstract class AbstractSplitsCacheSync implements ISplitsCacheSync { return false; } - abstract getNamesByFlagSets(flagSets: string[]): ISet + abstract getNamesByFlagSets(flagSets: string[]): ISet[] } diff --git a/src/storages/inLocalStorage/SplitsCacheInLocal.ts b/src/storages/inLocalStorage/SplitsCacheInLocal.ts index 9e02ee15..7511897a 100644 --- a/src/storages/inLocalStorage/SplitsCacheInLocal.ts +++ b/src/storages/inLocalStorage/SplitsCacheInLocal.ts @@ -4,7 +4,7 @@ import { isFiniteNumber, toNumber, isNaNNumber } from '../../utils/lang'; import { KeyBuilderCS } from '../KeyBuilderCS'; import { ILogger } from '../../logger/types'; import { LOG_PREFIX } from './constants'; -import { ISet, _Set, returnSetsUnion, setToArray } from '../../utils/lang/sets'; +import { ISet, _Set, setToArray } from '../../utils/lang/sets'; /** * ISplitsCacheSync implementation that stores split definitions in browser LocalStorage. @@ -257,19 +257,13 @@ export class SplitsCacheInLocal extends AbstractSplitsCacheSync { // if the filter didn't change, nothing is done } - getNamesByFlagSets(flagSets: string[]): ISet{ - let toReturn: ISet = new _Set([]); - flagSets.forEach(flagSet => { + getNamesByFlagSets(flagSets: string[]): ISet[] { + return flagSets.map(flagSet => { const flagSetKey = this.keys.buildFlagSetKey(flagSet); let flagSetFromLocalStorage = localStorage.getItem(flagSetKey); - if (flagSetFromLocalStorage) { - const flagSetCache = new _Set(JSON.parse(flagSetFromLocalStorage)); - toReturn = returnSetsUnion(toReturn, flagSetCache); - } + return new _Set(flagSetFromLocalStorage ? JSON.parse(flagSetFromLocalStorage) : []); }); - return toReturn; - } private addToFlagSets(featureFlag: ISplit) { diff --git a/src/storages/inLocalStorage/__tests__/SplitsCacheInLocal.spec.ts b/src/storages/inLocalStorage/__tests__/SplitsCacheInLocal.spec.ts index b40f5f8a..1d5bc441 100644 --- a/src/storages/inLocalStorage/__tests__/SplitsCacheInLocal.spec.ts +++ b/src/storages/inLocalStorage/__tests__/SplitsCacheInLocal.spec.ts @@ -174,32 +174,32 @@ test('SPLIT CACHE / LocalStorage / flag set cache tests', () => { ]); cache.addSplit(featureFlagWithEmptyFS.name, featureFlagWithEmptyFS); - expect(cache.getNamesByFlagSets(['o'])).toEqual(new _Set(['ff_one', 'ff_two'])); - expect(cache.getNamesByFlagSets(['n'])).toEqual(new _Set(['ff_one'])); - expect(cache.getNamesByFlagSets(['e'])).toEqual(new _Set(['ff_one','ff_three'])); - expect(cache.getNamesByFlagSets(['t'])).toEqual(emptySet); // 't' not in filter - expect(cache.getNamesByFlagSets(['o','n','e'])).toEqual(new _Set(['ff_one','ff_two','ff_three'])); + expect(cache.getNamesByFlagSets(['o'])).toEqual([new _Set(['ff_one', 'ff_two'])]); + expect(cache.getNamesByFlagSets(['n'])).toEqual([new _Set(['ff_one'])]); + expect(cache.getNamesByFlagSets(['e'])).toEqual([new _Set(['ff_one', 'ff_three'])]); + expect(cache.getNamesByFlagSets(['t'])).toEqual([emptySet]); // 't' not in filter + expect(cache.getNamesByFlagSets(['o', 'n', 'e'])).toEqual([new _Set(['ff_one', 'ff_two']), new _Set(['ff_one']), new _Set(['ff_one', 'ff_three'])]); - cache.addSplit(featureFlagOne.name, {...featureFlagOne, sets: ['1']}); + cache.addSplit(featureFlagOne.name, { ...featureFlagOne, sets: ['1'] }); - expect(cache.getNamesByFlagSets(['1'])).toEqual(emptySet); // '1' not in filter - expect(cache.getNamesByFlagSets(['o'])).toEqual(new _Set(['ff_two'])); - expect(cache.getNamesByFlagSets(['n'])).toEqual(emptySet); + expect(cache.getNamesByFlagSets(['1'])).toEqual([emptySet]); // '1' not in filter + expect(cache.getNamesByFlagSets(['o'])).toEqual([new _Set(['ff_two'])]); + expect(cache.getNamesByFlagSets(['n'])).toEqual([emptySet]); - cache.addSplit(featureFlagOne.name, {...featureFlagOne, sets: ['x']}); - expect(cache.getNamesByFlagSets(['x'])).toEqual(new _Set(['ff_one'])); - expect(cache.getNamesByFlagSets(['o','e','x'])).toEqual(new _Set(['ff_one','ff_two','ff_three'])); + cache.addSplit(featureFlagOne.name, { ...featureFlagOne, sets: ['x'] }); + expect(cache.getNamesByFlagSets(['x'])).toEqual([new _Set(['ff_one'])]); + expect(cache.getNamesByFlagSets(['o', 'e', 'x'])).toEqual([new _Set(['ff_two']), new _Set(['ff_three']), new _Set(['ff_one'])]); cache.removeSplit(featureFlagOne.name); - expect(cache.getNamesByFlagSets(['x'])).toEqual(emptySet); + expect(cache.getNamesByFlagSets(['x'])).toEqual([emptySet]); cache.removeSplit(featureFlagOne.name); - expect(cache.getNamesByFlagSets(['y'])).toEqual(emptySet); // 'y' not in filter - expect(cache.getNamesByFlagSets([])).toEqual(emptySet); + expect(cache.getNamesByFlagSets(['y'])).toEqual([emptySet]); // 'y' not in filter + expect(cache.getNamesByFlagSets([])).toEqual([]); cache.addSplit(featureFlagWithEmptyFS.name, featureFlagWithoutFS); - expect(cache.getNamesByFlagSets([])).toEqual(emptySet); + expect(cache.getNamesByFlagSets([])).toEqual([]); }); // if FlagSets are not defined, it should store all FlagSets in memory. @@ -214,10 +214,10 @@ test('SPLIT CACHE / LocalStorage / flag set cache tests without filters', () => ]); cacheWithoutFilters.addSplit(featureFlagWithEmptyFS.name, featureFlagWithEmptyFS); - expect(cacheWithoutFilters.getNamesByFlagSets(['o'])).toEqual(new _Set(['ff_one', 'ff_two'])); - expect(cacheWithoutFilters.getNamesByFlagSets(['n'])).toEqual(new _Set(['ff_one'])); - expect(cacheWithoutFilters.getNamesByFlagSets(['e'])).toEqual(new _Set(['ff_one','ff_three'])); - expect(cacheWithoutFilters.getNamesByFlagSets(['t'])).toEqual(new _Set(['ff_two','ff_three'])); - expect(cacheWithoutFilters.getNamesByFlagSets(['y'])).toEqual(emptySet); - expect(cacheWithoutFilters.getNamesByFlagSets(['o','n','e'])).toEqual(new _Set(['ff_one','ff_two','ff_three'])); + expect(cacheWithoutFilters.getNamesByFlagSets(['o'])).toEqual([new _Set(['ff_one', 'ff_two'])]); + expect(cacheWithoutFilters.getNamesByFlagSets(['n'])).toEqual([new _Set(['ff_one'])]); + expect(cacheWithoutFilters.getNamesByFlagSets(['e'])).toEqual([new _Set(['ff_one', 'ff_three'])]); + expect(cacheWithoutFilters.getNamesByFlagSets(['t'])).toEqual([new _Set(['ff_two', 'ff_three'])]); + expect(cacheWithoutFilters.getNamesByFlagSets(['y'])).toEqual([emptySet]); + expect(cacheWithoutFilters.getNamesByFlagSets(['o', 'n', 'e'])).toEqual([new _Set(['ff_one', 'ff_two']), new _Set(['ff_one']), new _Set(['ff_one', 'ff_three'])]); }); diff --git a/src/storages/inMemory/SplitsCacheInMemory.ts b/src/storages/inMemory/SplitsCacheInMemory.ts index 36a55fe1..cf570eea 100644 --- a/src/storages/inMemory/SplitsCacheInMemory.ts +++ b/src/storages/inMemory/SplitsCacheInMemory.ts @@ -1,7 +1,7 @@ import { ISplit, ISplitFiltersValidation } from '../../dtos/types'; import { AbstractSplitsCacheSync, usesSegments } from '../AbstractSplitsCacheSync'; import { isFiniteNumber } from '../../utils/lang'; -import { ISet, _Set, returnSetsUnion } from '../../utils/lang/sets'; +import { ISet, _Set } from '../../utils/lang/sets'; /** * Default ISplitsCacheSync implementation that stores split definitions in memory. @@ -105,15 +105,8 @@ export class SplitsCacheInMemory extends AbstractSplitsCacheSync { return this.getChangeNumber() === -1 || this.splitsWithSegmentsCount > 0; } - getNamesByFlagSets(flagSets: string[]): ISet{ - let toReturn: ISet = new _Set([]); - flagSets.forEach(flagSet => { - const featureFlagNames = this.flagSetsCache[flagSet]; - if (featureFlagNames) { - toReturn = returnSetsUnion(toReturn, featureFlagNames); - } - }); - return toReturn; + getNamesByFlagSets(flagSets: string[]): ISet[] { + return flagSets.map(flagSet => this.flagSetsCache[flagSet] || new _Set()); } private addToFlagSets(featureFlag: ISplit) { @@ -128,7 +121,7 @@ export class SplitsCacheInMemory extends AbstractSplitsCacheSync { }); } - private removeFromFlagSets(featureFlagName :string, flagSets: string[] | undefined) { + private removeFromFlagSets(featureFlagName: string, flagSets: string[] | undefined) { if (!flagSets) return; flagSets.forEach(flagSet => { this.removeNames(flagSet, featureFlagName); diff --git a/src/storages/inMemory/__tests__/SplitsCacheInMemory.spec.ts b/src/storages/inMemory/__tests__/SplitsCacheInMemory.spec.ts index 865705cf..14fa62fd 100644 --- a/src/storages/inMemory/__tests__/SplitsCacheInMemory.spec.ts +++ b/src/storages/inMemory/__tests__/SplitsCacheInMemory.spec.ts @@ -127,32 +127,32 @@ test('SPLITS CACHE / In Memory / flag set cache tests', () => { ]); cache.addSplit(featureFlagWithEmptyFS.name, featureFlagWithEmptyFS); - expect(cache.getNamesByFlagSets(['o'])).toEqual(new _Set(['ff_one', 'ff_two'])); - expect(cache.getNamesByFlagSets(['n'])).toEqual(new _Set(['ff_one'])); - expect(cache.getNamesByFlagSets(['e'])).toEqual(new _Set(['ff_one','ff_three'])); - expect(cache.getNamesByFlagSets(['t'])).toEqual(emptySet); // 't' not in filter - expect(cache.getNamesByFlagSets(['o','n','e'])).toEqual(new _Set(['ff_one','ff_two','ff_three'])); + expect(cache.getNamesByFlagSets(['o'])).toEqual([new _Set(['ff_one', 'ff_two'])]); + expect(cache.getNamesByFlagSets(['n'])).toEqual([new _Set(['ff_one'])]); + expect(cache.getNamesByFlagSets(['e'])).toEqual([new _Set(['ff_one', 'ff_three'])]); + expect(cache.getNamesByFlagSets(['t'])).toEqual([emptySet]); // 't' not in filter + expect(cache.getNamesByFlagSets(['o', 'n', 'e'])).toEqual([new _Set(['ff_one', 'ff_two']), new _Set(['ff_one']), new _Set(['ff_one', 'ff_three'])]); - cache.addSplit(featureFlagOne.name, {...featureFlagOne, sets: ['1']}); + cache.addSplit(featureFlagOne.name, { ...featureFlagOne, sets: ['1'] }); - expect(cache.getNamesByFlagSets(['1'])).toEqual(emptySet); // '1' not in filter - expect(cache.getNamesByFlagSets(['o'])).toEqual(new _Set(['ff_two'])); - expect(cache.getNamesByFlagSets(['n'])).toEqual(emptySet); + expect(cache.getNamesByFlagSets(['1'])).toEqual([emptySet]); // '1' not in filter + expect(cache.getNamesByFlagSets(['o'])).toEqual([new _Set(['ff_two'])]); + expect(cache.getNamesByFlagSets(['n'])).toEqual([emptySet]); - cache.addSplit(featureFlagOne.name, {...featureFlagOne, sets: ['x']}); - expect(cache.getNamesByFlagSets(['x'])).toEqual(new _Set(['ff_one'])); - expect(cache.getNamesByFlagSets(['o','e','x'])).toEqual(new _Set(['ff_one','ff_two','ff_three'])); + cache.addSplit(featureFlagOne.name, { ...featureFlagOne, sets: ['x'] }); + expect(cache.getNamesByFlagSets(['x'])).toEqual([new _Set(['ff_one'])]); + expect(cache.getNamesByFlagSets(['o', 'e', 'x'])).toEqual([new _Set(['ff_two']), new _Set(['ff_three']), new _Set(['ff_one'])]); cache.removeSplit(featureFlagOne.name); - expect(cache.getNamesByFlagSets(['x'])).toEqual(emptySet); + expect(cache.getNamesByFlagSets(['x'])).toEqual([emptySet]); cache.removeSplit(featureFlagOne.name); - expect(cache.getNamesByFlagSets(['y'])).toEqual(emptySet); // 'y' not in filter - expect(cache.getNamesByFlagSets([])).toEqual(emptySet); + expect(cache.getNamesByFlagSets(['y'])).toEqual([emptySet]); // 'y' not in filter + expect(cache.getNamesByFlagSets([])).toEqual([]); cache.addSplit(featureFlagWithEmptyFS.name, featureFlagWithoutFS); - expect(cache.getNamesByFlagSets([])).toEqual(emptySet); + expect(cache.getNamesByFlagSets([])).toEqual([]); }); // if FlagSets are not defined, it should store all FlagSets in memory. @@ -167,10 +167,10 @@ test('SPLIT CACHE / LocalStorage / flag set cache tests without filters', () => ]); cacheWithoutFilters.addSplit(featureFlagWithEmptyFS.name, featureFlagWithEmptyFS); - expect(cacheWithoutFilters.getNamesByFlagSets(['o'])).toEqual(new _Set(['ff_one', 'ff_two'])); - expect(cacheWithoutFilters.getNamesByFlagSets(['n'])).toEqual(new _Set(['ff_one'])); - expect(cacheWithoutFilters.getNamesByFlagSets(['e'])).toEqual(new _Set(['ff_one','ff_three'])); - expect(cacheWithoutFilters.getNamesByFlagSets(['t'])).toEqual(new _Set(['ff_two','ff_three'])); - expect(cacheWithoutFilters.getNamesByFlagSets(['y'])).toEqual(emptySet); - expect(cacheWithoutFilters.getNamesByFlagSets(['o','n','e'])).toEqual(new _Set(['ff_one','ff_two','ff_three'])); + expect(cacheWithoutFilters.getNamesByFlagSets(['o'])).toEqual([new _Set(['ff_one', 'ff_two'])]); + expect(cacheWithoutFilters.getNamesByFlagSets(['n'])).toEqual([new _Set(['ff_one'])]); + expect(cacheWithoutFilters.getNamesByFlagSets(['e'])).toEqual([new _Set(['ff_one', 'ff_three'])]); + expect(cacheWithoutFilters.getNamesByFlagSets(['t'])).toEqual([new _Set(['ff_two', 'ff_three'])]); + expect(cacheWithoutFilters.getNamesByFlagSets(['y'])).toEqual([emptySet]); + expect(cacheWithoutFilters.getNamesByFlagSets(['o', 'n', 'e'])).toEqual([new _Set(['ff_one', 'ff_two']), new _Set(['ff_one']), new _Set(['ff_one', 'ff_three'])]); }); diff --git a/src/storages/inRedis/SplitsCacheInRedis.ts b/src/storages/inRedis/SplitsCacheInRedis.ts index f58f49ab..6aa1136b 100644 --- a/src/storages/inRedis/SplitsCacheInRedis.ts +++ b/src/storages/inRedis/SplitsCacheInRedis.ts @@ -195,7 +195,7 @@ export class SplitsCacheInRedis extends AbstractSplitsCacheAsync { * or rejected if wrapper operation fails. * @todo this is a no-op method to be implemented */ - getNamesByFlagSets(): Promise> { + getNamesByFlagSets(): Promise[]> { this.log.error(LOG_PREFIX + 'ByFlagSet/s evaluations are not supported with Redis storage yet.'); return Promise.reject(); } diff --git a/src/storages/pluggable/SplitsCachePluggable.ts b/src/storages/pluggable/SplitsCachePluggable.ts index 8f355fb1..c2ce2e08 100644 --- a/src/storages/pluggable/SplitsCachePluggable.ts +++ b/src/storages/pluggable/SplitsCachePluggable.ts @@ -181,17 +181,11 @@ export class SplitsCachePluggable extends AbstractSplitsCacheAsync { * The returned promise is resolved with the list of split names, * or rejected if any wrapper operation fails. */ - getNamesByFlagSets(flagSets: string[]): Promise> { + getNamesByFlagSets(flagSets: string[]): Promise[]> { return Promise.all(flagSets.map(flagSet => { const flagSetKey = this.keys.buildFlagSetKey(flagSet); return this.wrapper.getItems(flagSetKey); - })).then(namesByFlagSets => { - const featureFlagNames = new _Set(); - namesByFlagSets.forEach(names => { - names.forEach(name => featureFlagNames.add(name)); - }); - return featureFlagNames; - }); + })).then(namesByFlagSets => namesByFlagSets.map(namesByFlagSet => new _Set(namesByFlagSet))); } /** diff --git a/src/storages/pluggable/__tests__/SplitsCachePluggable.spec.ts b/src/storages/pluggable/__tests__/SplitsCachePluggable.spec.ts index 3d67e454..03221de0 100644 --- a/src/storages/pluggable/__tests__/SplitsCachePluggable.spec.ts +++ b/src/storages/pluggable/__tests__/SplitsCachePluggable.spec.ts @@ -163,31 +163,31 @@ describe('SPLITS CACHE PLUGGABLE', () => { ]); await cache.addSplit(featureFlagWithEmptyFS.name, featureFlagWithEmptyFS); - expect(await cache.getNamesByFlagSets(['o'])).toEqual(new _Set(['ff_one', 'ff_two'])); - expect(await cache.getNamesByFlagSets(['n'])).toEqual(new _Set(['ff_one'])); - expect(await cache.getNamesByFlagSets(['e'])).toEqual(new _Set(['ff_one', 'ff_three'])); - expect(await cache.getNamesByFlagSets(['t'])).toEqual(emptySet); // 't' not in filter - expect(await cache.getNamesByFlagSets(['o', 'n', 'e'])).toEqual(new _Set(['ff_one', 'ff_two', 'ff_three'])); + expect(await cache.getNamesByFlagSets(['o'])).toEqual([new _Set(['ff_one', 'ff_two'])]); + expect(await cache.getNamesByFlagSets(['n'])).toEqual([new _Set(['ff_one'])]); + expect(await cache.getNamesByFlagSets(['e'])).toEqual([new _Set(['ff_one', 'ff_three'])]); + expect(await cache.getNamesByFlagSets(['t'])).toEqual([emptySet]); // 't' not in filter + expect(await cache.getNamesByFlagSets(['o', 'n', 'e'])).toEqual([new _Set(['ff_one', 'ff_two']), new _Set(['ff_one']), new _Set(['ff_one', 'ff_three'])]); await cache.addSplit(featureFlagOne.name, { ...featureFlagOne, sets: ['1'] }); - expect(await cache.getNamesByFlagSets(['1'])).toEqual(emptySet); // '1' not in filter - expect(await cache.getNamesByFlagSets(['o'])).toEqual(new _Set(['ff_two'])); - expect(await cache.getNamesByFlagSets(['n'])).toEqual(emptySet); + expect(await cache.getNamesByFlagSets(['1'])).toEqual([emptySet]); // '1' not in filter + expect(await cache.getNamesByFlagSets(['o'])).toEqual([new _Set(['ff_two'])]); + expect(await cache.getNamesByFlagSets(['n'])).toEqual([emptySet]); await cache.addSplit(featureFlagOne.name, { ...featureFlagOne, sets: ['x'] }); - expect(await cache.getNamesByFlagSets(['x'])).toEqual(new _Set(['ff_one'])); - expect(await cache.getNamesByFlagSets(['o', 'e', 'x'])).toEqual(new _Set(['ff_one', 'ff_two', 'ff_three'])); + expect(await cache.getNamesByFlagSets(['x'])).toEqual([new _Set(['ff_one'])]); + expect(await cache.getNamesByFlagSets(['o', 'e', 'x'])).toEqual([new _Set(['ff_two']), new _Set(['ff_three']), new _Set(['ff_one'])]); await cache.removeSplit(featureFlagOne.name); - expect(await cache.getNamesByFlagSets(['x'])).toEqual(emptySet); + expect(await cache.getNamesByFlagSets(['x'])).toEqual([emptySet]); await cache.removeSplit(featureFlagOne.name); - expect(await cache.getNamesByFlagSets(['y'])).toEqual(emptySet); // 'y' not in filter - expect(await cache.getNamesByFlagSets([])).toEqual(emptySet); + expect(await cache.getNamesByFlagSets(['y'])).toEqual([emptySet]); // 'y' not in filter + expect(await cache.getNamesByFlagSets([])).toEqual([]); await cache.addSplit(featureFlagWithEmptyFS.name, featureFlagWithoutFS); - expect(await cache.getNamesByFlagSets([])).toEqual(emptySet); + expect(await cache.getNamesByFlagSets([])).toEqual([]); }); // if FlagSets filter is not defined, it should store all FlagSets in memory. @@ -202,12 +202,12 @@ describe('SPLITS CACHE PLUGGABLE', () => { ]); await cacheWithoutFilters.addSplit(featureFlagWithEmptyFS.name, featureFlagWithEmptyFS); - expect(await cacheWithoutFilters.getNamesByFlagSets(['o'])).toEqual(new _Set(['ff_one', 'ff_two'])); - expect(await cacheWithoutFilters.getNamesByFlagSets(['n'])).toEqual(new _Set(['ff_one'])); - expect(await cacheWithoutFilters.getNamesByFlagSets(['e'])).toEqual(new _Set(['ff_one', 'ff_three'])); - expect(await cacheWithoutFilters.getNamesByFlagSets(['t'])).toEqual(new _Set(['ff_two', 'ff_three'])); - expect(await cacheWithoutFilters.getNamesByFlagSets(['y'])).toEqual(emptySet); - expect(await cacheWithoutFilters.getNamesByFlagSets(['o', 'n', 'e'])).toEqual(new _Set(['ff_one', 'ff_two', 'ff_three'])); + expect(await cacheWithoutFilters.getNamesByFlagSets(['o'])).toEqual([new _Set(['ff_one', 'ff_two'])]); + expect(await cacheWithoutFilters.getNamesByFlagSets(['n'])).toEqual([new _Set(['ff_one'])]); + expect(await cacheWithoutFilters.getNamesByFlagSets(['e'])).toEqual([new _Set(['ff_one', 'ff_three'])]); + expect(await cacheWithoutFilters.getNamesByFlagSets(['t'])).toEqual([new _Set(['ff_two', 'ff_three'])]); + expect(await cacheWithoutFilters.getNamesByFlagSets(['y'])).toEqual([emptySet]); + expect(await cacheWithoutFilters.getNamesByFlagSets(['o', 'n', 'e'])).toEqual([new _Set(['ff_one', 'ff_two']), new _Set(['ff_one']), new _Set(['ff_one', 'ff_three'])]); }); }); diff --git a/src/storages/types.ts b/src/storages/types.ts index 8f45f4b2..9e8dc563 100644 --- a/src/storages/types.ts +++ b/src/storages/types.ts @@ -210,7 +210,7 @@ export interface ISplitsCacheBase { // should never reject or throw an exception. Instead return false by default, to avoid emitting SDK_READY_FROM_CACHE. checkCache(): MaybeThenable, killLocally(name: string, defaultTreatment: string, changeNumber: number): MaybeThenable, - getNamesByFlagSets(flagSets: string[]): MaybeThenable> + getNamesByFlagSets(flagSets: string[]): MaybeThenable[]> } export interface ISplitsCacheSync extends ISplitsCacheBase { @@ -227,7 +227,7 @@ export interface ISplitsCacheSync extends ISplitsCacheBase { clear(): void, checkCache(): boolean, killLocally(name: string, defaultTreatment: string, changeNumber: number): boolean, - getNamesByFlagSets(flagSets: string[]): ISet + getNamesByFlagSets(flagSets: string[]): ISet[] } export interface ISplitsCacheAsync extends ISplitsCacheBase { @@ -244,7 +244,7 @@ export interface ISplitsCacheAsync extends ISplitsCacheBase { clear(): Promise, checkCache(): Promise, killLocally(name: string, defaultTreatment: string, changeNumber: number): Promise, - getNamesByFlagSets(flagSets: string[]): Promise> + getNamesByFlagSets(flagSets: string[]): Promise[]> } /** Segments cache */ From 5b3d04bf46ce048f4c258bd10e7a1219f64437f0 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Fri, 24 Nov 2023 17:16:25 -0300 Subject: [PATCH 12/45] Add changelog entry --- CHANGES.txt | 3 +++ package.json | 2 +- src/storages/pluggable/SplitsCachePluggable.ts | 4 ++-- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index a893bd27..03adce03 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,3 +1,6 @@ +1.12.0 (December XX, 2023) + - Added support for Flag Sets in "consumer" and "partial consumer" modes for pluggable storage. + 1.11.0 (November 3, 2023) - Added support for Flag Sets on the SDK, which enables grouping feature flags and interacting with the group rather than individually (more details in our documentation): - Added new variations of the get treatment methods to support evaluating flags in given flag set/s. diff --git a/package.json b/package.json index 2fda9f0b..5d2087b4 100644 --- a/package.json +++ b/package.json @@ -22,7 +22,7 @@ "build": "npm run build:cjs && npm run build:esm", "build:esm": "rimraf esm && tsc -m es2015 --outDir esm -d true --declarationDir types", "build:cjs": "rimraf cjs && tsc -m CommonJS --outDir cjs", - "test": "jest", + "test": "jest --runInBand", "test:coverage": "jest --coverage", "all": "npm run check && npm run build && npm run test", "publish:rc": "npm run check && npm run test && npm run build && npm publish --tag rc", diff --git a/src/storages/pluggable/SplitsCachePluggable.ts b/src/storages/pluggable/SplitsCachePluggable.ts index 8f355fb1..da409eb2 100644 --- a/src/storages/pluggable/SplitsCachePluggable.ts +++ b/src/storages/pluggable/SplitsCachePluggable.ts @@ -43,7 +43,7 @@ export class SplitsCachePluggable extends AbstractSplitsCacheAsync { return this.wrapper.incr(ttKey); } - private _updateFlagSets(name: string, flagSetsOfRemovedFlag?: string[], flagSetsOfAddedFlag?: string[]) { + private _updateFlagSets(featureFlagName: string, flagSetsOfRemovedFlag?: string[], flagSetsOfAddedFlag?: string[]) { const removeFromFlagSets = returnListDifference(flagSetsOfRemovedFlag, flagSetsOfAddedFlag); let addToFlagSets = returnListDifference(flagSetsOfAddedFlag, flagSetsOfRemovedFlag); @@ -53,7 +53,7 @@ export class SplitsCachePluggable extends AbstractSplitsCacheAsync { }); } - const items = [name]; + const items = [featureFlagName]; return Promise.all([ ...removeFromFlagSets.map(flagSetName => this.wrapper.removeItems(this.keys.buildFlagSetKey(flagSetName), items)), From 89e980b6c26a4ffe4b121ec3c2df850701947122 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Fri, 24 Nov 2023 17:20:13 -0300 Subject: [PATCH 13/45] Add changelog entry and prepare rc --- CHANGES.txt | 1 + package-lock.json | 4 ++-- package.json | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 03adce03..89b98466 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,5 +1,6 @@ 1.12.0 (December XX, 2023) - Added support for Flag Sets in "consumer" and "partial consumer" modes for pluggable storage. + - Updated evaluation flow to log a warning when using flag sets that don't contain cached feature flags. 1.11.0 (November 3, 2023) - Added support for Flag Sets on the SDK, which enables grouping feature flags and interacting with the group rather than individually (more details in our documentation): diff --git a/package-lock.json b/package-lock.json index 5c28d3a7..b79d5936 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@splitsoftware/splitio-commons", - "version": "1.11.0", + "version": "1.12.1-rc.0", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "@splitsoftware/splitio-commons", - "version": "1.11.0", + "version": "1.12.1-rc.0", "license": "Apache-2.0", "dependencies": { "tslib": "^2.3.1" diff --git a/package.json b/package.json index 5d2087b4..8d5952ce 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@splitsoftware/splitio-commons", - "version": "1.11.0", + "version": "1.12.1-rc.0", "description": "Split Javascript SDK common components", "main": "cjs/index.js", "module": "esm/index.js", From c7d2fae80d36ed2aaad620cef54a363e00606d56 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Mon, 27 Nov 2023 12:44:28 -0300 Subject: [PATCH 14/45] Implement SplitsCacheRedis::getNamesByFlagSets and _updateFlagSets methods --- CHANGES.txt | 2 +- src/storages/inRedis/SplitsCacheInRedis.ts | 52 +++++++++---- .../__tests__/SplitsCacheInRedis.spec.ts | 75 ++++++++++++++++++- 3 files changed, 112 insertions(+), 17 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 89b98466..7d0aca28 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,5 +1,5 @@ 1.12.0 (December XX, 2023) - - Added support for Flag Sets in "consumer" and "partial consumer" modes for pluggable storage. + - Added support for Flag Sets in "consumer" and "partial consumer" modes for Pluggable and Redis storages. - Updated evaluation flow to log a warning when using flag sets that don't contain cached feature flags. 1.11.0 (November 3, 2023) diff --git a/src/storages/inRedis/SplitsCacheInRedis.ts b/src/storages/inRedis/SplitsCacheInRedis.ts index 6aa1136b..1e065227 100644 --- a/src/storages/inRedis/SplitsCacheInRedis.ts +++ b/src/storages/inRedis/SplitsCacheInRedis.ts @@ -3,9 +3,9 @@ import { KeyBuilderSS } from '../KeyBuilderSS'; import { Redis } from 'ioredis'; import { ILogger } from '../../logger/types'; import { LOG_PREFIX } from './constants'; -import { ISplit } from '../../dtos/types'; +import { ISplit, ISplitFiltersValidation } from '../../dtos/types'; import { AbstractSplitsCacheAsync } from '../AbstractSplitsCacheAsync'; -import { ISet } from '../../utils/lang/sets'; +import { ISet, _Set, returnListDifference } from '../../utils/lang/sets'; /** * Discard errors for an answer of multiple operations. @@ -27,12 +27,14 @@ export class SplitsCacheInRedis extends AbstractSplitsCacheAsync { private readonly redis: Redis; private readonly keys: KeyBuilderSS; private redisError?: string; + private readonly flagSetsFilter: string[]; - constructor(log: ILogger, keys: KeyBuilderSS, redis: Redis) { + constructor(log: ILogger, keys: KeyBuilderSS, redis: Redis, splitFiltersValidation?: ISplitFiltersValidation) { super(); this.log = log; this.redis = redis; this.keys = keys; + this.flagSetsFilter = splitFiltersValidation ? splitFiltersValidation.groupedFilters.bySet : []; // There is no need to listen for redis 'error' event, because in that case ioredis calls will be rejected and handled by redis storage adapters. // But it is done just to avoid getting the ioredis message `Unhandled error event`. @@ -57,6 +59,24 @@ export class SplitsCacheInRedis extends AbstractSplitsCacheAsync { return this.redis.incr(ttKey); } + private _updateFlagSets(featureFlagName: string, flagSetsOfRemovedFlag?: string[], flagSetsOfAddedFlag?: string[]) { + const removeFromFlagSets = returnListDifference(flagSetsOfRemovedFlag, flagSetsOfAddedFlag); + + let addToFlagSets = returnListDifference(flagSetsOfAddedFlag, flagSetsOfRemovedFlag); + if (this.flagSetsFilter.length > 0) { + addToFlagSets = addToFlagSets.filter(flagSet => { + return this.flagSetsFilter.some(filterFlagSet => filterFlagSet === flagSet); + }); + } + + const items = [featureFlagName]; + + return Promise.all([ + ...removeFromFlagSets.map(flagSetName => this.redis.srem(this.keys.buildFlagSetKey(flagSetName), items)), + ...addToFlagSets.map(flagSetName => this.redis.sadd(this.keys.buildFlagSetKey(flagSetName), items)) + ]); + } + /** * Add a given split. * The returned promise is resolved when the operation success @@ -66,16 +86,16 @@ export class SplitsCacheInRedis extends AbstractSplitsCacheAsync { const splitKey = this.keys.buildSplitKey(name); return this.redis.get(splitKey).then(splitFromStorage => { - // handling parsing errors - let parsedPreviousSplit: ISplit, newStringifiedSplit; + // handling parsing error + let parsedPreviousSplit: ISplit, stringifiedNewSplit; try { parsedPreviousSplit = splitFromStorage ? JSON.parse(splitFromStorage) : undefined; - newStringifiedSplit = JSON.stringify(split); + stringifiedNewSplit = JSON.stringify(split); } catch (e) { throw new Error('Error parsing feature flag definition: ' + e); } - return this.redis.set(splitKey, newStringifiedSplit).then(() => { + return this.redis.set(splitKey, stringifiedNewSplit).then(() => { // avoid unnecessary increment/decrement operations if (parsedPreviousSplit && parsedPreviousSplit.trafficTypeName === split.trafficTypeName) return; @@ -83,7 +103,7 @@ export class SplitsCacheInRedis extends AbstractSplitsCacheAsync { return this._incrementCounts(split).then(() => { if (parsedPreviousSplit) return this._decrementCounts(parsedPreviousSplit); }); - }); + }).then(() => this._updateFlagSets(name, parsedPreviousSplit && parsedPreviousSplit.sets, split.sets)); }).then(() => true); } @@ -101,11 +121,12 @@ export class SplitsCacheInRedis extends AbstractSplitsCacheAsync { * The returned promise is resolved when the operation success, with 1 or 0 indicating if the split existed or not. * or rejected if it fails (e.g., redis operation fails). */ - removeSplit(name: string): Promise { + removeSplit(name: string) { return this.getSplit(name).then((split) => { if (split) { - this._decrementCounts(split); + return this._decrementCounts(split).then(() => this._updateFlagSets(name, split.sets)); } + }).then(() => { return this.redis.del(this.keys.buildSplitKey(name)); }); } @@ -192,12 +213,13 @@ export class SplitsCacheInRedis extends AbstractSplitsCacheAsync { /** * Get list of split names related to a given flag set names list. * The returned promise is resolved with the list of split names, - * or rejected if wrapper operation fails. - * @todo this is a no-op method to be implemented + * or rejected if any wrapper operation fails. */ - getNamesByFlagSets(): Promise[]> { - this.log.error(LOG_PREFIX + 'ByFlagSet/s evaluations are not supported with Redis storage yet.'); - return Promise.reject(); + getNamesByFlagSets(flagSets: string[]): Promise[]> { + return Promise.all(flagSets.map(flagSet => { + const flagSetKey = this.keys.buildFlagSetKey(flagSet); + return this.redis.smembers(flagSetKey); + })).then(namesByFlagSets => namesByFlagSets.map(namesByFlagSet => new _Set(namesByFlagSet))); } /** diff --git a/src/storages/inRedis/__tests__/SplitsCacheInRedis.spec.ts b/src/storages/inRedis/__tests__/SplitsCacheInRedis.spec.ts index 22d12190..665776c9 100644 --- a/src/storages/inRedis/__tests__/SplitsCacheInRedis.spec.ts +++ b/src/storages/inRedis/__tests__/SplitsCacheInRedis.spec.ts @@ -2,9 +2,10 @@ import Redis from 'ioredis'; import { SplitsCacheInRedis } from '../SplitsCacheInRedis'; import { KeyBuilderSS } from '../../KeyBuilderSS'; import { loggerMock } from '../../../logger/__tests__/sdkLogger.mock'; -import { splitWithUserTT, splitWithAccountTT } from '../../__tests__/testUtils'; +import { splitWithUserTT, splitWithAccountTT, featureFlagOne, featureFlagThree, featureFlagTwo, featureFlagWithEmptyFS, featureFlagWithoutFS } from '../../__tests__/testUtils'; import { ISplit } from '../../../dtos/types'; import { metadata } from '../../__tests__/KeyBuilder.spec'; +import { _Set } from '../../../utils/lang/sets'; const prefix = 'splits_cache_ut'; const keysBuilder = new KeyBuilderSS(prefix, metadata); @@ -143,4 +144,76 @@ describe('SPLITS CACHE REDIS', () => { await connection.quit(); }); + test('flag set cache tests', async () => { + const connection = new Redis(); // @ts-ignore + const cache = new SplitsCacheInRedis(loggerMock, keysBuilder, connection, { groupedFilters: { bySet: ['o', 'n', 'e', 'x'] } }); + + const emptySet = new _Set([]); + + await cache.addSplits([ + [featureFlagOne.name, featureFlagOne], + [featureFlagTwo.name, featureFlagTwo], + [featureFlagThree.name, featureFlagThree], + ]); + await cache.addSplit(featureFlagWithEmptyFS.name, featureFlagWithEmptyFS); + + expect(await cache.getNamesByFlagSets(['o'])).toEqual([new _Set(['ff_one', 'ff_two'])]); + expect(await cache.getNamesByFlagSets(['n'])).toEqual([new _Set(['ff_one'])]); + expect(await cache.getNamesByFlagSets(['e'])).toEqual([new _Set(['ff_one', 'ff_three'])]); + expect(await cache.getNamesByFlagSets(['t'])).toEqual([emptySet]); // 't' not in filter + expect(await cache.getNamesByFlagSets(['o', 'n', 'e'])).toEqual([new _Set(['ff_one', 'ff_two']), new _Set(['ff_one']), new _Set(['ff_one', 'ff_three'])]); + + await cache.addSplit(featureFlagOne.name, { ...featureFlagOne, sets: ['1'] }); + + expect(await cache.getNamesByFlagSets(['1'])).toEqual([emptySet]); // '1' not in filter + expect(await cache.getNamesByFlagSets(['o'])).toEqual([new _Set(['ff_two'])]); + expect(await cache.getNamesByFlagSets(['n'])).toEqual([emptySet]); + + await cache.addSplit(featureFlagOne.name, { ...featureFlagOne, sets: ['x'] }); + expect(await cache.getNamesByFlagSets(['x'])).toEqual([new _Set(['ff_one'])]); + expect(await cache.getNamesByFlagSets(['o', 'e', 'x'])).toEqual([new _Set(['ff_two']), new _Set(['ff_three']), new _Set(['ff_one'])]); + + await cache.removeSplit(featureFlagOne.name); + expect(await cache.getNamesByFlagSets(['x'])).toEqual([emptySet]); + + await cache.removeSplit(featureFlagOne.name); + expect(await cache.getNamesByFlagSets(['y'])).toEqual([emptySet]); // 'y' not in filter + expect(await cache.getNamesByFlagSets([])).toEqual([]); + + await cache.addSplit(featureFlagWithEmptyFS.name, featureFlagWithoutFS); + expect(await cache.getNamesByFlagSets([])).toEqual([]); + + // Delete splits, TT and flag set keys + await cache.removeSplits([featureFlagThree.name, featureFlagTwo.name, featureFlagWithEmptyFS.name]); + expect(await connection.keys(`${prefix}*`)).toHaveLength(0); + await connection.quit(); + }); + + // if FlagSets filter is not defined, it should store all FlagSets in memory. + test('flag set cache tests without filters', async () => { + const connection = new Redis(); // @ts-ignore + const cacheWithoutFilters = new SplitsCacheInRedis(loggerMock, keysBuilder, connection); + + const emptySet = new _Set([]); + + await cacheWithoutFilters.addSplits([ + [featureFlagOne.name, featureFlagOne], + [featureFlagTwo.name, featureFlagTwo], + [featureFlagThree.name, featureFlagThree], + ]); + await cacheWithoutFilters.addSplit(featureFlagWithEmptyFS.name, featureFlagWithEmptyFS); + + expect(await cacheWithoutFilters.getNamesByFlagSets(['o'])).toEqual([new _Set(['ff_one', 'ff_two'])]); + expect(await cacheWithoutFilters.getNamesByFlagSets(['n'])).toEqual([new _Set(['ff_one'])]); + expect(await cacheWithoutFilters.getNamesByFlagSets(['e'])).toEqual([new _Set(['ff_one', 'ff_three'])]); + expect(await cacheWithoutFilters.getNamesByFlagSets(['t'])).toEqual([new _Set(['ff_two', 'ff_three'])]); + expect(await cacheWithoutFilters.getNamesByFlagSets(['y'])).toEqual([emptySet]); + expect(await cacheWithoutFilters.getNamesByFlagSets(['o', 'n', 'e'])).toEqual([new _Set(['ff_one', 'ff_two']), new _Set(['ff_one']), new _Set(['ff_one', 'ff_three'])]); + + // Delete splits, TT and flag set keys + await cacheWithoutFilters.removeSplits([featureFlagThree.name, featureFlagTwo.name, featureFlagOne.name, featureFlagWithEmptyFS.name]); + expect(await connection.keys(`${prefix}*`)).toHaveLength(0); + await connection.quit(); + }); + }); From 92b8a19c7d90e583268af8b58e2bec198579e613 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Mon, 27 Nov 2023 13:42:42 -0300 Subject: [PATCH 15/45] prepare rc --- package-lock.json | 4 ++-- package.json | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/package-lock.json b/package-lock.json index b79d5936..a0e6bd3b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@splitsoftware/splitio-commons", - "version": "1.12.1-rc.0", + "version": "1.12.1-rc.1", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "@splitsoftware/splitio-commons", - "version": "1.12.1-rc.0", + "version": "1.12.1-rc.1", "license": "Apache-2.0", "dependencies": { "tslib": "^2.3.1" diff --git a/package.json b/package.json index 8d5952ce..a1b84218 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@splitsoftware/splitio-commons", - "version": "1.12.1-rc.0", + "version": "1.12.1-rc.1", "description": "Split Javascript SDK common components", "main": "cjs/index.js", "module": "esm/index.js", @@ -22,7 +22,7 @@ "build": "npm run build:cjs && npm run build:esm", "build:esm": "rimraf esm && tsc -m es2015 --outDir esm -d true --declarationDir types", "build:cjs": "rimraf cjs && tsc -m CommonJS --outDir cjs", - "test": "jest --runInBand", + "test": "jest", "test:coverage": "jest --coverage", "all": "npm run check && npm run build && npm run test", "publish:rc": "npm run check && npm run test && npm run build && npm publish --tag rc", From 9773c7aea8df42f654fa78b2cca3e40f08b81918 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Tue, 28 Nov 2023 12:38:50 -0300 Subject: [PATCH 16/45] Update SplitsCacheRedis::getNamesByFlagSets method to use Redis pipeline command --- CHANGES.txt | 2 +- src/storages/inRedis/SplitsCacheInRedis.ts | 11 +++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 7d0aca28..c827ea97 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -57,7 +57,7 @@ - Added a new impressions mode for the SDK called NONE, to be used in factory when there is no desire to capture impressions on an SDK factory to feed Split's analytics engine. Running NONE mode, the SDK will only capture unique keys evaluated for a particular feature flag instead of full blown impressions. - Updated SDK telemetry to support pluggable storage, partial consumer mode, and synchronizer. - Updated storage implementations to improve the performance of feature flag evaluations (i.e., `getTreatment(s)` method calls) when using the default storage in memory. - - Updated evaluation flow to avoid unnecessarily storage calls when the SDK is not ready. + - Updated evaluation flow (i.e., `getTreatment(s)` method calls) to avoid calling the storage for cached feature flags when the SDK is not ready or ready from cache. It applies to all SDK modes. 1.6.1 (July 22, 2022) - Updated GoogleAnalyticsToSplit integration to validate `autoRequire` config parameter and avoid some wrong warning logs when mapping GA hit fields to Split event properties. diff --git a/src/storages/inRedis/SplitsCacheInRedis.ts b/src/storages/inRedis/SplitsCacheInRedis.ts index 1e065227..aeed1c87 100644 --- a/src/storages/inRedis/SplitsCacheInRedis.ts +++ b/src/storages/inRedis/SplitsCacheInRedis.ts @@ -10,8 +10,8 @@ import { ISet, _Set, returnListDifference } from '../../utils/lang/sets'; /** * Discard errors for an answer of multiple operations. */ -function processPipelineAnswer(results: Array<[Error | null, string]>): string[] { - return results.reduce((accum: string[], errValuePair: [Error | null, string]) => { +function processPipelineAnswer(results: Array<[Error | null, T]>): T[] { + return results.reduce((accum: T[], errValuePair: [Error | null, T]) => { if (errValuePair[0] === null) accum.push(errValuePair[1]); return accum; }, []); @@ -216,10 +216,9 @@ export class SplitsCacheInRedis extends AbstractSplitsCacheAsync { * or rejected if any wrapper operation fails. */ getNamesByFlagSets(flagSets: string[]): Promise[]> { - return Promise.all(flagSets.map(flagSet => { - const flagSetKey = this.keys.buildFlagSetKey(flagSet); - return this.redis.smembers(flagSetKey); - })).then(namesByFlagSets => namesByFlagSets.map(namesByFlagSet => new _Set(namesByFlagSet))); + return this.redis.pipeline(flagSets.map(flagSet => ['smembers', this.keys.buildFlagSetKey(flagSet)])).exec() + .then(processPipelineAnswer) + .then(namesByFlagSets => namesByFlagSets.map(namesByFlagSet => new _Set(namesByFlagSet))); } /** From f61b65aa28b37570b149806308430a9863f4077a Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Tue, 28 Nov 2023 12:47:57 -0300 Subject: [PATCH 17/45] Polishing --- package.json | 2 +- src/storages/inLocalStorage/SplitsCacheInLocal.ts | 9 ++++----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/package.json b/package.json index 8d5952ce..65b3d060 100644 --- a/package.json +++ b/package.json @@ -22,7 +22,7 @@ "build": "npm run build:cjs && npm run build:esm", "build:esm": "rimraf esm && tsc -m es2015 --outDir esm -d true --declarationDir types", "build:cjs": "rimraf cjs && tsc -m CommonJS --outDir cjs", - "test": "jest --runInBand", + "test": "jest", "test:coverage": "jest --coverage", "all": "npm run check && npm run build && npm run test", "publish:rc": "npm run check && npm run test && npm run build && npm publish --tag rc", diff --git a/src/storages/inLocalStorage/SplitsCacheInLocal.ts b/src/storages/inLocalStorage/SplitsCacheInLocal.ts index 7511897a..acca8f3c 100644 --- a/src/storages/inLocalStorage/SplitsCacheInLocal.ts +++ b/src/storages/inLocalStorage/SplitsCacheInLocal.ts @@ -260,7 +260,7 @@ export class SplitsCacheInLocal extends AbstractSplitsCacheSync { getNamesByFlagSets(flagSets: string[]): ISet[] { return flagSets.map(flagSet => { const flagSetKey = this.keys.buildFlagSetKey(flagSet); - let flagSetFromLocalStorage = localStorage.getItem(flagSetKey); + const flagSetFromLocalStorage = localStorage.getItem(flagSetKey); return new _Set(flagSetFromLocalStorage ? JSON.parse(flagSetFromLocalStorage) : []); }); @@ -275,10 +275,9 @@ export class SplitsCacheInLocal extends AbstractSplitsCacheSync { const flagSetKey = this.keys.buildFlagSetKey(featureFlagSet); - let flagSetFromLocalStorage = localStorage.getItem(flagSetKey); - if (!flagSetFromLocalStorage) flagSetFromLocalStorage = '[]'; + const flagSetFromLocalStorage = localStorage.getItem(flagSetKey); - const flagSetCache = new _Set(JSON.parse(flagSetFromLocalStorage)); + const flagSetCache = new _Set(flagSetFromLocalStorage ? JSON.parse(flagSetFromLocalStorage) : []); flagSetCache.add(featureFlag.name); localStorage.setItem(flagSetKey, JSON.stringify(setToArray(flagSetCache))); @@ -296,7 +295,7 @@ export class SplitsCacheInLocal extends AbstractSplitsCacheSync { private removeNames(flagSetName: string, featureFlagName: string) { const flagSetKey = this.keys.buildFlagSetKey(flagSetName); - let flagSetFromLocalStorage = localStorage.getItem(flagSetKey); + const flagSetFromLocalStorage = localStorage.getItem(flagSetKey); if (!flagSetFromLocalStorage) return; From 0e1605ae4d5ff04218013eb1ae490414e97a33b4 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Wed, 29 Nov 2023 11:44:34 -0300 Subject: [PATCH 18/45] Update getNamesByFlagSets methods to handle command errors --- src/storages/inRedis/SplitsCacheInRedis.ts | 22 +++++++++++-------- .../__tests__/SplitsCacheInRedis.spec.ts | 9 ++++++++ .../pluggable/SplitsCachePluggable.ts | 10 ++++----- .../__tests__/SplitsCachePluggable.spec.ts | 8 +++++-- 4 files changed, 33 insertions(+), 16 deletions(-) diff --git a/src/storages/inRedis/SplitsCacheInRedis.ts b/src/storages/inRedis/SplitsCacheInRedis.ts index aeed1c87..77582d23 100644 --- a/src/storages/inRedis/SplitsCacheInRedis.ts +++ b/src/storages/inRedis/SplitsCacheInRedis.ts @@ -10,8 +10,8 @@ import { ISet, _Set, returnListDifference } from '../../utils/lang/sets'; /** * Discard errors for an answer of multiple operations. */ -function processPipelineAnswer(results: Array<[Error | null, T]>): T[] { - return results.reduce((accum: T[], errValuePair: [Error | null, T]) => { +function processPipelineAnswer(results: Array<[Error | null, string]>): string[] { + return results.reduce((accum: string[], errValuePair: [Error | null, string]) => { if (errValuePair[0] === null) accum.push(errValuePair[1]); return accum; }, []); @@ -195,7 +195,7 @@ export class SplitsCacheInRedis extends AbstractSplitsCacheAsync { .then((listOfKeys) => this.redis.pipeline(listOfKeys.map(k => ['get', k])).exec()) .then(processPipelineAnswer) .then((splitDefinitions) => splitDefinitions.map((splitDefinition) => { - return JSON.parse(splitDefinition as string); + return JSON.parse(splitDefinition); })); } @@ -211,13 +211,17 @@ export class SplitsCacheInRedis extends AbstractSplitsCacheAsync { } /** - * Get list of split names related to a given flag set names list. - * The returned promise is resolved with the list of split names, - * or rejected if any wrapper operation fails. + * Get list of feature flag names related to a given list of flag set names. + * The returned promise is resolved with the list of feature flag names per flag set, + * or rejected if the pipelined redis operation fails. */ getNamesByFlagSets(flagSets: string[]): Promise[]> { return this.redis.pipeline(flagSets.map(flagSet => ['smembers', this.keys.buildFlagSetKey(flagSet)])).exec() - .then(processPipelineAnswer) + .then((results) => results.map(([e, value], index) => { + if (e === null) return value; + + this.log.error(LOG_PREFIX + `Could not read result from get members of flag set ${flagSets[index]} due to an error: ${e}`); + })) .then(namesByFlagSets => namesByFlagSets.map(namesByFlagSet => new _Set(namesByFlagSet))); } @@ -235,14 +239,14 @@ export class SplitsCacheInRedis extends AbstractSplitsCacheAsync { ttCount = parseInt(ttCount as string, 10); if (!isFiniteNumber(ttCount) || ttCount < 0) { - this.log.info(LOG_PREFIX + `Could not validate traffic type existance of ${trafficType} due to data corruption of some sorts.`); + this.log.info(LOG_PREFIX + `Could not validate traffic type existence of ${trafficType} due to data corruption of some sorts.`); return false; } return ttCount > 0; }) .catch(e => { - this.log.error(LOG_PREFIX + `Could not validate traffic type existance of ${trafficType} due to an error: ${e}.`); + this.log.error(LOG_PREFIX + `Could not validate traffic type existence of ${trafficType} due to an error: ${e}.`); // If there is an error, bypass the validation so the event can get tracked. return true; }); diff --git a/src/storages/inRedis/__tests__/SplitsCacheInRedis.spec.ts b/src/storages/inRedis/__tests__/SplitsCacheInRedis.spec.ts index 665776c9..cb8093a8 100644 --- a/src/storages/inRedis/__tests__/SplitsCacheInRedis.spec.ts +++ b/src/storages/inRedis/__tests__/SplitsCacheInRedis.spec.ts @@ -173,6 +173,15 @@ describe('SPLITS CACHE REDIS', () => { expect(await cache.getNamesByFlagSets(['x'])).toEqual([new _Set(['ff_one'])]); expect(await cache.getNamesByFlagSets(['o', 'e', 'x'])).toEqual([new _Set(['ff_two']), new _Set(['ff_three']), new _Set(['ff_one'])]); + // @ts-ignore Simulate one error in connection.pipeline().exec() + jest.spyOn(connection, 'pipeline').mockImplementationOnce(() => { + return { + exec: () => Promise.resolve([['error', null], [null, ['ff_three']], [null, ['ff_one']]]), + }; + }); + expect(await cache.getNamesByFlagSets(['o', 'e', 'x'])).toEqual([emptySet, new _Set(['ff_three']), new _Set(['ff_one'])]); + (connection.pipeline as jest.Mock).mockRestore(); + await cache.removeSplit(featureFlagOne.name); expect(await cache.getNamesByFlagSets(['x'])).toEqual([emptySet]); diff --git a/src/storages/pluggable/SplitsCachePluggable.ts b/src/storages/pluggable/SplitsCachePluggable.ts index f485e1a5..9ed55b9f 100644 --- a/src/storages/pluggable/SplitsCachePluggable.ts +++ b/src/storages/pluggable/SplitsCachePluggable.ts @@ -177,14 +177,14 @@ export class SplitsCachePluggable extends AbstractSplitsCacheAsync { } /** - * Get list of split names related to a given flag set names list. - * The returned promise is resolved with the list of split names, - * or rejected if any wrapper operation fails. - */ + * Get list of feature flag names related to a given list of flag set names. + * The returned promise is resolved with the list of feature flag names per flag set. + * It never rejects (If there is a wrapper error for some flag set, an empty set is returned for it). + */ getNamesByFlagSets(flagSets: string[]): Promise[]> { return Promise.all(flagSets.map(flagSet => { const flagSetKey = this.keys.buildFlagSetKey(flagSet); - return this.wrapper.getItems(flagSetKey); + return this.wrapper.getItems(flagSetKey).catch(() => []); })).then(namesByFlagSets => namesByFlagSets.map(namesByFlagSet => new _Set(namesByFlagSet))); } diff --git a/src/storages/pluggable/__tests__/SplitsCachePluggable.spec.ts b/src/storages/pluggable/__tests__/SplitsCachePluggable.spec.ts index 03221de0..ea8aa73e 100644 --- a/src/storages/pluggable/__tests__/SplitsCachePluggable.spec.ts +++ b/src/storages/pluggable/__tests__/SplitsCachePluggable.spec.ts @@ -152,8 +152,8 @@ describe('SPLITS CACHE PLUGGABLE', () => { }); test('flag set cache tests', async () => { - // @ts-ignore - const cache = new SplitsCachePluggable(loggerMock, keysBuilder, wrapperMockFactory(), { groupedFilters: { bySet: ['o', 'n', 'e', 'x'] } }); + const wrapper = wrapperMockFactory(); // @ts-ignore + const cache = new SplitsCachePluggable(loggerMock, keysBuilder, wrapper, { groupedFilters: { bySet: ['o', 'n', 'e', 'x'] } }); const emptySet = new _Set([]); await cache.addSplits([ @@ -179,6 +179,10 @@ describe('SPLITS CACHE PLUGGABLE', () => { expect(await cache.getNamesByFlagSets(['x'])).toEqual([new _Set(['ff_one'])]); expect(await cache.getNamesByFlagSets(['o', 'e', 'x'])).toEqual([new _Set(['ff_two']), new _Set(['ff_three']), new _Set(['ff_one'])]); + // Simulate one error in getItems + wrapper.getItems.mockImplementationOnce(() => Promise.reject('error')); + expect(await cache.getNamesByFlagSets(['o', 'e', 'x'])).toEqual([emptySet, new _Set(['ff_three']), new _Set(['ff_one'])]); + await cache.removeSplit(featureFlagOne.name); expect(await cache.getNamesByFlagSets(['x'])).toEqual([emptySet]); From 9f2fdc0b54b15f4781c95a53b5c7ebc9c4241345 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Wed, 29 Nov 2023 12:24:26 -0300 Subject: [PATCH 19/45] Fix typo --- src/sdkClient/client.ts | 8 +++---- src/sdkManager/index.ts | 6 ++--- ...istance.spec.ts => splitExistence.spec.ts} | 22 +++++++++---------- ...e.spec.ts => trafficTypeExistence.spec.ts} | 18 +++++++-------- src/utils/inputValidation/index.ts | 4 ++-- .../{splitExistance.ts => splitExistence.ts} | 2 +- ...peExistance.ts => trafficTypeExistence.ts} | 8 +++---- 7 files changed, 34 insertions(+), 34 deletions(-) rename src/utils/inputValidation/__tests__/{splitExistance.spec.ts => splitExistence.spec.ts} (81%) rename src/utils/inputValidation/__tests__/{trafficTypeExistance.spec.ts => trafficTypeExistence.spec.ts} (92%) rename src/utils/inputValidation/{splitExistance.ts => splitExistence.ts} (93%) rename src/utils/inputValidation/{trafficTypeExistance.ts => trafficTypeExistence.ts} (81%) diff --git a/src/sdkClient/client.ts b/src/sdkClient/client.ts index 1c10da3a..1663af92 100644 --- a/src/sdkClient/client.ts +++ b/src/sdkClient/client.ts @@ -1,8 +1,8 @@ import { evaluateFeature, evaluateFeatures, evaluateFeaturesByFlagSets } from '../evaluator'; import { thenable } from '../utils/promise/thenable'; import { getMatching, getBucketing } from '../utils/key'; -import { validateSplitExistance } from '../utils/inputValidation/splitExistance'; -import { validateTrafficTypeExistance } from '../utils/inputValidation/trafficTypeExistance'; +import { validateSplitExistence } from '../utils/inputValidation/splitExistence'; +import { validateTrafficTypeExistence } from '../utils/inputValidation/trafficTypeExistence'; import { SDK_NOT_READY } from '../utils/labels'; import { CONTROL, TREATMENT, TREATMENTS, TREATMENT_WITH_CONFIG, TREATMENTS_WITH_CONFIG, TRACK, TREATMENTS_WITH_CONFIG_BY_FLAGSETS, TREATMENTS_BY_FLAGSETS, TREATMENTS_BY_FLAGSET, TREATMENTS_WITH_CONFIG_BY_FLAGSET } from '../utils/constants'; import { IEvaluationResult } from '../evaluator/types'; @@ -133,7 +133,7 @@ export function clientFactory(params: ISdkFactoryContext): SplitIO.IClient | Spl const { treatment, label, changeNumber, config = null } = evaluation; log.info(IMPRESSION, [featureFlagName, matchingKey, treatment, label]); - if (validateSplitExistance(log, readinessManager, featureFlagName, label, invokingMethodName)) { + if (validateSplitExistence(log, readinessManager, featureFlagName, label, invokingMethodName)) { log.info(IMPRESSION_QUEUEING); queue.push({ feature: featureFlagName, @@ -171,7 +171,7 @@ export function clientFactory(params: ISdkFactoryContext): SplitIO.IClient | Spl }; // This may be async but we only warn, we don't actually care if it is valid or not in terms of queueing the event. - validateTrafficTypeExistance(log, readinessManager, storage.splits, mode, trafficTypeName, 'track'); + validateTrafficTypeExistence(log, readinessManager, storage.splits, mode, trafficTypeName, 'track'); const result = eventTracker.track(eventData, size); diff --git a/src/sdkManager/index.ts b/src/sdkManager/index.ts index 1fec8784..5ca1386e 100644 --- a/src/sdkManager/index.ts +++ b/src/sdkManager/index.ts @@ -1,7 +1,7 @@ import { objectAssign } from '../utils/lang/objectAssign'; import { thenable } from '../utils/promise/thenable'; import { find } from '../utils/lang'; -import { validateSplit, validateSplitExistance, validateIfNotDestroyed, validateIfOperational } from '../utils/inputValidation'; +import { validateSplit, validateSplitExistence, validateIfNotDestroyed, validateIfOperational } from '../utils/inputValidation'; import { ISplitsCacheAsync, ISplitsCacheSync } from '../storages/types'; import { ISdkReadinessManager } from '../readiness/types'; import { ISplit } from '../dtos/types'; @@ -71,12 +71,12 @@ export function sdkManagerFactory null).then(result => { // handle possible rejections when using pluggable storage - validateSplitExistance(log, readinessManager, splitName, result, SPLIT_FN_LABEL); + validateSplitExistence(log, readinessManager, splitName, result, SPLIT_FN_LABEL); return objectToView(result); }); } - validateSplitExistance(log, readinessManager, splitName, split, SPLIT_FN_LABEL); + validateSplitExistence(log, readinessManager, splitName, split, SPLIT_FN_LABEL); return objectToView(split); }, diff --git a/src/utils/inputValidation/__tests__/splitExistance.spec.ts b/src/utils/inputValidation/__tests__/splitExistence.spec.ts similarity index 81% rename from src/utils/inputValidation/__tests__/splitExistance.spec.ts rename to src/utils/inputValidation/__tests__/splitExistence.spec.ts index 7d0d7b39..9d78df9e 100644 --- a/src/utils/inputValidation/__tests__/splitExistance.spec.ts +++ b/src/utils/inputValidation/__tests__/splitExistence.spec.ts @@ -3,7 +3,7 @@ import * as LabelConstants from '../../labels'; import { loggerMock } from '../../../logger/__tests__/sdkLogger.mock'; -import { validateSplitExistance } from '../splitExistance'; +import { validateSplitExistence } from '../splitExistence'; import { IReadinessManager } from '../../../readiness/types'; import { WARN_NOT_EXISTENT_SPLIT } from '../../../logger/constants'; @@ -17,11 +17,11 @@ describe('Split existence (special case)', () => { isReady: jest.fn(() => false) // Fake the signal for the non ready SDK } as IReadinessManager; - expect(validateSplitExistance(loggerMock, readinessManagerMock, 'some_split', {}, 'test_method')).toBe(true); // Should always return true when the SDK is not ready. - expect(validateSplitExistance(loggerMock, readinessManagerMock, 'some_split', null, 'test_method')).toBe(true); // Should always return true when the SDK is not ready. - expect(validateSplitExistance(loggerMock, readinessManagerMock, 'some_split', undefined, 'test_method')).toBe(true); // Should always return true when the SDK is not ready. - expect(validateSplitExistance(loggerMock, readinessManagerMock, 'some_split', 'a label', 'test_method')).toBe(true); // Should always return true when the SDK is not ready. - expect(validateSplitExistance(loggerMock, readinessManagerMock, 'some_split', LabelConstants.SPLIT_NOT_FOUND, 'test_method')).toBe(true); // Should always return true when the SDK is not ready. + expect(validateSplitExistence(loggerMock, readinessManagerMock, 'some_split', {}, 'test_method')).toBe(true); // Should always return true when the SDK is not ready. + expect(validateSplitExistence(loggerMock, readinessManagerMock, 'some_split', null, 'test_method')).toBe(true); // Should always return true when the SDK is not ready. + expect(validateSplitExistence(loggerMock, readinessManagerMock, 'some_split', undefined, 'test_method')).toBe(true); // Should always return true when the SDK is not ready. + expect(validateSplitExistence(loggerMock, readinessManagerMock, 'some_split', 'a label', 'test_method')).toBe(true); // Should always return true when the SDK is not ready. + expect(validateSplitExistence(loggerMock, readinessManagerMock, 'some_split', LabelConstants.SPLIT_NOT_FOUND, 'test_method')).toBe(true); // Should always return true when the SDK is not ready. expect(loggerMock.warn).not.toBeCalled(); // There should have been no warning logs since the SDK was not ready yet. expect(loggerMock.error).not.toBeCalled(); // There should have been no error logs since the SDK was not ready yet. @@ -29,15 +29,15 @@ describe('Split existence (special case)', () => { // Prepare the mock to fake that the SDK is ready now. (readinessManagerMock.isReady as jest.Mock).mockImplementation(() => true); - expect(validateSplitExistance(loggerMock, readinessManagerMock, 'other_split', {}, 'other_method')).toBe(true); // Should return true if it receives a Split Object instead of null (when the object is not found, for manager). - expect(validateSplitExistance(loggerMock, readinessManagerMock, 'other_split', 'a label', 'other_method')).toBe(true); // Should return true if it receives a Label and it is not split not found (when the Split was not found on the storage, for client). + expect(validateSplitExistence(loggerMock, readinessManagerMock, 'other_split', {}, 'other_method')).toBe(true); // Should return true if it receives a Split Object instead of null (when the object is not found, for manager). + expect(validateSplitExistence(loggerMock, readinessManagerMock, 'other_split', 'a label', 'other_method')).toBe(true); // Should return true if it receives a Label and it is not split not found (when the Split was not found on the storage, for client). expect(loggerMock.warn).not.toBeCalled(); // There should have been no warning logs since the values we used so far were considered valid. expect(loggerMock.error).not.toBeCalled(); // There should have been no error logs since the values we used so far were considered valid. - expect(validateSplitExistance(loggerMock, readinessManagerMock, 'other_split', null, 'other_method')).toBe(false); // Should return false if it receives a non-truthy value as a split object or label - expect(validateSplitExistance(loggerMock, readinessManagerMock, 'other_split', undefined, 'other_method')).toBe(false); // Should return false if it receives a non-truthy value as a split object or label - expect(validateSplitExistance(loggerMock, readinessManagerMock, 'other_split', LabelConstants.SPLIT_NOT_FOUND, 'other_method')).toBe(false); // Should return false if it receives a label but it is the split not found one. + expect(validateSplitExistence(loggerMock, readinessManagerMock, 'other_split', null, 'other_method')).toBe(false); // Should return false if it receives a non-truthy value as a split object or label + expect(validateSplitExistence(loggerMock, readinessManagerMock, 'other_split', undefined, 'other_method')).toBe(false); // Should return false if it receives a non-truthy value as a split object or label + expect(validateSplitExistence(loggerMock, readinessManagerMock, 'other_split', LabelConstants.SPLIT_NOT_FOUND, 'other_method')).toBe(false); // Should return false if it receives a label but it is the split not found one. expect(loggerMock.warn).toBeCalledTimes(3); // It should have logged 3 warnings, one per each time we called it loggerMock.warn.mock.calls.forEach(call => expect(call).toEqual([WARN_NOT_EXISTENT_SPLIT, ['other_method', 'other_split']])); // Warning logs should have the correct message. diff --git a/src/utils/inputValidation/__tests__/trafficTypeExistance.spec.ts b/src/utils/inputValidation/__tests__/trafficTypeExistence.spec.ts similarity index 92% rename from src/utils/inputValidation/__tests__/trafficTypeExistance.spec.ts rename to src/utils/inputValidation/__tests__/trafficTypeExistence.spec.ts index ad19302f..a71912cf 100644 --- a/src/utils/inputValidation/__tests__/trafficTypeExistance.spec.ts +++ b/src/utils/inputValidation/__tests__/trafficTypeExistence.spec.ts @@ -28,9 +28,9 @@ const splitsCacheMock = { } as ISplitsCacheBase & { trafficTypeExists: jest.Mock }; /** Test target */ -import { validateTrafficTypeExistance } from '../trafficTypeExistance'; +import { validateTrafficTypeExistence } from '../trafficTypeExistence'; -describe('validateTrafficTypeExistance', () => { +describe('validateTrafficTypeExistence', () => { afterEach(() => { loggerMock.mockClear(); @@ -39,14 +39,14 @@ describe('validateTrafficTypeExistance', () => { test('Should return true without going to the storage and log nothing if the SDK is not ready or in localhost mode', () => { // Not ready and not localstorage - expect(validateTrafficTypeExistance(loggerMock, readinessManagerMock, splitsCacheMock, STANDALONE_MODE, 'test_tt', 'test_method')).toBe(true); // If the SDK is not ready yet, it will return true. + expect(validateTrafficTypeExistence(loggerMock, readinessManagerMock, splitsCacheMock, STANDALONE_MODE, 'test_tt', 'test_method')).toBe(true); // If the SDK is not ready yet, it will return true. expect(splitsCacheMock.trafficTypeExists).not.toBeCalled(); // If the SDK is not ready yet, it does not try to go to the storage. expect(loggerMock.error).not.toBeCalled(); // If the SDK is not ready yet, it will not log any errors. expect(loggerMock.error).not.toBeCalled(); // If the SDK is not ready yet, it will not log any errors. // Ready and in localstorage mode. readinessManagerMock.isReady.mockImplementation(() => true); - expect(validateTrafficTypeExistance(loggerMock, readinessManagerMock, splitsCacheMock, LOCALHOST_MODE, 'test_tt', 'test_method')).toBe(true); // If the SDK is in localhost mode, it will return true. + expect(validateTrafficTypeExistence(loggerMock, readinessManagerMock, splitsCacheMock, LOCALHOST_MODE, 'test_tt', 'test_method')).toBe(true); // If the SDK is in localhost mode, it will return true. expect(splitsCacheMock.trafficTypeExists).not.toBeCalled(); // If the SDK is in localhost mode, it does not try to go to the storage. expect(loggerMock.warn).not.toBeCalled(); // If the SDK is in localhost mode, it will not log any warnings. expect(loggerMock.error).not.toBeCalled(); // If the SDK is in localhost mode, it will not log any errors. @@ -56,7 +56,7 @@ describe('validateTrafficTypeExistance', () => { // Ready, standalone, and the TT exists in the storage. readinessManagerMock.isReady.mockImplementation(() => true); - expect(validateTrafficTypeExistance(loggerMock, readinessManagerMock, splitsCacheMock, STANDALONE_MODE, TEST_EXISTENT_TT, 'test_method')).toBe(true); // If the SDK is in condition to validate but the TT exists, it will return true. + expect(validateTrafficTypeExistence(loggerMock, readinessManagerMock, splitsCacheMock, STANDALONE_MODE, TEST_EXISTENT_TT, 'test_method')).toBe(true); // If the SDK is in condition to validate but the TT exists, it will return true. expect(splitsCacheMock.trafficTypeExists.mock.calls).toEqual([[TEST_EXISTENT_TT]]); // If the SDK is in condition to validate, it checks that TT existence with the storage. expect(loggerMock.warn).not.toBeCalled(); // If the SDK is in condition to validate but the TT exists, it will not log any warnings. expect(loggerMock.error).not.toBeCalled(); // If the SDK is in condition to validate but the TT exists, it will not log any errors. @@ -64,16 +64,16 @@ describe('validateTrafficTypeExistance', () => { test('Should return false and log warning if SDK Ready, not localhost mode and the traffic type does NOT exist in the storage', () => { // Ready, standalone, and the TT not exists in the storage. - expect(validateTrafficTypeExistance(loggerMock, readinessManagerMock, splitsCacheMock, STANDALONE_MODE, TEST_NOT_EXISTENT_TT, 'test_method_y')).toBe(false); // If the SDK is in condition to validate but the TT does not exist in the storage, it will return false. + expect(validateTrafficTypeExistence(loggerMock, readinessManagerMock, splitsCacheMock, STANDALONE_MODE, TEST_NOT_EXISTENT_TT, 'test_method_y')).toBe(false); // If the SDK is in condition to validate but the TT does not exist in the storage, it will return false. expect(splitsCacheMock.trafficTypeExists.mock.calls).toEqual([[TEST_NOT_EXISTENT_TT]]); // If the SDK is in condition to validate, it checks that TT existence with the storage. expect(loggerMock.warn).toBeCalledWith(WARN_NOT_EXISTENT_TT, ['test_method_y', TEST_NOT_EXISTENT_TT]); // If the SDK is in condition to validate but the TT does not exist in the storage, it will log the expected warning. expect(loggerMock.error).not.toBeCalled(); // It logged a warning so no errors should be logged. }); - test('validateTrafficTypeExistance w/async storage - If the storage is async but the SDK is in condition to validate, it will validate that the TT exists on the storage', async () => { + test('validateTrafficTypeExistence w/async storage - If the storage is async but the SDK is in condition to validate, it will validate that the TT exists on the storage', async () => { // Ready, standalone, the TT exists in the storage. - const validationPromise = validateTrafficTypeExistance(loggerMock, readinessManagerMock, splitsCacheMock, STANDALONE_MODE, TEST_EXISTENT_ASYNC_TT, 'test_method_z'); + const validationPromise = validateTrafficTypeExistence(loggerMock, readinessManagerMock, splitsCacheMock, STANDALONE_MODE, TEST_EXISTENT_ASYNC_TT, 'test_method_z'); expect(thenable(validationPromise)).toBe(true); // If the storage is async, it should also return a promise. expect(splitsCacheMock.trafficTypeExists.mock.calls).toEqual([[TEST_EXISTENT_ASYNC_TT]]); // If the SDK is in condition to validate, it checks that TT existence with the async storage. expect(loggerMock.warn).not.toBeCalled(); // We are still fetching the data from the storage, no logs yet. @@ -88,7 +88,7 @@ describe('validateTrafficTypeExistance', () => { // Second round, a TT that does not exist on the asnyc storage splitsCacheMock.trafficTypeExists.mockClear(); - const validationPromise2 = validateTrafficTypeExistance(loggerMock, readinessManagerMock, splitsCacheMock, STANDALONE_MODE, TEST_NOT_EXISTENT_ASYNC_TT, 'test_method_z'); + const validationPromise2 = validateTrafficTypeExistence(loggerMock, readinessManagerMock, splitsCacheMock, STANDALONE_MODE, TEST_NOT_EXISTENT_ASYNC_TT, 'test_method_z'); expect(thenable(validationPromise2)).toBe(true); // If the storage is async, it should also return a promise. expect(splitsCacheMock.trafficTypeExists.mock.calls).toEqual([[TEST_NOT_EXISTENT_ASYNC_TT]]); // If the SDK is in condition to validate, it checks that TT existence with the async storage. expect(loggerMock.warn).not.toBeCalled(); // We are still fetching the data from the storage, no logs yet. diff --git a/src/utils/inputValidation/index.ts b/src/utils/inputValidation/index.ts index 31835a24..93b09ab7 100644 --- a/src/utils/inputValidation/index.ts +++ b/src/utils/inputValidation/index.ts @@ -8,6 +8,6 @@ export { validateSplit } from './split'; export { validateSplits } from './splits'; export { validateTrafficType } from './trafficType'; export { validateIfNotDestroyed, validateIfOperational } from './isOperational'; -export { validateSplitExistance } from './splitExistance'; -export { validateTrafficTypeExistance } from './trafficTypeExistance'; +export { validateSplitExistence } from './splitExistence'; +export { validateTrafficTypeExistence } from './trafficTypeExistence'; export { validatePreloadedData } from './preloadedData'; diff --git a/src/utils/inputValidation/splitExistance.ts b/src/utils/inputValidation/splitExistence.ts similarity index 93% rename from src/utils/inputValidation/splitExistance.ts rename to src/utils/inputValidation/splitExistence.ts index ddafa767..2f3105f9 100644 --- a/src/utils/inputValidation/splitExistance.ts +++ b/src/utils/inputValidation/splitExistence.ts @@ -7,7 +7,7 @@ import { WARN_NOT_EXISTENT_SPLIT } from '../../logger/constants'; * This is defined here and in this format mostly because of the logger and the fact that it's considered a validation at product level. * But it's not going to run on the input validation layer. In any case, the most compeling reason to use it as we do is to avoid going to Redis and get a split twice. */ -export function validateSplitExistance(log: ILogger, readinessManager: IReadinessManager, splitName: string, labelOrSplitObj: any, method: string): boolean { +export function validateSplitExistence(log: ILogger, readinessManager: IReadinessManager, splitName: string, labelOrSplitObj: any, method: string): boolean { if (readinessManager.isReady()) { // Only if it's ready we validate this, otherwise it may just be that the SDK is not ready yet. if (labelOrSplitObj === SPLIT_NOT_FOUND || labelOrSplitObj == null) { log.warn(WARN_NOT_EXISTENT_SPLIT, [method, splitName]); diff --git a/src/utils/inputValidation/trafficTypeExistance.ts b/src/utils/inputValidation/trafficTypeExistence.ts similarity index 81% rename from src/utils/inputValidation/trafficTypeExistance.ts rename to src/utils/inputValidation/trafficTypeExistence.ts index 743ceb8f..9619f197 100644 --- a/src/utils/inputValidation/trafficTypeExistance.ts +++ b/src/utils/inputValidation/trafficTypeExistence.ts @@ -7,14 +7,14 @@ import { MaybeThenable } from '../../dtos/types'; import { ILogger } from '../../logger/types'; import { WARN_NOT_EXISTENT_TT } from '../../logger/constants'; -function logTTExistanceWarning(log: ILogger, maybeTT: string, method: string) { +function logTTExistenceWarning(log: ILogger, maybeTT: string, method: string) { log.warn(WARN_NOT_EXISTENT_TT, [method, maybeTT]); } /** * Separated from the previous method since on some cases it'll be async. */ -export function validateTrafficTypeExistance(log: ILogger, readinessManager: IReadinessManager, splitsCache: ISplitsCacheBase, mode: SDKMode, maybeTT: string, method: string): MaybeThenable { +export function validateTrafficTypeExistence(log: ILogger, readinessManager: IReadinessManager, splitsCache: ISplitsCacheBase, mode: SDKMode, maybeTT: string, method: string): MaybeThenable { // If not ready or in localhost mode, we won't run the validation if (!readinessManager.isReady() || mode === LOCALHOST_MODE) return true; @@ -23,11 +23,11 @@ export function validateTrafficTypeExistance(log: ILogger, readinessManager: IRe if (thenable(res)) { return res.then(function (isValid) { - if (!isValid) logTTExistanceWarning(log, maybeTT, method); + if (!isValid) logTTExistenceWarning(log, maybeTT, method); return isValid; // propagate result }); } else { - if (!res) logTTExistanceWarning(log, maybeTT, method); + if (!res) logTTExistenceWarning(log, maybeTT, method); return res; } } From 8bf298b1041dc66e7397731994efbad4c375a2bf Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Wed, 29 Nov 2023 16:40:49 -0300 Subject: [PATCH 20/45] Wrap pipeline Redis operations --- src/storages/inRedis/RedisAdapter.ts | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/src/storages/inRedis/RedisAdapter.ts b/src/storages/inRedis/RedisAdapter.ts index 7beb2988..6f965ea7 100644 --- a/src/storages/inRedis/RedisAdapter.ts +++ b/src/storages/inRedis/RedisAdapter.ts @@ -8,7 +8,7 @@ import { timeout } from '../../utils/promise/timeout'; const LOG_PREFIX = 'storage:redis-adapter: '; // If we ever decide to fully wrap every method, there's a Commander.getBuiltinCommands from ioredis. -const METHODS_TO_PROMISE_WRAP = ['set', 'exec', 'del', 'get', 'keys', 'sadd', 'srem', 'sismember', 'smembers', 'incr', 'rpush', 'pipeline', 'expire', 'mget', 'lrange', 'ltrim', 'hset']; +const METHODS_TO_PROMISE_WRAP = ['set', 'exec', 'del', 'get', 'keys', 'sadd', 'srem', 'sismember', 'smembers', 'incr', 'rpush', 'expire', 'mget', 'lrange', 'ltrim', 'hset', 'pipelineExec']; // Not part of the settings since it'll vary on each storage. We should be removing storage specific logic from elsewhere. const DEFAULT_OPTIONS = { @@ -52,10 +52,16 @@ export class RedisAdapter extends ioredis { this._setDisconnectWrapper(); } + pipelineExec(commands?: (string | number)[][]): Promise> { + // @ts-ignore + return this.pipeline(commands).exec(); + } + _listenToEvents() { this.once('ready', () => { const commandsCount = this._notReadyCommandsQueue ? this._notReadyCommandsQueue.length : 0; this.log.info(LOG_PREFIX + `Redis connection established. Queued commands: ${commandsCount}.`); + this._notReadyCommandsQueue && this._notReadyCommandsQueue.forEach(queued => { this.log.info(LOG_PREFIX + `Executing queued ${queued.name} command.`); queued.command().then(queued.resolve).catch(queued.reject); @@ -71,29 +77,28 @@ export class RedisAdapter extends ioredis { _setTimeoutWrappers() { const instance: Record = this; - METHODS_TO_PROMISE_WRAP.forEach(method => { - const originalMethod = instance[method]; + METHODS_TO_PROMISE_WRAP.forEach(methodName => { + const originalMethod = instance[methodName]; - instance[method] = function () { + instance[methodName] = function () { const params = arguments; function commandWrapper() { - instance.log.debug(LOG_PREFIX + `Executing ${method}.`); - // Return original method + instance.log.debug(`${LOG_PREFIX}Executing ${methodName}.`); const result = originalMethod.apply(instance, params); if (thenable(result)) { // For handling pending commands on disconnect, add to the set and remove once finished. // On sync commands there's no need, only thenables. instance._runningCommands.add(result); - const cleanUpRunningCommandsCb = function () { + const cleanUpRunningCommandsCb = () => { instance._runningCommands.delete(result); }; // Both success and error remove from queue. result.then(cleanUpRunningCommandsCb, cleanUpRunningCommandsCb); return timeout(instance._options.operationTimeout, result).catch(err => { - instance.log.error(LOG_PREFIX + `${method} operation threw an error or exceeded configured timeout of ${instance._options.operationTimeout}ms. Message: ${err}`); + instance.log.error(`${LOG_PREFIX}${methodName} operation threw an error or exceeded configured timeout of ${instance._options.operationTimeout}ms. Message: ${err}`); // Handling is not the adapter responsibility. throw err; }); @@ -108,7 +113,7 @@ export class RedisAdapter extends ioredis { resolve: res, reject: rej, command: commandWrapper, - name: method.toUpperCase() + name: methodName.toUpperCase() }); }); } else { @@ -124,7 +129,7 @@ export class RedisAdapter extends ioredis { instance.disconnect = function disconnect(...params: []) { - setTimeout(function deferedDisconnect() { + setTimeout(function deferredDisconnect() { if (instance._runningCommands.size > 0) { instance.log.info(LOG_PREFIX + `Attempting to disconnect but there are ${instance._runningCommands.size} commands still waiting for resolution. Defering disconnection until those finish.`); From d5202b12528bbdc3fede5f4b6130bc335bbcef8c Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Wed, 29 Nov 2023 16:42:31 -0300 Subject: [PATCH 21/45] Update Redis storages to use the wrapped pipeline method. Update tests to provide the RedisAdapter to Redis caches, in order to make the validation more robust --- .../__tests__/sdkReadinessManager.spec.ts | 2 +- .../__tests__/index.asyncCache.spec.ts | 6 +-- src/storages/inRedis/EventsCacheInRedis.ts | 6 +-- .../inRedis/ImpressionCountsCacheInRedis.ts | 13 +++--- .../inRedis/ImpressionsCacheInRedis.ts | 6 +-- src/storages/inRedis/RedisAdapter.ts | 6 +-- src/storages/inRedis/SegmentsCacheInRedis.ts | 6 +-- src/storages/inRedis/SplitsCacheInRedis.ts | 10 ++--- src/storages/inRedis/TelemetryCacheInRedis.ts | 4 +- .../inRedis/UniqueKeysCacheInRedis.ts | 6 +-- .../__tests__/EventsCacheInRedis.spec.ts | 6 +-- .../ImpressionCountsCacheInRedis.spec.ts | 44 +++++++++---------- .../__tests__/ImpressionsCacheInRedis.spec.ts | 4 +- .../inRedis/__tests__/RedisAdapter.spec.ts | 2 +- .../__tests__/SegmentsCacheInRedis.spec.ts | 10 ++--- .../__tests__/SplitsCacheInRedis.spec.ts | 32 +++++++------- .../__tests__/TelemetryCacheInRedis.spec.ts | 6 +-- .../__tests__/UniqueKeysCacheInRedis.spec.ts | 20 +++++---- src/utils/redis/RedisMock.ts | 12 +---- 19 files changed, 94 insertions(+), 107 deletions(-) diff --git a/src/readiness/__tests__/sdkReadinessManager.spec.ts b/src/readiness/__tests__/sdkReadinessManager.spec.ts index 3c93b7a6..9c407052 100644 --- a/src/readiness/__tests__/sdkReadinessManager.spec.ts +++ b/src/readiness/__tests__/sdkReadinessManager.spec.ts @@ -49,7 +49,7 @@ describe('SDK Readiness Manager - Event emitter', () => { expect(sdkStatus[propName]).toBeTruthy(); // The sdkStatus exposes all minimal EventEmitter functionality. }); - expect(typeof sdkStatus['ready']).toBe('function'); // The sdkStatus exposes a .ready() function. + expect(typeof sdkStatus.ready).toBe('function'); // The sdkStatus exposes a .ready() function. expect(typeof sdkStatus.Event).toBe('object'); // It also exposes the Event map, expect(sdkStatus.Event.SDK_READY).toBe(SDK_READY); // which contains the constants for the events, for backwards compatibility. diff --git a/src/sdkManager/__tests__/index.asyncCache.spec.ts b/src/sdkManager/__tests__/index.asyncCache.spec.ts index 4081c10b..71c656c2 100644 --- a/src/sdkManager/__tests__/index.asyncCache.spec.ts +++ b/src/sdkManager/__tests__/index.asyncCache.spec.ts @@ -1,4 +1,3 @@ -import Redis from 'ioredis'; import splitObject from './mocks/input.json'; import splitView from './mocks/output.json'; import { sdkManagerFactory } from '../index'; @@ -9,6 +8,7 @@ import { KeyBuilderSS } from '../../storages/KeyBuilderSS'; import { ISdkReadinessManager } from '../../readiness/types'; import { loggerMock } from '../../logger/__tests__/sdkLogger.mock'; import { metadata } from '../../storages/__tests__/KeyBuilder.spec'; +import { RedisAdapter } from '../../storages/inRedis/RedisAdapter'; // @ts-expect-error const sdkReadinessManagerMock = { @@ -28,7 +28,7 @@ describe('MANAGER API', () => { test('Async cache (In Redis)', async () => { /** Setup: create manager */ - const connection = new Redis({}); + const connection = new RedisAdapter(loggerMock); const cache = new SplitsCacheInRedis(loggerMock, keys, connection); const manager = sdkManagerFactory(loggerMock, cache, sdkReadinessManagerMock); await cache.clear(); @@ -70,7 +70,7 @@ describe('MANAGER API', () => { /** Teardown */ await cache.removeSplit(splitObject.name); - await connection.quit(); + await connection.disconnect(); }); test('Async cache with error', async () => { diff --git a/src/storages/inRedis/EventsCacheInRedis.ts b/src/storages/inRedis/EventsCacheInRedis.ts index 20ae13af..ecd14a32 100644 --- a/src/storages/inRedis/EventsCacheInRedis.ts +++ b/src/storages/inRedis/EventsCacheInRedis.ts @@ -1,19 +1,19 @@ import { IEventsCacheAsync } from '../types'; import { IMetadata } from '../../dtos/types'; -import { Redis } from 'ioredis'; import { SplitIO } from '../../types'; import { ILogger } from '../../logger/types'; import { LOG_PREFIX } from './constants'; import { StoredEventWithMetadata } from '../../sync/submitters/types'; +import type { RedisAdapter } from './RedisAdapter'; export class EventsCacheInRedis implements IEventsCacheAsync { private readonly log: ILogger; private readonly key: string; - private readonly redis: Redis; + private readonly redis: RedisAdapter; private readonly metadata: IMetadata; - constructor(log: ILogger, key: string, redis: Redis, metadata: IMetadata) { + constructor(log: ILogger, key: string, redis: RedisAdapter, metadata: IMetadata) { this.log = log; this.key = key; this.redis = redis; diff --git a/src/storages/inRedis/ImpressionCountsCacheInRedis.ts b/src/storages/inRedis/ImpressionCountsCacheInRedis.ts index b0c563c0..4d5150e4 100644 --- a/src/storages/inRedis/ImpressionCountsCacheInRedis.ts +++ b/src/storages/inRedis/ImpressionCountsCacheInRedis.ts @@ -1,19 +1,19 @@ -import { Redis } from 'ioredis'; import { ILogger } from '../../logger/types'; import { ImpressionCountsPayload } from '../../sync/submitters/types'; import { forOwn } from '../../utils/lang'; import { ImpressionCountsCacheInMemory } from '../inMemory/ImpressionCountsCacheInMemory'; import { LOG_PREFIX, REFRESH_RATE, TTL_REFRESH } from './constants'; +import type { RedisAdapter } from './RedisAdapter'; export class ImpressionCountsCacheInRedis extends ImpressionCountsCacheInMemory { private readonly log: ILogger; private readonly key: string; - private readonly redis: Redis; + private readonly redis: RedisAdapter; private readonly refreshRate: number; private intervalId: any; - constructor(log: ILogger, key: string, redis: Redis, impressionCountsCacheSize?: number, refreshRate = REFRESH_RATE) { + constructor(log: ILogger, key: string, redis: RedisAdapter, impressionCountsCacheSize?: number, refreshRate = REFRESH_RATE) { super(impressionCountsCacheSize); this.log = log; this.key = key; @@ -27,11 +27,8 @@ export class ImpressionCountsCacheInRedis extends ImpressionCountsCacheInMemory const keys = Object.keys(counts); if (!keys.length) return Promise.resolve(false); - const pipeline = this.redis.pipeline(); - keys.forEach(key => { - pipeline.hincrby(this.key, key, counts[key]); - }); - return pipeline.exec() + // @ts-ignore + return this.redis.pipelineExec(keys.map(key => ['hincrby', this.key, key, counts[key]])) .then(data => { // If this is the creation of the key on Redis, set the expiration for it in 3600 seconds. if (data.length && data.length === keys.length) { diff --git a/src/storages/inRedis/ImpressionsCacheInRedis.ts b/src/storages/inRedis/ImpressionsCacheInRedis.ts index 12ee277c..4ac0acaa 100644 --- a/src/storages/inRedis/ImpressionsCacheInRedis.ts +++ b/src/storages/inRedis/ImpressionsCacheInRedis.ts @@ -1,10 +1,10 @@ import { IImpressionsCacheAsync } from '../types'; import { IMetadata } from '../../dtos/types'; import { ImpressionDTO } from '../../types'; -import { Redis } from 'ioredis'; import { StoredImpressionWithMetadata } from '../../sync/submitters/types'; import { ILogger } from '../../logger/types'; import { impressionsToJSON } from '../utils'; +import type { RedisAdapter } from './RedisAdapter'; const IMPRESSIONS_TTL_REFRESH = 3600; // 1 hr @@ -12,10 +12,10 @@ export class ImpressionsCacheInRedis implements IImpressionsCacheAsync { private readonly log: ILogger; private readonly key: string; - private readonly redis: Redis; + private readonly redis: RedisAdapter; private readonly metadata: IMetadata; - constructor(log: ILogger, key: string, redis: Redis, metadata: IMetadata) { + constructor(log: ILogger, key: string, redis: RedisAdapter, metadata: IMetadata) { this.log = log; this.key = key; this.redis = redis; diff --git a/src/storages/inRedis/RedisAdapter.ts b/src/storages/inRedis/RedisAdapter.ts index 6f965ea7..f4077ac6 100644 --- a/src/storages/inRedis/RedisAdapter.ts +++ b/src/storages/inRedis/RedisAdapter.ts @@ -8,7 +8,7 @@ import { timeout } from '../../utils/promise/timeout'; const LOG_PREFIX = 'storage:redis-adapter: '; // If we ever decide to fully wrap every method, there's a Commander.getBuiltinCommands from ioredis. -const METHODS_TO_PROMISE_WRAP = ['set', 'exec', 'del', 'get', 'keys', 'sadd', 'srem', 'sismember', 'smembers', 'incr', 'rpush', 'expire', 'mget', 'lrange', 'ltrim', 'hset', 'pipelineExec']; +const METHODS_TO_PROMISE_WRAP = ['set', 'exec', 'del', 'get', 'keys', 'sadd', 'srem', 'sismember', 'smembers', 'incr', 'rpush', 'expire', 'mget', 'lrange', 'ltrim', 'hset', 'hincrby', 'popNRaw', 'flushdb', 'pipelineExec']; // Not part of the settings since it'll vary on each storage. We should be removing storage specific logic from elsewhere. const DEFAULT_OPTIONS = { @@ -38,7 +38,7 @@ export class RedisAdapter extends ioredis { private _notReadyCommandsQueue?: IRedisCommand[]; private _runningCommands: ISet>; - constructor(log: ILogger, storageSettings: Record) { + constructor(log: ILogger, storageSettings?: Record) { const options = RedisAdapter._defineOptions(storageSettings); // Call the ioredis constructor super(...RedisAdapter._defineLibrarySettings(options)); @@ -182,7 +182,7 @@ export class RedisAdapter extends ioredis { /** * Parses the options into what we care about. */ - static _defineOptions({ connectionTimeout, operationTimeout, url, host, port, db, pass, tls }: Record) { + static _defineOptions({ connectionTimeout, operationTimeout, url, host, port, db, pass, tls }: Record = {}) { const parsedOptions = { connectionTimeout, operationTimeout, url, host, port, db, pass, tls }; diff --git a/src/storages/inRedis/SegmentsCacheInRedis.ts b/src/storages/inRedis/SegmentsCacheInRedis.ts index a144e2b7..3a18b535 100644 --- a/src/storages/inRedis/SegmentsCacheInRedis.ts +++ b/src/storages/inRedis/SegmentsCacheInRedis.ts @@ -1,17 +1,17 @@ -import { Redis } from 'ioredis'; import { ILogger } from '../../logger/types'; import { isNaNNumber } from '../../utils/lang'; import { LOG_PREFIX } from '../inLocalStorage/constants'; import { KeyBuilderSS } from '../KeyBuilderSS'; import { ISegmentsCacheAsync } from '../types'; +import type { RedisAdapter } from './RedisAdapter'; export class SegmentsCacheInRedis implements ISegmentsCacheAsync { private readonly log: ILogger; - private readonly redis: Redis; + private readonly redis: RedisAdapter; private readonly keys: KeyBuilderSS; - constructor(log: ILogger, keys: KeyBuilderSS, redis: Redis) { + constructor(log: ILogger, keys: KeyBuilderSS, redis: RedisAdapter) { this.log = log; this.redis = redis; this.keys = keys; diff --git a/src/storages/inRedis/SplitsCacheInRedis.ts b/src/storages/inRedis/SplitsCacheInRedis.ts index 77582d23..ae139773 100644 --- a/src/storages/inRedis/SplitsCacheInRedis.ts +++ b/src/storages/inRedis/SplitsCacheInRedis.ts @@ -1,11 +1,11 @@ import { isFiniteNumber, isNaNNumber } from '../../utils/lang'; import { KeyBuilderSS } from '../KeyBuilderSS'; -import { Redis } from 'ioredis'; import { ILogger } from '../../logger/types'; import { LOG_PREFIX } from './constants'; import { ISplit, ISplitFiltersValidation } from '../../dtos/types'; import { AbstractSplitsCacheAsync } from '../AbstractSplitsCacheAsync'; import { ISet, _Set, returnListDifference } from '../../utils/lang/sets'; +import type { RedisAdapter } from './RedisAdapter'; /** * Discard errors for an answer of multiple operations. @@ -24,12 +24,12 @@ function processPipelineAnswer(results: Array<[Error | null, string]>): string[] export class SplitsCacheInRedis extends AbstractSplitsCacheAsync { private readonly log: ILogger; - private readonly redis: Redis; + private readonly redis: RedisAdapter; private readonly keys: KeyBuilderSS; private redisError?: string; private readonly flagSetsFilter: string[]; - constructor(log: ILogger, keys: KeyBuilderSS, redis: Redis, splitFiltersValidation?: ISplitFiltersValidation) { + constructor(log: ILogger, keys: KeyBuilderSS, redis: RedisAdapter, splitFiltersValidation?: ISplitFiltersValidation) { super(); this.log = log; this.redis = redis; @@ -192,7 +192,7 @@ export class SplitsCacheInRedis extends AbstractSplitsCacheAsync { */ getAll(): Promise { return this.redis.keys(this.keys.searchPatternForSplitKeys()) - .then((listOfKeys) => this.redis.pipeline(listOfKeys.map(k => ['get', k])).exec()) + .then((listOfKeys) => this.redis.pipelineExec(listOfKeys.map(k => ['get', k]))) .then(processPipelineAnswer) .then((splitDefinitions) => splitDefinitions.map((splitDefinition) => { return JSON.parse(splitDefinition); @@ -216,7 +216,7 @@ export class SplitsCacheInRedis extends AbstractSplitsCacheAsync { * or rejected if the pipelined redis operation fails. */ getNamesByFlagSets(flagSets: string[]): Promise[]> { - return this.redis.pipeline(flagSets.map(flagSet => ['smembers', this.keys.buildFlagSetKey(flagSet)])).exec() + return this.redis.pipelineExec(flagSets.map(flagSet => ['smembers', this.keys.buildFlagSetKey(flagSet)])) .then((results) => results.map(([e, value], index) => { if (e === null) return value; diff --git a/src/storages/inRedis/TelemetryCacheInRedis.ts b/src/storages/inRedis/TelemetryCacheInRedis.ts index ab50fbe5..e77400cc 100644 --- a/src/storages/inRedis/TelemetryCacheInRedis.ts +++ b/src/storages/inRedis/TelemetryCacheInRedis.ts @@ -3,13 +3,13 @@ import { Method, MultiConfigs, MultiMethodExceptions, MultiMethodLatencies } fro import { KeyBuilderSS } from '../KeyBuilderSS'; import { ITelemetryCacheAsync } from '../types'; import { findLatencyIndex } from '../findLatencyIndex'; -import { Redis } from 'ioredis'; import { getTelemetryConfigStats } from '../../sync/submitters/telemetrySubmitter'; import { CONSUMER_MODE, STORAGE_REDIS } from '../../utils/constants'; import { isNaNNumber, isString } from '../../utils/lang'; import { _Map } from '../../utils/lang/maps'; import { MAX_LATENCY_BUCKET_COUNT, newBuckets } from '../inMemory/TelemetryCacheInMemory'; import { parseLatencyField, parseExceptionField, parseMetadata } from '../utils'; +import type { RedisAdapter } from './RedisAdapter'; export class TelemetryCacheInRedis implements ITelemetryCacheAsync { @@ -19,7 +19,7 @@ export class TelemetryCacheInRedis implements ITelemetryCacheAsync { * @param keys Key builder. * @param redis Redis client. */ - constructor(private readonly log: ILogger, private readonly keys: KeyBuilderSS, private readonly redis: Redis) { } + constructor(private readonly log: ILogger, private readonly keys: KeyBuilderSS, private readonly redis: RedisAdapter) { } recordLatency(method: Method, latencyMs: number) { const [key, field] = this.keys.buildLatencyKey(method, findLatencyIndex(latencyMs)).split('::'); diff --git a/src/storages/inRedis/UniqueKeysCacheInRedis.ts b/src/storages/inRedis/UniqueKeysCacheInRedis.ts index c6dd71dd..6abdb88a 100644 --- a/src/storages/inRedis/UniqueKeysCacheInRedis.ts +++ b/src/storages/inRedis/UniqueKeysCacheInRedis.ts @@ -1,21 +1,21 @@ import { IUniqueKeysCacheBase } from '../types'; -import { Redis } from 'ioredis'; import { UniqueKeysCacheInMemory } from '../inMemory/UniqueKeysCacheInMemory'; import { setToArray } from '../../utils/lang/sets'; import { DEFAULT_CACHE_SIZE, REFRESH_RATE, TTL_REFRESH } from './constants'; import { LOG_PREFIX } from './constants'; import { ILogger } from '../../logger/types'; import { UniqueKeysItemSs } from '../../sync/submitters/types'; +import type { RedisAdapter } from './RedisAdapter'; export class UniqueKeysCacheInRedis extends UniqueKeysCacheInMemory implements IUniqueKeysCacheBase { private readonly log: ILogger; private readonly key: string; - private readonly redis: Redis; + private readonly redis: RedisAdapter; private readonly refreshRate: number; private intervalId: any; - constructor(log: ILogger, key: string, redis: Redis, uniqueKeysQueueSize = DEFAULT_CACHE_SIZE, refreshRate = REFRESH_RATE) { + constructor(log: ILogger, key: string, redis: RedisAdapter, uniqueKeysQueueSize = DEFAULT_CACHE_SIZE, refreshRate = REFRESH_RATE) { super(uniqueKeysQueueSize); this.log = log; this.key = key; diff --git a/src/storages/inRedis/__tests__/EventsCacheInRedis.spec.ts b/src/storages/inRedis/__tests__/EventsCacheInRedis.spec.ts index 708e3fca..953b96d3 100644 --- a/src/storages/inRedis/__tests__/EventsCacheInRedis.spec.ts +++ b/src/storages/inRedis/__tests__/EventsCacheInRedis.spec.ts @@ -1,14 +1,14 @@ -import Redis from 'ioredis'; import { loggerMock } from '../../../logger/__tests__/sdkLogger.mock'; import { EventsCacheInRedis } from '../EventsCacheInRedis'; import { metadata, fakeEvent1, fakeEvent1stored, fakeEvent2, fakeEvent2stored, fakeEvent3, fakeEvent3stored } from '../../pluggable/__tests__/EventsCachePluggable.spec'; +import { RedisAdapter } from '../RedisAdapter'; const prefix = 'events_cache_ut'; const eventsKey = `${prefix}.events`; const nonListKey = 'non-list-key'; test('EVENTS CACHE IN REDIS / `track`, `count`, `popNWithMetadata` and `drop` methods', async () => { - const connection = new Redis(); + const connection = new RedisAdapter(loggerMock); // Clean up in case there are still keys there. await connection.del(eventsKey); @@ -54,5 +54,5 @@ test('EVENTS CACHE IN REDIS / `track`, `count`, `popNWithMetadata` and `drop` me // Clean up then end. await connection.del(eventsKey, nonListKey); - await connection.quit(); + await connection.disconnect(); }); diff --git a/src/storages/inRedis/__tests__/ImpressionCountsCacheInRedis.spec.ts b/src/storages/inRedis/__tests__/ImpressionCountsCacheInRedis.spec.ts index e28011c6..960a2cb0 100644 --- a/src/storages/inRedis/__tests__/ImpressionCountsCacheInRedis.spec.ts +++ b/src/storages/inRedis/__tests__/ImpressionCountsCacheInRedis.spec.ts @@ -1,9 +1,9 @@ // @ts-nocheck import { ImpressionCountsCacheInRedis } from '../ImpressionCountsCacheInRedis'; import { truncateTimeFrame } from '../../../utils/time'; -import Redis from 'ioredis'; import { RedisMock } from '../../../utils/redis/RedisMock'; import { loggerMock } from '../../../logger/__tests__/sdkLogger.mock'; +import { RedisAdapter } from '../RedisAdapter'; describe('IMPRESSION COUNTS CACHE IN REDIS', () => { const key = 'impression_count_post'; @@ -16,7 +16,7 @@ describe('IMPRESSION COUNTS CACHE IN REDIS', () => { expected[`feature2::${truncateTimeFrame(nextHourTimestamp)}`] = '4'; test('Impression Counter Test makeKey', async () => { - const connection = new Redis(); + const connection = new RedisAdapter(loggerMock); const counter = new ImpressionCountsCacheInRedis(loggerMock, key, connection); const timestamp1 = new Date(2020, 9, 2, 10, 0, 0).getTime(); @@ -25,11 +25,11 @@ describe('IMPRESSION COUNTS CACHE IN REDIS', () => { expect(counter._makeKey(null, new Date(2020, 9, 2, 10, 53, 12).getTime())).toBe(`null::${timestamp1}`); expect(counter._makeKey(null, 0)).toBe('null::0'); - await connection.quit(); + await connection.disconnect(); }); test('Impression Counter Test BasicUsage', async () => { - const connection = new Redis(); + const connection = new RedisAdapter(loggerMock); const counter = new ImpressionCountsCacheInRedis(loggerMock, key, connection); counter.track('feature1', timestamp, 1); @@ -76,11 +76,14 @@ describe('IMPRESSION COUNTS CACHE IN REDIS', () => { expect(Object.keys(counter.pop()).length).toBe(0); await connection.del(key); - await connection.quit(); + await connection.disconnect(); }); - test('POST IMPRESSION COUNTS IN REDIS FUNCTION', (done) => { - const connection = new Redis(); + test('POST IMPRESSION COUNTS IN REDIS FUNCTION', async () => { + const connection = new RedisAdapter(loggerMock); + // @TODO next line is not required with ioredis + await new Promise(res => connection.once('ready', res)); + const counter = new ImpressionCountsCacheInRedis(loggerMock, key, connection); // Clean up in case there are still keys there. connection.del(key); @@ -95,15 +98,12 @@ describe('IMPRESSION COUNTS CACHE IN REDIS', () => { counter.track('feature2', nextHourTimestamp + 3, 2); counter.track('feature2', nextHourTimestamp + 4, 2); - counter.postImpressionCountsInRedis().then(() => { + await counter.postImpressionCountsInRedis(); - connection.hgetall(key).then(async data => { - expect(data).toStrictEqual(expected); - await connection.del(key); - await connection.quit(); - done(); - }); - }); + const data = await connection.hgetall(key); + expect(data).toStrictEqual(expected); + await connection.del(key); + await connection.disconnect(); }); test('start and stop task', (done) => { @@ -122,22 +122,22 @@ describe('IMPRESSION COUNTS CACHE IN REDIS', () => { counter.track('feature2', nextHourTimestamp + 3, 2); counter.track('feature2', nextHourTimestamp + 4, 2); - expect(connection.pipeline).not.toBeCalled(); + expect(connection.pipelineExec).not.toBeCalled(); counter.start(); setTimeout(() => { - expect(connection.pipeline).toBeCalledTimes(1); + expect(connection.pipelineExec).toBeCalledTimes(1); expect(counter.isEmpty()).toBe(true); counter.stop(); - expect(connection.pipeline).toBeCalledTimes(1); // Stopping when cache is empty, does not call the wrapper + expect(connection.pipelineExec).toBeCalledTimes(1); // Stopping when cache is empty, does not call the wrapper counter.track('feature3', nextHourTimestamp + 4, 2); }, refreshRate + 30); setTimeout(() => { - expect(connection.pipeline).toBeCalledTimes(1); + expect(connection.pipelineExec).toBeCalledTimes(1); counter.start(); setTimeout(() => { - expect(connection.pipeline).toBeCalledTimes(2); + expect(connection.pipelineExec).toBeCalledTimes(2); counter.stop(); done(); }, refreshRate + 30); @@ -146,7 +146,7 @@ describe('IMPRESSION COUNTS CACHE IN REDIS', () => { }); test('Should call "onFullQueueCb" when the queue is full. "getImpressionsCount" should pop data.', async () => { - const connection = new Redis(); + const connection = new RedisAdapter(loggerMock); const counter = new ImpressionCountsCacheInRedis(loggerMock, key, connection, 5); // Clean up in case there are still keys there. await connection.del(key); @@ -183,6 +183,6 @@ describe('IMPRESSION COUNTS CACHE IN REDIS', () => { expect(await connection.hgetall(key)).toStrictEqual({}); await connection.del(key); - await connection.quit(); + await connection.disconnect(); }); }); diff --git a/src/storages/inRedis/__tests__/ImpressionsCacheInRedis.spec.ts b/src/storages/inRedis/__tests__/ImpressionsCacheInRedis.spec.ts index 0657d9ab..c4b17886 100644 --- a/src/storages/inRedis/__tests__/ImpressionsCacheInRedis.spec.ts +++ b/src/storages/inRedis/__tests__/ImpressionsCacheInRedis.spec.ts @@ -44,7 +44,7 @@ describe('IMPRESSIONS CACHE IN REDIS', () => { expect(await c.count()).toBe(0); // storage should be empty after dropping it await connection.del(impressionsKey); - await connection.quit(); + await connection.disconnect(); }); test('`track` should not resolve before calling expire', async () => { @@ -76,7 +76,7 @@ describe('IMPRESSIONS CACHE IN REDIS', () => { // @ts-expect-error await c.track([i1, i2]).then(() => { connection.del(impressionsKey); - connection.quit(); // Try to disconnect right away. + connection.disconnect(); // Try to disconnect right away. expect(spy1).toBeCalled(); // Redis rpush was called once before executing external callback. // Following assertion fails if the expire takes place after disconnected and throws unhandledPromiseRejection expect(spy2).toBeCalled(); // Redis expire was called once before executing external callback. diff --git a/src/storages/inRedis/__tests__/RedisAdapter.spec.ts b/src/storages/inRedis/__tests__/RedisAdapter.spec.ts index ecdf855a..2f10e7e1 100644 --- a/src/storages/inRedis/__tests__/RedisAdapter.spec.ts +++ b/src/storages/inRedis/__tests__/RedisAdapter.spec.ts @@ -11,7 +11,7 @@ const LOG_PREFIX = 'storage:redis-adapter: '; // Mocking ioredis // The list of methods we're wrapping on a promise (for timeout) on the adapter. -const METHODS_TO_PROMISE_WRAP = ['set', 'exec', 'del', 'get', 'keys', 'sadd', 'srem', 'sismember', 'smembers', 'incr', 'rpush', 'pipeline', 'expire', 'mget', 'lrange', 'ltrim', 'hset']; +const METHODS_TO_PROMISE_WRAP = ['set', 'exec', 'del', 'get', 'keys', 'sadd', 'srem', 'sismember', 'smembers', 'incr', 'rpush', 'expire', 'mget', 'lrange', 'ltrim', 'hset', 'pipelineExec']; const ioredisMock = reduce([...METHODS_TO_PROMISE_WRAP, 'disconnect'], (acc, methodName) => { acc[methodName] = jest.fn(() => Promise.resolve(methodName)); diff --git a/src/storages/inRedis/__tests__/SegmentsCacheInRedis.spec.ts b/src/storages/inRedis/__tests__/SegmentsCacheInRedis.spec.ts index be4b4f3b..009d6b10 100644 --- a/src/storages/inRedis/__tests__/SegmentsCacheInRedis.spec.ts +++ b/src/storages/inRedis/__tests__/SegmentsCacheInRedis.spec.ts @@ -1,15 +1,15 @@ -import Redis from 'ioredis'; import { SegmentsCacheInRedis } from '../SegmentsCacheInRedis'; import { KeyBuilderSS } from '../../KeyBuilderSS'; import { loggerMock } from '../../../logger/__tests__/sdkLogger.mock'; import { metadata } from '../../__tests__/KeyBuilder.spec'; +import { RedisAdapter } from '../RedisAdapter'; const keys = new KeyBuilderSS('prefix', metadata); describe('SEGMENTS CACHE IN REDIS', () => { test('isInSegment, set/getChangeNumber, add/removeFromSegment', async () => { - const connection = new Redis(); + const connection = new RedisAdapter(loggerMock); const cache = new SegmentsCacheInRedis(loggerMock, keys, connection); await cache.clear(); @@ -37,11 +37,11 @@ describe('SEGMENTS CACHE IN REDIS', () => { expect(await cache.isInSegment('mocked-segment', 'e')).toBe(true); await cache.clear(); - await connection.quit(); + await connection.disconnect(); }); test('registerSegment / getRegisteredSegments', async () => { - const connection = new Redis(); + const connection = new RedisAdapter(loggerMock); const cache = new SegmentsCacheInRedis(loggerMock, keys, connection); await cache.clear(); @@ -55,7 +55,7 @@ describe('SEGMENTS CACHE IN REDIS', () => { ['s1', 's2', 's3', 's4'].forEach(s => expect(segments.indexOf(s) !== -1).toBe(true)); await cache.clear(); - await connection.quit(); + await connection.disconnect(); }); }); diff --git a/src/storages/inRedis/__tests__/SplitsCacheInRedis.spec.ts b/src/storages/inRedis/__tests__/SplitsCacheInRedis.spec.ts index cb8093a8..913121c0 100644 --- a/src/storages/inRedis/__tests__/SplitsCacheInRedis.spec.ts +++ b/src/storages/inRedis/__tests__/SplitsCacheInRedis.spec.ts @@ -1,4 +1,3 @@ -import Redis from 'ioredis'; import { SplitsCacheInRedis } from '../SplitsCacheInRedis'; import { KeyBuilderSS } from '../../KeyBuilderSS'; import { loggerMock } from '../../../logger/__tests__/sdkLogger.mock'; @@ -6,6 +5,7 @@ import { splitWithUserTT, splitWithAccountTT, featureFlagOne, featureFlagThree, import { ISplit } from '../../../dtos/types'; import { metadata } from '../../__tests__/KeyBuilder.spec'; import { _Set } from '../../../utils/lang/sets'; +import { RedisAdapter } from '../RedisAdapter'; const prefix = 'splits_cache_ut'; const keysBuilder = new KeyBuilderSS(prefix, metadata); @@ -13,7 +13,7 @@ const keysBuilder = new KeyBuilderSS(prefix, metadata); describe('SPLITS CACHE REDIS', () => { test('add/remove/get splits & set/get change number', async () => { - const connection = new Redis(); + const connection = new RedisAdapter(loggerMock); const cache = new SplitsCacheInRedis(loggerMock, keysBuilder, connection); await cache.addSplits([ @@ -55,11 +55,11 @@ describe('SPLITS CACHE REDIS', () => { await connection.del(keysBuilder.buildTrafficTypeKey('account_tt')); await connection.del(keysBuilder.buildSplitKey('lol2')); await connection.del(keysBuilder.buildSplitsTillKey()); - await connection.quit(); + await connection.disconnect(); }); test('trafficTypeExists', async () => { - const connection = new Redis(); + const connection = new RedisAdapter(loggerMock); const cache = new SplitsCacheInRedis(loggerMock, keysBuilder, connection); await cache.addSplits([ @@ -103,11 +103,11 @@ describe('SPLITS CACHE REDIS', () => { await connection.del(keysBuilder.buildTrafficTypeKey('account_tt')); await connection.del(keysBuilder.buildSplitKey('malformed')); await connection.del(keysBuilder.buildSplitKey('split1')); - await connection.quit(); + await connection.disconnect(); }); test('killLocally', async () => { - const connection = new Redis(); + const connection = new RedisAdapter(loggerMock); const cache = new SplitsCacheInRedis(loggerMock, keysBuilder, connection); await cache.addSplit('lol1', splitWithUserTT); @@ -141,11 +141,11 @@ describe('SPLITS CACHE REDIS', () => { // Delete splits and TT keys await cache.removeSplits(['lol1', 'lol2']); expect(await connection.keys(`${prefix}*`)).toHaveLength(0); - await connection.quit(); + await connection.disconnect(); }); test('flag set cache tests', async () => { - const connection = new Redis(); // @ts-ignore + const connection = new RedisAdapter(loggerMock); // @ts-ignore const cache = new SplitsCacheInRedis(loggerMock, keysBuilder, connection, { groupedFilters: { bySet: ['o', 'n', 'e', 'x'] } }); const emptySet = new _Set([]); @@ -173,14 +173,12 @@ describe('SPLITS CACHE REDIS', () => { expect(await cache.getNamesByFlagSets(['x'])).toEqual([new _Set(['ff_one'])]); expect(await cache.getNamesByFlagSets(['o', 'e', 'x'])).toEqual([new _Set(['ff_two']), new _Set(['ff_three']), new _Set(['ff_one'])]); - // @ts-ignore Simulate one error in connection.pipeline().exec() - jest.spyOn(connection, 'pipeline').mockImplementationOnce(() => { - return { - exec: () => Promise.resolve([['error', null], [null, ['ff_three']], [null, ['ff_one']]]), - }; + // @ts-ignore Simulate one error in connection.pipelineExec() + jest.spyOn(connection, 'pipelineExec').mockImplementationOnce(() => { + return Promise.resolve([['error', null], [null, ['ff_three']], [null, ['ff_one']]]); }); expect(await cache.getNamesByFlagSets(['o', 'e', 'x'])).toEqual([emptySet, new _Set(['ff_three']), new _Set(['ff_one'])]); - (connection.pipeline as jest.Mock).mockRestore(); + (connection.pipelineExec as jest.Mock).mockRestore(); await cache.removeSplit(featureFlagOne.name); expect(await cache.getNamesByFlagSets(['x'])).toEqual([emptySet]); @@ -195,12 +193,12 @@ describe('SPLITS CACHE REDIS', () => { // Delete splits, TT and flag set keys await cache.removeSplits([featureFlagThree.name, featureFlagTwo.name, featureFlagWithEmptyFS.name]); expect(await connection.keys(`${prefix}*`)).toHaveLength(0); - await connection.quit(); + await connection.disconnect(); }); // if FlagSets filter is not defined, it should store all FlagSets in memory. test('flag set cache tests without filters', async () => { - const connection = new Redis(); // @ts-ignore + const connection = new RedisAdapter(loggerMock); const cacheWithoutFilters = new SplitsCacheInRedis(loggerMock, keysBuilder, connection); const emptySet = new _Set([]); @@ -222,7 +220,7 @@ describe('SPLITS CACHE REDIS', () => { // Delete splits, TT and flag set keys await cacheWithoutFilters.removeSplits([featureFlagThree.name, featureFlagTwo.name, featureFlagOne.name, featureFlagWithEmptyFS.name]); expect(await connection.keys(`${prefix}*`)).toHaveLength(0); - await connection.quit(); + await connection.disconnect(); }); }); diff --git a/src/storages/inRedis/__tests__/TelemetryCacheInRedis.spec.ts b/src/storages/inRedis/__tests__/TelemetryCacheInRedis.spec.ts index 5eea4329..82685d18 100644 --- a/src/storages/inRedis/__tests__/TelemetryCacheInRedis.spec.ts +++ b/src/storages/inRedis/__tests__/TelemetryCacheInRedis.spec.ts @@ -1,9 +1,9 @@ -import Redis from 'ioredis'; import { loggerMock } from '../../../logger/__tests__/sdkLogger.mock'; import { KeyBuilderSS } from '../../KeyBuilderSS'; import { TelemetryCacheInRedis } from '../TelemetryCacheInRedis'; import { newBuckets } from '../../inMemory/TelemetryCacheInMemory'; import { metadata } from '../../__tests__/KeyBuilder.spec'; +import { RedisAdapter } from '../RedisAdapter'; const prefix = 'telemetry_cache_ut'; const exceptionKey = `${prefix}.telemetry.exceptions`; @@ -14,7 +14,7 @@ const fieldVersionablePrefix = `${metadata.s}/${metadata.n}/${metadata.i}`; test('TELEMETRY CACHE IN REDIS', async () => { const keysBuilder = new KeyBuilderSS(prefix, metadata); - const connection = new Redis(); + const connection = new RedisAdapter(loggerMock); const cache = new TelemetryCacheInRedis(loggerMock, keysBuilder, connection); // recordException @@ -86,5 +86,5 @@ test('TELEMETRY CACHE IN REDIS', async () => { expect((await cache.popExceptions()).size).toBe(0); expect((await cache.popConfigs()).size).toBe(0); - await connection.quit(); + await connection.disconnect(); }); diff --git a/src/storages/inRedis/__tests__/UniqueKeysCacheInRedis.spec.ts b/src/storages/inRedis/__tests__/UniqueKeysCacheInRedis.spec.ts index 3fcbb3a5..e1fa2148 100644 --- a/src/storages/inRedis/__tests__/UniqueKeysCacheInRedis.spec.ts +++ b/src/storages/inRedis/__tests__/UniqueKeysCacheInRedis.spec.ts @@ -1,15 +1,15 @@ //@ts-nocheck -import Redis from 'ioredis'; import { UniqueKeysCacheInRedis } from '../UniqueKeysCacheInRedis'; import { loggerMock } from '../../../logger/__tests__/sdkLogger.mock'; import { RedisMock } from '../../../utils/redis/RedisMock'; +import { RedisAdapter } from '../RedisAdapter'; describe('UNIQUE KEYS CACHE IN REDIS', () => { const key = 'unique_key_post'; test('should incrementally store values, clear the queue, and tell if it is empty', async () => { - const connection = new Redis(); + const connection = new RedisAdapter(loggerMock); const cache = new UniqueKeysCacheInRedis(loggerMock, key, connection); @@ -46,12 +46,12 @@ describe('UNIQUE KEYS CACHE IN REDIS', () => { expect(cache.pop()).toEqual({ keys: [] }); expect(cache.isEmpty()).toBe(true); - await connection.quit(); + await connection.disconnect(); }); test('Should call "onFullQueueCb" when the queue is full.', async () => { let cbCalled = 0; - const connection = new Redis(); + const connection = new RedisAdapter(loggerMock); const cache = new UniqueKeysCacheInRedis(loggerMock, key, connection, 3); // small uniqueKeysCache size to be reached cache.setOnFullQueueCb(() => { cbCalled++; cache.clear(); }); @@ -75,11 +75,11 @@ describe('UNIQUE KEYS CACHE IN REDIS', () => { expect(cbCalled).toBe(2); // Until the queue is filled with events again. await connection.del(key); - await connection.quit(); + await connection.disconnect(); }); test('post unique keys in redis method', async () => { - const connection = new Redis(); + const connection = new RedisAdapter(loggerMock); const cache = new UniqueKeysCacheInRedis(loggerMock, key, connection, 20); cache.track('key1', 'feature1'); @@ -99,7 +99,7 @@ describe('UNIQUE KEYS CACHE IN REDIS', () => { expect(data).toStrictEqual(expected); await connection.del(key); - await connection.quit(); + await connection.disconnect(); }); }); @@ -145,7 +145,9 @@ describe('UNIQUE KEYS CACHE IN REDIS', () => { }); test('Should call "onFullQueueCb" when the queue is full. "popNRaw" should pop items.', async () => { - const connection = new Redis(); + const connection = new RedisAdapter(loggerMock); + // @TODO next line is not required with ioredis + await new Promise(res => connection.once('ready', res)); const cache = new UniqueKeysCacheInRedis(loggerMock, key, connection, 3); @@ -184,6 +186,6 @@ describe('UNIQUE KEYS CACHE IN REDIS', () => { expect(data).toStrictEqual([]); await connection.del(key); - await connection.quit(); + await connection.disconnect(); }); }); diff --git a/src/utils/redis/RedisMock.ts b/src/utils/redis/RedisMock.ts index e4df1136..6d13ec6d 100644 --- a/src/utils/redis/RedisMock.ts +++ b/src/utils/redis/RedisMock.ts @@ -8,13 +8,10 @@ function asyncFunction(data: any): Promise { } const IDENTITY_METHODS: string[] = []; -const ASYNC_METHODS = ['rpush', 'hincrby']; -const PIPELINE_METHODS = ['rpush', 'hincrby']; +const ASYNC_METHODS = ['rpush', 'hincrby', 'pipelineExec']; export class RedisMock { - private pipelineMethods: any = { exec: jest.fn(asyncFunction) }; - constructor() { IDENTITY_METHODS.forEach(method => { this[method] = jest.fn(identityFunction); @@ -22,12 +19,5 @@ export class RedisMock { ASYNC_METHODS.forEach(method => { this[method] = jest.fn(asyncFunction); }); - PIPELINE_METHODS.forEach(method => { - this.pipelineMethods[method] = this[method]; - }); - - this.pipeline = jest.fn(() => {return this.pipelineMethods;}); } - - } From bda2acfd87d0d264806e4c09295e0e32c78ab6b4 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Wed, 29 Nov 2023 17:02:14 -0300 Subject: [PATCH 22/45] Polishing --- src/storages/inRedis/ImpressionCountsCacheInRedis.ts | 1 - src/storages/inRedis/RedisAdapter.ts | 3 +-- src/storages/inRedis/__tests__/RedisAdapter.spec.ts | 2 +- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/storages/inRedis/ImpressionCountsCacheInRedis.ts b/src/storages/inRedis/ImpressionCountsCacheInRedis.ts index 4d5150e4..36ce0ebf 100644 --- a/src/storages/inRedis/ImpressionCountsCacheInRedis.ts +++ b/src/storages/inRedis/ImpressionCountsCacheInRedis.ts @@ -27,7 +27,6 @@ export class ImpressionCountsCacheInRedis extends ImpressionCountsCacheInMemory const keys = Object.keys(counts); if (!keys.length) return Promise.resolve(false); - // @ts-ignore return this.redis.pipelineExec(keys.map(key => ['hincrby', this.key, key, counts[key]])) .then(data => { // If this is the creation of the key on Redis, set the expiration for it in 3600 seconds. diff --git a/src/storages/inRedis/RedisAdapter.ts b/src/storages/inRedis/RedisAdapter.ts index f4077ac6..1aac4552 100644 --- a/src/storages/inRedis/RedisAdapter.ts +++ b/src/storages/inRedis/RedisAdapter.ts @@ -53,8 +53,7 @@ export class RedisAdapter extends ioredis { } pipelineExec(commands?: (string | number)[][]): Promise> { - // @ts-ignore - return this.pipeline(commands).exec(); + return this.pipeline(commands as string[][]).exec(); } _listenToEvents() { diff --git a/src/storages/inRedis/__tests__/RedisAdapter.spec.ts b/src/storages/inRedis/__tests__/RedisAdapter.spec.ts index 2f10e7e1..6cea3755 100644 --- a/src/storages/inRedis/__tests__/RedisAdapter.spec.ts +++ b/src/storages/inRedis/__tests__/RedisAdapter.spec.ts @@ -11,7 +11,7 @@ const LOG_PREFIX = 'storage:redis-adapter: '; // Mocking ioredis // The list of methods we're wrapping on a promise (for timeout) on the adapter. -const METHODS_TO_PROMISE_WRAP = ['set', 'exec', 'del', 'get', 'keys', 'sadd', 'srem', 'sismember', 'smembers', 'incr', 'rpush', 'expire', 'mget', 'lrange', 'ltrim', 'hset', 'pipelineExec']; +const METHODS_TO_PROMISE_WRAP = ['set', 'exec', 'del', 'get', 'keys', 'sadd', 'srem', 'sismember', 'smembers', 'incr', 'rpush', 'expire', 'mget', 'lrange', 'ltrim', 'hset', 'hincrby', 'popNRaw', 'flushdb', 'pipelineExec']; const ioredisMock = reduce([...METHODS_TO_PROMISE_WRAP, 'disconnect'], (acc, methodName) => { acc[methodName] = jest.fn(() => Promise.resolve(methodName)); From eed54592729da7c4b46beb95f05450ae66725ed2 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Wed, 29 Nov 2023 17:08:41 -0300 Subject: [PATCH 23/45] Refactor tests: use connection.disconnect rather than connection.quit --- .../__tests__/sdkReadinessManager.spec.ts | 2 +- .../__tests__/index.asyncCache.spec.ts | 2 +- .../__tests__/EventsCacheInRedis.spec.ts | 2 +- .../ImpressionCountsCacheInRedis.spec.ts | 21 ++++++++----------- .../__tests__/ImpressionsCacheInRedis.spec.ts | 4 ++-- .../__tests__/SegmentsCacheInRedis.spec.ts | 4 ++-- .../__tests__/SplitsCacheInRedis.spec.ts | 10 ++++----- .../__tests__/TelemetryCacheInRedis.spec.ts | 2 +- .../__tests__/UniqueKeysCacheInRedis.spec.ts | 8 +++---- 9 files changed, 26 insertions(+), 29 deletions(-) diff --git a/src/readiness/__tests__/sdkReadinessManager.spec.ts b/src/readiness/__tests__/sdkReadinessManager.spec.ts index 3c93b7a6..9c407052 100644 --- a/src/readiness/__tests__/sdkReadinessManager.spec.ts +++ b/src/readiness/__tests__/sdkReadinessManager.spec.ts @@ -49,7 +49,7 @@ describe('SDK Readiness Manager - Event emitter', () => { expect(sdkStatus[propName]).toBeTruthy(); // The sdkStatus exposes all minimal EventEmitter functionality. }); - expect(typeof sdkStatus['ready']).toBe('function'); // The sdkStatus exposes a .ready() function. + expect(typeof sdkStatus.ready).toBe('function'); // The sdkStatus exposes a .ready() function. expect(typeof sdkStatus.Event).toBe('object'); // It also exposes the Event map, expect(sdkStatus.Event.SDK_READY).toBe(SDK_READY); // which contains the constants for the events, for backwards compatibility. diff --git a/src/sdkManager/__tests__/index.asyncCache.spec.ts b/src/sdkManager/__tests__/index.asyncCache.spec.ts index 4081c10b..8ab2f7d2 100644 --- a/src/sdkManager/__tests__/index.asyncCache.spec.ts +++ b/src/sdkManager/__tests__/index.asyncCache.spec.ts @@ -70,7 +70,7 @@ describe('MANAGER API', () => { /** Teardown */ await cache.removeSplit(splitObject.name); - await connection.quit(); + await connection.disconnect(); }); test('Async cache with error', async () => { diff --git a/src/storages/inRedis/__tests__/EventsCacheInRedis.spec.ts b/src/storages/inRedis/__tests__/EventsCacheInRedis.spec.ts index 708e3fca..134c746e 100644 --- a/src/storages/inRedis/__tests__/EventsCacheInRedis.spec.ts +++ b/src/storages/inRedis/__tests__/EventsCacheInRedis.spec.ts @@ -54,5 +54,5 @@ test('EVENTS CACHE IN REDIS / `track`, `count`, `popNWithMetadata` and `drop` me // Clean up then end. await connection.del(eventsKey, nonListKey); - await connection.quit(); + await connection.disconnect(); }); diff --git a/src/storages/inRedis/__tests__/ImpressionCountsCacheInRedis.spec.ts b/src/storages/inRedis/__tests__/ImpressionCountsCacheInRedis.spec.ts index e28011c6..a2f0452a 100644 --- a/src/storages/inRedis/__tests__/ImpressionCountsCacheInRedis.spec.ts +++ b/src/storages/inRedis/__tests__/ImpressionCountsCacheInRedis.spec.ts @@ -25,7 +25,7 @@ describe('IMPRESSION COUNTS CACHE IN REDIS', () => { expect(counter._makeKey(null, new Date(2020, 9, 2, 10, 53, 12).getTime())).toBe(`null::${timestamp1}`); expect(counter._makeKey(null, 0)).toBe('null::0'); - await connection.quit(); + await connection.disconnect(); }); test('Impression Counter Test BasicUsage', async () => { @@ -76,10 +76,10 @@ describe('IMPRESSION COUNTS CACHE IN REDIS', () => { expect(Object.keys(counter.pop()).length).toBe(0); await connection.del(key); - await connection.quit(); + await connection.disconnect(); }); - test('POST IMPRESSION COUNTS IN REDIS FUNCTION', (done) => { + test('POST IMPRESSION COUNTS IN REDIS FUNCTION', async () => { const connection = new Redis(); const counter = new ImpressionCountsCacheInRedis(loggerMock, key, connection); // Clean up in case there are still keys there. @@ -95,15 +95,12 @@ describe('IMPRESSION COUNTS CACHE IN REDIS', () => { counter.track('feature2', nextHourTimestamp + 3, 2); counter.track('feature2', nextHourTimestamp + 4, 2); - counter.postImpressionCountsInRedis().then(() => { + await counter.postImpressionCountsInRedis(); - connection.hgetall(key).then(async data => { - expect(data).toStrictEqual(expected); - await connection.del(key); - await connection.quit(); - done(); - }); - }); + const data = await connection.hgetall(key); + expect(data).toStrictEqual(expected); + await connection.del(key); + await connection.disconnect(); }); test('start and stop task', (done) => { @@ -183,6 +180,6 @@ describe('IMPRESSION COUNTS CACHE IN REDIS', () => { expect(await connection.hgetall(key)).toStrictEqual({}); await connection.del(key); - await connection.quit(); + await connection.disconnect(); }); }); diff --git a/src/storages/inRedis/__tests__/ImpressionsCacheInRedis.spec.ts b/src/storages/inRedis/__tests__/ImpressionsCacheInRedis.spec.ts index 0657d9ab..c4b17886 100644 --- a/src/storages/inRedis/__tests__/ImpressionsCacheInRedis.spec.ts +++ b/src/storages/inRedis/__tests__/ImpressionsCacheInRedis.spec.ts @@ -44,7 +44,7 @@ describe('IMPRESSIONS CACHE IN REDIS', () => { expect(await c.count()).toBe(0); // storage should be empty after dropping it await connection.del(impressionsKey); - await connection.quit(); + await connection.disconnect(); }); test('`track` should not resolve before calling expire', async () => { @@ -76,7 +76,7 @@ describe('IMPRESSIONS CACHE IN REDIS', () => { // @ts-expect-error await c.track([i1, i2]).then(() => { connection.del(impressionsKey); - connection.quit(); // Try to disconnect right away. + connection.disconnect(); // Try to disconnect right away. expect(spy1).toBeCalled(); // Redis rpush was called once before executing external callback. // Following assertion fails if the expire takes place after disconnected and throws unhandledPromiseRejection expect(spy2).toBeCalled(); // Redis expire was called once before executing external callback. diff --git a/src/storages/inRedis/__tests__/SegmentsCacheInRedis.spec.ts b/src/storages/inRedis/__tests__/SegmentsCacheInRedis.spec.ts index be4b4f3b..3e3bf503 100644 --- a/src/storages/inRedis/__tests__/SegmentsCacheInRedis.spec.ts +++ b/src/storages/inRedis/__tests__/SegmentsCacheInRedis.spec.ts @@ -37,7 +37,7 @@ describe('SEGMENTS CACHE IN REDIS', () => { expect(await cache.isInSegment('mocked-segment', 'e')).toBe(true); await cache.clear(); - await connection.quit(); + await connection.disconnect(); }); test('registerSegment / getRegisteredSegments', async () => { @@ -55,7 +55,7 @@ describe('SEGMENTS CACHE IN REDIS', () => { ['s1', 's2', 's3', 's4'].forEach(s => expect(segments.indexOf(s) !== -1).toBe(true)); await cache.clear(); - await connection.quit(); + await connection.disconnect(); }); }); diff --git a/src/storages/inRedis/__tests__/SplitsCacheInRedis.spec.ts b/src/storages/inRedis/__tests__/SplitsCacheInRedis.spec.ts index cb8093a8..638af5d1 100644 --- a/src/storages/inRedis/__tests__/SplitsCacheInRedis.spec.ts +++ b/src/storages/inRedis/__tests__/SplitsCacheInRedis.spec.ts @@ -55,7 +55,7 @@ describe('SPLITS CACHE REDIS', () => { await connection.del(keysBuilder.buildTrafficTypeKey('account_tt')); await connection.del(keysBuilder.buildSplitKey('lol2')); await connection.del(keysBuilder.buildSplitsTillKey()); - await connection.quit(); + await connection.disconnect(); }); test('trafficTypeExists', async () => { @@ -103,7 +103,7 @@ describe('SPLITS CACHE REDIS', () => { await connection.del(keysBuilder.buildTrafficTypeKey('account_tt')); await connection.del(keysBuilder.buildSplitKey('malformed')); await connection.del(keysBuilder.buildSplitKey('split1')); - await connection.quit(); + await connection.disconnect(); }); test('killLocally', async () => { @@ -141,7 +141,7 @@ describe('SPLITS CACHE REDIS', () => { // Delete splits and TT keys await cache.removeSplits(['lol1', 'lol2']); expect(await connection.keys(`${prefix}*`)).toHaveLength(0); - await connection.quit(); + await connection.disconnect(); }); test('flag set cache tests', async () => { @@ -195,7 +195,7 @@ describe('SPLITS CACHE REDIS', () => { // Delete splits, TT and flag set keys await cache.removeSplits([featureFlagThree.name, featureFlagTwo.name, featureFlagWithEmptyFS.name]); expect(await connection.keys(`${prefix}*`)).toHaveLength(0); - await connection.quit(); + await connection.disconnect(); }); // if FlagSets filter is not defined, it should store all FlagSets in memory. @@ -222,7 +222,7 @@ describe('SPLITS CACHE REDIS', () => { // Delete splits, TT and flag set keys await cacheWithoutFilters.removeSplits([featureFlagThree.name, featureFlagTwo.name, featureFlagOne.name, featureFlagWithEmptyFS.name]); expect(await connection.keys(`${prefix}*`)).toHaveLength(0); - await connection.quit(); + await connection.disconnect(); }); }); diff --git a/src/storages/inRedis/__tests__/TelemetryCacheInRedis.spec.ts b/src/storages/inRedis/__tests__/TelemetryCacheInRedis.spec.ts index 5eea4329..f55addd6 100644 --- a/src/storages/inRedis/__tests__/TelemetryCacheInRedis.spec.ts +++ b/src/storages/inRedis/__tests__/TelemetryCacheInRedis.spec.ts @@ -86,5 +86,5 @@ test('TELEMETRY CACHE IN REDIS', async () => { expect((await cache.popExceptions()).size).toBe(0); expect((await cache.popConfigs()).size).toBe(0); - await connection.quit(); + await connection.disconnect(); }); diff --git a/src/storages/inRedis/__tests__/UniqueKeysCacheInRedis.spec.ts b/src/storages/inRedis/__tests__/UniqueKeysCacheInRedis.spec.ts index 3fcbb3a5..e5f723cd 100644 --- a/src/storages/inRedis/__tests__/UniqueKeysCacheInRedis.spec.ts +++ b/src/storages/inRedis/__tests__/UniqueKeysCacheInRedis.spec.ts @@ -46,7 +46,7 @@ describe('UNIQUE KEYS CACHE IN REDIS', () => { expect(cache.pop()).toEqual({ keys: [] }); expect(cache.isEmpty()).toBe(true); - await connection.quit(); + await connection.disconnect(); }); test('Should call "onFullQueueCb" when the queue is full.', async () => { @@ -75,7 +75,7 @@ describe('UNIQUE KEYS CACHE IN REDIS', () => { expect(cbCalled).toBe(2); // Until the queue is filled with events again. await connection.del(key); - await connection.quit(); + await connection.disconnect(); }); test('post unique keys in redis method', async () => { @@ -99,7 +99,7 @@ describe('UNIQUE KEYS CACHE IN REDIS', () => { expect(data).toStrictEqual(expected); await connection.del(key); - await connection.quit(); + await connection.disconnect(); }); }); @@ -184,6 +184,6 @@ describe('UNIQUE KEYS CACHE IN REDIS', () => { expect(data).toStrictEqual([]); await connection.del(key); - await connection.quit(); + await connection.disconnect(); }); }); From 2601452c1a31bf5551686776079b99d22684b765 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Wed, 29 Nov 2023 17:13:08 -0300 Subject: [PATCH 24/45] Add changelog entry --- CHANGES.txt | 1 + package-lock.json | 4 ++-- package.json | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index c827ea97..36b87426 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,6 +1,7 @@ 1.12.0 (December XX, 2023) - Added support for Flag Sets in "consumer" and "partial consumer" modes for Pluggable and Redis storages. - Updated evaluation flow to log a warning when using flag sets that don't contain cached feature flags. + - Updated Redis adapter to wrap missing commands: 'hincrby', 'popNRaw', 'flushdb' and 'pipeline.exec'. 1.11.0 (November 3, 2023) - Added support for Flag Sets on the SDK, which enables grouping feature flags and interacting with the group rather than individually (more details in our documentation): diff --git a/package-lock.json b/package-lock.json index a0e6bd3b..f131008e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@splitsoftware/splitio-commons", - "version": "1.12.1-rc.1", + "version": "1.12.1-rc.2", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "@splitsoftware/splitio-commons", - "version": "1.12.1-rc.1", + "version": "1.12.1-rc.2", "license": "Apache-2.0", "dependencies": { "tslib": "^2.3.1" diff --git a/package.json b/package.json index a1b84218..df3097c7 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@splitsoftware/splitio-commons", - "version": "1.12.1-rc.1", + "version": "1.12.1-rc.2", "description": "Split Javascript SDK common components", "main": "cjs/index.js", "module": "esm/index.js", From 54afc00c83fadb7ee80e02d4b56c444da73e3ef8 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Wed, 29 Nov 2023 18:11:38 -0300 Subject: [PATCH 25/45] polishing --- .../__tests__/ImpressionCountsCacheInRedis.spec.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/storages/inRedis/__tests__/ImpressionCountsCacheInRedis.spec.ts b/src/storages/inRedis/__tests__/ImpressionCountsCacheInRedis.spec.ts index 960a2cb0..f257747d 100644 --- a/src/storages/inRedis/__tests__/ImpressionCountsCacheInRedis.spec.ts +++ b/src/storages/inRedis/__tests__/ImpressionCountsCacheInRedis.spec.ts @@ -9,11 +9,12 @@ describe('IMPRESSION COUNTS CACHE IN REDIS', () => { const key = 'impression_count_post'; const timestamp = new Date(2020, 9, 2, 10, 10, 12).getTime(); const nextHourTimestamp = new Date(2020, 9, 2, 11, 10, 12).getTime(); - const expected = {}; - expected[`feature1::${truncateTimeFrame(timestamp)}`] = '3'; - expected[`feature1::${truncateTimeFrame(nextHourTimestamp)}`] = '3'; - expected[`feature2::${truncateTimeFrame(timestamp)}`] = '4'; - expected[`feature2::${truncateTimeFrame(nextHourTimestamp)}`] = '4'; + const expected = { + [`feature1::${truncateTimeFrame(timestamp)}`]: '3', + [`feature1::${truncateTimeFrame(nextHourTimestamp)}`]: '3', + [`feature2::${truncateTimeFrame(timestamp)}`]: '4', + [`feature2::${truncateTimeFrame(nextHourTimestamp)}`]: '4' + }; test('Impression Counter Test makeKey', async () => { const connection = new RedisAdapter(loggerMock); From d0c33a7e8fbc1fe5b2656ffc9a7af27b86effe3b Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Wed, 29 Nov 2023 22:30:17 -0300 Subject: [PATCH 26/45] Remove pipelineExec method from RedisAdapter --- src/storages/inRedis/ImpressionCountsCacheInRedis.ts | 6 +++++- src/storages/inRedis/RedisAdapter.ts | 6 +----- src/storages/inRedis/SplitsCacheInRedis.ts | 4 ++-- .../__tests__/ImpressionCountsCacheInRedis.spec.ts | 10 +++++----- src/storages/inRedis/__tests__/RedisAdapter.spec.ts | 2 +- .../inRedis/__tests__/SplitsCacheInRedis.spec.ts | 8 ++++---- src/utils/redis/RedisMock.ts | 10 +++++++++- 7 files changed, 27 insertions(+), 19 deletions(-) diff --git a/src/storages/inRedis/ImpressionCountsCacheInRedis.ts b/src/storages/inRedis/ImpressionCountsCacheInRedis.ts index 36ce0ebf..efbb0e9b 100644 --- a/src/storages/inRedis/ImpressionCountsCacheInRedis.ts +++ b/src/storages/inRedis/ImpressionCountsCacheInRedis.ts @@ -27,7 +27,11 @@ export class ImpressionCountsCacheInRedis extends ImpressionCountsCacheInMemory const keys = Object.keys(counts); if (!keys.length) return Promise.resolve(false); - return this.redis.pipelineExec(keys.map(key => ['hincrby', this.key, key, counts[key]])) + const pipeline = this.redis.pipeline(); + keys.forEach(key => { + pipeline.hincrby(this.key, key, counts[key]); + }); + return pipeline.exec() .then(data => { // If this is the creation of the key on Redis, set the expiration for it in 3600 seconds. if (data.length && data.length === keys.length) { diff --git a/src/storages/inRedis/RedisAdapter.ts b/src/storages/inRedis/RedisAdapter.ts index 1aac4552..191834c4 100644 --- a/src/storages/inRedis/RedisAdapter.ts +++ b/src/storages/inRedis/RedisAdapter.ts @@ -8,7 +8,7 @@ import { timeout } from '../../utils/promise/timeout'; const LOG_PREFIX = 'storage:redis-adapter: '; // If we ever decide to fully wrap every method, there's a Commander.getBuiltinCommands from ioredis. -const METHODS_TO_PROMISE_WRAP = ['set', 'exec', 'del', 'get', 'keys', 'sadd', 'srem', 'sismember', 'smembers', 'incr', 'rpush', 'expire', 'mget', 'lrange', 'ltrim', 'hset', 'hincrby', 'popNRaw', 'flushdb', 'pipelineExec']; +const METHODS_TO_PROMISE_WRAP = ['set', 'exec', 'del', 'get', 'keys', 'sadd', 'srem', 'sismember', 'smembers', 'incr', 'rpush', 'expire', 'mget', 'lrange', 'ltrim', 'hset', 'hincrby', 'popNRaw', 'flushdb']; // Not part of the settings since it'll vary on each storage. We should be removing storage specific logic from elsewhere. const DEFAULT_OPTIONS = { @@ -52,10 +52,6 @@ export class RedisAdapter extends ioredis { this._setDisconnectWrapper(); } - pipelineExec(commands?: (string | number)[][]): Promise> { - return this.pipeline(commands as string[][]).exec(); - } - _listenToEvents() { this.once('ready', () => { const commandsCount = this._notReadyCommandsQueue ? this._notReadyCommandsQueue.length : 0; diff --git a/src/storages/inRedis/SplitsCacheInRedis.ts b/src/storages/inRedis/SplitsCacheInRedis.ts index ae139773..a9370ea3 100644 --- a/src/storages/inRedis/SplitsCacheInRedis.ts +++ b/src/storages/inRedis/SplitsCacheInRedis.ts @@ -192,7 +192,7 @@ export class SplitsCacheInRedis extends AbstractSplitsCacheAsync { */ getAll(): Promise { return this.redis.keys(this.keys.searchPatternForSplitKeys()) - .then((listOfKeys) => this.redis.pipelineExec(listOfKeys.map(k => ['get', k]))) + .then((listOfKeys) => this.redis.pipeline(listOfKeys.map(k => ['get', k])).exec()) .then(processPipelineAnswer) .then((splitDefinitions) => splitDefinitions.map((splitDefinition) => { return JSON.parse(splitDefinition); @@ -216,7 +216,7 @@ export class SplitsCacheInRedis extends AbstractSplitsCacheAsync { * or rejected if the pipelined redis operation fails. */ getNamesByFlagSets(flagSets: string[]): Promise[]> { - return this.redis.pipelineExec(flagSets.map(flagSet => ['smembers', this.keys.buildFlagSetKey(flagSet)])) + return this.redis.pipeline(flagSets.map(flagSet => ['smembers', this.keys.buildFlagSetKey(flagSet)])).exec() .then((results) => results.map(([e, value], index) => { if (e === null) return value; diff --git a/src/storages/inRedis/__tests__/ImpressionCountsCacheInRedis.spec.ts b/src/storages/inRedis/__tests__/ImpressionCountsCacheInRedis.spec.ts index f257747d..e9caf0fd 100644 --- a/src/storages/inRedis/__tests__/ImpressionCountsCacheInRedis.spec.ts +++ b/src/storages/inRedis/__tests__/ImpressionCountsCacheInRedis.spec.ts @@ -123,22 +123,22 @@ describe('IMPRESSION COUNTS CACHE IN REDIS', () => { counter.track('feature2', nextHourTimestamp + 3, 2); counter.track('feature2', nextHourTimestamp + 4, 2); - expect(connection.pipelineExec).not.toBeCalled(); + expect(connection.pipeline).not.toBeCalled(); counter.start(); setTimeout(() => { - expect(connection.pipelineExec).toBeCalledTimes(1); + expect(connection.pipeline).toBeCalledTimes(1); expect(counter.isEmpty()).toBe(true); counter.stop(); - expect(connection.pipelineExec).toBeCalledTimes(1); // Stopping when cache is empty, does not call the wrapper + expect(connection.pipeline).toBeCalledTimes(1); // Stopping when cache is empty, does not call the wrapper counter.track('feature3', nextHourTimestamp + 4, 2); }, refreshRate + 30); setTimeout(() => { - expect(connection.pipelineExec).toBeCalledTimes(1); + expect(connection.pipeline).toBeCalledTimes(1); counter.start(); setTimeout(() => { - expect(connection.pipelineExec).toBeCalledTimes(2); + expect(connection.pipeline).toBeCalledTimes(2); counter.stop(); done(); }, refreshRate + 30); diff --git a/src/storages/inRedis/__tests__/RedisAdapter.spec.ts b/src/storages/inRedis/__tests__/RedisAdapter.spec.ts index 6cea3755..90b8743f 100644 --- a/src/storages/inRedis/__tests__/RedisAdapter.spec.ts +++ b/src/storages/inRedis/__tests__/RedisAdapter.spec.ts @@ -11,7 +11,7 @@ const LOG_PREFIX = 'storage:redis-adapter: '; // Mocking ioredis // The list of methods we're wrapping on a promise (for timeout) on the adapter. -const METHODS_TO_PROMISE_WRAP = ['set', 'exec', 'del', 'get', 'keys', 'sadd', 'srem', 'sismember', 'smembers', 'incr', 'rpush', 'expire', 'mget', 'lrange', 'ltrim', 'hset', 'hincrby', 'popNRaw', 'flushdb', 'pipelineExec']; +const METHODS_TO_PROMISE_WRAP = ['set', 'exec', 'del', 'get', 'keys', 'sadd', 'srem', 'sismember', 'smembers', 'incr', 'rpush', 'expire', 'mget', 'lrange', 'ltrim', 'hset', 'hincrby', 'popNRaw', 'flushdb']; const ioredisMock = reduce([...METHODS_TO_PROMISE_WRAP, 'disconnect'], (acc, methodName) => { acc[methodName] = jest.fn(() => Promise.resolve(methodName)); diff --git a/src/storages/inRedis/__tests__/SplitsCacheInRedis.spec.ts b/src/storages/inRedis/__tests__/SplitsCacheInRedis.spec.ts index 913121c0..c7ad85c7 100644 --- a/src/storages/inRedis/__tests__/SplitsCacheInRedis.spec.ts +++ b/src/storages/inRedis/__tests__/SplitsCacheInRedis.spec.ts @@ -173,12 +173,12 @@ describe('SPLITS CACHE REDIS', () => { expect(await cache.getNamesByFlagSets(['x'])).toEqual([new _Set(['ff_one'])]); expect(await cache.getNamesByFlagSets(['o', 'e', 'x'])).toEqual([new _Set(['ff_two']), new _Set(['ff_three']), new _Set(['ff_one'])]); - // @ts-ignore Simulate one error in connection.pipelineExec() - jest.spyOn(connection, 'pipelineExec').mockImplementationOnce(() => { - return Promise.resolve([['error', null], [null, ['ff_three']], [null, ['ff_one']]]); + // @ts-ignore Simulate an error in connection.pipeline().exec() + jest.spyOn(connection, 'pipeline').mockImplementationOnce(() => { + return { exec: () => Promise.resolve([['error', null], [null, ['ff_three']], [null, ['ff_one']]]) }; }); expect(await cache.getNamesByFlagSets(['o', 'e', 'x'])).toEqual([emptySet, new _Set(['ff_three']), new _Set(['ff_one'])]); - (connection.pipelineExec as jest.Mock).mockRestore(); + (connection.pipeline as jest.Mock).mockRestore(); await cache.removeSplit(featureFlagOne.name); expect(await cache.getNamesByFlagSets(['x'])).toEqual([emptySet]); diff --git a/src/utils/redis/RedisMock.ts b/src/utils/redis/RedisMock.ts index 6d13ec6d..71c0c654 100644 --- a/src/utils/redis/RedisMock.ts +++ b/src/utils/redis/RedisMock.ts @@ -8,10 +8,13 @@ function asyncFunction(data: any): Promise { } const IDENTITY_METHODS: string[] = []; -const ASYNC_METHODS = ['rpush', 'hincrby', 'pipelineExec']; +const ASYNC_METHODS = ['rpush', 'hincrby']; +const PIPELINE_METHODS = ['rpush', 'hincrby']; export class RedisMock { + private pipelineMethods: any = { exec: jest.fn(asyncFunction) }; + constructor() { IDENTITY_METHODS.forEach(method => { this[method] = jest.fn(identityFunction); @@ -19,5 +22,10 @@ export class RedisMock { ASYNC_METHODS.forEach(method => { this[method] = jest.fn(asyncFunction); }); + PIPELINE_METHODS.forEach(method => { + this.pipelineMethods[method] = this[method]; + }); + + this.pipeline = jest.fn(() => { return this.pipelineMethods; }); } } From 1ce4acbc4fb405b9fc4853d0a903d385b6a7056f Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Wed, 29 Nov 2023 23:13:54 -0300 Subject: [PATCH 27/45] Fix typescript issue in RedisAdapter --- src/storages/inRedis/RedisAdapter.ts | 4 ++-- src/storages/inRedis/SplitsCacheInRedis.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/storages/inRedis/RedisAdapter.ts b/src/storages/inRedis/RedisAdapter.ts index dcdadb4a..81a2b4fd 100644 --- a/src/storages/inRedis/RedisAdapter.ts +++ b/src/storages/inRedis/RedisAdapter.ts @@ -91,9 +91,9 @@ export class RedisAdapter extends ioredis { const wrapCommand = (originalMethod: Function, methodName: string) => { // The value of "this" in this function should be the instance actually executing the method. It might be the instance referred (the base one) // or it can be the instance of a Pipeline object. - return function () { + return function (this: RedisAdapter | Pipeline) { const params = arguments; - const caller: (Pipeline | RedisAdapter) = this; // Need to change this crap to have TypeScript stop complaining + const caller: (Pipeline | RedisAdapter) = this; const commandWrapper = () => { instance.log.debug(`${LOG_PREFIX}Executing ${methodName}.`); diff --git a/src/storages/inRedis/SplitsCacheInRedis.ts b/src/storages/inRedis/SplitsCacheInRedis.ts index 70abd74b..a9370ea3 100644 --- a/src/storages/inRedis/SplitsCacheInRedis.ts +++ b/src/storages/inRedis/SplitsCacheInRedis.ts @@ -10,7 +10,7 @@ import type { RedisAdapter } from './RedisAdapter'; /** * Discard errors for an answer of multiple operations. */ -function processPipelineAnswer(results: Array<[Error | null, string]>): (string | string[])[] { +function processPipelineAnswer(results: Array<[Error | null, string]>): string[] { return results.reduce((accum: string[], errValuePair: [Error | null, string]) => { if (errValuePair[0] === null) accum.push(errValuePair[1]); return accum; From f13ce83f08aa70d76ed66c8c4d4a25bd06ae1db4 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Wed, 29 Nov 2023 22:03:51 -0300 Subject: [PATCH 28/45] Fixed manager methods to return a promise in consumer modes while the SDK is not operational --- CHANGES.txt | 3 ++ src/sdkFactory/index.ts | 2 +- src/sdkFactory/types.ts | 10 ++--- .../__tests__/index.asyncCache.spec.ts | 43 +++++++++++++++---- .../__tests__/index.syncCache.spec.ts | 26 ++++++++++- src/sdkManager/index.ts | 17 +++++--- src/trackers/impressionObserver/utils.ts | 2 +- 7 files changed, 77 insertions(+), 26 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index a893bd27..a44fed08 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,3 +1,6 @@ +1.11.1 (December XX, 2023) + - Bugfixing - Fixed manager methods to return a promise in consumer modes when called while the SDK is not operational (not ready or destroyed). + 1.11.0 (November 3, 2023) - Added support for Flag Sets on the SDK, which enables grouping feature flags and interacting with the group rather than individually (more details in our documentation): - Added new variations of the get treatment methods to support evaluating flags in given flag set/s. diff --git a/src/sdkFactory/index.ts b/src/sdkFactory/index.ts index 7778c325..cd15e9ef 100644 --- a/src/sdkFactory/index.ts +++ b/src/sdkFactory/index.ts @@ -83,7 +83,7 @@ export function sdkFactory(params: ISdkFactoryParams): SplitIO.ICsSDK | SplitIO. // SDK client and manager const clientMethod = sdkClientMethodFactory(ctx); - const managerInstance = sdkManagerFactory(log, storage.splits, sdkReadinessManager); + const managerInstance = sdkManagerFactory(settings, storage.splits, sdkReadinessManager); syncManager && syncManager.start(); signalListener && signalListener.start(); diff --git a/src/sdkFactory/types.ts b/src/sdkFactory/types.ts index 3cce9c9f..917d7f18 100644 --- a/src/sdkFactory/types.ts +++ b/src/sdkFactory/types.ts @@ -1,9 +1,9 @@ import { IIntegrationManager, IIntegrationFactoryParams } from '../integrations/types'; import { ISignalListener } from '../listeners/types'; -import { ILogger } from '../logger/types'; import { IReadinessManager, ISdkReadinessManager } from '../readiness/types'; +import type { sdkManagerFactory } from '../sdkManager'; import { IFetch, ISplitApi, IEventSourceConstructor } from '../services/types'; -import { IStorageAsync, IStorageSync, ISplitsCacheSync, ISplitsCacheAsync, IStorageFactoryParams } from '../storages/types'; +import { IStorageAsync, IStorageSync, IStorageFactoryParams } from '../storages/types'; import { ISyncManager } from '../sync/types'; import { IImpressionObserver } from '../trackers/impressionObserver/types'; import { IImpressionsTracker, IEventTracker, ITelemetryTracker, IFilterAdapter, IUniqueKeysTracker } from '../trackers/types'; @@ -87,11 +87,7 @@ export interface ISdkFactoryParams { syncManagerFactory?: (params: ISdkFactoryContextSync) => ISyncManager, // Sdk manager factory - sdkManagerFactory: ( - log: ILogger, - splits: ISplitsCacheSync | ISplitsCacheAsync, - sdkReadinessManager: ISdkReadinessManager - ) => SplitIO.IManager | SplitIO.IAsyncManager, + sdkManagerFactory: typeof sdkManagerFactory, // Sdk client method factory (ISDK::client method). // It Allows to distinguish SDK clients with the client-side API (`ICsSDK`) or server-side API (`ISDK` or `IAsyncSDK`). diff --git a/src/sdkManager/__tests__/index.asyncCache.spec.ts b/src/sdkManager/__tests__/index.asyncCache.spec.ts index 4081c10b..acff3bee 100644 --- a/src/sdkManager/__tests__/index.asyncCache.spec.ts +++ b/src/sdkManager/__tests__/index.asyncCache.spec.ts @@ -9,6 +9,7 @@ import { KeyBuilderSS } from '../../storages/KeyBuilderSS'; import { ISdkReadinessManager } from '../../readiness/types'; import { loggerMock } from '../../logger/__tests__/sdkLogger.mock'; import { metadata } from '../../storages/__tests__/KeyBuilder.spec'; +import { SplitIO } from '../../types'; // @ts-expect-error const sdkReadinessManagerMock = { @@ -21,16 +22,16 @@ const sdkReadinessManagerMock = { const keys = new KeyBuilderSS('prefix', metadata); -describe('MANAGER API', () => { +describe('Manager with async cache', () => { afterEach(() => { loggerMock.mockClear(); }); - test('Async cache (In Redis)', async () => { + test('returns the expected data from the cache', async () => { /** Setup: create manager */ const connection = new Redis({}); const cache = new SplitsCacheInRedis(loggerMock, keys, connection); - const manager = sdkManagerFactory(loggerMock, cache, sdkReadinessManagerMock); + const manager = sdkManagerFactory({ mode: 'consumer', log: loggerMock }, cache, sdkReadinessManagerMock); await cache.clear(); await cache.addSplit(splitObject.name, splitObject as any); @@ -43,7 +44,6 @@ describe('MANAGER API', () => { const split = await manager.split(splitObject.name); expect(split).toEqual(splitView); - /** List all the split names */ const names = await manager.names(); @@ -66,18 +66,16 @@ describe('MANAGER API', () => { expect(await manager.splits()).toEqual([]); // If the factory/client is destroyed, `manager.splits()` will return empty array either way since the storage is not valid. expect(await manager.names()).toEqual([]); // If the factory/client is destroyed, `manager.names()` will return empty array either way since the storage is not valid. - - /** Teardown */ await cache.removeSplit(splitObject.name); await connection.quit(); }); - test('Async cache with error', async () => { + test('handles storage errors', async () => { // passing an empty object as wrapper, to make method calls of splits cache fail returning a rejected promise. // @ts-expect-error const cache = new SplitsCachePluggable(loggerMock, keys, wrapperAdapter(loggerMock, {})); - const manager = sdkManagerFactory(loggerMock, cache, sdkReadinessManagerMock); + const manager = sdkManagerFactory({ mode: 'consumer_partial', log: loggerMock }, cache, sdkReadinessManagerMock); expect(await manager.split('some_spplit')).toEqual(null); expect(await manager.splits()).toEqual([]); @@ -86,4 +84,33 @@ describe('MANAGER API', () => { expect(loggerMock.error).toBeCalledTimes(3); // 3 error logs, one for each attempt to call a wrapper method }); + test('returns empty results when not operational', async () => { + // SDK is flagged as destroyed + const sdkReadinessManagerMock = { + readinessManager: { + isReady: () => true, + isReadyFromCache: () => true, + isDestroyed: () => true + }, + sdkStatus: {} + }; + // @ts-expect-error + const manager = sdkManagerFactory({ mode: 'consumer_partial', log: loggerMock }, {}, sdkReadinessManagerMock) as SplitIO.IAsyncManager; + + function validateManager() { + expect(manager.split('some_spplit')).resolves.toBe(null); + expect(manager.splits()).resolves.toEqual([]); + expect(manager.names()).resolves.toEqual([]); + } + + validateManager(); + + // SDK is not ready + sdkReadinessManagerMock.readinessManager.isReady = () => false; + sdkReadinessManagerMock.readinessManager.isReadyFromCache = () => false; + sdkReadinessManagerMock.readinessManager.isDestroyed = () => false; + + validateManager(); + }); + }); diff --git a/src/sdkManager/__tests__/index.syncCache.spec.ts b/src/sdkManager/__tests__/index.syncCache.spec.ts index 2ce3721b..18e03d21 100644 --- a/src/sdkManager/__tests__/index.syncCache.spec.ts +++ b/src/sdkManager/__tests__/index.syncCache.spec.ts @@ -14,11 +14,11 @@ const sdkReadinessManagerMock = { sdkStatus: jest.fn() } as ISdkReadinessManager; -describe('MANAGER API / Sync cache (In Memory)', () => { +describe('Manager with sync cache (In Memory)', () => { /** Setup: create manager */ const cache = new SplitsCacheInMemory(); - const manager = sdkManagerFactory(loggerMock, cache, sdkReadinessManagerMock); + const manager = sdkManagerFactory({ mode: 'standalone', log: loggerMock }, cache, sdkReadinessManagerMock); cache.addSplit(splitObject.name, splitObject as any); test('List all splits', () => { @@ -57,4 +57,26 @@ describe('MANAGER API / Sync cache (In Memory)', () => { expect(manager.names()).toEqual([]); // If the factory/client is destroyed, `manager.names()` will return empty array either way since the storage is not valid. }); + // @TODO tests for not operational + + test('returns empty results when not operational', async () => { + // SDK is flagged as destroyed + sdkReadinessManagerMock.readinessManager.isDestroyed = () => true; + + function validateManager() { + expect(manager.split('some_spplit')).toBe(null); + expect(manager.splits()).toEqual([]); + expect(manager.names()).toEqual([]); + } + + validateManager(); + + // SDK is not ready + sdkReadinessManagerMock.readinessManager.isReady = () => false; + sdkReadinessManagerMock.readinessManager.isReadyFromCache = () => false; + sdkReadinessManagerMock.readinessManager.isDestroyed = () => false; + + validateManager(); + }); + }); diff --git a/src/sdkManager/index.ts b/src/sdkManager/index.ts index 1fec8784..670113be 100644 --- a/src/sdkManager/index.ts +++ b/src/sdkManager/index.ts @@ -5,8 +5,8 @@ import { validateSplit, validateSplitExistance, validateIfNotDestroyed, validate import { ISplitsCacheAsync, ISplitsCacheSync } from '../storages/types'; import { ISdkReadinessManager } from '../readiness/types'; import { ISplit } from '../dtos/types'; -import { SplitIO } from '../types'; -import { ILogger } from '../logger/types'; +import { ISettings, SplitIO } from '../types'; +import { isStorageSync } from '../trackers/impressionObserver/utils'; const SPLIT_FN_LABEL = 'split'; const SPLITS_FN_LABEL = 'splits'; @@ -49,11 +49,14 @@ function objectsToViews(splitObjects: ISplit[]) { } export function sdkManagerFactory( - log: ILogger, + settings: Pick, splits: TSplitCache, - { readinessManager, sdkStatus }: ISdkReadinessManager + { readinessManager, sdkStatus }: ISdkReadinessManager, ): TSplitCache extends ISplitsCacheAsync ? SplitIO.IAsyncManager : SplitIO.IManager { + const log = settings.log; + const isSync = isStorageSync(settings); + return objectAssign( // Proto-linkage of the readiness Event Emitter Object.create(sdkStatus), @@ -64,7 +67,7 @@ export function sdkManagerFactory) { return [CONSUMER_MODE, CONSUMER_PARTIAL_MODE].indexOf(settings.mode) === -1 ? true : false; } From 94f666a34b30a380c957d744e2ed37e17510b767 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Thu, 30 Nov 2023 12:04:37 -0300 Subject: [PATCH 29/45] Remove TODO comment --- src/sdkManager/__tests__/index.syncCache.spec.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/sdkManager/__tests__/index.syncCache.spec.ts b/src/sdkManager/__tests__/index.syncCache.spec.ts index 18e03d21..3ca1cfa0 100644 --- a/src/sdkManager/__tests__/index.syncCache.spec.ts +++ b/src/sdkManager/__tests__/index.syncCache.spec.ts @@ -57,8 +57,6 @@ describe('Manager with sync cache (In Memory)', () => { expect(manager.names()).toEqual([]); // If the factory/client is destroyed, `manager.names()` will return empty array either way since the storage is not valid. }); - // @TODO tests for not operational - test('returns empty results when not operational', async () => { // SDK is flagged as destroyed sdkReadinessManagerMock.readinessManager.isDestroyed = () => true; From 12a7952c779f7e59fdc224f3fefd8c505eabd262 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Thu, 30 Nov 2023 12:10:10 -0300 Subject: [PATCH 30/45] Update changelog entry --- CHANGES.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES.txt b/CHANGES.txt index a44fed08..9c8cc18c 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,5 +1,5 @@ 1.11.1 (December XX, 2023) - - Bugfixing - Fixed manager methods to return a promise in consumer modes when called while the SDK is not operational (not ready or destroyed). + - Bugfixing - Fixed manager methods in consumer modes to return results in a promise when the SDK is not operational (not ready or destroyed). 1.11.0 (November 3, 2023) - Added support for Flag Sets on the SDK, which enables grouping feature flags and interacting with the group rather than individually (more details in our documentation): From 81b438265343c99fabfec131d77060f22880dabe Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Thu, 30 Nov 2023 12:53:50 -0300 Subject: [PATCH 31/45] Remove usage of Redis flushdb operation --- CHANGES.txt | 2 +- src/storages/inRedis/RedisAdapter.ts | 2 +- src/storages/inRedis/SegmentsCacheInRedis.ts | 4 ++-- src/storages/inRedis/SplitsCacheInRedis.ts | 10 +++------- src/storages/inRedis/__tests__/RedisAdapter.spec.ts | 2 +- .../inRedis/__tests__/SegmentsCacheInRedis.spec.ts | 13 ++++++------- 6 files changed, 14 insertions(+), 19 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 46996d43..25b37ee5 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,7 +1,7 @@ 1.12.0 (December XX, 2023) - Added support for Flag Sets in "consumer" and "partial consumer" modes for Pluggable and Redis storages. - Updated evaluation flow to log a warning when using flag sets that don't contain cached feature flags. - - Updated Redis adapter to wrap missing commands: 'hincrby', 'popNRaw', 'flushdb' and 'pipeline.exec'. + - Updated Redis adapter to wrap missing commands: 'hincrby', 'popNRaw', and 'pipeline.exec'. - Bugfixing - Fixed manager methods in consumer modes to return results in a promise when the SDK is not operational (not ready or destroyed). 1.11.0 (November 3, 2023) diff --git a/src/storages/inRedis/RedisAdapter.ts b/src/storages/inRedis/RedisAdapter.ts index 191834c4..e6425636 100644 --- a/src/storages/inRedis/RedisAdapter.ts +++ b/src/storages/inRedis/RedisAdapter.ts @@ -8,7 +8,7 @@ import { timeout } from '../../utils/promise/timeout'; const LOG_PREFIX = 'storage:redis-adapter: '; // If we ever decide to fully wrap every method, there's a Commander.getBuiltinCommands from ioredis. -const METHODS_TO_PROMISE_WRAP = ['set', 'exec', 'del', 'get', 'keys', 'sadd', 'srem', 'sismember', 'smembers', 'incr', 'rpush', 'expire', 'mget', 'lrange', 'ltrim', 'hset', 'hincrby', 'popNRaw', 'flushdb']; +const METHODS_TO_PROMISE_WRAP = ['set', 'exec', 'del', 'get', 'keys', 'sadd', 'srem', 'sismember', 'smembers', 'incr', 'rpush', 'expire', 'mget', 'lrange', 'ltrim', 'hset', 'hincrby', 'popNRaw']; // Not part of the settings since it'll vary on each storage. We should be removing storage specific logic from elsewhere. const DEFAULT_OPTIONS = { diff --git a/src/storages/inRedis/SegmentsCacheInRedis.ts b/src/storages/inRedis/SegmentsCacheInRedis.ts index 3a18b535..7ec2f20f 100644 --- a/src/storages/inRedis/SegmentsCacheInRedis.ts +++ b/src/storages/inRedis/SegmentsCacheInRedis.ts @@ -72,8 +72,8 @@ export class SegmentsCacheInRedis implements ISegmentsCacheAsync { return this.redis.smembers(this.keys.buildRegisteredSegmentsKey()); } - // @TODO remove/review. It is not being used. + // @TODO remove or implement. It is not being used. clear() { - return this.redis.flushdb().then(status => status === 'OK'); + return Promise.resolve(); } } diff --git a/src/storages/inRedis/SplitsCacheInRedis.ts b/src/storages/inRedis/SplitsCacheInRedis.ts index a9370ea3..e05c56ba 100644 --- a/src/storages/inRedis/SplitsCacheInRedis.ts +++ b/src/storages/inRedis/SplitsCacheInRedis.ts @@ -213,7 +213,7 @@ export class SplitsCacheInRedis extends AbstractSplitsCacheAsync { /** * Get list of feature flag names related to a given list of flag set names. * The returned promise is resolved with the list of feature flag names per flag set, - * or rejected if the pipelined redis operation fails. + * or rejected if the pipelined redis operation fails (e.g., timeout). */ getNamesByFlagSets(flagSets: string[]): Promise[]> { return this.redis.pipeline(flagSets.map(flagSet => ['smembers', this.keys.buildFlagSetKey(flagSet)])).exec() @@ -252,13 +252,9 @@ export class SplitsCacheInRedis extends AbstractSplitsCacheAsync { }); } - /** - * Delete everything in the current database. - * - * @NOTE documentation says it never fails. - */ + // @TODO remove or implement. It is not being used. clear() { - return this.redis.flushdb().then(status => status === 'OK'); + return Promise.resolve(); } /** diff --git a/src/storages/inRedis/__tests__/RedisAdapter.spec.ts b/src/storages/inRedis/__tests__/RedisAdapter.spec.ts index 90b8743f..b982d06a 100644 --- a/src/storages/inRedis/__tests__/RedisAdapter.spec.ts +++ b/src/storages/inRedis/__tests__/RedisAdapter.spec.ts @@ -11,7 +11,7 @@ const LOG_PREFIX = 'storage:redis-adapter: '; // Mocking ioredis // The list of methods we're wrapping on a promise (for timeout) on the adapter. -const METHODS_TO_PROMISE_WRAP = ['set', 'exec', 'del', 'get', 'keys', 'sadd', 'srem', 'sismember', 'smembers', 'incr', 'rpush', 'expire', 'mget', 'lrange', 'ltrim', 'hset', 'hincrby', 'popNRaw', 'flushdb']; +const METHODS_TO_PROMISE_WRAP = ['set', 'exec', 'del', 'get', 'keys', 'sadd', 'srem', 'sismember', 'smembers', 'incr', 'rpush', 'expire', 'mget', 'lrange', 'ltrim', 'hset', 'hincrby', 'popNRaw']; const ioredisMock = reduce([...METHODS_TO_PROMISE_WRAP, 'disconnect'], (acc, methodName) => { acc[methodName] = jest.fn(() => Promise.resolve(methodName)); diff --git a/src/storages/inRedis/__tests__/SegmentsCacheInRedis.spec.ts b/src/storages/inRedis/__tests__/SegmentsCacheInRedis.spec.ts index 009d6b10..6222af95 100644 --- a/src/storages/inRedis/__tests__/SegmentsCacheInRedis.spec.ts +++ b/src/storages/inRedis/__tests__/SegmentsCacheInRedis.spec.ts @@ -4,7 +4,8 @@ import { loggerMock } from '../../../logger/__tests__/sdkLogger.mock'; import { metadata } from '../../__tests__/KeyBuilder.spec'; import { RedisAdapter } from '../RedisAdapter'; -const keys = new KeyBuilderSS('prefix', metadata); +const prefix = 'prefix'; +const keys = new KeyBuilderSS(prefix, metadata); describe('SEGMENTS CACHE IN REDIS', () => { @@ -12,8 +13,6 @@ describe('SEGMENTS CACHE IN REDIS', () => { const connection = new RedisAdapter(loggerMock); const cache = new SegmentsCacheInRedis(loggerMock, keys, connection); - await cache.clear(); - await cache.addToSegment('mocked-segment', ['a', 'b', 'c']); await cache.setChangeNumber('mocked-segment', 1); @@ -36,7 +35,8 @@ describe('SEGMENTS CACHE IN REDIS', () => { expect(await cache.isInSegment('mocked-segment', 'd')).toBe(true); expect(await cache.isInSegment('mocked-segment', 'e')).toBe(true); - await cache.clear(); + // Teardown + await connection.del(await connection.keys(`${prefix}.segment*`)); // @TODO use `cache.clear` method when implemented await connection.disconnect(); }); @@ -44,8 +44,6 @@ describe('SEGMENTS CACHE IN REDIS', () => { const connection = new RedisAdapter(loggerMock); const cache = new SegmentsCacheInRedis(loggerMock, keys, connection); - await cache.clear(); - await cache.registerSegments(['s1']); await cache.registerSegments(['s2']); await cache.registerSegments(['s2', 's3', 's4']); @@ -54,7 +52,8 @@ describe('SEGMENTS CACHE IN REDIS', () => { ['s1', 's2', 's3', 's4'].forEach(s => expect(segments.indexOf(s) !== -1).toBe(true)); - await cache.clear(); + // Teardown + await connection.del(await connection.keys(`${prefix}.segment*`)); // @TODO use `cache.clear` method when implemented await connection.disconnect(); }); From 355a4bdefe33c23161eeb30536fbb1315c4136da Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Thu, 30 Nov 2023 13:03:07 -0300 Subject: [PATCH 32/45] Update comments --- src/storages/inRedis/__tests__/SplitsCacheInRedis.spec.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/storages/inRedis/__tests__/SplitsCacheInRedis.spec.ts b/src/storages/inRedis/__tests__/SplitsCacheInRedis.spec.ts index c7ad85c7..60867f83 100644 --- a/src/storages/inRedis/__tests__/SplitsCacheInRedis.spec.ts +++ b/src/storages/inRedis/__tests__/SplitsCacheInRedis.spec.ts @@ -52,6 +52,7 @@ describe('SPLITS CACHE REDIS', () => { expect(splits['lol1']).toEqual(null); expect(splits['lol2']).toEqual(splitWithAccountTT); + // Teardown @TODO use cache clear method when implemented await connection.del(keysBuilder.buildTrafficTypeKey('account_tt')); await connection.del(keysBuilder.buildSplitKey('lol2')); await connection.del(keysBuilder.buildSplitsTillKey()); @@ -100,6 +101,7 @@ describe('SPLITS CACHE REDIS', () => { expect(await cache.trafficTypeExists('account_tt')).toBe(true); expect(await cache.trafficTypeExists('user_tt')).toBe(false); + // Teardown @TODO use cache clear method when implemented await connection.del(keysBuilder.buildTrafficTypeKey('account_tt')); await connection.del(keysBuilder.buildSplitKey('malformed')); await connection.del(keysBuilder.buildSplitKey('split1')); From d9cd9fd78e9f745561c69f6bd89e82e647eb815c Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Thu, 30 Nov 2023 13:20:47 -0300 Subject: [PATCH 33/45] Updated changelog entry --- CHANGES.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES.txt b/CHANGES.txt index 46996d43..4d6e3899 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,7 +1,7 @@ 1.12.0 (December XX, 2023) - Added support for Flag Sets in "consumer" and "partial consumer" modes for Pluggable and Redis storages. - Updated evaluation flow to log a warning when using flag sets that don't contain cached feature flags. - - Updated Redis adapter to wrap missing commands: 'hincrby', 'popNRaw', 'flushdb' and 'pipeline.exec'. + - Updated Redis adapter to handle timeouts and queueing of some missing commands: 'hincrby', 'popNRaw', and 'pipeline.exec'. - Bugfixing - Fixed manager methods in consumer modes to return results in a promise when the SDK is not operational (not ready or destroyed). 1.11.0 (November 3, 2023) From d2d7f2fbb8a3f2699c23710f28bf48022c8c0ec5 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Thu, 30 Nov 2023 15:39:49 -0300 Subject: [PATCH 34/45] Nico's feedback --- CHANGES.txt | 2 +- src/storages/inRedis/RedisAdapter.ts | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 4d6e3899..9f3f4063 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,7 +1,7 @@ 1.12.0 (December XX, 2023) - Added support for Flag Sets in "consumer" and "partial consumer" modes for Pluggable and Redis storages. - Updated evaluation flow to log a warning when using flag sets that don't contain cached feature flags. - - Updated Redis adapter to handle timeouts and queueing of some missing commands: 'hincrby', 'popNRaw', and 'pipeline.exec'. + - Updated Redis adapter to handle timeouts and queueing of some missing commands: 'hincrby' and 'popNRaw'. - Bugfixing - Fixed manager methods in consumer modes to return results in a promise when the SDK is not operational (not ready or destroyed). 1.11.0 (November 3, 2023) diff --git a/src/storages/inRedis/RedisAdapter.ts b/src/storages/inRedis/RedisAdapter.ts index e6425636..6365a8b8 100644 --- a/src/storages/inRedis/RedisAdapter.ts +++ b/src/storages/inRedis/RedisAdapter.ts @@ -38,7 +38,7 @@ export class RedisAdapter extends ioredis { private _notReadyCommandsQueue?: IRedisCommand[]; private _runningCommands: ISet>; - constructor(log: ILogger, storageSettings?: Record) { + constructor(log: ILogger, storageSettings: Record = {}) { const options = RedisAdapter._defineOptions(storageSettings); // Call the ioredis constructor super(...RedisAdapter._defineLibrarySettings(options)); @@ -86,7 +86,7 @@ export class RedisAdapter extends ioredis { // For handling pending commands on disconnect, add to the set and remove once finished. // On sync commands there's no need, only thenables. instance._runningCommands.add(result); - const cleanUpRunningCommandsCb = () => { + const cleanUpRunningCommandsCb = function () { instance._runningCommands.delete(result); }; // Both success and error remove from queue. @@ -177,7 +177,7 @@ export class RedisAdapter extends ioredis { /** * Parses the options into what we care about. */ - static _defineOptions({ connectionTimeout, operationTimeout, url, host, port, db, pass, tls }: Record = {}) { + static _defineOptions({ connectionTimeout, operationTimeout, url, host, port, db, pass, tls }: Record) { const parsedOptions = { connectionTimeout, operationTimeout, url, host, port, db, pass, tls }; From 58ca122ddcd159c2124bfbfdaea7fec75f354f74 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Thu, 30 Nov 2023 15:59:12 -0300 Subject: [PATCH 35/45] Polishing --- src/storages/inRedis/RedisAdapter.ts | 36 ++++++++++++---------------- 1 file changed, 15 insertions(+), 21 deletions(-) diff --git a/src/storages/inRedis/RedisAdapter.ts b/src/storages/inRedis/RedisAdapter.ts index 1c4b3ba7..6d738606 100644 --- a/src/storages/inRedis/RedisAdapter.ts +++ b/src/storages/inRedis/RedisAdapter.ts @@ -73,29 +73,14 @@ export class RedisAdapter extends ioredis { _setTimeoutWrappers() { const instance: Record = this; - const wrapWithQueueOrExecute = (method: string, commandWrapper: Function) => { - if (instance._notReadyCommandsQueue) { - return new Promise((resolve, reject) => { - instance._notReadyCommandsQueue.unshift({ - resolve, - reject, - command: commandWrapper, - name: method.toUpperCase() - }); - }); - } else { - return commandWrapper(); - } - }; - const wrapCommand = (originalMethod: Function, methodName: string) => { // The value of "this" in this function should be the instance actually executing the method. It might be the instance referred (the base one) // or it can be the instance of a Pipeline object. return function (this: RedisAdapter | Pipeline) { const params = arguments; - const caller: (Pipeline | RedisAdapter) = this; + const caller = this; - const commandWrapper = () => { + function commandWrapper() { instance.log.debug(`${LOG_PREFIX}Executing ${methodName}.`); const result = originalMethod.apply(caller, params); @@ -117,9 +102,20 @@ export class RedisAdapter extends ioredis { } return result; - }; + } - return wrapWithQueueOrExecute(methodName, commandWrapper); + if (instance._notReadyCommandsQueue) { + return new Promise((resolve, reject) => { + instance._notReadyCommandsQueue.unshift({ + resolve, + reject, + command: commandWrapper, + name: methodName.toUpperCase() + }); + }); + } else { + return commandWrapper(); + } }; }; @@ -134,8 +130,6 @@ export class RedisAdapter extends ioredis { const originalFn = instance[methodName]; // "First level wrapper" to handle the sync execution and wrap async, queueing later if applicable. instance[methodName] = function () { - instance.log.debug(`${LOG_PREFIX} Creating ${methodName}.`); - const res = originalFn.apply(instance, arguments); const originalExec = res.exec; From e44196ce8a79ded95219d350270f9805a6f1cdc8 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Thu, 30 Nov 2023 17:25:21 -0300 Subject: [PATCH 36/45] Unit test --- package-lock.json | 4 +- package.json | 2 +- .../inRedis/__tests__/RedisAdapter.spec.ts | 42 ++++++++++++++----- 3 files changed, 35 insertions(+), 13 deletions(-) diff --git a/package-lock.json b/package-lock.json index f131008e..416f9ff2 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@splitsoftware/splitio-commons", - "version": "1.12.1-rc.2", + "version": "1.12.1-rc.3", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "@splitsoftware/splitio-commons", - "version": "1.12.1-rc.2", + "version": "1.12.1-rc.3", "license": "Apache-2.0", "dependencies": { "tslib": "^2.3.1" diff --git a/package.json b/package.json index df3097c7..b4bd3540 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@splitsoftware/splitio-commons", - "version": "1.12.1-rc.2", + "version": "1.12.1-rc.3", "description": "Split Javascript SDK common components", "main": "cjs/index.js", "module": "esm/index.js", diff --git a/src/storages/inRedis/__tests__/RedisAdapter.spec.ts b/src/storages/inRedis/__tests__/RedisAdapter.spec.ts index b982d06a..a8ef69da 100644 --- a/src/storages/inRedis/__tests__/RedisAdapter.spec.ts +++ b/src/storages/inRedis/__tests__/RedisAdapter.spec.ts @@ -12,13 +12,27 @@ const LOG_PREFIX = 'storage:redis-adapter: '; // The list of methods we're wrapping on a promise (for timeout) on the adapter. const METHODS_TO_PROMISE_WRAP = ['set', 'exec', 'del', 'get', 'keys', 'sadd', 'srem', 'sismember', 'smembers', 'incr', 'rpush', 'expire', 'mget', 'lrange', 'ltrim', 'hset', 'hincrby', 'popNRaw']; +const METHODS_TO_PROMISE_WRAP_EXEC = ['pipeline']; +const pipelineExecMock = jest.fn(() => Promise.resolve('exec')); const ioredisMock = reduce([...METHODS_TO_PROMISE_WRAP, 'disconnect'], (acc, methodName) => { acc[methodName] = jest.fn(() => Promise.resolve(methodName)); return acc; +}, reduce(METHODS_TO_PROMISE_WRAP_EXEC, (acc, methodName) => { + acc[methodName] = jest.fn(() => { + const pipelineAlikeMock = Object.assign(reduce(METHODS_TO_PROMISE_WRAP, (acc, methodName) => { + acc[methodName] = jest.fn(() => pipelineAlikeMock); + return acc; + }, {}), { + exec: pipelineExecMock + }); + + return pipelineAlikeMock; + }); + return acc; }, { once: jest.fn() -}) as { once: jest.Mock }; +}) as { once: jest.Mock }); let constructorParams: any = false; @@ -247,14 +261,16 @@ describe('STORAGE Redis Adapter', () => { url: 'redis://localhost:6379/0' }); - forEach(METHODS_TO_PROMISE_WRAP, methodName => { + forEach([...METHODS_TO_PROMISE_WRAP, ...METHODS_TO_PROMISE_WRAP_EXEC], methodName => { expect(instance[methodName]).not.toBe(ioredisMock[methodName]); // Method "${methodName}" from ioredis library should be wrapped. expect(ioredisMock[methodName]).not.toBeCalled(); // Checking that the method was not called yet. const startingQueueLength = instance._notReadyCommandsQueue.length; // We do have the commands queue on this state, so a call for this methods will queue the command. - const wrapperResult = instance[methodName](methodName); + const wrapperResult = METHODS_TO_PROMISE_WRAP_EXEC.includes(methodName) ? + instance[methodName](methodName).exec() : + instance[methodName](methodName); expect(wrapperResult instanceof Promise).toBe(true); // The result is a promise since we are queueing commands on this state. expect(instance._notReadyCommandsQueue.length).toBe(startingQueueLength + 1); // The queue should have one more item. @@ -263,19 +279,24 @@ describe('STORAGE Redis Adapter', () => { expect(typeof queuedCommand.resolve).toBe('function'); // The queued item should have the correct form. expect(typeof queuedCommand.reject).toBe('function'); // The queued item should have the correct form. expect(typeof queuedCommand.command).toBe('function'); // The queued item should have the correct form. - expect(queuedCommand.name).toBe(methodName.toUpperCase()); // The queued item should have the correct form. + expect(queuedCommand.name).toBe((METHODS_TO_PROMISE_WRAP_EXEC.includes(methodName) ? methodName + '.exec' : methodName).toUpperCase()); // The queued item should have the correct form. }); instance._notReadyCommandsQueue = false; // Remove the queue. loggerMock.error.resetHistory; - forEach(METHODS_TO_PROMISE_WRAP, (methodName, index) => { + forEach([...METHODS_TO_PROMISE_WRAP, ...METHODS_TO_PROMISE_WRAP_EXEC], (methodName, index) => { // We do NOT have the commands queue on this state, so a call for this methods will execute the command. - expect(ioredisMock[methodName]).not.toBeCalled(); // Control assertion - Original method (${methodName}) was not called yet + if (METHODS_TO_PROMISE_WRAP.includes(methodName)) expect(ioredisMock[methodName]).not.toBeCalled(); // Control assertion - Original method (${methodName}) was not called yet + else expect(pipelineExecMock).not.toBeCalled(); // Control assertion - Original Pipeline exec method was not called yet const previousTimeoutCalls = timeout.mock.calls.length; let previousRunningCommandsSize = instance._runningCommands.size; - instance[methodName](methodName).catch(() => { }); // Swallow exception so it's not spread to logs. + + (METHODS_TO_PROMISE_WRAP_EXEC.includes(methodName) ? + instance[methodName](methodName).exec() : + instance[methodName](methodName) + ).catch(() => { }); // Swallow exception so it's not spread to logs. expect(ioredisMock[methodName]).toBeCalled(); // Original method (${methodName}) is called right away (through wrapper) when we are not queueing anymore. expect(instance._runningCommands.size).toBe(previousRunningCommandsSize + 1); // If the result of the operation was a thenable it will add the item to the running commands queue. @@ -290,7 +311,7 @@ describe('STORAGE Redis Adapter', () => { commandTimeoutResolver.rej('test'); setTimeout(() => { // Allow the promises to tick. expect(instance._runningCommands.has(commandTimeoutResolver.originalPromise)).toBe(false); // After a command finishes with error, it's promise is removed from the instance._runningCommands queue. - expect(loggerMock.error.mock.calls[index]).toEqual([`${LOG_PREFIX}${methodName} operation threw an error or exceeded configured timeout of 5000ms. Message: test`]); // The log error method should be called with the corresponding messages, depending on the method, error and operationTimeout. + expect(loggerMock.error.mock.calls[index]).toEqual([`${LOG_PREFIX}${METHODS_TO_PROMISE_WRAP_EXEC.includes(methodName) ? methodName + '.exec' : methodName} operation threw an error or exceeded configured timeout of 5000ms. Message: test`]); // The log error method should be called with the corresponding messages, depending on the method, error and operationTimeout. }, 0); }); @@ -306,9 +327,10 @@ describe('STORAGE Redis Adapter', () => { instance._notReadyCommandsQueue = false; // Connection is "ready" - forEach(METHODS_TO_PROMISE_WRAP, methodName => { + forEach([...METHODS_TO_PROMISE_WRAP, ...METHODS_TO_PROMISE_WRAP_EXEC], methodName => { // Just call the wrapped method, we don't care about all the paths tested on the previous case, just how it behaves when the command is resolved. - instance[methodName](methodName); + if (METHODS_TO_PROMISE_WRAP_EXEC.includes(methodName)) instance[methodName](methodName).exec(); + else instance[methodName](methodName); // Get the original promise (the one passed to timeout) const commandTimeoutResolver = timeoutPromiseResolvers[0]; From b2a2cb0bf1b0f5f0280cf7cf9d4561e81c068163 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Fri, 1 Dec 2023 13:26:38 -0300 Subject: [PATCH 37/45] Update to fix some logs that were not printing properly the name of flag sets evaluation methods --- src/sdkClient/client.ts | 30 ++++++++++++------------ src/sdkClient/clientInputValidation.ts | 32 +++++++++++++------------- src/sdkManager/index.ts | 5 +--- src/utils/constants/index.ts | 16 +++++++++++++ 4 files changed, 48 insertions(+), 35 deletions(-) diff --git a/src/sdkClient/client.ts b/src/sdkClient/client.ts index 1663af92..8da54dd3 100644 --- a/src/sdkClient/client.ts +++ b/src/sdkClient/client.ts @@ -4,7 +4,7 @@ import { getMatching, getBucketing } from '../utils/key'; import { validateSplitExistence } from '../utils/inputValidation/splitExistence'; import { validateTrafficTypeExistence } from '../utils/inputValidation/trafficTypeExistence'; import { SDK_NOT_READY } from '../utils/labels'; -import { CONTROL, TREATMENT, TREATMENTS, TREATMENT_WITH_CONFIG, TREATMENTS_WITH_CONFIG, TRACK, TREATMENTS_WITH_CONFIG_BY_FLAGSETS, TREATMENTS_BY_FLAGSETS, TREATMENTS_BY_FLAGSET, TREATMENTS_WITH_CONFIG_BY_FLAGSET } from '../utils/constants'; +import { CONTROL, TREATMENT, TREATMENTS, TREATMENT_WITH_CONFIG, TREATMENTS_WITH_CONFIG, TRACK, TREATMENTS_WITH_CONFIG_BY_FLAGSETS, TREATMENTS_BY_FLAGSETS, TREATMENTS_BY_FLAGSET, TREATMENTS_WITH_CONFIG_BY_FLAGSET, GET_TREATMENTS_WITH_CONFIG, GET_TREATMENTS_BY_FLAG_SETS, GET_TREATMENTS_WITH_CONFIG_BY_FLAG_SETS, GET_TREATMENTS_BY_FLAG_SET, GET_TREATMENTS_WITH_CONFIG_BY_FLAG_SET, GET_TREATMENT_WITH_CONFIG, GET_TREATMENT, GET_TREATMENTS, TRACK_FN_LABEL } from '../utils/constants'; import { IEvaluationResult } from '../evaluator/types'; import { SplitIO, ImpressionDTO } from '../types'; import { IMPRESSION, IMPRESSION_QUEUEING } from '../logger/constants'; @@ -29,12 +29,12 @@ export function clientFactory(params: ISdkFactoryContext): SplitIO.IClient | Spl const { sdkReadinessManager: { readinessManager }, storage, settings, impressionsTracker, eventTracker, telemetryTracker } = params; const { log, mode } = settings; - function getTreatment(key: SplitIO.SplitKey, featureFlagName: string, attributes: SplitIO.Attributes | undefined, withConfig = false) { + function getTreatment(key: SplitIO.SplitKey, featureFlagName: string, attributes: SplitIO.Attributes | undefined, withConfig = false, methodName = GET_TREATMENT) { const stopTelemetryTracker = telemetryTracker.trackEval(withConfig ? TREATMENT_WITH_CONFIG : TREATMENT); const wrapUp = (evaluationResult: IEvaluationResult) => { const queue: ImpressionDTO[] = []; - const treatment = processEvaluation(evaluationResult, featureFlagName, key, attributes, withConfig, `getTreatment${withConfig ? 'withConfig' : ''}`, queue); + const treatment = processEvaluation(evaluationResult, featureFlagName, key, attributes, withConfig, methodName, queue); impressionsTracker.track(queue, attributes); stopTelemetryTracker(queue[0] && queue[0].label); @@ -51,17 +51,17 @@ export function clientFactory(params: ISdkFactoryContext): SplitIO.IClient | Spl } function getTreatmentWithConfig(key: SplitIO.SplitKey, featureFlagName: string, attributes: SplitIO.Attributes | undefined) { - return getTreatment(key, featureFlagName, attributes, true); + return getTreatment(key, featureFlagName, attributes, true, GET_TREATMENT_WITH_CONFIG); } - function getTreatments(key: SplitIO.SplitKey, featureFlagNames: string[], attributes: SplitIO.Attributes | undefined, withConfig = false) { + function getTreatments(key: SplitIO.SplitKey, featureFlagNames: string[], attributes: SplitIO.Attributes | undefined, withConfig = false, methodName = GET_TREATMENTS) { const stopTelemetryTracker = telemetryTracker.trackEval(withConfig ? TREATMENTS_WITH_CONFIG : TREATMENTS); const wrapUp = (evaluationResults: Record) => { const queue: ImpressionDTO[] = []; const treatments: Record = {}; Object.keys(evaluationResults).forEach(featureFlagName => { - treatments[featureFlagName] = processEvaluation(evaluationResults[featureFlagName], featureFlagName, key, attributes, withConfig, `getTreatments${withConfig ? 'withConfig' : ''}`, queue); + treatments[featureFlagName] = processEvaluation(evaluationResults[featureFlagName], featureFlagName, key, attributes, withConfig, methodName, queue); }); impressionsTracker.track(queue, attributes); @@ -79,18 +79,18 @@ export function clientFactory(params: ISdkFactoryContext): SplitIO.IClient | Spl } function getTreatmentsWithConfig(key: SplitIO.SplitKey, featureFlagNames: string[], attributes: SplitIO.Attributes | undefined) { - return getTreatments(key, featureFlagNames, attributes, true); + return getTreatments(key, featureFlagNames, attributes, true, GET_TREATMENTS_WITH_CONFIG); } - function getTreatmentsByFlagSets(key: SplitIO.SplitKey, flagSetNames: string[], attributes: SplitIO.Attributes | undefined, withConfig = false, method: Method = TREATMENTS_BY_FLAGSETS) { + function getTreatmentsByFlagSets(key: SplitIO.SplitKey, flagSetNames: string[], attributes: SplitIO.Attributes | undefined, withConfig = false, method: Method = TREATMENTS_BY_FLAGSETS, methodName = GET_TREATMENTS_BY_FLAG_SETS) { const stopTelemetryTracker = telemetryTracker.trackEval(method); - const wrapUp = (evaluationResults: Record) => { + const wrapUp = (evaluationResults: Record) => { const queue: ImpressionDTO[] = []; const treatments: Record = {}; const evaluations = evaluationResults; Object.keys(evaluations).forEach(featureFlagName => { - treatments[featureFlagName] = processEvaluation(evaluations[featureFlagName], featureFlagName, key, attributes, withConfig, `getTreatmentsByFlagSets${withConfig ? 'WithConfig' : ''}`, queue); + treatments[featureFlagName] = processEvaluation(evaluations[featureFlagName], featureFlagName, key, attributes, withConfig, methodName, queue); }); impressionsTracker.track(queue, attributes); @@ -99,22 +99,22 @@ export function clientFactory(params: ISdkFactoryContext): SplitIO.IClient | Spl }; const evaluations = readinessManager.isReady() || readinessManager.isReadyFromCache() ? - evaluateFeaturesByFlagSets(log, key, flagSetNames, attributes, storage, method) : + evaluateFeaturesByFlagSets(log, key, flagSetNames, attributes, storage, methodName) : isStorageSync(settings) ? {} : Promise.resolve({}); // Promisify if async return thenable(evaluations) ? evaluations.then((res) => wrapUp(res)) : wrapUp(evaluations); } function getTreatmentsWithConfigByFlagSets(key: SplitIO.SplitKey, flagSetNames: string[], attributes: SplitIO.Attributes | undefined) { - return getTreatmentsByFlagSets(key, flagSetNames, attributes, true, TREATMENTS_WITH_CONFIG_BY_FLAGSETS); + return getTreatmentsByFlagSets(key, flagSetNames, attributes, true, TREATMENTS_WITH_CONFIG_BY_FLAGSETS, GET_TREATMENTS_WITH_CONFIG_BY_FLAG_SETS); } function getTreatmentsByFlagSet(key: SplitIO.SplitKey, flagSetName: string, attributes: SplitIO.Attributes | undefined) { - return getTreatmentsByFlagSets(key, [flagSetName], attributes, false, TREATMENTS_BY_FLAGSET); + return getTreatmentsByFlagSets(key, [flagSetName], attributes, false, TREATMENTS_BY_FLAGSET, GET_TREATMENTS_BY_FLAG_SET); } function getTreatmentsWithConfigByFlagSet(key: SplitIO.SplitKey, flagSetName: string, attributes: SplitIO.Attributes | undefined) { - return getTreatmentsByFlagSets(key, [flagSetName], attributes, true, TREATMENTS_WITH_CONFIG_BY_FLAGSET); + return getTreatmentsByFlagSets(key, [flagSetName], attributes, true, TREATMENTS_WITH_CONFIG_BY_FLAGSET, GET_TREATMENTS_WITH_CONFIG_BY_FLAG_SET); } // Internal function @@ -171,7 +171,7 @@ export function clientFactory(params: ISdkFactoryContext): SplitIO.IClient | Spl }; // This may be async but we only warn, we don't actually care if it is valid or not in terms of queueing the event. - validateTrafficTypeExistence(log, readinessManager, storage.splits, mode, trafficTypeName, 'track'); + validateTrafficTypeExistence(log, readinessManager, storage.splits, mode, trafficTypeName, TRACK_FN_LABEL); const result = eventTracker.track(eventData, size); diff --git a/src/sdkClient/clientInputValidation.ts b/src/sdkClient/clientInputValidation.ts index da0b87ff..f384ad22 100644 --- a/src/sdkClient/clientInputValidation.ts +++ b/src/sdkClient/clientInputValidation.ts @@ -12,7 +12,7 @@ import { validateIfOperational } from '../utils/inputValidation'; import { startsWith } from '../utils/lang'; -import { CONTROL, CONTROL_WITH_CONFIG } from '../utils/constants'; +import { CONTROL, CONTROL_WITH_CONFIG, GET_TREATMENT, GET_TREATMENTS, GET_TREATMENTS_BY_FLAG_SET, GET_TREATMENTS_BY_FLAG_SETS, GET_TREATMENTS_WITH_CONFIG, GET_TREATMENTS_WITH_CONFIG_BY_FLAG_SET, GET_TREATMENTS_WITH_CONFIG_BY_FLAG_SETS, GET_TREATMENT_WITH_CONFIG, TRACK_FN_LABEL } from '../utils/constants'; import { IReadinessManager } from '../readiness/types'; import { MaybeThenable } from '../dtos/types'; import { ISettings, SplitIO } from '../types'; @@ -32,7 +32,7 @@ export function clientInputValidationDecorator Date: Fri, 1 Dec 2023 13:29:43 -0300 Subject: [PATCH 38/45] rc --- package-lock.json | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index 416f9ff2..d19598ea 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@splitsoftware/splitio-commons", - "version": "1.12.1-rc.3", + "version": "1.12.1-rc.4", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "@splitsoftware/splitio-commons", - "version": "1.12.1-rc.3", + "version": "1.12.1-rc.4", "license": "Apache-2.0", "dependencies": { "tslib": "^2.3.1" diff --git a/package.json b/package.json index b4bd3540..d666fba7 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@splitsoftware/splitio-commons", - "version": "1.12.1-rc.3", + "version": "1.12.1-rc.4", "description": "Split Javascript SDK common components", "main": "cjs/index.js", "module": "esm/index.js", From b06e1a243886f08f74ff5c97287d06b43aaca317 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Fri, 1 Dec 2023 17:10:34 -0300 Subject: [PATCH 39/45] Rename flagSetsAreValid function to validateFlagSets for consistency with other validation function names --- src/sdkClient/clientInputValidation.ts | 4 +-- .../__tests__/splitFilters.spec.ts | 36 +++++++++---------- src/utils/settingsValidation/splitFilters.ts | 4 +-- 3 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/sdkClient/clientInputValidation.ts b/src/sdkClient/clientInputValidation.ts index f384ad22..c5b49b49 100644 --- a/src/sdkClient/clientInputValidation.ts +++ b/src/sdkClient/clientInputValidation.ts @@ -17,7 +17,7 @@ import { IReadinessManager } from '../readiness/types'; import { MaybeThenable } from '../dtos/types'; import { ISettings, SplitIO } from '../types'; import { isStorageSync } from '../trackers/impressionObserver/utils'; -import { flagSetsAreValid } from '../utils/settingsValidation/splitFilters'; +import { validateFlagSets } from '../utils/settingsValidation/splitFilters'; /** * Decorator that validates the input before actually executing the client methods. @@ -42,7 +42,7 @@ export function clientInputValidationDecorator { @@ -131,71 +131,71 @@ describe('validateSplitFilters', () => { expect(loggerMock.error.mock.calls.length).toEqual(3); }); - test('flagSetsAreValid - Flag set validation for evaluations', () => { + test('validateFlagSets - Flag set validation for evaluations', () => { let flagSetsFilter = ['set_1', 'set_2']; // empty array - expect(flagSetsAreValid(loggerMock, 'test_method', [], flagSetsFilter)).toEqual([]); + expect(validateFlagSets(loggerMock, 'test_method', [], flagSetsFilter)).toEqual([]); // must start with a letter or number - expect(flagSetsAreValid(loggerMock, 'test_method', ['_set_1'], flagSetsFilter)).toEqual([]); + expect(validateFlagSets(loggerMock, 'test_method', ['_set_1'], flagSetsFilter)).toEqual([]); expect(loggerMock.warn.mock.calls[0]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['_set_1', regexp, '_set_1']]); // can contain _a-z0-9 - expect(flagSetsAreValid(loggerMock, 'test_method', ['set*1'], flagSetsFilter)).toEqual([]); + expect(validateFlagSets(loggerMock, 'test_method', ['set*1'], flagSetsFilter)).toEqual([]); expect(loggerMock.warn.mock.calls[1]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set*1', regexp, 'set*1']]); // have a max length of 50 characters const longName = '1234567890_1234567890_1234567890_1234567890_1234567890'; - expect(flagSetsAreValid(loggerMock, 'test_method', [longName], flagSetsFilter)).toEqual([]); + expect(validateFlagSets(loggerMock, 'test_method', [longName], flagSetsFilter)).toEqual([]); expect(loggerMock.warn.mock.calls[2]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, [longName, regexp, longName]]); // both set names invalid -> empty list & warn - expect(flagSetsAreValid(loggerMock, 'test_method', ['set*1', 'set*3'], flagSetsFilter)).toEqual([]); + expect(validateFlagSets(loggerMock, 'test_method', ['set*1', 'set*3'], flagSetsFilter)).toEqual([]); expect(loggerMock.warn.mock.calls[3]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set*1', regexp, 'set*1']]); expect(loggerMock.warn.mock.calls[4]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set*3', regexp, 'set*3']]); // only set_1 is valid => [set_1] & warn - expect(flagSetsAreValid(loggerMock, 'test_method', ['set_1', 'set*3'], flagSetsFilter)).toEqual(['set_1']); + expect(validateFlagSets(loggerMock, 'test_method', ['set_1', 'set*3'], flagSetsFilter)).toEqual(['set_1']); expect(loggerMock.warn.mock.calls[5]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set*3', regexp, 'set*3']]); // set_3 not included in configuration but set_1 included => [set_1] & warn - expect(flagSetsAreValid(loggerMock, 'test_method', ['set_1', 'set_3'], flagSetsFilter)).toEqual(['set_1']); + expect(validateFlagSets(loggerMock, 'test_method', ['set_1', 'set_3'], flagSetsFilter)).toEqual(['set_1']); expect(loggerMock.warn.mock.calls[6]).toEqual([WARN_FLAGSET_NOT_CONFIGURED, ['test_method', 'set_3']]); // set_3 not included in configuration => [] & warn - expect(flagSetsAreValid(loggerMock, 'test_method', ['set_3'], flagSetsFilter)).toEqual([]); + expect(validateFlagSets(loggerMock, 'test_method', ['set_3'], flagSetsFilter)).toEqual([]); expect(loggerMock.warn.mock.calls[7]).toEqual([WARN_FLAGSET_NOT_CONFIGURED, ['test_method', 'set_3']]); // empty config // must start with a letter or number - expect(flagSetsAreValid(loggerMock, 'test_method', ['_set_1'], [])).toEqual([]); + expect(validateFlagSets(loggerMock, 'test_method', ['_set_1'], [])).toEqual([]); expect(loggerMock.warn.mock.calls[8]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['_set_1', regexp, '_set_1']]); // can contain _a-z0-9 - expect(flagSetsAreValid(loggerMock, 'test_method', ['set*1'], [])).toEqual([]); + expect(validateFlagSets(loggerMock, 'test_method', ['set*1'], [])).toEqual([]); expect(loggerMock.warn.mock.calls[9]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set*1', regexp, 'set*1']]); // have a max length of 50 characters - expect(flagSetsAreValid(loggerMock, 'test_method', [longName], [])).toEqual([]); + expect(validateFlagSets(loggerMock, 'test_method', [longName], [])).toEqual([]); expect(loggerMock.warn.mock.calls[10]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, [longName, regexp, longName]]); // both set names invalid -> empty list & warn - expect(flagSetsAreValid(loggerMock, 'test_method', ['set*1', 'set*3'], [])).toEqual([]); + expect(validateFlagSets(loggerMock, 'test_method', ['set*1', 'set*3'], [])).toEqual([]); expect(loggerMock.warn.mock.calls[11]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set*1', regexp, 'set*1']]); expect(loggerMock.warn.mock.calls[12]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set*3', regexp, 'set*3']]); // only set_1 is valid => [set_1] & warn - expect(flagSetsAreValid(loggerMock, 'test_method', ['set_1', 'set*3'], [])).toEqual(['set_1']); + expect(validateFlagSets(loggerMock, 'test_method', ['set_1', 'set*3'], [])).toEqual(['set_1']); expect(loggerMock.warn.mock.calls[13]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set*3', regexp, 'set*3']]); // any set should be returned if there isn't flag sets in filter - expect(flagSetsAreValid(loggerMock, 'test_method', ['set_1'], [])).toEqual(['set_1']); - expect(flagSetsAreValid(loggerMock, 'test_method', ['set_1', 'set_2'], [])).toEqual(['set_1', 'set_2']); - expect(flagSetsAreValid(loggerMock, 'test_method', ['set_3'], [])).toEqual(['set_3']); + expect(validateFlagSets(loggerMock, 'test_method', ['set_1'], [])).toEqual(['set_1']); + expect(validateFlagSets(loggerMock, 'test_method', ['set_1', 'set_2'], [])).toEqual(['set_1', 'set_2']); + expect(validateFlagSets(loggerMock, 'test_method', ['set_3'], [])).toEqual(['set_3']); }); diff --git a/src/utils/settingsValidation/splitFilters.ts b/src/utils/settingsValidation/splitFilters.ts index a1d9e6eb..74548e36 100644 --- a/src/utils/settingsValidation/splitFilters.ts +++ b/src/utils/settingsValidation/splitFilters.ts @@ -88,7 +88,7 @@ function queryStringBuilder(groupedFilters: Record 0) { From 6eba36d2d2b82aa33fd0bc2d54cbb8378ccb07ad Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Fri, 1 Dec 2023 17:24:43 -0300 Subject: [PATCH 40/45] prepare stable version --- CHANGES.txt | 2 +- package-lock.json | 4 ++-- package.json | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 4d6e3899..5071bf03 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,4 @@ -1.12.0 (December XX, 2023) +1.12.0 (December 4, 2023) - Added support for Flag Sets in "consumer" and "partial consumer" modes for Pluggable and Redis storages. - Updated evaluation flow to log a warning when using flag sets that don't contain cached feature flags. - Updated Redis adapter to handle timeouts and queueing of some missing commands: 'hincrby', 'popNRaw', and 'pipeline.exec'. diff --git a/package-lock.json b/package-lock.json index d19598ea..048f996a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@splitsoftware/splitio-commons", - "version": "1.12.1-rc.4", + "version": "1.12.0", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "@splitsoftware/splitio-commons", - "version": "1.12.1-rc.4", + "version": "1.12.0", "license": "Apache-2.0", "dependencies": { "tslib": "^2.3.1" diff --git a/package.json b/package.json index d666fba7..7241aa86 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@splitsoftware/splitio-commons", - "version": "1.12.1-rc.4", + "version": "1.12.0", "description": "Split Javascript SDK common components", "main": "cjs/index.js", "module": "esm/index.js", From b62a6530f770845472978ae2d3c191a33b15602a Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Mon, 4 Dec 2023 11:25:49 -0300 Subject: [PATCH 41/45] rename function --- src/storages/inRedis/SplitsCacheInRedis.ts | 6 +++--- src/storages/pluggable/SplitsCachePluggable.ts | 6 +++--- src/utils/lang/sets.ts | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/storages/inRedis/SplitsCacheInRedis.ts b/src/storages/inRedis/SplitsCacheInRedis.ts index e05c56ba..8822647e 100644 --- a/src/storages/inRedis/SplitsCacheInRedis.ts +++ b/src/storages/inRedis/SplitsCacheInRedis.ts @@ -4,7 +4,7 @@ import { ILogger } from '../../logger/types'; import { LOG_PREFIX } from './constants'; import { ISplit, ISplitFiltersValidation } from '../../dtos/types'; import { AbstractSplitsCacheAsync } from '../AbstractSplitsCacheAsync'; -import { ISet, _Set, returnListDifference } from '../../utils/lang/sets'; +import { ISet, _Set, returnDifference } from '../../utils/lang/sets'; import type { RedisAdapter } from './RedisAdapter'; /** @@ -60,9 +60,9 @@ export class SplitsCacheInRedis extends AbstractSplitsCacheAsync { } private _updateFlagSets(featureFlagName: string, flagSetsOfRemovedFlag?: string[], flagSetsOfAddedFlag?: string[]) { - const removeFromFlagSets = returnListDifference(flagSetsOfRemovedFlag, flagSetsOfAddedFlag); + const removeFromFlagSets = returnDifference(flagSetsOfRemovedFlag, flagSetsOfAddedFlag); - let addToFlagSets = returnListDifference(flagSetsOfAddedFlag, flagSetsOfRemovedFlag); + let addToFlagSets = returnDifference(flagSetsOfAddedFlag, flagSetsOfRemovedFlag); if (this.flagSetsFilter.length > 0) { addToFlagSets = addToFlagSets.filter(flagSet => { return this.flagSetsFilter.some(filterFlagSet => filterFlagSet === flagSet); diff --git a/src/storages/pluggable/SplitsCachePluggable.ts b/src/storages/pluggable/SplitsCachePluggable.ts index 9ed55b9f..d35299f6 100644 --- a/src/storages/pluggable/SplitsCachePluggable.ts +++ b/src/storages/pluggable/SplitsCachePluggable.ts @@ -5,7 +5,7 @@ import { ILogger } from '../../logger/types'; import { ISplit, ISplitFiltersValidation } from '../../dtos/types'; import { LOG_PREFIX } from './constants'; import { AbstractSplitsCacheAsync } from '../AbstractSplitsCacheAsync'; -import { ISet, _Set, returnListDifference } from '../../utils/lang/sets'; +import { ISet, _Set, returnDifference } from '../../utils/lang/sets'; /** * ISplitsCacheAsync implementation for pluggable storages. @@ -44,9 +44,9 @@ export class SplitsCachePluggable extends AbstractSplitsCacheAsync { } private _updateFlagSets(featureFlagName: string, flagSetsOfRemovedFlag?: string[], flagSetsOfAddedFlag?: string[]) { - const removeFromFlagSets = returnListDifference(flagSetsOfRemovedFlag, flagSetsOfAddedFlag); + const removeFromFlagSets = returnDifference(flagSetsOfRemovedFlag, flagSetsOfAddedFlag); - let addToFlagSets = returnListDifference(flagSetsOfAddedFlag, flagSetsOfRemovedFlag); + let addToFlagSets = returnDifference(flagSetsOfAddedFlag, flagSetsOfRemovedFlag); if (this.flagSetsFilter.length > 0) { addToFlagSets = addToFlagSets.filter(flagSet => { return this.flagSetsFilter.some(filterFlagSet => filterFlagSet === flagSet); diff --git a/src/utils/lang/sets.ts b/src/utils/lang/sets.ts index bda2c612..d8d63e7a 100644 --- a/src/utils/lang/sets.ts +++ b/src/utils/lang/sets.ts @@ -120,7 +120,7 @@ export function returnSetsUnion(set: ISet, set2: ISet): ISet { return result; } -export function returnListDifference(list: T[] = [], list2: T[] = []): T[] { +export function returnDifference(list: T[] = [], list2: T[] = []): T[] { const result = new _Set(list); list2.forEach(item => { result.delete(item); From d64f5507ecd0285c981c49bc41517e55806c56eb Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Mon, 4 Dec 2023 12:59:09 -0300 Subject: [PATCH 42/45] Add function call argument --- src/storages/inRedis/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storages/inRedis/index.ts b/src/storages/inRedis/index.ts index 0b6f923f..c5b4d3ad 100644 --- a/src/storages/inRedis/index.ts +++ b/src/storages/inRedis/index.ts @@ -45,7 +45,7 @@ export function InRedisStorage(options: InRedisStorageOptions = {}): IStorageAsy }); return { - splits: new SplitsCacheInRedis(log, keys, redisClient), + splits: new SplitsCacheInRedis(log, keys, redisClient, settings.sync.__splitFiltersValidation), segments: new SegmentsCacheInRedis(log, keys, redisClient), impressions: new ImpressionsCacheInRedis(log, keys.buildImpressionsKey(), redisClient, metadata), impressionCounts: impressionCountsCache, From 75f6df54e3f6cb03e3212a218e4accc50bc5ef3b Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Mon, 4 Dec 2023 14:21:06 -0300 Subject: [PATCH 43/45] Fixed some warning logs for flag sets, to properly print the method name when evaluating by flag sets --- src/logger/constants.ts | 4 +- src/logger/messages/warn.ts | 4 +- .../__tests__/splitFilters.spec.ts | 83 ++++++++++--------- src/utils/settingsValidation/splitFilters.ts | 13 +-- 4 files changed, 53 insertions(+), 51 deletions(-) diff --git a/src/logger/constants.ts b/src/logger/constants.ts index 91e601fc..e5b9fb3b 100644 --- a/src/logger/constants.ts +++ b/src/logger/constants.ts @@ -97,8 +97,8 @@ export const WARN_SPLITS_FILTER_EMPTY = 221; export const WARN_SDK_KEY = 222; export const STREAMING_PARSING_MY_SEGMENTS_UPDATE_V2 = 223; export const STREAMING_PARSING_SPLIT_UPDATE = 224; -export const WARN_SPLITS_FILTER_INVALID_SET = 225; -export const WARN_SPLITS_FILTER_LOWERCASE_SET = 226; +export const WARN_INVALID_FLAGSET = 225; +export const WARN_LOWERCASE_FLAGSET = 226; export const WARN_FLAGSET_NOT_CONFIGURED = 227; export const WARN_FLAGSET_WITHOUT_FLAGS = 228; diff --git a/src/logger/messages/warn.ts b/src/logger/messages/warn.ts index 3feca12f..1e9fa22b 100644 --- a/src/logger/messages/warn.ts +++ b/src/logger/messages/warn.ts @@ -34,7 +34,7 @@ export const codesWarn: [number, string][] = codesError.concat([ [c.STREAMING_PARSING_MY_SEGMENTS_UPDATE_V2, c.LOG_PREFIX_SYNC_STREAMING + 'Fetching MySegments due to an error processing %s notification: %s'], [c.STREAMING_PARSING_SPLIT_UPDATE, c.LOG_PREFIX_SYNC_STREAMING + 'Fetching SplitChanges due to an error processing SPLIT_UPDATE notification: %s'], - [c.WARN_SPLITS_FILTER_INVALID_SET, c.LOG_PREFIX_SETTINGS + ': you passed %s, flag set must adhere to the regular expressions %s. This means a flag set must start with a letter or number, be in lowercase, alphanumeric and have a max length of 50 characters. %s was discarded.'], - [c.WARN_SPLITS_FILTER_LOWERCASE_SET, c.LOG_PREFIX_SETTINGS + ': flag set %s should be all lowercase - converting string to lowercase.'], + [c.WARN_INVALID_FLAGSET, '%s: you passed %s, flag set must adhere to the regular expressions %s. This means a flag set must start with a letter or number, be in lowercase, alphanumeric and have a max length of 50 characters. %s was discarded.'], + [c.WARN_LOWERCASE_FLAGSET, '%s: flag set %s should be all lowercase - converting string to lowercase.'], [c.WARN_FLAGSET_WITHOUT_FLAGS, '%s: you passed %s flag set that does not contain cached feature flag names. Please double check what flag sets are in use in the Split user interface.'], ]); diff --git a/src/utils/settingsValidation/__tests__/splitFilters.spec.ts b/src/utils/settingsValidation/__tests__/splitFilters.spec.ts index d8cf6df0..c1bfd1f2 100644 --- a/src/utils/settingsValidation/__tests__/splitFilters.spec.ts +++ b/src/utils/settingsValidation/__tests__/splitFilters.spec.ts @@ -6,7 +6,7 @@ import { splitFilters, queryStrings, groupedFilters } from '../../../__tests__/m // Test target import { validateFlagSets, validateSplitFilters } from '../splitFilters'; -import { SETTINGS_SPLITS_FILTER, ERROR_INVALID, ERROR_EMPTY_ARRAY, ERROR_SETS_FILTER_EXCLUSIVE, WARN_SPLITS_FILTER_INVALID, WARN_SPLITS_FILTER_EMPTY, WARN_TRIMMING, WARN_SPLITS_FILTER_INVALID_SET, WARN_SPLITS_FILTER_LOWERCASE_SET, WARN_FLAGSET_NOT_CONFIGURED, WARN_SPLITS_FILTER_IGNORED } from '../../../logger/constants'; +import { SETTINGS_SPLITS_FILTER, ERROR_INVALID, ERROR_EMPTY_ARRAY, ERROR_SETS_FILTER_EXCLUSIVE, WARN_SPLITS_FILTER_INVALID, WARN_SPLITS_FILTER_EMPTY, WARN_TRIMMING, WARN_INVALID_FLAGSET, WARN_LOWERCASE_FLAGSET, WARN_FLAGSET_NOT_CONFIGURED, WARN_SPLITS_FILTER_IGNORED, LOG_PREFIX_SETTINGS } from '../../../logger/constants'; describe('validateSplitFilters', () => { @@ -113,18 +113,18 @@ describe('validateSplitFilters', () => { expect(loggerMock.error.mock.calls[0]).toEqual([ERROR_SETS_FILTER_EXCLUSIVE]); expect(validateSplitFilters(loggerMock, splitFilters[7], LOCALHOST_MODE)).toEqual(getOutput(7)); // lowercase and regexp - expect(loggerMock.warn.mock.calls[3]).toEqual([WARN_SPLITS_FILTER_LOWERCASE_SET, ['seT_c']]); // lowercase - expect(loggerMock.warn.mock.calls[4]).toEqual([WARN_SPLITS_FILTER_LOWERCASE_SET, ['set_B']]); // lowercase - expect(loggerMock.warn.mock.calls[5]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set_ 1', regexp, 'set_ 1']]); // empty spaces - expect(loggerMock.warn.mock.calls[6]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set _3', regexp, 'set _3']]); // empty spaces - expect(loggerMock.warn.mock.calls[7]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['_set_2', regexp, '_set_2']]); // start with a letter - expect(loggerMock.warn.mock.calls[8]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set_1234567890_1234567890_234567890_1234567890_1234567890', regexp, 'set_1234567890_1234567890_234567890_1234567890_1234567890']]); // max of 50 characters + expect(loggerMock.warn.mock.calls[3]).toEqual([WARN_LOWERCASE_FLAGSET, [LOG_PREFIX_SETTINGS, 'seT_c']]); // lowercase + expect(loggerMock.warn.mock.calls[4]).toEqual([WARN_LOWERCASE_FLAGSET, [LOG_PREFIX_SETTINGS, 'set_B']]); // lowercase + expect(loggerMock.warn.mock.calls[5]).toEqual([WARN_INVALID_FLAGSET, [LOG_PREFIX_SETTINGS, 'set_ 1', regexp, 'set_ 1']]); // empty spaces + expect(loggerMock.warn.mock.calls[6]).toEqual([WARN_INVALID_FLAGSET, [LOG_PREFIX_SETTINGS, 'set _3', regexp, 'set _3']]); // empty spaces + expect(loggerMock.warn.mock.calls[7]).toEqual([WARN_INVALID_FLAGSET, [LOG_PREFIX_SETTINGS, '_set_2', regexp, '_set_2']]); // start with a letter + expect(loggerMock.warn.mock.calls[8]).toEqual([WARN_INVALID_FLAGSET, [LOG_PREFIX_SETTINGS, 'set_1234567890_1234567890_234567890_1234567890_1234567890', regexp, 'set_1234567890_1234567890_234567890_1234567890_1234567890']]); // max of 50 characters expect(loggerMock.error.mock.calls[1]).toEqual([ERROR_SETS_FILTER_EXCLUSIVE]); expect(validateSplitFilters(loggerMock, splitFilters[8], PRODUCER_MODE)).toEqual(getOutput(8)); // lowercase and dedupe - expect(loggerMock.warn.mock.calls[9]).toEqual([WARN_SPLITS_FILTER_LOWERCASE_SET, ['SET_2']]); // lowercase - expect(loggerMock.warn.mock.calls[10]).toEqual([WARN_SPLITS_FILTER_LOWERCASE_SET, ['set_B']]); // lowercase - expect(loggerMock.warn.mock.calls[11]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set_3!', regexp, 'set_3!']]); // special character + expect(loggerMock.warn.mock.calls[9]).toEqual([WARN_LOWERCASE_FLAGSET, [LOG_PREFIX_SETTINGS, 'SET_2']]); // lowercase + expect(loggerMock.warn.mock.calls[10]).toEqual([WARN_LOWERCASE_FLAGSET, [LOG_PREFIX_SETTINGS, 'set_B']]); // lowercase + expect(loggerMock.warn.mock.calls[11]).toEqual([WARN_INVALID_FLAGSET, [LOG_PREFIX_SETTINGS, 'set_3!', regexp, 'set_3!']]); // special character expect(loggerMock.error.mock.calls[2]).toEqual([ERROR_SETS_FILTER_EXCLUSIVE]); expect(loggerMock.warn.mock.calls.length).toEqual(12); @@ -133,69 +133,70 @@ describe('validateSplitFilters', () => { test('validateFlagSets - Flag set validation for evaluations', () => { - let flagSetsFilter = ['set_1', 'set_2']; + const flagSetsFilter = ['set_1', 'set_2']; + const METHOD_NAME = 'test_method'; // empty array - expect(validateFlagSets(loggerMock, 'test_method', [], flagSetsFilter)).toEqual([]); + expect(validateFlagSets(loggerMock, METHOD_NAME, [], flagSetsFilter)).toEqual([]); // must start with a letter or number - expect(validateFlagSets(loggerMock, 'test_method', ['_set_1'], flagSetsFilter)).toEqual([]); - expect(loggerMock.warn.mock.calls[0]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['_set_1', regexp, '_set_1']]); + expect(validateFlagSets(loggerMock, METHOD_NAME, ['_set_1'], flagSetsFilter)).toEqual([]); + expect(loggerMock.warn.mock.calls[0]).toEqual([WARN_INVALID_FLAGSET, [METHOD_NAME, '_set_1', regexp, '_set_1']]); // can contain _a-z0-9 - expect(validateFlagSets(loggerMock, 'test_method', ['set*1'], flagSetsFilter)).toEqual([]); - expect(loggerMock.warn.mock.calls[1]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set*1', regexp, 'set*1']]); + expect(validateFlagSets(loggerMock, METHOD_NAME, ['set*1'], flagSetsFilter)).toEqual([]); + expect(loggerMock.warn.mock.calls[1]).toEqual([WARN_INVALID_FLAGSET, [METHOD_NAME, 'set*1', regexp, 'set*1']]); // have a max length of 50 characters const longName = '1234567890_1234567890_1234567890_1234567890_1234567890'; - expect(validateFlagSets(loggerMock, 'test_method', [longName], flagSetsFilter)).toEqual([]); - expect(loggerMock.warn.mock.calls[2]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, [longName, regexp, longName]]); + expect(validateFlagSets(loggerMock, METHOD_NAME, [longName], flagSetsFilter)).toEqual([]); + expect(loggerMock.warn.mock.calls[2]).toEqual([WARN_INVALID_FLAGSET, [METHOD_NAME, longName, regexp, longName]]); // both set names invalid -> empty list & warn - expect(validateFlagSets(loggerMock, 'test_method', ['set*1', 'set*3'], flagSetsFilter)).toEqual([]); - expect(loggerMock.warn.mock.calls[3]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set*1', regexp, 'set*1']]); - expect(loggerMock.warn.mock.calls[4]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set*3', regexp, 'set*3']]); + expect(validateFlagSets(loggerMock, METHOD_NAME, ['set*1', 'set*3'], flagSetsFilter)).toEqual([]); + expect(loggerMock.warn.mock.calls[3]).toEqual([WARN_INVALID_FLAGSET, [METHOD_NAME, 'set*1', regexp, 'set*1']]); + expect(loggerMock.warn.mock.calls[4]).toEqual([WARN_INVALID_FLAGSET, [METHOD_NAME, 'set*3', regexp, 'set*3']]); // only set_1 is valid => [set_1] & warn - expect(validateFlagSets(loggerMock, 'test_method', ['set_1', 'set*3'], flagSetsFilter)).toEqual(['set_1']); - expect(loggerMock.warn.mock.calls[5]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set*3', regexp, 'set*3']]); + expect(validateFlagSets(loggerMock, METHOD_NAME, ['set_1', 'set*3'], flagSetsFilter)).toEqual(['set_1']); + expect(loggerMock.warn.mock.calls[5]).toEqual([WARN_INVALID_FLAGSET, [METHOD_NAME, 'set*3', regexp, 'set*3']]); // set_3 not included in configuration but set_1 included => [set_1] & warn - expect(validateFlagSets(loggerMock, 'test_method', ['set_1', 'set_3'], flagSetsFilter)).toEqual(['set_1']); - expect(loggerMock.warn.mock.calls[6]).toEqual([WARN_FLAGSET_NOT_CONFIGURED, ['test_method', 'set_3']]); + expect(validateFlagSets(loggerMock, METHOD_NAME, ['set_1', 'set_3'], flagSetsFilter)).toEqual(['set_1']); + expect(loggerMock.warn.mock.calls[6]).toEqual([WARN_FLAGSET_NOT_CONFIGURED, [METHOD_NAME, 'set_3']]); // set_3 not included in configuration => [] & warn - expect(validateFlagSets(loggerMock, 'test_method', ['set_3'], flagSetsFilter)).toEqual([]); - expect(loggerMock.warn.mock.calls[7]).toEqual([WARN_FLAGSET_NOT_CONFIGURED, ['test_method', 'set_3']]); + expect(validateFlagSets(loggerMock, METHOD_NAME, ['set_3'], flagSetsFilter)).toEqual([]); + expect(loggerMock.warn.mock.calls[7]).toEqual([WARN_FLAGSET_NOT_CONFIGURED, [METHOD_NAME, 'set_3']]); // empty config // must start with a letter or number - expect(validateFlagSets(loggerMock, 'test_method', ['_set_1'], [])).toEqual([]); - expect(loggerMock.warn.mock.calls[8]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['_set_1', regexp, '_set_1']]); + expect(validateFlagSets(loggerMock, METHOD_NAME, ['_set_1'], [])).toEqual([]); + expect(loggerMock.warn.mock.calls[8]).toEqual([WARN_INVALID_FLAGSET, [METHOD_NAME, '_set_1', regexp, '_set_1']]); // can contain _a-z0-9 - expect(validateFlagSets(loggerMock, 'test_method', ['set*1'], [])).toEqual([]); - expect(loggerMock.warn.mock.calls[9]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set*1', regexp, 'set*1']]); + expect(validateFlagSets(loggerMock, METHOD_NAME, ['set*1'], [])).toEqual([]); + expect(loggerMock.warn.mock.calls[9]).toEqual([WARN_INVALID_FLAGSET, [METHOD_NAME, 'set*1', regexp, 'set*1']]); // have a max length of 50 characters - expect(validateFlagSets(loggerMock, 'test_method', [longName], [])).toEqual([]); - expect(loggerMock.warn.mock.calls[10]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, [longName, regexp, longName]]); + expect(validateFlagSets(loggerMock, METHOD_NAME, [longName], [])).toEqual([]); + expect(loggerMock.warn.mock.calls[10]).toEqual([WARN_INVALID_FLAGSET, [METHOD_NAME, longName, regexp, longName]]); // both set names invalid -> empty list & warn - expect(validateFlagSets(loggerMock, 'test_method', ['set*1', 'set*3'], [])).toEqual([]); - expect(loggerMock.warn.mock.calls[11]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set*1', regexp, 'set*1']]); - expect(loggerMock.warn.mock.calls[12]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set*3', regexp, 'set*3']]); + expect(validateFlagSets(loggerMock, METHOD_NAME, ['set*1', 'set*3'], [])).toEqual([]); + expect(loggerMock.warn.mock.calls[11]).toEqual([WARN_INVALID_FLAGSET, [METHOD_NAME, 'set*1', regexp, 'set*1']]); + expect(loggerMock.warn.mock.calls[12]).toEqual([WARN_INVALID_FLAGSET, [METHOD_NAME, 'set*3', regexp, 'set*3']]); // only set_1 is valid => [set_1] & warn - expect(validateFlagSets(loggerMock, 'test_method', ['set_1', 'set*3'], [])).toEqual(['set_1']); - expect(loggerMock.warn.mock.calls[13]).toEqual([WARN_SPLITS_FILTER_INVALID_SET, ['set*3', regexp, 'set*3']]); + expect(validateFlagSets(loggerMock, METHOD_NAME, ['set_1', 'set*3'], [])).toEqual(['set_1']); + expect(loggerMock.warn.mock.calls[13]).toEqual([WARN_INVALID_FLAGSET, [METHOD_NAME, 'set*3', regexp, 'set*3']]); // any set should be returned if there isn't flag sets in filter - expect(validateFlagSets(loggerMock, 'test_method', ['set_1'], [])).toEqual(['set_1']); - expect(validateFlagSets(loggerMock, 'test_method', ['set_1', 'set_2'], [])).toEqual(['set_1', 'set_2']); - expect(validateFlagSets(loggerMock, 'test_method', ['set_3'], [])).toEqual(['set_3']); + expect(validateFlagSets(loggerMock, METHOD_NAME, ['set_1'], [])).toEqual(['set_1']); + expect(validateFlagSets(loggerMock, METHOD_NAME, ['set_1', 'set_2'], [])).toEqual(['set_1', 'set_2']); + expect(validateFlagSets(loggerMock, METHOD_NAME, ['set_3'], [])).toEqual(['set_3']); }); diff --git a/src/utils/settingsValidation/splitFilters.ts b/src/utils/settingsValidation/splitFilters.ts index 74548e36..ebe76610 100644 --- a/src/utils/settingsValidation/splitFilters.ts +++ b/src/utils/settingsValidation/splitFilters.ts @@ -3,7 +3,7 @@ import { validateSplits } from '../inputValidation/splits'; import { ISplitFiltersValidation } from '../../dtos/types'; import { SplitIO } from '../../types'; import { ILogger } from '../../logger/types'; -import { WARN_SPLITS_FILTER_IGNORED, WARN_SPLITS_FILTER_EMPTY, WARN_SPLITS_FILTER_INVALID, SETTINGS_SPLITS_FILTER, LOG_PREFIX_SETTINGS, ERROR_SETS_FILTER_EXCLUSIVE, WARN_SPLITS_FILTER_LOWERCASE_SET, WARN_SPLITS_FILTER_INVALID_SET, WARN_FLAGSET_NOT_CONFIGURED } from '../../logger/constants'; +import { WARN_SPLITS_FILTER_IGNORED, WARN_SPLITS_FILTER_EMPTY, WARN_SPLITS_FILTER_INVALID, SETTINGS_SPLITS_FILTER, LOG_PREFIX_SETTINGS, ERROR_SETS_FILTER_EXCLUSIVE, WARN_LOWERCASE_FLAGSET, WARN_INVALID_FLAGSET, WARN_FLAGSET_NOT_CONFIGURED } from '../../logger/constants'; import { objectAssign } from '../lang/objectAssign'; import { find, uniq } from '../lang'; @@ -54,7 +54,7 @@ function validateSplitFilter(log: ILogger, type: SplitIO.SplitFilterType, values if (result) { if (type === 'bySet') { - result = sanitizeFlagSets(log, result); + result = sanitizeFlagSets(log, result, LOG_PREFIX_SETTINGS); } // check max length @@ -98,20 +98,21 @@ function queryStringBuilder(groupedFilters: Record { if (CAPITAL_LETTERS_REGEX.test(flagSet)) { - log.warn(WARN_SPLITS_FILTER_LOWERCASE_SET, [flagSet]); + log.warn(WARN_LOWERCASE_FLAGSET, [method, flagSet]); flagSet = flagSet.toLowerCase(); } return flagSet; }) .filter(flagSet => { if (!VALID_FLAGSET_REGEX.test(flagSet)) { - log.warn(WARN_SPLITS_FILTER_INVALID_SET, [flagSet, VALID_FLAGSET_REGEX, flagSet]); + log.warn(WARN_INVALID_FLAGSET, [method, flagSet, VALID_FLAGSET_REGEX, flagSet]); return false; } if (typeof flagSet !== 'string') return false; @@ -190,7 +191,7 @@ export function validateSplitFilters(log: ILogger, maybeSplitFilters: any, mode: export function validateFlagSets(log: ILogger, method: string, flagSets: string[], flagSetsInConfig: string[]): string[] { const sets = validateSplits(log, flagSets, method, 'flag sets', 'flag set'); - let toReturn = sets ? sanitizeFlagSets(log, sets) : []; + let toReturn = sets ? sanitizeFlagSets(log, sets, method) : []; if (flagSetsInConfig.length > 0) { toReturn = toReturn.filter(flagSet => { if (flagSetsInConfig.indexOf(flagSet) > -1) { From 067621b8d59f70a14cd5dcdfe7ddd637c8f7e532 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Mon, 4 Dec 2023 14:28:32 -0300 Subject: [PATCH 44/45] Prepare rc --- package-lock.json | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index 048f996a..a9386764 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@splitsoftware/splitio-commons", - "version": "1.12.0", + "version": "1.12.0-rc.0", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "@splitsoftware/splitio-commons", - "version": "1.12.0", + "version": "1.12.0-rc.0", "license": "Apache-2.0", "dependencies": { "tslib": "^2.3.1" diff --git a/package.json b/package.json index 7241aa86..e1955989 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@splitsoftware/splitio-commons", - "version": "1.12.0", + "version": "1.12.0-rc.0", "description": "Split Javascript SDK common components", "main": "cjs/index.js", "module": "esm/index.js", From 5925973c240cadabbfc6473e782badbeb57873f8 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Mon, 4 Dec 2023 14:32:54 -0300 Subject: [PATCH 45/45] Stable version --- package-lock.json | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index a9386764..048f996a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@splitsoftware/splitio-commons", - "version": "1.12.0-rc.0", + "version": "1.12.0", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "@splitsoftware/splitio-commons", - "version": "1.12.0-rc.0", + "version": "1.12.0", "license": "Apache-2.0", "dependencies": { "tslib": "^2.3.1" diff --git a/package.json b/package.json index e1955989..7241aa86 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@splitsoftware/splitio-commons", - "version": "1.12.0-rc.0", + "version": "1.12.0", "description": "Split Javascript SDK common components", "main": "cjs/index.js", "module": "esm/index.js",