diff --git a/CHANGES.txt b/CHANGES.txt index 89b98466..c827ea97 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) @@ -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/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 65b3d060..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", 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/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/__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/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/storages/inRedis/SplitsCacheInRedis.ts b/src/storages/inRedis/SplitsCacheInRedis.ts index 6aa1136b..77582d23 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)); }); } @@ -174,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); })); } @@ -190,14 +211,18 @@ 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 + * 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(): Promise[]> { - this.log.error(LOG_PREFIX + 'ByFlagSet/s evaluations are not supported with Redis storage yet.'); - return Promise.reject(); + getNamesByFlagSets(flagSets: string[]): Promise[]> { + 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; + + 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))); } /** @@ -214,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__/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 22d12190..638af5d1 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); @@ -54,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 () => { @@ -102,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 () => { @@ -140,7 +141,88 @@ 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 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'])]); + + // @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]); + + 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.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 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.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(); }); }); 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]); 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; } }