Skip to content

Commit

Permalink
refactor: avoid inlining segments for supported clients (#1640)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@olav
olav committed Jun 2, 2022
1 parent 00c84f3 commit 7e3f032
Show file tree
Hide file tree
Showing 16 changed files with 261 additions and 93 deletions.
9 changes: 5 additions & 4 deletions package.json
View file
Expand Up @@ -76,6 +76,7 @@
]
},
"dependencies": {
"@unleash/express-openapi": "^0.2.0",
"async": "^3.2.3",
"bcryptjs": "^2.4.3",
"compression": "^1.7.4",
Expand All @@ -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",
Expand All @@ -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",
"semver": "^7.3.5"
"uuid": "^8.3.2"
},
"devDependencies": {
"@babel/core": "7.18.2",
Expand All @@ -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",
Expand Down
7 changes: 5 additions & 2 deletions src/lib/db/feature-toggle-client-store.ts
View file
Expand Up @@ -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;
Expand Down
64 changes: 35 additions & 29 deletions src/lib/routes/client-api/feature.test.ts
View file
Expand Up @@ -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)}`;
Expand All @@ -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;
Expand Down Expand Up @@ -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 featureToggleServiceV2 = {
getClientFeatures,
};

const segmentService = {
getActive,
};
const clientSpecService = new ClientSpecService({ getLogger });
const featureToggleServiceV2 = { getClientFeatures };
const segmentService = { getActive };

const controller = new FeatureController(
// @ts-ignore
{ featureToggleServiceV2, segmentService },
{
clientSpecService,
// @ts-expect-error
featureToggleServiceV2,
// @ts-expect-error
segmentService,
},
{
getLogger,
experimental: {
Expand All @@ -83,29 +92,27 @@ test('if caching is enabled should memoize', async () => {
},
},
);
// @ts-ignore
await controller.getAll({ query: {} }, { json: () => {} });
// @ts-ignore
await controller.getAll({ query: {} }, { json: () => {} });

await callGetAll(controller);
await callGetAll(controller);
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 featureToggleServiceV2 = {
getClientFeatures,
};

const segmentService = {
getActive,
};
const clientSpecService = new ClientSpecService({ getLogger });
const featureToggleServiceV2 = { getClientFeatures };
const segmentService = { getActive };

const controller = new FeatureController(
// @ts-ignore
{ featureToggleServiceV2, segmentService },
{
clientSpecService,
// @ts-expect-error
featureToggleServiceV2,
// @ts-expect-error
segmentService,
},
{
getLogger,
experimental: {
Expand All @@ -116,10 +123,9 @@ test('if caching is not enabled all calls goes to service', async () => {
},
},
);
// @ts-ignore
await controller.getAll({ query: {} }, { json: () => {} });
// @ts-ignore
await controller.getAll({ query: {} }, { json: () => {} });

await callGetAll(controller);
await callGetAll(controller);
expect(getClientFeatures).toHaveBeenCalledTimes(2);
});

Expand Down
79 changes: 41 additions & 38 deletions src/lib/routes/client-api/feature.ts
View file
Expand Up @@ -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;

Expand All @@ -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);
Expand All @@ -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(),
]);
}

Expand All @@ -101,8 +98,14 @@ export default class FeatureController extends Controller {
}
}

const q = { ...query, ...override };
return this.prepQuery(q);
const inlineSegmentConstraints =
!this.clientSpecService.requestSupportsSpec(req, 'segments');

return this.prepQuery({
...query,
...override,
inlineSegmentConstraints,
});
}

// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
Expand All @@ -118,49 +121,49 @@ 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({
tag: tagQuery,
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);
let features, segments;
if (this.cache) {
[features, segments] = await this.cachedFeatures(featureQuery);
} else {
features = await this.featureToggleServiceV2.getClientFeatures(
featureQuery,
);
segments = await this.resolveSegments();
}
const query = await this.resolveQuery(req);

const [features, segments] = this.cache
? await this.cachedFeatures(query)
: await Promise.all([
this.featureToggleServiceV2.getClientFeatures(query),
this.segmentService.getActive(),
]);

const response = {
version,
features,
query: featureQuery,
};

if (this.useGlobalSegments) {
res.json({
...response,
segments,
});
if (this.clientSpecService.requestSupportsSpec(req, 'segments')) {
res.json({ version, features, query, segments });
} else {
res.json(response);
res.json({ version, features, query });
}
}

Expand Down
1 change: 1 addition & 0 deletions src/lib/schema/feature-schema.ts
View file
Expand Up @@ -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 });

Expand Down
29 changes: 29 additions & 0 deletions src/lib/services/client-spec-service.test.ts
View file
@@ -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);
});

0 comments on commit 7e3f032

Please sign in to comment.