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

@@ -0,0 +1,6 @@
+import { UnleashError } from './unleash-error.js';
+
+class ConflictError extends UnleashError {
+    statusCode = 409;
+}
+export default ConflictError;

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',

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<IContextField[]> {
         const rows = await this.db
-            .select(
+            .select([
-                this.prefixColumns(),
+                ...this.prefixColumns(),
-                'used_in_projects',
+                this.db.raw(
-                'used_in_features',
+                    `COUNT(DISTINCT CASE
-            )
+                        WHEN ${T.features}.archived_at IS NULL
-            .countDistinct(
+                        THEN ${T.featureStrategies}.project_name
-                `${T.featureStrategies}.project_name AS used_in_projects`,
+                    END) AS used_in_projects`,
-            )
+                ),
-            .countDistinct(
+                this.db.raw(
-                `${T.featureStrategies}.feature_name AS used_in_features`,
+                    `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<IContextField> {
         const [row] = await this.db(T.contextFields)
             .insert(this.fieldToRow(contextField))

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<void> {
         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({

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<IFeatureStrategiesTable>(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' = ?)",

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);
 });