diff --git a/.claude/plans/precomputed-segments-experiments.md b/.claude/plans/precomputed-segments-experiments.md new file mode 100644 index 0000000000..e6a9fd8099 --- /dev/null +++ b/.claude/plans/precomputed-segments-experiments.md @@ -0,0 +1,291 @@ +# Plan: Precomputed Segment Lists for Experiments + +Port the feature-flag precomputed segment list pattern to experiments. +The feature-flag implementation is complete and stable on `wip/segments-precalculated` — this is a 1:1 parallel. + +## Background + +Experiments currently resolve segment inclusion/exclusion **on-the-fly at assignment time** via recursive `resolveSegment()` calls in `ExperimentAssignmentService`. This is the same slow path that feature flags moved away from. The goal is to precompute flat `inclusionIds[]` / `exclusionIds[]` arrays per experiment, stored in a new `precomputed_experiment_segment` table, so the assignment path becomes a fast in-memory `Set.has()` check. + +Key difference from feature flags: experiment join tables (`ExperimentSegmentInclusion`, `ExperimentSegmentExclusion`) have **no `enabled` column** — all rows are active. The `where: { enabled: true }` filter in `recomputeForFlag` does not apply here. + +## Files to create + +- `src/api/models/PrecomputedExperimentSegment.ts` — entity +- `src/api/repositories/PrecomputedExperimentSegmentRepository.ts` — repo +- `src/database/migrations/-precomputedExperimentSegment.ts` — migration +- `src/init/seed/backfillPrecomputedExperimentSegments.ts` — startup backfill + +## Files to modify + +- `src/api/services/PrecomputedSegmentService.ts` — add experiment methods +- `src/api/services/ExperimentAssignmentService.ts` — replace `resolveSegment()` with precomputed lookup +- `src/api/services/ExperimentService.ts` — add recompute triggers on segment list mutations +- `src/app.ts` — wire startup backfill + +--- + +## Step 1 — Entity + +**`src/api/models/PrecomputedExperimentSegment.ts`** + +Mirror `PrecomputedSegment` exactly, replacing the feature flag FK with an experiment FK: + +```ts +@Entity() +export class PrecomputedExperimentSegment extends BaseModel { + @PrimaryColumn('uuid') + public experimentId: string; + + @ManyToOne(() => Experiment, { onDelete: 'CASCADE' }) + @JoinColumn({ name: 'experimentId' }) + public experiment: Experiment; + + @Column('text', { array: true, default: '{}' }) + public inclusionIds: string[]; + + @Column('text', { array: true, default: '{}' }) + public exclusionIds: string[]; +} +``` + +Register the entity in `env.ts` (wherever the entities glob is — confirm it picks up `src/api/models/*.ts` automatically; if so no change needed). + +--- + +## Step 2 — Repository + +**`src/api/repositories/PrecomputedExperimentSegmentRepository.ts`** + +Mirror `PrecomputedSegmentRepository`: + +```ts +@EntityRepository(PrecomputedExperimentSegment) +export class PrecomputedExperimentSegmentRepository extends Repository { + public async upsertByExperimentId(experimentId: string, inclusionIds: string[], exclusionIds: string[]): Promise { + await this.createQueryBuilder() + .insert() + .into(PrecomputedExperimentSegment) + .values({ experimentId, inclusionIds, exclusionIds }) + .orUpdate(['inclusionIds', 'exclusionIds', 'updatedAt'], ['experimentId']) + .execute(); + } + + public async findByExperimentIds(experimentIds: string[]): Promise<(PrecomputedExperimentSegment | null)[]> { + if (!experimentIds.length) return []; + const rows = await this.createQueryBuilder('ps') + .where('ps.experimentId IN (:...ids)', { ids: experimentIds }) + .getMany(); + return experimentIds.map((id) => rows.find((r) => r.experimentId === id) ?? null); + } +} +``` + +--- + +## Step 3 — Migration + +Generate via: +```bash +npm run migration:generate -- -n precomputedExperimentSegment +``` + +Expected output — creates `precomputed_experiment_segment` table with `experimentId` PK (uuid), `inclusionIds` text[], `exclusionIds` text[], standard `BaseModel` timestamp columns, FK to `experiment` with `ON DELETE CASCADE`. + +Verify the generated migration matches intent before running. + +--- + +## Step 4 — Service methods + +**`src/api/services/PrecomputedSegmentService.ts`** — inject `PrecomputedExperimentSegmentRepository` and `ExperimentSegmentInclusionRepository` / `ExperimentSegmentExclusionRepository` and `ExperimentRepository`, then add: + +```ts +public async recomputeForExperiment(experimentId: string, logger: UpgradeLogger): Promise { + const [inclusionRecords, exclusionRecords] = await Promise.all([ + this.experimentSegmentInclusionRepository.find({ + where: { experiment: { id: experimentId } }, + relations: ['segment'], + }), + this.experimentSegmentExclusionRepository.find({ + where: { experiment: { id: experimentId } }, + relations: ['segment'], + }), + ]); + + const inclusionSegmentIds = inclusionRecords.map((r) => r.segment.id); + const exclusionSegmentIds = exclusionRecords.map((r) => r.segment.id); + + const [inclusionIds, exclusionIds] = await Promise.all([ + this.flattenSegmentMembers(inclusionSegmentIds, new Set()), + this.flattenSegmentMembers(exclusionSegmentIds, new Set()), + ]); + + await this.precomputedExperimentSegmentRepository.upsertByExperimentId( + experimentId, + [...new Set(inclusionIds)], + [...new Set(exclusionIds)] + ); + + await this.cacheService.delCache(CACHE_PREFIX.PRECOMPUTED_EXPERIMENT_SEGMENT_KEY_PREFIX + experimentId); + logger.info({ message: `Recomputed precomputed_experiment_segment for experiment ${experimentId}` }); +} + +public scheduleRecomputeForExperimentSegment(segmentId: string, logger: UpgradeLogger): void { + this.collectAffectedExperimentIds(segmentId, new Set()) + .then((experimentIds) => Promise.all([...experimentIds].map((id) => this.recomputeForExperiment(id, logger)))) + .catch((err) => logger.error({ message: `Error in scheduleRecomputeForExperimentSegment: ${err}` })); +} + +public async getAffectedExperimentIds(segmentId: string): Promise { + return [...(await this.collectAffectedExperimentIds(segmentId, new Set()))]; +} + +public async getExperimentPrecomputedSets(experimentIds: string[]): Promise> { + if (!experimentIds.length) return new Map(); + const results = await this.cacheService.wrapFunction( + CACHE_PREFIX.PRECOMPUTED_EXPERIMENT_SEGMENT_KEY_PREFIX, + experimentIds, + () => this.precomputedExperimentSegmentRepository.findByExperimentIds(experimentIds) + ); + const map = new Map(); + experimentIds.forEach((id, i) => { + if (results[i]) map.set(id, results[i] as PrecomputedExperimentSegment); + }); + return map; +} + +public async backfillMissingExperiments(logger: UpgradeLogger): Promise { + const [allExperiments, existingRows] = await Promise.all([ + this.experimentRepository.find({ select: ['id'] }), + this.precomputedExperimentSegmentRepository.find({ select: ['experimentId'] }), + ]); + const existingIds = new Set(existingRows.map((r) => r.experimentId)); + const missing = allExperiments.filter((e) => !existingIds.has(e.id)); + if (!missing.length) { + logger.info({ message: 'precomputed_experiment_segment backfill: all experiments already have rows, nothing to do' }); + return; + } + for (const exp of missing) { + try { + await this.recomputeForExperiment(exp.id, logger); + } catch (err) { + logger.error({ message: `Failed to backfill precomputed_experiment_segment for experiment ${exp.id}: ${err}` }); + } + } + logger.info({ message: `precomputed_experiment_segment backfill complete: computed ${missing.length} of ${allExperiments.length} experiments` }); +} + +private async collectAffectedExperimentIds(segmentId: string, visited: Set): Promise> { + if (visited.has(segmentId)) return new Set(); + visited.add(segmentId); + + const [inclusionRecords, exclusionRecords] = await Promise.all([ + this.experimentSegmentInclusionRepository.find({ + where: { segment: { id: segmentId } }, + relations: ['experiment'], + }), + this.experimentSegmentExclusionRepository.find({ + where: { segment: { id: segmentId } }, + relations: ['experiment'], + }), + ]); + + const experimentIds = new Set([ + ...inclusionRecords.map((r) => r.experiment.id), + ...exclusionRecords.map((r) => r.experiment.id), + ]); + + const parentIds = await this.segmentRepository.findParentSegmentIds(segmentId); + await Promise.all( + parentIds.map(async (parentId) => { + const parentExpIds = await this.collectAffectedExperimentIds(parentId, visited); + parentExpIds.forEach((id) => experimentIds.add(id)); + }) + ); + + return experimentIds; +} +``` + +Note: `flattenSegmentMembers` is shared — no duplication needed. Only the join table queries and repository differ. + +**Add `PRECOMPUTED_EXPERIMENT_SEGMENT_KEY_PREFIX` to `CACHE_PREFIX` in `packages/types`.** + +--- + +## Step 5 — ExperimentAssignmentService read path + +This is the largest change. Currently `getIncludedAndExcludedExperiments()` → `resolveSegmentsForEntities()` → `resolveSegment()` do recursive DB queries. Replace that with a precomputed lookup. + +The pattern to follow is `FeatureFlagService.featureFlagLevelInclusionExclusion()` — call `getExperimentPrecomputedSets(experimentIds)` and replace the `includeData` / `excludeData` maps that `inclusionExclusionLogic()` currently receives from recursive resolution with maps built from the precomputed flat arrays. + +Specific methods to audit and update in `ExperimentAssignmentService.ts`: +- `getSegmentObject()` (line ~2102) — currently extracts segment IDs to resolve +- `resolveSegmentsForEntities()` (line ~2140) — drives resolution; replace with precomputed map build +- `getIncludedAndExcludedExperiments()` (line ~2156) — wires the above together +- `inclusionExclusionLogic()` (line ~2231) — the actual include/exclude evaluation; this should be largely untouched if the input maps have the same shape + +The key question to verify before implementing: does `inclusionExclusionLogic()` expect `{users: userId[], groups: {type, groupId}[]}` shaped data from resolved segments, or does it work with flat ID arrays? Flat inclusion/exclusion arrays may need a small shim to match the expected shape. Inspect the method signature and callers carefully before changing the data shape. + +--- + +## Step 6 — ExperimentService triggers + +Find the equivalents of the three `FeatureFlagService` trigger points and add matching calls. Look for methods that: +- Add a segment to an experiment's inclusion/exclusion list → `recomputeForExperiment` after +- Remove a segment from an experiment's inclusion/exclusion list → `recomputeForExperiment` after +- Update segment members in an experiment context → `recomputeForExperiment` after +- Delete an experiment's segment entirely → collect IDs before, recompute after (same pattern as `SegmentService.deleteSegment`) + +Also check if `ExperimentService` has a `deleteExperiment` path — if so, the FK cascade handles cleanup (same as feature flags), no extra work needed. + +The `SegmentService` triggers (`scheduleRecomputeForExperimentSegment`) should be added alongside the existing `scheduleRecomputeForSegment` calls at lines 440, 468, and 1027 — both feature flags and experiments need recomputing when shared segment members change. + +--- + +## Step 7 — Startup backfill + +**`src/init/seed/backfillPrecomputedExperimentSegments.ts`**: + +```ts +import { PrecomputedSegmentService } from '../../api/services/PrecomputedSegmentService'; +import { UpgradeLogger } from '../../lib/logger/UpgradeLogger'; +import Container from 'typedi'; + +export async function backfillPrecomputedExperimentSegments(logger: UpgradeLogger): Promise { + const precomputedSegmentService = Container.get(PrecomputedSegmentService); + await precomputedSegmentService.backfillMissingExperiments(logger); +} +``` + +**`src/app.ts`** — add after the existing `backfillPrecomputedSegments` call: + +```ts +.then(() => { + return backfillPrecomputedExperimentSegments(logger); +}); +``` + +--- + +## Step 8 — Update CLAUDE.md + +Add a matching section to `packages/backend/CLAUDE.md` documenting the experiment precomputed segment pattern (mirror the feature flag section already there). + +--- + +## Checklist + +- [ ] Step 1: `PrecomputedExperimentSegment` entity +- [ ] Step 2: `PrecomputedExperimentSegmentRepository` +- [ ] Step 3: Migration generated and verified +- [ ] Step 4: `PrecomputedSegmentService` experiment methods + `CACHE_PREFIX` constant +- [ ] Step 5: `ExperimentAssignmentService` read path refactored +- [ ] Step 6: `ExperimentService` write triggers added +- [ ] Step 6b: `SegmentService` triggers extended for experiments (lines 440, 468, 1027) +- [ ] Step 7: Startup backfill wired into `app.ts` +- [ ] Step 8: CLAUDE.md updated +- [ ] Typecheck passes +- [ ] Migration runs cleanly +- [ ] Manual smoke test: pre-existing experiment with segment lists shows correct assignment after restart diff --git a/.gitignore b/.gitignore index 3c398197d8..73449c09b4 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,7 @@ .DS_Store .dccache node_modules +.claude* .vscode .claude setup_upgrade.sh diff --git a/clientlibs/js/src/ApiService/ApiService.spec.ts b/clientlibs/js/src/ApiService/ApiService.spec.ts index cb42ebf3e5..6fe698e533 100644 --- a/clientlibs/js/src/ApiService/ApiService.spec.ts +++ b/clientlibs/js/src/ApiService/ApiService.spec.ts @@ -1,4 +1,10 @@ -import { CaliperEnvelope, EXPERIMENT_TYPE, ILogRequestBody, MARKED_DECISION_POINT_STATUS, PAYLOAD_TYPE } from 'upgrade_types'; +import { + CaliperEnvelope, + EXPERIMENT_TYPE, + ILogRequestBody, + MARKED_DECISION_POINT_STATUS, + PAYLOAD_TYPE, +} from 'upgrade_types'; import ApiService from './ApiService'; import { UpGradeClientInterfaces } from './../types/Interfaces'; import { UpGradeClientRequests } from './../types/requests'; @@ -263,7 +269,14 @@ describe('ApiService', () => { const mockAssignment = { site: 'testSite', target: 'testTarget', - assignedCondition: [{ conditionCode: 'original_condition', payload: { type: PAYLOAD_TYPE.STRING, value: 'val' }, id: 'id1', experimentId: 'exp1' }], + assignedCondition: [ + { + conditionCode: 'original_condition', + payload: { type: PAYLOAD_TYPE.STRING, value: 'val' }, + id: 'id1', + experimentId: 'exp1', + }, + ], experimentType: EXPERIMENT_TYPE.SIMPLE, }; diff --git a/clientlibs/js/src/UpGradeClient/UpgradeClient.spec.ts b/clientlibs/js/src/UpGradeClient/UpgradeClient.spec.ts index 2f2d3d93c1..01ccba7894 100644 --- a/clientlibs/js/src/UpGradeClient/UpgradeClient.spec.ts +++ b/clientlibs/js/src/UpGradeClient/UpgradeClient.spec.ts @@ -284,7 +284,12 @@ describe('UpgradeClient', () => { }); it('should call apiService markDecisionPoint using positional arguments', async () => { - await upgradeClient.markDecisionPoint('testSite', 'testTarget', 'variant_x', MARKED_DECISION_POINT_STATUS.CONDITION_APPLIED); + await upgradeClient.markDecisionPoint( + 'testSite', + 'testTarget', + 'variant_x', + MARKED_DECISION_POINT_STATUS.CONDITION_APPLIED + ); expect(ApiService.prototype.markDecisionPoint).toHaveBeenCalledWith({ site: 'testSite', @@ -295,7 +300,6 @@ describe('UpgradeClient', () => { clientError: undefined, }); }); - }); describe('#hasFeatureFlag', () => { diff --git a/clientlibs/js/src/UpGradeClient/generateUUID.spec.ts b/clientlibs/js/src/UpGradeClient/generateUUID.spec.ts index 0f366061e1..363ba8385d 100644 --- a/clientlibs/js/src/UpGradeClient/generateUUID.spec.ts +++ b/clientlibs/js/src/UpGradeClient/generateUUID.spec.ts @@ -83,4 +83,3 @@ describe('generateUUID (via constructor clientSessionId)', () => { expect(sessionId).toBe(providedId); }); }); - diff --git a/packages/backend/CLAUDE.md b/packages/backend/CLAUDE.md index 6ba35867cc..3d3ccc7097 100644 --- a/packages/backend/CLAUDE.md +++ b/packages/backend/CLAUDE.md @@ -74,3 +74,42 @@ Available at `/swagger` when `SWAGGER_ENABLED=true`. Auto-generated from JSDoc ` ## Test Coverage Target ~49% current, 80% goal. Tests live in `test/` (not alongside source files). + +## Precomputed Segment Lists (Feature Flags) + +Segment inclusion/exclusion for **feature flags** is precomputed and stored flat in the `feature_flag_precomputed_segment` table rather than resolved on-the-fly at assignment time. + +### How it works + +- **`FeatureFlagPrecomputedSegment` entity** (`src/api/models/FeatureFlagPrecomputedSegment.ts`) — one row per feature flag, columns: `featureFlagId` (PK), `inclusionIds: text[]`, `exclusionIds: text[]`. FK to `feature_flag` with `onDelete: CASCADE`. +- **`FeatureFlagPrecomputedSegmentService`** (`src/api/services/FeatureFlagPrecomputedSegmentService.ts`) — owns all computation and cache logic: + - `recomputeForFlag(flagId)` — flattens all enabled inclusion/exclusion segments (recursive sub-segments) into flat ID arrays and upserts the row. This is the only method that `await`s — callers on write paths never call it directly. + - `scheduleRecomputeForFlags(flagIds[])` — fire-and-forget recompute for a known set of flags (swallows/logs errors). The flag-side counterpart to `scheduleRecomputeForSegment`. + - `scheduleRecomputeForSegment(segmentId)` — fire-and-forget; finds all flags referencing a segment (and its parents) and recomputes each. + - `withRecompute(logger, resolveAffectedFlagIds, work)` — **the wrapper all top-level write methods use.** Resolves affected flag IDs *before* `work`, runs `work` (which must own/commit its own transaction), then fires a fire-and-forget recompute *after* commit. Keeps mutation + recompute in one call so a refactor can't drop the recompute. `work` is never blocked on the recompute. + - `getAffectedFlagIds(segmentId)` — public helper that returns flag IDs affected by a given segment (used as the `resolveAffectedFlagIds` for segment deletes). + - `getPrecomputedSets(flagIds[])` — cache-wrapped batch fetch, returns a `Map`. + - `backfillMissingFlags(logger)` — called at startup; computes rows only for flags that have none yet (no-op once all flags are populated). + - `recomputeAllFlags(logger)` — full refresh of every flag; not called automatically, available for manual recovery. +- **Assignment read path** — `FeatureFlagService.featureFlagLevelInclusionExclusion()` calls `getPrecomputedSets()` and does in-memory `Set.has()` checks. No recursive segment queries at assignment time. +- **Cache** — keyed by `CACHE_PREFIX.PRECOMPUTED_SEGMENT_KEY_PREFIX + flagId`. Invalidated by `recomputeForFlag`. + +### What triggers a recompute + +| Event | Trigger | +|---|---| +| Segment list added to a flag | `FeatureFlagService.addList` → `withRecompute` | +| Segment list removed from a flag | `FeatureFlagService.deleteList` → delegates to `SegmentService.deleteSegment` (which owns the recompute) | +| Segment list members updated on a flag | `FeatureFlagService.updateList` → `withRecompute` | +| Flag context changed (deletes all its lists) | `FeatureFlagService.updateFeatureFlagInDB` → `withRecompute` (recomputes to empty; the segment delete does **not** cascade to the precomputed row) | +| Private list added to a shared segment | `SegmentService.addList` → `scheduleRecomputeForSegment` | +| Private list removed from a shared segment | `SegmentService.deleteList` → `scheduleRecomputeForSegment` | +| Segment members/structure updated | `SegmentService.addSegmentDataWithPipeline` → `scheduleRecomputeForSegment` | +| Segment deleted entirely | `SegmentService.deleteSegment` → `withRecompute` (collects affected flag IDs **before** the delete, recomputes **after** commit) | +| Server startup | `app.ts` → `backfillMissingFlags` — backfills any flag with no row | + +All recomputes triggered from write paths are **fire-and-forget** — no request handler (flag-side or segment-side) ever blocks on a recompute. The `import*` paths are the one exception: they `await recomputeForFlag` so "import complete" means the rows are ready. + +### Key invariant + +The `feature_flag_precomputed_segment` row must always be recomputed **after** the structural change commits, so the flat arrays reflect the new state. For deletions specifically, affected flag IDs must be collected **before** the delete because the join table records are gone afterward. Both halves of this invariant are enforced by `withRecompute` (resolve-before → work → recompute-after), so top-level write methods get the ordering for free rather than hand-rolling it. diff --git a/packages/backend/src/api/controllers/FeatureFlagController.ts b/packages/backend/src/api/controllers/FeatureFlagController.ts index b9851e316b..10c74754d5 100644 --- a/packages/backend/src/api/controllers/FeatureFlagController.ts +++ b/packages/backend/src/api/controllers/FeatureFlagController.ts @@ -30,7 +30,7 @@ import { IdValidator, } from './validators/FeatureFlagValidator'; import { ExperimentUserService } from '../services/ExperimentUserService'; -import { FeatureFlagListValidator } from './validators/FeatureFlagListValidator'; +import { FeatureFlagListValidator, FeatureFlagListStatusValidator } from './validators/FeatureFlagListValidator'; import { Segment } from '../models/Segment'; import { Response } from 'express'; import { UserDTO } from '../DTO/UserDTO'; @@ -214,7 +214,7 @@ export class FeatureFlagsController { @Params({ validate: true }) { id }: IdValidator, @Req() request: AppRequest ): Promise { - return this.featureFlagService.findOne(id, request.logger); + return this.featureFlagService.findOneForDetails(id, request.logger); } /** @@ -630,6 +630,98 @@ export class FeatureFlagsController { return this.featureFlagService.updateList(inclusionList, LIST_FILTER_MODE.INCLUSION, currentUser, request.logger); } + /** + * @swagger + * /flags/inclusionList/{id}/status: + * patch: + * description: Toggle the enabled status of a Feature Flag inclusion list without modifying its members + * consumes: + * - application/json + * parameters: + * - in: path + * name: id + * required: true + * schema: + * type: string + * description: Segment id of the list + * - in: body + * name: status + * description: New enabled status + * schema: + * type: object + * properties: + * enabled: + * type: boolean + * tags: + * - Feature Flags + * produces: + * - application/json + * responses: + * '200': + * description: Feature flag inclusion list status is updated + */ + @Patch('/inclusionList/:id/status') + public async updateInclusionListStatus( + @Params({ validate: true }) { id }: IdValidator, + @Body({ validate: true }) { enabled }: FeatureFlagListStatusValidator, + @CurrentUser() currentUser: UserDTO, + @Req() request: AppRequest + ): Promise { + return this.featureFlagService.updateListStatus( + id, + enabled, + LIST_FILTER_MODE.INCLUSION, + currentUser, + request.logger + ); + } + + /** + * @swagger + * /flags/exclusionList/{id}/status: + * patch: + * description: Toggle the enabled status of a Feature Flag exclusion list without modifying its members + * consumes: + * - application/json + * parameters: + * - in: path + * name: id + * required: true + * schema: + * type: string + * description: Segment id of the list + * - in: body + * name: status + * description: New enabled status + * schema: + * type: object + * properties: + * enabled: + * type: boolean + * tags: + * - Feature Flags + * produces: + * - application/json + * responses: + * '200': + * description: Feature flag exclusion list status is updated + */ + @Patch('/exclusionList/:id/status') + public async updateExclusionListStatus( + @Params({ validate: true }) { id }: IdValidator, + @Body({ validate: true }) { enabled }: FeatureFlagListStatusValidator, + @CurrentUser() currentUser: UserDTO, + @Req() request: AppRequest + ): Promise { + return this.featureFlagService.updateListStatus( + id, + enabled, + LIST_FILTER_MODE.EXCLUSION, + currentUser, + request.logger + ); + } + /** * @swagger * /flags/inclusionList: diff --git a/packages/backend/src/api/controllers/SegmentController.ts b/packages/backend/src/api/controllers/SegmentController.ts index 134147bde2..980ec9488d 100644 --- a/packages/backend/src/api/controllers/SegmentController.ts +++ b/packages/backend/src/api/controllers/SegmentController.ts @@ -412,6 +412,43 @@ export class SegmentController { return segment; } + /** + * @swagger + * /segments/{segmentId}/members: + * get: + * description: Get a segment (including private lists) by id with its full member lists + * tags: + * - Segment + * produces: + * - application/json + * parameters: + * - in: path + * name: segmentId + * description: Segment id + * required: true + * schema: + * type: string + * responses: + * '200': + * description: Get segment with members by id + * schema: + * $ref: '#/definitions/segmentResponse' + * '404': + * description: Segment not found + */ + @Get('/:segmentId/members') + public async getSegmentByIdWithMembers( + @Params({ validate: true }) { segmentId }: IdValidator, + @Req() request: AppRequest + ): Promise { + const segment = await this.segmentService.getSegmentByIdWithMembers(segmentId, request.logger); + if (!segment) { + throw new NotFoundException('Segment not found.'); + } + + return segment; + } + /** * @swagger * /segments/status/{segmentId}: diff --git a/packages/backend/src/api/controllers/validators/FeatureFlagListValidator.ts b/packages/backend/src/api/controllers/validators/FeatureFlagListValidator.ts index 1e9a057fe6..a07afa0d81 100644 --- a/packages/backend/src/api/controllers/validators/FeatureFlagListValidator.ts +++ b/packages/backend/src/api/controllers/validators/FeatureFlagListValidator.ts @@ -18,3 +18,9 @@ export class FeatureFlagListValidator { @Type(() => SegmentInputValidator) public segment: SegmentInputValidator; } + +export class FeatureFlagListStatusValidator { + @IsDefined() + @IsBoolean() + public enabled: boolean; +} diff --git a/packages/backend/src/api/models/FeatureFlagPrecomputedSegment.ts b/packages/backend/src/api/models/FeatureFlagPrecomputedSegment.ts new file mode 100644 index 0000000000..8d2e364549 --- /dev/null +++ b/packages/backend/src/api/models/FeatureFlagPrecomputedSegment.ts @@ -0,0 +1,19 @@ +import { Column, Entity, JoinColumn, ManyToOne, PrimaryColumn } from 'typeorm'; +import { BaseModel } from './base/BaseModel'; +import { FeatureFlag } from './FeatureFlag'; + +@Entity() +export class FeatureFlagPrecomputedSegment extends BaseModel { + @PrimaryColumn('uuid') + public featureFlagId: string; + + @ManyToOne(() => FeatureFlag, { onDelete: 'CASCADE' }) + @JoinColumn({ name: 'featureFlagId' }) + public featureFlag: FeatureFlag; + + @Column('text', { array: true, default: '{}' }) + public inclusionIds: string[]; + + @Column('text', { array: true, default: '{}' }) + public exclusionIds: string[]; +} diff --git a/packages/backend/src/api/models/Segment.ts b/packages/backend/src/api/models/Segment.ts index 38101daa29..c7f098225a 100644 --- a/packages/backend/src/api/models/Segment.ts +++ b/packages/backend/src/api/models/Segment.ts @@ -48,6 +48,10 @@ export class Segment extends BaseModel { @Type(() => GroupForSegment) public groupForSegment: GroupForSegment[]; + // Not persisted; populated via loadRelationCountAndMap for counts-only loads (see findOneForDetails). + public individualForSegmentCount?: number; + public groupForSegmentCount?: number; + @ManyToMany(() => Segment, (segment) => segment.subSegments) @JoinTable({ name: 'segment_for_segment', diff --git a/packages/backend/src/api/repositories/FeatureFlagPrecomputedSegmentRepository.ts b/packages/backend/src/api/repositories/FeatureFlagPrecomputedSegmentRepository.ts new file mode 100644 index 0000000000..8d11152976 --- /dev/null +++ b/packages/backend/src/api/repositories/FeatureFlagPrecomputedSegmentRepository.ts @@ -0,0 +1,22 @@ +import { Repository } from 'typeorm'; +import { EntityRepository } from '../../typeorm-typedi-extensions'; +import { FeatureFlagPrecomputedSegment } from '../models/FeatureFlagPrecomputedSegment'; + +@EntityRepository(FeatureFlagPrecomputedSegment) +export class FeatureFlagPrecomputedSegmentRepository extends Repository { + public async upsertByFlagId(flagId: string, inclusionIds: string[], exclusionIds: string[]): Promise { + await this.createQueryBuilder() + .insert() + .into(FeatureFlagPrecomputedSegment) + .values({ featureFlagId: flagId, inclusionIds, exclusionIds }) + .orUpdate(['inclusionIds', 'exclusionIds', 'updatedAt'], ['featureFlagId']) + .execute(); + } + + public async findByFlagIds(flagIds: string[]): Promise<(FeatureFlagPrecomputedSegment | null)[]> { + if (!flagIds.length) return []; + const rows = await this.createQueryBuilder('ps').where('ps.featureFlagId IN (:...ids)', { ids: flagIds }).getMany(); + const rowsByFlagId = new Map(rows.map((r) => [r.featureFlagId, r])); + return flagIds.map((id) => rowsByFlagId.get(id) ?? null); + } +} diff --git a/packages/backend/src/api/repositories/FeatureFlagRepository.ts b/packages/backend/src/api/repositories/FeatureFlagRepository.ts index 82f348543f..c6a1ea1f29 100644 --- a/packages/backend/src/api/repositories/FeatureFlagRepository.ts +++ b/packages/backend/src/api/repositories/FeatureFlagRepository.ts @@ -135,6 +135,21 @@ export class FeatureFlagRepository extends Repository { return [...includeAllFlags, ...excludeAllFlags]; } + // Minimal projection for getKeys — only id, key, filterMode needed; no segment joins + public async getFlagsForKeys(context: string): Promise[]> { + const result = await this.createQueryBuilder('feature_flag') + .select(['feature_flag.id', 'feature_flag.key', 'feature_flag.filterMode']) + .where('feature_flag.context @> :searchContext', { searchContext: [context] }) + .andWhere('feature_flag.status = :status', { status: FEATURE_FLAG_STATUS.ENABLED }) + .getMany() + .catch((errorMsg: any) => { + const errorMsgString = repositoryError('FeatureFlagRepository', 'getFlagsForKeys', { context }, errorMsg); + throw errorMsgString; + }); + + return result; + } + public async validateUniqueKey(flagDTO: FeatureFlagValidation) { const queryBuilder = this.createQueryBuilder('feature_flag') .where('feature_flag.key = :key', { key: flagDTO.key }) diff --git a/packages/backend/src/api/repositories/GroupForSegmentRepository.ts b/packages/backend/src/api/repositories/GroupForSegmentRepository.ts index d9960607df..34e9f9d64b 100644 --- a/packages/backend/src/api/repositories/GroupForSegmentRepository.ts +++ b/packages/backend/src/api/repositories/GroupForSegmentRepository.ts @@ -28,26 +28,38 @@ export class GroupForSegmentRepository extends Repository { entityManager: EntityManager, logger: UpgradeLogger ): Promise { - const result = await entityManager - .createQueryBuilder() - .insert() - .into(GroupForSegment) - .values(data) - .orIgnore() - .returning('*') - .execute() - .catch((errorMsg: any) => { - const errorMsgString = repositoryError( - 'groupForSegmentRepository', - 'insertGroupForSegment', - { data }, - errorMsg - ); - logger.error(errorMsg); - throw errorMsgString; - }); + if (!data.length) return []; - return result.raw; + // PostgreSQL's wire protocol supports at most 65535 bind parameters per statement. + // GroupForSegment has 3 bound columns (segmentId, groupId, type), so cap at 5000 rows + // per chunk (5000 × 3 = 15000, well under the limit). + const CHUNK_SIZE = 5000; + const results: GroupForSegment[] = []; + + for (let i = 0; i < data.length; i += CHUNK_SIZE) { + const chunk = data.slice(i, i + CHUNK_SIZE); + const result = await entityManager + .createQueryBuilder() + .insert() + .into(GroupForSegment) + .values(chunk) + .orIgnore() + .returning('*') + .execute() + .catch((errorMsg: any) => { + const errorMsgString = repositoryError( + 'groupForSegmentRepository', + 'insertGroupForSegment', + { data: chunk }, + errorMsg + ); + logger.error(errorMsg); + throw errorMsgString; + }); + results.push(...result.raw); + } + + return results; } public async deleteGroupForSegment( diff --git a/packages/backend/src/api/repositories/IndividualForSegmentRepository.ts b/packages/backend/src/api/repositories/IndividualForSegmentRepository.ts index 85030de66c..507ab30323 100644 --- a/packages/backend/src/api/repositories/IndividualForSegmentRepository.ts +++ b/packages/backend/src/api/repositories/IndividualForSegmentRepository.ts @@ -29,26 +29,38 @@ export class IndividualForSegmentRepository extends Repository { - const result = await entityManager - .createQueryBuilder() - .insert() - .into(IndividualForSegment) - .values(data) - .orIgnore() - .returning('*') - .execute() - .catch((errorMsg: any) => { - const errorMsgString = repositoryError( - 'individualForSegmentRepository', - 'insertIndividualForSegment', - { data }, - errorMsg - ); - logger.error(errorMsg); - throw errorMsgString; - }); + if (!data.length) return []; - return result.raw; + // PostgreSQL's wire protocol supports at most 65535 bind parameters per statement. + // IndividualForSegment has 2 bound columns (segmentId, userId), so cap at 5000 rows + // per chunk (5000 × 2 = 10000, well under the limit). + const CHUNK_SIZE = 5000; + const results: IndividualForSegment[] = []; + + for (let i = 0; i < data.length; i += CHUNK_SIZE) { + const chunk = data.slice(i, i + CHUNK_SIZE); + const result = await entityManager + .createQueryBuilder() + .insert() + .into(IndividualForSegment) + .values(chunk) + .orIgnore() + .returning('*') + .execute() + .catch((errorMsg: any) => { + const errorMsgString = repositoryError( + 'individualForSegmentRepository', + 'insertIndividualForSegment', + { data: chunk }, + errorMsg + ); + logger.error(errorMsg); + throw errorMsgString; + }); + results.push(...result.raw); + } + + return results; } public async deleteIndividualForSegment( diff --git a/packages/backend/src/api/repositories/SegmentRepository.ts b/packages/backend/src/api/repositories/SegmentRepository.ts index 2941bf50c7..2759e17efa 100644 --- a/packages/backend/src/api/repositories/SegmentRepository.ts +++ b/packages/backend/src/api/repositories/SegmentRepository.ts @@ -139,6 +139,14 @@ export class SegmentRepository extends Repository { return result.raw; } + public async findParentSegmentIds(segmentId: string): Promise { + const rows = await this.manager.query( + `SELECT "parentSegmentId" FROM "segment_for_segment" WHERE "childSegmentId" = $1`, + [segmentId] + ); + return rows.map((r: { parentSegmentId: string }) => r.parentSegmentId); + } + public async deleteSegments(ids: string[], logger: UpgradeLogger, entityManager?: EntityManager): Promise { const queryRunner = entityManager ? entityManager : this; diff --git a/packages/backend/src/api/services/FeatureFlagPrecomputedSegmentService.ts b/packages/backend/src/api/services/FeatureFlagPrecomputedSegmentService.ts new file mode 100644 index 0000000000..c77fdfb4ba --- /dev/null +++ b/packages/backend/src/api/services/FeatureFlagPrecomputedSegmentService.ts @@ -0,0 +1,249 @@ +import { Service } from 'typedi'; +import { InjectRepository } from '../../typeorm-typedi-extensions'; +import { FeatureFlagPrecomputedSegmentRepository } from '../repositories/FeatureFlagPrecomputedSegmentRepository'; +import { FeatureFlagSegmentInclusionRepository } from '../repositories/FeatureFlagSegmentInclusionRepository'; +import { FeatureFlagSegmentExclusionRepository } from '../repositories/FeatureFlagSegmentExclusionRepository'; +import { FeatureFlagRepository } from '../repositories/FeatureFlagRepository'; +import { SegmentRepository } from '../repositories/SegmentRepository'; +import { FeatureFlagPrecomputedSegment } from '../models/FeatureFlagPrecomputedSegment'; +import { CacheService } from './CacheService'; +import { CACHE_PREFIX } from 'upgrade_types'; +import { UpgradeLogger } from '../../lib/logger/UpgradeLogger'; +import { EntityManager } from 'typeorm'; + +// Group IDs are stored namespaced with their group type in the same flat arrays as bare individual +// user IDs. This (a) prevents a group ID from ever colliding with an individual user ID that happens +// to share the same string, and (b) keeps matching type-aware, matching the experiment / on-the-fly +// resolution path (see ExperimentAssignmentService.inclusionExclusionLogic). Both the write path +// here and the read path in FeatureFlagService MUST compose the key with this same helper. The type +// is recoverable by splitting on the FIRST delimiter — group types (e.g. 'schoolId') never contain +// ':', though group IDs may. +export const PRECOMPUTED_GROUP_DELIMITER = ':'; +export function precomputedGroupKey(type: string, groupId: string): string { + return `${type}${PRECOMPUTED_GROUP_DELIMITER}${groupId}`; +} + +@Service() +export class FeatureFlagPrecomputedSegmentService { + constructor( + @InjectRepository() private precomputedSegmentRepository: FeatureFlagPrecomputedSegmentRepository, + @InjectRepository() private featureFlagSegmentInclusionRepository: FeatureFlagSegmentInclusionRepository, + @InjectRepository() private featureFlagSegmentExclusionRepository: FeatureFlagSegmentExclusionRepository, + @InjectRepository() private featureFlagRepository: FeatureFlagRepository, + @InjectRepository() private segmentRepository: SegmentRepository, + private cacheService: CacheService + ) {} + + public async recomputeForFlag(flagId: string, logger: UpgradeLogger): Promise { + const [inclusionRecords, exclusionRecords] = await Promise.all([ + this.featureFlagSegmentInclusionRepository.find({ + where: { featureFlag: { id: flagId }, enabled: true }, + relations: { segment: true }, + }), + this.featureFlagSegmentExclusionRepository.find({ + where: { featureFlag: { id: flagId }, enabled: true }, + relations: { segment: true }, + }), + ]); + + const inclusionSegmentIds = inclusionRecords.map((r) => r.segment.id); + const exclusionSegmentIds = exclusionRecords.map((r) => r.segment.id); + + const [inclusionIds, exclusionIds] = await Promise.all([ + this.flattenSegmentMembers(inclusionSegmentIds, new Set()), + this.flattenSegmentMembers(exclusionSegmentIds, new Set()), + ]); + + await this.precomputedSegmentRepository.upsertByFlagId( + flagId, + [...new Set(inclusionIds)], + [...new Set(exclusionIds)] + ); + + await this.cacheService.delCache(CACHE_PREFIX.FEATURE_FLAG_PRECOMPUTED_SEGMENT_KEY_PREFIX + flagId); + logger.info({ message: `Recomputed feature_flag_precomputed_segment for flag ${flagId}` }); + } + + // Seed an empty feature_flag_precomputed_segment row for a brand-new flag, inside the flag's own + // creation transaction so the row is atomic with the flag insert. A new flag has no + // segment lists, so empty arrays are the correct initial state. `orIgnore` keeps this a + // no-op if a row somehow already exists. Seeding here guarantees getPrecomputedSets never + // returns a missing row for a freshly created flag (keeps the read-path cache effective). + public async seedEmptyRowForFlag(flagId: string, manager: EntityManager): Promise { + await manager + .createQueryBuilder() + .insert() + .into(FeatureFlagPrecomputedSegment) + .values({ featureFlagId: flagId, inclusionIds: [], exclusionIds: [] }) + .orIgnore() + .execute(); + } + + // Triggered on segment member or structure changes — finds all affected flags and recomputes (fire-and-forget) + public scheduleRecomputeForSegment(segmentId: string, logger: UpgradeLogger): void { + this.collectAffectedFlagIds(segmentId, new Set()) + .then((flagIds) => Promise.all([...flagIds].map((flagId) => this.recomputeForFlag(flagId, logger)))) + .catch((err) => logger.error({ message: `Error in scheduleRecomputeForSegment: ${err}` })); + } + + // Fire-and-forget recompute for a known set of flags — the flag-side counterpart to + // scheduleRecomputeForSegment. Callers on the write path MUST NOT await this: the recompute + // is a read-through cache refresh that can run after the response is returned. Errors are + // swallowed (logged) so an unhandled rejection can never crash the process. + public scheduleRecomputeForFlags(flagIds: string[], logger: UpgradeLogger): void { + Promise.all([...new Set(flagIds)].map((flagId) => this.recomputeForFlag(flagId, logger))).catch((err) => + logger.error({ message: `Error in scheduleRecomputeForFlags: ${err}` }) + ); + } + + // Run a segment/flag-list mutation and guarantee the affected flags' precomputed rows are + // refreshed afterward — without the caller ever awaiting (or having to remember) the recompute. + // + // The ordering contract is enforced here, once, so individual write methods can't get it wrong + // during a later refactor: + // 1. `resolveAffectedFlagIds` runs BEFORE `work`. Required for deletes (once the join rows are + // gone we can no longer discover which flags referenced the segment) and harmless for + // adds/updates (the flags are already known / already attached). + // 2. `work` runs to completion. It MUST own and commit its own transaction: the recompute reads + // through this service's own repositories and cannot see a still-open transaction's writes. + // 3. The recompute is fired fire-and-forget AFTER `work` resolves (post-commit) and is NOT + // awaited, so the HTTP response is never blocked on it. + // + // Because the mutation and its recompute live in a single call, a change to the mutation body + // can't silently drop the recompute — the two can't drift apart. + public async withRecompute( + logger: UpgradeLogger, + resolveAffectedFlagIds: () => string[] | Promise, + work: () => Promise + ): Promise { + const affectedFlagIds = await resolveAffectedFlagIds(); + const result = await work(); + this.scheduleRecomputeForFlags(affectedFlagIds, logger); + return result; + } + + public async getPrecomputedSets(flagIds: string[]): Promise> { + if (!flagIds.length) return new Map(); + + const results = await this.cacheService.wrapFunction( + CACHE_PREFIX.FEATURE_FLAG_PRECOMPUTED_SEGMENT_KEY_PREFIX, + flagIds, + () => this.precomputedSegmentRepository.findByFlagIds(flagIds) + ); + + const map = new Map(); + flagIds.forEach((id, i) => { + if (results[i]) map.set(id, results[i] as FeatureFlagPrecomputedSegment); + }); + return map; + } + + // One-time backfill for all existing flags — call at startup or after migration + public async recomputeAllFlags(logger: UpgradeLogger): Promise { + const flags = await this.featureFlagRepository.find({ select: { id: true } }); + for (const flag of flags) { + try { + await this.recomputeForFlag(flag.id, logger); + } catch (err) { + logger.error({ message: `Failed to recompute feature_flag_precomputed_segment for flag ${flag.id}: ${err}` }); + } + } + logger.info({ message: `Backfill complete: recomputed ${flags.length} flags` }); + } + + // Backfill only flags that have no feature_flag_precomputed_segment row yet — safe to run every startup + public async backfillMissingFlags(logger: UpgradeLogger): Promise { + const [allFlags, existingRows] = await Promise.all([ + this.featureFlagRepository.find({ select: { id: true } }), + this.precomputedSegmentRepository.find({ select: { featureFlagId: true } }), + ]); + + const existingFlagIds = new Set(existingRows.map((r) => r.featureFlagId)); + const missingFlags = allFlags.filter((f) => !existingFlagIds.has(f.id)); + + if (!missingFlags.length) { + logger.info({ message: 'feature_flag_precomputed_segment backfill: all flags already have rows, nothing to do' }); + return; + } + + for (const flag of missingFlags) { + try { + await this.recomputeForFlag(flag.id, logger); + } catch (err) { + logger.error({ message: `Failed to backfill feature_flag_precomputed_segment for flag ${flag.id}: ${err}` }); + } + } + logger.info({ + message: `feature_flag_precomputed_segment backfill complete: computed ${missingFlags.length} of ${allFlags.length} flags`, + }); + } + + private async flattenSegmentMembers(segmentIds: string[], seen: Set): Promise { + const unresolved = segmentIds.filter((id) => !seen.has(id)); + if (!unresolved.length) return []; + + unresolved.forEach((id) => seen.add(id)); + + const segments = await this.segmentRepository + .createQueryBuilder('segment') + .leftJoinAndSelect('segment.individualForSegment', 'individual') + .leftJoinAndSelect('segment.groupForSegment', 'group') + .leftJoinAndSelect('segment.subSegments', 'subSegment') + .where('segment.id IN (:...ids)', { ids: unresolved }) + .getMany(); + + const ids: string[] = []; + const subSegmentIds: string[] = []; + + for (const segment of segments) { + // Individuals are stored bare; groups are namespaced with their type (see precomputedGroupKey). + segment.individualForSegment.forEach((ind) => ids.push(ind.userId)); + segment.groupForSegment.forEach((grp) => ids.push(precomputedGroupKey(grp.type, grp.groupId))); + segment.subSegments.forEach((sub) => { + if (!seen.has(sub.id)) subSegmentIds.push(sub.id); + }); + } + + if (subSegmentIds.length) { + const subIds = await this.flattenSegmentMembers(subSegmentIds, seen); + ids.push(...subIds); + } + + return ids; + } + + public async getAffectedFlagIds(segmentId: string): Promise { + return [...(await this.collectAffectedFlagIds(segmentId, new Set()))]; + } + + private async collectAffectedFlagIds(segmentId: string, visited: Set): Promise> { + if (visited.has(segmentId)) return new Set(); + visited.add(segmentId); + + const [inclusionRecords, exclusionRecords] = await Promise.all([ + this.featureFlagSegmentInclusionRepository.find({ + where: { segment: { id: segmentId } }, + relations: { featureFlag: true }, + }), + this.featureFlagSegmentExclusionRepository.find({ + where: { segment: { id: segmentId } }, + relations: { featureFlag: true }, + }), + ]); + + const flagIds = new Set([ + ...inclusionRecords.map((r) => r.featureFlag.id), + ...exclusionRecords.map((r) => r.featureFlag.id), + ]); + + const parentIds = await this.segmentRepository.findParentSegmentIds(segmentId); + await Promise.all( + parentIds.map(async (parentId) => { + const parentFlagIds = await this.collectAffectedFlagIds(parentId, visited); + parentFlagIds.forEach((id) => flagIds.add(id)); + }) + ); + + return flagIds; + } +} diff --git a/packages/backend/src/api/services/FeatureFlagService.ts b/packages/backend/src/api/services/FeatureFlagService.ts index 6f8c225a7f..823125d67c 100644 --- a/packages/backend/src/api/services/FeatureFlagService.ts +++ b/packages/backend/src/api/services/FeatureFlagService.ts @@ -1,8 +1,11 @@ import { Service } from 'typedi'; import { FeatureFlag } from '../models/FeatureFlag'; import { Segment } from '../models/Segment'; +import { IndividualForSegment } from '../models/IndividualForSegment'; +import { GroupForSegment } from '../models/GroupForSegment'; import { FeatureFlagSegmentInclusion } from '../models/FeatureFlagSegmentInclusion'; import { FeatureFlagSegmentExclusion } from '../models/FeatureFlagSegmentExclusion'; +import { FeatureFlagPrecomputedSegment } from '../models/FeatureFlagPrecomputedSegment'; import { FeatureFlagRepository } from '../repositories/FeatureFlagRepository'; import { FeatureFlagExposureRepository } from '../repositories/FeatureFlagExposureRepository'; import { FeatureFlagSegmentInclusionRepository } from '../repositories/FeatureFlagSegmentInclusionRepository'; @@ -54,6 +57,7 @@ import { SegmentRepository } from '../repositories/SegmentRepository'; import { ExperimentAuditLog } from '../models/ExperimentAuditLog'; import { NotFoundException } from '@nestjs/common/exceptions'; import { CacheService } from './CacheService'; +import { FeatureFlagPrecomputedSegmentService, precomputedGroupKey } from './FeatureFlagPrecomputedSegmentService'; import { SegmentFile, SegmentInputValidator } from '../controllers/validators/SegmentInputValidator'; import dayjs from 'dayjs'; import { getDateRangeNames } from '../repositories/utils/dateQuery'; @@ -71,7 +75,8 @@ export class FeatureFlagService { @InjectDataSource() private dataSource: DataSource, public experimentAssignmentService: ExperimentAssignmentService, public segmentService: SegmentService, - public cacheService: CacheService + public cacheService: CacheService, + public featureFlagPrecomputedSegmentService: FeatureFlagPrecomputedSegmentService ) {} public find(logger: UpgradeLogger): Promise { @@ -100,9 +105,14 @@ export class FeatureFlagService { throw error; } - const filteredFeatureFlags = await this.getCachedFlagsFromContext(context); + const filteredFeatureFlags = await this.getCachedFlagsForKeys(context); - const includedFeatureFlags = await this.featureFlagLevelInclusionExclusion(filteredFeatureFlags, experimentUserDoc); + const includedFeatureFlags = await this.featureFlagLevelInclusionExclusion( + filteredFeatureFlags, + experimentUserDoc, + context, + logger + ); // save exposures in db if (includedFeatureFlags.length > 0) { @@ -128,9 +138,17 @@ export class FeatureFlagService { return JSON.parse(JSON.stringify(flags)); } + public async getCachedFlagsForKeys(context: string): Promise[]> { + const cacheKey = CACHE_PREFIX.FEATURE_FLAG_KEY_PREFIX + 'keys-' + context; + return this.cacheService.wrap( + cacheKey, + this.featureFlagRepository.getFlagsForKeys.bind(this.featureFlagRepository, context) + ); + } + public async clearCachedFlagsForContext(context: string): Promise { - const cacheKey = CACHE_PREFIX.FEATURE_FLAG_KEY_PREFIX + context; - return this.cacheService.delCache(cacheKey); + await this.cacheService.delCache(CACHE_PREFIX.FEATURE_FLAG_KEY_PREFIX + context); + await this.cacheService.delCache(CACHE_PREFIX.FEATURE_FLAG_KEY_PREFIX + 'keys-' + context); } public async findOne(id: string, logger?: UpgradeLogger): Promise { @@ -156,6 +174,67 @@ export class FeatureFlagService { return featureFlag; } + // Counts-only variant of findOne for the details page: maps member counts instead of loading + // the member lists. Callers that need the actual members (e.g. exports) must use findOne. + public async findOneForDetails(id: string, logger?: UpgradeLogger): Promise { + if (logger) { + logger.info({ message: `Find feature flag (details view) by id => ${id}` }); + } + const featureFlag = await this.featureFlagRepository + .createQueryBuilder('feature_flag') + .leftJoinAndSelect('feature_flag.featureFlagSegmentInclusion', 'featureFlagSegmentInclusion') + .leftJoinAndSelect('featureFlagSegmentInclusion.segment', 'segmentInclusion') + .leftJoinAndSelect('segmentInclusion.subSegments', 'subSegment') + .leftJoinAndSelect('feature_flag.featureFlagSegmentExclusion', 'featureFlagSegmentExclusion') + .leftJoinAndSelect('featureFlagSegmentExclusion.segment', 'segmentExclusion') + .leftJoinAndSelect('segmentExclusion.subSegments', 'subSegmentExclusion') + .where({ id }) + .getOne(); + + if (!featureFlag) { + return undefined; + } + + // loadRelationCountAndMap was removed in TypeORM 1.0; fetch member counts with two batch queries. + const segments = [ + ...(featureFlag.featureFlagSegmentInclusion ?? []).map((r) => r.segment), + ...(featureFlag.featureFlagSegmentExclusion ?? []).map((r) => r.segment), + ].filter(Boolean); + + if (segments.length > 0) { + const segmentIds = segments.map((s) => s.id); + + const [individualCounts, groupCounts] = await Promise.all([ + this.dataSource + .createQueryBuilder() + .select('ifs.segmentId', 'segmentId') + .addSelect('COUNT(*)', 'count') + .from(IndividualForSegment, 'ifs') + .where('ifs.segmentId IN (:...segmentIds)', { segmentIds }) + .groupBy('ifs.segmentId') + .getRawMany<{ segmentId: string; count: string }>(), + this.dataSource + .createQueryBuilder() + .select('gfs.segmentId', 'segmentId') + .addSelect('COUNT(*)', 'count') + .from(GroupForSegment, 'gfs') + .where('gfs.segmentId IN (:...segmentIds)', { segmentIds }) + .groupBy('gfs.segmentId') + .getRawMany<{ segmentId: string; count: string }>(), + ]); + + const individualCountMap = new Map(individualCounts.map((r) => [r.segmentId, Number.parseInt(r.count, 10)])); + const groupCountMap = new Map(groupCounts.map((r) => [r.segmentId, Number.parseInt(r.count, 10)])); + + segments.forEach((segment) => { + segment.individualForSegmentCount = individualCountMap.get(segment.id) ?? 0; + segment.groupForSegmentCount = groupCountMap.get(segment.id) ?? 0; + }); + } + + return featureFlag; + } + public async create( flagDTO: FeatureFlagValidation, currentUser: UserDTO, @@ -253,7 +332,7 @@ export class FeatureFlagService { ): Promise { logger.info({ message: `Delete Feature Flag => ${featureFlagId}` }); return await this.dataSource.transaction(async (transactionalEntityManager) => { - const featureFlag = await this.findOne(featureFlagId, logger); + const featureFlag = await this.findOneForDetails(featureFlagId, logger); if (featureFlag) { await this.clearCachedFlagsForContext(featureFlag.context[0]); @@ -297,7 +376,7 @@ export class FeatureFlagService { } public async updateState(flagId: string, status: FEATURE_FLAG_STATUS, currentUser: UserDTO): Promise { - const oldFeatureFlag = await this.findOne(flagId); + const oldFeatureFlag = await this.findOneForDetails(flagId); await this.clearCachedFlagsForContext(oldFeatureFlag.context[0]); let updatedState: FeatureFlag; try { @@ -404,6 +483,12 @@ export class FeatureFlagService { flagName: featureFlagDoc.name, }; await this.experimentAuditLogRepository.saveRawJson(LOG_TYPE.FEATURE_FLAG_CREATED, createAuditLogData, user); + + // Seed an empty feature_flag_precomputed_segment row in the same transaction so the new flag always + // has a row (no segment lists yet => empty arrays). This keeps the assignment read path + // off the on-the-fly fallback for the common case and keeps the getKeys cache effective. + await this.featureFlagPrecomputedSegmentService.seedEmptyRowForFlag(featureFlagDoc.id, manager); + return featureFlagDoc; }; @@ -427,7 +512,7 @@ export class FeatureFlagService { createdAt, updatedAt, ...oldFlagDoc - } = await this.findOne(flag.id); + } = await this.findOneForDetails(flag.id); let includeList = [...featureFlagSegmentInclusion]; let excludeList = [...featureFlagSegmentExclusion]; @@ -435,7 +520,13 @@ export class FeatureFlagService { const includeListIds = includeList.map((list) => list.segment.id); const excludeListIds = excludeList.map((list) => list.segment.id); - return await this.dataSource.transaction(async (transactionalEntityManager) => { + // A context change (below) deletes all of this flag's inclusion/exclusion lists. That delete does + // NOT cascade to feature_flag_precomputed_segment (its FK is to feature_flag, not segment), so the + // row would otherwise keep stale member IDs. Recompute it (to empty) after commit via withRecompute. + // Non-context updates don't touch lists, so there is nothing to recompute. + const contextChanged = oldFlagDoc.context[0] !== flag.context[0]; + + const applyUpdate = async (transactionalEntityManager: EntityManager) => { const { featureFlagSegmentExclusion, featureFlagSegmentInclusion, @@ -500,7 +591,13 @@ export class FeatureFlagService { featureFlagSegmentInclusion: includeList, featureFlagSegmentExclusion: excludeList, }; - }); + }; + + return this.featureFlagPrecomputedSegmentService.withRecompute( + logger, + () => (contextChanged ? [flag.id] : []), + () => this.dataSource.transaction(applyUpdate) + ); } public async deleteList( @@ -511,6 +608,9 @@ export class FeatureFlagService { ): Promise { await this.createDeleteListAuditLogs([segmentId], filterType, currentUser); await this.cacheService.resetPrefixCache(CACHE_PREFIX.FEATURE_FLAG_KEY_PREFIX); + + // segmentService.deleteSegment collects the affected flags before deletion and fires the + // fire-and-forget recompute itself (via withRecompute), so no separate recompute is needed here. return this.segmentService.deleteSegment(segmentId, logger); } @@ -663,15 +763,24 @@ export class FeatureFlagService { return featureFlagSegmentInclusionOrExclusionArray; }; + let result: (FeatureFlagSegmentInclusion | FeatureFlagSegmentExclusion)[]; if (transactionalEntityManager) { - // Use the provided entity manager - return await executeTransaction(transactionalEntityManager); + // The caller owns the outer transaction. We must NOT recompute here: recomputeForFlag + // reads through its own repositories and cannot see this transaction's uncommitted writes, + // so it would persist an empty/stale feature_flag_precomputed_segment row that never self-heals. The + // caller is responsible for calling recomputeForFlag after its transaction commits. + result = await executeTransaction(transactionalEntityManager); } else { - // Create a new transaction if no entity manager is provided - return await this.dataSource.transaction(async (manager) => { - return await executeTransaction(manager); - }); + // withRecompute runs the mutation in its own transaction, then fires a fire-and-forget + // recompute for the affected flags after commit — the caller never awaits it. + result = await this.featureFlagPrecomputedSegmentService.withRecompute( + logger, + () => [...new Set(listsInput.map((l) => l.id))], + () => this.dataSource.transaction((manager) => executeTransaction(manager)) + ); } + + return result; } public async getExposureStatsByDate( @@ -712,10 +821,10 @@ export class FeatureFlagService { ): Promise { logger.info({ message: `Update ${filterType} list for feature flag` }); await this.cacheService.resetPrefixCache(CACHE_PREFIX.FEATURE_FLAG_KEY_PREFIX); - return await this.dataSource.transaction(async (transactionalEntityManager) => { - // Find the existing record + const doUpdate = async (transactionalEntityManager: EntityManager) => { + // Only the flag id/name are needed here (audit log), so use the counts-only variant. let existingRecord: FeatureFlagSegmentInclusion | FeatureFlagSegmentExclusion; - const featureFlag = await this.findOne(listInput.id); + const featureFlag = await this.findOneForDetails(listInput.id); if (filterType === LIST_FILTER_MODE.INCLUSION) { existingRecord = await this.featureFlagSegmentInclusionRepository.findOne({ @@ -751,12 +860,15 @@ export class FeatureFlagService { const oldSegmentDocClone = JSON.parse(JSON.stringify(oldSegmentDoc)); let newSegmentDocClone; - // Update the segment + // Update the segment. Pass skipScheduleRecompute=true because updateList calls + // recomputeForFlag explicitly after the transaction — firing scheduleRecomputeForSegment + // from inside the transaction risks a stale-read race on the enabled flag. try { const updatedSegment = await this.segmentService.upsertSegmentInPipeline( listInput.segment, logger, - transactionalEntityManager + transactionalEntityManager, + true ); existingRecord.segment = updatedSegment; @@ -823,7 +935,99 @@ export class FeatureFlagService { await this.experimentAuditLogRepository.saveRawJson(LOG_TYPE.FEATURE_FLAG_UPDATED, updateAuditLog, currentUser); return existingRecord; - }); + }; + + // withRecompute runs the update transaction, then fires a fire-and-forget recompute for the + // affected flag after commit — the caller never awaits it. + const result = await this.featureFlagPrecomputedSegmentService.withRecompute( + logger, + () => [listInput.id], + () => this.dataSource.transaction(doUpdate) + ); + + return result; + } + + public async updateListStatus( + segmentId: string, + enabled: boolean, + filterType: LIST_FILTER_MODE, + currentUser: UserDTO, + logger: UpgradeLogger + ): Promise { + logger.info({ message: `Update ${filterType} list status for feature flag => segment ${segmentId}` }); + + let existingRecord: FeatureFlagSegmentInclusion | FeatureFlagSegmentExclusion; + if (filterType === LIST_FILTER_MODE.INCLUSION) { + existingRecord = await this.featureFlagSegmentInclusionRepository.findOne({ + where: { segment: { id: segmentId } }, + relations: { featureFlag: true, segment: true }, + }); + } else { + existingRecord = await this.featureFlagSegmentExclusionRepository.findOne({ + where: { segment: { id: segmentId } }, + relations: { featureFlag: true, segment: true }, + }); + } + + if (!existingRecord) { + const error = new Error(`No existing ${filterType} record found for segment ${segmentId}`); + (error as any).type = SERVER_ERROR.QUERY_FAILED; + logger.error(error); + throw error; + } + + const statusChanged = existingRecord.enabled !== enabled; + + // Route the status flip through withRecompute so the feature_flag_precomputed_segment row is + // refreshed after the change commits. recomputeForFlag only flattens *enabled* inclusion/ + // exclusion lists, so toggling `enabled` changes the precomputed member set even though no + // members are rewritten. When the value is unchanged there is nothing to recompute, so the + // resolver yields no flag ids and withRecompute fires nothing. + return await this.featureFlagPrecomputedSegmentService.withRecompute( + logger, + () => (statusChanged ? [existingRecord.featureFlag.id] : []), + async () => { + existingRecord.enabled = enabled; + + try { + if (filterType === LIST_FILTER_MODE.INCLUSION) { + await this.featureFlagSegmentInclusionRepository.save(existingRecord); + } else { + await this.featureFlagSegmentExclusionRepository.save(existingRecord); + } + } catch (err) { + const error = new Error(`Error in updating ${filterType} list status: ${err}`); + (error as any).type = SERVER_ERROR.QUERY_FAILED; + logger.error(error); + throw error; + } + + await this.clearCachedFlagsForContext(existingRecord.featureFlag.context[0]); + + if (statusChanged) { + const listData: ListOperationsData = { + listId: existingRecord.segment.id, + listName: existingRecord.segment.name, + filterType: filterType, + enabled: enabled, + operation: FEATURE_FLAG_LIST_OPERATION.STATUS_CHANGED, + }; + const updateAuditLog: FeatureFlagUpdatedData = { + flagId: existingRecord.featureFlag.id, + flagName: existingRecord.featureFlag.name, + list: listData, + }; + await this.experimentAuditLogRepository.saveRawJson( + LOG_TYPE.FEATURE_FLAG_UPDATED, + updateAuditLog, + currentUser + ); + } + + return existingRecord; + } + ); } private paginatedSearchString(params: IFeatureFlagSearchParams): string { @@ -880,23 +1084,106 @@ export class FeatureFlagService { } private async featureFlagLevelInclusionExclusion( - featureFlags: FeatureFlag[], - experimentUser: ExperimentUser - ): Promise { - const segmentObjMap = {}; - const getEnabledSegmentIds = (list: FeatureFlagSegmentExclusion[] | FeatureFlagSegmentInclusion[]) => { - return list.filter((item) => item.enabled).map((item) => item.segment.id); - }; + featureFlags: Pick[], + experimentUser: ExperimentUser, + context: string, + logger: UpgradeLogger + ): Promise[]> { + const flagIds = featureFlags.map((f) => f.id); + // getPrecomputedSets can throw if the feature_flag_precomputed_segment table is unavailable + // (e.g. the migration hasn't been run yet). Treat that identically to every row being missing: + // swallow the error and fall through to on-the-fly segment resolution below rather than failing + // the whole assignment request. Rows self-heal on the next restart (backfill) or list mutation. + let precomputedMap: Map; + try { + precomputedMap = await this.featureFlagPrecomputedSegmentService.getPrecomputedSets(flagIds); + } catch (err) { + logger.error({ + message: `featureFlagLevelInclusionExclusion: failed to read feature_flag_precomputed_segment; falling back to on-the-fly resolution for all flags: ${err}`, + }); + precomputedMap = new Map(); + } - featureFlags.forEach((flag) => { - const excludeIds = getEnabledSegmentIds(flag.featureFlagSegmentExclusion); - let includeIds = []; + // Build type-qualified group keys from the user's group map so they match the namespaced group + // IDs stored in the precomputed arrays (individuals are matched bare against experimentUser.id). + // Must use the same precomputedGroupKey helper as the write path. + const userGroupKeys: string[] = experimentUser.group + ? Object.entries(experimentUser.group).flatMap(([type, groupIds]) => + groupIds.map((groupId) => precomputedGroupKey(type, groupId)) + ) + : []; + + // Any flag without a precomputed row falls back to on-the-fly segment resolution so a + // truly-missing row never silently produces a wrong include/exclude decision. Seeding on + // create (and recompute on every list mutation) should make this rare — log it so a + // persistent fallback is visible rather than silently masking a recompute gap. + const missingFlagIds = flagIds.filter((id) => !precomputedMap.has(id)); + const onTheFlyIncludedIds = missingFlagIds.length + ? await this.resolveFlagsOnTheFly(missingFlagIds, context, experimentUser, logger) + : new Set(); + + return featureFlags.filter((flag) => { + const computed = precomputedMap.get(flag.id); + + if (!computed) { + // No precomputed row — fall back to the on-the-fly resolution result for this flag + return onTheFlyIncludedIds.has(flag.id); + } - // this should be fixed upstream also so featureFlagSegmentInclusion is always an empty array already, - // but this will at least catch it here also if something was missed so that we aren't caching or running logic on irrelevant segments - if (flag.filterMode !== FILTER_MODE.INCLUDE_ALL) { - includeIds = getEnabledSegmentIds(flag.featureFlagSegmentInclusion); + const exclusionSet = new Set(computed.exclusionIds); + const inclusionSet = new Set(computed.inclusionIds); + + // Individual exclusion always wins + if (exclusionSet.has(experimentUser.id)) return false; + + // Individual inclusion bypasses group checks + if (inclusionSet.has(experimentUser.id)) return true; + + const inGroupExclusion = userGroupKeys.some((key) => exclusionSet.has(key)); + const inGroupInclusion = userGroupKeys.some((key) => inclusionSet.has(key)); + + if (flag.filterMode === FILTER_MODE.INCLUDE_ALL) { + return !inGroupExclusion; + } else { + // EXCLUDE_ALL: include only if in inclusion group and not in exclusion group + return inGroupInclusion && !inGroupExclusion; } + }); + } + + /** + * Fallback assignment path for flags that have no feature_flag_precomputed_segment row. Resolves segment + * inclusion/exclusion on-the-fly using the same recursive resolution the codebase used before + * precomputed segments (and that experiments still use), preserving full group-type matching. + * Returns the set of flag IDs the user should be included in. + */ + private async resolveFlagsOnTheFly( + missingFlagIds: string[], + context: string, + experimentUser: ExperimentUser, + logger: UpgradeLogger + ): Promise> { + logger.warn({ + message: `featureFlagLevelInclusionExclusion: ${missingFlagIds.length} flag(s) missing a feature_flag_precomputed_segment row; resolving on-the-fly`, + details: { context, missingFlagIds }, + }); + + // Load the full flags (with segment relations) for this context and keep only the missing ones + const missingIdSet = new Set(missingFlagIds); + const fullFlags = (await this.getCachedFlagsFromContext(context)).filter((flag) => missingIdSet.has(flag.id)); + if (!fullFlags.length) { + return new Set(); + } + + const getEnabledSegmentIds = (list: (FeatureFlagSegmentExclusion | FeatureFlagSegmentInclusion)[]) => + (list ?? []).filter((item) => item.enabled).map((item) => item.segment.id); + + const segmentObjMap: Record = {}; + fullFlags.forEach((flag) => { + const excludeIds = getEnabledSegmentIds(flag.featureFlagSegmentExclusion); + // INCLUDE_ALL flags ignore inclusion segments (matches the precomputed-path semantics) + const includeIds = + flag.filterMode !== FILTER_MODE.INCLUDE_ALL ? getEnabledSegmentIds(flag.featureFlagSegmentInclusion) : []; segmentObjMap[flag.id] = { segmentIdsQueue: [...includeIds, ...excludeIds], @@ -907,20 +1194,16 @@ export class FeatureFlagService { }; }); - const featureFlagIdsWithFilter: { id: string; filterMode: FILTER_MODE }[] = featureFlags.map( - ({ id, filterMode }) => ({ id, filterMode }) - ); + const flagIdsWithFilter = fullFlags.map(({ id, filterMode }) => ({ id, filterMode })); const [includeData, excludeData] = await this.experimentAssignmentService.resolveSegmentsForEntities(segmentObjMap); - - const [includedFeatureFlagIds] = await this.experimentAssignmentService.inclusionExclusionLogic( + const [includedFlagIds] = await this.experimentAssignmentService.inclusionExclusionLogic( includeData, excludeData, experimentUser, - featureFlagIdsWithFilter + flagIdsWithFilter ); - const includedFeatureFlags = featureFlags.filter(({ id }) => includedFeatureFlagIds.includes(id)); - return includedFeatureFlags; + return new Set(includedFlagIds); } public async importFeatureFlags( @@ -1078,6 +1361,12 @@ export class FeatureFlagService { }); createdFlags.push(createdFlag); + + // The outer transaction has committed — recompute now (addList skipped it because it ran + // inside the transaction) so the imported enabled lists are reflected in feature_flag_precomputed_segment. + // Unlike the interactive write paths (which fire-and-forget via withRecompute), import intentionally + // awaits so a successful import response means the precomputed rows are already consistent. + await this.featureFlagPrecomputedSegmentService.recomputeForFlag(createdFlag.id, logger); } logger.info({ message: 'Imported feature flags', details: createdFlags }); @@ -1252,7 +1541,7 @@ export class FeatureFlagService { const featureFlagListFile = featureFlagListFiles.find((file) => file.fileName === fileStatus.fileName); return this.segmentService.convertJSONStringToSegInputValFormat(featureFlagListFile.fileContent as string); }); - const featureFlag = await this.findOne(featureFlagId, logger); + const featureFlag = await this.findOneForDetails(featureFlagId, logger); const createdLists: (FeatureFlagSegmentInclusion | FeatureFlagSegmentExclusion)[] = await this.dataSource.transaction(async (transactionalEntityManager) => { @@ -1271,6 +1560,12 @@ export class FeatureFlagService { return await this.addList(listDocs, filterType, currentUser, logger, transactionalEntityManager); }); + // The outer transaction has committed — recompute now (addList skipped it because it ran + // inside the transaction) so the imported lists are reflected in feature_flag_precomputed_segment. + // Unlike the interactive write paths (which fire-and-forget via withRecompute), import intentionally + // awaits so a successful import response means the precomputed rows are already consistent. + await this.featureFlagPrecomputedSegmentService.recomputeForFlag(featureFlagId, logger); + logger.info({ message: 'Imported feature flags', details: createdLists }); fileStatusArray.forEach((fileStatus) => { diff --git a/packages/backend/src/api/services/SegmentService.ts b/packages/backend/src/api/services/SegmentService.ts index 7d500ef047..bebcb74e03 100644 --- a/packages/backend/src/api/services/SegmentService.ts +++ b/packages/backend/src/api/services/SegmentService.ts @@ -37,11 +37,10 @@ import { FeatureFlagSegmentExclusionRepository } from '../repositories/FeatureFl import { FeatureFlagSegmentInclusionRepository } from '../repositories/FeatureFlagSegmentInclusionRepository'; import { getSegmentData, getSegmentsData } from '../controllers/SegmentController'; import { CacheService } from './CacheService'; +import { FeatureFlagPrecomputedSegmentService } from './FeatureFlagPrecomputedSegmentService'; import { isUUID, validate } from 'class-validator'; import { plainToClass } from 'class-transformer'; import path from 'path'; -import { IndividualForSegment } from '../models/IndividualForSegment'; -import { GroupForSegment } from '../models/GroupForSegment'; import { ISegmentSearchParams, ISegmentSortParams } from '../controllers/validators/SegmentPaginatedParamsValidator'; import { ExperimentSegmentExclusion } from 'src/api/models/ExperimentSegmentExclusion'; import { ExperimentSegmentInclusion } from 'src/api/models/ExperimentSegmentInclusion'; @@ -83,7 +82,8 @@ export class SegmentService { private featureFlagSegmentExclusionRepository: FeatureFlagSegmentExclusionRepository, @InjectRepository() private featureFlagSegmentInclusionRepository: FeatureFlagSegmentInclusionRepository, - private cacheService: CacheService + private cacheService: CacheService, + private featureFlagPrecomputedSegmentService: FeatureFlagPrecomputedSegmentService ) {} public async getAllSegments(logger: UpgradeLogger): Promise { @@ -150,6 +150,18 @@ export class SegmentService { return segmentDoc; } + // Like getSegmentById but includes private lists, so a flag/experiment list can be loaded for editing. + public async getSegmentByIdWithMembers(id: string, logger: UpgradeLogger): Promise { + logger.info({ message: `Find segment (including private) with members by id. segmentId: ${id}` }); + return this.segmentRepository + .createQueryBuilder('segment') + .leftJoinAndSelect('segment.individualForSegment', 'individualForSegment') + .leftJoinAndSelect('segment.groupForSegment', 'groupForSegment') + .leftJoinAndSelect('segment.subSegments', 'subSegment') + .where({ id }) + .getOne(); + } + public async getSegmentByIds(ids: string[]): Promise { return this.cacheService.wrapFunction(CACHE_PREFIX.SEGMENT_KEY_PREFIX, ids, async () => { const result = await this.segmentRepository @@ -429,6 +441,9 @@ export class SegmentService { await transactionalEntityManager.getRepository(Segment).save(parentSegment); return createdSegment; }); + + this.featureFlagPrecomputedSegmentService.scheduleRecomputeForSegment(parentSegmentId, logger); + return createdSegment; } @@ -455,6 +470,8 @@ export class SegmentService { return deletedSegmentResponse; }); + this.featureFlagPrecomputedSegmentService.scheduleRecomputeForSegment(parentSegmentId, logger); + // reset cache await this.cacheService.resetPrefixCache(CACHE_PREFIX.SEGMENT_KEY_PREFIX); await this.cacheService.resetPrefixCache(CACHE_PREFIX.GLOBAL_EXCLUDE_SEGMENT_KEY_PREFIX); @@ -465,18 +482,28 @@ export class SegmentService { public upsertSegmentInPipeline( segment: SegmentInputValidator, logger: UpgradeLogger, - transactionalEntityManager: EntityManager + transactionalEntityManager: EntityManager, + skipScheduleRecompute = false ): Promise { logger.info({ message: `Upsert segment => ${JSON.stringify(segment, undefined, 2)}` }); - return this.addSegmentDataWithPipeline(segment, logger, transactionalEntityManager); + return this.addSegmentDataWithPipeline(segment, logger, transactionalEntityManager, skipScheduleRecompute); } public async deleteSegment(id: string, logger: UpgradeLogger): Promise { logger.info({ message: `Delete segment by id. segmentId: ${id}` }); - const manager = this.dataSource; - const deletedSegment = await manager.transaction(async (transactionalEntityManager) => { - return this.deleteSegmentAndPrivateSubsegments(id, logger, transactionalEntityManager); - }); + + // withRecompute collects the affected flags BEFORE the delete (the join rows are gone after), + // runs the delete in its own transaction, then fires a fire-and-forget recompute for those + // flags after commit. Keeping the ordering inside the wrapper means a future refactor of this + // method can't accidentally break it. + const deletedSegment = await this.featureFlagPrecomputedSegmentService.withRecompute( + logger, + () => this.featureFlagPrecomputedSegmentService.getAffectedFlagIds(id), + () => + this.dataSource.transaction((transactionalEntityManager) => + this.deleteSegmentAndPrivateSubsegments(id, logger, transactionalEntityManager) + ) + ); // reset cache await this.cacheService.resetPrefixCache(CACHE_PREFIX.SEGMENT_KEY_PREFIX); @@ -888,39 +915,25 @@ export class SegmentService { async addSegmentDataWithPipeline( segment: SegmentInputValidator, logger: UpgradeLogger, - transactionalEntityManager: EntityManager + transactionalEntityManager: EntityManager, + skipScheduleRecompute = false ): Promise { let segmentDoc: Segment; - let usersToDelete = [], - groupsToDelete = []; if (segment.id) { try { - // get segment by ids - segmentDoc = await transactionalEntityManager.getRepository(Segment).findOne({ - where: { id: segment.id }, - relations: { - individualForSegment: true, - groupForSegment: true, - subSegments: true, - }, - }); - - // delete individual for segment - if (segmentDoc && segmentDoc.individualForSegment && segmentDoc.individualForSegment.length > 0) { - usersToDelete = segmentDoc.individualForSegment.map((individual) => { - return { userId: individual.userId, segment: segment }; - }); - await transactionalEntityManager.getRepository(IndividualForSegment).delete(usersToDelete as any); - } - - // delete group for segment - if (segmentDoc && segmentDoc.groupForSegment && segmentDoc.groupForSegment.length > 0) { - groupsToDelete = segmentDoc.groupForSegment.map((group) => { - return { groupId: group.groupId, type: group.type, segment: segment }; - }); - await transactionalEntityManager.getRepository(GroupForSegment).delete(groupsToDelete as any); - } + // Full replace: clear members with a single delete-by-segmentId per member table. A per-row + // criteria array (the previous approach) expands into a giant OR predicate that is very slow + // for large lists. The delete is cheap even when there are no members, so we skip the + // pre-SELECT that used to load the full member arrays just to decide whether to delete. + await Promise.all([ + this.individualForSegmentRepository.deleteIndividualForSegmentById( + segment.id, + transactionalEntityManager, + logger + ), + this.groupForSegmentRepository.deleteGroupForSegmentById(segment.id, transactionalEntityManager, logger), + ]); } catch (err) { const error = err as ErrorWithType; error.details = 'Error in deleting segment from DB'; @@ -1033,13 +1046,17 @@ export class SegmentService { await this.cacheService.resetPrefixCache(CACHE_PREFIX.SEGMENT_KEY_PREFIX); await this.cacheService.resetPrefixCache(CACHE_PREFIX.GLOBAL_EXCLUDE_SEGMENT_KEY_PREFIX); + // Recompute precomputed sets for all flags that reference this segment (fire-and-forget). + // Skip when the caller already owns an explicit recomputeForFlag after the transaction — + // firing this from inside a transaction risks a stale-read race where the fire-and-forget + // reads the old enabled value and its upsert overwrites the correct post-commit result. + if (!skipScheduleRecompute) { + this.featureFlagPrecomputedSegmentService.scheduleRecomputeForSegment(segmentDoc.id, logger); + } + return transactionalEntityManager.getRepository(Segment).findOne({ where: { id: segmentDoc.id }, - relations: { - individualForSegment: true, - groupForSegment: true, - subSegments: true, - }, + relations: { subSegments: true, individualForSegment: true, groupForSegment: true }, }); } diff --git a/packages/backend/src/app.ts b/packages/backend/src/app.ts index 4e579ef0b5..2eb8f6f3bf 100644 --- a/packages/backend/src/app.ts +++ b/packages/backend/src/app.ts @@ -19,6 +19,7 @@ import { enableMetricFiltering } from './init/seed/EnableMetricFiltering'; import { InitMetrics } from './init/seed/initMetrics'; import { banner } from './lib/banner'; import { createGlobalExcludeSegment } from './init/seed/globalExcludeSegment'; +import { backfillFeatureFlagPrecomputedSegments } from './init/seed/backfillFeatureFlagPrecomputedSegments'; /* * EXPRESS TYPESCRIPT BOILERPLATE @@ -47,4 +48,16 @@ bootstrapMicroframework({ .then(() => { // Create global exclude segment return createGlobalExcludeSegment(logger); + }) + .then(() => { + // Best-effort: if the feature_flag_precomputed_segment table hasn't been migrated yet (or the + // backfill otherwise fails), log and continue instead of crashing startup with an unhandled + // rejection. The assignment read path falls back to on-the-fly segment resolution when a + // precomputed row — or the whole table — is unavailable, so the server stays fully functional; + // rows self-heal on a later restart (backfill) or list mutation (recompute). + return backfillFeatureFlagPrecomputedSegments(logger).catch((err) => { + logger.error({ + message: `feature_flag_precomputed_segment backfill failed at startup; continuing with on-the-fly fallback: ${err}`, + }); + }); }); diff --git a/packages/backend/src/database/migrations/1782926517264-featureFlagPrecomputedSegment.ts b/packages/backend/src/database/migrations/1782926517264-featureFlagPrecomputedSegment.ts new file mode 100644 index 0000000000..caa7464983 --- /dev/null +++ b/packages/backend/src/database/migrations/1782926517264-featureFlagPrecomputedSegment.ts @@ -0,0 +1,25 @@ +import { MigrationInterface, QueryRunner } from 'typeorm'; + +export class FeatureFlagPrecomputedSegment1782926517264 implements MigrationInterface { + name = 'FeatureFlagPrecomputedSegment1782926517264'; + + public async up(queryRunner: QueryRunner): Promise { + await queryRunner.query(` + CREATE TABLE "feature_flag_precomputed_segment" ( + "featureFlagId" uuid NOT NULL, + "inclusionIds" text[] NOT NULL DEFAULT '{}', + "exclusionIds" text[] NOT NULL DEFAULT '{}', + "createdAt" TIMESTAMP NOT NULL DEFAULT now(), + "updatedAt" TIMESTAMP NOT NULL DEFAULT now(), + "versionNumber" integer NOT NULL DEFAULT 1, + CONSTRAINT "PK_feature_flag_precomputed_segment" PRIMARY KEY ("featureFlagId"), + CONSTRAINT "FK_feature_flag_precomputed_segment_feature_flag" + FOREIGN KEY ("featureFlagId") REFERENCES "feature_flag"("id") ON DELETE CASCADE + ) + `); + } + + public async down(queryRunner: QueryRunner): Promise { + await queryRunner.query(`DROP TABLE "feature_flag_precomputed_segment"`); + } +} diff --git a/packages/backend/src/init/seed/backfillFeatureFlagPrecomputedSegments.ts b/packages/backend/src/init/seed/backfillFeatureFlagPrecomputedSegments.ts new file mode 100644 index 0000000000..e945b775c2 --- /dev/null +++ b/packages/backend/src/init/seed/backfillFeatureFlagPrecomputedSegments.ts @@ -0,0 +1,10 @@ +import { FeatureFlagPrecomputedSegmentService } from '../../api/services/FeatureFlagPrecomputedSegmentService'; +import { UpgradeLogger } from '../../lib/logger/UpgradeLogger'; +import Container from 'typedi'; + +export async function backfillFeatureFlagPrecomputedSegments(logger: UpgradeLogger): Promise { + const featureFlagPrecomputedSegmentService = Container.get( + FeatureFlagPrecomputedSegmentService + ); + await featureFlagPrecomputedSegmentService.backfillMissingFlags(logger); +} diff --git a/packages/backend/test/integration/FeatureFlags/FeatureFlagInclusionExclusion.ts b/packages/backend/test/integration/FeatureFlags/FeatureFlagInclusionExclusion.ts index 962365a0ad..292204778f 100644 --- a/packages/backend/test/integration/FeatureFlags/FeatureFlagInclusionExclusion.ts +++ b/packages/backend/test/integration/FeatureFlags/FeatureFlagInclusionExclusion.ts @@ -1,6 +1,7 @@ import { Container } from 'typedi'; import { UpgradeLogger } from '../../../src/lib/logger/UpgradeLogger'; import { FeatureFlagService } from '../../../src/api/services/FeatureFlagService'; +import { SegmentService } from '../../../src/api/services/SegmentService'; import { featureFlag } from '../mockData/featureFlag'; import { experimentUsers } from '../mockData/experimentUsers/index'; import { LIST_FILTER_MODE, SEGMENT_TYPE } from 'upgrade_types'; @@ -119,4 +120,43 @@ export default async function FeatureFlagInclusionExclusionLogic(): Promise(SegmentService); + const inclusionSegmentId = detailsInclusionSegment.id; + const segmentWithMembers = await segmentService.getSegmentByIdWithMembers(inclusionSegmentId, new UpgradeLogger()); + expect(segmentWithMembers).toBeTruthy(); + expect(segmentWithMembers.type).toEqual(SEGMENT_TYPE.PRIVATE); + expect(segmentWithMembers.groupForSegment.length).toEqual(1); + // the plain getSegmentById excludes private lists — which is exactly why /members exists + const publicOnlyLookup = await segmentService.getSegmentById(inclusionSegmentId, new UpgradeLogger()); + expect(publicOnlyLookup).toBeFalsy(); + + // --- updateListStatus: toggles enabled without rewriting the segment's members --- + const toggledRecord = await featureFlagService.updateListStatus( + inclusionSegmentId, + false, + LIST_FILTER_MODE.INCLUSION, + user, + new UpgradeLogger() + ); + expect(toggledRecord.enabled).toEqual(false); + const flagAfterToggle = await featureFlagService.findOne(flag.id, new UpgradeLogger()); + const inclusionAfterToggle = flagAfterToggle.featureFlagSegmentInclusion.find( + (inclusion) => inclusion.segment.id === inclusionSegmentId + ); + expect(inclusionAfterToggle.enabled).toEqual(false); + expect(inclusionAfterToggle.segment.groupForSegment.length).toEqual(1); } diff --git a/packages/backend/test/unit/controllers/FeatureFlagController.test.ts b/packages/backend/test/unit/controllers/FeatureFlagController.test.ts index cc15f375ab..c09f639acc 100644 --- a/packages/backend/test/unit/controllers/FeatureFlagController.test.ts +++ b/packages/backend/test/unit/controllers/FeatureFlagController.test.ts @@ -83,6 +83,14 @@ describe('Feature Flag Controller Testing', () => { .expect(200); }); + test('Get request for /api/flags/id', () => { + return request(app) + .get('/api/flags/' + crypto.randomUUID()) + .set('Accept', 'application/json') + .expect('Content-Type', /json/) + .expect(200); + }); + test('Delete request for /api/flags/id', () => { return request(app) .delete('/api/flags/' + crypto.randomUUID()) @@ -91,6 +99,24 @@ describe('Feature Flag Controller Testing', () => { .expect(200); }); + test('Patch request for /api/flags/inclusionList/id/status', () => { + return request(app) + .patch('/api/flags/inclusionList/' + crypto.randomUUID() + '/status') + .send({ enabled: false }) + .set('Accept', 'application/json') + .expect('Content-Type', /json/) + .expect(200); + }); + + test('Patch request for /api/flags/exclusionList/id/status', () => { + return request(app) + .patch('/api/flags/exclusionList/' + crypto.randomUUID() + '/status') + .send({ enabled: true }) + .set('Accept', 'application/json') + .expect('Content-Type', /json/) + .expect(200); + }); + test('Put request for /api/flags/id', () => { return request(app) .put('/api/flags/' + crypto.randomUUID()) diff --git a/packages/backend/test/unit/controllers/SegmentController.test.ts b/packages/backend/test/unit/controllers/SegmentController.test.ts index cc7a6a0cfa..fa3b6126e8 100644 --- a/packages/backend/test/unit/controllers/SegmentController.test.ts +++ b/packages/backend/test/unit/controllers/SegmentController.test.ts @@ -66,6 +66,14 @@ describe('Segment Controller Testing', () => { .expect(200); }); + test('Get request for /api/segments/:segmentId/members', () => { + return request(app) + .get(`/api/segments/${crypto.randomUUID()}/members`) + .set('Accept', 'application/json') + .expect('Content-Type', /json/) + .expect(200); + }); + test('Get request for /api/segments/status/:segmentId', () => { return request(app) .get(`/api/segments/status/${crypto.randomUUID()}`) diff --git a/packages/backend/test/unit/controllers/mocks/FeatureFlagServiceMock.ts b/packages/backend/test/unit/controllers/mocks/FeatureFlagServiceMock.ts index 91c65265b1..c90f3320ec 100644 --- a/packages/backend/test/unit/controllers/mocks/FeatureFlagServiceMock.ts +++ b/packages/backend/test/unit/controllers/mocks/FeatureFlagServiceMock.ts @@ -59,6 +59,20 @@ export default class FeatureFlagServiceMock { return Promise.resolve([]); } + public findOneForDetails(id: string, logger: UpgradeLogger): Promise> { + return Promise.resolve({}); + } + + public updateListStatus( + segmentId: string, + enabled: boolean, + filterType: string, + currentUser: unknown, + logger: UpgradeLogger + ): Promise> { + return Promise.resolve({}); + } + public validateFeatureFlagContext(flag: FeatureFlag): boolean { return false; } diff --git a/packages/backend/test/unit/controllers/mocks/SegmentServiceMock.ts b/packages/backend/test/unit/controllers/mocks/SegmentServiceMock.ts index cbab6ddd9d..959e29a5da 100644 --- a/packages/backend/test/unit/controllers/mocks/SegmentServiceMock.ts +++ b/packages/backend/test/unit/controllers/mocks/SegmentServiceMock.ts @@ -15,6 +15,10 @@ export default class SegmentServiceMock { return Promise.resolve([]); } + public getSegmentByIdWithMembers(id: string): Promise> { + return Promise.resolve({}); + } + public getSegmentWithStatusById(id: string): Promise<[]> { return Promise.resolve([]); } diff --git a/packages/backend/test/unit/services/FeatureFlagPrecomputedSegmentService.test.ts b/packages/backend/test/unit/services/FeatureFlagPrecomputedSegmentService.test.ts new file mode 100644 index 0000000000..3eeb8f2f60 --- /dev/null +++ b/packages/backend/test/unit/services/FeatureFlagPrecomputedSegmentService.test.ts @@ -0,0 +1,271 @@ +import { FeatureFlagPrecomputedSegmentService } from '../../../src/api/services/FeatureFlagPrecomputedSegmentService'; +import { UpgradeLogger } from '../../../src/lib/logger/UpgradeLogger'; +import { CACHE_PREFIX } from 'upgrade_types'; +import { configureLogger } from '../../utils/logger'; + +const logger = new UpgradeLogger(); + +// Build a segment-repository query-builder mock whose getMany() returns the fixtures +// matching the ids captured from the `.where('segment.id IN (:...ids)', { ids })` call. +// flattenSegmentMembers calls createQueryBuilder() once per recursion level, so each call +// returns a fresh builder that resolves against the shared fixture map. +function makeSegmentRepoMock(fixtures: Record) { + const createQueryBuilder = jest.fn(() => { + let capturedIds: string[] = []; + const qb: any = { + leftJoinAndSelect: jest.fn().mockReturnThis(), + where: jest.fn((_sql: string, params: { ids: string[] }) => { + capturedIds = params.ids; + return qb; + }), + getMany: jest.fn(() => Promise.resolve(capturedIds.map((id) => fixtures[id]).filter(Boolean))), + }; + return qb; + }); + return { createQueryBuilder, findParentSegmentIds: jest.fn().mockResolvedValue([]) }; +} + +describe('FeatureFlagPrecomputedSegmentService', () => { + beforeAll(() => { + configureLogger(); + }); + + let precomputedSegmentRepository: any; + let featureFlagSegmentInclusionRepository: any; + let featureFlagSegmentExclusionRepository: any; + let featureFlagRepository: any; + let segmentRepository: any; + let cacheService: any; + let service: FeatureFlagPrecomputedSegmentService; + + beforeEach(() => { + precomputedSegmentRepository = { + upsertByFlagId: jest.fn().mockResolvedValue(undefined), + findByFlagIds: jest.fn().mockResolvedValue([]), + find: jest.fn().mockResolvedValue([]), + }; + featureFlagSegmentInclusionRepository = { find: jest.fn().mockResolvedValue([]) }; + featureFlagSegmentExclusionRepository = { find: jest.fn().mockResolvedValue([]) }; + featureFlagRepository = { find: jest.fn().mockResolvedValue([]) }; + segmentRepository = makeSegmentRepoMock({}); + cacheService = { + delCache: jest.fn().mockResolvedValue(undefined), + wrapFunction: jest.fn(), + }; + + service = new FeatureFlagPrecomputedSegmentService( + precomputedSegmentRepository, + featureFlagSegmentInclusionRepository, + featureFlagSegmentExclusionRepository, + featureFlagRepository, + segmentRepository, + cacheService + ); + }); + + describe('recomputeForFlag', () => { + it('flattens individual + group members, recurses into sub-segments, dedupes, and upserts', async () => { + // segA -> members u1, g1, and a sub-segment segChild (-> u2). segB (exclusion) -> u3. + segmentRepository = makeSegmentRepoMock({ + segA: { + id: 'segA', + individualForSegment: [{ userId: 'u1' }], + groupForSegment: [{ groupId: 'g1', type: 'schoolId' }], + subSegments: [{ id: 'segChild' }], + }, + segChild: { + id: 'segChild', + individualForSegment: [{ userId: 'u2' }], + groupForSegment: [], + subSegments: [], + }, + segB: { + id: 'segB', + individualForSegment: [{ userId: 'u3' }], + groupForSegment: [], + subSegments: [], + }, + }); + featureFlagSegmentInclusionRepository.find = jest.fn().mockResolvedValue([{ segment: { id: 'segA' } }]); + featureFlagSegmentExclusionRepository.find = jest.fn().mockResolvedValue([{ segment: { id: 'segB' } }]); + + service = new FeatureFlagPrecomputedSegmentService( + precomputedSegmentRepository, + featureFlagSegmentInclusionRepository, + featureFlagSegmentExclusionRepository, + featureFlagRepository, + segmentRepository, + cacheService + ); + + await service.recomputeForFlag('flag1', logger); + + expect(precomputedSegmentRepository.upsertByFlagId).toHaveBeenCalledTimes(1); + const [flagId, inclusionIds, exclusionIds] = precomputedSegmentRepository.upsertByFlagId.mock.calls[0]; + expect(flagId).toEqual('flag1'); + // recursive sub-segment member u2 must be included alongside the direct members; + // group members are namespaced with their type (schoolId:g1), individuals stay bare + expect(inclusionIds.sort()).toEqual(['schoolId:g1', 'u1', 'u2']); + expect(exclusionIds).toEqual(['u3']); + // only enabled lists are queried + expect(featureFlagSegmentInclusionRepository.find).toHaveBeenCalledWith( + expect.objectContaining({ where: { featureFlag: { id: 'flag1' }, enabled: true } }) + ); + // cache for this flag is invalidated + expect(cacheService.delCache).toHaveBeenCalledWith( + CACHE_PREFIX.FEATURE_FLAG_PRECOMPUTED_SEGMENT_KEY_PREFIX + 'flag1' + ); + }); + + it('produces empty arrays when the flag has no enabled lists', async () => { + await service.recomputeForFlag('flag-empty', logger); + + expect(precomputedSegmentRepository.upsertByFlagId).toHaveBeenCalledWith('flag-empty', [], []); + }); + }); + + describe('getAffectedFlagIds (ancestor walk)', () => { + it('includes flags that reference an ANCESTOR (parent) segment of the edited segment', async () => { + // A flag references segParent; segChild is a sub-segment of segParent. Editing segChild + // must mark the flag referencing segParent as affected. + featureFlagSegmentInclusionRepository.find = jest.fn(({ where }: any) => + Promise.resolve(where.segment.id === 'segParent' ? [{ featureFlag: { id: 'flagP' } }] : []) + ); + featureFlagSegmentExclusionRepository.find = jest.fn().mockResolvedValue([]); + segmentRepository.findParentSegmentIds = jest.fn((id: string) => + Promise.resolve(id === 'segChild' ? ['segParent'] : []) + ); + + const affected = await service.getAffectedFlagIds('segChild'); + + expect(affected).toEqual(['flagP']); + expect(segmentRepository.findParentSegmentIds).toHaveBeenCalledWith('segChild'); + }); + + it('does not infinitely recurse on a segment cycle', async () => { + featureFlagSegmentInclusionRepository.find = jest.fn().mockResolvedValue([]); + featureFlagSegmentExclusionRepository.find = jest.fn().mockResolvedValue([]); + // segX <-> segY reference each other as parents + segmentRepository.findParentSegmentIds = jest.fn((id: string) => + Promise.resolve(id === 'segX' ? ['segY'] : ['segX']) + ); + + const affected = await service.getAffectedFlagIds('segX'); + + expect(affected).toEqual([]); + }); + }); + + describe('getPrecomputedSets', () => { + it('returns an empty map for an empty flag id list without hitting the cache', async () => { + const result = await service.getPrecomputedSets([]); + + expect(result.size).toEqual(0); + expect(cacheService.wrapFunction).not.toHaveBeenCalled(); + }); + + it('maps flag ids to rows positionally and skips missing (null) rows', async () => { + const rowA = { featureFlagId: 'fa', inclusionIds: ['u1'], exclusionIds: [] }; + cacheService.wrapFunction = jest.fn().mockResolvedValue([rowA, null]); + + const result = await service.getPrecomputedSets(['fa', 'fb']); + + expect(result.get('fa')).toEqual(rowA); + expect(result.has('fb')).toEqual(false); + }); + }); + + describe('seedEmptyRowForFlag', () => { + it('inserts an empty row through the provided transaction manager (orIgnore)', async () => { + const execute = jest.fn().mockResolvedValue(undefined); + const values = jest.fn().mockReturnThis(); + const orIgnore = jest.fn().mockReturnThis(); + const qb: any = { + insert: jest.fn().mockReturnThis(), + into: jest.fn().mockReturnThis(), + values, + orIgnore, + execute, + }; + const manager: any = { createQueryBuilder: jest.fn(() => qb) }; + + await service.seedEmptyRowForFlag('flag-new', manager); + + expect(values).toHaveBeenCalledWith({ featureFlagId: 'flag-new', inclusionIds: [], exclusionIds: [] }); + expect(orIgnore).toHaveBeenCalled(); + expect(execute).toHaveBeenCalled(); + }); + }); + + describe('backfillMissingFlags', () => { + it('recomputes only flags that have no precomputed row yet', async () => { + featureFlagRepository.find = jest.fn().mockResolvedValue([{ id: 'f1' }, { id: 'f2' }]); + precomputedSegmentRepository.find = jest.fn().mockResolvedValue([{ featureFlagId: 'f1' }]); + const recomputeSpy = jest.spyOn(service, 'recomputeForFlag').mockResolvedValue(undefined); + + await service.backfillMissingFlags(logger); + + expect(recomputeSpy).toHaveBeenCalledTimes(1); + expect(recomputeSpy).toHaveBeenCalledWith('f2', logger); + }); + }); + + describe('withRecompute', () => { + it('resolves affected flag ids BEFORE running work, returns work result, and recomputes after', async () => { + const order: string[] = []; + const resolveAffectedFlagIds = jest.fn(async () => { + order.push('resolve'); + return ['f1']; + }); + const work = jest.fn(async () => { + order.push('work'); + return 'done'; + }); + + const result = await service.withRecompute(logger, resolveAffectedFlagIds, work); + + expect(result).toBe('done'); + expect(order).toEqual(['resolve', 'work']); // resolve strictly before the mutation + expect(resolveAffectedFlagIds).toHaveBeenCalledTimes(1); + expect(work).toHaveBeenCalledTimes(1); + + // the recompute is fired after work; let the fire-and-forget chain settle + await new Promise((r) => setImmediate(r)); + expect(precomputedSegmentRepository.upsertByFlagId).toHaveBeenCalledWith('f1', [], []); + }); + + it('does not await the recompute — resolves even if the recompute never settles', async () => { + // upsertByFlagId never resolves => recomputeForFlag never settles. If withRecompute awaited + // the recompute, this would hang and time out. + precomputedSegmentRepository.upsertByFlagId = jest.fn(() => new Promise(() => undefined)); + + await expect( + service.withRecompute( + logger, + () => ['f1'], + async () => 'ok' + ) + ).resolves.toBe('ok'); + }); + + it('still resolves (and logs) when the fire-and-forget recompute fails', async () => { + precomputedSegmentRepository.upsertByFlagId = jest.fn().mockRejectedValue(new Error('recompute boom')); + const errorSpy = jest.spyOn(logger, 'error'); + + await expect( + service.withRecompute( + logger, + () => ['f1'], + async () => 'ok' + ) + ).resolves.toBe('ok'); + + // let the fire-and-forget .catch settle so the rejection is handled (no unhandled rejection) + await new Promise((r) => setImmediate(r)); + expect(errorSpy).toHaveBeenCalledWith( + expect.objectContaining({ message: expect.stringContaining('scheduleRecomputeForFlags') }) + ); + errorSpy.mockRestore(); + }); + }); +}); diff --git a/packages/backend/test/unit/services/FeatureFlagService.test.ts b/packages/backend/test/unit/services/FeatureFlagService.test.ts index 37df4d3048..0c4452d84a 100644 --- a/packages/backend/test/unit/services/FeatureFlagService.test.ts +++ b/packages/backend/test/unit/services/FeatureFlagService.test.ts @@ -28,6 +28,7 @@ import { ExperimentAssignmentService } from '../../../src/api/services/Experimen import { FeatureFlagValidation } from '../../../src/api/controllers/validators/FeatureFlagValidator'; import { FeatureFlagListValidator } from '../../../src/api/controllers/validators/FeatureFlagListValidator'; import { SegmentService } from '../../../src/api/services/SegmentService'; +import { FeatureFlagPrecomputedSegmentService } from '../../../src/api/services/FeatureFlagPrecomputedSegmentService'; import { FeatureFlagSegmentExclusionRepository } from '../../../src/api/repositories/FeatureFlagSegmentExclusionRepository'; import { FeatureFlagSegmentInclusionRepository } from '../../../src/api/repositories/FeatureFlagSegmentInclusionRepository'; import { FeatureFlagExposureRepository } from '../../../src/api/repositories/FeatureFlagExposureRepository'; @@ -203,11 +204,33 @@ describe('Feature Flag Service Testing', () => { getSegmentByIds: jest.fn().mockResolvedValue([mockSegment]), }, }, + { + provide: FeatureFlagPrecomputedSegmentService, + useValue: { + // Empty map by default => every flag is "missing" a precomputed row, so getKeys + // routes through the on-the-fly fallback (resolveSegmentsForEntities/inclusionExclusionLogic). + // Individual tests override getPrecomputedSets to exercise the fast in-memory path. + getPrecomputedSets: jest.fn().mockResolvedValue(new Map()), + recomputeForFlag: jest.fn().mockResolvedValue(undefined), + seedEmptyRowForFlag: jest.fn().mockResolvedValue(undefined), + scheduleRecomputeForSegment: jest.fn(), + scheduleRecomputeForFlags: jest.fn(), + // Faithful stub: run the resolver + work so the mutation still executes; the real + // wrapper's fire-and-forget recompute behavior is covered in the precompute service's suite. + withRecompute: jest.fn(async (_logger, resolveAffectedFlagIds, work) => { + await resolveAffectedFlagIds(); + return work(); + }), + getAffectedFlagIds: jest.fn().mockResolvedValue([]), + }, + }, { provide: getRepositoryToken(FeatureFlagRepository), useValue: { find: jest.fn().mockResolvedValue(mockFlagArr), findBy: jest.fn().mockResolvedValue(mockFlagArr), + getFlagsForKeys: jest.fn().mockResolvedValue(mockFlagArr), + getFlagsFromContext: jest.fn().mockResolvedValue(mockFlagArr), findOne: jest.fn().mockResolvedValue(mockFlag1), findWithNames: jest.fn().mockResolvedValue(mockFlagArr), findOneById: jest.fn().mockResolvedValue(mockFlag1), @@ -443,11 +466,64 @@ describe('Feature Flag Service Testing', () => { ); }); + it('recomputes the precomputed row when a flag update changes its context (lists are deleted)', async () => { + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); + // old flag has a different context than the incoming mockFlag2 (context: ['context']) and has lists. + // updateFeatureFlagInDB reads the old flag through the counts-only findOneForDetails. + service.findOneForDetails = jest.fn().mockResolvedValue({ + id: mockFlag2.id, + name: 'name', + key: 'key', + description: 'description', + context: ['old-context'], + status: FEATURE_FLAG_STATUS.ENABLED, + featureFlagSegmentInclusion: [{ segment: { id: 'inc-seg' } }], + featureFlagSegmentExclusion: [{ segment: { id: 'exc-seg' } }], + }); + + await service.update(mockFlag2, mockUser1, logger); + + expect(precomputed.withRecompute).toHaveBeenCalled(); + // the resolver targets this flag so its (now empty) row is rebuilt instead of left stale + const [, resolveAffectedFlagIds] = (precomputed.withRecompute as jest.Mock).mock.calls[0]; + expect(await resolveAffectedFlagIds()).toEqual([mockFlag2.id]); + }); + + it('does not recompute the precomputed row when a flag update leaves the context unchanged', async () => { + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); + // old flag has the SAME context as the incoming mockFlag2 (context: ['context']) — lists untouched. + // updateFeatureFlagInDB reads the old flag through the counts-only findOneForDetails. + service.findOneForDetails = jest.fn().mockResolvedValue({ + id: mockFlag2.id, + name: 'name', + key: 'key', + description: 'description', + context: ['context'], + status: FEATURE_FLAG_STATUS.ENABLED, + featureFlagSegmentInclusion: [], + featureFlagSegmentExclusion: [], + }); + + await service.update(mockFlag2, mockUser1, logger); + + // withRecompute still wraps the write, but its resolver yields no flags => no recompute fired + const [, resolveAffectedFlagIds] = (precomputed.withRecompute as jest.Mock).mock.calls[0]; + expect(await resolveAffectedFlagIds()).toEqual([]); + }); + it('should update the flag state', async () => { const results = await service.updateState(mockFlag1.id, FEATURE_FLAG_STATUS.ENABLED, mockUser1); expect(results).toBeTruthy(); }); + it('should use the counts-only fetch (not the full-member findOne) when toggling flag state', async () => { + const detailsSpy = jest.spyOn(service, 'findOneForDetails'); + const findOneSpy = jest.spyOn(service, 'findOne'); + await service.updateState(mockFlag1.id, FEATURE_FLAG_STATUS.ENABLED, mockUser1); + expect(detailsSpy).toHaveBeenCalledWith(mockFlag1.id); + expect(findOneSpy).not.toHaveBeenCalled(); + }); + it('should update the filter mode', async () => { flagRepo.updateFilterMode = jest.fn().mockResolvedValue(mockFlag1); const results = await service.updateFilterMode(mockFlag1.id, FILTER_MODE.EXCLUDE_ALL, mockUser1); @@ -461,7 +537,7 @@ describe('Feature Flag Service Testing', () => { }); it('should return undefined when no flag to delete', async () => { - service.findOne = jest.fn().mockResolvedValue(undefined); + service.findOneForDetails = jest.fn().mockResolvedValue(undefined); const results = await service.delete(mockFlag1.id, mockUser1, logger); expect(results).toEqual(undefined); }); @@ -500,6 +576,95 @@ describe('Feature Flag Service Testing', () => { expect(result).toBeTruthy(); }); + it('should find one flag for the details view', async () => { + const result = await service.findOneForDetails(mockFlag1.id, logger); + expect(result).toEqual(mockFlag1); + }); + + describe('updateListStatus', () => { + it('should update an inclusion list enabled status without rewriting its members', async () => { + const inclusionRepo = module.get(getRepositoryToken(FeatureFlagSegmentInclusionRepository)) as any; + const segmentService = module.get(SegmentService); + inclusionRepo.findOne = jest.fn().mockResolvedValue({ + enabled: false, + featureFlag: { id: mockFlag1.id, name: mockFlag1.name, context: ['context1'] }, + segment: { id: 'segment-1', name: 'list' }, + }); + inclusionRepo.save = jest.fn().mockResolvedValue({}); + mockExperimentAuditLogRepository.saveRawJson.mockClear(); + + const result = await service.updateListStatus('segment-1', true, LIST_FILTER_MODE.INCLUSION, mockUser1, logger); + + expect(result.enabled).toBe(true); + expect(inclusionRepo.save).toHaveBeenCalled(); + // a status-only toggle must NOT re-upsert the segment (which would rewrite all members) + expect(segmentService.upsertSegmentInPipeline).not.toHaveBeenCalled(); + // a status change is recorded in the audit log + expect(mockExperimentAuditLogRepository.saveRawJson).toHaveBeenCalled(); + }); + + it('should update an exclusion list enabled status', async () => { + const exclusionRepo = module.get(getRepositoryToken(FeatureFlagSegmentExclusionRepository)) as any; + exclusionRepo.findOne = jest.fn().mockResolvedValue({ + enabled: true, + featureFlag: { id: mockFlag1.id, name: mockFlag1.name, context: ['context1'] }, + segment: { id: 'segment-2', name: 'list' }, + }); + exclusionRepo.save = jest.fn().mockResolvedValue({}); + + const result = await service.updateListStatus('segment-2', false, LIST_FILTER_MODE.EXCLUSION, mockUser1, logger); + + expect(result.enabled).toBe(false); + expect(exclusionRepo.save).toHaveBeenCalled(); + }); + + it('should throw when no existing list record is found', async () => { + const inclusionRepo = module.get(getRepositoryToken(FeatureFlagSegmentInclusionRepository)) as any; + inclusionRepo.findOne = jest.fn().mockResolvedValue(undefined); + + await expect( + service.updateListStatus('missing-segment', true, LIST_FILTER_MODE.INCLUSION, mockUser1, logger) + ).rejects.toThrow(); + }); + + it('recomputes the affected flag after a status toggle via withRecompute', async () => { + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); + const inclusionRepo = module.get(getRepositoryToken(FeatureFlagSegmentInclusionRepository)) as any; + inclusionRepo.findOne = jest.fn().mockResolvedValue({ + enabled: false, + featureFlag: { id: mockFlag1.id, name: mockFlag1.name, context: ['context1'] }, + segment: { id: 'segment-1', name: 'list' }, + }); + inclusionRepo.save = jest.fn().mockResolvedValue({}); + (precomputed.withRecompute as jest.Mock).mockClear(); + + await service.updateListStatus('segment-1', true, LIST_FILTER_MODE.INCLUSION, mockUser1, logger); + + expect(precomputed.withRecompute).toHaveBeenCalled(); + // the resolver handed to withRecompute yields the affected flag id + const [, resolveAffectedFlagIds] = (precomputed.withRecompute as jest.Mock).mock.calls[0]; + expect(await resolveAffectedFlagIds()).toContain(mockFlag1.id); + }); + + it('does not recompute when the status is unchanged', async () => { + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); + const inclusionRepo = module.get(getRepositoryToken(FeatureFlagSegmentInclusionRepository)) as any; + inclusionRepo.findOne = jest.fn().mockResolvedValue({ + enabled: true, + featureFlag: { id: mockFlag1.id, name: mockFlag1.name, context: ['context1'] }, + segment: { id: 'segment-1', name: 'list' }, + }); + inclusionRepo.save = jest.fn().mockResolvedValue({}); + (precomputed.withRecompute as jest.Mock).mockClear(); + + // toggling to the value it already has => no change, so the resolver yields no flags + await service.updateListStatus('segment-1', true, LIST_FILTER_MODE.INCLUSION, mockUser1, logger); + + const [, resolveAffectedFlagIds] = (precomputed.withRecompute as jest.Mock).mock.calls[0]; + expect(await resolveAffectedFlagIds()).toEqual([]); + }); + }); + it('should import a feature flag from a valid file', async () => { const result = await service.importFeatureFlags( [{ fileName: 'import.json', fileContent: JSON.stringify(mockFlag4) }], @@ -682,4 +847,146 @@ describe('Feature Flag Service Testing', () => { expect(segmentObjMap[flagWithDisabledInclusion.id].currentIncludedSegmentIds).toEqual([]); }); }); + + describe('getKeys - precomputed segment fast path', () => { + const fastFlag = { id: 'fast-flag-id', key: 'fast-key', filterMode: FILTER_MODE.INCLUDE_ALL }; + + it('uses the precomputed set and skips on-the-fly resolution when a row exists', async () => { + const userDoc = { id: 'user123', group: {}, workingGroup: {} } as any; + const experimentAssignmentService = module.get(ExperimentAssignmentService); + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); + + service.cacheService.wrap = jest.fn().mockResolvedValue([fastFlag]); + (precomputed.getPrecomputedSets as jest.Mock).mockResolvedValue( + new Map([[fastFlag.id, { inclusionIds: [], exclusionIds: ['user123'] }]]) + ); + + const result = await service.getKeys(userDoc, 'context1', logger); + + // user123 is individually excluded -> flag filtered out + expect(result).toEqual([]); + // fast path must not fall back to recursive resolution + expect(experimentAssignmentService.resolveSegmentsForEntities).not.toHaveBeenCalled(); + }); + + it('individual inclusion beats group exclusion on the fast path', async () => { + const userDoc = { id: 'user123', group: { classId: ['bad-class'] }, workingGroup: {} } as any; + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); + + service.cacheService.wrap = jest.fn().mockResolvedValue([fastFlag]); + // stored group IDs are namespaced with their type (classId:bad-class); individuals stay bare + (precomputed.getPrecomputedSets as jest.Mock).mockResolvedValue( + new Map([[fastFlag.id, { inclusionIds: ['user123'], exclusionIds: ['classId:bad-class'] }]]) + ); + + const result = await service.getKeys(userDoc, 'context1', logger); + + expect(result).toEqual([fastFlag.key]); + }); + + it('matches a group exclusion only when the group type also matches (type-aware)', async () => { + const excludeFlag = { id: 'ex-flag-id', key: 'ex-key', filterMode: FILTER_MODE.INCLUDE_ALL }; + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); + service.cacheService.wrap = jest.fn().mockResolvedValue([excludeFlag]); + + // User is in group 'grpA' under type 'classId'. The stored exclusion targets 'grpA' under a + // DIFFERENT type ('schoolId'), so with type-aware matching the user is NOT excluded. + const wrongType = { id: 'user123', group: { classId: ['grpA'] }, workingGroup: {} } as any; + (precomputed.getPrecomputedSets as jest.Mock).mockResolvedValue( + new Map([[excludeFlag.id, { inclusionIds: [], exclusionIds: ['schoolId:grpA'] }]]) + ); + expect(await service.getKeys(wrongType, 'context1', logger)).toEqual([excludeFlag.key]); + + // Same group ID under the MATCHING type -> excluded. + const rightType = { id: 'user123', group: { schoolId: ['grpA'] }, workingGroup: {} } as any; + (precomputed.getPrecomputedSets as jest.Mock).mockResolvedValue( + new Map([[excludeFlag.id, { inclusionIds: [], exclusionIds: ['schoolId:grpA'] }]]) + ); + expect(await service.getKeys(rightType, 'context1', logger)).toEqual([]); + }); + + it('does not treat a group ID that collides with the user ID as an individual match', async () => { + // A group named the same string as the user's individual ID is excluded. Because groups are + // namespaced (schoolId:user123) and the individual check is bare (user123), the user must NOT + // be individually excluded — they are only excluded if they actually belong to that group. + const collideFlag = { id: 'col-flag-id', key: 'col-key', filterMode: FILTER_MODE.INCLUDE_ALL }; + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); + service.cacheService.wrap = jest.fn().mockResolvedValue([collideFlag]); + + const userDoc = { id: 'user123', group: {}, workingGroup: {} } as any; + (precomputed.getPrecomputedSets as jest.Mock).mockResolvedValue( + new Map([[collideFlag.id, { inclusionIds: [], exclusionIds: ['schoolId:user123'] }]]) + ); + + // Not in the excluded group -> stays included (INCLUDE_ALL) + expect(await service.getKeys(userDoc, 'context1', logger)).toEqual([collideFlag.key]); + }); + + it('falls back to on-the-fly resolution when the precomputed row is missing', async () => { + const userDoc = { id: 'user123', group: {}, workingGroup: {} } as any; + const experimentAssignmentService = module.get(ExperimentAssignmentService); + const resolveSegmentsSpy = experimentAssignmentService.resolveSegmentsForEntities as jest.Mock; + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); + + service.cacheService.wrap = jest.fn().mockResolvedValue([fastFlag]); + (precomputed.getPrecomputedSets as jest.Mock).mockResolvedValue(new Map()); // no row -> fallback + resolveSegmentsSpy.mockResolvedValue([{}, {}]); + + await service.getKeys(userDoc, 'context1', logger); + + expect(resolveSegmentsSpy).toHaveBeenCalledTimes(1); + }); + + it('falls back to on-the-fly resolution (does not throw) when the precomputed table read fails', async () => { + const userDoc = { id: 'user123', group: {}, workingGroup: {} } as any; + const experimentAssignmentService = module.get(ExperimentAssignmentService); + const resolveSegmentsSpy = experimentAssignmentService.resolveSegmentsForEntities as jest.Mock; + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); + + service.cacheService.wrap = jest.fn().mockResolvedValue([fastFlag]); + // Simulates the table not existing yet (e.g. migration not run): getPrecomputedSets rejects. + (precomputed.getPrecomputedSets as jest.Mock).mockRejectedValue( + new Error('relation "feature_flag_precomputed_segment" does not exist') + ); + resolveSegmentsSpy.mockResolvedValue([{}, {}]); + + // must resolve (not reject) and still route through the on-the-fly fallback + await expect(service.getKeys(userDoc, 'context1', logger)).resolves.toBeDefined(); + expect(resolveSegmentsSpy).toHaveBeenCalledTimes(1); + }); + }); + + describe('precomputed recompute + seed triggers', () => { + it('seeds an empty precomputed row in-transaction when a flag is created', async () => { + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); + flagRepo.insertFeatureFlag = jest.fn().mockResolvedValue([mockFlag1]); + + await service.create(mockFlag2, mockUser1, logger); + + expect(precomputed.seedEmptyRowForFlag).toHaveBeenCalledWith(mockFlag1.id, expect.anything()); + }); + + it('recomputes the affected flag after addList (standalone) via withRecompute', async () => { + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); + + await service.addList([mockList], LIST_FILTER_MODE.INCLUSION, mockUser1, logger); + + expect(precomputed.withRecompute).toHaveBeenCalled(); + // the resolver handed to withRecompute yields the affected flag id + const [, resolveAffectedFlagIds] = (precomputed.withRecompute as jest.Mock).mock.calls[0]; + expect(await resolveAffectedFlagIds()).toEqual([mockList.id]); + }); + + it('recomputes imported flags after the import transaction commits', async () => { + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); + + await service.importFeatureFlags( + [{ fileName: 'import.json', fileContent: JSON.stringify(mockFlag4) }], + mockUser1, + logger + ); + + expect(precomputed.recomputeForFlag).toHaveBeenCalled(); + }); + }); }); diff --git a/packages/backend/test/unit/services/SegmentService.test.ts b/packages/backend/test/unit/services/SegmentService.test.ts index f3042d969e..d40e0a1daa 100644 --- a/packages/backend/test/unit/services/SegmentService.test.ts +++ b/packages/backend/test/unit/services/SegmentService.test.ts @@ -12,6 +12,7 @@ import { ExperimentSegmentInclusionRepository } from '../../../src/api/repositor import { FeatureFlagSegmentExclusionRepository } from '../../../src/api/repositories/FeatureFlagSegmentExclusionRepository'; import { FeatureFlagSegmentInclusionRepository } from '../../../src/api/repositories/FeatureFlagSegmentInclusionRepository'; import { CacheService } from '../../../src/api/services/CacheService'; +import { FeatureFlagPrecomputedSegmentService } from '../../../src/api/services/FeatureFlagPrecomputedSegmentService'; import { ListInputValidator, SegmentFile, @@ -195,6 +196,23 @@ describe('Segment Service Testing', () => { FeatureFlagSegmentInclusionRepository, CacheService, SegmentRepository, + { + provide: FeatureFlagPrecomputedSegmentService, + useValue: { + scheduleRecomputeForSegment: jest.fn(), + scheduleRecomputeForFlags: jest.fn(), + recomputeForFlag: jest.fn().mockResolvedValue(undefined), + getAffectedFlagIds: jest.fn().mockResolvedValue([]), + seedEmptyRowForFlag: jest.fn().mockResolvedValue(undefined), + getPrecomputedSets: jest.fn().mockResolvedValue(new Map()), + // Faithful stub: run the resolver + work so the mutation still executes; the real + // wrapper's fire-and-forget recompute behavior is covered in the precompute service's suite. + withRecompute: jest.fn(async (_logger, resolveAffectedFlagIds, work) => { + await resolveAffectedFlagIds(); + return work(); + }), + }, + }, { provide: getDataSourceToken('default'), useValue: dataSource, @@ -355,6 +373,11 @@ describe('Segment Service Testing', () => { expect(segments).toEqual(seg1); }); + it('should get a segment (including private) with members by id', async () => { + const segment = await service.getSegmentByIdWithMembers(seg1.id, logger); + expect(segment).toEqual(seg1); + }); + it('should get segments by ids', async () => { const segments = await service.getSegmentByIds([seg1.id]); expect(segments).toEqual([seg1]); @@ -513,6 +536,21 @@ describe('Segment Service Testing', () => { expect(segments).toEqual(seg1); }); + it('should clear existing members with a single delete-by-segmentId when editing', async () => { + // Editing is a full replace: members are deleted then re-inserted. The delete must be a + // single "WHERE segmentId = :id" per member table rather than a per-row criteria list. + service.checkIsDuplicateSegmentName = jest.fn().mockResolvedValue(false); + const indivRepo = module.get(getRepositoryToken(IndividualForSegmentRepository)); + const groupRepo = module.get(getRepositoryToken(GroupForSegmentRepository)); + indivRepo.deleteIndividualForSegmentById = jest.fn(); + groupRepo.deleteGroupForSegmentById = jest.fn(); + + await service.upsertSegment(segVal, logger); + + expect(indivRepo.deleteIndividualForSegmentById).toHaveBeenCalledWith(segVal.id, expect.anything(), logger); + expect(groupRepo.deleteGroupForSegmentById).toHaveBeenCalledWith(segVal.id, expect.anything(), logger); + }); + it('should upsert a segment with trimmed whitespace and removed newline or carriage return', async () => { service.checkIsDuplicateSegmentName = jest.fn().mockResolvedValue(false); const segmentWithIdsToCleanUp = new SegmentInputValidator(); @@ -736,6 +774,39 @@ describe('Segment Service Testing', () => { }).rejects.toThrow(err); }); + describe('precomputed segment recompute triggers', () => { + it('delegates the collect-before-mutate-then-recompute ordering for deleteSegment to withRecompute', async () => { + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); + (precomputed.getAffectedFlagIds as jest.Mock).mockResolvedValue(['flagA']); + + await service.deleteSegment(seg1.id, logger); + + expect(precomputed.withRecompute).toHaveBeenCalled(); + // the resolver handed to withRecompute collects the affected flags for this segment + const [, resolveAffectedFlagIds] = (precomputed.withRecompute as jest.Mock).mock.calls[0]; + await expect(resolveAffectedFlagIds()).resolves.toEqual(['flagA']); + expect(precomputed.getAffectedFlagIds).toHaveBeenCalledWith(seg1.id); + }); + + it('schedules a recompute when a list is added to a segment', async () => { + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); + service.upsertSegmentInPipeline = jest.fn().mockResolvedValue(segValSegment); + + await service.addList(listVal, logger); + + expect(precomputed.scheduleRecomputeForSegment).toHaveBeenCalled(); + }); + + it('schedules a recompute when a list is deleted from a segment', async () => { + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); + service.getSegmentById = jest.fn().mockResolvedValue(newSeg); + + await service.deleteList(newList.id, newSeg.id, logger); + + expect(precomputed.scheduleRecomputeForSegment).toHaveBeenCalled(); + }); + }); + it('should find all paginated segments with search string all', async () => { const res = [ { diff --git a/packages/frontend/projects/upgrade/src/app/core/experiments/experiments.service.ts b/packages/frontend/projects/upgrade/src/app/core/experiments/experiments.service.ts index 12e15a88fd..26d06218c5 100644 --- a/packages/frontend/projects/upgrade/src/app/core/experiments/experiments.service.ts +++ b/packages/frontend/projects/upgrade/src/app/core/experiments/experiments.service.ts @@ -23,6 +23,7 @@ import { selectAllExperiment, selectHasInitialExperimentsDataLoaded, selectIsLoadingExperiment, + selectIsLoadingUpsertPrivateSegmentList, selectSelectedExperiment, selectExperimentOverviewDetails, selectSearchExperimentParams, @@ -71,6 +72,7 @@ export class ExperimentService { experiments$: Observable = this.store$.pipe(select(selectAllExperiment)); currentUserEmailAddress$ = this.store$.pipe(select(selectCurrentUserEmail)); isLoadingExperiment$ = this.store$.pipe(select(selectIsLoadingExperiment)); + isLoadingUpsertPrivateSegmentList$ = this.store$.pipe(select(selectIsLoadingUpsertPrivateSegmentList)); selectedExperiment$ = this.store$.pipe(select(selectSelectedExperiment)); selectedExperimentOverviewDetails$ = this.store$.pipe(select(selectExperimentOverviewDetails)); searchParams$ = this.store$.pipe(select(selectSearchExperimentParams)); diff --git a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts index b176ad751e..881d4dad6c 100644 --- a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts +++ b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts @@ -627,6 +627,7 @@ export interface ExperimentState { isLoadingImportExperiment: boolean; isLoadingRewardsSummary: boolean; rewardsSummaries: Record; + isLoadingUpsertPrivateSegmentList?: boolean; } export interface State extends AppState { diff --git a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.reducer.ts b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.reducer.ts index ee3b08cf76..95fb31e96a 100644 --- a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.reducer.ts +++ b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.reducer.ts @@ -30,6 +30,7 @@ export const initialState: ExperimentState = { isLoadingImportExperiment: false, isLoadingRewardsSummary: false, rewardsSummaries: {}, + isLoadingUpsertPrivateSegmentList: false, }; const reducer = createReducer( diff --git a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.selectors.ts b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.selectors.ts index e1aa1045da..102e8bcf39 100644 --- a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.selectors.ts +++ b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.selectors.ts @@ -42,6 +42,11 @@ export const selectAllExperiment = createSelector( export const selectIsLoadingExperiment = createSelector(selectExperimentState, (state) => state.isLoadingExperiment); +export const selectIsLoadingUpsertPrivateSegmentList = createSelector( + selectExperimentState, + (state) => state.isLoadingUpsertPrivateSegmentList +); + export const selectHasInitialExperimentsDataLoaded = createSelector( selectExperimentState, (state) => state.hasInitialExperimentsDataLoaded diff --git a/packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.data.service.ts b/packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.data.service.ts index ad9ec4753f..fc751c59dd 100644 --- a/packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.data.service.ts +++ b/packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.data.service.ts @@ -125,6 +125,11 @@ export class FeatureFlagsDataService { return this.http.delete(url); } + updateInclusionListStatus(segmentId: string, enabled: boolean) { + const url = `${API_ENDPOINTS.addFlagInclusionList}/${segmentId}/status`; + return this.http.patch(url, { enabled }); + } + addExclusionList(list: AddPrivateSegmentListRequest): Observable { const url = API_ENDPOINTS.addFlagExclusionList; return this.http.post(url, list); @@ -140,6 +145,11 @@ export class FeatureFlagsDataService { return this.http.delete(url); } + updateExclusionListStatus(segmentId: string, enabled: boolean) { + const url = `${API_ENDPOINTS.addFlagExclusionList}/${segmentId}/status`; + return this.http.patch(url, { enabled }); + } + fetchFeatureFlagGraphInfo(params: { flagId: string; range: DATE_RANGE; clientOffset: number }) { const url = API_ENDPOINTS.featureFlagGraphInfo; return this.http.post(url, params); diff --git a/packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.service.ts b/packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.service.ts index b958eea94e..64d8324d63 100644 --- a/packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.service.ts +++ b/packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.service.ts @@ -10,6 +10,7 @@ import { selectSearchKey, selectSearchString, selectIsLoadingUpsertFeatureFlag, + selectIsLoadingUpsertPrivateSegmentList, selectIsLoadingUpdateFeatureFlagStatus, selectSelectedFeatureFlag, selectSearchFeatureFlagParams, @@ -61,7 +62,7 @@ export class FeatureFlagsService { isLoadingFeatureFlagDelete$ = this.store$.pipe(select(selectIsLoadingFeatureFlagDelete)); isLoadingImportFeatureFlag$ = this.store$.pipe(select(selectIsLoadingImportFeatureFlag)); isLoadingUpdateFeatureFlagStatus$ = this.store$.pipe(select(selectIsLoadingUpdateFeatureFlagStatus)); - isLoadingUpsertPrivateSegmentList$ = this.store$.pipe(select(selectIsLoadingUpsertFeatureFlag)); + isLoadingUpsertPrivateSegmentList$ = this.store$.pipe(select(selectIsLoadingUpsertPrivateSegmentList)); featureFlags$ = this.store$.pipe(select(selectAllFeatureFlags)); allFeatureFlags$ = this.store$.pipe(select(selectAllFeatureFlagsSortedByDate)); appContexts$ = this.store$.pipe(select(selectAppContexts)); @@ -186,6 +187,10 @@ export class FeatureFlagsService { this.store$.dispatch(FeatureFlagsActions.actionDeleteFeatureFlagInclusionList({ segmentId })); } + updateFeatureFlagInclusionListStatus(segmentId: string, enabled: boolean) { + this.store$.dispatch(FeatureFlagsActions.actionUpdateFeatureFlagInclusionListStatus({ segmentId, enabled })); + } + addFeatureFlagExclusionPrivateSegmentList(list: AddPrivateSegmentListRequest) { this.store$.dispatch(FeatureFlagsActions.actionAddFeatureFlagExclusionList({ list })); } @@ -198,6 +203,10 @@ export class FeatureFlagsService { this.store$.dispatch(FeatureFlagsActions.actionDeleteFeatureFlagExclusionList({ segmentId })); } + updateFeatureFlagExclusionListStatus(segmentId: string, enabled: boolean) { + this.store$.dispatch(FeatureFlagsActions.actionUpdateFeatureFlagExclusionListStatus({ segmentId, enabled })); + } + setGraphRange(range: DATE_RANGE | null, flagId: string, clientOffset: number) { this.store$.dispatch(FeatureFlagsActions.actionSetFeatureFlagGraphRange({ range, flagId, clientOffset })); } diff --git a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.actions.ts b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.actions.ts index 31f07a5081..30d846ac11 100644 --- a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.actions.ts +++ b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.actions.ts @@ -200,6 +200,21 @@ export const actionUpdateFeatureFlagInclusionListFailure = createAction( props<{ error: any }>() ); +export const actionUpdateFeatureFlagInclusionListStatus = createAction( + '[Feature Flags] Update Feature Flag Inclusion List Status', + props<{ segmentId: string; enabled: boolean }>() +); + +export const actionUpdateFeatureFlagInclusionListStatusSuccess = createAction( + '[Feature Flags] Update Feature Flag Inclusion List Status Success', + props<{ segmentId: string; enabled: boolean }>() +); + +export const actionUpdateFeatureFlagInclusionListStatusFailure = createAction( + '[Feature Flags] Update Feature Flag Inclusion List Status Failure', + props<{ error: any }>() +); + export const actionDeleteFeatureFlagInclusionList = createAction( '[Feature Flags] Delete Feature Flag Inclusion List', props<{ segmentId: string }>() @@ -245,6 +260,21 @@ export const actionUpdateFeatureFlagExclusionListFailure = createAction( props<{ error: any }>() ); +export const actionUpdateFeatureFlagExclusionListStatus = createAction( + '[Feature Flags] Update Feature Flag Exclusion List Status', + props<{ segmentId: string; enabled: boolean }>() +); + +export const actionUpdateFeatureFlagExclusionListStatusSuccess = createAction( + '[Feature Flags] Update Feature Flag Exclusion List Status Success', + props<{ segmentId: string; enabled: boolean }>() +); + +export const actionUpdateFeatureFlagExclusionListStatusFailure = createAction( + '[Feature Flags] Update Feature Flag Exclusion List Status Failure', + props<{ error: any }>() +); + export const actionDeleteFeatureFlagExclusionList = createAction( '[Feature Flags] Delete Feature Flag Exclusion List', props<{ segmentId: string }>() diff --git a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.ts b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.ts index 4431795cce..41d2304efe 100644 --- a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.ts +++ b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.ts @@ -2,7 +2,7 @@ import { FeatureFlagsDataService } from '../feature-flags.data.service'; import { Actions, createEffect, ofType } from '@ngrx/effects'; import { Injectable } from '@angular/core'; import * as FeatureFlagsActions from './feature-flags.actions'; -import { catchError, switchMap, map, filter, withLatestFrom, tap, first } from 'rxjs/operators'; +import { catchError, switchMap, mergeMap, map, filter, withLatestFrom, tap, first } from 'rxjs/operators'; import { FeatureFlag, FeatureFlagsPaginationParams, NUMBER_OF_FLAGS } from './feature-flags.model'; import { DATE_RANGE } from '../../experiments/store/experiments.model'; import { Router } from '@angular/router'; @@ -221,6 +221,26 @@ export class FeatureFlagsEffects { ) ); + updateFeatureFlagInclusionListStatus$ = createEffect(() => + this.actions$.pipe( + ofType(FeatureFlagsActions.actionUpdateFeatureFlagInclusionListStatus), + mergeMap(({ segmentId, enabled }) => { + return this.featureFlagsDataService.updateInclusionListStatus(segmentId, enabled).pipe( + map(() => { + this.notificationService.showSuccess( + this.translate.instant('feature-flags.inclusions.update-success.text') + ); + return FeatureFlagsActions.actionUpdateFeatureFlagInclusionListStatusSuccess({ segmentId, enabled }); + }), + catchError((error) => { + this.notificationService.showError(this.translate.instant('feature-flags.inclusions.update-error.text')); + return of(FeatureFlagsActions.actionUpdateFeatureFlagInclusionListStatusFailure({ error })); + }) + ); + }) + ) + ); + deleteFeatureFlagInclusionList$ = createEffect(() => this.actions$.pipe( ofType(FeatureFlagsActions.actionDeleteFeatureFlagInclusionList), @@ -433,6 +453,26 @@ export class FeatureFlagsEffects { ) ); + updateFeatureFlagExclusionListStatus$ = createEffect(() => + this.actions$.pipe( + ofType(FeatureFlagsActions.actionUpdateFeatureFlagExclusionListStatus), + mergeMap(({ segmentId, enabled }) => { + return this.featureFlagsDataService.updateExclusionListStatus(segmentId, enabled).pipe( + map(() => { + this.notificationService.showSuccess( + this.translate.instant('feature-flags.exclusions.update-success.text') + ); + return FeatureFlagsActions.actionUpdateFeatureFlagExclusionListStatusSuccess({ segmentId, enabled }); + }), + catchError((error) => { + this.notificationService.showError(this.translate.instant('feature-flags.exclusions.update-error.text')); + return of(FeatureFlagsActions.actionUpdateFeatureFlagExclusionListStatusFailure({ error })); + }) + ); + }) + ) + ); + setFeatureFlagGraphRange$ = createEffect( () => this.actions$.pipe( diff --git a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.reducer.ts b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.reducer.ts index 3715016d93..0794e7ff10 100644 --- a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.reducer.ts +++ b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.reducer.ts @@ -182,6 +182,10 @@ const reducer = createReducer( }), // Feature Flag Inclusion List Update Actions + on(FeatureFlagsActions.actionUpdateFeatureFlagInclusionList, (state) => ({ + ...state, + isLoadingUpsertPrivateSegmentList: true, + })), on(FeatureFlagsActions.actionUpdateFeatureFlagInclusionListSuccess, (state, { listResponse }) => { const { featureFlag } = listResponse; @@ -203,6 +207,28 @@ const reducer = createReducer( isLoadingUpsertPrivateSegmentList: false, }; }), + on(FeatureFlagsActions.actionUpdateFeatureFlagInclusionListFailure, (state) => ({ + ...state, + isLoadingUpsertPrivateSegmentList: false, + })), + + // Feature Flag Inclusion List Status Toggle Actions + on(FeatureFlagsActions.actionUpdateFeatureFlagInclusionListStatusSuccess, (state, { segmentId, enabled }) => { + const updatedSelectedFlag = state.selectedFlag + ? { + ...state.selectedFlag, + featureFlagSegmentInclusion: + state.selectedFlag.featureFlagSegmentInclusion?.map((inclusion) => + inclusion.segment.id === segmentId ? { ...inclusion, enabled } : inclusion + ) ?? [], + } + : state.selectedFlag; + + return { + ...state, + selectedFlag: updatedSelectedFlag, + }; + }), // Feature Flag Inclusion List Delete Actions on(FeatureFlagsActions.actionDeleteFeatureFlagInclusionList, (state) => ({ @@ -259,6 +285,10 @@ const reducer = createReducer( }), // Feature Flag Exclusion List Update Actions + on(FeatureFlagsActions.actionUpdateFeatureFlagExclusionList, (state) => ({ + ...state, + isLoadingUpsertPrivateSegmentList: true, + })), on(FeatureFlagsActions.actionUpdateFeatureFlagExclusionListSuccess, (state, { listResponse }) => { const { featureFlag } = listResponse; @@ -280,6 +310,28 @@ const reducer = createReducer( isLoadingUpsertPrivateSegmentList: false, }; }), + on(FeatureFlagsActions.actionUpdateFeatureFlagExclusionListFailure, (state) => ({ + ...state, + isLoadingUpsertPrivateSegmentList: false, + })), + + // Feature Flag Exclusion List Status Toggle Actions + on(FeatureFlagsActions.actionUpdateFeatureFlagExclusionListStatusSuccess, (state, { segmentId, enabled }) => { + const updatedSelectedFlag = state.selectedFlag + ? { + ...state.selectedFlag, + featureFlagSegmentExclusion: + state.selectedFlag.featureFlagSegmentExclusion?.map((exclusion) => + exclusion.segment.id === segmentId ? { ...exclusion, enabled } : exclusion + ) ?? [], + } + : state.selectedFlag; + + return { + ...state, + selectedFlag: updatedSelectedFlag, + }; + }), // Feature Flag Exclusion List Delete Actions on(FeatureFlagsActions.actionDeleteFeatureFlagExclusionList, (state) => ({ diff --git a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.selectors.ts b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.selectors.ts index 52bba20bc8..b413120939 100644 --- a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.selectors.ts +++ b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.selectors.ts @@ -48,6 +48,11 @@ export const selectIsLoadingUpsertFeatureFlag = createSelector( (state) => state.isLoadingUpsertFeatureFlag ); +export const selectIsLoadingUpsertPrivateSegmentList = createSelector( + selectFeatureFlagsState, + (state) => state.isLoadingUpsertPrivateSegmentList +); + export const selectDuplicateKeyFound = createSelector(selectFeatureFlagsState, (state) => state.duplicateKeyFound); export const selectSelectedFeatureFlag = createSelector( diff --git a/packages/frontend/projects/upgrade/src/app/core/segments/segments.data.service.ts b/packages/frontend/projects/upgrade/src/app/core/segments/segments.data.service.ts index c0d3a36660..c533fb3d5f 100644 --- a/packages/frontend/projects/upgrade/src/app/core/segments/segments.data.service.ts +++ b/packages/frontend/projects/upgrade/src/app/core/segments/segments.data.service.ts @@ -48,6 +48,12 @@ export class SegmentsDataService { return this.http.get(url); } + // Lazy-loads a list's members for editing; the /members endpoint also returns private lists. + fetchSegmentWithMembersById(id: string): Observable { + const url = `${API_ENDPOINTS.segments}/${id}/members`; + return this.http.get(url); + } + deleteSegment(id: string) { const url = `${API_ENDPOINTS.segments}/${id}`; return this.http.delete(url); diff --git a/packages/frontend/projects/upgrade/src/app/core/segments/segments.service.ts b/packages/frontend/projects/upgrade/src/app/core/segments/segments.service.ts index 8357f49e64..12dd838630 100644 --- a/packages/frontend/projects/upgrade/src/app/core/segments/segments.service.ts +++ b/packages/frontend/projects/upgrade/src/app/core/segments/segments.service.ts @@ -34,6 +34,7 @@ import { AddSegmentRequest, EditPrivateSegmentListRequest, LIST_OPTION_TYPE, + Segment, SegmentInput, SegmentLocalStorageKeys, UpdateSegmentRequest, @@ -122,6 +123,11 @@ export class SegmentsService { this.store$.dispatch(SegmentsActions.actionGetSegmentById({ segmentId })); } + // Lazy-loads a list's members for editing (bypasses the store). + fetchSegmentWithMembersById(segmentId: string): Observable { + return this.segmentsDataService.fetchSegmentWithMembersById(segmentId); + } + refetchCurrentSelectedSegment() { this.selectedSegment$.pipe(take(1)).subscribe((segment) => { if (segment) { diff --git a/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.model.ts b/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.model.ts index aafb3bb37d..bb36019ebb 100644 --- a/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.model.ts +++ b/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.model.ts @@ -107,6 +107,9 @@ export interface Segment { individualForSegment: IndividualForSegment[]; groupForSegment: GroupForSegment[]; subSegments: Segment[]; + // Member counts from counts-only loads (e.g. flag details); undefined when full lists are present. + individualForSegmentCount?: number; + groupForSegmentCount?: number; listType?: MemberTypes | string; type: SEGMENT_TYPE; status: SEGMENT_STATUS; diff --git a/packages/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/pages/feature-flag-details-page/feature-flag-details-page-content/feature-flag-inclusions-section-card/feature-flag-inclusions-section-card.component.ts b/packages/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/pages/feature-flag-details-page/feature-flag-details-page-content/feature-flag-inclusions-section-card/feature-flag-inclusions-section-card.component.ts index 235df40d29..369475e629 100644 --- a/packages/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/pages/feature-flag-details-page/feature-flag-details-page-content/feature-flag-inclusions-section-card/feature-flag-inclusions-section-card.component.ts +++ b/packages/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/pages/feature-flag-details-page/feature-flag-details-page-content/feature-flag-inclusions-section-card/feature-flag-inclusions-section-card.component.ts @@ -6,7 +6,7 @@ import { } from '@shared-component-lib'; import { TranslateModule } from '@ngx-translate/core'; import { CommonModule } from '@angular/common'; -import { IMenuButtonItem, FILTER_MODE, SEGMENT_TYPE } from 'upgrade_types'; +import { IMenuButtonItem, FILTER_MODE } from 'upgrade_types'; import { FeatureFlagInclusionsTableComponent } from './feature-flag-inclusions-table/feature-flag-inclusions-table.component'; import { FeatureFlagsService } from '../../../../../../../core/feature-flags/feature-flags.service'; import { DialogService } from '../../../../../../../shared/services/common-dialog.service'; @@ -21,11 +21,7 @@ import { ParticipantListRowActionEvent, ParticipantListTableRow, } from '../../../../../../../core/feature-flags/store/feature-flags.model'; -import { - EditPrivateSegmentListDetails, - EditPrivateSegmentListRequest, - Segment, -} from '../../../../../../../core/segments/store/segments.model'; +import { Segment } from '../../../../../../../core/segments/store/segments.model'; import { UserPermission } from '../../../../../../../core/auth/store/auth.models'; import { AuthService } from '../../../../../../../core/auth/auth.service'; @@ -164,10 +160,10 @@ export class FeatureFlagInclusionsSectionCardComponent { onRowAction(event: ParticipantListRowActionEvent, flagId: string): void { switch (event.action) { case PARTICIPANT_LIST_ROW_ACTION.ENABLE: - this.onEnableIncludeList(event.rowData, flagId); + this.onEnableIncludeList(event.rowData); break; case PARTICIPANT_LIST_ROW_ACTION.DISABLE: - this.onDisableIncludeList(event.rowData, flagId); + this.onDisableIncludeList(event.rowData); break; case PARTICIPANT_LIST_ROW_ACTION.EDIT: this.onEditIncludeList(event.rowData, flagId); @@ -178,24 +174,24 @@ export class FeatureFlagInclusionsSectionCardComponent { } } - onEnableIncludeList(rowData: ParticipantListTableRow, flagId: string): void { + onEnableIncludeList(rowData: ParticipantListTableRow): void { this.dialogService .openEnableIncludeListModal(rowData.segment.name) .afterClosed() .subscribe((confirmClicked) => { if (confirmClicked) { - this.sendUpdateIncludeListRequest(flagId, true, rowData.listType, rowData.segment); + this.featureFlagService.updateFeatureFlagInclusionListStatus(rowData.segment.id, true); } }); } - onDisableIncludeList(rowData: ParticipantListTableRow, flagId: string): void { + onDisableIncludeList(rowData: ParticipantListTableRow): void { this.dialogService .openDisableIncludeListModal(rowData.segment.name) .afterClosed() .subscribe((confirmClicked) => { if (confirmClicked) { - this.sendUpdateIncludeListRequest(flagId, false, rowData.listType, rowData.segment); + this.featureFlagService.updateFeatureFlagInclusionListStatus(rowData.segment.id, false); } }); } @@ -204,38 +200,6 @@ export class FeatureFlagInclusionsSectionCardComponent { this.dialogService.openFeatureFlagEditIncludeListModal(rowData, rowData.segment.context, flagId); } - sendUpdateIncludeListRequest(flagId: string, enabled: boolean, listType: string, segment: Segment): void { - const list: EditPrivateSegmentListDetails = this.createEditPrivateSegmentListDetails(segment); - - const listRequest: EditPrivateSegmentListRequest = { - id: flagId, - enabled, - listType, - segment: list, - }; - - this.sendUpdateFeatureFlagInclusionRequest(listRequest); - } - - createEditPrivateSegmentListDetails(segment: Segment): EditPrivateSegmentListDetails { - const editPrivateSegmentListDetails: EditPrivateSegmentListDetails = { - id: segment.id, - name: segment.name, - description: segment.description, - context: segment.context, - type: SEGMENT_TYPE.PRIVATE, - userIds: segment.individualForSegment.map((individual) => individual.userId), - groups: segment.groupForSegment, - subSegmentIds: segment.subSegments.map((subSegment) => subSegment.id), - }; - - return editPrivateSegmentListDetails; - } - - sendUpdateFeatureFlagInclusionRequest(request: EditPrivateSegmentListRequest): void { - this.featureFlagService.updateFeatureFlagInclusionPrivateSegmentList(request); - } - onDeleteIncludeList(segment: Segment): void { this.dialogService .openDeleteIncludeListModal(segment.name) diff --git a/packages/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-private-segment-list-modal/upsert-private-segment-list-modal.component.ts b/packages/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-private-segment-list-modal/upsert-private-segment-list-modal.component.ts index 74ada3f6c3..c1fccbf556 100644 --- a/packages/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-private-segment-list-modal/upsert-private-segment-list-modal.component.ts +++ b/packages/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-private-segment-list-modal/upsert-private-segment-list-modal.component.ts @@ -1,4 +1,4 @@ -import { ChangeDetectionStrategy, Component, Inject, ViewChild } from '@angular/core'; +import { ChangeDetectionStrategy, ChangeDetectorRef, Component, Inject, ViewChild } from '@angular/core'; import { CommonModalComponent, CommonTagsInputComponent } from '@shared-component-lib'; import { MAT_DIALOG_DATA, MatDialog, MatDialogRef } from '@angular/material/dialog'; import { CommonModule } from '@angular/common'; @@ -35,7 +35,18 @@ import { UpsertPrivateSegmentListParams, } from '../../../../../core/segments/store/segments.model'; import { MatAutocompleteModule } from '@angular/material/autocomplete'; -import { BehaviorSubject, combineLatestWith, map, Observable, startWith, Subscription, timer } from 'rxjs'; +import { + BehaviorSubject, + catchError, + combineLatest, + combineLatestWith, + EMPTY, + map, + Observable, + startWith, + Subscription, + timer, +} from 'rxjs'; import { SEGMENT_TYPE } from '../../../../../../../../../../types/src'; import isEqual from 'lodash.isequal'; import { FeatureFlagsService } from '../../../../../core/feature-flags/feature-flags.service'; @@ -63,7 +74,18 @@ import { SharedModule } from '../../../../../shared/shared.module'; export class UpsertPrivateSegmentListModalComponent { @ViewChild('typeSelectRef') typeSelectRef: MatSelect; listOptionTypes$: Observable<{ value: string; viewValue: string }[]>; - isLoadingUpsertFeatureFlagList$ = this.featureFlagService.isLoadingUpsertPrivateSegmentList$; + // Disable the primary button while an add/edit is in flight in any of the three stores this + // modal drives (flag/experiment/segment), to prevent double-submits. + isUpsertLoading$ = combineLatest([ + this.featureFlagService.isLoadingUpsertPrivateSegmentList$, + this.experimentService.isLoadingUpsertPrivateSegmentList$, + this.segmentsService.isLoadingSegments$, + ]).pipe(map((loadingFlags) => loadingFlags.some(Boolean))); + // True while the lazy member fetch (for counts-only edit sources) is in flight. Until it + // resolves the form's values control holds only the partial (counts-only) data, so submitting + // would send a full-replacement update that drops the unloaded members. Included in + // isPrimaryButtonDisabled$ to block saving during the fetch. + isLoadingMembers$ = new BehaviorSubject(false); initialFormValues$ = new BehaviorSubject(null); subscriptions = new Subscription(); @@ -86,6 +108,7 @@ export class UpsertPrivateSegmentListModalComponent { private experimentService: ExperimentService, private featureFlagService: FeatureFlagsService, private commonExportHelpersService: CommonExportHelpersService, + private changeDetectorRef: ChangeDetectorRef, public dialogRef: MatDialogRef ) {} @@ -187,13 +210,42 @@ export class UpsertPrivateSegmentListModalComponent { return; } - const values = this.determineValues(sourceList.listType, sourceList.segment); + this.applyEditFormValues(sourceList.listType, sourceList.segment); + + // Lazy-load the full members when the (counts-only) source list didn't include them. + if (this.segmentMembersNeedFetch(sourceList.listType, sourceList.segment)) { + // Block saving until the members load; otherwise a submit could full-replace with partial data. + this.isLoadingMembers$.next(true); + this.subscriptions.add( + this.segmentsService + .fetchSegmentWithMembersById(sourceList.segment.id) + .pipe( + catchError(() => { + // The HTTP interceptor shows the error; close the modal so a partially-loaded list can't be saved. + this.isLoadingMembers$.next(false); + this.closeModal(); + return EMPTY; + }) + ) + .subscribe((segment) => { + if (segment) { + this.applyEditFormValues(sourceList.listType, segment); + this.changeDetectorRef.markForCheck(); + } + this.isLoadingMembers$.next(false); + }) + ); + } + } + + private applyEditFormValues(listType: string, segment: Segment): void { + const values = this.determineValues(listType, segment); const formValue: PrivateSegmentListFormData = { - listType: sourceList.listType as LIST_OPTION_TYPE, - segment: sourceList.segment, + listType: listType as LIST_OPTION_TYPE, + segment, values, - name: sourceList.segment.name, - description: sourceList.segment.description, + name: segment.name, + description: segment.description, }; this.privateSegmentListForm.patchValue(formValue, { emitEvent: false }); @@ -202,17 +254,28 @@ export class UpsertPrivateSegmentListModalComponent { this.initialFormValues$.next(formValue); // Trigger validators after populating the form - this.setValidatorsBasedOnListType(sourceList.listType); + this.setValidatorsBasedOnListType(listType); + } + + // True when members exist (count > 0) but weren't loaded, so they must be fetched before editing. + private segmentMembersNeedFetch(listType: string, segment: Segment): boolean { + if (!segment?.id || listType === LIST_OPTION_TYPE.SEGMENT) { + return false; + } + if (listType === LIST_OPTION_TYPE.INDIVIDUAL) { + return !segment.individualForSegment?.length && (segment.individualForSegmentCount ?? 0) > 0; + } + return !segment.groupForSegment?.length && (segment.groupForSegmentCount ?? 0) > 0; } determineValues(listType: string, segment: Segment): string[] { switch (listType) { case LIST_OPTION_TYPE.INDIVIDUAL: - return segment.individualForSegment.map((individual) => individual.userId); + return segment.individualForSegment?.map((individual) => individual.userId) ?? []; case LIST_OPTION_TYPE.SEGMENT: return []; default: - return segment.groupForSegment.map((group) => group.groupId); + return segment.groupForSegment?.map((group) => group.groupId) ?? []; } } @@ -231,9 +294,12 @@ export class UpsertPrivateSegmentListModalComponent { } listenForPrimaryButtonDisabled() { - this.isPrimaryButtonDisabled$ = this.isLoadingUpsertFeatureFlagList$.pipe( - combineLatestWith(this.isInitialFormValueChanged$), - map(([isLoading, isInitialFormValueChanged]) => isLoading || !isInitialFormValueChanged) + this.isPrimaryButtonDisabled$ = this.isUpsertLoading$.pipe( + combineLatestWith(this.isInitialFormValueChanged$, this.isLoadingMembers$), + map( + ([isLoading, isInitialFormValueChanged, isLoadingMembers]) => + isLoading || isLoadingMembers || !isInitialFormValueChanged + ) ); this.subscriptions.add(this.isPrimaryButtonDisabled$.subscribe()); } diff --git a/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-details-participant-list-table/common-details-participant-list-table.component.html b/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-details-participant-list-table/common-details-participant-list-table.component.html index f9671645da..4a0ee1114e 100644 --- a/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-details-participant-list-table/common-details-participant-list-table.component.html +++ b/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-details-participant-list-table/common-details-participant-list-table.component.html @@ -27,12 +27,7 @@ @if (rowData?.segment && rowData.listType?.toLowerCase() !== memberTypes.SEGMENT.toLowerCase()) { - + {{ getValuesText(rowData) }} } diff --git a/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-details-participant-list-table/common-details-participant-list-table.component.ts b/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-details-participant-list-table/common-details-participant-list-table.component.ts index 1bd7e2e056..ec86904cb3 100644 --- a/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-details-participant-list-table/common-details-participant-list-table.component.ts +++ b/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-details-participant-list-table/common-details-participant-list-table.component.ts @@ -81,8 +81,6 @@ export class CommonDetailsParticipantListTableComponent { ACTIONS: 'segments.global-actions.text', }; - private readonly MAX_TOOLTIP_VALUES = 10; - ngOnInit() { this.displayedColumns = this.tableType === LIST_FILTER_MODE.INCLUSION @@ -94,10 +92,11 @@ export class CommonDetailsParticipantListTableComponent { const listType = rowData.listType; let count: number; + // Prefer the count field (counts-only load); fall back to array length when full lists are present. if (listType?.toLowerCase() === this.memberTypes.INDIVIDUAL.toLowerCase()) { - count = rowData.segment.individualForSegment?.length || 0; + count = rowData.segment.individualForSegmentCount ?? rowData.segment.individualForSegment?.length ?? 0; } else { - count = rowData.segment.groupForSegment?.length || 0; + count = rowData.segment.groupForSegmentCount ?? rowData.segment.groupForSegment?.length ?? 0; } if (count === 0) { @@ -109,24 +108,6 @@ export class CommonDetailsParticipantListTableComponent { } } - getValuesTooltipText(rowData: ParticipantListTableRow): string { - const listType = rowData.listType; - let values: string[]; - - if (listType?.toLowerCase() === this.memberTypes.INDIVIDUAL.toLowerCase()) { - values = rowData.segment.individualForSegment?.map((item) => item.userId) || []; - } else { - values = rowData.segment.groupForSegment?.map((item) => item.groupId) || []; - } - - // Show only first 10 values if there are more - if (values.length > this.MAX_TOOLTIP_VALUES) { - return values.slice(0, this.MAX_TOOLTIP_VALUES).join(', ') + '...'; - } - - return values.join(', '); - } - getFormattedListType(rowData: ParticipantListTableRow): string { const listType = rowData.listType; diff --git a/packages/types/src/Experiment/enums.ts b/packages/types/src/Experiment/enums.ts index e55e3d7d8b..e2206c258c 100644 --- a/packages/types/src/Experiment/enums.ts +++ b/packages/types/src/Experiment/enums.ts @@ -340,6 +340,7 @@ export enum CACHE_PREFIX { GLOBAL_EXCLUDE_SEGMENT_KEY_PREFIX = 'globalExcludeSegment-', MARK_KEY_PREFIX = 'markExperiments-', FEATURE_FLAG_KEY_PREFIX = 'featureFlags-', + FEATURE_FLAG_PRECOMPUTED_SEGMENT_KEY_PREFIX = 'featureFlagPrecomputedSegments-', } export enum STATUS_INDICATOR_CHIP_TYPE {