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-7657] fix SDK_UPDATE issue #264

Closed
wants to merge 2 commits into from
Closed
Changes from 1 commit
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
Next Next commit
[SDKS-7657] fix SDK_UPDATE issue
  • Loading branch information
Emmanuel Zamora committed Oct 31, 2023
commit 39661866daba65d5cefd68d60ebfdab86db4721f
68 changes: 57 additions & 11 deletions src/sync/polling/updaters/__tests__/splitChangesUpdater.spec.ts
Original file line number Diff line number Diff line change
@@ -4,14 +4,15 @@ import { splitApiFactory } from '../../../../services/splitApi';
import { SegmentsCacheInMemory } from '../../../../storages/inMemory/SegmentsCacheInMemory';
import { SplitsCacheInMemory } from '../../../../storages/inMemory/SplitsCacheInMemory';
import { splitChangesFetcherFactory } from '../../fetchers/splitChangesFetcher';
import { splitChangesUpdaterFactory, parseSegments, computeSplitsMutation } from '../splitChangesUpdater';
import { splitChangesUpdaterFactory, parseSegments, computeSplitsMutation, computeFromSets } from '../splitChangesUpdater';
import splitChangesMock1 from '../../../../__tests__/mocks/splitchanges.since.-1.json';
import fetchMock from '../../../../__tests__/testUtils/fetchMock';
import { settingsSplitApi } from '../../../../utils/settingsValidation/__tests__/settings.mocks';
import { EventEmitter } from '../../../../utils/MinEvents';
import { loggerMock } from '../../../../logger/__tests__/sdkLogger.mock';
import { telemetryTrackerFactory } from '../../../../trackers/telemetryTracker';
import { splitNotifications } from '../../../streaming/__tests__/dataMocks';
import { ISplitsCacheBase } from '../../../../storages/types';

const ARCHIVED_FF = 'ARCHIVED';

@@ -111,35 +112,44 @@ test('splitChangesUpdater / compute splits mutation', () => {

test('splitChangesUpdater / compute splits mutation with filters', () => {
// SDK initialization with sets: [set_a, set_b]
let splitFiltersValidation = { queryString: '&sets=set_a,set_b', groupedFilters: { bySet: ['set_a','set_b'], byName: ['name_1'], byPrefix: [] }, validFilters: [] };
const configuredSets = ['set_a','set_b'];

const updateCache = (splits: ISplitsCacheBase, mutation: any) => {
splits.addSplits(mutation.added);
splits.removeSplits(mutation.removed);
};

let splitsCache = new SplitsCacheInMemory();
// fetching new feature flag in sets A & B
let splitsMutation = computeSplitsMutation([testFFSetsAB], splitFiltersValidation);
let splitsMutation = computeFromSets([testFFSetsAB], configuredSets, splitsCache);

// should add it to mutations
expect(splitsMutation.added).toEqual([[testFFSetsAB.name, testFFSetsAB]]);
expect(splitsMutation.removed).toEqual([]);
updateCache(splitsCache, splitsMutation);

// fetching existing test feature flag removed from set B
splitsMutation = computeSplitsMutation([testFFRemoveSetB], splitFiltersValidation);
splitsMutation = computeFromSets([testFFRemoveSetB], configuredSets, splitsCache);

expect(splitsMutation.added).toEqual([[testFFRemoveSetB.name, testFFRemoveSetB]]);
expect(splitsMutation.removed).toEqual([]);
updateCache(splitsCache, splitsMutation);

// fetching existing test feature flag removed from set B
splitsMutation = computeSplitsMutation([testFFRemoveSetA], splitFiltersValidation);
// fetching existing test feature flag removed from set A
splitsMutation = computeFromSets([testFFRemoveSetA], configuredSets, splitsCache);

expect(splitsMutation.added).toEqual([]);
expect(splitsMutation.removed).toEqual([testFFRemoveSetA.name]);
updateCache(splitsCache, splitsMutation);

// fetching existing test feature flag removed from set B
splitsMutation = computeSplitsMutation([testFFEmptySet], splitFiltersValidation);
splitsMutation = computeFromSets([testFFEmptySet], configuredSets, splitsCache);

expect(splitsMutation.added).toEqual([]);
expect(splitsMutation.removed).toEqual([testFFEmptySet.name]);
expect(splitsMutation.removed).toEqual([]);

// SDK initialization with names: ['test2']
splitFiltersValidation = { queryString: '&names=test2', groupedFilters: { bySet: [], byName: ['test2'], byPrefix: [] }, validFilters: [] };
const splitFiltersValidation = { queryString: '&names=test2', groupedFilters: { bySet: [], byName: ['test2'], byPrefix: [] }, validFilters: [] };
splitsMutation = computeSplitsMutation([testFFSetsAB], splitFiltersValidation);

expect(splitsMutation.added).toEqual([]);
@@ -168,9 +178,9 @@ describe('splitChangesUpdater', () => {
const readinessManager = readinessManagerFactory(EventEmitter);
const splitsEmitSpy = jest.spyOn(readinessManager.splits, 'emit');

const splitFiltersValidation = { queryString: null, groupedFilters: { bySet: [], byName: [], byPrefix: [] }, validFilters: [] };
let splitFiltersValidation = { queryString: null, groupedFilters: { bySet: [], byName: [], byPrefix: [] }, validFilters: [] };

const splitChangesUpdater = splitChangesUpdaterFactory(loggerMock, splitChangesFetcher, splitsCache, segmentsCache, splitFiltersValidation, readinessManager.splits, 1000, 1);
let splitChangesUpdater = splitChangesUpdaterFactory(loggerMock, splitChangesFetcher, splitsCache, segmentsCache, splitFiltersValidation, readinessManager.splits, 1000, 1);

afterEach(() => {
jest.clearAllMocks();
@@ -213,4 +223,40 @@ describe('splitChangesUpdater', () => {
index++;
}
});

test('flag sets splits-arrived emition', async () => {
const payload = splitNotifications[3].decoded as Pick<ISplit, 'name' | 'changeNumber' | 'killed' | 'defaultTreatment' | 'trafficTypeName' | 'conditions' | 'status' | 'seed' | 'trafficAllocation' | 'trafficAllocationSeed' | 'configurations'>;
const setMocks = [
{ sets: [], shouldEmit: false }, /* should not emit if flag does not have any set */
{ sets: ['set_a'], shouldEmit: true }, /* should emit if flag is in configured sets */
{ sets: ['set_b'], shouldEmit: true }, /* should emit if flag was just removed from configured sets */
{ sets: ['set_b'], shouldEmit: false }, /* should NOT emit if flag is nor was just removed from configured sets */
{ sets: ['set_c'], shouldEmit: false }, /* should NOT emit if flag is nor was just removed from configured sets */
{ sets: ['set_a'], shouldEmit: true }, /* should emit if flag is back in configured sets */
];

splitChangesUpdater = splitChangesUpdaterFactory(loggerMock, splitChangesFetcher, new SplitsCacheInMemory(), segmentsCache, splitFiltersValidation, readinessManager.splits, 1000, 1, true);

let index = 0;
let calls = 0;
// emit always if not configured sets
for (const setMock of setMocks) {
await expect(splitChangesUpdater(undefined, undefined, { payload: {...payload, sets: setMock.sets, status: 'ACTIVE'}, changeNumber: index })).resolves.toBe(true);
expect(splitsEmitSpy.mock.calls[index][0]).toBe('state::splits-arrived');
index++;
}

// @ts-ignore
splitFiltersValidation = { queryString: null, groupedFilters: { bySet: ['set_a'], byName: [], byPrefix: [] }, validFilters: [] };
splitChangesUpdater = splitChangesUpdaterFactory(loggerMock, splitChangesFetcher, new SplitsCacheInMemory(), segmentsCache, splitFiltersValidation, readinessManager.splits, 1000, 1, true);
splitsEmitSpy.mockReset();
index = 0;
for (const setMock of setMocks) {
await expect(splitChangesUpdater(undefined, undefined, { payload: {...payload, sets: setMock.sets, status: 'ACTIVE'}, changeNumber: index })).resolves.toBe(true);
if (setMock.shouldEmit) calls++;
expect(splitsEmitSpy.mock.calls.length).toBe(calls);
index++;
}

});
});
47 changes: 43 additions & 4 deletions src/sync/polling/updaters/splitChangesUpdater.ts
Original file line number Diff line number Diff line change
@@ -54,8 +54,8 @@ interface ISplitMutations {
* @param filters splitFiltersValidation bySet | byName
*/
function matchFilters(featureFlag: ISplit, filters: ISplitFiltersValidation) {
const { bySet: setsFilter, byName: namesFilter, byPrefix: prefixFilter} = filters.groupedFilters;
if (setsFilter.length > 0) return featureFlag.sets && featureFlag.sets.some((featureFlagSet: string) => setsFilter.indexOf(featureFlagSet) > -1);
const { byName: namesFilter, byPrefix: prefixFilter} = filters.groupedFilters;
// if (setsFilter.length > 0) return featureFlag.sets && featureFlag.sets.some((featureFlagSet: string) => setsFilter.indexOf(featureFlagSet) > -1);

const namesFilterConfigured = namesFilter.length > 0;
const prefixFilterConfigured = prefixFilter.length > 0;
@@ -67,6 +67,42 @@ function matchFilters(featureFlag: ISplit, filters: ISplitFiltersValidation) {
return matchNames || matchPrefix;
}

/**
* Given the list of splits from /splitChanges endpoint, configured sets & splitsCache it returns the mutations for configured sets,
* i.e., an object with added splits, removed splits and used segments.
* Exported for testing purposes.
*/
export function computeFromSets(entries: ISplit[], setsFilter: string[], splitsCache: ISplitsCacheBase): ISplitMutations {
const segments = new _Set<string>();
const computedFromSets = entries.reduce((accum, flag) => {

// validate that flag belongs to configured flag sets
if (flag.sets && flag.sets.some((featureFlagSet: string) => setsFilter.indexOf(featureFlagSet) > -1)) {
if (flag.status === 'ACTIVE') {
accum.added.push([flag.name, flag]);

parseSegments(flag).forEach((segmentName: string) => {
segments.add(segmentName);
});

} else {
accum.removed.push(flag.name);
}
} else {

// check if it is a flag removed from a configured flag set
if (splitsCache.getSplit(flag.name))
accum.removed.push(flag.name);
}

return accum;
}, { added: [], removed: [], segments: [] } as ISplitMutations);

computedFromSets.segments = setToArray(segments);

return computedFromSets;
}

/**
* Given the list of splits from /splitChanges endpoint, it returns the mutations,
* i.e., an object with added splits, removed splits and used segments.
@@ -148,8 +184,9 @@ export function splitChangesUpdaterFactory(
)
.then((splitChanges: ISplitChangesResponse) => {
startingUp = false;

const mutation = computeSplitsMutation(splitChanges.splits, splitFiltersValidation);
const setsFilter = splitFiltersValidation.groupedFilters.bySet;
const configuredFilters = setsFilter.length > 0;
const mutation = configuredFilters ? computeFromSets(splitChanges.splits, setsFilter, splits) : computeSplitsMutation(splitChanges.splits, splitFiltersValidation);

log.debug(SYNC_SPLITS_NEW, [mutation.added.length]);
log.debug(SYNC_SPLITS_REMOVED, [mutation.removed.length]);
@@ -170,6 +207,8 @@ export function splitChangesUpdaterFactory(
return Promise.resolve(!splitsEventEmitter.splitsArrived || (since !== splitChanges.till && (isClientSide || checkAllSegmentsExist(segments))))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can follow the same approach than Android SDK https://github.com/splitio/android-client/pull/550/files
Remove computeFromSets function, and instead use the return value of splits.removeSplits(mutation.removed):

          return Promise.all([
            splits.setChangeNumber(splitChanges.till),
            splits.addSplits(mutation.added),
            splits.removeSplits(mutation.removed),
            segments.registerSegments(mutation.segments)
          ]).then(([,addedList,removedList,]) => {

.catch(() => false /** noop. just to handle a possible `checkAllSegmentsExist` rejection, before emitting SDK event */)
.then(emitSplitsArrivedEvent => {
// if there are configured filters and there isn't any change on mutations, skip emiting
if (configuredFilters && mutation.added.length === 0 && mutation.removed.length === 0) return true;
// emit SDK events
if (emitSplitsArrivedEvent) splitsEventEmitter.emit(SDK_SPLITS_ARRIVED);
return true;
Loading