From f7e32d4c7e504bb63f6449b667b91b2d9a98f4e7 Mon Sep 17 00:00:00 2001 From: sjaanus Date: Fri, 13 Dec 2024 11:54:17 +0200 Subject: [PATCH 1/4] feat: deleted feature names should come from event --- .../cache/client-feature-toggle-cache.ts | 92 ++++++++----------- .../client-feature-toggle-service.ts | 13 +-- .../client-feature-toggle.controller.ts | 10 +- 3 files changed, 46 insertions(+), 69 deletions(-) diff --git a/src/lib/features/client-feature-toggles/cache/client-feature-toggle-cache.ts b/src/lib/features/client-feature-toggles/cache/client-feature-toggle-cache.ts index feaf4d27e798..106be8168333 100644 --- a/src/lib/features/client-feature-toggles/cache/client-feature-toggle-cache.ts +++ b/src/lib/features/client-feature-toggles/cache/client-feature-toggle-cache.ts @@ -1,30 +1,27 @@ import type { IEventStore, - IFeatureToggleClient, + IFeatureToggleCacheQuery, IFeatureToggleQuery, IFlagResolver, } from '../../../types'; -import type { FeatureConfigurationClient } from '../../feature-toggle/types/feature-toggle-strategies-store-type'; import type ConfigurationRevisionService from '../../feature-toggle/configuration-revision-service'; import { UPDATE_REVISION } from '../../feature-toggle/configuration-revision-service'; import { RevisionCache } from './revision-cache'; -import type { IClientFeatureToggleCacheReadModel } from './client-feature-toggle-cache-read-model-type'; - -type DeletedFeature = { - name: string; - project: string; -}; +import type { + FeatureConfigurationCacheClient, + IClientFeatureToggleCacheReadModel, +} from './client-feature-toggle-cache-read-model-type'; -export type ClientFeatureChange = { - updated: IFeatureToggleClient[]; - removed: DeletedFeature[]; +export type RevisionCacheEntry = { + updated: FeatureConfigurationCacheClient[]; revisionId: number; + removed: string[]; }; export type Revision = { revisionId: number; updated: any[]; - removed: DeletedFeature[]; + removed: string[]; }; type Revisions = Record; @@ -36,15 +33,10 @@ const applyRevision = (first: Revision, last: Revision): Revision => { feature, ]), ); - const removedMap = new Map( - [...first.removed, ...last.removed].map((feature) => [ - feature.name, - feature, - ]), - ); + const removedMap = new Set([...first.removed, ...last.removed]); for (const feature of last.removed) { - updatedMap.delete(feature.name); + updatedMap.delete(feature); } for (const feature of last.updated) { @@ -67,8 +59,7 @@ const filterRevisionByProject = ( projects.includes('*') || projects.includes(feature.project), ); const removed = revision.removed.filter( - (feature) => - projects.includes('*') || projects.includes(feature.project), + (feature) => projects.includes('*') || projects.includes(feature), ); return { ...revision, updated, removed }; }; @@ -132,9 +123,11 @@ export class ClientFeatureToggleCache { async getDelta( sdkRevisionId: number | undefined, - environment: string, - projects: string[], - ): Promise { + query: IFeatureToggleQuery, + ): Promise { + const projects = query.project ? query.project : ['*']; + const environment = query.environment ? query.environment : 'default'; + // TODO: filter by tags, what is namePrefix? anything else? const requiredRevisionId = sdkRevisionId || 0; const hasCache = this.cache[environment] !== undefined; @@ -153,6 +146,7 @@ export class ClientFeatureToggleCache { sdkRevisionId !== this.currentRevisionId && !this.cache[environment].hasRevision(sdkRevisionId)) ) { + //TODO: populate cache based on this? return { revisionId: this.currentRevisionId, // @ts-ignore @@ -201,44 +195,36 @@ export class ClientFeatureToggleCache { .map((event) => event.featureName!), ), ]; - const newToggles = await this.getChangedToggles( - changedToggles, - latestRevision, // TODO: this should come back from the same query to not be out of sync - ); - // TODO: Discussion point. Should we filter events by environment and only add revisions in the cache - // for the environment that changed? If we do that we can also save CPU and memory, because we don't need - // to talk to the database if the cache is not initialized for that environment - for (const key of keys) { - this.cache[key].addRevision(newToggles); + const removed = changeEvents + .filter((event) => event.type === 'feature-deleted') + .map((event) => event.featureName!); + + // TODO: we might want to only update the environments that had events changed for performance + for (const environment of keys) { + const newToggles = await this.getChangedToggles( + environment, + changedToggles, + ); + this.cache[environment].addRevision({ + updated: newToggles, + revisionId: latestRevision, + removed, + }); } this.currentRevisionId = latestRevision; } async getChangedToggles( + environment: string, toggles: string[], - revisionId: number, - ): Promise { + ): Promise { const foundToggles = await this.getClientFeatures({ - // @ts-ignore removed toggleNames from the type, we should not use this method at all, toggleNames: toggles, - environment: 'development', + environment, }); - - const foundToggleNames = foundToggles.map((toggle) => toggle.name); - const removed = toggles - .filter((toggle) => !foundToggleNames.includes(toggle)) - .map((name) => ({ - name, - project: 'default', // TODO: this needs to be smart and figure out the project . IMPORTANT - })); - - return { - updated: foundToggles as any, // impressionData is not on the type but should be - removed, - revisionId, - }; + return foundToggles; } public async initEnvironmentCache(environment: string) { @@ -262,8 +248,8 @@ export class ClientFeatureToggleCache { } async getClientFeatures( - query: IFeatureToggleQuery, - ): Promise { + query: IFeatureToggleCacheQuery, + ): Promise { const result = await this.clientFeatureToggleCacheReadModel.getAll(query); return result; diff --git a/src/lib/features/client-feature-toggles/client-feature-toggle-service.ts b/src/lib/features/client-feature-toggles/client-feature-toggle-service.ts index 92aefffb799d..f80f2341cfb5 100644 --- a/src/lib/features/client-feature-toggles/client-feature-toggle-service.ts +++ b/src/lib/features/client-feature-toggles/client-feature-toggle-service.ts @@ -10,7 +10,7 @@ import type { Logger } from '../../logger'; import type { FeatureConfigurationClient } from '../feature-toggle/types/feature-toggle-strategies-store-type'; import type { - ClientFeatureChange, + RevisionCacheEntry, ClientFeatureToggleCache, } from './cache/client-feature-toggle-cache'; @@ -43,15 +43,10 @@ export class ClientFeatureToggleService { async getClientDelta( revisionId: number | undefined, - projects: string[], - environment: string, - ): Promise { + query: IFeatureToggleQuery, + ): Promise { if (this.clientFeatureToggleCache !== null) { - return this.clientFeatureToggleCache.getDelta( - revisionId, - environment, - projects, - ); + return this.clientFeatureToggleCache.getDelta(revisionId, query); } else { throw new Error( 'Calling the partial updates but the cache is not initialized', diff --git a/src/lib/features/client-feature-toggles/client-feature-toggle.controller.ts b/src/lib/features/client-feature-toggles/client-feature-toggle.controller.ts index f20a06c81b02..de1f6a41c3d2 100644 --- a/src/lib/features/client-feature-toggles/client-feature-toggle.controller.ts +++ b/src/lib/features/client-feature-toggles/client-feature-toggle.controller.ts @@ -33,7 +33,7 @@ import { } from '../../openapi/spec/client-features-schema'; import type ConfigurationRevisionService from '../feature-toggle/configuration-revision-service'; import type { ClientFeatureToggleService } from './client-feature-toggle-service'; -import type { ClientFeatureChange } from './cache/client-feature-toggle-cache'; +import type { RevisionCacheEntry } from './cache/client-feature-toggle-cache'; const version = 2; @@ -300,24 +300,20 @@ export default class FeatureController extends Controller { async getDelta( req: IAuthRequest, - res: Response, + res: Response, ): Promise { if (!this.flagResolver.isEnabled('deltaApi')) { throw new NotFoundError(); } const query = await this.resolveQuery(req); const etag = req.headers['if-none-match']; - const meta = await this.calculateMeta(query); const currentSdkRevisionId = etag ? Number.parseInt(etag) : undefined; - const projects = query.project ? query.project : ['*']; - const environment = query.environment ? query.environment : 'default'; const changedFeatures = await this.clientFeatureToggleService.getClientDelta( currentSdkRevisionId, - projects, - environment, + query, ); if (!changedFeatures) { From b063e82f24ee287a8943c24b6ff18691668f3e88 Mon Sep 17 00:00:00 2001 From: sjaanus Date: Fri, 13 Dec 2024 12:27:17 +0200 Subject: [PATCH 2/4] Fix --- .../cache/client-feature-toggle-cache.test.ts | 53 ++----------------- .../cache/client-feature-toggle-cache.ts | 5 +- 2 files changed, 5 insertions(+), 53 deletions(-) diff --git a/src/lib/features/client-feature-toggles/cache/client-feature-toggle-cache.test.ts b/src/lib/features/client-feature-toggles/cache/client-feature-toggle-cache.test.ts index 5fd8302ad68c..6573adff0ccd 100644 --- a/src/lib/features/client-feature-toggles/cache/client-feature-toggle-cache.test.ts +++ b/src/lib/features/client-feature-toggles/cache/client-feature-toggle-cache.test.ts @@ -51,12 +51,7 @@ test('revision that adds, removes then adds again does not end up with the remov { revisionId: 2, updated: [], - removed: [ - { - name: 'some-toggle', - project: 'default', - }, - ], + removed: ['some-toggle'], }, { revisionId: 3, @@ -81,12 +76,7 @@ test('revision that removes, adds then removes again does not end up with the re { revisionId: 1, updated: [], - removed: [ - { - name: 'some-toggle', - project: 'default', - }, - ], + removed: ['some-toggle'], }, { revisionId: 2, @@ -96,12 +86,7 @@ test('revision that removes, adds then removes again does not end up with the re { revisionId: 3, updated: [], - removed: [ - { - name: 'some-toggle', - project: 'default', - }, - ], + removed: ['some-toggle'], }, ]; @@ -112,12 +97,7 @@ test('revision that removes, adds then removes again does not end up with the re expect(revisions).toEqual({ revisionId: 3, updated: [], - removed: [ - { - name: 'some-toggle', - project: 'default', - }, - ], + removed: ['some-toggle'], }); }); @@ -154,28 +134,3 @@ test('revision equal to the base case returns only later revisions ', () => { removed: [], }); }); - -test('project filter removes features not in project', () => { - const revisionList = [ - { - revisionId: 1, - updated: [mockAdd({ name: 'feature1', project: 'project1' })], - removed: [], - }, - { - revisionId: 2, - updated: [mockAdd({ name: 'feature2', project: 'project2' })], - removed: [], - }, - ]; - - const revisions = calculateRequiredClientRevision(revisionList, 0, [ - 'project1', - ]); - - expect(revisions).toEqual({ - revisionId: 2, - updated: [mockAdd({ name: 'feature1', project: 'project1' })], - removed: [], - }); -}); diff --git a/src/lib/features/client-feature-toggles/cache/client-feature-toggle-cache.ts b/src/lib/features/client-feature-toggles/cache/client-feature-toggle-cache.ts index 106be8168333..a3db4c5d24ff 100644 --- a/src/lib/features/client-feature-toggles/cache/client-feature-toggle-cache.ts +++ b/src/lib/features/client-feature-toggles/cache/client-feature-toggle-cache.ts @@ -73,11 +73,8 @@ export const calculateRequiredClientRevision = ( (revision) => revision.revisionId > requiredRevisionId, ); console.log('targeted revisions', targetedRevisions); - const projectFeatureRevisions = targetedRevisions.map((revision) => - filterRevisionByProject(revision, projects), - ); - return projectFeatureRevisions.reduce(applyRevision); + return targetedRevisions.reduce(applyRevision); }; export class ClientFeatureToggleCache { From fba22c353113087283a1d7592a6044cbbade27c4 Mon Sep 17 00:00:00 2001 From: sjaanus Date: Fri, 13 Dec 2024 14:33:02 +0200 Subject: [PATCH 3/4] Fix --- .../cache/client-feature-toggle-cache.test.ts | 53 +++++++++++++++++-- .../cache/client-feature-toggle-cache.ts | 26 ++++++--- 2 files changed, 69 insertions(+), 10 deletions(-) diff --git a/src/lib/features/client-feature-toggles/cache/client-feature-toggle-cache.test.ts b/src/lib/features/client-feature-toggles/cache/client-feature-toggle-cache.test.ts index 6573adff0ccd..5fd8302ad68c 100644 --- a/src/lib/features/client-feature-toggles/cache/client-feature-toggle-cache.test.ts +++ b/src/lib/features/client-feature-toggles/cache/client-feature-toggle-cache.test.ts @@ -51,7 +51,12 @@ test('revision that adds, removes then adds again does not end up with the remov { revisionId: 2, updated: [], - removed: ['some-toggle'], + removed: [ + { + name: 'some-toggle', + project: 'default', + }, + ], }, { revisionId: 3, @@ -76,7 +81,12 @@ test('revision that removes, adds then removes again does not end up with the re { revisionId: 1, updated: [], - removed: ['some-toggle'], + removed: [ + { + name: 'some-toggle', + project: 'default', + }, + ], }, { revisionId: 2, @@ -86,7 +96,12 @@ test('revision that removes, adds then removes again does not end up with the re { revisionId: 3, updated: [], - removed: ['some-toggle'], + removed: [ + { + name: 'some-toggle', + project: 'default', + }, + ], }, ]; @@ -97,7 +112,12 @@ test('revision that removes, adds then removes again does not end up with the re expect(revisions).toEqual({ revisionId: 3, updated: [], - removed: ['some-toggle'], + removed: [ + { + name: 'some-toggle', + project: 'default', + }, + ], }); }); @@ -134,3 +154,28 @@ test('revision equal to the base case returns only later revisions ', () => { removed: [], }); }); + +test('project filter removes features not in project', () => { + const revisionList = [ + { + revisionId: 1, + updated: [mockAdd({ name: 'feature1', project: 'project1' })], + removed: [], + }, + { + revisionId: 2, + updated: [mockAdd({ name: 'feature2', project: 'project2' })], + removed: [], + }, + ]; + + const revisions = calculateRequiredClientRevision(revisionList, 0, [ + 'project1', + ]); + + expect(revisions).toEqual({ + revisionId: 2, + updated: [mockAdd({ name: 'feature1', project: 'project1' })], + removed: [], + }); +}); diff --git a/src/lib/features/client-feature-toggles/cache/client-feature-toggle-cache.ts b/src/lib/features/client-feature-toggles/cache/client-feature-toggle-cache.ts index a3db4c5d24ff..15de4d1f53b0 100644 --- a/src/lib/features/client-feature-toggles/cache/client-feature-toggle-cache.ts +++ b/src/lib/features/client-feature-toggles/cache/client-feature-toggle-cache.ts @@ -12,16 +12,21 @@ import type { IClientFeatureToggleCacheReadModel, } from './client-feature-toggle-cache-read-model-type'; +type DeletedFeature = { + name: string; + project: string; +}; + export type RevisionCacheEntry = { updated: FeatureConfigurationCacheClient[]; revisionId: number; - removed: string[]; + removed: DeletedFeature[]; }; export type Revision = { revisionId: number; updated: any[]; - removed: string[]; + removed: DeletedFeature[]; }; type Revisions = Record; @@ -33,10 +38,15 @@ const applyRevision = (first: Revision, last: Revision): Revision => { feature, ]), ); - const removedMap = new Set([...first.removed, ...last.removed]); + const removedMap = new Map( + [...first.removed, ...last.removed].map((feature) => [ + feature.name, + feature, + ]), + ); for (const feature of last.removed) { - updatedMap.delete(feature); + updatedMap.delete(feature.name); } for (const feature of last.updated) { @@ -59,7 +69,8 @@ const filterRevisionByProject = ( projects.includes('*') || projects.includes(feature.project), ); const removed = revision.removed.filter( - (feature) => projects.includes('*') || projects.includes(feature), + (feature) => + projects.includes('*') || projects.includes(feature.project), ); return { ...revision, updated, removed }; }; @@ -73,8 +84,11 @@ export const calculateRequiredClientRevision = ( (revision) => revision.revisionId > requiredRevisionId, ); console.log('targeted revisions', targetedRevisions); + const projectFeatureRevisions = targetedRevisions.map((revision) => + filterRevisionByProject(revision, projects), + ); - return targetedRevisions.reduce(applyRevision); + return projectFeatureRevisions.reduce(applyRevision); }; export class ClientFeatureToggleCache { From 4b7cd16e02053e0a015a9e4671ec6b67ec8c11e5 Mon Sep 17 00:00:00 2001 From: sjaanus Date: Fri, 13 Dec 2024 14:38:47 +0200 Subject: [PATCH 4/4] Fix --- .../cache/client-feature-toggle-cache.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/lib/features/client-feature-toggles/cache/client-feature-toggle-cache.ts b/src/lib/features/client-feature-toggles/cache/client-feature-toggle-cache.ts index 15de4d1f53b0..45d83a302990 100644 --- a/src/lib/features/client-feature-toggles/cache/client-feature-toggle-cache.ts +++ b/src/lib/features/client-feature-toggles/cache/client-feature-toggle-cache.ts @@ -208,8 +208,12 @@ export class ClientFeatureToggleCache { ]; const removed = changeEvents + .filter((event) => event.featureName && event.project) .filter((event) => event.type === 'feature-deleted') - .map((event) => event.featureName!); + .map((event) => ({ + name: event.featureName!, + project: event.project!, + })); // TODO: we might want to only update the environments that had events changed for performance for (const environment of keys) {