Merge pull request #273 from splitio/flagsets_no_feature_flags_warning

[Flag Sets] Log warning if evaluating with flag sets that don't contain feature flags

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):

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",
@@ -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",

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']],
+    ]);
+  });
 });

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<Record<string, IEvaluationResult>> {
-  let storedFlagNames: MaybeThenable<ISet<string>>;
+  let storedFlagNames: MaybeThenable<ISet<string>[]>;
+
+  function evaluate(
+    featureFlagsByFlagSets: ISet<string>[],
+  ) {
+    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(

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;

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.'],
 ]);

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);

src/storages/AbstractSplitsCacheAsync.ts

@@ -18,7 +18,7 @@ export abstract class AbstractSplitsCacheAsync implements ISplitsCacheAsync {
   abstract getChangeNumber(): Promise<number>
   abstract getAll(): Promise<ISplit[]>
   abstract getSplitNames(): Promise<string[]>
-  abstract getNamesByFlagSets(flagSets: string[]): Promise<ISet<string>>
+  abstract getNamesByFlagSets(flagSets: string[]): Promise<ISet<string>[]>
   abstract trafficTypeExists(trafficType: string): Promise<boolean>
   abstract clear(): Promise<boolean | void>
 

src/storages/AbstractSplitsCacheSync.ts

@@ -79,7 +79,7 @@ export abstract class AbstractSplitsCacheSync implements ISplitsCacheSync {
     return false;
   }
 
-  abstract getNamesByFlagSets(flagSets: string[]): ISet<string>
+  abstract getNamesByFlagSets(flagSets: string[]): ISet<string>[]
 
 }
 

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<string>{
-    let toReturn: ISet<string> = new _Set([]);
-    flagSets.forEach(flagSet => {
+  getNamesByFlagSets(flagSets: string[]): ISet<string>[] {
+    return flagSets.map(flagSet => {
       const flagSetKey = this.keys.buildFlagSetKey(flagSet);
-      let flagSetFromLocalStorage = localStorage.getItem(flagSetKey);
+      const 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) {
@@ -281,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)));
@@ -302,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;