diff --git a/packages/backend/src/api/Algorithms.ts b/packages/backend/src/api/Algorithms.ts index b93814536..54048d604 100644 --- a/packages/backend/src/api/Algorithms.ts +++ b/packages/backend/src/api/Algorithms.ts @@ -117,7 +117,16 @@ export function buildWithinSubjectOrderedConditions( orderedConditions: IExperimentAssignmentv5['assignedCondition']; orderedFactors: Record[] | null; } { - const baseConditions: IExperimentAssignmentv5['assignedCondition'] = experiment.conditions.map((condition) => ({ + // Order matters here: ORDERED_ROUND_ROBIN rotates this array directly, and the RANDOM / + // RANDOM_ROUND_ROBIN shuffles are seeded, so their output depends on the input sequence too. + // `getValidExperiments` (the assignment read path) does not ORDER BY conditions.order — only + // `findOneExperiment` does — so sort explicitly rather than inheriting whatever order the DB + // returned. Sort a copy: `experiment.conditions` can be the array owned by the in-memory cache. + const orderedExperimentConditions = [...experiment.conditions].sort( + (condition1, condition2) => condition1.order - condition2.order + ); + + const baseConditions: IExperimentAssignmentv5['assignedCondition'] = orderedExperimentConditions.map((condition) => ({ conditionCode: condition.conditionCode, payload: undefined, experimentId: experiment.id, @@ -126,7 +135,7 @@ export function buildWithinSubjectOrderedConditions( const baseFactors: Record[] | null = experiment.type === EXPERIMENT_TYPE.FACTORIAL - ? experiment.conditions.map((condition) => getAssignedFactor(condition, factors)) + ? orderedExperimentConditions.map((condition) => getAssignedFactor(condition, factors)) : null; let assignedData: IExperimentAssignmentv5 = { diff --git a/packages/backend/src/api/services/ExperimentAssignmentService.ts b/packages/backend/src/api/services/ExperimentAssignmentService.ts index a618edb56..790bec91f 100644 --- a/packages/backend/src/api/services/ExperimentAssignmentService.ts +++ b/packages/backend/src/api/services/ExperimentAssignmentService.ts @@ -708,7 +708,7 @@ export class ExperimentAssignmentService { const experiments = previewUser ? await this.experimentRepository.getValidExperimentsWithPreview(context) : await this.experimentService.getCachedValidExperiments(context); - // adding conditionPayloads at the root level instead of inside conditions + // adding conditionPayloads at the root level instead of inside conditions. return experiments.map((exp) => this.experimentService.formattingConditionPayload(exp)); } @@ -2067,7 +2067,8 @@ export class ExperimentAssignmentService { ? `${experiment.id}_${user.id}` : `${experiment.id}_${user.workingGroup?.[experiment.group]}`; - const sortedExperimentCondition = experiment.conditions.sort( + // Make a copy before sorting so we don't mutate the original array + const sortedExperimentCondition = [...experiment.conditions].sort( (condition1, condition2) => condition1.order - condition2.order ); let spec = sortedExperimentCondition.map((condition) => condition.assignmentWeight); @@ -2085,7 +2086,10 @@ export class ExperimentAssignmentService { break; } } - const experimentalCondition = experiment.conditions[randomConditions]; + // Index the sorted copy: `randomConditions` is an index into `spec`, which is derived from it. + // (This used to read `experiment.conditions`, which was equivalent only because the sort above + // mutated that array in place.) + const experimentalCondition = sortedExperimentCondition[randomConditions]; return experimentalCondition; } diff --git a/packages/backend/src/api/services/ExperimentService.ts b/packages/backend/src/api/services/ExperimentService.ts index f3ef511a1..b13d9f959 100644 --- a/packages/backend/src/api/services/ExperimentService.ts +++ b/packages/backend/src/api/services/ExperimentService.ts @@ -302,13 +302,22 @@ export class ExperimentService { }; } + /** + * Returns the valid experiments for a context. + * + * The cache is an in-memory store, so this hands back the same object reference on every hit. That + * used to require a defensive `JSON.parse(JSON.stringify(...))` deep copy on every assignment + * request, because `formattingConditionPayload` mutated what it was given. That function now builds + * new objects instead, so the cached graph is never written to and the per-request copy is gone. + * + * Callers must keep it that way: treat this result, and everything reachable from it, as read-only. + */ public async getCachedValidExperiments(context: string): Promise { const cacheKey = CACHE_PREFIX.EXPERIMENT_KEY_PREFIX + context; - return this.cacheService - .wrap(cacheKey, this.experimentRepository.getValidExperiments.bind(this.experimentRepository, context)) - .then((validExperiment) => { - return JSON.parse(JSON.stringify(validExperiment)); - }); + return this.cacheService.wrap( + cacheKey, + this.experimentRepository.getValidExperiments.bind(this.experimentRepository, context) + ); } public async create( @@ -1898,35 +1907,47 @@ export class ExperimentService { return searchStringConcatenated; } + /** + * Hoists conditionPayloads from conditions (factorial) or decision points (everything else) up to + * the root of the experiment. + * + * This does not mutate `experiment` or anything reachable from it. That matters because the + * assignment read path calls this on experiments handed out by the in-memory cache, which returns + * the same object reference to every request: mutating here would corrupt the cached graph for all + * subsequent requests, which is why this call site previously needed a full deep copy of the + * experiment on every request. Stripped conditions/partitions are rebuilt as new objects instead, + * and the `parentCondition`/`decisionPoint` back-references point at those same rebuilt objects, so + * reference identity within the returned experiment matches what the old in-place version produced. + */ public formattingConditionPayload(experiment: Experiment): Experiment { if (experiment.type === EXPERIMENT_TYPE.FACTORIAL) { const conditionPayload: ConditionPayload[] = []; - experiment.conditions.forEach((condition) => { - const conditionPayloads = condition.conditionPayloads.map((conditionPayload) => { - return { ...conditionPayload, parentCondition: condition }; + const conditions = experiment.conditions.map(({ conditionPayloads, ...rest }) => rest as ExperimentCondition); + + experiment.conditions.forEach((condition, index) => { + (condition.conditionPayloads || []).forEach((payload) => { + conditionPayload.push({ ...payload, parentCondition: conditions[index] }); }); - conditionPayload.push(...conditionPayloads); - delete condition.conditionPayloads; }); - return { ...experiment, conditionPayloads: conditionPayload }; + return { ...experiment, conditions, conditionPayloads: conditionPayload }; } - const { conditions, partitions } = experiment; + const partitions = experiment.partitions.map(({ conditionPayloads, ...rest }) => rest as DecisionPoint); const conditionPayload: ConditionPayload[] = []; - partitions.forEach((partition) => { - const conditionPayloadData = partition.conditionPayloads; - delete partition.conditionPayloads; + experiment.partitions.forEach((partition, index) => { + // Copy before sorting — the source array belongs to the (possibly cached) experiment. + const conditionPayloadData = [...(partition.conditionPayloads || [])]; conditionPayloadData.sort((a, b) => a.parentCondition.order - b.parentCondition.order); conditionPayloadData.forEach((x) => { - if (x && conditions.filter((con) => con.id === x.parentCondition.id).length > 0) { - conditionPayload.push({ ...x, decisionPoint: partition }); + if (x && experiment.conditions.some((con) => con.id === x.parentCondition.id)) { + conditionPayload.push({ ...x, decisionPoint: partitions[index] }); } }); }); - return { ...experiment, conditionPayloads: conditionPayload }; + return { ...experiment, partitions, conditionPayloads: conditionPayload }; } public reducedConditionPayload(experiment: Experiment): any { diff --git a/packages/backend/test/unit/services/Algorithms.test.ts b/packages/backend/test/unit/services/Algorithms.test.ts new file mode 100644 index 000000000..41a630b9c --- /dev/null +++ b/packages/backend/test/unit/services/Algorithms.test.ts @@ -0,0 +1,175 @@ +import { buildWithinSubjectOrderedConditions } from '../../../src/api/Algorithms'; +import { Experiment } from '../../../src/api/models/Experiment'; +import { ExperimentCondition } from '../../../src/api/models/ExperimentCondition'; +import { FactorDTO } from '../../../src/api/DTO/FactorDTO'; +import { CONDITION_ORDER, EXPERIMENT_TYPE } from 'upgrade_types'; + +/** + * These guard two invariants that the removal of the per-request deep copy in + * getCachedValidExperiments made load-bearing: + * + * 1. ORDER INDEPENDENCE — the assignment read path (`getValidExperiments`) does NOT + * `ORDER BY conditions.order`; only the admin path (`findOneExperiment`) does. So whatever + * sequence Postgres happens to return rows in is what this code receives. Output must depend on + * each condition's `order` field, never on its position in the incoming array. + * + * This is not hypothetical: ORDERED_ROUND_ROBIN previously produced correct output only because + * `assignRandom` had sorted `experiment.conditions` IN PLACE earlier in the same request, and + * this function silently consumed that side effect. Removing the in-place sort broke it. Worse, + * `assignRandom` is skipped when a user is already enrolled, so the ordering a user got on their + * first request differed from later ones. + * + * 2. NON-MUTATION — `experiment.conditions` can be the array owned by the in-memory experiment + * cache, which hands the same reference to every request. + */ +describe('Algorithms: buildWithinSubjectOrderedConditions', () => { + const USER_ID = 'user-123'; + + const makeCondition = (conditionCode: string, order: number, levelId?: string): ExperimentCondition => + ({ + id: `condition-${conditionCode}`, + conditionCode, + order, + assignmentWeight: 50, + levelCombinationElements: levelId ? [{ level: { id: levelId } }] : [], + } as unknown as ExperimentCondition); + + // Deliberately stored out of `order`, the way an unordered query can return them. + const scrambled = (): ExperimentCondition[] => [ + makeCondition('C', 3, 'level-c'), + makeCondition('A', 1, 'level-a'), + makeCondition('B', 2, 'level-b'), + ]; + + const sorted = (): ExperimentCondition[] => [ + makeCondition('A', 1, 'level-a'), + makeCondition('B', 2, 'level-b'), + makeCondition('C', 3, 'level-c'), + ]; + + const makeExperiment = ( + conditions: ExperimentCondition[], + conditionOrder: CONDITION_ORDER, + type: EXPERIMENT_TYPE = EXPERIMENT_TYPE.SIMPLE + ): Experiment => + ({ + id: 'experiment-1', + type, + conditionOrder, + conditions, + } as unknown as Experiment); + + const factors: FactorDTO[] = [ + { + name: 'Color', + order: 1, + levels: [ + { id: 'level-a', name: 'Red', payload: { type: 'string', value: 'red' } }, + { id: 'level-b', name: 'Blue', payload: { type: 'string', value: 'blue' } }, + { id: 'level-c', name: 'Green', payload: { type: 'string', value: 'green' } }, + ], + }, + ] as unknown as FactorDTO[]; + + const CONDITION_ORDERS = [ + CONDITION_ORDER.ORDERED_ROUND_ROBIN, + CONDITION_ORDER.RANDOM, + CONDITION_ORDER.RANDOM_ROUND_ROBIN, + ]; + + describe.each(CONDITION_ORDERS)('with conditionOrder %s', (conditionOrder) => { + it.each([0, 1, 2, 5])( + 'should produce identical output regardless of incoming condition order (enrollment count %i)', + (repeatedEnrollmentLength) => { + const fromScrambled = buildWithinSubjectOrderedConditions( + makeExperiment(scrambled(), conditionOrder), + factors, + USER_ID, + repeatedEnrollmentLength + ); + const fromSorted = buildWithinSubjectOrderedConditions( + makeExperiment(sorted(), conditionOrder), + factors, + USER_ID, + repeatedEnrollmentLength + ); + + expect(fromScrambled.orderedConditions.map((condition) => condition.conditionCode)).toEqual( + fromSorted.orderedConditions.map((condition) => condition.conditionCode) + ); + } + ); + + it('should not mutate the experiment conditions it was handed', () => { + const conditions = scrambled(); + const experiment = makeExperiment(conditions, conditionOrder); + const snapshot = JSON.parse(JSON.stringify(experiment)); + + buildWithinSubjectOrderedConditions(experiment, factors, USER_ID, 1); + buildWithinSubjectOrderedConditions(experiment, factors, USER_ID, 1); + + expect(JSON.parse(JSON.stringify(experiment))).toEqual(snapshot); + expect(experiment.conditions).toBe(conditions); + expect(experiment.conditions.map((condition) => condition.conditionCode)).toEqual(['C', 'A', 'B']); + }); + }); + + describe('ORDERED_ROUND_ROBIN', () => { + // The strongest statement of the bug CI caught: the rotation baseline is `order`, not array + // position, so an unsorted input must still start at the order-1 condition. + it('should start the rotation at the lowest-order condition, not the first array element', () => { + const { orderedConditions } = buildWithinSubjectOrderedConditions( + makeExperiment(scrambled(), CONDITION_ORDER.ORDERED_ROUND_ROBIN), + factors, + USER_ID, + 0 + ); + + expect(orderedConditions.map((condition) => condition.conditionCode)).toEqual(['A', 'B', 'C']); + }); + + it('should advance the rotation by the repeated enrollment count', () => { + const rotationFor = (repeatedEnrollmentLength: number) => + buildWithinSubjectOrderedConditions( + makeExperiment(scrambled(), CONDITION_ORDER.ORDERED_ROUND_ROBIN), + factors, + USER_ID, + repeatedEnrollmentLength + ).orderedConditions.map((condition) => condition.conditionCode); + + expect(rotationFor(1)).toEqual(['B', 'C', 'A']); + expect(rotationFor(2)).toEqual(['C', 'A', 'B']); + // wraps back around + expect(rotationFor(3)).toEqual(['A', 'B', 'C']); + }); + }); + + describe('factorial experiments', () => { + it('should keep orderedFactors aligned with orderedConditions regardless of incoming order', () => { + const fromScrambled = buildWithinSubjectOrderedConditions( + makeExperiment(scrambled(), CONDITION_ORDER.ORDERED_ROUND_ROBIN, EXPERIMENT_TYPE.FACTORIAL), + factors, + USER_ID, + 0 + ); + + expect(fromScrambled.orderedConditions.map((condition) => condition.conditionCode)).toEqual(['A', 'B', 'C']); + // condition A carries level-a (Red), B carries level-b (Blue), C carries level-c (Green) — the + // factor array must be permuted in lockstep with the conditions, not left in arrival order. + expect(fromScrambled.orderedFactors.map((factor) => factor['Color'].level)).toEqual(['Red', 'Blue', 'Green']); + }); + }); + + describe('single-condition experiments', () => { + it('should return the condition untouched without consulting conditionOrder', () => { + const { orderedConditions } = buildWithinSubjectOrderedConditions( + makeExperiment([makeCondition('solo', 1)], CONDITION_ORDER.RANDOM), + factors, + USER_ID, + 3 + ); + + expect(orderedConditions.map((condition) => condition.conditionCode)).toEqual(['solo']); + }); + }); +}); diff --git a/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts b/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts index 2b039345f..3f3abc211 100644 --- a/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts +++ b/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts @@ -27,6 +27,7 @@ import { factorialIndividualAssignmentExperiment, simpleDPExperiment, simpleWithinSubjectOrderedRoundRobinExperiment, + simpleWithinSubjectRandomRoundRobinExperiment, withinSubjectDPExperiment, } from '../mockdata'; import { GroupEnrollment } from '../../../src/api/models/GroupEnrollment'; @@ -238,6 +239,199 @@ describe('Experiment Assignment Service Test', () => { expect(result[0].assignedCondition[0]).toEqual(cond); }); + describe('cached experiment graph is never mutated', () => { + // getCachedValidExperiments hands the SAME object graph to every request — the cache is an + // in-memory store and the per-request `JSON.parse(JSON.stringify(...))` defensive copy was removed + // for performance (it cost ~8ms of blocking CPU per request on a production-sized payload). + // + // That makes non-mutation a load-bearing invariant rather than a nicety: an in-place write here + // (`.sort()`, `.splice()`, `delete`, a nested field assignment) corrupts the graph for every + // subsequent request in that context until the cache entry expires, silently and with no error. + // It also will not reproduce in local dev, where caching is typically off and every request gets + // a freshly loaded graph. These tests fail loudly instead. + // + // Note that top-level writes are safe by construction — formattingConditionPayload returns a + // `{...experiment}` shallow copy — so what these guard is mutation one or more levels down. + const expectNoMutation = async (exp: any, run: () => Promise) => { + const snapshot = JSON.parse(JSON.stringify(exp)); + + // Twice: a mutation that is idempotent (an in-place sort, say) can still be caught on the first + // pass, and running twice also covers request N+1 seeing what request N left behind. + await run(); + await run(); + + expect(JSON.parse(JSON.stringify(exp))).toEqual(snapshot); + }; + + it('should not mutate the cached experiment for a simple individual experiment', async () => { + const context = 'context'; + const userDoc = { id: 'user123', group: { schoolId: ['school1'] }, workingGroup: {} }; + const exp = structuredClone(simpleIndividualAssignmentExperiment); + + testedModule.cacheService.wrap = sandbox.stub().resolves([exp]); + testedModule.experimentRepository.getValidExperimentsWithPreview = sandbox.stub().resolves([]); + testedModule.experimentService.getCachedValidExperiments = sandbox.stub().resolves([exp]); + testedModule.experimentUserService = { getOriginalUserDoc: sandbox.stub().resolves(userDoc) }; + + await expectNoMutation(exp, () => testedModule.getAllExperimentConditions(userDoc, context, loggerMock)); + }); + + it('should not reorder the cached conditions of a simple group experiment', async () => { + // This fixture stores its conditions out of `order` on purpose, so an in-place sort on the + // assignment path — which is exactly the bug this guards — shows up as a reordering. + const context = 'context'; + const userDoc = { + id: 'user123', + group: { 'add-group1': ['school1'] }, + workingGroup: { 'add-group1': 'school1' }, + }; + const exp: any = structuredClone(simpleGroupAssignmentExperiment); + expect(exp.conditions.map((condition) => condition.order)).toEqual([2, 1]); + + const groupEnrollment = new GroupEnrollment(); + groupEnrollment.experiment = exp; + groupEnrollment.condition = exp.conditions[0]; + groupEnrollment.groupId = 'add-group1'; + + groupEnrollmentRepositoryMock = { + findEnrollments: sandbox.stub().resolves([groupEnrollment]), + delete: sandbox.stub().resolves(), + }; + + testedModule.experimentService.getCachedValidExperiments = sandbox.stub().resolves([exp]); + testedModule.experimentService.checkUserOrGroupIsGloballyExcluded = sandbox.stub().resolves([false, false]); + testedModule.experimentService.getAssignmentsAndExclusionsForUser = sandbox + .stub() + .resolves([ + individualEnrollmentRepositoryMock, + groupEnrollmentRepositoryMock, + individualExclusionRepositoryMock, + groupExclusionRepositoryMock, + ]); + testedModule.experimentUserService = { getOriginalUserDoc: sandbox.stub().resolves(userDoc) }; + + await expectNoMutation(exp, () => testedModule.getAllExperimentConditions(userDoc, context, loggerMock)); + }); + + it('should not mutate the cached experiment for a factorial group experiment', async () => { + const context = 'context'; + const userDoc = { + id: 'user123', + group: { 'add-group1': ['school1'] }, + workingGroup: { 'add-group1': 'school1' }, + }; + const exp: any = structuredClone(factorialGroupAssignmentExperiment); + + testedModule.experimentService.getCachedValidExperiments = sandbox.stub().resolves([exp]); + testedModule.experimentService.checkUserOrGroupIsGloballyExcluded = sandbox.stub().resolves([false, false]); + testedModule.experimentUserService = { getOriginalUserDoc: sandbox.stub().resolves(userDoc) }; + + await expectNoMutation(exp, () => testedModule.getAllExperimentConditions(userDoc, context, loggerMock)); + }); + }); + + describe('assignment does not depend on the order rows come back from the database', () => { + // `getValidExperiments` — the assignment read path — has no `ORDER BY conditions.order`; only the + // admin-side `findOneExperiment` does. So the sequence conditions arrive in is whatever Postgres + // felt like, and it can change as rows are updated. + // + // For a long time one caller got away with depending on arrival order: the within-subjects + // builder read `experiment.conditions` positionally and was correct only because `assignRandom` + // had sorted that array in place earlier in the same request. These tests state the invariant + // directly — same input, shuffled, must produce the same assignment — so the next caller that + // leans on arrival order fails here rather than in CI's integration suite or in production data. + const expectOrderIndependence = async ( + buildExperiment: () => any, + setup: (exp: any, userDoc: any) => void, + userDoc: any + ) => { + const context = 'context'; + + const run = async (reverseConditions: boolean) => { + const exp = buildExperiment(); + if (reverseConditions) { + exp.conditions = [...exp.conditions].reverse(); + } + setup(exp, userDoc); + return testedModule.getAllExperimentConditions(userDoc, context, loggerMock); + }; + + const asStored = await run(false); + const reversed = await run(true); + + expect(reversed).toEqual(asStored); + // guard the guard: a fixture with one condition would make this vacuous + expect(buildExperiment().conditions.length).toBeGreaterThan(1); + }; + + it('should assign the same condition for a simple individual experiment', async () => { + const userDoc = { id: 'user123', group: { schoolId: ['school1'] }, workingGroup: {} }; + + await expectOrderIndependence( + () => structuredClone(simpleIndividualAssignmentExperiment), + (exp) => { + testedModule.experimentService.getCachedValidExperiments = sandbox.stub().resolves([exp]); + testedModule.experimentUserService = { getOriginalUserDoc: sandbox.stub().resolves(userDoc) }; + }, + userDoc + ); + }); + + it('should produce the same rotation for a within-subjects ORDERED_ROUND_ROBIN experiment', async () => { + // The unit-level analogue of the integration failure: rotation is anchored to each condition's + // `order`, so reversing the incoming array must not change the sequence the user receives. + const userDoc = { id: 'user123', group: { schoolId: ['school1'] }, workingGroup: {} }; + + await expectOrderIndependence( + () => structuredClone(simpleWithinSubjectOrderedRoundRobinExperiment), + (exp) => { + testedModule.experimentService.getCachedValidExperiments = sandbox.stub().resolves([exp]); + testedModule.experimentUserService = { getOriginalUserDoc: sandbox.stub().resolves(userDoc) }; + // a non-zero count so the rotation actually advances rather than sitting at position 0 + testedModule.repeatedEnrollmentRepository = { + getRepeatedEnrollmentCount: sandbox + .stub() + .resolves([{ userId: userDoc.id, experimentId: exp.id, count: 1 }]), + }; + }, + userDoc + ); + }); + + it('should produce the same rotation for a within-subjects RANDOM_ROUND_ROBIN experiment', async () => { + // The seeded shuffles consume the array too, so their output is order-dependent unless the + // input is normalized first. + const userDoc = { id: 'user123', group: { schoolId: ['school1'] }, workingGroup: {} }; + + await expectOrderIndependence( + () => structuredClone(simpleWithinSubjectRandomRoundRobinExperiment), + (exp) => { + testedModule.experimentService.getCachedValidExperiments = sandbox.stub().resolves([exp]); + testedModule.experimentUserService = { getOriginalUserDoc: sandbox.stub().resolves(userDoc) }; + testedModule.repeatedEnrollmentRepository = { + getRepeatedEnrollmentCount: sandbox + .stub() + .resolves([{ userId: userDoc.id, experimentId: exp.id, count: 1 }]), + }; + }, + userDoc + ); + }); + + it('should assign the same condition and factors for a factorial individual experiment', async () => { + const userDoc = { id: 'user123', group: { schoolId: ['school1'] }, workingGroup: {} }; + + await expectOrderIndependence( + () => structuredClone(factorialIndividualAssignmentExperiment), + (exp) => { + testedModule.experimentService.getCachedValidExperiments = sandbox.stub().resolves([exp]); + testedModule.experimentUserService = { getOriginalUserDoc: sandbox.stub().resolves(userDoc) }; + }, + userDoc + ); + }); + }); + it('should not pool experiments together due to a shared pending decision point site/target', async () => { const context = 'context'; const userDoc = { id: 'user123', group: { schoolId: ['school1'] }, workingGroup: {} }; @@ -492,12 +686,18 @@ describe('Experiment Assignment Service Test', () => { const result = await testedModule.getAllExperimentConditions(userDoc, context, loggerMock); - const cond = { ...exp.conditions[0], experimentId: exp.id, payload: undefined }; + // Assignment picks from the conditions sorted by `order`, and this fixture is deliberately stored + // out of order. Name the expected condition by its order rather than by fixture position — the + // assignment path must not reorder `exp.conditions` in place (it can be the cached array). + const lowestOrderCondition = [...exp.conditions].sort((a, b) => a.order - b.order)[0]; + const cond = { ...lowestOrderCondition, experimentId: exp.id, payload: undefined }; expect(result.length).toEqual(1); expect(result[0].site).toEqual(exp.partitions[0].site); expect(result[0].target).toEqual(exp.partitions[0].target); expect(result[0].assignedFactor).toBeNull(); expect(result[0].assignedCondition[0]).toEqual(cond); + // the fixture order is untouched by the assignment path + expect(exp.conditions.map((condition) => condition.conditionCode)).toEqual(['add-con2', 'add-con1']); }); it('should return the assigned condition for a factorial group experiment', async () => { @@ -1795,13 +1995,17 @@ describe('Experiment Assignment Service Test', () => { expect(Object.keys(result)).toHaveLength(2); + // Assignment picks from the conditions sorted by `order`, and this fixture is deliberately + // stored out of order — the assignment path must not reorder `exp.conditions` in place. + const lowestOrderCondition = [...exp.conditions].sort((a, b) => a.order - b.order)[0]; + // Both users should get same assignment since they're in same group for (const userId of ['user1', 'user2']) { expect(result[userId]).toBeDefined(); expect(result[userId].site).toEqual(site); expect(result[userId].target).toEqual(target); expect(result[userId].assignedCondition[0].experimentId).toEqual(exp.id); - expect(result[userId].assignedCondition[0].conditionCode).toEqual(exp.conditions[0].conditionCode); + expect(result[userId].assignedCondition[0].conditionCode).toEqual(lowestOrderCondition.conditionCode); } }); diff --git a/packages/backend/test/unit/services/ExperimentService.test.ts b/packages/backend/test/unit/services/ExperimentService.test.ts index 3ce8faa3d..918b5628e 100644 --- a/packages/backend/test/unit/services/ExperimentService.test.ts +++ b/packages/backend/test/unit/services/ExperimentService.test.ts @@ -1173,4 +1173,90 @@ describe('ExperimentService Testing', () => { expect(result).toContain(`name ILIKE '%100\\%\\_path\\\\name%' ESCAPE '\\'`); }); }); + + describe('formattingConditionPayload()', () => { + // The assignment read path calls this on experiments handed out by the in-memory cache, which + // returns the same object reference to every request. Mutating the input would corrupt the cached + // graph for every subsequent request — which is exactly why this call site used to need a full + // deep copy of the experiment per request. These tests guard the invariant that removed it. + const buildSimpleExperiment = (): Experiment => { + const conditionA = { id: 'condition-a', order: 1 } as ExperimentCondition; + const conditionB = { id: 'condition-b', order: 2 } as ExperimentCondition; + const decisionPoint = { id: 'dp-1', site: 'SelectSection', target: 'target-1' } as DecisionPoint; + + // Deliberately out of order so the sort inside the formatter has something to do. + decisionPoint.conditionPayloads = [ + { id: 'payload-b', parentCondition: conditionB } as ConditionPayload, + { id: 'payload-a', parentCondition: conditionA } as ConditionPayload, + ]; + + return { + id: 'experiment-1', + type: EXPERIMENT_TYPE.SIMPLE, + conditions: [conditionA, conditionB], + partitions: [decisionPoint], + } as Experiment; + }; + + const buildFactorialExperiment = (): Experiment => { + const condition = { id: 'condition-a', order: 1 } as ExperimentCondition; + condition.conditionPayloads = [{ id: 'payload-a' } as ConditionPayload]; + + return { + id: 'experiment-2', + type: EXPERIMENT_TYPE.FACTORIAL, + conditions: [condition], + partitions: [], + } as Experiment; + }; + + it('should not mutate the input experiment for a simple experiment', () => { + const experiment = buildSimpleExperiment(); + const snapshot = JSON.parse(JSON.stringify(experiment)); + + service.formattingConditionPayload(experiment); + + expect(JSON.parse(JSON.stringify(experiment))).toEqual(snapshot); + expect(experiment.partitions[0].conditionPayloads).toHaveLength(2); + }); + + it('should not mutate the input experiment for a factorial experiment', () => { + const experiment = buildFactorialExperiment(); + const snapshot = JSON.parse(JSON.stringify(experiment)); + + service.formattingConditionPayload(experiment); + + expect(JSON.parse(JSON.stringify(experiment))).toEqual(snapshot); + expect(experiment.conditions[0].conditionPayloads).toHaveLength(1); + }); + + it('should hoist payloads to the root, strip them from decision points, and sort by condition order', () => { + const result = service.formattingConditionPayload(buildSimpleExperiment()); + + expect(result.conditionPayloads.map((payload) => payload.id)).toEqual(['payload-a', 'payload-b']); + expect(result.partitions[0].conditionPayloads).toBeUndefined(); + // decisionPoint back-references must point at the stripped partition on the returned experiment + expect(result.conditionPayloads[0].decisionPoint).toBe(result.partitions[0]); + }); + + it('should hoist payloads to the root and strip them from conditions for a factorial experiment', () => { + const result = service.formattingConditionPayload(buildFactorialExperiment()); + + expect(result.conditionPayloads.map((payload) => payload.id)).toEqual(['payload-a']); + expect(result.conditions[0].conditionPayloads).toBeUndefined(); + // parentCondition back-references must point at the stripped condition on the returned experiment + expect(result.conditionPayloads[0].parentCondition).toBe(result.conditions[0]); + }); + + it('should leave repeated formatting of the same cached experiment stable', () => { + const experiment = buildSimpleExperiment(); + + const first = service.formattingConditionPayload(experiment); + const second = service.formattingConditionPayload(experiment); + + expect(second.conditionPayloads.map((payload) => payload.id)).toEqual( + first.conditionPayloads.map((payload) => payload.id) + ); + }); + }); });