1
0
mirror of https://github.com/Unleash/unleash.git synced 2025-01-20 00:08:02 +01:00

feat: project-specific segments tests and fixes (#3339)

## About the changes
- Refactored some E2E tests to use our APIs
- Added test cases for project-specific segments
- Added validation to check a project can access a specific segment
- Fixed an OpenAPI schema that was missing segments

## Discussion points
https://github.com/Unleash/unleash/pull/3339/files#r1140008992
This commit is contained in:
Gastón Fournier 2023-03-17 12:43:34 +01:00 committed by GitHub
parent 335374aa6d
commit 7a9ea22eed
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 341 additions and 103 deletions

View File

@ -100,23 +100,25 @@ const createVariants = async (feature: string, variants: IVariant[]) => {
);
};
const createProject = async () => {
const createProjects = async (projects: string[] = [DEFAULT_PROJECT]) => {
await db.stores.environmentStore.create({
name: DEFAULT_ENV,
type: 'production',
});
await db.stores.projectStore.create({
name: DEFAULT_PROJECT,
description: '',
id: DEFAULT_PROJECT,
mode: 'open' as const,
});
await app.request
.post(`/api/admin/projects/${DEFAULT_PROJECT}/environments`)
.send({
environment: DEFAULT_ENV,
})
.expect(200);
for (const project of projects) {
await db.stores.projectStore.create({
name: project,
description: '',
id: project,
mode: 'open' as const,
});
await app.request
.post(`/api/admin/projects/${project}/environments`)
.send({
environment: DEFAULT_ENV,
})
.expect(200);
}
};
const createSegment = (postData: UpsertSegmentSchema): Promise<ISegment> => {
@ -193,9 +195,74 @@ afterAll(async () => {
await db.destroy();
});
describe('import-export for project-specific segments', () => {
test('exports features with project-specific-segments', async () => {
const segmentName = 'my-segment';
const project = 'with-segments';
await createProjects([project]);
const segment = await createSegment({
name: segmentName,
project,
constraints: [],
});
const strategy = {
name: 'default',
parameters: { rollout: '100', stickiness: 'default' },
constraints: [
{
contextName: 'appName',
values: ['test'],
operator: 'IN' as const,
},
],
segments: [segment.id],
};
await createToggle(
{
name: 'first_feature',
description: 'the #1 feature',
},
strategy,
[],
project,
);
const { body } = await app.request
.post('/api/admin/features-batch/export')
.send({
features: ['first_feature'],
environment: 'default',
})
.set('Content-Type', 'application/json')
.expect(200);
const { name, ...resultStrategy } = strategy;
expect(body).toMatchObject({
features: [
{
name: 'first_feature',
},
],
featureStrategies: [resultStrategy],
featureEnvironments: [
{
enabled: false,
environment: 'default',
featureName: 'first_feature',
},
],
segments: [
{
id: segment.id,
name: segmentName,
},
],
});
});
});
test('exports features', async () => {
const segmentName = 'my-segment';
await createProject();
await createProjects();
const segment = await createSegment({ name: segmentName, constraints: [] });
const strategy = {
name: 'default',
@ -249,6 +316,7 @@ test('exports features', async () => {
],
segments: [
{
id: segment.id,
name: segmentName,
},
],
@ -256,7 +324,7 @@ test('exports features', async () => {
});
test('should export custom context fields from strategies and variants', async () => {
await createProject();
await createProjects();
const strategyContext = {
name: 'strategy-context',
legalValues: [
@ -358,7 +426,7 @@ test('should export custom context fields from strategies and variants', async (
test('should export tags', async () => {
const featureName = 'first_feature';
await createProject();
await createProjects();
await createToggle(
{
name: featureName,
@ -397,7 +465,7 @@ test('should export tags', async () => {
});
test('returns no features, when no feature was requested', async () => {
await createProject();
await createProjects();
await createToggle({
name: 'first_feature',
description: 'the #1 feature',
@ -551,7 +619,7 @@ const validateImport = (importPayload: ImportTogglesSchema, status = 200) =>
.expect(status);
test('import features to existing project and environment', async () => {
await createProject();
await createProjects();
await importToggles(defaultImportPayload);
@ -587,7 +655,7 @@ test('import features to existing project and environment', async () => {
});
test('importing same JSON should work multiple times in a row', async () => {
await createProject();
await createProjects();
await importToggles(defaultImportPayload);
await importToggles(defaultImportPayload);
@ -619,7 +687,7 @@ test('importing same JSON should work multiple times in a row', async () => {
});
test('reject import with unknown context fields', async () => {
await createProject();
await createProjects();
const contextField = {
name: 'ContextField1',
legalValues: [{ value: 'Value1', description: '' }],
@ -650,7 +718,7 @@ test('reject import with unknown context fields', async () => {
});
test('reject import with unsupported strategies', async () => {
await createProject();
await createProjects();
const importPayloadWithContextFields: ImportTogglesSchema = {
...defaultImportPayload,
data: {
@ -673,7 +741,7 @@ test('reject import with unsupported strategies', async () => {
});
test('validate import data', async () => {
await createProject();
await createProjects();
const contextField: IContextFieldDto = {
name: 'validate_context_field',
legalValues: [{ value: 'Value1' }],
@ -731,7 +799,7 @@ test('validate import data', async () => {
});
test('should create new context', async () => {
await createProject();
await createProjects();
const context = {
name: 'create-new-context',
legalValues: [{ value: 'Value1' }],
@ -751,7 +819,7 @@ test('should create new context', async () => {
});
test('should not import archived features tags', async () => {
await createProject();
await createProjects();
await importToggles(defaultImportPayload);
await archiveFeature(defaultFeature);

View File

@ -22,6 +22,13 @@ export const createFeatureStrategySchema = {
parameters: {
$ref: '#/components/schemas/parametersSchema',
},
segments: {
type: 'array',
description: 'Ids of segments to use for this strategy',
items: {
type: 'number',
},
},
},
components: {
schemas: {

View File

@ -80,10 +80,7 @@ const PATH_STRATEGY = `${PATH_STRATEGIES}/:strategyId`;
type ProjectFeaturesServices = Pick<
IUnleashServices,
| 'featureToggleServiceV2'
| 'projectHealthService'
| 'openApiService'
| 'segmentService'
'featureToggleServiceV2' | 'projectHealthService' | 'openApiService'
>;
export default class ProjectFeaturesController extends Controller {

View File

@ -11,6 +11,8 @@ export interface ISegmentService {
getByStrategy(strategyId: string): Promise<ISegment[]>;
get(id: number): Promise<ISegment>;
getActive(): Promise<ISegment[]>;
getAll(): Promise<ISegment[]>;

View File

@ -194,7 +194,7 @@ class FeatureToggleService {
}
}
async validateFeatureContext({
async validateFeatureBelongsToProject({
featureName,
projectId,
}: IFeatureContext): Promise<void> {
@ -207,9 +207,9 @@ class FeatureToggleService {
}
}
validateFeatureStrategyContext(
validateUpdatedProperties(
{ featureName, projectId }: IFeatureContext,
strategy: IFeatureStrategy,
{ featureName, projectId }: IFeatureStrategyContext,
): void {
if (strategy.projectId !== projectId) {
throw new InvalidOperationError(
@ -224,6 +224,27 @@ class FeatureToggleService {
}
}
async validateProjectCanAccessSegments(
projectId: string,
segmentIds?: number[],
): Promise<void> {
if (segmentIds && segmentIds.length > 0) {
await Promise.all(
segmentIds.map((segmentId) =>
this.segmentService.get(segmentId),
),
).then((segments) =>
segments.map((segment) => {
if (segment.project && segment.project !== projectId) {
throw new BadDataError(
`The segment "${segment.name}" with id ${segment.id} does not belong to project "${projectId}".`,
);
}
}),
);
}
}
async validateConstraints(
constraints: IConstraint[],
): Promise<IConstraint[]> {
@ -373,7 +394,12 @@ class FeatureToggleService {
createdBy: string,
): Promise<Saved<IStrategyConfig>> {
const { featureName, projectId, environment } = context;
await this.validateFeatureContext(context);
await this.validateFeatureBelongsToProject(context);
await this.validateProjectCanAccessSegments(
projectId,
strategyConfig.segments,
);
if (
strategyConfig.constraints &&
@ -468,7 +494,11 @@ class FeatureToggleService {
): Promise<Saved<IStrategyConfig>> {
const { projectId, environment, featureName } = context;
const existingStrategy = await this.featureStrategiesStore.get(id);
this.validateFeatureStrategyContext(existingStrategy, context);
this.validateUpdatedProperties(context, existingStrategy);
await this.validateProjectCanAccessSegments(
projectId,
updates.segments,
);
if (existingStrategy.id === id) {
if (updates.constraints && updates.constraints.length > 0) {
@ -526,7 +556,7 @@ class FeatureToggleService {
const { projectId, environment, featureName } = context;
const existingStrategy = await this.featureStrategiesStore.get(id);
this.validateFeatureStrategyContext(existingStrategy, context);
this.validateUpdatedProperties(context, existingStrategy);
if (existingStrategy.id === id) {
existingStrategy.parameters[name] = String(value);
@ -589,7 +619,7 @@ class FeatureToggleService {
): Promise<void> {
const existingStrategy = await this.featureStrategiesStore.get(id);
const { featureName, projectId, environment } = context;
this.validateFeatureStrategyContext(existingStrategy, context);
this.validateUpdatedProperties(context, existingStrategy);
await this.featureStrategiesStore.delete(id);
@ -667,7 +697,10 @@ class FeatureToggleService {
userId,
}: IGetFeatureParams): Promise<FeatureToggleWithEnvironment> {
if (projectId) {
await this.validateFeatureContext({ featureName, projectId });
await this.validateFeatureBelongsToProject({
featureName,
projectId,
});
}
if (environmentVariants) {
@ -901,7 +934,7 @@ class FeatureToggleService {
userName: string,
featureName: string,
): Promise<FeatureToggle> {
await this.validateFeatureContext({ featureName, projectId });
await this.validateFeatureBelongsToProject({ featureName, projectId });
this.logger.info(`${userName} updates feature toggle ${featureName}`);
@ -1066,7 +1099,10 @@ class FeatureToggleService {
const feature = await this.featureToggleStore.get(featureName);
if (projectId) {
await this.validateFeatureContext({ featureName, projectId });
await this.validateFeatureBelongsToProject({
featureName,
projectId,
});
}
await this.featureToggleStore.archive(featureName);

View File

@ -14,12 +14,18 @@ import {
} from '../../../../lib/util/segments';
import { collectIds } from '../../../../lib/util/collect-ids';
import { arraysHaveSameItems } from '../../../../lib/util/arraysHaveSameItems';
import { UpsertSegmentSchema } from 'lib/openapi';
import {
CreateFeatureSchema,
CreateFeatureStrategySchema,
FeatureStrategySchema,
UpsertSegmentSchema,
} from 'lib/openapi';
import { DEFAULT_ENV } from '../../../../lib/util';
import { DEFAULT_PROJECT } from '../../../../lib/types';
let db: ITestDb;
let app: IUnleashTest;
const FEATURES_ADMIN_BASE_PATH = '/api/admin/features';
const FEATURES_CLIENT_BASE_PATH = '/api/client/features';
const fetchSegments = (): Promise<ISegment[]> => {
@ -28,7 +34,7 @@ const fetchSegments = (): Promise<ISegment[]> => {
const fetchFeatures = (): Promise<IFeatureToggleClient[]> => {
return app.request
.get(FEATURES_ADMIN_BASE_PATH)
.get(`/api/admin/features`)
.expect(200)
.then((res) => res.body.features);
};
@ -40,33 +46,86 @@ const fetchClientFeatures = (): Promise<IFeatureToggleClient[]> => {
.then((res) => res.body.features);
};
const createSegment = (postData: UpsertSegmentSchema): Promise<unknown> => {
const createSegment = (postData: UpsertSegmentSchema): Promise<ISegment> => {
return app.services.segmentService.create(postData, {
email: 'test@example.com',
});
};
const createFeatureToggle = (
postData: object,
expectStatusCode = 201,
): Promise<unknown> => {
return app.request
.post(FEATURES_ADMIN_BASE_PATH)
.send(postData)
.expect(expectStatusCode);
};
const addSegmentToStrategy = (
segmentId: number,
strategyId: string,
): Promise<unknown> => {
return app.services.segmentService.addToStrategy(segmentId, strategyId);
};
const mockFeatureToggle = (): object => {
const mockStrategy = (segments: number[] = []) => {
return {
name: randomId(),
parameters: {},
constraints: [],
segments,
};
};
const createProjects = async (projects: string[] = [DEFAULT_PROJECT]) => {
for (const project of projects) {
await db.stores.projectStore.create({
name: project,
description: '',
id: project,
mode: 'open' as const,
});
await app.request
.post(`/api/admin/projects/${project}/environments`)
.send({
environment: DEFAULT_ENV,
})
.expect(200);
}
};
const createFeatureToggle = async (
feature: CreateFeatureSchema,
strategies: CreateFeatureStrategySchema[] = [mockStrategy()],
project = DEFAULT_PROJECT,
environment = DEFAULT_ENV,
expectStatusCode = 201,
expectSegmentStatusCodes: { status: number; message?: string }[] = [
{ status: 200 },
],
): Promise<void> => {
await app.request
.post(`/api/admin/projects/${project}/features`)
.send(feature)
.expect(expectStatusCode);
let processed = 0;
for (const strategy of strategies) {
const { body, status } = await app.request
.post(
`/api/admin/projects/${project}/features/${feature.name}/environments/${environment}/strategies`,
)
.send(strategy);
const expectation = expectSegmentStatusCodes[processed++];
expect(status).toBe(expectation.status);
if (expectation.message) {
expect(JSON.stringify(body)).toContain(expectation.message);
}
}
};
const updateFeatureStrategy = async (
featureName: string,
strategy: FeatureStrategySchema,
project = DEFAULT_PROJECT,
environment = DEFAULT_ENV,
expectedStatus = 200,
): Promise<void> => {
const { status } = await app.request
.put(
`/api/admin/projects/${project}/features/${featureName}/environments/${environment}/strategies/${strategy.id}`,
)
.send(strategy);
expect(status).toBe(expectedStatus);
};
const mockFeatureToggle = () => {
return {
name: randomId(),
strategies: [{ name: randomId(), constraints: [], parameters: {} }],
};
};
@ -99,19 +158,19 @@ const fetchClientResponse = (): Promise<{
const createTestSegments = async () => {
const constraints = mockConstraints();
await createSegment({ name: 'S1', constraints });
await createSegment({ name: 'S2', constraints });
await createSegment({ name: 'S3', constraints });
const segment1 = await createSegment({ name: 'S1', constraints });
const segment2 = await createSegment({ name: 'S2', constraints });
const segment3 = await createSegment({ name: 'S3', constraints });
await createFeatureToggle(mockFeatureToggle());
await createFeatureToggle(mockFeatureToggle());
await createFeatureToggle(mockFeatureToggle(), [
mockStrategy([segment1.id, segment2.id]),
]);
await createFeatureToggle(mockFeatureToggle(), [
mockStrategy([segment2.id]),
]);
await createFeatureToggle(mockFeatureToggle());
const [feature1, feature2] = await fetchFeatures();
const [segment1, segment2] = await fetchSegments();
await addSegmentToStrategy(segment1.id, feature1.strategies[0].id);
await addSegmentToStrategy(segment2.id, feature1.strategies[0].id);
await addSegmentToStrategy(segment2.id, feature2.strategies[0].id);
return [segment1, segment2, segment3];
};
beforeAll(async () => {
@ -168,53 +227,50 @@ test('should validate segment constraint values limit with multiple constraints'
});
test('should validate feature strategy segment limit', async () => {
await createSegment({ name: 'S1', constraints: [] });
await createSegment({ name: 'S2', constraints: [] });
await createSegment({ name: 'S3', constraints: [] });
await createSegment({ name: 'S4', constraints: [] });
await createSegment({ name: 'S5', constraints: [] });
await createSegment({ name: 'S6', constraints: [] });
await createFeatureToggle(mockFeatureToggle());
const [feature1] = await fetchFeatures();
const segments = await fetchSegments();
const segments: ISegment[] = [];
for (const id of [1, 2, 3, 4, 5, 6]) {
segments.push(await createSegment({ name: `S${id}`, constraints: [] }));
}
await addSegmentToStrategy(segments[0].id, feature1.strategies[0].id);
await addSegmentToStrategy(segments[1].id, feature1.strategies[0].id);
await addSegmentToStrategy(segments[2].id, feature1.strategies[0].id);
await addSegmentToStrategy(segments[3].id, feature1.strategies[0].id);
await addSegmentToStrategy(segments[4].id, feature1.strategies[0].id);
await expect(
addSegmentToStrategy(segments[5].id, feature1.strategies[0].id),
).rejects.toThrow(
`Strategies may not have more than ${DEFAULT_STRATEGY_SEGMENTS_LIMIT} segments`,
await createFeatureToggle(
mockFeatureToggle(),
[mockStrategy(segments.map((s) => s.id))],
DEFAULT_PROJECT,
DEFAULT_ENV,
201,
[
{
status: 400,
message: `Strategies may not have more than ${DEFAULT_STRATEGY_SEGMENTS_LIMIT} segments`,
},
],
);
});
test('should clone feature strategy segments', async () => {
const constraints = mockConstraints();
await createSegment({ name: 'S1', constraints });
await createFeatureToggle(mockFeatureToggle());
const segment1 = await createSegment({ name: 'S1', constraints });
await createFeatureToggle(mockFeatureToggle(), [
mockStrategy([segment1.id]),
]);
await createFeatureToggle(mockFeatureToggle());
const [feature1, feature2] = await fetchFeatures();
const strategy1 = feature1.strategies[0].id;
const strategy2 = feature2.strategies[0].id;
const [segment1] = await fetchSegments();
await addSegmentToStrategy(segment1.id, feature1.strategies[0].id);
let segments1 = await app.services.segmentService.getByStrategy(strategy1);
let segments2 = await app.services.segmentService.getByStrategy(strategy2);
let segments1 = await app.services.segmentService.getByStrategy(strategy1!);
let segments2 = await app.services.segmentService.getByStrategy(strategy2!);
expect(collectIds(segments1)).toEqual([segment1.id]);
expect(collectIds(segments2)).toEqual([]);
await app.services.segmentService.cloneStrategySegments(
strategy1,
strategy2,
strategy1!,
strategy2!,
);
segments1 = await app.services.segmentService.getByStrategy(strategy1);
segments2 = await app.services.segmentService.getByStrategy(strategy2);
segments1 = await app.services.segmentService.getByStrategy(strategy1!);
segments2 = await app.services.segmentService.getByStrategy(strategy2!);
expect(collectIds(segments1)).toEqual([segment1.id]);
expect(collectIds(segments2)).toEqual([segment1.id]);
});
@ -239,9 +295,18 @@ test('should inline segment constraints into features by default', async () => {
const [feature1, feature2, feature3] = await fetchFeatures();
const [, , segment3] = await fetchSegments();
await addSegmentToStrategy(segment3.id, feature1.strategies[0].id);
await addSegmentToStrategy(segment3.id, feature2.strategies[0].id);
await addSegmentToStrategy(segment3.id, feature3.strategies[0].id);
// add segment3 to all features
for (const feature of [feature1, feature2, feature3]) {
const strategy = {
...feature.strategies[0],
segments: feature.strategies[0].segments ?? [],
};
await updateFeatureStrategy(feature.name, {
...strategy,
segments: [...strategy.segments, segment3.id],
});
}
const clientFeatures = await fetchClientFeatures();
const clientStrategies = clientFeatures.flatMap((f) => f.strategies);
@ -316,3 +381,59 @@ test('should send all segments that are in use by feature', async () => {
true,
);
});
describe('project-specific segments', () => {
test(`can create a toggle with a project-specific segment`, async () => {
const segmentName = 'my-segment';
const project = randomId();
await createProjects([project]);
const segment = await createSegment({
name: segmentName,
project,
constraints: [],
});
const strategy = {
name: 'default',
parameters: {},
constraints: [],
segments: [segment.id],
};
await createFeatureToggle(
{
name: 'first_feature',
description: 'the #1 feature',
},
[strategy],
project,
);
});
test(`can't create a toggle with a segment from a different project`, async () => {
const segmentName = 'my-segment';
const project1 = randomId();
const project2 = randomId();
await createProjects([project1, project2]);
const segment = await createSegment({
name: segmentName,
project: project1,
constraints: [],
});
const strategy = {
name: 'default',
parameters: {},
constraints: [],
segments: [segment.id],
};
await createFeatureToggle(
{
name: 'first_feature',
description: 'the #1 feature',
},
[strategy],
project2,
DEFAULT_ENV,
201,
[{ status: 400 }],
);
});
});

View File

@ -889,6 +889,13 @@ exports[`should serve the OpenAPI spec 1`] = `
"parameters": {
"$ref": "#/components/schemas/parametersSchema",
},
"segments": {
"description": "Ids of segments to use for this strategy",
"items": {
"type": "number",
},
"type": "array",
},
"sortOrder": {
"type": "number",
},