Merge pull request #97 from splitio/input_validation_for_getTreatments

[Flag sets] Input validation for getTreatments action creator

CHANGES.txt

@@ -1,3 +1,12 @@
+1.10.0 (December 18, 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 a new optional `flagSets` property to the param object of the `getTreatments` action creator, to support evaluating flags in given flag set/s. Either `splitNames` or `flagSets` must be provided to the function. If both are provided, `splitNames` will be used.
+      - Added a new optional Split Filter configuration option. This allows the SDK and Split services to only synchronize the flags in the specified flag sets, avoiding unused or unwanted flags from being synced on the SDK instance, bringing all the benefits from a reduced payload.
+      - Added `sets` property to the `SplitView` object returned by the `getSplit` and `getSplits` helper functions to expose flag sets on flag views.
+ - Added `defaultTreatment` property to the `SplitView` object returned by the `getSplit` and `getSplits` helper functions (Related to issue https://github.com/splitio/javascript-commons/issues/225).
+ - Updated `getTreatments` action creator to validate the provided params object, in order to log a descriptive error when an invalid object is provided rather than throwing a cryptic error.
+ - Updated @splitsoftware/splitio package to version 10.24.1 that includes flag sets support, vulnerability fixes and other improvements.
+
 1.9.0 (July 18, 2023)
  - Updated some transitive dependencies for vulnerability fixes.
  - Updated @splitsoftware/splitio package to version 10.23.0 that includes:
@@ -68,7 +77,7 @@
  - Updated Split's SDK dependency to fix vulnerabilities.
 
 1.3.0 (December 9, 2020)
- - Added a new parameter to `getTreatments` actions creator: `evalOnReadyFromCache` to evaluate feature flags when the SDK_READY_FROM_CACHE event is emitted. Learn more in our Redux SDK documentation.
+ - Added a new parameter to `getTreatments` action creator: `evalOnReadyFromCache` to evaluate feature flags when the SDK_READY_FROM_CACHE event is emitted. Learn more in our Redux SDK documentation.
  - Updated how feature flag evaluations are handled on SDK_READY, SDK_READY_FROM_CACHE and SDK_UPDATE events, to dispatch a single action with evaluations that results in all treatments updates in the state at once, instead of having multiple actions that might lead to multiple store notifications.
  - Updated some NPM dependencies for vulnerability fixes.
 

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@splitsoftware/splitio-redux",
-  "version": "1.9.0",
+  "version": "1.10.0",
   "description": "A library to easily use Split JS SDK with Redux and React Redux",
   "main": "lib/index.js",
   "module": "es/index.js",
@@ -59,7 +59,7 @@
   },
   "homepage": "https://github.com/splitio/redux-client#readme",
   "dependencies": {
-    "@splitsoftware/splitio": "10.23.0"
+    "@splitsoftware/splitio": "10.24.1"
   },
   "devDependencies": {
     "@testing-library/jest-dom": "^5.16.5",

src/__tests__/actions.browser.test.ts

@@ -13,7 +13,7 @@ import { sdkBrowserConfig } from './utils/sdkConfigs';
 import {
   SPLIT_READY, SPLIT_READY_WITH_EVALUATIONS, SPLIT_READY_FROM_CACHE, SPLIT_READY_FROM_CACHE_WITH_EVALUATIONS,
   SPLIT_UPDATE, SPLIT_UPDATE_WITH_EVALUATIONS, SPLIT_TIMEDOUT, SPLIT_DESTROY, ADD_TREATMENTS,
-  ERROR_GETT_NO_INITSPLITSDK, ERROR_DESTROY_NO_INITSPLITSDK, getControlTreatmentsWithConfig,
+  ERROR_GETT_NO_INITSPLITSDK, ERROR_DESTROY_NO_INITSPLITSDK, getControlTreatmentsWithConfig, ERROR_GETT_NO_PARAM_OBJECT,
 } from '../constants';
 
 /** Test targets */
@@ -160,6 +160,24 @@ describe('getTreatments', () => {
     expect(store.getActions().length).toBe(0);
   });
 
+  it('logs error and dispatches a no-op async action if the provided param is invalid', async () => {
+    // Init SDK and set ready
+    const store = mockStore(STATE_INITIAL);
+    const actionResult = store.dispatch<any>(initSplitSdk({ config: sdkBrowserConfig }));
+    (splitSdk.factory as any).client().__emitter__.emit(Event.SDK_READY);
+    await actionResult;
+
+    const consoleLogSpy = jest.spyOn(console, 'log');
+    store.clearActions();
+
+    // @ts-expect-error testing invalid input
+    store.dispatch<any>(getTreatments());
+
+    expect(store.getActions().length).toBe(0);
+    expect(consoleLogSpy).toBeCalledWith(ERROR_GETT_NO_PARAM_OBJECT);
+    consoleLogSpy.mockRestore();
+  });
+
   it('dispatches an ADD_TREATMENTS action if Split SDK is ready', (done) => {
 
     // Init SDK and set ready
@@ -169,13 +187,20 @@ describe('getTreatments', () => {
 
     actionResult.then(() => {
       store.dispatch<any>(getTreatments({ splitNames: 'split1' }));
+      store.dispatch<any>(getTreatments({ flagSets: 'set1' }));
 
-      const action = store.getActions()[1];
-      expect(action.type).toBe(ADD_TREATMENTS);
-      expect(action.payload.key).toBe(sdkBrowserConfig.core.key);
+      const actions = [store.getActions()[1], store.getActions()[2]];
+      actions.forEach(action => {
+        expect(action.type).toBe(ADD_TREATMENTS);
+        expect(action.payload.key).toBe(sdkBrowserConfig.core.key);
+      });
 
       // getting the evaluation result and validating it matches the results from SDK
-      expect(splitSdk.factory.client().getTreatmentsWithConfig).toHaveLastReturnedWith(action.payload.treatments);
+      expect(splitSdk.factory.client().getTreatmentsWithConfig).toHaveBeenLastCalledWith(['split1'], undefined);
+      expect(splitSdk.factory.client().getTreatmentsWithConfig).toHaveLastReturnedWith(actions[0].payload.treatments);
+      expect(splitSdk.factory.client().getTreatmentsWithConfigByFlagSets).toHaveBeenLastCalledWith(['set1'], undefined);
+      expect(splitSdk.factory.client().getTreatmentsWithConfigByFlagSets).toHaveLastReturnedWith(actions[1].payload.treatments);
+
       expect(getClient(splitSdk).evalOnUpdate).toEqual({});
       expect(getClient(splitSdk).evalOnReady.length).toEqual(0);
 
@@ -238,10 +263,16 @@ describe('getTreatments', () => {
 
         // The first ADD_TREATMENTS actions is dispatched again, but this time is registered for 'evalOnUpdate'
         store.dispatch<any>(getTreatments({ splitNames: 'split1', evalOnUpdate: true }));
+        // Dispatch another ADD_TREATMENTS action with flag sets
+        store.dispatch<any>(getTreatments({ flagSets: 'set1', evalOnUpdate: true }));
 
         // Validate action and registered callback
         expect(splitSdk.factory.client().getTreatmentsWithConfig).toBeCalledTimes(5);
-        expect(Object.values(getClient(splitSdk).evalOnUpdate).length).toBe(1);
+        expect(splitSdk.factory.client().getTreatmentsWithConfigByFlagSets).toBeCalledTimes(1);
+        expect(getClient(splitSdk).evalOnUpdate).toEqual({
+          'flag::split1': { evalOnUpdate: true, flagSets: undefined, splitNames: ['split1'] },
+          'set::set1': { evalOnUpdate: true, flagSets: ['set1'], splitNames: undefined }
+        });
 
         done();
       });
@@ -253,40 +284,45 @@ describe('getTreatments', () => {
     const store = mockStore(STATE_INITIAL);
     const actionResult = store.dispatch<any>(initSplitSdk({ config: sdkBrowserConfig, onReadyFromCache: onReadyFromCacheCb }));
     store.dispatch<any>(getTreatments({ splitNames: 'split2' })); // `evalOnUpdate` and `evalOnReadyFromCache` params are false by default
+    store.dispatch<any>(getTreatments({ flagSets: 'set2' }));
 
-    // If SDK is not operational, an ADD_TREATMENTS action is dispatched with control treatments
-    // without calling SDK client, but the item is added to 'evalOnReady' list.
-    expect(store.getActions().length).toBe(1);
-    expect(getClient(splitSdk).evalOnReady.length).toEqual(1);
-    expect(getClient(splitSdk).evalOnUpdate).toEqual({});
-    let action = store.getActions()[0];
-    expect(action.type).toBe(ADD_TREATMENTS);
-    expect(action.payload.key).toBe(sdkBrowserConfig.core.key);
-    expect(action.payload.treatments).toEqual(getControlTreatmentsWithConfig(['split2']));
+    // If SDK is not operational, ADD_TREATMENTS actions are dispatched, with control treatments for provided feature flag names, and no treatments for provided flag sets.
+
+    expect(store.getActions()).toEqual([
+      { type: ADD_TREATMENTS, payload: { key: sdkBrowserConfig.core.key, treatments: getControlTreatmentsWithConfig(['split2']) } },
+      { type: ADD_TREATMENTS, payload: { key: sdkBrowserConfig.core.key, treatments: {} } },
+    ]);
+    // SDK client is not called, but items are added to 'evalOnReady' list.
     expect(splitSdk.factory.client().getTreatmentsWithConfig).toBeCalledTimes(0);
+    expect(splitSdk.factory.client().getTreatmentsWithConfigByFlagSets).toBeCalledTimes(0);
+    expect(getClient(splitSdk).evalOnReady.length).toEqual(2);
+    expect(getClient(splitSdk).evalOnUpdate).toEqual({});
 
     // When the SDK is ready from cache, the SPLIT_READY_FROM_CACHE_WITH_EVALUATIONS action is not dispatched if the `getTreatments` action was dispatched with `evalOnReadyFromCache` false
     (splitSdk.factory as any).client().__emitter__.emit(Event.SDK_READY_FROM_CACHE);
     function onReadyFromCacheCb() {
-      expect(store.getActions().length).toBe(2);
-      action = store.getActions()[1];
+      expect(store.getActions().length).toBe(3);
+      const action = store.getActions()[2];
       expect(action.type).toBe(SPLIT_READY_FROM_CACHE);
     }
 
     (splitSdk.factory as any).client().__emitter__.emit(Event.SDK_READY);
 
     actionResult.then(() => {
       // The SPLIT_READY_WITH_EVALUATIONS action is dispatched if the SDK is ready and there are pending evaluations.
-      action = store.getActions()[2];
+      const action = store.getActions()[3];
       expect(action.type).toBe(SPLIT_READY_WITH_EVALUATIONS);
       expect(action.payload.key).toBe(sdkBrowserConfig.core.key);
 
       // getting the evaluation result and validating it matches the results from SDK
       const treatments = action.payload.treatments;
-      expect(splitSdk.factory.client().getTreatmentsWithConfig).lastCalledWith(['split2'], undefined);
-      expect(splitSdk.factory.client().getTreatmentsWithConfig).toHaveLastReturnedWith(treatments);
+      expect(splitSdk.factory.client().getTreatmentsWithConfig).toBeCalledWith(['split2'], undefined);
+      expect(splitSdk.factory.client().getTreatmentsWithConfigByFlagSets).toBeCalledWith(['set2'], undefined);
+      expect(treatments).toEqual({
+        ...(splitSdk.factory.client().getTreatmentsWithConfig as jest.Mock).mock.results[0].value,
+        ...(splitSdk.factory.client().getTreatmentsWithConfigByFlagSets as jest.Mock).mock.results[0].value,
+      })
 
-      expect(splitSdk.factory.client().getTreatmentsWithConfig).toBeCalledTimes(1); // control assertion - getTreatmentsWithConfig calls
       expect(getClient(splitSdk).evalOnUpdate).toEqual({}); // control assertion - cbs scheduled for update
 
       // The ADD_TREATMENTS actions is dispatched again, but this time is registered for 'evalOnUpdate'

src/__tests__/actions.node.test.ts

@@ -167,22 +167,27 @@ describe('getTreatments', () => {
 
         // Invoke with a feature flag name string and no attributes
         store.dispatch<any>(getTreatments({ key: splitKey, splitNames: 'split1' }));
+        store.dispatch<any>(getTreatments({ key: splitKey, flagSets: ['set1'] }));
 
-        let action = store.getActions()[1]; // action 0 is SPLIT_READY
-        expect(action.type).toBe(ADD_TREATMENTS);
-        expect(action.payload.key).toBe(splitKey);
+        const actions = [store.getActions()[1], store.getActions()[2]]; // action 0 is SPLIT_READY
+        actions.forEach(action => {
+          expect(action.type).toBe(ADD_TREATMENTS);
+          expect(action.payload.key).toBe(splitKey);
+        });
         expect(splitSdk.factory.client().getTreatmentsWithConfig).toHaveBeenLastCalledWith(splitKey, ['split1'], undefined);
-        expect(splitSdk.factory.client().getTreatmentsWithConfig).toHaveLastReturnedWith(action.payload.treatments);
+        expect(splitSdk.factory.client().getTreatmentsWithConfig).toHaveLastReturnedWith(actions[0].payload.treatments);
+        expect(splitSdk.factory.client().getTreatmentsWithConfigByFlagSets).toHaveBeenLastCalledWith(splitKey, ['set1'], undefined);
+        expect(splitSdk.factory.client().getTreatmentsWithConfigByFlagSets).toHaveLastReturnedWith(actions[1].payload.treatments);
 
         // Invoke with a list of feature flag names and a attributes object
         const featureFlagNames = ['split1', 'split2'];
         const attributes = { att1: 'att1' };
-        store.dispatch<any>(getTreatments({ key: splitKey, splitNames: featureFlagNames, attributes }));
+        store.dispatch<any>(getTreatments({ key: 'other_user', splitNames: featureFlagNames, attributes }));
 
-        action = store.getActions()[2];
+        const action = store.getActions()[3];
         expect(action.type).toBe(ADD_TREATMENTS);
-        expect(action.payload.key).toBe(splitKey);
-        expect(splitSdk.factory.client().getTreatmentsWithConfig).toHaveBeenLastCalledWith(splitKey, featureFlagNames, attributes);
+        expect(action.payload.key).toBe('other_user');
+        expect(splitSdk.factory.client().getTreatmentsWithConfig).toHaveBeenLastCalledWith('other_user', featureFlagNames, attributes);
         expect(splitSdk.factory.client().getTreatmentsWithConfig).toHaveLastReturnedWith(action.payload.treatments);
       }
 

src/__tests__/helpers.browser.test.ts

@@ -29,13 +29,17 @@ const featureFlagViews: SplitIO.SplitViews = [
     treatments: ['on', 'off'],
     changeNumber: 0,
     configs: { on: null, off: null },
+    sets: [],
+    defaultTreatment: 'off',
   }, {
     name: 'split_2',
     trafficType: 'user',
     killed: false,
     treatments: ['on', 'off'],
     changeNumber: 0,
     configs: { on: null, off: null },
+    sets: [],
+    defaultTreatment: 'off',
   },
 ];
 

src/__tests__/helpers.node.test.ts

@@ -29,13 +29,17 @@ const featureFlagViews: SplitIO.SplitViews = [
     treatments: ['on', 'off'],
     changeNumber: 0,
     configs: { on: null, off: null },
+    sets: [],
+    defaultTreatment: 'off',
   }, {
     name: 'split_2',
     trafficType: 'user',
     killed: false,
     treatments: ['on', 'off'],
     changeNumber: 0,
     configs: { on: null, off: null },
+    sets: [],
+    defaultTreatment: 'off',
   },
 ];
 

src/__tests__/index.test.ts

@@ -0,0 +1,46 @@
+import {
+  splitReducer as exportedSplitReducer,
+  initSplitSdk as exportedInitSplitSdk,
+  getTreatments as exportedGetTreatments,
+  destroySplitSdk as exportedDestroySplitSdk,
+  splitSdk as exportedSplitSdk,
+  track as exportedTrack,
+  getSplitNames as exportedGetSplitNames,
+  getSplit as exportedGetSplit,
+  getSplits as exportedGetSplits,
+  selectTreatmentValue as exportedSelectTreatmentValue,
+  selectTreatmentWithConfig as exportedSelectTreatmentWithConfig,
+  connectSplit as exportedConnectSplit,
+  connectToggler as exportedConnectToggler,
+  mapTreatmentToProps as exportedMapTreatmentToProps,
+  mapIsFeatureOnToProps as exportedMapIsFeatureOnToProps,
+  /* eslint-disable @typescript-eslint/no-unused-vars */ // Checks that types are exported. Otherwise, the test would fail with a TS error.
+  ISplitState,
+} from '../index';
+
+import { splitReducer } from '../reducer';
+import { initSplitSdk, getTreatments, destroySplitSdk, splitSdk } from '../asyncActions';
+import { track, getSplitNames, getSplit, getSplits } from '../helpers';
+import { selectTreatmentValue, selectTreatmentWithConfig } from '../selectors';
+import { connectSplit } from '../react-redux/connectSplit';
+import { connectToggler, mapTreatmentToProps, mapIsFeatureOnToProps } from '../react-redux/connectToggler';
+
+it('index should export modules', () => {
+
+  expect(exportedSplitReducer).toBe(splitReducer);
+  expect(exportedInitSplitSdk).toBe(initSplitSdk);
+  expect(exportedGetTreatments).toBe(getTreatments);
+  expect(exportedDestroySplitSdk).toBe(destroySplitSdk);
+  expect(exportedSplitSdk).toBe(splitSdk);
+  expect(exportedTrack).toBe(track);
+  expect(exportedGetSplitNames).toBe(getSplitNames);
+  expect(exportedGetSplit).toBe(getSplit);
+  expect(exportedGetSplits).toBe(getSplits);
+  expect(exportedSelectTreatmentValue).toBe(selectTreatmentValue);
+  expect(exportedSelectTreatmentWithConfig).toBe(selectTreatmentWithConfig);
+  expect(exportedConnectSplit).toBe(connectSplit);
+  expect(exportedConnectToggler).toBe(connectToggler);
+  expect(exportedMapTreatmentToProps).toBe(mapTreatmentToProps);
+  expect(exportedMapIsFeatureOnToProps).toBe(mapIsFeatureOnToProps);
+
+});

src/__tests__/utils.test.ts

@@ -1,5 +1,6 @@
 /** Test targets */
-import { matching } from '../utils';
+import { WARN_FEATUREFLAGS_AND_FLAGSETS } from '../constants';
+import { matching, validateGetTreatmentsParams } from '../utils';
 
 describe('matching', () => {
 
@@ -31,3 +32,43 @@ describe('matching', () => {
   });
 
 });
+
+describe('validateGetTreatmentsParams', () => {
+
+  it('should return a sanitized copy of the provided params object', () => {
+    // String values are converted to arrays
+    expect(validateGetTreatmentsParams({ splitNames: 'some split' })).toStrictEqual({ splitNames: ['some split'], flagSets: undefined });
+    expect(validateGetTreatmentsParams({ flagSets: 'flag set' })).toStrictEqual({ splitNames: undefined, flagSets: ['flag set'] });
+
+    // Feature flag names are sanitized because they are used by getControlTreatmentsWithConfig function while the SDK is not ready
+    expect(validateGetTreatmentsParams({ splitNames: ['some split', null] })).toStrictEqual({ splitNames: ['some split'], flagSets: undefined });
+    // Flag set names are not sanitized, because they are not used by Redux SDK directly
+    expect(validateGetTreatmentsParams({ flagSets: ['flag set', null] })).toStrictEqual({ splitNames: undefined, flagSets: ['flag set', null] });
+  });
+
+  it('should ignore flagSets if both splitNames and flagSets are provided', () => {
+    const consoleSpy = jest.spyOn(console, 'log');
+
+    expect(validateGetTreatmentsParams({ splitNames: ['some split'], flagSets: ['flag set', null] })).toStrictEqual({ splitNames: ['some split'], flagSets: undefined });
+
+    expect(consoleSpy.mock.calls).toEqual([[WARN_FEATUREFLAGS_AND_FLAGSETS]]);
+    consoleSpy.mockRestore();
+  });
+
+  it('should return false if splitNames and flagSets values are invalid', () => {
+    // Invalid values for splitNames and flagSets are converted to empty arrays
+    expect(validateGetTreatmentsParams({ splitNames: {} })).toStrictEqual(false);
+    expect(validateGetTreatmentsParams({ flagSets: {} })).toStrictEqual(false);
+    expect(validateGetTreatmentsParams({ splitNames: true })).toStrictEqual(false);
+    expect(validateGetTreatmentsParams({ flagSets: true })).toStrictEqual(false);
+    expect(validateGetTreatmentsParams({ splitNames: null, flagSets: null })).toStrictEqual(false);
+    expect(validateGetTreatmentsParams({})).toStrictEqual(false);
+  });
+
+  it('should return false if the provided param is not an object', () => { // @ts-expect-error testing invalid input
+    expect(validateGetTreatmentsParams()).toStrictEqual(false);
+    expect(validateGetTreatmentsParams([])).toStrictEqual(false);
+    expect(validateGetTreatmentsParams('invalid')).toStrictEqual(false);
+  });
+
+});

src/__tests__/utils/mockBrowserSplitSdk.ts

@@ -60,6 +60,12 @@ export function mockSdk() {
           return acc;
         }, {});
       });
+      const getTreatmentsWithConfigByFlagSets: jest.Mock = jest.fn((flagSets) => {
+        return flagSets.reduce((acc: SplitIO.TreatmentsWithConfig, flagSet: string) => {
+          acc[flagSet + '_feature_flag'] = { treatment: 'fakeTreatment', config: null };
+          return acc;
+        }, {});
+      });
       const setAttributes: jest.Mock = jest.fn(() => {
         return true;
       });
@@ -89,6 +95,7 @@ export function mockSdk() {
 
       return Object.assign(Object.create(__emitter__), {
         getTreatmentsWithConfig,
+        getTreatmentsWithConfigByFlagSets,
         track,
         ready,
         destroy,