refactor: avoid inlining segments for supported clients (#1640)

* refactor: add semver lib types

* refactor: avoid inlining segments for supported clients

* refactor: fix FeatureController tests

* refactor: use spec version instead of client version

* refactor: improve header validation errors

package.json

@@ -76,6 +76,7 @@
     ]
   },
   "dependencies": {
+    "@unleash/express-openapi": "^0.2.0",
     "async": "^3.2.3",
     "bcryptjs": "^2.4.3",
     "compression": "^1.7.4",
@@ -96,8 +97,8 @@
     "helmet": "^5.0.0",
     "joi": "^17.3.0",
     "js-yaml": "^4.1.0",
-    "knex": "^2.0.0",
     "json-schema-to-ts": "^2.0.0",
+    "knex": "^2.0.0",
     "log4js": "^6.0.0",
     "make-fetch-happen": "^10.1.2",
     "memoizee": "^0.4.15",
@@ -113,13 +114,12 @@
     "pkginfo": "^0.4.1",
     "prom-client": "^14.0.0",
     "response-time": "^2.3.2",
+    "semver": "^7.3.5",
     "serve-favicon": "^2.5.0",
     "stoppable": "^1.1.0",
     "type-is": "^1.6.18",
-    "@unleash/express-openapi": "^0.2.0",
     "unleash-frontend": "4.12.3",
-    "uuid": "^8.3.2",
+    "uuid": "^8.3.2"
-    "semver": "^7.3.5"
   },
   "devDependencies": {
     "@babel/core": "7.18.2",
@@ -135,6 +135,7 @@
     "@types/node": "16.6.1",
     "@types/nodemailer": "6.4.4",
     "@types/owasp-password-strength-test": "1.3.0",
+    "@types/semver": "^7.3.9",
     "@types/stoppable": "1.1.1",
     "@types/supertest": "2.0.12",
     "@types/type-is": "1.6.3",

src/lib/db/feature-toggle-client-store.ts

@@ -139,9 +139,12 @@ export default class FeatureToggleClientStore
                     FeatureToggleClientStore.rowToStrategy(r),
                 );
             }
-            if (this.inlineSegmentConstraints && r.segment_id) {
+            if (featureQuery?.inlineSegmentConstraints && r.segment_id) {
                 this.addSegmentToStrategy(feature, r);
-            } else if (!this.inlineSegmentConstraints && r.segment_id) {
+            } else if (
+                !featureQuery?.inlineSegmentConstraints &&
+                r.segment_id
+            ) {
                 this.addSegmentIdsToStrategy(feature, r);
             }
             feature.impressionData = r.impression_data;

src/lib/routes/client-api/feature.test.ts

@@ -6,6 +6,7 @@ import { createServices } from '../../services';
 import FeatureController from './feature';
 import { createTestConfig } from '../../../test/config/test-config';
 import { secondsToMilliseconds } from 'date-fns';
+import { ClientSpecService } from '../../services/client-spec-service';
 
 async function getSetup() {
     const base = `/random${Math.round(Math.random() * 1000)}`;
@@ -30,6 +31,14 @@ async function getSetup() {
     };
 }
 
+const callGetAll = async (controller: FeatureController) => {
+    await controller.getAll(
+        // @ts-expect-error
+        { query: {}, header: () => undefined },
+        { json: () => {} },
+    );
+};
+
 let base;
 let request;
 let destroy;
@@ -61,18 +70,18 @@ test('should get empty getFeatures via client', () => {
 test('if caching is enabled should memoize', async () => {
     const getClientFeatures = jest.fn().mockReturnValue([]);
     const getActive = jest.fn().mockReturnValue([]);
-
+    const clientSpecService = new ClientSpecService({ getLogger });
-    const featureToggleServiceV2 = {
+    const featureToggleServiceV2 = { getClientFeatures };
-        getClientFeatures,
+    const segmentService = { getActive };
-    };
-
-    const segmentService = {
-        getActive,
-    };
 
     const controller = new FeatureController(
-        // @ts-ignore
+        {
-        { featureToggleServiceV2, segmentService },
+            clientSpecService,
+            // @ts-expect-error
+            featureToggleServiceV2,
+            // @ts-expect-error
+            segmentService,
+        },
         {
             getLogger,
             experimental: {
@@ -83,29 +92,27 @@ test('if caching is enabled should memoize', async () => {
             },
         },
     );
-    // @ts-ignore
+
-    await controller.getAll({ query: {} }, { json: () => {} });
+    await callGetAll(controller);
-    // @ts-ignore
+    await callGetAll(controller);
-    await controller.getAll({ query: {} }, { json: () => {} });
     expect(getClientFeatures).toHaveBeenCalledTimes(1);
 });
 
 test('if caching is not enabled all calls goes to service', async () => {
     const getClientFeatures = jest.fn().mockReturnValue([]);
-
     const getActive = jest.fn().mockReturnValue([]);
-
+    const clientSpecService = new ClientSpecService({ getLogger });
-    const featureToggleServiceV2 = {
+    const featureToggleServiceV2 = { getClientFeatures };
-        getClientFeatures,
+    const segmentService = { getActive };
-    };
-
-    const segmentService = {
-        getActive,
-    };
 
     const controller = new FeatureController(
-        // @ts-ignore
+        {
-        { featureToggleServiceV2, segmentService },
+            clientSpecService,
+            // @ts-expect-error
+            featureToggleServiceV2,
+            // @ts-expect-error
+            segmentService,
+        },
         {
             getLogger,
             experimental: {
@@ -116,10 +123,9 @@ test('if caching is not enabled all calls goes to service', async () => {
             },
         },
     );
-    // @ts-ignore
+
-    await controller.getAll({ query: {} }, { json: () => {} });
+    await callGetAll(controller);
-    // @ts-ignore
+    await callGetAll(controller);
-    await controller.getAll({ query: {} }, { json: () => {} });
     expect(getClientFeatures).toHaveBeenCalledTimes(2);
 });
 

src/lib/routes/client-api/feature.ts

@@ -13,6 +13,7 @@ import ApiUser from '../../types/api-user';
 import { ALL, isAllProjects } from '../../types/models/api-token';
 import { SegmentService } from '../../services/segment-service';
 import { FeatureConfigurationClient } from '../../types/stores/feature-strategies-store';
+import { ClientSpecService } from '../../services/client-spec-service';
 
 const version = 2;
 
@@ -28,25 +29,29 @@ export default class FeatureController extends Controller {
 
     private segmentService: SegmentService;
 
+    private clientSpecService: ClientSpecService;
+
     private readonly cache: boolean;
 
     private cachedFeatures: any;
 
-    private useGlobalSegments: boolean;
-
     constructor(
         {
             featureToggleServiceV2,
             segmentService,
-        }: Pick<IUnleashServices, 'featureToggleServiceV2' | 'segmentService'>,
+            clientSpecService,
+        }: Pick<
+            IUnleashServices,
+            'featureToggleServiceV2' | 'segmentService' | 'clientSpecService'
+        >,
         config: IUnleashConfig,
     ) {
         super(config);
         const { experimental } = config;
         this.featureToggleServiceV2 = featureToggleServiceV2;
         this.segmentService = segmentService;
+        this.clientSpecService = clientSpecService;
         this.logger = config.getLogger('client-api/feature.js');
-        this.useGlobalSegments = !this.config.inlineSegmentConstraints;
 
         this.get('/', this.getAll);
         this.get('/:featureName', this.getFeatureToggle);
@@ -69,20 +74,12 @@ export default class FeatureController extends Controller {
         }
     }
 
-    private async resolveSegments() {
-        if (this.useGlobalSegments) {
-            return this.segmentService.getActive();
-        }
-        return Promise.resolve([]);
-    }
-
     private async resolveFeaturesAndSegments(
         query?: IFeatureToggleQuery,
     ): Promise<[FeatureConfigurationClient[], ISegment[]]> {
-        let segments = this.resolveSegments();
         return Promise.all([
             this.featureToggleServiceV2.getClientFeatures(query),
-            segments,
+            this.segmentService.getActive(),
         ]);
     }
 
@@ -101,8 +98,14 @@ export default class FeatureController extends Controller {
             }
         }
 
-        const q = { ...query, ...override };
+        const inlineSegmentConstraints =
-        return this.prepQuery(q);
+            !this.clientSpecService.requestSupportsSpec(req, 'segments');
+
+        return this.prepQuery({
+            ...query,
+            ...override,
+            inlineSegmentConstraints,
+        });
     }
 
     // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
@@ -118,10 +121,18 @@ export default class FeatureController extends Controller {
         project,
         namePrefix,
         environment,
+        inlineSegmentConstraints,
     }: IFeatureToggleQuery): Promise<IFeatureToggleQuery> {
-        if (!tag && !project && !namePrefix && !environment) {
+        if (
+            !tag &&
+            !project &&
+            !namePrefix &&
+            !environment &&
+            !inlineSegmentConstraints
+        ) {
             return null;
         }
+
         const tagQuery = this.paramToArray(tag);
         const projectQuery = this.paramToArray(project);
         const query = await querySchema.validateAsync({
@@ -129,38 +140,30 @@ export default class FeatureController extends Controller {
             project: projectQuery,
             namePrefix,
             environment,
+            inlineSegmentConstraints,
         });
+
         if (query.tag) {
             query.tag = query.tag.map((q) => q.split(':'));
         }
+
         return query;
     }
 
     async getAll(req: IAuthRequest, res: Response): Promise<void> {
-        const featureQuery = await this.resolveQuery(req);
+        const query = await this.resolveQuery(req);
-        let features, segments;
-        if (this.cache) {
-            [features, segments] = await this.cachedFeatures(featureQuery);
-        } else {
-            features = await this.featureToggleServiceV2.getClientFeatures(
-                featureQuery,
-            );
-            segments = await this.resolveSegments();
-        }
 
-        const response = {
+        const [features, segments] = this.cache
-            version,
+            ? await this.cachedFeatures(query)
-            features,
+            : await Promise.all([
-            query: featureQuery,
+                  this.featureToggleServiceV2.getClientFeatures(query),
-        };
+                  this.segmentService.getActive(),
+              ]);
 
-        if (this.useGlobalSegments) {
+        if (this.clientSpecService.requestSupportsSpec(req, 'segments')) {
-            res.json({
+            res.json({ version, features, query, segments });
-                ...response,
-                segments,
-            });
         } else {
-            res.json(response);
+            res.json({ version, features, query });
         }
     }
 

src/lib/schema/feature-schema.ts

@@ -116,6 +116,7 @@ export const querySchema = joi
         project: joi.array().allow(null).items(nameType).optional(),
         namePrefix: joi.string().allow(null).optional(),
         environment: joi.string().allow(null).optional(),
+        inlineSegmentConstraints: joi.boolean().optional(),
     })
     .options({ allowUnknown: false, stripUnknown: true, abortEarly: false });
 

src/lib/services/client-spec-service.test.ts

@@ -0,0 +1,29 @@
+import { ClientSpecService } from './client-spec-service';
+import getLogger from '../../test/fixtures/no-logger';
+
+test('ClientSpecService validation', async () => {
+    const service = new ClientSpecService({ getLogger });
+    const fn = service.versionSupportsSpec.bind(service);
+
+    expect(fn('segments', undefined)).toEqual(false);
+    expect(fn('segments', '')).toEqual(false);
+
+    expect(() => fn('segments', 'a')).toThrow('Invalid prefix');
+    expect(() => fn('segments', '1.2')).toThrow('Invalid SemVer');
+    expect(() => fn('segments', 'v1.2.3')).toThrow('Invalid prefix');
+    expect(() => fn('segments', '=1.2.3')).toThrow('Invalid prefix');
+    expect(() => fn('segments', '1.2.3.4')).toThrow('Invalid SemVer');
+});
+
+test('ClientSpecService segments', async () => {
+    const service = new ClientSpecService({ getLogger });
+    const fn = service.versionSupportsSpec.bind(service);
+
+    expect(fn('segments', '0.0.0')).toEqual(false);
+    expect(fn('segments', '1.0.0')).toEqual(false);
+    expect(fn('segments', '4.1.9')).toEqual(false);
+
+    expect(fn('segments', '4.2.0')).toEqual(true);
+    expect(fn('segments', '4.2.1')).toEqual(true);
+    expect(fn('segments', '5.0.0')).toEqual(true);
+});

src/lib/services/client-spec-service.ts

@@ -0,0 +1,54 @@
+import { IUnleashConfig } from '../types/option';
+import { Logger } from '../logger';
+import { Request } from 'express';
+import semver, { SemVer } from 'semver';
+import BadDataError from '../error/bad-data-error';
+import { mustParseStrictSemVer, parseStrictSemVer } from '../util/semver';
+
+export type ClientSpecFeature = 'segments';
+
+export class ClientSpecService {
+    private readonly logger: Logger;
+
+    private readonly clientSpecHeader = 'Unleash-Client-Spec';
+
+    private readonly clientSpecFeatures: Record<ClientSpecFeature, SemVer> = {
+        segments: mustParseStrictSemVer('4.2.0'),
+    };
+
+    constructor(config: Pick<IUnleashConfig, 'getLogger'>) {
+        this.logger = config.getLogger('services/capability-service.ts');
+    }
+
+    requestSupportsSpec(request: Request, feature: ClientSpecFeature): boolean {
+        return this.versionSupportsSpec(
+            feature,
+            request.header(this.clientSpecHeader),
+        );
+    }
+
+    versionSupportsSpec(
+        feature: ClientSpecFeature,
+        version: string | undefined,
+    ): boolean {
+        if (!version) {
+            return false;
+        }
+
+        const parsedVersion = parseStrictSemVer(version);
+
+        if (!parsedVersion && !/^\d/.test(version)) {
+            throw new BadDataError(
+                `Invalid prefix in the ${this.clientSpecHeader} header: "${version}".`,
+            );
+        }
+
+        if (!parsedVersion) {
+            throw new BadDataError(
+                `Invalid SemVer in the ${this.clientSpecHeader} header: "${version}".`,
+            );
+        }
+
+        return semver.gte(parsedVersion, this.clientSpecFeatures[feature]);
+    }
+}

src/lib/services/index.ts

@@ -30,6 +30,7 @@ import ProjectHealthService from './project-health-service';
 import UserSplashService from './user-splash-service';
 import { SegmentService } from './segment-service';
 import { OpenApiService } from './openapi-service';
+import { ClientSpecService } from './client-spec-service';
 
 export const createServices = (
     stores: IUnleashStores,
@@ -78,6 +79,7 @@ export const createServices = (
     const userSplashService = new UserSplashService(stores, config);
     const segmentService = new SegmentService(stores, config);
     const openApiService = new OpenApiService(config);
+    const clientSpecService = new ClientSpecService(config);
 
     return {
         accessService,
@@ -109,6 +111,7 @@ export const createServices = (
         userSplashService,
         segmentService,
         openApiService,
+        clientSpecService,
     };
 };
 

src/lib/types/model.ts

@@ -180,6 +180,7 @@ export interface IFeatureToggleQuery {
     project?: string[];
     namePrefix?: string;
     environment?: string;
+    inlineSegmentConstraints?: boolean;
 }
 
 export interface ITag {

src/lib/types/services.ts

@@ -26,6 +26,7 @@ import ClientMetricsServiceV2 from '../services/client-metrics/metrics-service-v
 import UserSplashService from '../services/user-splash-service';
 import { SegmentService } from '../services/segment-service';
 import { OpenApiService } from '../services/openapi-service';
+import { ClientSpecService } from '../services/client-spec-service';
 
 export interface IUnleashServices {
     accessService: AccessService;
@@ -57,4 +58,5 @@ export interface IUnleashServices {
     userSplashService: UserSplashService;
     segmentService: SegmentService;
     openApiService: OpenApiService;
+    clientSpecService: ClientSpecService;
 }

src/lib/util/semver.test.ts

@@ -0,0 +1,23 @@
+import { mustParseStrictSemVer, parseStrictSemVer } from './semver';
+
+test('parseStrictSemVer', () => {
+    expect(parseStrictSemVer('')).toEqual(null);
+    expect(parseStrictSemVer('v')).toEqual(null);
+    expect(parseStrictSemVer('v1')).toEqual(null);
+    expect(parseStrictSemVer('v1.2.3')).toEqual(null);
+    expect(parseStrictSemVer('=1.2.3')).toEqual(null);
+    expect(parseStrictSemVer('1.2')).toEqual(null);
+    expect(parseStrictSemVer('1.2.3.4')).toEqual(null);
+    expect(parseStrictSemVer('1.2.3')!.version).toEqual('1.2.3');
+});
+
+test('mustParseSemVer', () => {
+    expect(() => mustParseStrictSemVer('').version).toThrow();
+    expect(() => mustParseStrictSemVer('1').version).toThrow();
+    expect(() => mustParseStrictSemVer('1.2').version).toThrow();
+    expect(() => mustParseStrictSemVer('v1.2').version).toThrow();
+    expect(() => mustParseStrictSemVer('v1.2.3').version).toThrow();
+    expect(() => mustParseStrictSemVer('=1.2.3').version).toThrow();
+    expect(() => mustParseStrictSemVer('1.2.3.4').version).toThrow();
+    expect(mustParseStrictSemVer('1.2.3').version).toEqual('1.2.3');
+});

src/lib/util/semver.ts

@@ -0,0 +1,23 @@
+import semver, { SemVer } from 'semver';
+
+export const parseStrictSemVer = (version: string): SemVer | null => {
+    if (semver.clean(version) !== version) {
+        return null;
+    }
+
+    try {
+        return semver.parse(version, { loose: false });
+    } catch {
+        return null;
+    }
+};
+
+export const mustParseStrictSemVer = (version: string): SemVer => {
+    const parsedVersion = parseStrictSemVer(version);
+
+    if (!parsedVersion) {
+        throw new Error('Could not parse SemVer string: ${version}');
+    }
+
+    return parsedVersion;
+};

src/lib/util/validators/constraint-types.ts

@@ -1,5 +1,3 @@
-import semver from 'semver';
-
 import {
     constraintDateTypeSchema,
     constraintNumberTypeSchema,
@@ -7,6 +5,7 @@ import {
 } from '../../schema/constraint-value-types';
 import BadDataError from '../../error/bad-data-error';
 import { ILegalValue } from '../../types/stores/context-field-store';
+import { parseStrictSemVer } from '../semver';
 
 export const validateNumber = async (value: unknown): Promise<void> => {
     await constraintNumberTypeSchema.validateAsync(value);
@@ -17,14 +16,15 @@ export const validateString = async (value: unknown): Promise<void> => {
 };
 
 export const validateSemver = (value: unknown): void => {
-    const cleanValue = semver.clean(value) === value;
+    if (typeof value !== 'string') {
+        throw new BadDataError(`the provided value is not a string.`);
+    }
 
-    const result = semver.valid(value);
+    if (!parseStrictSemVer(value)) {
-
-    if (result && cleanValue) return;
         throw new BadDataError(
             `the provided value is not a valid semver format. The value provided was: ${value}`,
         );
+    }
 };
 
 export const validateDate = async (value: unknown): Promise<void> => {

src/test/e2e/api/client/global.segment.e2e.test.ts

@@ -38,6 +38,7 @@ const fetchFeatures = (): Promise<IFeatureToggleClient[]> => {
 const fetchClientResponse = (): Promise<ApiResponse> => {
     return app.request
         .get(FEATURES_CLIENT_BASE_PATH)
+        .set('Unleash-Client-Spec', '4.2.0')
         .expect(200)
         .then((res) => res.body);
 };

src/test/e2e/api/client/segment.e2e.test.ts

@@ -12,6 +12,7 @@ import {
     DEFAULT_SEGMENT_VALUES_LIMIT,
     DEFAULT_STRATEGY_SEGMENTS_LIMIT,
 } from '../../../../lib/util/segments';
+import { collectIds } from '../../../../lib/util/collect-ids';
 
 let db: ITestDb;
 let app: IUnleashTest;
@@ -37,13 +38,6 @@ const fetchClientFeatures = (): Promise<IFeatureToggleClient[]> => {
         .then((res) => res.body.features);
 };
 
-const fetchGlobalSegments = (): Promise<ISegment[] | undefined> => {
-    return app.request
-        .get(FEATURES_CLIENT_BASE_PATH)
-        .expect(200)
-        .then((res) => res.body.segments);
-};
-
 const createSegment = (postData: object): Promise<unknown> => {
     const user = { email: 'test@example.com' } as User;
     return app.services.segmentService.create(postData, user);
@@ -102,7 +96,7 @@ afterEach(async () => {
     await db.stores.featureToggleStore.deleteAll();
 });
 
-test('should add segments to features as constraints', async () => {
+test('should inline segment constraints into features by default', async () => {
     const constraints = mockConstraints();
     await createSegment({ name: 'S1', constraints });
     await createSegment({ name: 'S2', constraints });
@@ -194,7 +188,7 @@ test('should validate feature strategy segment limit', async () => {
     );
 });
 
-test('should not return segments in base of toggle response if inline is enabled', async () => {
+test('should only return segments to clients with the segments capability', async () => {
     const constraints = mockConstraints();
     await createSegment({ name: 'S1', constraints });
     await createSegment({ name: 'S2', constraints });
@@ -204,11 +198,30 @@ test('should not return segments in base of toggle response if inline is enabled
     await createFeatureToggle(mockFeatureToggle());
     const [feature1, feature2] = await fetchFeatures();
     const [segment1, segment2] = await fetchSegments();
+    const segmentIds = collectIds([segment1, segment2]);
 
     await addSegmentToStrategy(segment1.id, feature1.strategies[0].id);
     await addSegmentToStrategy(segment2.id, feature1.strategies[0].id);
     await addSegmentToStrategy(segment2.id, feature2.strategies[0].id);
 
-    const globalSegments = await fetchGlobalSegments();
+    const unknownClientResponse = await app.request
-    expect(globalSegments).toBe(undefined);
+        .get(FEATURES_CLIENT_BASE_PATH)
+        .expect(200)
+        .then((res) => res.body);
+    const unknownClientConstraints = unknownClientResponse.features
+        .flatMap((f) => f.strategies)
+        .flatMap((s) => s.constraints);
+    expect(unknownClientResponse.segments).toEqual(undefined);
+    expect(unknownClientConstraints.length).toEqual(15);
+
+    const supportedClientResponse = await app.request
+        .get(FEATURES_CLIENT_BASE_PATH)
+        .set('Unleash-Client-Spec', '4.2.0')
+        .expect(200)
+        .then((res) => res.body);
+    const supportedClientConstraints = supportedClientResponse.features
+        .flatMap((f) => f.strategies)
+        .flatMap((s) => s.constraints);
+    expect(collectIds(supportedClientResponse.segments)).toEqual(segmentIds);
+    expect(supportedClientConstraints.length).toEqual(0);
 });

yarn.lock

@@ -1121,6 +1121,11 @@
   resolved "https://registry.npmjs.org/@types/retry/-/retry-0.12.2.tgz"
   integrity sha512-XISRgDJ2Tc5q4TRqvgJtzsRkFYNJzZrhTdtMoGVBttwzzQJkPnS3WWTFc7kuDRoPtPakl+T+OfdEUjYJj7Jbow==
 
+"@types/semver@^7.3.9":
+  version "7.3.9"
+  resolved "https://registry.yarnpkg.com/@types/semver/-/semver-7.3.9.tgz#152c6c20a7688c30b967ec1841d31ace569863fc"
+  integrity sha512-L/TMpyURfBkf+o/526Zb6kd/tchUP3iBDEPjqjb+U2MAJhVRxxrmr2fwpe08E7QsV7YLcpq0tUaQ9O9x97ZIxQ==
+
 "@types/serve-static@*":
   version "1.13.10"
   resolved "https://registry.npmjs.org/@types/serve-static/-/serve-static-1.13.10.tgz"