Skip to content

Chore(1-3753)!: block deletion of context fields in use #10005

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
May 19, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/lib/error/conflict-error.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import { UnleashError } from './unleash-error.js';

class ConflictError extends UnleashError {
statusCode = 409;
}
export default ConflictError;
1 change: 1 addition & 0 deletions src/lib/error/unleash-error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import type { FromSchema } from 'json-schema-to-ts';

export const UnleashApiErrorTypes = [
'ContentTypeError',
'ConflictErrror',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'ConflictErrror',
'ConflictError',

'DisabledError',
'FeatureHasTagError',
'IncompatibleProjectError',
Expand Down
34 changes: 22 additions & 12 deletions src/lib/features/context/context-field-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ const COLUMNS = [
const T = {
contextFields: 'context_fields',
featureStrategies: 'feature_strategies',
features: 'features',
};

type ContextFieldDB = {
Expand Down Expand Up @@ -88,17 +89,21 @@ class ContextFieldStore implements IContextFieldStore {

async getAll(): Promise<IContextField[]> {
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 (
Expand All @@ -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);
}

Expand Down Expand Up @@ -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))
Expand Down
10 changes: 10 additions & 0 deletions src/lib/features/context/context-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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' = ?)",
Expand Down
73 changes: 72 additions & 1 deletion src/test/e2e/api/admin/context.e2e.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import dbInit, { type ITestDb } from '../../helpers/database-init.js';
import { v4 as uuidv4 } from 'uuid';
import {
type IUnleashTest,
setupAppWithCustomConfig,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
});