From f2de6a33331a0755836310548684c5cdcfbd8c02 Mon Sep 17 00:00:00 2001 From: Benjamin Blanchard Date: Thu, 23 Jul 2026 16:08:46 -0400 Subject: [PATCH] rotate all condition lists in an experiment on mark --- .../client/ExperimentClient.java | 42 +++++++----- .../js/src/ApiService/ApiService.spec.ts | 44 +++++++++++- clientlibs/js/src/ApiService/ApiService.ts | 12 ++-- .../js/src/DataService/DataService.spec.ts | 67 +++++++++++++++++++ clientlibs/js/src/DataService/DataService.ts | 14 ++++ .../python/src/upgrade_client_lib/client.py | 12 ++-- .../src/upgrade_client_lib/data_service.py | 13 +++- clientlibs/python/tests/test_client.py | 47 ++++++++++++- clientlibs/python/tests/test_data_service.py | 40 +++++++++++ 9 files changed, 262 insertions(+), 29 deletions(-) diff --git a/clientlibs/java/src/main/java/org/upgradeplatform/client/ExperimentClient.java b/clientlibs/java/src/main/java/org/upgradeplatform/client/ExperimentClient.java index 2481c48ccb..6c14034b8c 100644 --- a/clientlibs/java/src/main/java/org/upgradeplatform/client/ExperimentClient.java +++ b/clientlibs/java/src/main/java/org/upgradeplatform/client/ExperimentClient.java @@ -343,22 +343,25 @@ private ExperimentsResponse findExperimentResponse(String site, String target, .orElse(new ExperimentsResponse()); } - private void rotateConditions(String site, String target) { - if (this.allExperiments != null) { - ExperimentsResponse result = this.allExperiments.stream().filter(t -> t.getSite().equalsIgnoreCase(site) && - (isStringNull(target) ? isStringNull(t.getTarget().toString()) - : t.getTarget().toString().equalsIgnoreCase(target))) - .findFirst().orElse(null); - if (result != null) { - Condition[] rotatedCondition = Arrays.copyOf(result.getAssignedCondition(), - result.getAssignedCondition().length); - List rotatedList = Arrays.asList(rotatedCondition); - Collections.rotate(rotatedList, -1); - rotatedCondition = rotatedList.toArray(rotatedCondition); - result.setAssignedCondition(rotatedCondition); - result.setAssignedFactor(result.getAssignedFactor()); - } + private void rotateConditions(String experimentId) { + if (this.allExperiments == null || isStringNull(experimentId)) { + return; } + + this.allExperiments.stream() + .filter(experiment -> experiment.getAssignedCondition() != null + && experiment.getAssignedCondition().length > 0 + && Arrays.stream(experiment.getAssignedCondition()) + .anyMatch(condition -> condition != null + && experimentId.equals(condition.getExperimentId()))) + .forEach(experiment -> { + Condition[] rotatedCondition = Arrays.copyOf(experiment.getAssignedCondition(), + experiment.getAssignedCondition().length); + List rotatedList = Arrays.asList(rotatedCondition); + Collections.rotate(rotatedList, -1); + experiment.setAssignedCondition(rotatedCondition); + experiment.setAssignedFactor(experiment.getAssignedFactor()); + }); } private static ExperimentsResponse copyExperimentResponse(ExperimentsResponse experimentsResponse) { @@ -437,9 +440,12 @@ public void markDecisionPoint(MarkedDecisionPointStatus status, MarkExperimentRe @Override public void completed(Response response) { if (response.getStatus() == Response.Status.OK.getStatusCode()) { - - readResponseToCallback(response, callbacks, MarkDecisionPoint.class); - rotateConditions(data.getSite(), data.getTarget()); + readResponseToCallback(response, callbacks, MarkDecisionPoint.class, + markDecisionPoint -> { + String experimentId = markDecisionPoint.getExperimentId(); + rotateConditions(experimentId); + return markDecisionPoint; + }); } else { String status = Response.Status.fromStatusCode(response.getStatus()).toString(); ErrorResponse error = new ErrorResponse(response.getStatus(), diff --git a/clientlibs/js/src/ApiService/ApiService.spec.ts b/clientlibs/js/src/ApiService/ApiService.spec.ts index 6fe698e533..656ee8315c 100644 --- a/clientlibs/js/src/ApiService/ApiService.spec.ts +++ b/clientlibs/js/src/ApiService/ApiService.spec.ts @@ -12,6 +12,7 @@ import { UpGradeClientRequests } from './../types/requests'; const MockDataService = { findExperimentAssignmentBySiteAndTarget: jest.fn(), rotateAssignmentList: jest.fn(), + rotateAssignmentsByExperimentId: jest.fn(), }; const mockHttpClient = { @@ -282,7 +283,7 @@ describe('ApiService', () => { beforeEach(() => { MockDataService.findExperimentAssignmentBySiteAndTarget.mockReturnValue(mockAssignment); - MockDataService.rotateAssignmentList.mockImplementation((a: any) => a); + MockDataService.rotateAssignmentsByExperimentId.mockClear(); mockHttpClient.doPost.mockClear(); }); @@ -349,6 +350,47 @@ describe('ApiService', () => { expect(mockHttpClient.doPost).toHaveBeenCalledWith(expectedUrl, expectedRequestBody, expectedOptions); }); + + it('should rotate cached assignments for the returned experiment id after marking', async () => { + const params = { + site: 'testSite', + target: 'testTarget', + condition: 'variant_x', + status: MARKED_DECISION_POINT_STATUS.CONDITION_APPLIED, + }; + + mockHttpClient.doPost.mockResolvedValue({ + id: 'mark123', + site: 'testSite', + target: 'testTarget', + userId: defaultConfig.userId, + experimentId: 'exp1', + }); + + await apiService.markDecisionPoint(params); + + expect(MockDataService.rotateAssignmentsByExperimentId).toHaveBeenCalledWith('exp1'); + }); + + it('should not rotate cached assignments when response has no experiment id', async () => { + const params = { + site: 'testSite', + target: 'testTarget', + condition: 'variant_x', + status: MARKED_DECISION_POINT_STATUS.CONDITION_APPLIED, + }; + + mockHttpClient.doPost.mockResolvedValue({ + id: 'mark123', + site: 'testSite', + target: 'testTarget', + userId: defaultConfig.userId, + }); + + await apiService.markDecisionPoint(params); + + expect(MockDataService.rotateAssignmentsByExperimentId).not.toHaveBeenCalled(); + }); }); describe('#setFeatureFlagUserGroupsForSession', () => { diff --git a/clientlibs/js/src/ApiService/ApiService.ts b/clientlibs/js/src/ApiService/ApiService.ts index ae88f72df9..aec63c460a 100644 --- a/clientlibs/js/src/ApiService/ApiService.ts +++ b/clientlibs/js/src/ApiService/ApiService.ts @@ -222,7 +222,7 @@ export default class ApiService { }); } - public markDecisionPoint({ + public async markDecisionPoint({ site, target, condition, @@ -232,8 +232,6 @@ export default class ApiService { }: UpGradeClientInterfaces.IMarkDecisionPointOptions): Promise { const assignment = this.dataService.findExperimentAssignmentBySiteAndTarget(site, target); - this.dataService.rotateAssignmentList(assignment); - const data = { ...assignment, assignedCondition: { ...assignment.assignedCondition[0], conditionCode: condition }, @@ -258,7 +256,7 @@ export default class ApiService { } // send request - return this.sendRequest< + const response = await this.sendRequest< UpGradeClientInterfaces.IMarkDecisionPoint, UpGradeClientRequests.IMarkDecisionPointRequestBody >({ @@ -266,6 +264,12 @@ export default class ApiService { method: UpGradeClientEnums.REQUEST_METHOD.POST, body: requestBody, }); + + if (response?.experimentId) { + this.dataService.rotateAssignmentsByExperimentId(response.experimentId); + } + + return response; } public log(logData: ILogInput[]): Promise { diff --git a/clientlibs/js/src/DataService/DataService.spec.ts b/clientlibs/js/src/DataService/DataService.spec.ts index 4c4a8f9fb2..ff0a1b2a71 100644 --- a/clientlibs/js/src/DataService/DataService.spec.ts +++ b/clientlibs/js/src/DataService/DataService.spec.ts @@ -252,6 +252,73 @@ describe('DataService', () => { }); }); + describe('#rotateAssignmentsByExperimentId', () => { + it('should rotate only assignments that contain the provided experiment id', () => { + const experimentAssignmentData: IExperimentAssignmentv5[] = [ + { + site: 'siteA', + target: 'targetA', + assignedCondition: [ + { + conditionCode: 'control', + payload: { type: PAYLOAD_TYPE.STRING, value: 'testControlA' }, + experimentId: 'exp-1', + id: 'cond-a1', + }, + { + conditionCode: 'variant_x', + payload: { type: PAYLOAD_TYPE.STRING, value: 'testVariantA' }, + experimentId: 'exp-1', + id: 'cond-a2', + }, + ], + assignedFactor: [ + { + factor1: { level: 'level1', payload: { type: PAYLOAD_TYPE.STRING, value: 'testLevel1' } }, + }, + { + factor2: { level: 'level2', payload: { type: PAYLOAD_TYPE.STRING, value: 'testLevel2' } }, + }, + ], + experimentType: EXPERIMENT_TYPE.FACTORIAL, + }, + { + site: 'siteB', + target: 'targetB', + assignedCondition: [ + { + conditionCode: 'control', + payload: { type: PAYLOAD_TYPE.STRING, value: 'testControlB' }, + experimentId: 'exp-2', + id: 'cond-b1', + }, + { + conditionCode: 'variant_y', + payload: { type: PAYLOAD_TYPE.STRING, value: 'testVariantB' }, + experimentId: 'exp-2', + id: 'cond-b2', + }, + ], + assignedFactor: [], + experimentType: EXPERIMENT_TYPE.SIMPLE, + }, + ]; + + dataService.setExperimentAssignmentData(experimentAssignmentData); + + dataService.rotateAssignmentsByExperimentId('exp-1'); + + expect(experimentAssignmentData[0].assignedCondition[0].id).toBe('cond-a2'); + expect(experimentAssignmentData[0].assignedCondition[1].id).toBe('cond-a1'); + expect(experimentAssignmentData[1].assignedCondition[0].id).toBe('cond-b1'); + expect(experimentAssignmentData[1].assignedCondition[1].id).toBe('cond-b2'); + }); + + it('should do nothing when there is no cached assignment data', () => { + expect(() => dataService.rotateAssignmentsByExperimentId('exp-1')).not.toThrow(); + }); + }); + describe('#findExperimentAssignmentBySiteAndTarget', () => { it('should return the experiment assignment', () => { const experimentAssignmentData: IExperimentAssignmentv5[] = [ diff --git a/clientlibs/js/src/DataService/DataService.ts b/clientlibs/js/src/DataService/DataService.ts index 14a4537a85..c1f4266a79 100644 --- a/clientlibs/js/src/DataService/DataService.ts +++ b/clientlibs/js/src/DataService/DataService.ts @@ -52,6 +52,20 @@ export class DataService { return assignment; } + public rotateAssignmentsByExperimentId(experimentId: string): void { + if (!experimentId || !this.experimentAssignmentData) { + return; + } + + this.experimentAssignmentData + .filter( + (assignment) => + Array.isArray(assignment.assignedCondition) && + assignment.assignedCondition.some((condition) => condition?.experimentId === experimentId) + ) + .forEach((assignment) => this.rotateAssignmentList(assignment)); + } + public findExperimentAssignmentBySiteAndTarget(site: string, target?: string): IExperimentAssignmentv5 { const normalizedTarget = target ?? ''; const assignment = this.experimentAssignmentData.find( diff --git a/clientlibs/python/src/upgrade_client_lib/client.py b/clientlibs/python/src/upgrade_client_lib/client.py index fd685ac843..b7598b418a 100644 --- a/clientlibs/python/src/upgrade_client_lib/client.py +++ b/clientlibs/python/src/upgrade_client_lib/client.py @@ -200,8 +200,8 @@ async def mark_decision_point( ) -> MarkDecisionPointResponse: """Record that the user encountered a decision point. - Rotates the cached assignment's condition/factor lists so that - successive within-subjects marks cycle through conditions in order. + On success, rotates cached assignments that belong to the returned + experiment id so successive within-subjects marks cycle in order. """ if self._data_service.get_all_assignments() is None: await self.get_all_experiment_conditions() @@ -220,9 +220,8 @@ async def mark_decision_point( factor_entry = cached.assignedFactor[0] if cached.assignedFactor else None if factor_entry: factor_dict = {k: v.model_dump() for k, v in factor_entry.items()} - self._data_service.rotate_assignment(cached) - return await self._api_service.mark_decision_point( + response = await self._api_service.mark_decision_point( site=site, target=target, condition_code=condition, @@ -235,6 +234,11 @@ async def mark_decision_point( client_error=client_error or None, ) + if response.experimentId: + self._data_service.rotate_assignments_by_experiment_id(response.experimentId) + + return response + def mark_decision_point_sync( self, condition: str, diff --git a/clientlibs/python/src/upgrade_client_lib/data_service.py b/clientlibs/python/src/upgrade_client_lib/data_service.py index 0b56184076..80a812dcb8 100644 --- a/clientlibs/python/src/upgrade_client_lib/data_service.py +++ b/clientlibs/python/src/upgrade_client_lib/data_service.py @@ -16,7 +16,9 @@ The UpGrade backend may return multiple conditions for the same decision point (e.g. for gradual roll-outs). :meth:`rotate_assignment` advances the head of the condition/factor lists after each mark so that successive calls -cycle through them. This mirrors the behaviour of the JS ``DataService``. +cycle through them. :meth:`rotate_assignments_by_experiment_id` applies that +rotation across every cached assignment that belongs to the marked experiment. +This mirrors the behaviour of the JS ``DataService``. """ from __future__ import annotations @@ -67,6 +69,15 @@ def rotate_assignment(self, assignment: ExperimentAssignment) -> ExperimentAssig assignment.assignedFactor.append(assignment.assignedFactor.pop(0)) return assignment + def rotate_assignments_by_experiment_id(self, experiment_id: str) -> None: + """Rotate every cached assignment whose conditions match ``experiment_id``.""" + if not experiment_id or self._assignments is None: + return + + for assignment in self._assignments.values(): + if any(c.experimentId == experiment_id for c in assignment.assignedCondition): + self.rotate_assignment(assignment) + # ------------------------------------------------------------------ # Feature flags # ------------------------------------------------------------------ diff --git a/clientlibs/python/tests/test_client.py b/clientlibs/python/tests/test_client.py index 48b322698a..4d878172ac 100644 --- a/clientlibs/python/tests/test_client.py +++ b/clientlibs/python/tests/test_client.py @@ -334,7 +334,19 @@ async def test_mark_body_contains_experiment_metadata(self) -> None: async def test_rotation_advances_condition_on_successive_marks(self) -> None: """Second mark on a multi-condition assignment should use condition[1].""" respx.post(f"{BASE}/assign").mock(return_value=Response(200, json=FACTORIAL_PAYLOAD)) - mark_route = respx.post(f"{BASE}/mark").mock(return_value=Response(200, json=MARK_PAYLOAD)) + mark_route = respx.post(f"{BASE}/mark").mock( + return_value=Response( + 200, + json={ + "id": "mark-factorial", + "userId": USER, + "site": "quiz", + "target": "hint", + "experimentId": "exp-2", + "condition": "combo-A", + }, + ) + ) client = make_client() await client.get_all_experiment_conditions() @@ -352,6 +364,39 @@ async def test_rotation_advances_condition_on_successive_marks(self) -> None: assert first_body["data"]["assignedCondition"]["id"] == "cond-A" assert second_body["data"]["assignedCondition"]["id"] == "cond-B" + @respx.mock + async def test_mark_response_with_mismatched_experiment_id_does_not_rotate(self) -> None: + """Rotation only applies to assignments matching the returned experiment id.""" + respx.post(f"{BASE}/assign").mock(return_value=Response(200, json=FACTORIAL_PAYLOAD)) + mark_route = respx.post(f"{BASE}/mark").mock( + return_value=Response( + 200, + json={ + "id": "mark-mismatch", + "userId": USER, + "site": "quiz", + "target": "hint", + "experimentId": "exp-other", + "condition": "combo-A", + }, + ) + ) + client = make_client() + await client.get_all_experiment_conditions() + + await client.mark_decision_point( + "combo-A", MarkedDecisionPointStatus.CONDITION_APPLIED, "quiz", "hint" + ) + first_body = json.loads(mark_route.calls[0].request.content) + + await client.mark_decision_point( + "combo-B", MarkedDecisionPointStatus.CONDITION_APPLIED, "quiz", "hint" + ) + second_body = json.loads(mark_route.calls[1].request.content) + + assert first_body["data"]["assignedCondition"]["id"] == "cond-A" + assert second_body["data"]["assignedCondition"]["id"] == "cond-A" + @respx.mock async def test_mark_unknown_decision_point_omits_metadata(self) -> None: """Marking an unknown site/target omits experiment metadata gracefully.""" diff --git a/clientlibs/python/tests/test_data_service.py b/clientlibs/python/tests/test_data_service.py index 1ec33f1bc6..f55a93e628 100644 --- a/clientlibs/python/tests/test_data_service.py +++ b/clientlibs/python/tests/test_data_service.py @@ -300,6 +300,46 @@ def test_returns_assignment(self) -> None: assert result is a +class TestRotateAssignmentsByExperimentId: + def test_rotates_only_matching_assignments(self) -> None: + ds = DataService() + match_assignment = make_assignment( + site="quiz", + target="hint", + conditions=[ + AssignedCondition(id="cond-A", conditionCode="A", experimentId="exp-2"), + AssignedCondition(id="cond-B", conditionCode="B", experimentId="exp-2"), + ], + experiment_type=ExperimentType.SIMPLE, + ) + non_match_assignment = make_assignment( + site="home", + target="banner", + conditions=[ + AssignedCondition(id="cond-1", conditionCode="control", experimentId="exp-1"), + AssignedCondition(id="cond-2", conditionCode="treatment", experimentId="exp-1"), + ], + experiment_type=ExperimentType.SIMPLE, + ) + + ds.set_assignments([match_assignment, non_match_assignment]) + ds.rotate_assignments_by_experiment_id("exp-2") + + matched = ds.get_assignment("quiz", "hint") + unchanged = ds.get_assignment("home", "banner") + assert matched is not None + assert unchanged is not None + assert matched.assignedCondition[0].id == "cond-B" + assert matched.assignedCondition[1].id == "cond-A" + assert unchanged.assignedCondition[0].id == "cond-1" + assert unchanged.assignedCondition[1].id == "cond-2" + + def test_noop_when_cache_is_cold(self) -> None: + ds = DataService() + ds.rotate_assignments_by_experiment_id("exp-1") + assert ds.get_all_assignments() is None + + # --------------------------------------------------------------------------- # Instance isolation # ---------------------------------------------------------------------------