From 333c0c0db112d4f9448351727beb49b69e22ec88 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Mon, 17 Jul 2023 09:53:32 +0200 Subject: [PATCH] #4205: update to prepare for emitting potentially stale events (#4239) This PR adds updates the potentially stale status change events whenever the potentially stale update function is run. No events are emitted yet. While the emission is only a few lines of code, I'd like to do that in a separate PR so that we can give it the attention it deserves in the form of tests, etc. This PR also moves the potentially stale update functionality from the `update` method to only being done in the `updatePotentiallyStaleFeatures` method. This keeps all functionality related to marking `potentiallyStale` in one place. The emission implementation was removed in https://github.com/Unleash/unleash/commit/4fb7cbde03b43320442633888f5ba4c0c3c77b31 ## The update queries While it would be possible to do the state updates in a single query instead of three separate ones, wrangling this into knex proved to be troublesome (and would also probably be harder to understand and reason about). The current solution uses three smaller queries (one select, two updates), as Jaanus suggested in a private slack thread. --- src/lib/db/feature-toggle-store.ts | 83 ++++++++++--------- src/lib/services/feature-toggle-service.ts | 4 +- src/lib/services/index.ts | 2 +- src/lib/types/events.ts | 4 + src/lib/types/stores/feature-toggle-store.ts | 4 +- .../stores/feature-toggle-store.e2e.test.ts | 70 ++++++++++------ .../fixtures/fake-feature-toggle-store.ts | 4 +- 7 files changed, 101 insertions(+), 70 deletions(-) diff --git a/src/lib/db/feature-toggle-store.ts b/src/lib/db/feature-toggle-store.ts index 2da29c7d75..2ec492a4b8 100644 --- a/src/lib/db/feature-toggle-store.ts +++ b/src/lib/db/feature-toggle-store.ts @@ -263,25 +263,7 @@ export default class FeatureToggleStore implements IFeatureToggleStore { .update(this.dtoToRow(project, data)) .returning(FEATURE_COLUMNS); - const feature = this.rowToFeature(row[0]); - // if a feature toggle's type or createdAt has changed, update its potentially stale status - if (!feature.stale && (data.type || data.createdAt)) { - await this.db(TABLE) - .where({ name: data.name }) - .update( - 'potentially_stale', - this.db.raw( - `(? > (features.created_at + (( - SELECT feature_types.lifetime_days - FROM feature_types - WHERE feature_types.id = features.type - ) * INTERVAL '1 day')))`, - this.db.fn.now(), - ), - ); - } - - return feature; + return this.rowToFeature(row[0]); } async archive(name: string): Promise { @@ -397,29 +379,50 @@ export default class FeatureToggleStore implements IFeatureToggleStore { return toggle; } - async markPotentiallyStaleFeatures( + async updatePotentiallyStaleFeatures( currentTime?: string, - ): Promise { - const query = this.db(TABLE) - .update('potentially_stale', true) - .whereRaw( - `? > (features.created_at + (( - SELECT feature_types.lifetime_days - FROM feature_types - WHERE feature_types.id = features.type - ) * INTERVAL '1 day'))`, - [currentTime || this.db.fn.now()], - ) - .andWhere(function () { - this.where('potentially_stale', null).orWhere( - 'potentially_stale', - false, - ); - }) - .andWhereNot('stale', true); + ): Promise<{ name: string; potentiallyStale: boolean }[]> { + const query = this.db.raw( + `SELECT name, potentially_stale, (? > (features.created_at + (( + SELECT feature_types.lifetime_days + FROM feature_types + WHERE feature_types.id = features.type + ) * INTERVAL '1 day'))) as current_staleness + FROM features + WHERE NOT stale = true`, + [currentTime || this.db.fn.now()], + ); - const updatedFeatures = await query.returning(FEATURE_COLUMNS); - return updatedFeatures.map(({ name }) => name); + const featuresToUpdate = (await query).rows + .filter( + ({ potentially_stale, current_staleness }) => + (potentially_stale ?? false) !== + (current_staleness ?? false), + ) + .map(({ current_staleness, name }) => ({ + potentiallyStale: current_staleness ?? false, + name, + })); + + await this.db(TABLE) + .update('potentially_stale', true) + .whereIn( + 'name', + featuresToUpdate + .filter((feature) => feature.potentiallyStale === true) + .map((feature) => feature.name), + ); + + await this.db(TABLE) + .update('potentially_stale', false) + .whereIn( + 'name', + featuresToUpdate + .filter((feature) => feature.potentiallyStale !== true) + .map((feature) => feature.name), + ); + + return featuresToUpdate; } async isPotentiallyStale(featureName: string): Promise { diff --git a/src/lib/services/feature-toggle-service.ts b/src/lib/services/feature-toggle-service.ts index 57eb4891a1..f369397d18 100644 --- a/src/lib/services/feature-toggle-service.ts +++ b/src/lib/services/feature-toggle-service.ts @@ -1983,8 +1983,8 @@ class FeatureToggleService { } } - async markPotentiallyStaleFeatures(): Promise { - await this.featureToggleStore.markPotentiallyStaleFeatures(); + async updatePotentiallyStaleFeatures(): Promise { + await this.featureToggleStore.updatePotentiallyStaleFeatures(); } } diff --git a/src/lib/services/index.ts b/src/lib/services/index.ts index d85842d050..6800a6c7f3 100644 --- a/src/lib/services/index.ts +++ b/src/lib/services/index.ts @@ -128,7 +128,7 @@ export const scheduleServices = async ( ); schedulerService.schedule( - featureToggleService.markPotentiallyStaleFeatures.bind( + featureToggleService.updatePotentiallyStaleFeatures.bind( featureToggleService, ), minutesToMilliseconds(1), diff --git a/src/lib/types/events.ts b/src/lib/types/events.ts index f8749511c9..d0bb6452a2 100644 --- a/src/lib/types/events.ts +++ b/src/lib/types/events.ts @@ -122,6 +122,9 @@ export const SERVICE_ACCOUNT_CREATED = 'service-account-created' as const; export const SERVICE_ACCOUNT_UPDATED = 'service-account-updated' as const; export const SERVICE_ACCOUNT_DELETED = 'service-account-deleted' as const; +export const FEATURE_POTENTIALLY_STALE_UPDATED = + 'feature-potentially-stale-updated' as const; + export const IEventTypes = [ APPLICATION_CREATED, FEATURE_CREATED, @@ -223,6 +226,7 @@ export const IEventTypes = [ SERVICE_ACCOUNT_CREATED, SERVICE_ACCOUNT_DELETED, SERVICE_ACCOUNT_UPDATED, + FEATURE_POTENTIALLY_STALE_UPDATED, ] as const; export type IEventType = typeof IEventTypes[number]; diff --git a/src/lib/types/stores/feature-toggle-store.ts b/src/lib/types/stores/feature-toggle-store.ts index ac0f0f8316..76c029bbfc 100644 --- a/src/lib/types/stores/feature-toggle-store.ts +++ b/src/lib/types/stores/feature-toggle-store.ts @@ -32,7 +32,9 @@ export interface IFeatureToggleStore extends Store { range?: string[]; dateAccessor: string; }): Promise; - markPotentiallyStaleFeatures(currentTime?: string): Promise; + updatePotentiallyStaleFeatures( + currentTime?: string, + ): Promise<{ name: string; potentiallyStale: boolean }[]>; isPotentiallyStale(featureName: string): Promise; /** diff --git a/src/test/e2e/stores/feature-toggle-store.e2e.test.ts b/src/test/e2e/stores/feature-toggle-store.e2e.test.ts index 0b927ea3ff..f25b30f17b 100644 --- a/src/test/e2e/stores/feature-toggle-store.e2e.test.ts +++ b/src/test/e2e/stores/feature-toggle-store.e2e.test.ts @@ -42,7 +42,7 @@ describe('potentially_stale marking', () => { ).toISOString(); }; - test('it returns an empty list of no toggles were updated', async () => { + test('it returns an empty list if no toggles were updated', async () => { const features: FeatureToggleDTO[] = [ { name: 'feature1', @@ -56,11 +56,12 @@ describe('potentially_stale marking', () => { ); const markedToggles = - await featureToggleStore.markPotentiallyStaleFeatures(); + await featureToggleStore.updatePotentiallyStaleFeatures(); expect(markedToggles).toStrictEqual([]); }); - test('it returns the names of only the marked toggles', async () => { + + test('it returns only updated toggles', async () => { const features: FeatureToggleDTO[] = [ { name: 'feature1', @@ -78,11 +79,13 @@ describe('potentially_stale marking', () => { ); const markedToggles = - await featureToggleStore.markPotentiallyStaleFeatures( + await featureToggleStore.updatePotentiallyStaleFeatures( getFutureTimestamp(41), ); - expect(markedToggles).toStrictEqual(['feature1']); + expect(markedToggles).toStrictEqual([ + { name: 'feature1', potentiallyStale: true }, + ]); }); test.each([ @@ -111,12 +114,17 @@ describe('potentially_stale marking', () => { } const markedToggles = - await featureToggleStore.markPotentiallyStaleFeatures( + await featureToggleStore.updatePotentiallyStaleFeatures( getFutureTimestamp(daysElapsed), ); expect(markedToggles).toEqual( - expect.arrayContaining(expectedMarkedFeatures), + expect.arrayContaining( + expectedMarkedFeatures.map((name: string) => ({ + name, + potentiallyStale: true, + })), + ), ); expect(markedToggles.length).toEqual(expectedMarkedFeatures.length); @@ -141,11 +149,12 @@ describe('potentially_stale marking', () => { ), ); const markedToggles = - await featureToggleStore.markPotentiallyStaleFeatures( + await featureToggleStore.updatePotentiallyStaleFeatures( getFutureTimestamp(1000), ); expect(markedToggles).toStrictEqual([]); }); + test('it does not return toggles previously marked as potentially_stale', async () => { const features: FeatureToggleDTO[] = [ { @@ -159,28 +168,27 @@ describe('potentially_stale marking', () => { ), ); const markedToggles = - await featureToggleStore.markPotentiallyStaleFeatures( + await featureToggleStore.updatePotentiallyStaleFeatures( getFutureTimestamp(50), ); - expect(markedToggles).toStrictEqual(['feature1']); + + expect(markedToggles).toStrictEqual([ + { name: 'feature1', potentiallyStale: true }, + ]); + + expect( + await featureToggleStore.isPotentiallyStale('feature1'), + ).toBeTruthy(); const secondPass = - await featureToggleStore.markPotentiallyStaleFeatures( + await featureToggleStore.updatePotentiallyStaleFeatures( getFutureTimestamp(100), ); + expect(secondPass).toStrictEqual([]); }); describe('changing feature types', () => { - const getPastDate = (days: number) => { - return new Date( - new Date().getTime() - - days * 24 * 60 * 60 * 1000 - - // subtract an extra second - 1000, - ); - }; - test("if a potentially stale feature changes to a type that shouldn't be stale, it's 'potentially_stale' marker is removed.", async () => { const features: FeatureToggleDTO[] = [ { @@ -194,11 +202,14 @@ describe('potentially_stale marking', () => { ), ); const markedToggles = - await featureToggleStore.markPotentiallyStaleFeatures( + await featureToggleStore.updatePotentiallyStaleFeatures( getFutureTimestamp(50), ); - expect(markedToggles).toStrictEqual(['feature1']); + expect(markedToggles).toStrictEqual([ + { name: 'feature1', potentiallyStale: true }, + ]); + expect( await featureToggleStore.isPotentiallyStale('feature1'), ).toBeTruthy(); @@ -208,6 +219,10 @@ describe('potentially_stale marking', () => { type: 'kill-switch', }); + expect( + await featureToggleStore.updatePotentiallyStaleFeatures(), + ).toStrictEqual([{ name: 'feature1', potentiallyStale: false }]); + const potentiallyStale = await featureToggleStore.isPotentiallyStale('feature1'); @@ -227,7 +242,7 @@ describe('potentially_stale marking', () => { ), ); const markedToggles = - await featureToggleStore.markPotentiallyStaleFeatures( + await featureToggleStore.updatePotentiallyStaleFeatures( getFutureTimestamp(50), ); @@ -236,9 +251,11 @@ describe('potentially_stale marking', () => { await featureToggleStore.update('default', { name: 'feature1', type: 'release', - createdAt: getPastDate(40), }); + await featureToggleStore.updatePotentiallyStaleFeatures( + getFutureTimestamp(40), + ); const potentiallyStale = await featureToggleStore.isPotentiallyStale('feature1'); @@ -262,9 +279,12 @@ describe('potentially_stale marking', () => { await featureToggleStore.update('default', { name: 'feature1', type: 'release', - createdAt: getPastDate(40), }); + await featureToggleStore.updatePotentiallyStaleFeatures( + getFutureTimestamp(40), + ); + const potentiallyStale = await featureToggleStore.isPotentiallyStale('feature1'); diff --git a/src/test/fixtures/fake-feature-toggle-store.ts b/src/test/fixtures/fake-feature-toggle-store.ts index e35d9d768e..e214a237e1 100644 --- a/src/test/fixtures/fake-feature-toggle-store.ts +++ b/src/test/fixtures/fake-feature-toggle-store.ts @@ -253,7 +253,9 @@ export default class FakeFeatureToggleStore implements IFeatureToggleStore { return Promise.resolve(); } - markPotentiallyStaleFeatures(): Promise { + updatePotentiallyStaleFeatures(): Promise< + { name: string; potentiallyStale: boolean }[] + > { throw new Error('Method not implemented.'); }