feat: add implicit surrounding ^ and $ to patterns (#4664)

This PR updates the back-end handling of feature naming patterns to add
implicit leading `^`s and trailing `$`s to the regexes when comparing
them.

It also adds tests for the new behavior, both for new flag names and for
examples.

## Discussion points

Regarding stripping incoming ^ and $: We don't actually need to strip
incoming `^`s and `$`s: it appears that `^^^^^x$$$$$` is just as valid
as `^x$`. As such, we can leave that in. However, if we think it's
better to strip, we can do that too.

Second, I'm considering moving the flag naming validation into a
dedicated module to encapsulate everything a little better. Not sure if
this is the time or where it would live, but open to hearing
suggestions.

src/lib/features/export-import-toggles/export-import-service.ts

@@ -48,7 +48,7 @@ import {
 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';
+import { FeatureNameCheckResultWithFeaturePattern } from '../../services/feature-toggle-service';
 
 export default class ExportImportService {
     private logger: Logger;
@@ -510,7 +510,7 @@ export default class ExportImportService {
     private async getInvalidFeatureNames({
         project,
         data,
-    }: ImportTogglesSchema): Promise<FeatureNameCheckResult> {
+    }: ImportTogglesSchema): Promise<FeatureNameCheckResultWithFeaturePattern> {
         return this.featureToggleService.checkFeatureFlagNamesAgainstProjectPattern(
             project,
             data.features.map((f) => f.name),

src/lib/features/export-import-toggles/import-validation-messages.ts

@@ -1,9 +1,9 @@
-import { FeatureNameCheckResult } from 'lib/services/feature-toggle-service';
 import {
     FeatureStrategySchema,
     ImportTogglesValidateItemSchema,
 } from '../../openapi';
 import { IContextFieldDto } from '../../types/stores/context-field-store';
+import { FeatureNameCheckResultWithFeaturePattern } from '../../services/feature-toggle-service';
 import { ProjectFeaturesLimit } from './import-toggles-store-type';
 
 export interface IErrorsParams {
@@ -12,7 +12,7 @@ export interface IErrorsParams {
     contextFields: IContextFieldDto[];
     otherProjectFeatures: string[];
     duplicateFeatures: string[];
-    featureNameCheckResult: FeatureNameCheckResult;
+    featureNameCheckResult: FeatureNameCheckResultWithFeaturePattern;
     featureLimitResult: ProjectFeaturesLimit;
 }
 

src/lib/features/feature-naming-pattern/feature-naming-validation.test.ts

@@ -0,0 +1,126 @@
+import {
+    checkFeatureFlagNamesAgainstPattern,
+    checkFeatureNamingData,
+} from './feature-naming-validation';
+
+describe('validate incoming feature naming data', () => {
+    test('patterns with leading ^ and trailing $ are treated the same as without', () => {
+        const basePattern = '[a-z]+';
+        const leading = `^${basePattern}`;
+        const trailing = `${basePattern}$`;
+        const leadingAndTrailing = `^${basePattern}$`;
+
+        const allPatterns = [
+            basePattern,
+            leading,
+            trailing,
+            leadingAndTrailing,
+        ];
+
+        const invalidExamples = ['-name-', 'name-', '-name'];
+        const validExample = 'justalpha';
+
+        for (const pattern of allPatterns) {
+            for (const example of invalidExamples) {
+                expect(
+                    checkFeatureNamingData({ pattern, example }),
+                ).toMatchObject({
+                    state: 'invalid',
+                });
+            }
+            const validData = {
+                pattern,
+                example: validExample,
+            };
+            expect(checkFeatureNamingData(validData)).toMatchObject({
+                state: 'valid',
+            });
+        }
+    });
+
+    test('pattern examples are tested against the pattern as if it were surrounded by ^ and $', () => {
+        const pattern = '-[0-9]+';
+        const validExample = '-23';
+        const invalidExample1 = 'feat-15';
+        const invalidExample2 = '-15-';
+
+        expect(
+            checkFeatureNamingData({
+                pattern,
+                example: validExample,
+            }),
+        ).toMatchObject({ state: 'valid' });
+
+        for (const example of [invalidExample1, invalidExample2]) {
+            expect(
+                checkFeatureNamingData({
+                    pattern,
+                    example,
+                }),
+            ).toMatchObject({ state: 'invalid' });
+        }
+    });
+});
+
+describe('validate feature flag names against a pattern', () => {
+    test('should validate names against a pattern', async () => {
+        const featureNaming = {
+            pattern: 'testpattern.+',
+            example: 'testpattern-one!',
+            description: 'naming description',
+        };
+
+        const validFeatures = ['testpattern-feature', 'testpattern-feature2'];
+        const invalidFeatures = ['a', 'b', 'c'];
+        const result = checkFeatureFlagNamesAgainstPattern(
+            [...validFeatures, ...invalidFeatures],
+            featureNaming.pattern,
+        );
+
+        expect(result).toMatchObject({
+            state: 'invalid',
+            invalidNames: new Set(invalidFeatures),
+        });
+
+        const validResult = checkFeatureFlagNamesAgainstPattern(
+            validFeatures,
+            featureNaming.pattern,
+        );
+
+        expect(validResult).toMatchObject({ state: 'valid' });
+    });
+
+    test.each([null, undefined, ''])(
+        'should not validate names if the pattern is %s',
+        (pattern) => {
+            const featureNaming = {
+                pattern,
+            };
+            const features = ['a', 'b'];
+            const result = checkFeatureFlagNamesAgainstPattern(
+                features,
+                featureNaming.pattern,
+            );
+
+            expect(result).toMatchObject({ state: 'valid' });
+        },
+    );
+
+    test('should validate names as if the pattern is surrounded by ^ and $.', async () => {
+        const pattern = '-[0-9]+';
+        const featureNaming = {
+            pattern,
+        };
+
+        const features = ['a-95', '-95-', 'b-52-z'];
+        const result = checkFeatureFlagNamesAgainstPattern(
+            features,
+            featureNaming.pattern,
+        );
+
+        expect(result).toMatchObject({
+            state: 'invalid',
+            invalidNames: new Set(features),
+        });
+    });
+});

src/lib/features/feature-naming-pattern/feature-naming-validation.ts

@@ -0,0 +1,57 @@
+import { IFeatureNaming } from '../../types/model';
+
+const compileRegex = (pattern: string) => new RegExp(`^${pattern}$`);
+
+export const checkFeatureNamingData = (
+    featureNaming?: IFeatureNaming,
+): { state: 'valid' } | { state: 'invalid'; reason: string } => {
+    if (featureNaming) {
+        const { pattern, example } = featureNaming;
+        if (
+            pattern != null &&
+            example &&
+            !example.match(compileRegex(pattern))
+        ) {
+            return {
+                state: 'invalid',
+                reason: `You've provided a feature flag naming example ("${example}") that doesn't match your feature flag naming pattern ("${pattern}"). Please provide an example that matches your supplied pattern. Bear in mind that the pattern must match the whole example, as if it were surrounded by '^' and "$".`,
+            };
+        }
+
+        if (!pattern && example) {
+            return {
+                state: 'invalid',
+                reason: "You've provided a feature flag naming example, but no feature flag naming pattern. You must specify a pattern to use an example.",
+            };
+        }
+    }
+
+    return { state: 'valid' };
+};
+
+export type FeatureNameCheckResult =
+    | { state: 'valid' }
+    | {
+          state: 'invalid';
+          invalidNames: Set<string>;
+      };
+
+export const checkFeatureFlagNamesAgainstPattern = (
+    featureNames: string[],
+    pattern: string | null | undefined,
+): FeatureNameCheckResult => {
+    if (pattern) {
+        const regex = compileRegex(pattern);
+        const mismatchedNames = featureNames.filter(
+            (name) => !regex.test(name),
+        );
+
+        if (mismatchedNames.length > 0) {
+            return {
+                state: 'invalid',
+                invalidNames: new Set(mismatchedNames),
+            };
+        }
+    }
+    return { state: 'valid' };
+};

src/lib/services/feature-toggle-service.ts

@@ -94,6 +94,7 @@ import { IFeatureProjectUserParams } from '../routes/admin-api/project/project-f
 import { unique } from '../util/unique';
 import { ISegmentService } from 'lib/segments/segment-service-interface';
 import { IChangeRequestAccessReadModel } from '../features/change-request-access-service/change-request-access-read-model';
+import { checkFeatureFlagNamesAgainstPattern } from '../features/feature-naming-pattern/feature-naming-validation';
 
 interface IFeatureContext {
     featureName: string;
@@ -112,7 +113,7 @@ export interface IGetFeatureParams {
     userId?: number;
 }
 
-export type FeatureNameCheckResult =
+export type FeatureNameCheckResultWithFeaturePattern =
     | { state: 'valid' }
     | {
           state: 'invalid';
@@ -1103,24 +1104,20 @@ class FeatureToggleService {
     async checkFeatureFlagNamesAgainstProjectPattern(
         projectId: string,
         featureNames: string[],
-    ): Promise<FeatureNameCheckResult> {
+    ): Promise<FeatureNameCheckResultWithFeaturePattern> {
         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 result = checkFeatureFlagNamesAgainstPattern(
-                const mismatchedNames = featureNames.filter(
+                    featureNames,
-                    (name) => !regex.test(name),
+                    namingPattern,
                 );
 
-                if (mismatchedNames.length > 0) {
+                if (result.state === 'invalid') {
-                    return {
+                    return { ...result, featureNaming: patternData };
-                        state: 'invalid',
-                        invalidNames: new Set(mismatchedNames),
-                        featureNaming: patternData,
-                    };
                 }
             }
         }

src/lib/services/project-service.ts

@@ -59,6 +59,7 @@ import { IProjectStatsStore } from 'lib/types/stores/project-stats-store-type';
 import { uniqueByKey } from '../util/unique';
 import { BadDataError, PermissionError } from '../error';
 import { ProjectDoraMetricsSchema } from 'lib/openapi';
+import { checkFeatureNamingData } from '../features/feature-naming-pattern/feature-naming-validation';
 
 const getCreatedBy = (user: IUser) => user.email || user.username || 'unknown';
 
@@ -169,25 +170,23 @@ export default class ProjectService {
         return this.store.get(id);
     }
 
-    private validateFlagNaming = (naming?: IFeatureNaming) => {
+    private validateAndProcessFeatureNamingPattern = (
-        if (naming) {
+        featureNaming: IFeatureNaming,
-            const { pattern, example } = naming;
+    ): IFeatureNaming => {
-            if (
+        const validationResult = checkFeatureNamingData(featureNaming);
-                pattern != null &&
+
-                example &&
+        if (validationResult.state === 'invalid') {
-                !example.match(new RegExp(pattern))
+            throw new BadDataError(validationResult.reason);
-            ) {
-                throw new BadDataError(
-                    `You've provided a feature flag naming example ("${example}") that doesn't match your feature flag naming pattern ("${pattern}"). Please provide an example that matches your supplied pattern.`,
-                );
         }
 
-            if (!pattern && example) {
+        if (featureNaming.pattern && !featureNaming.example) {
-                throw new BadDataError(
+            featureNaming.example = null;
-                    "You've provided a feature flag naming example, but no feature flag naming pattern. You must specify a pattern to use an example.",
-                );
         }
+        if (featureNaming.pattern && !featureNaming.description) {
+            featureNaming.description = null;
         }
+
+        return featureNaming;
     };
 
     async createProject(
@@ -197,7 +196,9 @@ export default class ProjectService {
         const data = await projectSchema.validateAsync(newProject);
         await this.validateUniqueId(data.id);
 
-        this.validateFlagNaming(data.featureNaming);
+        if (data.featureNaming) {
+            this.validateAndProcessFeatureNamingPattern(data.featureNaming);
+        }
 
         await this.store.create(data);
 
@@ -231,13 +232,9 @@ export default class ProjectService {
         const preData = await this.store.get(updatedProject.id);
 
         if (updatedProject.featureNaming) {
-            this.validateFlagNaming(updatedProject.featureNaming);
+            this.validateAndProcessFeatureNamingPattern(
-        }
+                updatedProject.featureNaming,
-        if (
+            );
-            updatedProject.featureNaming?.pattern &&
-            !updatedProject.featureNaming?.example
-        ) {
-            updatedProject.featureNaming.example = null;
         }
 
         await this.store.update(updatedProject);

src/test/e2e/services/feature-toggle-service-v2.e2e.test.ts

@@ -11,7 +11,11 @@ import { FeatureStrategySchema } from '../../../lib/openapi';
 import User from '../../../lib/types/user';
 import { IConstraint, IVariant, SKIP_CHANGE_REQUEST } from '../../../lib/types';
 import EnvironmentService from '../../../lib/services/environment-service';
-import { ForbiddenError, PermissionError } from '../../../lib/error';
+import {
+    ForbiddenError,
+    PatternError,
+    PermissionError,
+} from '../../../lib/error';
 import { ISegmentService } from '../../../lib/segments/segment-service-interface';
 import { ChangeRequestAccessReadModel } from '../../../lib/features/change-request-access-service/sql-change-request-access-read-model';
 
@@ -642,7 +646,7 @@ test('getPlaygroundFeatures should return ids and titles (if they exist) on clie
 });
 
 describe('flag name validation', () => {
-    test('should validate names if project has flag name pattern', async () => {
+    test('should validate feature names if the project has flag name pattern', async () => {
         const projectId = 'pattern-validation';
         const featureNaming = {
             pattern: 'testpattern.+',
@@ -661,51 +665,23 @@ describe('flag name validation', () => {
 
         const validFeatures = ['testpattern-feature', 'testpattern-feature2'];
         const invalidFeatures = ['a', 'b', 'c'];
-        const result = await service.checkFeatureFlagNamesAgainstProjectPattern(
+
+        for (const feature of invalidFeatures) {
+            await expect(
+                service.validateFeatureFlagNameAgainstPattern(
+                    feature,
                     projectId,
-            [...validFeatures, ...invalidFeatures],
+                ),
-        );
+            ).rejects.toBeInstanceOf(PatternError);
+        }
 
-        expect(result).toMatchObject({
+        for (const feature of validFeatures) {
-            state: 'invalid',
+            await expect(
-            invalidNames: new Set(invalidFeatures),
+                service.validateFeatureFlagNameAgainstPattern(
-            featureNaming: featureNaming,
+                    feature,
-        });
-
-        const validResult =
-            await service.checkFeatureFlagNamesAgainstProjectPattern(
                     projectId,
-                validFeatures,
+                ),
-            );
+            ).resolves.toBeFalsy();
-
+        }
-        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' });
-        },
-    );
 });