From 911496986945db24aa4568e1e334d159d4093501 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Tue, 12 Sep 2023 10:19:40 +0200 Subject: [PATCH] feat: make import/export work with project patterns (#4652) This PR adds feature name pattern validation to the import validation step. When errors occur, they are rendered with all the offending features, the pattern to match, plus the pattern's description and example if available. ![image](https://github.com/Unleash/unleash/assets/17786332/69956090-afc6-41c8-8f6e-fb45dfaf0a9d) To achieve this I've added an extra method to the feature toggle service that checks feature names without throwing errors (because catching `n` async errors in a loop became tricky and hard to grasp). This method is also reused in the existing feature name validation method and handles the feature enabled chcek. In doing so, I've also added tests to check that the pattern is applied. --- .../export-import-service.ts | 14 ++++ .../export-import.e2e.test.ts | 58 +++++++++++++++ .../import-validation-messages.ts | 20 +++++ src/lib/routes/admin-api/feature.ts | 5 +- src/lib/services/feature-toggle-service.ts | 60 ++++++++++++--- src/lib/types/model.ts | 4 +- .../feature-toggle-service-v2.e2e.test.ts | 73 ++++++++++++++++++- 7 files changed, 219 insertions(+), 15 deletions(-) diff --git a/src/lib/features/export-import-toggles/export-import-service.ts b/src/lib/features/export-import-toggles/export-import-service.ts index 674f8bc26a..e43b261b87 100644 --- a/src/lib/features/export-import-toggles/export-import-service.ts +++ b/src/lib/features/export-import-toggles/export-import-service.ts @@ -45,6 +45,7 @@ import { IImportTogglesStore } from './import-toggles-store-type'; import { ImportPermissionsService, Mode } from './import-permissions-service'; import { ImportValidationMessages } from './import-validation-messages'; import { findDuplicates } from '../../util/findDuplicates'; +import { FeatureNameCheckResult } from 'lib/services/feature-toggle-service'; export default class ExportImportService { private logger: Logger; @@ -156,6 +157,7 @@ export default class ExportImportService { existingProjectFeatures, missingPermissions, duplicateFeatures, + featureNameCheckResult, ] = await Promise.all([ this.getUnsupportedStrategies(dto), this.getUsedCustomStrategies(dto), @@ -169,6 +171,7 @@ export default class ExportImportService { mode, ), this.getDuplicateFeatures(dto), + this.getInvalidFeatureNames(dto), ]); const errors = ImportValidationMessages.compileErrors({ @@ -177,6 +180,7 @@ export default class ExportImportService { contextFields: unsupportedContextFields || [], otherProjectFeatures, duplicateFeatures, + featureNameCheckResult, }); const warnings = ImportValidationMessages.compileWarnings({ archivedFeatures, @@ -500,6 +504,16 @@ export default class ExportImportService { } } + private async getInvalidFeatureNames({ + project, + data, + }: ImportTogglesSchema): Promise { + return this.featureToggleService.checkFeatureFlagNamesAgainstProjectPattern( + project, + data.features.map((f) => f.name), + ); + } + private async getUnsupportedStrategies( dto: ImportTogglesSchema, ): Promise { diff --git a/src/lib/features/export-import-toggles/export-import.e2e.test.ts b/src/lib/features/export-import-toggles/export-import.e2e.test.ts index 2c812adfef..5e94c0d786 100644 --- a/src/lib/features/export-import-toggles/export-import.e2e.test.ts +++ b/src/lib/features/export-import-toggles/export-import.e2e.test.ts @@ -144,6 +144,7 @@ beforeAll(async () => { experimental: { flags: { featuresExportImport: true, + featureNamingPattern: true, }, }, }, @@ -833,6 +834,7 @@ test('reject import with duplicate features', async () => { test('validate import data', async () => { await createProjects(); + const contextField: IContextFieldDto = { name: 'validate_context_field', legalValues: [{ value: 'Value1' }], @@ -864,6 +866,16 @@ test('validate import data', async () => { }, }; + // note: this must be done after creating the feature on the earlier lines, + // to prevent the pattern from blocking the creation. + await projectStore.update({ + id: DEFAULT_PROJECT, + name: 'default', + description: '', + mode: 'open', + featureNaming: { pattern: 'testpattern.+' }, + }); + const { body } = await validateImport(importPayloadWithContextFields, 200); expect(body).toMatchObject({ @@ -883,6 +895,11 @@ test('validate import data', async () => { 'We detected the following features are duplicate in your import data:', affectedItems: [defaultFeatureName], }, + + { + message: expect.stringMatching(/\btestpattern.+\b/), + affectedItems: [defaultFeatureName], + }, ], warnings: [ { @@ -941,3 +958,44 @@ test('should not import archived features tags', async () => { tags: resultTags, }); }); + +test(`should give errors with flag names if the flags don't match the project pattern`, async () => { + await db.stores.environmentStore.create({ + name: DEFAULT_ENV, + type: 'production', + }); + + const pattern = 'testpattern.+'; + for (const project of [DEFAULT_PROJECT]) { + await db.stores.projectStore.create({ + name: project, + description: '', + id: project, + mode: 'open' as const, + featureNaming: { pattern }, + }); + await app.linkProjectToEnvironment(project, DEFAULT_ENV); + } + + const flagName = 'unusedfeaturenamethatdoesntmatchpattern'; + + const { body } = await app.importToggles( + { + ...defaultImportPayload, + data: { + ...defaultImportPayload.data, + features: [ + { + project: 'old_project', + name: flagName, + type: 'release', + }, + ], + }, + }, + 400, + ); + + expect(body.message).toContain(pattern); + expect(body.message).toContain(flagName); +}); diff --git a/src/lib/features/export-import-toggles/import-validation-messages.ts b/src/lib/features/export-import-toggles/import-validation-messages.ts index 40308fd40f..e590a349e4 100644 --- a/src/lib/features/export-import-toggles/import-validation-messages.ts +++ b/src/lib/features/export-import-toggles/import-validation-messages.ts @@ -1,3 +1,4 @@ +import { FeatureNameCheckResult } from 'lib/services/feature-toggle-service'; import { FeatureStrategySchema, ImportTogglesValidateItemSchema, @@ -10,6 +11,7 @@ export interface IErrorsParams { contextFields: IContextFieldDto[]; otherProjectFeatures: string[]; duplicateFeatures: string[]; + featureNameCheckResult: FeatureNameCheckResult; } export interface IWarningParams { @@ -40,6 +42,7 @@ export class ImportValidationMessages { contextFields, otherProjectFeatures, duplicateFeatures, + featureNameCheckResult, }: IErrorsParams): ImportTogglesValidateItemSchema[] { const errors: ImportTogglesValidateItemSchema[] = []; @@ -72,6 +75,23 @@ export class ImportValidationMessages { affectedItems: duplicateFeatures, }); } + if (featureNameCheckResult.state === 'invalid') { + const baseError = `Features imported into this project must match the project's feature naming pattern: "${featureNameCheckResult.featureNaming.pattern}".`; + + const exampleInfo = featureNameCheckResult.featureNaming.example + ? ` For example: "${featureNameCheckResult.featureNaming.example}".` + : ''; + + const descriptionInfo = featureNameCheckResult.featureNaming + .description + ? ` The pattern is described as follows: "${featureNameCheckResult.featureNaming.description}"` + : ''; + + errors.push({ + message: `${baseError}${exampleInfo}${descriptionInfo} The following features do not match the pattern:`, + affectedItems: [...featureNameCheckResult.invalidNames].sort(), + }); + } return errors; } diff --git a/src/lib/routes/admin-api/feature.ts b/src/lib/routes/admin-api/feature.ts index 8dc8a6ec80..e83e36f23b 100644 --- a/src/lib/routes/admin-api/feature.ts +++ b/src/lib/routes/admin-api/feature.ts @@ -310,7 +310,10 @@ class FeatureController extends Controller { const { name, projectId } = req.body; await this.service.validateName(name); - await this.service.validateFeatureFlagPattern(name, projectId); + await this.service.validateFeatureFlagNameAgainstPattern( + name, + projectId ?? undefined, + ); res.status(200).end(); } diff --git a/src/lib/services/feature-toggle-service.ts b/src/lib/services/feature-toggle-service.ts index 0c1c30667f..bc0859e168 100644 --- a/src/lib/services/feature-toggle-service.ts +++ b/src/lib/services/feature-toggle-service.ts @@ -43,6 +43,7 @@ import { StrategiesOrderChangedEvent, PotentiallyStaleOnEvent, IStrategyStore, + IFeatureNaming, } from '../types'; import { Logger } from '../logger'; import { PatternError } from '../error'; @@ -111,6 +112,14 @@ export interface IGetFeatureParams { userId?: number; } +export type FeatureNameCheckResult = + | { state: 'valid' } + | { + state: 'invalid'; + invalidNames: Set; + featureNaming: IFeatureNaming; + }; + const oneOf = (values: string[], match: string) => { return values.some((value) => value === match); }; @@ -1035,7 +1044,7 @@ class FeatureToggleService { ): Promise { this.logger.info(`${createdBy} creates feature toggle ${value.name}`); await this.validateName(value.name); - await this.validateFeatureFlagPattern(value.name, projectId); + await this.validateFeatureFlagNameAgainstPattern(value.name, projectId); const exists = await this.projectStore.hasProject(projectId); @@ -1091,20 +1100,49 @@ class FeatureToggleService { throw new NotFoundError(`Project with id ${projectId} does not exist`); } - async validateFeatureFlagPattern( + async checkFeatureFlagNamesAgainstProjectPattern( + projectId: string, + featureNames: string[], + ): Promise { + if (this.flagResolver.isEnabled('featureNamingPattern')) { + const project = await this.projectStore.get(projectId); + const patternData = project.featureNaming; + const namingPattern = patternData?.pattern; + + if (namingPattern) { + const regex = new RegExp(namingPattern); + const mismatchedNames = featureNames.filter( + (name) => !regex.test(name), + ); + + if (mismatchedNames.length > 0) { + return { + state: 'invalid', + invalidNames: new Set(mismatchedNames), + featureNaming: patternData, + }; + } + } + } + return { state: 'valid' }; + } + + async validateFeatureFlagNameAgainstPattern( featureName: string, projectId?: string, ): Promise { - if (this.flagResolver.isEnabled('featureNamingPattern') && projectId) { - const project = await this.projectStore.get(projectId); - const namingPattern = project.featureNaming?.pattern; - const namingExample = project.featureNaming?.example; - const namingDescription = project.featureNaming?.description; + if (projectId) { + const result = + await this.checkFeatureFlagNamesAgainstProjectPattern( + projectId, + [featureName], + ); + + if (result.state === 'invalid') { + const namingPattern = result.featureNaming.pattern; + const namingExample = result.featureNaming.example; + const namingDescription = result.featureNaming.description; - if ( - namingPattern && - !featureName.match(new RegExp(namingPattern)) - ) { const error = `The feature flag name "${featureName}" does not match the project's naming pattern: "${namingPattern}".`; const example = namingExample ? ` Here's an example of a name that does match the pattern: "${namingExample}"."` diff --git a/src/lib/types/model.ts b/src/lib/types/model.ts index 986ce07add..e2773fa6e3 100644 --- a/src/lib/types/model.ts +++ b/src/lib/types/model.ts @@ -191,8 +191,8 @@ export type ProjectMode = 'open' | 'protected'; export interface IFeatureNaming { pattern: string | null; - example: string | null; - description: string | null; + example?: string | null; + description?: string | null; } export interface IProjectOverview { diff --git a/src/test/e2e/services/feature-toggle-service-v2.e2e.test.ts b/src/test/e2e/services/feature-toggle-service-v2.e2e.test.ts index 0e3da6e685..a60f76086e 100644 --- a/src/test/e2e/services/feature-toggle-service-v2.e2e.test.ts +++ b/src/test/e2e/services/feature-toggle-service-v2.e2e.test.ts @@ -33,7 +33,9 @@ const mockConstraints = (): IConstraint[] => { const irrelevantDate = new Date(); beforeAll(async () => { - const config = createTestConfig(); + const config = createTestConfig({ + experimental: { flags: { featureNamingPattern: true } }, + }); db = await dbInit( 'feature_toggle_service_v2_service_serial', config.getLogger, @@ -638,3 +640,72 @@ test('getPlaygroundFeatures should return ids and titles (if they exist) on clie expect(strategy.id).not.toBeUndefined(); } }); + +describe('flag name validation', () => { + test('should validate names if project has flag name pattern', async () => { + const projectId = 'pattern-validation'; + const featureNaming = { + pattern: 'testpattern.+', + example: 'testpattern-one!', + description: 'naming description', + }; + const project = { + id: projectId, + name: projectId, + mode: 'open' as const, + defaultStickiness: 'default', + featureNaming, + }; + + await stores.projectStore.create(project); + + const validFeatures = ['testpattern-feature', 'testpattern-feature2']; + const invalidFeatures = ['a', 'b', 'c']; + const result = await service.checkFeatureFlagNamesAgainstProjectPattern( + projectId, + [...validFeatures, ...invalidFeatures], + ); + + expect(result).toMatchObject({ + state: 'invalid', + invalidNames: new Set(invalidFeatures), + featureNaming: featureNaming, + }); + + const validResult = + await service.checkFeatureFlagNamesAgainstProjectPattern( + projectId, + validFeatures, + ); + + expect(validResult).toMatchObject({ state: 'valid' }); + }); + + test.each([null, undefined, ''])( + 'should not validate names if the pattern is %s', + async (pattern) => { + const projectId = `empty-pattern-validation-${pattern}`; + const featureNaming = { + pattern, + }; + const project = { + id: projectId, + name: projectId, + mode: 'open' as const, + defaultStickiness: 'default', + featureNaming, + }; + + await stores.projectStore.create(project); + + const features = ['a', 'b']; + const result = + await service.checkFeatureFlagNamesAgainstProjectPattern( + projectId, + features, + ); + + expect(result).toMatchObject({ state: 'valid' }); + }, + ); +});