From 082a03afd735012883b0b77dcd183c8b1cb3842b Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Thu, 15 May 2025 13:08:54 +0200 Subject: [PATCH] Fix(1-3485)/handle deleted constraints (#9999) Improves handling of constraints in use that have been deleted. This change implments a few small changes on both the front and the back end on how we deal with constraints that have been deleted. The most important change is on the back end, in the `/constraints/validate` endpoint. We used to throw here if the constraint couldn't be found, but the only reason we wanted to look for the constraint in the db was to check for legal values. Now, instead, we'll allow you to pass a constraint field that doesn't exist in the database. We'll still check the values against the operator for validity, we just don't control legal values anymore (because there aren't any). On the front end, we improve the handling by showing the deleted context filed in the dropdown, both when the selector dropdown is closed and when it is open. However, if you change the context field, we remove the deleted field from the list. This seems like a sensible tradeoff. Means you can't select it if you've deselected it. --- .../EditableConstraint/EditableConstraint.tsx | 18 +++++++--- src/lib/features/context/context.ts | 2 +- .../feature-toggle/feature-toggle-service.ts | 36 ++++++++++--------- .../e2e/api/admin/constraints.e2e.test.ts | 22 ++++++++++++ 4 files changed, 56 insertions(+), 22 deletions(-) diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/EditableConstraint.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/EditableConstraint.tsx index 937dc0a034..f70a2cf7f4 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/EditableConstraint.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/EditableConstraint.tsx @@ -258,9 +258,19 @@ export const EditableConstraint: FC = ({ return null; } - const constraintNameOptions = context.map((context) => { - return { key: context.name, label: context.name }; - }); + const extantContextFieldNames = context.map((context) => context.name); + const contextFieldHasBeenDeleted = !extantContextFieldNames.includes( + localConstraint.contextName, + ); + + const availableContextFieldNames = contextFieldHasBeenDeleted + ? [...extantContextFieldNames, localConstraint.contextName].toSorted() + : extantContextFieldNames; + + const contextFieldOptions = availableContextFieldNames.map((option) => ({ + key: option, + label: option, + })); return ( @@ -272,7 +282,7 @@ export const EditableConstraint: FC = ({ id='context-field-select' name='contextName' label='Context Field' - options={constraintNameOptions} + options={contextFieldOptions} value={contextName || ''} onChange={(contextField) => updateConstraint({ diff --git a/src/lib/features/context/context.ts b/src/lib/features/context/context.ts index b33d7e71ee..2d719504e1 100644 --- a/src/lib/features/context/context.ts +++ b/src/lib/features/context/context.ts @@ -245,7 +245,7 @@ export class ContextController extends Controller { tags: ['Context'], summary: 'Validate a context field', description: - 'Check whether the provided data can be used to create a context field. If the data is not valid, ...?', + 'Check whether the provided data can be used to create a context field. If the data is not valid, returns a 400 status code with the reason why it is not valid.', operationId: 'validate', requestBody: createRequestSchema('nameSchema'), responses: { diff --git a/src/lib/features/feature-toggle/feature-toggle-service.ts b/src/lib/features/feature-toggle/feature-toggle-service.ts index 350c46d3f6..7d4c483c66 100644 --- a/src/lib/features/feature-toggle/feature-toggle-service.ts +++ b/src/lib/features/feature-toggle/feature-toggle-service.ts @@ -497,10 +497,6 @@ export class FeatureToggleService { async validateConstraint(input: IConstraint): Promise { const constraint = await constraintSchema.validateAsync(input); const { operator } = constraint; - const contextDefinition = await this.contextFieldStore.get( - constraint.contextName, - ); - if (oneOf(NUM_OPERATORS, operator)) { await validateNumber(constraint.value); } @@ -519,20 +515,26 @@ export class FeatureToggleService { await validateDate(constraint.value); } - if ( - contextDefinition?.legalValues && - contextDefinition.legalValues.length > 0 - ) { - const valuesToValidate = oneOf( - [...DATE_OPERATORS, ...SEMVER_OPERATORS, ...NUM_OPERATORS], - operator, - ) - ? constraint.value - : constraint.values; - validateLegalValues( - contextDefinition.legalValues, - valuesToValidate, + if (await this.contextFieldStore.exists(constraint.contextName)) { + const contextDefinition = await this.contextFieldStore.get( + constraint.contextName, ); + + if ( + contextDefinition?.legalValues && + contextDefinition.legalValues.length > 0 + ) { + const valuesToValidate = oneOf( + [...DATE_OPERATORS, ...SEMVER_OPERATORS, ...NUM_OPERATORS], + operator, + ) + ? constraint.value + : constraint.values; + validateLegalValues( + contextDefinition.legalValues, + valuesToValidate, + ); + } } return constraint; diff --git a/src/test/e2e/api/admin/constraints.e2e.test.ts b/src/test/e2e/api/admin/constraints.e2e.test.ts index bf7c398cac..2e337dec73 100644 --- a/src/test/e2e/api/admin/constraints.e2e.test.ts +++ b/src/test/e2e/api/admin/constraints.e2e.test.ts @@ -43,3 +43,25 @@ test('should accept valid constraints', async () => { .send({ contextName: 'environment', operator: 'IN', values: ['a'] }) .expect(204); }); + +test('should allow unknown constraints if their values are valid', async () => { + await app.request + .post(PATH) + .send({ + contextName: 'not-a-default-context-value', + operator: 'NUM_EQ', + value: 1, + }) + .expect(204); +}); + +test('should block unknown constraints if their values are invalid', async () => { + await app.request + .post(PATH) + .send({ + contextName: 'not-a-default-context-value', + operator: 'IN', + value: 1, + }) + .expect(400); +});