Skip to content

Commit

Permalink
chore: refactor segments to stop depending on the implementation (#3315)
Browse files Browse the repository at this point in the history
## About the changes
- Introducing ISegmentService interface to decouple from the actual
implementation
- Moving UpsertSegmentSchema to OSS to be able to use types
- Added comments where our code is coupled with segments just to
highlight and have a conversation about some use cases if needed, but
they can be removed before merging
- Removed segment service from some project features as it was not used
  • Loading branch information
gastonfournier committed Mar 15, 2023
1 parent 69a11ba commit 1d0bc83
Show file tree
Hide file tree
Showing 21 changed files with 132 additions and 55 deletions.
Expand Up @@ -20,6 +20,7 @@ import { DEFAULT_ENV } from '../../util';
import {
ContextFieldSchema,
ImportTogglesSchema,
UpsertSegmentSchema,
VariantsSchema,
} from '../../openapi';
import User from '../../types/user';
Expand Down Expand Up @@ -117,7 +118,7 @@ const createProject = async () => {
.expect(200);
};

const createSegment = (postData: object): Promise<ISegment> => {
const createSegment = (postData: UpsertSegmentSchema): Promise<ISegment> => {
return app.services.segmentService.create(postData, {
email: 'test@example.com',
});
Expand Down
2 changes: 2 additions & 0 deletions src/lib/openapi/index.ts
Expand Up @@ -117,6 +117,7 @@ import {
updateTagTypeSchema,
updateUserSchema,
upsertContextFieldSchema,
upsertSegmentSchema,
upsertStrategySchema,
userSchema,
usersGroupsBaseSchema,
Expand Down Expand Up @@ -272,6 +273,7 @@ export const schemas = {
updateUserSchema,
updateTagsSchema,
upsertContextFieldSchema,
upsertSegmentSchema,
upsertStrategySchema,
userSchema,
usersGroupsBaseSchema,
Expand Down
1 change: 1 addition & 0 deletions src/lib/openapi/spec/index.ts
Expand Up @@ -131,4 +131,5 @@ export * from './import-toggles-validate-schema';
export * from './import-toggles-schema';
export * from './stickiness-schema';
export * from './tags-bulk-add-schema';
export * from './upsert-segment-schema';
export * from './batch-features-schema';
34 changes: 34 additions & 0 deletions src/lib/openapi/spec/upsert-segment-schema.ts
@@ -0,0 +1,34 @@
import { FromSchema } from 'json-schema-to-ts';
import { constraintSchema } from './constraint-schema';

export const upsertSegmentSchema = {
$id: '#/components/schemas/upsertSegmentSchema',
type: 'object',
required: ['name', 'constraints'],
properties: {
name: {
type: 'string',
},
description: {
type: 'string',
nullable: true,
},
project: {
type: 'string',
nullable: true,
},
constraints: {
type: 'array',
items: {
$ref: '#/components/schemas/constraintSchema',
},
},
},
components: {
schemas: {
constraintSchema,
},
},
} as const;

export type UpsertSegmentSchema = FromSchema<typeof upsertSegmentSchema>;
2 changes: 1 addition & 1 deletion src/lib/proxy/proxy-repository.ts
Expand Up @@ -139,7 +139,7 @@ export class ProxyRepository

private async segmentsForToken(): Promise<Segment[]> {
return mapSegmentsForClient(
await this.services.segmentService.getAll(),
await this.services.segmentService.getAll(), // TODO coupled with enterprise feature
);
}

Expand Down
15 changes: 2 additions & 13 deletions src/lib/routes/admin-api/project/project-features.ts
Expand Up @@ -39,11 +39,7 @@ import {
UpdateFeatureSchema,
UpdateFeatureStrategySchema,
} from '../../../openapi';
import {
OpenApiService,
SegmentService,
FeatureToggleService,
} from '../../../services';
import { OpenApiService, FeatureToggleService } from '../../../services';
import { querySchema } from '../../../schema/feature-schema';
import NotFoundError from '../../../error/notfound-error';
import { BatchStaleSchema } from '../../../openapi/spec/batch-stale-schema';
Expand Down Expand Up @@ -97,24 +93,17 @@ export default class ProjectFeaturesController extends Controller {

private openApiService: OpenApiService;

private segmentService: SegmentService;

private flagResolver: IFlagResolver;

private readonly logger: Logger;

constructor(
config: IUnleashConfig,
{
featureToggleServiceV2,
openApiService,
segmentService,
}: ProjectFeaturesServices,
{ featureToggleServiceV2, openApiService }: ProjectFeaturesServices,
) {
super(config);
this.featureService = featureToggleServiceV2;
this.openApiService = openApiService;
this.segmentService = segmentService;
this.flagResolver = config.flagResolver;
this.logger = config.getLogger('/admin-api/project/features.ts');

Expand Down
4 changes: 2 additions & 2 deletions src/lib/routes/client-api/feature.ts
Expand Up @@ -10,7 +10,6 @@ import NotFoundError from '../../error/notfound-error';
import { IAuthRequest } from '../unleash-types';
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';
import { OpenApiService } from '../../services/openapi-service';
Expand All @@ -25,6 +24,7 @@ import {
clientFeaturesSchema,
ClientFeaturesSchema,
} from '../../openapi/spec/client-features-schema';
import { ISegmentService } from 'lib/segments/segment-service-interface';

const version = 2;

Expand All @@ -38,7 +38,7 @@ export default class FeatureController extends Controller {

private featureToggleServiceV2: FeatureToggleService;

private segmentService: SegmentService;
private segmentService: ISegmentService;

private clientSpecService: ClientSpecService;

Expand Down
29 changes: 29 additions & 0 deletions src/lib/segments/segment-service-interface.ts
@@ -0,0 +1,29 @@
import { UpsertSegmentSchema } from 'lib/openapi';
import { ISegment, IUser } from 'lib/types';

export interface ISegmentService {
updateStrategySegments: (
strategyId: string,
segmentIds: number[],
) => Promise<void>;

addToStrategy(id: number, strategyId: string): Promise<void>;

getByStrategy(strategyId: string): Promise<ISegment[]>;

getActive(): Promise<ISegment[]>;

getAll(): Promise<ISegment[]>;

create(
data: UpsertSegmentSchema,
user: Partial<Pick<IUser, 'username' | 'email'>>,
): Promise<ISegment>;

delete(id: number, user: IUser): Promise<void>;

cloneStrategySegments(
sourceStrategyId: string,
targetStrategyId: string,
): Promise<void>;
}
23 changes: 13 additions & 10 deletions src/lib/services/feature-toggle-service.ts
Expand Up @@ -71,7 +71,6 @@ import {
} from '../util/validators/constraint-types';
import { IContextFieldStore } from 'lib/types/stores/context-field-store';
import { Saved, Unsaved } from '../types/saved';
import { SegmentService } from './segment-service';
import { SetStrategySortOrderSchema } from 'lib/openapi/spec/set-strategy-sort-order-schema';
import { getDefaultStrategy } from '../util/feature-evaluator/helpers';
import { AccessService } from './access-service';
Expand All @@ -83,6 +82,7 @@ import {
import NoAccessError from '../error/no-access-error';
import { IFeatureProjectUserParams } from '../routes/admin-api/project/project-features';
import { unique } from '../util/unique';
import { ISegmentService } from 'lib/segments/segment-service-interface';

interface IFeatureContext {
featureName: string;
Expand Down Expand Up @@ -124,7 +124,7 @@ class FeatureToggleService {

private contextFieldStore: IContextFieldStore;

private segmentService: SegmentService;
private segmentService: ISegmentService;

private accessService: AccessService;

Expand Down Expand Up @@ -155,7 +155,7 @@ class FeatureToggleService {
getLogger,
flagResolver,
}: Pick<IUnleashConfig, 'getLogger' | 'flagResolver'>,
segmentService: SegmentService,
segmentService: ISegmentService,
accessService: AccessService,
) {
this.logger = getLogger('services/feature-toggle-service.ts');
Expand Down Expand Up @@ -375,7 +375,10 @@ class FeatureToggleService {
const { featureName, projectId, environment } = context;
await this.validateFeatureContext(context);

if (strategyConfig.constraints?.length > 0) {
if (
strategyConfig.constraints &&
strategyConfig.constraints.length > 0
) {
strategyConfig.constraints = await this.validateConstraints(
strategyConfig.constraints,
);
Expand Down Expand Up @@ -406,7 +409,7 @@ class FeatureToggleService {
const tags = await this.tagStore.getAllTagsForFeature(featureName);
const segments = await this.segmentService.getByStrategy(
newFeatureStrategy.id,
);
); // TODO coupled with enterprise feature
const strategy = this.featureStrategyToPublic(
newFeatureStrategy,
segments,
Expand Down Expand Up @@ -468,7 +471,7 @@ class FeatureToggleService {
this.validateFeatureStrategyContext(existingStrategy, context);

if (existingStrategy.id === id) {
if (updates.constraints?.length > 0) {
if (updates.constraints && updates.constraints.length > 0) {
updates.constraints = await this.validateConstraints(
updates.constraints,
);
Expand All @@ -488,7 +491,7 @@ class FeatureToggleService {

const segments = await this.segmentService.getByStrategy(
strategy.id,
);
); // TODO coupled with enterprise feature

// Store event!
const tags = await this.tagStore.getAllTagsForFeature(featureName);
Expand Down Expand Up @@ -534,7 +537,7 @@ class FeatureToggleService {
const tags = await this.tagStore.getAllTagsForFeature(featureName);
const segments = await this.segmentService.getByStrategy(
strategy.id,
);
); // TODO coupled with enterprise feature
const data = this.featureStrategyToPublic(strategy, segments);
const preData = this.featureStrategyToPublic(
existingStrategy,
Expand Down Expand Up @@ -633,7 +636,7 @@ class FeatureToggleService {
const segments =
(await this.segmentService.getByStrategy(strat.id)).map(
(segment) => segment.id,
) ?? [];
) ?? []; // TODO coupled with enterprise feature
result.push({
id: strat.id,
name: strat.strategyName,
Expand Down Expand Up @@ -950,7 +953,7 @@ class FeatureToggleService {
strategyId,
);

const segments = await this.segmentService.getByStrategy(strategyId);
const segments = await this.segmentService.getByStrategy(strategyId); // TODO coupled with enterprise feature
let result: Saved<IStrategyConfig> = {
id: strategy.id,
name: strategy.strategyName,
Expand Down
2 changes: 1 addition & 1 deletion src/lib/services/index.ts
Expand Up @@ -138,7 +138,7 @@ export const createServices = (
const versionService = new VersionService(stores, config);
const healthService = new HealthService(stores, config);
const userFeedbackService = new UserFeedbackService(stores, config);
const segmentService = new SegmentService(stores, config);
const segmentService = new SegmentService(stores, config); // TODO coupled with enterprise feature
const featureToggleServiceV2 = new FeatureToggleService(
stores,
config,
Expand Down
2 changes: 1 addition & 1 deletion src/lib/services/instance-stats-service.ts
Expand Up @@ -193,7 +193,7 @@ export class InstanceStatsService {
this.groupStore.count(),
this.roleStore.count(),
this.environmentStore.count(),
this.segmentStore.count(),
this.segmentStore.count(), // TODO coupled with enterprise feature
this.strategyStore.count(),
this.hasSAML(),
this.hasOIDC(),
Expand Down
4 changes: 2 additions & 2 deletions src/lib/services/playground-service.ts
Expand Up @@ -8,14 +8,14 @@ import { IUnleashConfig } from 'lib/types';
import { offlineUnleashClient } from '../util/offline-unleash-client';
import { FeatureInterface } from 'lib/util/feature-evaluator/feature';
import { FeatureStrategiesEvaluationResult } from 'lib/util/feature-evaluator/client';
import { SegmentService } from './segment-service';
import { ISegmentService } from 'lib/segments/segment-service-interface';

export class PlaygroundService {
private readonly logger: Logger;

private readonly featureToggleService: FeatureToggleService;

private readonly segmentService: SegmentService;
private readonly segmentService: ISegmentService;

constructor(
config: IUnleashConfig,
Expand Down
3 changes: 2 additions & 1 deletion src/lib/services/segment-service.ts
Expand Up @@ -14,8 +14,9 @@ import {
import User from '../types/user';
import { IFeatureStrategiesStore } from '../types/stores/feature-strategies-store';
import BadDataError from '../error/bad-data-error';
import { ISegmentService } from '../segments/segment-service-interface';

export class SegmentService {
export class SegmentService implements ISegmentService {
private logger: Logger;

private segmentStore: ISegmentStore;
Expand Down
6 changes: 0 additions & 6 deletions src/lib/services/state-service.test.ts
Expand Up @@ -20,7 +20,6 @@ function getSetup() {
return {
stateService: new StateService(stores, {
getLogger,
flagResolver: { isEnabled: () => true, getAll: () => ({}) },
}),
stores,
};
Expand Down Expand Up @@ -65,10 +64,6 @@ async function setupV3VariantsCompatibilityScenario(
return {
stateService: new StateService(stores, {
getLogger,
flagResolver: {
isEnabled: () => true,
getAll: () => ({}),
},
}),
stores,
};
Expand Down Expand Up @@ -579,7 +574,6 @@ test('exporting to new format works', async () => {
const stores = createStores();
const stateService = new StateService(stores, {
getLogger,
flagResolver: { isEnabled: () => true, getAll: () => ({}) },
});
await stores.projectStore.create({
id: 'fancy',
Expand Down
9 changes: 1 addition & 8 deletions src/lib/services/state-service.ts
Expand Up @@ -50,7 +50,6 @@ import { DEFAULT_ENV } from '../util/constants';
import { GLOBAL_ENV } from '../types/environment';
import { ISegmentStore } from '../types/stores/segment-store';
import { PartialSome } from '../types/partial';
import { IFlagResolver } from 'lib/types';

export interface IBackupOption {
includeFeatureToggles: boolean;
Expand Down Expand Up @@ -93,14 +92,9 @@ export default class StateService {

private segmentStore: ISegmentStore;

private flagResolver: IFlagResolver;

constructor(
stores: IUnleashStores,
{
getLogger,
flagResolver,
}: Pick<IUnleashConfig, 'getLogger' | 'flagResolver'>,
{ getLogger }: Pick<IUnleashConfig, 'getLogger'>,
) {
this.eventStore = stores.eventStore;
this.toggleStore = stores.featureToggleStore;
Expand All @@ -113,7 +107,6 @@ export default class StateService {
this.featureTagStore = stores.featureTagStore;
this.environmentStore = stores.environmentStore;
this.segmentStore = stores.segmentStore;
this.flagResolver = flagResolver;
this.logger = getLogger('services/state-service.js');
}

Expand Down

0 comments on commit 1d0bc83

Please sign in to comment.