From f26ebc828a8d948a409c5657e9a4c7260d156291 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Thu, 15 May 2025 13:38:14 +0200 Subject: [PATCH 1/8] block requests on the server side --- src/lib/error/conflict-error.ts | 6 ++++ src/lib/error/unleash-error.ts | 1 + src/lib/features/context/context-service.ts | 10 +++++++ src/test/e2e/api/admin/context.e2e.test.ts | 33 +++++++++++++++++++++ 4 files changed, 50 insertions(+) create mode 100644 src/lib/error/conflict-error.ts diff --git a/src/lib/error/conflict-error.ts b/src/lib/error/conflict-error.ts new file mode 100644 index 000000000000..f59cab9401da --- /dev/null +++ b/src/lib/error/conflict-error.ts @@ -0,0 +1,6 @@ +import { UnleashError } from './unleash-error.js'; + +class ConflictError extends UnleashError { + statusCode = 409; +} +export default ConflictError; diff --git a/src/lib/error/unleash-error.ts b/src/lib/error/unleash-error.ts index 0136c472d0d3..2d6ff187c827 100644 --- a/src/lib/error/unleash-error.ts +++ b/src/lib/error/unleash-error.ts @@ -3,6 +3,7 @@ import type { FromSchema } from 'json-schema-to-ts'; export const UnleashApiErrorTypes = [ 'ContentTypeError', + 'ConflictErrror', 'DisabledError', 'FeatureHasTagError', 'IncompatibleProjectError', diff --git a/src/lib/features/context/context-service.ts b/src/lib/features/context/context-service.ts index 38adcc190389..57e3ca510988 100644 --- a/src/lib/features/context/context-service.ts +++ b/src/lib/features/context/context-service.ts @@ -29,6 +29,7 @@ import { CONTEXT_FIELD_UPDATED, CONTEXT_FIELD_DELETED, } from '../../events/index.js'; +import ConflictError from '../../error/conflict-error.js'; class ContextService { private eventService: EventService; @@ -239,6 +240,15 @@ class ContextService { ): Promise { const contextField = await this.contextFieldStore.get(name); + const strategies = + await this.featureStrategiesStore.getStrategiesByContextField(name); + + if (strategies.length > 0) { + throw new ConflictError( + `This context field is in use by existing flags. To delete it, first remove its usage from all flags.`, + ); + } + // delete await this.contextFieldStore.delete(name); await this.eventService.storeEvent({ diff --git a/src/test/e2e/api/admin/context.e2e.test.ts b/src/test/e2e/api/admin/context.e2e.test.ts index a4749d8f403f..4a41a256a714 100644 --- a/src/test/e2e/api/admin/context.e2e.test.ts +++ b/src/test/e2e/api/admin/context.e2e.test.ts @@ -179,6 +179,39 @@ test('should delete context field', async () => { return app.request.delete('/api/admin/context/userId').expect(200); }); +test('should not delete a context field that is in use', async () => { + const context = 'appName'; + const feature = 'contextFeature'; + await app.request + .post('/api/admin/projects/default/features') + .send({ + name: feature, + enabled: false, + strategies: [{ name: 'default' }], + }) + .set('Content-Type', 'application/json') + .expect(201); + await app.request + .post( + `/api/admin/projects/default/features/${feature}/environments/default/strategies`, + ) + .send({ + name: 'default', + constraints: [ + { + contextName: context, + operator: 'IN', + values: ['test'], + caseInsensitive: false, + inverted: false, + }, + ], + }) + .expect(200); + + return app.request.delete(`/api/admin/context/${context}`).expect(409); +}); + test('refuses to create a context not url-friendly name', async () => { expect.assertions(0); return app.request From 2202fa6474affd7bd138422a422fdbba6186ebbb Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Fri, 16 May 2025 09:28:23 +0200 Subject: [PATCH 2/8] update tests to not count or block on archived flags --- src/test/e2e/api/admin/context.e2e.test.ts | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/src/test/e2e/api/admin/context.e2e.test.ts b/src/test/e2e/api/admin/context.e2e.test.ts index 4a41a256a714..ed38b12aea84 100644 --- a/src/test/e2e/api/admin/context.e2e.test.ts +++ b/src/test/e2e/api/admin/context.e2e.test.ts @@ -179,7 +179,7 @@ test('should delete context field', async () => { return app.request.delete('/api/admin/context/userId').expect(200); }); -test('should not delete a context field that is in use', async () => { +test('should not delete a context field that is in use by active flags', async () => { const context = 'appName'; const feature = 'contextFeature'; await app.request @@ -209,7 +209,15 @@ test('should not delete a context field that is in use', async () => { }) .expect(200); - return app.request.delete(`/api/admin/context/${context}`).expect(409); + app.request.delete(`/api/admin/context/${context}`).expect(409); + + await app.archiveFeature('contextFeature').expect(202); + + const { body: postArchiveBody } = await app.request.get( + `/api/admin/context/${context}/strategies`, + ); + + expect(postArchiveBody.strategies).toHaveLength(0); }); test('refuses to create a context not url-friendly name', async () => { @@ -274,7 +282,7 @@ test('should update context field with stickiness', async () => { expect(contextField.stickiness).toBe(true); }); -test('should show context field usage', async () => { +test('should show context field usage for active flags', async () => { const context = 'appName'; const feature = 'contextFeature'; await app.request @@ -320,4 +328,12 @@ test('should show context field usage', async () => { expect(body).toMatchObject({ strategies: [{ environment: 'default', featureName: 'contextFeature' }], }); + + await app.archiveFeature('contextFeature').expect(202); + + const { body: postArchiveBody } = await app.request.get( + `/api/admin/context/${context}/strategies`, + ); + + expect(postArchiveBody.strategies).toHaveLength(0); }); From 7f33a7c3b161dc26993408abc99cec771c47c790 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Fri, 16 May 2025 09:37:32 +0200 Subject: [PATCH 3/8] filter for active flags --- .../feature-toggle/feature-toggle-strategies-store.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/lib/features/feature-toggle/feature-toggle-strategies-store.ts b/src/lib/features/feature-toggle/feature-toggle-strategies-store.ts index 4eb763e32afa..6303b9487fac 100644 --- a/src/lib/features/feature-toggle/feature-toggle-strategies-store.ts +++ b/src/lib/features/feature-toggle/feature-toggle-strategies-store.ts @@ -925,6 +925,12 @@ class FeatureStrategiesStore implements IFeatureStrategiesStore { const rows = await this.db .select(this.prefixColumns()) .from(T.featureStrategies) + .join( + T.features, + `${T.features}.name`, + `${T.featureStrategies}.feature_name`, + ) + .where(`${T.features}.archived_at`, 'IS', null) .where( this.db.raw( "EXISTS (SELECT 1 FROM jsonb_array_elements(constraints) AS elem WHERE elem ->> 'contextName' = ?)", From 0955b07fbafdca54e66b4189a4e87307cb423361 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Fri, 16 May 2025 09:55:40 +0200 Subject: [PATCH 4/8] remove comment --- src/lib/features/context/context-field-store.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/lib/features/context/context-field-store.ts b/src/lib/features/context/context-field-store.ts index 603b3174789f..8b4043b361fe 100644 --- a/src/lib/features/context/context-field-store.ts +++ b/src/lib/features/context/context-field-store.ts @@ -20,6 +20,7 @@ const COLUMNS = [ const T = { contextFields: 'context_fields', featureStrategies: 'feature_strategies', + features: 'features', }; type ContextFieldDB = { @@ -107,6 +108,12 @@ class ContextFieldStore implements IContextFieldStore { WHERE elem ->> 'contextName' = ${T.contextFields}.name )`, ) + .join( + T.features, + `${T.features}.name`, + `${T.featureStrategies}.feature_name`, + ) + .where(`${T.features}.archived_at`, 'IS', null) .groupBy( this.prefixColumns( COLUMNS.filter((column) => column !== 'legal_values'), @@ -144,7 +151,6 @@ class ContextFieldStore implements IContextFieldStore { return present; } - // TODO: write tests for the changes you made here? async create(contextField: IContextFieldDto): Promise { const [row] = await this.db(T.contextFields) .insert(this.fieldToRow(contextField)) From 5ae29c9c18dbedd4655fc29a49bd3e697b70208a Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Fri, 16 May 2025 10:16:05 +0200 Subject: [PATCH 5/8] add some more tests --- src/test/e2e/api/admin/context.e2e.test.ts | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/test/e2e/api/admin/context.e2e.test.ts b/src/test/e2e/api/admin/context.e2e.test.ts index ed38b12aea84..73987e64a1f7 100644 --- a/src/test/e2e/api/admin/context.e2e.test.ts +++ b/src/test/e2e/api/admin/context.e2e.test.ts @@ -329,6 +329,14 @@ test('should show context field usage for active flags', async () => { strategies: [{ environment: 'default', featureName: 'contextFeature' }], }); + const { body: getAllBody } = await app.request + .get(`/api/admin/context`) + .expect(200); + + expect( + getAllBody.find((field) => field.name === context)?.usedInFeatures, + ).toBe(1); + await app.archiveFeature('contextFeature').expect(202); const { body: postArchiveBody } = await app.request.get( @@ -336,4 +344,17 @@ test('should show context field usage for active flags', async () => { ); expect(postArchiveBody.strategies).toHaveLength(0); + + const { body: getContextBody } = await app.request.get( + `/api/admin/context/${context}/strategies`, + ); + + const { body: postArchiveGetAllBody } = await app.request + .get(`/api/admin/context`) + .expect(200); + + expect( + postArchiveGetAllBody.find((field) => field.name === context) + ?.usedInFeatures, + ).toBe(0); }); From fd66fac8f70dc6e73ab766c43adaac9645df5ab5 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Fri, 16 May 2025 11:36:51 +0200 Subject: [PATCH 6/8] fix query --- .../features/context/context-field-store.ts | 30 +++++++++++-------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/src/lib/features/context/context-field-store.ts b/src/lib/features/context/context-field-store.ts index 8b4043b361fe..9ae159e272c4 100644 --- a/src/lib/features/context/context-field-store.ts +++ b/src/lib/features/context/context-field-store.ts @@ -89,17 +89,21 @@ class ContextFieldStore implements IContextFieldStore { async getAll(): Promise { const rows = await this.db - .select( - this.prefixColumns(), - 'used_in_projects', - 'used_in_features', - ) - .countDistinct( - `${T.featureStrategies}.project_name AS used_in_projects`, - ) - .countDistinct( - `${T.featureStrategies}.feature_name AS used_in_features`, - ) + .select([ + ...this.prefixColumns(), + this.db.raw( + `COUNT(DISTINCT CASE + WHEN ${T.features}.archived_at IS NULL + THEN ${T.featureStrategies}.project_name + END) AS used_in_projects`, + ), + this.db.raw( + `COUNT(DISTINCT CASE + WHEN ${T.features}.archived_at IS NULL + THEN ${T.featureStrategies}.feature_name + END) AS used_in_features`, + ), + ]) .from(T.contextFields) .joinRaw( `LEFT JOIN ${T.featureStrategies} ON EXISTS ( @@ -108,18 +112,18 @@ class ContextFieldStore implements IContextFieldStore { WHERE elem ->> 'contextName' = ${T.contextFields}.name )`, ) - .join( + .leftJoin( T.features, `${T.features}.name`, `${T.featureStrategies}.feature_name`, ) - .where(`${T.features}.archived_at`, 'IS', null) .groupBy( this.prefixColumns( COLUMNS.filter((column) => column !== 'legal_values'), ), ) .orderBy('name', 'asc'); + return rows.map(mapRow); } From d6e466df332756af551c54137c23d86df4228962 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Mon, 19 May 2025 11:44:37 +0200 Subject: [PATCH 7/8] use unique name --- src/test/e2e/api/admin/context.e2e.test.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/test/e2e/api/admin/context.e2e.test.ts b/src/test/e2e/api/admin/context.e2e.test.ts index 73987e64a1f7..33934e9cd857 100644 --- a/src/test/e2e/api/admin/context.e2e.test.ts +++ b/src/test/e2e/api/admin/context.e2e.test.ts @@ -1,4 +1,5 @@ import dbInit, { type ITestDb } from '../../helpers/database-init.js'; +import { v4 as uuidv4 } from 'uuid'; import { type IUnleashTest, setupAppWithCustomConfig, @@ -181,7 +182,7 @@ test('should delete context field', async () => { test('should not delete a context field that is in use by active flags', async () => { const context = 'appName'; - const feature = 'contextFeature'; + const feature = uuidv4(); await app.request .post('/api/admin/projects/default/features') .send({ @@ -211,7 +212,7 @@ test('should not delete a context field that is in use by active flags', async ( app.request.delete(`/api/admin/context/${context}`).expect(409); - await app.archiveFeature('contextFeature').expect(202); + await app.archiveFeature(feature).expect(202); const { body: postArchiveBody } = await app.request.get( `/api/admin/context/${context}/strategies`, @@ -337,7 +338,11 @@ test('should show context field usage for active flags', async () => { getAllBody.find((field) => field.name === context)?.usedInFeatures, ).toBe(1); - await app.archiveFeature('contextFeature').expect(202); + await app + .archiveFeature( + 'csrc/test/e2e/api/admin/context.e2e.test.tsontextFeature', + ) + .expect(202); const { body: postArchiveBody } = await app.request.get( `/api/admin/context/${context}/strategies`, From 9e0a7448d4706065cc228054fd9f685cc3e341ca Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Mon, 19 May 2025 12:16:01 +0200 Subject: [PATCH 8/8] minimize changes --- src/test/e2e/api/admin/context.e2e.test.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/test/e2e/api/admin/context.e2e.test.ts b/src/test/e2e/api/admin/context.e2e.test.ts index 33934e9cd857..37a40723681a 100644 --- a/src/test/e2e/api/admin/context.e2e.test.ts +++ b/src/test/e2e/api/admin/context.e2e.test.ts @@ -338,11 +338,7 @@ test('should show context field usage for active flags', async () => { getAllBody.find((field) => field.name === context)?.usedInFeatures, ).toBe(1); - await app - .archiveFeature( - 'csrc/test/e2e/api/admin/context.e2e.test.tsontextFeature', - ) - .expect(202); + await app.archiveFeature('contextFeature').expect(202); const { body: postArchiveBody } = await app.request.get( `/api/admin/context/${context}/strategies`,