From 9955267d39c917c5b63291d9bf5666295922b423 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Mon, 19 May 2025 13:02:01 +0200 Subject: [PATCH] Chore(1-3753)!: block deletion of context fields in use (#10005) Blocks deletion of context fields that are in use and updates the "active usage" count to exclude use in archived flags. - Before allowing you to delete a context field, checks if it is in use by any strategies. If so, returns a 409 error. - Updates what we count as "in use" to exclude flags that have been archived. BREAKING CHANGE: Context fields can no longer be deleted if they are in use by active (non-archived) flags. --- src/lib/error/conflict-error.ts | 6 ++ src/lib/error/unleash-error.ts | 1 + .../features/context/context-field-store.ts | 34 ++++++--- src/lib/features/context/context-service.ts | 10 +++ .../feature-toggle-strategies-store.ts | 6 ++ src/test/e2e/api/admin/context.e2e.test.ts | 73 ++++++++++++++++++- 6 files changed, 117 insertions(+), 13 deletions(-) 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 0000000000..f59cab9401 --- /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 0136c472d0..2d6ff187c8 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-field-store.ts b/src/lib/features/context/context-field-store.ts index 603b317478..9ae159e272 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 = { @@ -88,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 ( @@ -107,12 +112,18 @@ class ContextFieldStore implements IContextFieldStore { WHERE elem ->> 'contextName' = ${T.contextFields}.name )`, ) + .leftJoin( + T.features, + `${T.features}.name`, + `${T.featureStrategies}.feature_name`, + ) .groupBy( this.prefixColumns( COLUMNS.filter((column) => column !== 'legal_values'), ), ) .orderBy('name', 'asc'); + return rows.map(mapRow); } @@ -144,7 +155,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)) diff --git a/src/lib/features/context/context-service.ts b/src/lib/features/context/context-service.ts index 38adcc1903..57e3ca5109 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/lib/features/feature-toggle/feature-toggle-strategies-store.ts b/src/lib/features/feature-toggle/feature-toggle-strategies-store.ts index 4eb763e32a..6303b9487f 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' = ?)", diff --git a/src/test/e2e/api/admin/context.e2e.test.ts b/src/test/e2e/api/admin/context.e2e.test.ts index a4749d8f40..37a4072368 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, @@ -179,6 +180,47 @@ 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 by active flags', async () => { + const context = 'appName'; + const feature = uuidv4(); + 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); + + app.request.delete(`/api/admin/context/${context}`).expect(409); + + await app.archiveFeature(feature).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 () => { expect.assertions(0); return app.request @@ -241,7 +283,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 @@ -287,4 +329,33 @@ test('should show context field usage', async () => { expect(body).toMatchObject({ 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( + `/api/admin/context/${context}/strategies`, + ); + + 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); });