Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SDKS-7795] Validation of feature flag filters in consumer and partial consumer modes #271

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/logger/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
13 changes: 7 additions & 6 deletions src/logger/messages/warn.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.'],
]);
51 changes: 28 additions & 23 deletions src/utils/settingsValidation/__tests__/splitFilters.spec.ts
Original file line number Diff line number Diff line change
@@ -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', () => {

Expand All @@ -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();
Expand All @@ -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();

Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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

Expand All @@ -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']);

});
Expand Down
2 changes: 1 addition & 1 deletion src/utils/settingsValidation/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
19 changes: 13 additions & 6 deletions src/utils/settingsValidation/splitFilters.ts
Original file line number Diff line number Diff line change
@@ -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';

Expand Down Expand Up @@ -102,15 +103,15 @@ function queryStringBuilder(groupedFilters: Record<SplitIO.SplitFilterType, stri
function sanitizeFlagSets(log: ILogger, flagSets: string[]) {
let sanitizedSets = flagSets
.map(flagSet => {
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;
Expand All @@ -128,14 +129,15 @@ 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.
* - `groupedFilters`: list of values grouped by filter type.
*
* @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: [],
Expand All @@ -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);
Expand Down
Loading