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' }); + }, + ); +});