From eadf141b00d21131ca0e214d10d351b6156b2c34 Mon Sep 17 00:00:00 2001 From: Farhan Anjum Date: Fri, 2 Jan 2026 22:04:18 +0600 Subject: [PATCH 01/12] [FSSDK-12035] Update: Exclude CMAB from UserProfileService (#474) * Python: Exclude CMAB experiments from user profile updates and add related tests * linting fix * chore: trigger tests * Python: Exclude CMAB experiments from user profile updates and update related tests * Python: Exclude CMAB experiments from user profile storage in decision service * Python: Remove test for CMAB experiments with IGNORE_USER_PROFILE_SERVICE option --- optimizely/decision_service.py | 5 ++ tests/test_decision_service.py | 118 +++++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+) diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index 28275ef6..be2be2c5 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -515,6 +515,11 @@ def get_variation( 'reasons': decide_reasons, 'variation': None } + ignore_user_profile = True + self.logger.debug( + f'Skipping user profile service for CMAB experiment "{experiment.key}". ' + f'CMAB decisions are dynamic and not stored for sticky bucketing.' + ) variation_id = cmab_decision['variation_id'] if cmab_decision else None cmab_uuid = cmab_decision['cmab_uuid'] if cmab_decision else None variation = project_config.get_variation_from_id(experiment_key=experiment.key, diff --git a/tests/test_decision_service.py b/tests/test_decision_service.py index dbcb7436..b38a03b2 100644 --- a/tests/test_decision_service.py +++ b/tests/test_decision_service.py @@ -1074,6 +1074,124 @@ def test_get_variation_cmab_experiment_with_whitelisted_variation(self): mock_bucket.assert_not_called() mock_cmab_decision.assert_not_called() + def test_get_variation_cmab_experiment_does_not_save_user_profile(self): + """Test that CMAB experiments do not save bucketing decisions to user profile.""" + + # Create a user context + user = optimizely_user_context.OptimizelyUserContext( + optimizely_client=None, + logger=None, + user_id="test_user", + user_attributes={} + ) + + # Create a user profile service and tracker + user_profile_service = user_profile.UserProfileService() + user_profile_tracker = user_profile.UserProfileTracker(user.user_id, user_profile_service) + + # Create a CMAB experiment + cmab_experiment = entities.Experiment( + '111150', + 'cmab_experiment', + 'Running', + '111150', + [], # No audience IDs + {}, + [ + entities.Variation('111151', 'variation_1'), + entities.Variation('111152', 'variation_2') + ], + [ + {'entityId': '111151', 'endOfRange': 5000}, + {'entityId': '111152', 'endOfRange': 10000} + ], + cmab={'trafficAllocation': 5000} + ) + + with mock.patch('optimizely.helpers.experiment.is_experiment_running', return_value=True), \ + mock.patch('optimizely.helpers.audience.does_user_meet_audience_conditions', return_value=[True, []]), \ + mock.patch.object(self.decision_service.bucketer, 'bucket_to_entity_id', + return_value=['$', []]), \ + mock.patch.object(self.decision_service, 'cmab_service') as mock_cmab_service, \ + mock.patch.object(self.project_config, 'get_variation_from_id', + return_value=entities.Variation('111151', 'variation_1')), \ + mock.patch.object(user_profile_tracker, 'update_user_profile') as mock_update_profile, \ + mock.patch.object(self.decision_service, 'logger') as mock_logger: + + # Configure CMAB service to return a decision + mock_cmab_service.get_decision.return_value = ( + { + 'variation_id': '111151', + 'cmab_uuid': 'test-cmab-uuid-123' + }, + [] # reasons list + ) + + # Call get_variation with the CMAB experiment and user profile tracker + variation_result = self.decision_service.get_variation( + self.project_config, + cmab_experiment, + user, + user_profile_tracker + ) + variation = variation_result['variation'] + cmab_uuid = variation_result['cmab_uuid'] + + # Verify the variation and cmab_uuid are returned + self.assertEqual(entities.Variation('111151', 'variation_1'), variation) + self.assertEqual('test-cmab-uuid-123', cmab_uuid) + + # Verify user profile was NOT updated for CMAB experiment + mock_update_profile.assert_not_called() + + # Verify debug log was called to explain CMAB exclusion + mock_logger.debug.assert_any_call( + 'Skipping user profile service for CMAB experiment "cmab_experiment". ' + 'CMAB decisions are dynamic and not stored for sticky bucketing.' + ) + + def test_get_variation_standard_experiment_saves_user_profile(self): + """Test that standard (non-CMAB) experiments DO save bucketing decisions to user profile.""" + + user = optimizely_user_context.OptimizelyUserContext( + optimizely_client=None, + logger=None, + user_id="test_user", + user_attributes={} + ) + + # Create a user profile service and tracker + user_profile_service = user_profile.UserProfileService() + user_profile_tracker = user_profile.UserProfileTracker(user.user_id, user_profile_service) + + # Get a standard (non-CMAB) experiment + experiment = self.project_config.get_experiment_from_key("test_experiment") + + with mock.patch('optimizely.decision_service.DecisionService.get_whitelisted_variation', + return_value=[None, []]), \ + mock.patch('optimizely.decision_service.DecisionService.get_stored_variation', + return_value=None), \ + mock.patch('optimizely.helpers.audience.does_user_meet_audience_conditions', + return_value=[True, []]), \ + mock.patch('optimizely.bucketer.Bucketer.bucket', + return_value=[entities.Variation("111129", "variation"), []]), \ + mock.patch.object(user_profile_tracker, 'update_user_profile') as mock_update_profile: + + # Call get_variation with standard experiment and user profile tracker + variation_result = self.decision_service.get_variation( + self.project_config, + experiment, + user, + user_profile_tracker + ) + variation = variation_result['variation'] + + # Verify variation was returned + self.assertEqual(entities.Variation("111129", "variation"), variation) + + # Verify user profile WAS updated for standard experiment + mock_update_profile.assert_called_once_with(experiment, variation) + class FeatureFlagDecisionTests(base.BaseTest): def setUp(self): From f98886af41e22d3d565c880b8e98efb054ae641a Mon Sep 17 00:00:00 2001 From: esrakartalOpt <102107327+esrakartalOpt@users.noreply.github.com> Date: Tue, 13 Jan 2026 15:56:55 -0600 Subject: [PATCH 02/12] [FSSDK-12149] [Python] Add Event Retries (#475) --- optimizely/event_dispatcher.py | 2 +- optimizely/odp/odp_event_manager.py | 9 ++++++++- tests/test_odp_event_manager.py | 12 ++++++++---- 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/optimizely/event_dispatcher.py b/optimizely/event_dispatcher.py index 55209dc8..b06b9e18 100644 --- a/optimizely/event_dispatcher.py +++ b/optimizely/event_dispatcher.py @@ -49,7 +49,7 @@ def dispatch_event(event: event_builder.Event) -> None: session = requests.Session() retries = Retry(total=EventDispatchConfig.RETRIES, - backoff_factor=0.1, + backoff_factor=0.2, status_forcelist=[500, 502, 503, 504]) adapter = HTTPAdapter(max_retries=retries) diff --git a/optimizely/odp/odp_event_manager.py b/optimizely/odp/odp_event_manager.py index 85512e90..3fb961ac 100644 --- a/optimizely/odp/odp_event_manager.py +++ b/optimizely/odp/odp_event_manager.py @@ -163,6 +163,8 @@ def _flush_batch(self) -> None: self.logger.debug(f'ODP event queue: flushing batch size {batch_len}.') should_retry = False + initial_retry_interval = 0.2 # 200ms + max_retry_interval = 1.0 # 1 second for i in range(1 + self.retry_count): try: @@ -176,7 +178,12 @@ def _flush_batch(self) -> None: if not should_retry: break if i < self.retry_count: - self.logger.debug('Error dispatching ODP events, scheduled to retry.') + # Exponential backoff: 200ms, 400ms, 800ms, ... capped at 1s + delay = initial_retry_interval * (2 ** i) + if delay > max_retry_interval: + delay = max_retry_interval + self.logger.debug(f'Error dispatching ODP events, retrying after {delay}s.') + time.sleep(delay) if should_retry: self.logger.error(Errors.ODP_EVENT_FAILED.format(f'Failed after {i} retries: {self._current_batch}')) diff --git a/tests/test_odp_event_manager.py b/tests/test_odp_event_manager.py index d9d29eab..acec396f 100644 --- a/tests/test_odp_event_manager.py +++ b/tests/test_odp_event_manager.py @@ -265,7 +265,7 @@ def test_odp_event_manager_retry_failure(self, *args): with mock.patch.object( event_manager.api_manager, 'send_odp_events', new_callable=CopyingMock, return_value=True - ) as mock_send: + ) as mock_send, mock.patch('time.sleep') as mock_sleep: event_manager.send_event(**self.events[0]) event_manager.send_event(**self.events[1]) event_manager.flush() @@ -275,7 +275,9 @@ def test_odp_event_manager_retry_failure(self, *args): [mock.call(self.api_key, self.api_host, self.processed_events)] * number_of_tries ) self.assertEqual(len(event_manager._current_batch), 0) - mock_logger.debug.assert_any_call('Error dispatching ODP events, scheduled to retry.') + # Verify exponential backoff delays: 0.2s, 0.4s, 0.8s + mock_sleep.assert_has_calls([mock.call(0.2), mock.call(0.4), mock.call(0.8)]) + mock_logger.debug.assert_any_call('Error dispatching ODP events, retrying after 0.2s.') mock_logger.error.assert_called_once_with( f'ODP event send failed (Failed after 3 retries: {self.processed_events}).' ) @@ -288,7 +290,7 @@ def test_odp_event_manager_retry_success(self, *args): with mock.patch.object( event_manager.api_manager, 'send_odp_events', new_callable=CopyingMock, side_effect=[True, True, False] - ) as mock_send: + ) as mock_send, mock.patch('time.sleep') as mock_sleep: event_manager.send_event(**self.events[0]) event_manager.send_event(**self.events[1]) event_manager.flush() @@ -296,7 +298,9 @@ def test_odp_event_manager_retry_success(self, *args): mock_send.assert_has_calls([mock.call(self.api_key, self.api_host, self.processed_events)] * 3) self.assertEqual(len(event_manager._current_batch), 0) - mock_logger.debug.assert_any_call('Error dispatching ODP events, scheduled to retry.') + # Verify exponential backoff delays: 0.2s, 0.4s (only 2 delays for 3 attempts) + mock_sleep.assert_has_calls([mock.call(0.2), mock.call(0.4)]) + mock_logger.debug.assert_any_call('Error dispatching ODP events, retrying after 0.2s.') mock_logger.error.assert_not_called() self.assertStrictTrue(event_manager.is_running) event_manager.stop() From 494e18b4134c43f375c106433288e438e7df1934 Mon Sep 17 00:00:00 2001 From: Farhan Anjum Date: Tue, 10 Feb 2026 22:11:41 +0600 Subject: [PATCH 03/12] [FSSDK-12276] update: replace flake8 with ruff (#492) * Add Ruff configuration file * Update target version in Ruff configuration to Python 3.9 * Replace Flake8 with Ruff for linting in CI and update contribution guidelines * Remove Flake8 configuration file as part of replacing Flake8 with Ruff * ruff autofix * testing ruff fail in ci * Fix formatting in ruff.toml by ensuring newline at end of file * Update .github/workflows/python.yml Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * ruff fix --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .flake8 | 8 ------- .github/workflows/python.yml | 15 +++++-------- CONTRIBUTING.md | 6 +++--- optimizely/helpers/validator.py | 4 ++-- requirements/test.txt | 2 +- ruff.toml | 38 +++++++++++++++++++++++++++++++++ 6 files changed, 49 insertions(+), 24 deletions(-) delete mode 100644 .flake8 create mode 100644 ruff.toml diff --git a/.flake8 b/.flake8 deleted file mode 100644 index 0fc0cadc..00000000 --- a/.flake8 +++ /dev/null @@ -1,8 +0,0 @@ -[flake8] -# E722 - do not use bare 'except' -# W504 - Either W503 (Line break after Operand) or W503 ( -# Line break before operand needs to be ignored for line lengths -# greater than max-line-length. Best practice shows W504 -ignore = E722, W504 -exclude = optimizely/lib/pymmh3.py,*virtualenv*,tests/testapp/application.py -max-line-length = 120 diff --git a/.github/workflows/python.yml b/.github/workflows/python.yml index 24d95e6a..20894bbb 100644 --- a/.github/workflows/python.yml +++ b/.github/workflows/python.yml @@ -33,17 +33,12 @@ jobs: uses: actions/setup-python@v4 with: python-version: '3.12' - # flake8 version should be same as the version in requirements/test.txt - # to avoid lint errors on CI - - name: pip install flak8 - run: pip install flake8>=4.1.0 - - name: Lint with flake8 + - name: Install dependencies for linting run: | - flake8 - # stop the build if there are Python syntax errors or undefined names - flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics - # exit-zero treats all errors as warnings. The GitHub editor is 127 chars wide - flake8 . --count --exit-zero --max-complexity=10 --max-line-length=127 --statistics + python -m pip install --upgrade pip + pip install -r requirements/test.txt + - name: Lint with Ruff + run: ruff check . integration_tests: uses: optimizely/python-sdk/.github/workflows/integration_test.yml@master diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d14002e1..9c149710 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -15,7 +15,7 @@ Development process 2. Please follow the [commit message guidelines](https://github.com/angular/angular/blob/master/CONTRIBUTING.md#-commit-message-guidelines) for each commit message. 3. Make sure to add tests! -4. Run `flake8` to ensure there are no lint errors. +4. Run `ruff check .` to ensure there are no lint errors. 5. `git push` your changes to GitHub. 6. Open a PR from your fork into the master branch of the original repo. @@ -34,12 +34,12 @@ Pull request acceptance criteria - Tests are located in `/tests` with one file per class. - Please don't change the `__version__`. We'll take care of bumping the version when we next release. -- Lint your code with Flake8 before submitting. +- Lint your code with Ruff before submitting. Style ----- -We enforce Flake8 rules. +We enforce Ruff linting rules (Flake8-equivalent: pycodestyle + Pyflakes). License ------- diff --git a/optimizely/helpers/validator.py b/optimizely/helpers/validator.py index b9e4fcc5..59328331 100644 --- a/optimizely/helpers/validator.py +++ b/optimizely/helpers/validator.py @@ -193,7 +193,7 @@ def is_user_profile_valid(user_profile: dict[str, Any]) -> bool: if not user_profile: return False - if not type(user_profile) is dict: + if type(user_profile) is not dict: return False if UserProfile.USER_ID_KEY not in user_profile: @@ -203,7 +203,7 @@ def is_user_profile_valid(user_profile: dict[str, Any]) -> bool: return False experiment_bucket_map = user_profile.get(UserProfile.EXPERIMENT_BUCKET_MAP_KEY) - if not type(experiment_bucket_map) is dict: + if type(experiment_bucket_map) is not dict: return False for decision in experiment_bucket_map.values(): diff --git a/requirements/test.txt b/requirements/test.txt index c2e086c8..c54f5bfa 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -1,5 +1,5 @@ coverage -flake8 >= 4.0.1 +ruff >= 0.15.0 funcsigs >= 0.4 pytest >= 6.2.0 pytest-cov diff --git a/ruff.toml b/ruff.toml new file mode 100644 index 00000000..5365a7b4 --- /dev/null +++ b/ruff.toml @@ -0,0 +1,38 @@ +# Ruff configuration +# https://docs.astral.sh/ruff/configuration/ + +line-length = 120 +target-version = "py39" + +exclude = [ + ".git", + ".venv", + "__pycache__", + "build", + "dist", + "*.egg-info", + "*virtualenv*", + "optimizely/lib/pymmh3.py", + "tests/testapp/application.py", +] + +[lint] +# Flake8-equivalent rules: +# E, W = pycodestyle (style errors & warnings) +# F = Pyflakes (logic errors, undefined names, unused imports) +select = ["E", "W", "F"] + +# Match current flake8 ignores +# E722 - do not use bare 'except' +ignore = ["E722"] + +# Allow autofix for all rules +fixable = ["ALL"] +unfixable = [] + +[lint.per-file-ignores] +# Ignore unused imports in __init__ files +"__init__.py" = ["F401"] + +[lint.isort] +known-first-party = ["optimizely"] From da4caeba4604859507f943aba5dfd09c4b7996a4 Mon Sep 17 00:00:00 2001 From: Farhan Anjum Date: Mon, 23 Feb 2026 22:41:34 +0600 Subject: [PATCH 04/12] [FSSDK-12320] bugfix: PollingConfigManager rejects valid URL-only configuration (#497) * fix: update PollingConfigManager initialization requirements and enhance tests for url handling * fix: clarify initialization requirements for PollingConfigManager --- optimizely/config_manager.py | 3 --- tests/test_config_manager.py | 32 +++++++++++++++++++++++++++++++- 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/optimizely/config_manager.py b/optimizely/config_manager.py index 3dce2741..21c74973 100644 --- a/optimizely/config_manager.py +++ b/optimizely/config_manager.py @@ -236,9 +236,6 @@ def __init__( ) self._sdk_key = sdk_key or self._sdk_key - if self._sdk_key is None: - raise optimizely_exceptions.InvalidInputException(enums.Errors.MISSING_SDK_KEY) - self.datafile_url = self.get_datafile_url( self._sdk_key, url, url_template or self.DATAFILE_URL_TEMPLATE ) diff --git a/tests/test_config_manager.py b/tests/test_config_manager.py index 1930520e..0e8cb7b3 100644 --- a/tests/test_config_manager.py +++ b/tests/test_config_manager.py @@ -224,12 +224,34 @@ def test_init__no_sdk_key_no_datafile__fails(self, _): """ Test that initialization fails if there is no sdk_key or datafile provided. """ self.assertRaisesRegex( optimizely_exceptions.InvalidInputException, - enums.Errors.MISSING_SDK_KEY, + 'Must provide at least one of sdk_key or url.', config_manager.PollingConfigManager, sdk_key=None, datafile=None, ) + def test_init__url_only__succeeds(self, _): + """ Test that initialization succeeds when only url is provided (no sdk_key). """ + test_url = 'https://mydatafiles.com/my_datafile.json' + test_headers = {'Last-Modified': 'New Time'} + test_datafile = json.dumps(self.config_dict_with_features) + test_response = requests.Response() + test_response.status_code = 200 + test_response.headers = test_headers + test_response._content = test_datafile + + with mock.patch('requests.Session.get', return_value=test_response) as mock_request: + project_config_manager = config_manager.PollingConfigManager(url=test_url) + project_config_manager.stop() + + mock_request.assert_called_once_with( + test_url, + headers={}, + timeout=enums.ConfigManager.REQUEST_TIMEOUT + ) + self.assertEqual(test_url, project_config_manager.datafile_url) + self.assertIsInstance(project_config_manager.get_config(), project_config.ProjectConfig) + def test_get_datafile_url__no_sdk_key_no_url_raises(self, _): """ Test that get_datafile_url raises exception if no sdk_key or url is provided. """ self.assertRaisesRegex( @@ -273,6 +295,14 @@ def test_get_datafile_url__sdk_key_and_template_provided(self, _): expected_url, config_manager.PollingConfigManager.get_datafile_url(test_sdk_key, None, test_url_template), ) + def test_get_datafile_url__url_provided(self, _): + """ Test get_datafile_url when only url is provided (no sdk_key). """ + test_url = 'www.myoptimizelydatafiles.com/my_key.json' + # url_template is ignored when url is provided + self.assertEqual( + test_url, config_manager.PollingConfigManager.get_datafile_url(None, test_url, None), + ) + def test_get_datafile_url__url_and_template_provided(self, _): """ Test get_datafile_url when url and url_template are provided. """ test_url_template = 'www.optimizelydatafiles.com/{sdk_key}.json' From d6519113a1f522cb6ede15a9574fcc267e2ccf45 Mon Sep 17 00:00:00 2001 From: Farhan Anjum Date: Fri, 27 Feb 2026 01:16:34 +0600 Subject: [PATCH 05/12] [FSSDK-12317] update: clear arnica risks (#498) * chore: remove outdated source clear GitHub Actions workflow * chore: add persist-credentials option to checkout steps in workflows --- .github/workflows/integration_test.yml | 1 + .github/workflows/python.yml | 8 ++++++++ .github/workflows/source_clear_cron.yml | 16 ---------------- 3 files changed, 9 insertions(+), 16 deletions(-) delete mode 100644 .github/workflows/source_clear_cron.yml diff --git a/.github/workflows/integration_test.yml b/.github/workflows/integration_test.yml index 7619ca51..bc08c2cb 100644 --- a/.github/workflows/integration_test.yml +++ b/.github/workflows/integration_test.yml @@ -17,6 +17,7 @@ jobs: steps: - uses: actions/checkout@v3 with: + persist-credentials: false # You should create a personal access token and store it in your repository token: ${{ secrets.CI_USER_TOKEN }} repository: 'optimizely/travisci-tools' diff --git a/.github/workflows/python.yml b/.github/workflows/python.yml index 20894bbb..830ca0f4 100644 --- a/.github/workflows/python.yml +++ b/.github/workflows/python.yml @@ -14,6 +14,8 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v3 + with: + persist-credentials: false - name: Set up Ruby uses: ruby/setup-ruby@v1 with: @@ -29,6 +31,8 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v3 + with: + persist-credentials: false - name: Set up Python 3.12 uses: actions/setup-python@v4 with: @@ -68,6 +72,8 @@ jobs: - "3.12" steps: - uses: actions/checkout@v3 + with: + persist-credentials: false - name: Set up Python ${{ matrix.python-version }} uses: actions/setup-python@v4 with: @@ -95,6 +101,8 @@ jobs: - "3.12" steps: - uses: actions/checkout@v3 + with: + persist-credentials: false - name: Set up Python ${{ matrix.python-version }} uses: actions/setup-python@v4 with: diff --git a/.github/workflows/source_clear_cron.yml b/.github/workflows/source_clear_cron.yml deleted file mode 100644 index 862b4a3f..00000000 --- a/.github/workflows/source_clear_cron.yml +++ /dev/null @@ -1,16 +0,0 @@ -name: Source clear - -on: - schedule: - # Runs "weekly" - - cron: '0 0 * * 0' - -jobs: - source_clear: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v3 - - name: Source clear scan - env: - SRCCLR_API_TOKEN: ${{ secrets.SRCCLR_API_TOKEN }} - run: curl -sSL https://download.sourceclear.com/ci.sh | bash -s – scan From 55924e5a3e157e627a8e7b5b3c6df19d5a48ffc8 Mon Sep 17 00:00:00 2001 From: Farhan Anjum Date: Tue, 31 Mar 2026 01:57:52 +0600 Subject: [PATCH 06/12] [AI-FSSDK] [FSSDK-12337] Add Feature Rollout support (#499) * [AI-FSSDK] [FSSDK-12337] Add Feature Rollout support to project config parsing * [AI-FSSDK] [FSSDK-12337] Fix test structure, mypy and ruff compliance - Move feature rollout tests from standalone test_feature_rollout.py into test_config.py following module-level testing convention - Use base.BaseTest instead of unittest.TestCase for consistency - Fix mypy strict type errors in Variation construction using cast - All checks pass: ruff, mypy --strict, pytest (941/941) Co-Authored-By: Claude Opus 4.6 (1M context) * [AI-FSSDK] [FSSDK-12337] Simplify feature rollout config parsing - Inline _get_everyone_else_variation logic, remove unnecessary static method - Use get_rollout_from_id() to match TDD pseudocode - Remove isinstance check (rollout experiments are always dicts) - Remove 3 unit tests for deleted helper method (edge cases already covered by integration-level tests) Co-Authored-By: Claude Opus 4.6 (1M context) * [AI-FSSDK] [FSSDK-12337] Restore _get_everyone_else_variation as method - Restore as instance method matching TDD pseudocode structure - Takes flag (FeatureFlag) param, calls get_rollout_from_id internally - Caller simplified to: everyone_else_variation = self._get_everyone_else_variation(flag) * [AI-FSSDK] [FSSDK-12337] Add ExperimentTypes constants for type field - Add ExperimentTypes enum in helpers/enums.py with AB, MAB, CMAB, FEATURE_ROLLOUT - Use ExperimentTypes.FEATURE_ROLLOUT constant in config parsing instead of raw string * [AI-FSSDK] [FSSDK-12337] Type-restrict Experiment.type to ExperimentType - Add ExperimentType Literal type in helpers/types.py: 'a/b', 'mab', 'cmab', 'feature_rollout' - Change Experiment.type from Optional[str] to Optional[ExperimentType] * [AI-FSSDK] [FSSDK-12337] Remove ExperimentTypes class, simplify type check - Remove redundant ExperimentTypes class from enums.py (ExperimentType Literal suffices) - Simplify getattr(experiment, 'type', None) to experiment.type * [AI-FSSDK] [FSSDK-12337] Return Variation entity from _get_everyone_else_variation - Build Variation entity once in helper, derive dict from it in caller - Addresses PR review comment from jaeopt * [AI-FSSDK] [FSSDK-12337] Remove redundant tests, keep 6 essential ones Removed 7 tests that were covered by other tests: - test_experiment_type_field_parsed (covered by injection test) - test_feature_rollout_with_empty_rollout_experiments (similar to no_rollout) - test_feature_rollout_multiple_experiments_mixed_types (covered by injection + unchanged) - test_feature_rollout_flag_variations_map_includes_injected (subset of maps test) - test_experiment_type_ab (just string assignment) - test_feature_rollout_with_variables_on_everyone_else (edge case) - test_existing_datafile_not_broken (covered by none_when_missing + unchanged) * [AI-FSSDK] [FSSDK-12337] Add ExperimentTypes constant and targeted_delivery type - Add ExperimentTypes class in enums.py with ab, mab, cmab, td, fr - Add 'targeted_delivery' to ExperimentType Literal in types.py - Use enums.ExperimentTypes.fr constant in injection check - Add test for type field parsing from datafile * [AI-FSSDK] [FSSDK-12337] Remove test not in ticket spec Remove test_feature_rollout_everyone_else_is_last_rollout_rule to match updated Jira ticket test requirements. * [FSSDK-12337] Fix experiment type values to match backend Update ExperimentTypes and ExperimentType Literal to use actual backend values: 'multi_armed_bandit' and 'contextual_multi_armed_bandit' instead of shorthand 'mab' and 'cmab'. * Format ExperimentType definition for ruff check * [AI-FSSDK] [FSSDK-12337] Update experiment type values to short-form abbreviations * [AI-FSSDK] [FSSDK-12337] Add validation for experiment type field * trigger CI --------- Co-authored-by: Claude Opus 4.6 (1M context) --- optimizely/entities.py | 4 +- optimizely/helpers/enums.py | 8 + optimizely/helpers/types.py | 4 + optimizely/project_config.py | 83 ++++++++ tests/test_config.py | 363 +++++++++++++++++++++++++++++++++++ 5 files changed, 461 insertions(+), 1 deletion(-) diff --git a/optimizely/entities.py b/optimizely/entities.py index 12f4f849..6aa0060d 100644 --- a/optimizely/entities.py +++ b/optimizely/entities.py @@ -22,7 +22,7 @@ if TYPE_CHECKING: # prevent circular dependenacy by skipping import at runtime - from .helpers.types import ExperimentDict, TrafficAllocation, VariableDict, VariationDict, CmabDict + from .helpers.types import ExperimentDict, ExperimentType, TrafficAllocation, VariableDict, VariationDict, CmabDict class BaseEntity: @@ -87,6 +87,7 @@ def __init__( groupId: Optional[str] = None, groupPolicy: Optional[str] = None, cmab: Optional[CmabDict] = None, + type: Optional[ExperimentType] = None, **kwargs: Any ): self.id = id @@ -101,6 +102,7 @@ def __init__( self.groupId = groupId self.groupPolicy = groupPolicy self.cmab = cmab + self.type = type def get_audience_conditions_or_ids(self) -> Sequence[str | list[str]]: """ Returns audienceConditions if present, otherwise audienceIds. """ diff --git a/optimizely/helpers/enums.py b/optimizely/helpers/enums.py index 74acdcfa..2d1d4676 100644 --- a/optimizely/helpers/enums.py +++ b/optimizely/helpers/enums.py @@ -229,6 +229,14 @@ class OdpManagerConfig: EVENT_TYPE: Final = 'fullstack' +class ExperimentTypes: + ab: Final = 'ab' + mab: Final = 'mab' + cmab: Final = 'cmab' + td: Final = 'td' + fr: Final = 'fr' + + class OdpSegmentsCacheConfig: """ODP Segment Cache configs.""" DEFAULT_CAPACITY: Final = 10_000 diff --git a/optimizely/helpers/types.py b/optimizely/helpers/types.py index d4177dc0..7076dc4d 100644 --- a/optimizely/helpers/types.py +++ b/optimizely/helpers/types.py @@ -117,6 +117,10 @@ class CmabDict(BaseEntity): trafficAllocation: int +ExperimentType = Literal[ + 'ab', 'mab', 'cmab', 'td', 'fr' +] + HoldoutStatus = Literal['Draft', 'Running', 'Concluded', 'Archived'] diff --git a/optimizely/project_config.py b/optimizely/project_config.py index 74442d7a..3aa1cc2b 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -189,7 +189,24 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): self.variation_key_map_by_experiment_id: dict[str, dict[str, Union[entities.Variation, VariationDict]]] = {} self.flag_variations_map: dict[str, list[entities.Variation]] = {} + valid_experiment_types = { + enums.ExperimentTypes.ab, + enums.ExperimentTypes.mab, + enums.ExperimentTypes.cmab, + enums.ExperimentTypes.td, + enums.ExperimentTypes.fr, + } for experiment in self.experiment_id_map.values(): + if experiment.type is not None and experiment.type not in valid_experiment_types: + self.logger.error( + f'Experiment "{experiment.key}" has invalid type "{experiment.type}". ' + f'Valid types: {valid_experiment_types}.' + ) + self.error_handler.handle_error( + exceptions.InvalidExperimentException( + f'Invalid experiment type: {experiment.type}' + ) + ) self.experiment_key_map[experiment.key] = experiment self.variation_key_map[experiment.key] = self._generate_key_map( experiment.variations, 'key', entities.Variation @@ -232,6 +249,37 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): self.experiment_feature_map[exp_id] = [feature.id] rules.append(self.experiment_id_map[exp_id]) + # Feature Rollout support: inject the "everyone else" variation + # into any experiment with type == "feature_rollout" + everyone_else_variation = self._get_everyone_else_variation(feature) + if everyone_else_variation is not None: + for experiment in rules: + if experiment.type == enums.ExperimentTypes.fr: + experiment.variations.append({ + 'id': everyone_else_variation.id, + 'key': everyone_else_variation.key, + 'featureEnabled': everyone_else_variation.featureEnabled, + 'variables': cast( + list[types.VariableDict], + everyone_else_variation.variables, + ), + }) + experiment.trafficAllocation.append({ + 'entityId': everyone_else_variation.id, + 'endOfRange': 10000, + }) + self.variation_key_map[experiment.key][everyone_else_variation.key] = everyone_else_variation + self.variation_id_map[experiment.key][everyone_else_variation.id] = everyone_else_variation + self.variation_id_map_by_experiment_id[experiment.id][everyone_else_variation.id] = ( + everyone_else_variation + ) + self.variation_key_map_by_experiment_id[experiment.id][everyone_else_variation.key] = ( + everyone_else_variation + ) + self.variation_variable_usage_map[everyone_else_variation.id] = self._generate_key_map( + everyone_else_variation.variables, 'id', entities.Variation.VariableUsage + ) + flag_id = feature.id applicable_holdouts: list[entities.Holdout] = [] @@ -667,6 +715,41 @@ def get_rollout_from_id(self, rollout_id: str) -> Optional[entities.Layer]: self.logger.error(f'Rollout with ID "{rollout_id}" is not in datafile.') return None + def _get_everyone_else_variation(self, flag: entities.FeatureFlag) -> Optional[entities.Variation]: + """ Get the "everyone else" variation for a feature flag. + + The "everyone else" rule is the last experiment in the flag's rollout, + and its first variation is the "everyone else" variation. + + Args: + flag: The feature flag to get the everyone else variation for. + + Returns: + The "everyone else" Variation entity, or None if not available. + """ + if not flag.rolloutId: + return None + + rollout = self.get_rollout_from_id(flag.rolloutId) + if not rollout or not rollout.experiments: + return None + + everyone_else_rule = rollout.experiments[-1] + variations = everyone_else_rule.get('variations', []) + if not variations: + return None + + variation_dict = variations[0] + return entities.Variation( + id=variation_dict['id'], + key=variation_dict['key'], + featureEnabled=bool(variation_dict.get('featureEnabled', False)), + variables=cast( + Optional[list[entities.Variable]], + variation_dict.get('variables'), + ), + ) + def get_variable_value_for_variation( self, variable: Optional[entities.Variable], variation: Optional[Union[entities.Variation, VariationDict]] ) -> Optional[str]: diff --git a/tests/test_config.py b/tests/test_config.py index 81228feb..eba36bc4 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1545,3 +1545,366 @@ def test_holdout_initialization__only_processes_running_holdouts(self): boolean_feature_id = '91111' included_for_boolean = self.config_with_holdouts.included_holdouts.get(boolean_feature_id) self.assertIsNone(included_for_boolean) + + +class FeatureRolloutConfigTest(base.BaseTest): + """Tests for Feature Rollout support in ProjectConfig parsing.""" + + def _build_datafile(self, experiments=None, rollouts=None, feature_flags=None): + """Build a minimal valid datafile with the given components.""" + datafile = { + 'version': '4', + 'accountId': '12001', + 'projectId': '111001', + 'revision': '1', + 'experiments': experiments or [], + 'events': [], + 'attributes': [], + 'audiences': [], + 'groups': [], + 'rollouts': rollouts or [], + 'featureFlags': feature_flags or [], + } + return datafile + + def test_experiment_type_field_parsed(self): + """Test that the 'type' field value is preserved on Experiment after config parsing.""" + datafile = self._build_datafile( + experiments=[ + { + 'id': 'exp_1', + 'key': 'feature_rollout_exp', + 'status': 'Running', + 'forcedVariations': {}, + 'layerId': 'layer_1', + 'audienceIds': [], + 'trafficAllocation': [{'entityId': 'var_1', 'endOfRange': 5000}], + 'variations': [{'key': 'var_1', 'id': 'var_1', 'featureEnabled': True}], + 'type': 'fr', + }, + ], + rollouts=[ + { + 'id': 'rollout_1', + 'experiments': [ + { + 'id': 'rollout_rule_1', + 'key': 'rollout_rule_1', + 'status': 'Running', + 'forcedVariations': {}, + 'layerId': 'rollout_1', + 'audienceIds': [], + 'trafficAllocation': [{'entityId': 'everyone_var', 'endOfRange': 10000}], + 'variations': [ + {'key': 'everyone_var', 'id': 'everyone_var', 'featureEnabled': False} + ], + } + ], + } + ], + feature_flags=[ + { + 'id': 'flag_1', + 'key': 'test_flag', + 'experimentIds': ['exp_1'], + 'rolloutId': 'rollout_1', + 'variables': [], + }, + ], + ) + + opt = optimizely.Optimizely(json.dumps(datafile)) + config = opt.config_manager.get_config() + + experiment = config.experiment_id_map['exp_1'] + self.assertEqual(experiment.type, 'fr') + + def test_experiment_type_field_none_when_missing(self): + """Test that experiments without 'type' field have type=None.""" + datafile = self._build_datafile( + experiments=[ + { + 'id': 'exp_ab', + 'key': 'ab_test_exp', + 'status': 'Running', + 'forcedVariations': {}, + 'layerId': 'layer_1', + 'audienceIds': [], + 'trafficAllocation': [{'entityId': 'var_1', 'endOfRange': 5000}], + 'variations': [{'key': 'var_1', 'id': 'var_1', 'featureEnabled': True}], + }, + ], + feature_flags=[ + { + 'id': 'flag_1', + 'key': 'test_flag', + 'experimentIds': ['exp_ab'], + 'rolloutId': '', + 'variables': [], + }, + ], + ) + + opt = optimizely.Optimizely(json.dumps(datafile)) + config = opt.config_manager.get_config() + + experiment = config.experiment_id_map['exp_ab'] + self.assertIsNone(experiment.type) + + def test_feature_rollout_injects_everyone_else_variation(self): + """Test that feature_rollout experiments get the everyone else variation injected.""" + datafile = self._build_datafile( + experiments=[ + { + 'id': 'exp_fr', + 'key': 'feature_rollout_exp', + 'status': 'Running', + 'forcedVariations': {}, + 'layerId': 'layer_1', + 'audienceIds': [], + 'trafficAllocation': [{'entityId': 'rollout_var', 'endOfRange': 5000}], + 'variations': [ + {'key': 'rollout_var', 'id': 'rollout_var', 'featureEnabled': True} + ], + 'type': 'fr', + }, + ], + rollouts=[ + { + 'id': 'rollout_1', + 'experiments': [ + { + 'id': 'rollout_targeted_rule', + 'key': 'rollout_targeted_rule', + 'status': 'Running', + 'forcedVariations': {}, + 'layerId': 'rollout_1', + 'audienceIds': ['audience_1'], + 'trafficAllocation': [{'entityId': 'targeted_var', 'endOfRange': 10000}], + 'variations': [ + {'key': 'targeted_var', 'id': 'targeted_var', 'featureEnabled': True} + ], + }, + { + 'id': 'rollout_everyone_else', + 'key': 'rollout_everyone_else', + 'status': 'Running', + 'forcedVariations': {}, + 'layerId': 'rollout_1', + 'audienceIds': [], + 'trafficAllocation': [ + {'entityId': 'everyone_else_var', 'endOfRange': 10000} + ], + 'variations': [ + { + 'key': 'everyone_else_var', + 'id': 'everyone_else_var', + 'featureEnabled': False, + } + ], + }, + ], + } + ], + feature_flags=[ + { + 'id': 'flag_1', + 'key': 'test_flag', + 'experimentIds': ['exp_fr'], + 'rolloutId': 'rollout_1', + 'variables': [], + }, + ], + ) + + opt = optimizely.Optimizely(json.dumps(datafile)) + config = opt.config_manager.get_config() + + experiment = config.experiment_id_map['exp_fr'] + + # Should now have 2 variations: original + everyone else + self.assertEqual(len(experiment.variations), 2) + + # Verify the everyone else variation was appended + variation_ids = [v['id'] if isinstance(v, dict) else v.id for v in experiment.variations] + self.assertIn('everyone_else_var', variation_ids) + + # Verify traffic allocation was appended with endOfRange=10000 + self.assertEqual(len(experiment.trafficAllocation), 2) + last_allocation = experiment.trafficAllocation[-1] + self.assertEqual(last_allocation['entityId'], 'everyone_else_var') + self.assertEqual(last_allocation['endOfRange'], 10000) + + def test_feature_rollout_variation_maps_updated(self): + """Test that variation maps are properly updated after injection.""" + datafile = self._build_datafile( + experiments=[ + { + 'id': 'exp_fr', + 'key': 'feature_rollout_exp', + 'status': 'Running', + 'forcedVariations': {}, + 'layerId': 'layer_1', + 'audienceIds': [], + 'trafficAllocation': [{'entityId': 'rollout_var', 'endOfRange': 5000}], + 'variations': [ + {'key': 'rollout_var', 'id': 'rollout_var', 'featureEnabled': True} + ], + 'type': 'fr', + }, + ], + rollouts=[ + { + 'id': 'rollout_1', + 'experiments': [ + { + 'id': 'rollout_everyone_else', + 'key': 'rollout_everyone_else', + 'status': 'Running', + 'forcedVariations': {}, + 'layerId': 'rollout_1', + 'audienceIds': [], + 'trafficAllocation': [ + {'entityId': 'everyone_else_var', 'endOfRange': 10000} + ], + 'variations': [ + { + 'key': 'everyone_else_var', + 'id': 'everyone_else_var', + 'featureEnabled': False, + } + ], + }, + ], + } + ], + feature_flags=[ + { + 'id': 'flag_1', + 'key': 'test_flag', + 'experimentIds': ['exp_fr'], + 'rolloutId': 'rollout_1', + 'variables': [], + }, + ], + ) + + opt = optimizely.Optimizely(json.dumps(datafile)) + config = opt.config_manager.get_config() + + # Check variation_key_map is updated + self.assertIn('everyone_else_var', config.variation_key_map['feature_rollout_exp']) + + # Check variation_id_map is updated + self.assertIn('everyone_else_var', config.variation_id_map['feature_rollout_exp']) + + # Check variation_id_map_by_experiment_id is updated + self.assertIn('everyone_else_var', config.variation_id_map_by_experiment_id['exp_fr']) + + # Check variation_key_map_by_experiment_id is updated + self.assertIn('everyone_else_var', config.variation_key_map_by_experiment_id['exp_fr']) + + def test_non_feature_rollout_experiments_unchanged(self): + """Test that experiments without type=feature_rollout are not modified.""" + datafile = self._build_datafile( + experiments=[ + { + 'id': 'exp_ab', + 'key': 'ab_test_exp', + 'status': 'Running', + 'forcedVariations': {}, + 'layerId': 'layer_1', + 'audienceIds': [], + 'trafficAllocation': [{'entityId': 'var_1', 'endOfRange': 5000}], + 'variations': [ + {'key': 'var_1', 'id': 'var_1', 'featureEnabled': True} + ], + 'type': 'ab', + }, + ], + rollouts=[ + { + 'id': 'rollout_1', + 'experiments': [ + { + 'id': 'rollout_everyone_else', + 'key': 'rollout_everyone_else', + 'status': 'Running', + 'forcedVariations': {}, + 'layerId': 'rollout_1', + 'audienceIds': [], + 'trafficAllocation': [ + {'entityId': 'everyone_else_var', 'endOfRange': 10000} + ], + 'variations': [ + { + 'key': 'everyone_else_var', + 'id': 'everyone_else_var', + 'featureEnabled': False, + } + ], + }, + ], + } + ], + feature_flags=[ + { + 'id': 'flag_1', + 'key': 'test_flag', + 'experimentIds': ['exp_ab'], + 'rolloutId': 'rollout_1', + 'variables': [], + }, + ], + ) + + opt = optimizely.Optimizely(json.dumps(datafile)) + config = opt.config_manager.get_config() + + experiment = config.experiment_id_map['exp_ab'] + + # Should still have only 1 variation + self.assertEqual(len(experiment.variations), 1) + # Should still have only 1 traffic allocation + self.assertEqual(len(experiment.trafficAllocation), 1) + + def test_feature_rollout_with_no_rollout(self): + """Test feature_rollout experiment with empty rolloutId is not modified.""" + datafile = self._build_datafile( + experiments=[ + { + 'id': 'exp_fr', + 'key': 'feature_rollout_exp', + 'status': 'Running', + 'forcedVariations': {}, + 'layerId': 'layer_1', + 'audienceIds': [], + 'trafficAllocation': [{'entityId': 'var_1', 'endOfRange': 5000}], + 'variations': [ + {'key': 'var_1', 'id': 'var_1', 'featureEnabled': True} + ], + 'type': 'fr', + }, + ], + feature_flags=[ + { + 'id': 'flag_1', + 'key': 'test_flag', + 'experimentIds': ['exp_fr'], + 'rolloutId': '', + 'variables': [], + }, + ], + ) + + opt = optimizely.Optimizely(json.dumps(datafile)) + config = opt.config_manager.get_config() + + experiment = config.experiment_id_map['exp_fr'] + + # Without a rollout, no injection should occur + self.assertEqual(len(experiment.variations), 1) + self.assertEqual(len(experiment.trafficAllocation), 1) + + + From 54ae4c6bd51d9a1475b2af04b02060d64d700162 Mon Sep 17 00:00:00 2001 From: Farhan Anjum Date: Wed, 8 Apr 2026 22:09:09 +0600 Subject: [PATCH 07/12] [AI-FSSDK] [FSSDK-12418] Remove experiment type validation from config parsing (#500) --- optimizely/entities.py | 4 ++-- optimizely/project_config.py | 17 ----------------- tests/test_config.py | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 37 insertions(+), 19 deletions(-) diff --git a/optimizely/entities.py b/optimizely/entities.py index 6aa0060d..589ca984 100644 --- a/optimizely/entities.py +++ b/optimizely/entities.py @@ -22,7 +22,7 @@ if TYPE_CHECKING: # prevent circular dependenacy by skipping import at runtime - from .helpers.types import ExperimentDict, ExperimentType, TrafficAllocation, VariableDict, VariationDict, CmabDict + from .helpers.types import ExperimentDict, TrafficAllocation, VariableDict, VariationDict, CmabDict class BaseEntity: @@ -87,7 +87,7 @@ def __init__( groupId: Optional[str] = None, groupPolicy: Optional[str] = None, cmab: Optional[CmabDict] = None, - type: Optional[ExperimentType] = None, + type: Optional[str] = None, **kwargs: Any ): self.id = id diff --git a/optimizely/project_config.py b/optimizely/project_config.py index 3aa1cc2b..5b752538 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -189,24 +189,7 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): self.variation_key_map_by_experiment_id: dict[str, dict[str, Union[entities.Variation, VariationDict]]] = {} self.flag_variations_map: dict[str, list[entities.Variation]] = {} - valid_experiment_types = { - enums.ExperimentTypes.ab, - enums.ExperimentTypes.mab, - enums.ExperimentTypes.cmab, - enums.ExperimentTypes.td, - enums.ExperimentTypes.fr, - } for experiment in self.experiment_id_map.values(): - if experiment.type is not None and experiment.type not in valid_experiment_types: - self.logger.error( - f'Experiment "{experiment.key}" has invalid type "{experiment.type}". ' - f'Valid types: {valid_experiment_types}.' - ) - self.error_handler.handle_error( - exceptions.InvalidExperimentException( - f'Invalid experiment type: {experiment.type}' - ) - ) self.experiment_key_map[experiment.key] = experiment self.variation_key_map[experiment.key] = self._generate_key_map( experiment.variations, 'key', entities.Variation diff --git a/tests/test_config.py b/tests/test_config.py index eba36bc4..c21f9b34 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1651,6 +1651,41 @@ def test_experiment_type_field_none_when_missing(self): experiment = config.experiment_id_map['exp_ab'] self.assertIsNone(experiment.type) + def test_unknown_experiment_type_accepted(self): + """Test that experiments with unknown type values are accepted without error.""" + datafile = self._build_datafile( + experiments=[ + { + 'id': 'exp_unknown', + 'key': 'unknown_type_exp', + 'status': 'Running', + 'forcedVariations': {}, + 'layerId': 'layer_1', + 'audienceIds': [], + 'trafficAllocation': [{'entityId': 'var_1', 'endOfRange': 5000}], + 'variations': [{'key': 'var_1', 'id': 'var_1', 'featureEnabled': True}], + 'type': 'new_unknown_type', + }, + ], + feature_flags=[ + { + 'id': 'flag_1', + 'key': 'test_flag', + 'experimentIds': ['exp_unknown'], + 'rolloutId': '', + 'variables': [], + }, + ], + ) + + opt = optimizely.Optimizely(json.dumps(datafile)) + config = opt.config_manager.get_config() + + self.assertIsNotNone(config) + experiment = config.experiment_id_map['exp_unknown'] + self.assertEqual(experiment.type, 'new_unknown_type') + self.assertEqual(experiment.key, 'unknown_type_exp') + def test_feature_rollout_injects_everyone_else_variation(self): """Test that feature_rollout experiments get the everyone else variation injected.""" datafile = self._build_datafile( From f02b3ce1cf38382a46f0d90f2dbe3ec4c282df65 Mon Sep 17 00:00:00 2001 From: Farhan Anjum Date: Fri, 17 Apr 2026 14:52:56 +0600 Subject: [PATCH 08/12] [FSSDK-12497] Fix return in finally block silently swallowing exceptions (#505) Move error-handling logic out of finally block into normal control flow and change bare except to except Exception. Per Python docs, a return in finally suppresses any in-flight exception, which silently swallows errors raised inside except blocks. Fixes https://github.com/optimizely/python-sdk/issues/439 Co-authored-by: Claude Opus 4.6 (1M context) --- optimizely/config_manager.py | 12 ++++++------ tests/test_config_manager.py | 30 ++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/optimizely/config_manager.py b/optimizely/config_manager.py index 21c74973..22468b05 100644 --- a/optimizely/config_manager.py +++ b/optimizely/config_manager.py @@ -145,14 +145,14 @@ def _set_config(self, datafile: Optional[str | bytes]) -> None: except optimizely_exceptions.UnsupportedDatafileVersionException as error: error_msg = error.args[0] error_to_handle = error - except: + except Exception: error_msg = enums.Errors.INVALID_INPUT.format('datafile') error_to_handle = optimizely_exceptions.InvalidInputException(error_msg) - finally: - if error_msg or config is None: - self.logger.error(error_msg) - self.error_handler.handle_error(error_to_handle or Exception('Unknown Error')) - return + + if error_msg or config is None: + self.logger.error(error_msg) + self.error_handler.handle_error(error_to_handle or Exception('Unknown Error')) + return previous_revision = self._config.get_revision() if self._config else None diff --git a/tests/test_config_manager.py b/tests/test_config_manager.py index 0e8cb7b3..8fa19693 100644 --- a/tests/test_config_manager.py +++ b/tests/test_config_manager.py @@ -200,6 +200,36 @@ def test_set_config__invalid_datafile(self): mock_logger.error.assert_called_once_with('Provided "datafile" is in an invalid format.') self.assertEqual(0, mock_notification_center.call_count) + def test_set_config__exception_in_except_block_is_not_swallowed(self): + """ Test that an exception raised inside the except block propagates + to the caller instead of being silently swallowed. + + Per Python docs, a return inside a finally clause suppresses any + in-flight exception. The _set_config method must not use return in + finally, so that unexpected errors in the error-handling path are + surfaced rather than silently lost. + """ + test_datafile = json.dumps(self.config_dict_with_features) + mock_logger = mock.Mock() + + with mock.patch('optimizely.config_manager.BaseConfigManager._validate_instantiation_options'): + project_config_manager = config_manager.StaticConfigManager( + datafile=test_datafile, logger=mock_logger, + ) + + # Make ProjectConfig raise so we enter the bare except block, + # then make the except block itself raise by breaking INVALID_INPUT. + with mock.patch( + 'optimizely.project_config.ProjectConfig.__init__', + side_effect=Exception('something broke'), + ), mock.patch.object( + enums.Errors, 'INVALID_INPUT', new=None, + ): + # The except block will raise AttributeError ('NoneType' has no 'format'). + # Correct behavior: this exception must propagate to the caller. + with self.assertRaises(AttributeError): + project_config_manager._set_config(test_datafile) + def test_get_config(self): """ Test get_config. """ test_datafile = json.dumps(self.config_dict_with_features) From c05c09b375574da9ad48bdeaebe8ea614e28f129 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Fri, 24 Apr 2026 08:01:03 -0700 Subject: [PATCH 09/12] [AI-FSSDK] [FSSDK-12368] Cleanup flag base setup (local holdouts) (#507) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [FSSDK-12368] Remove legacy flag-level holdout fields Remove deprecated flag-level holdout functionality as cleanup after local holdouts (FSSDK-12369) implementation. Changes: - Removed includedFlags and excludedFlags from Holdout entity - Removed includedFlags and excludedFlags from HoldoutDict type - Removed get_holdouts_for_flag() method from ProjectConfig - Removed flag-level holdout infrastructure: - global_holdouts list - included_holdouts dict - excluded_holdouts dict - flag_holdouts_map dict - Flag-level population logic in ProjectConfig.__init__ - Removed flag-level holdout checking from DecisionService.get_variation_for_feature() - Removed 10 test methods testing removed functionality - Removed includedFlags/excludedFlags from all test fixtures Impact: 7 files modified, 310 lines deleted, 5 lines added (net: -305 lines) Verification: - grep for "includedFlags", "excludedFlags", "getHoldoutsForFlag" returns zero results - grep for "global_holdouts", "included_holdouts", "excluded_holdouts", "flag_holdouts_map" returns zero results Co-Authored-By: Claude Sonnet 4.5 * [FSSDK-12368] Restore holdout evaluation in decide flow, remove only flag-level include/exclude fields The previous cleanup removed the entire holdout evaluation from the decide flow. This restores holdout checking while keeping the removal of legacy includedFlags/excludedFlags fields — all running holdouts now apply to all flags uniformly. Co-Authored-By: Claude Opus 4.6 (1M context) --------- Co-authored-by: Claude Sonnet 4.5 Co-authored-by: Farhan Anjum --- optimizely/entities.py | 4 - optimizely/helpers/types.py | 2 - optimizely/project_config.py | 40 +------- tests/test_bucketing_holdout.py | 4 - tests/test_config.py | 95 +---------------- tests/test_decision_service_holdout.py | 137 ------------------------- 6 files changed, 6 insertions(+), 276 deletions(-) diff --git a/optimizely/entities.py b/optimizely/entities.py index 589ca984..f67dee89 100644 --- a/optimizely/entities.py +++ b/optimizely/entities.py @@ -222,8 +222,6 @@ def __init__( variations: list[VariationDict], trafficAllocation: list[TrafficAllocation], audienceIds: list[str], - includedFlags: Optional[list[str]] = None, - excludedFlags: Optional[list[str]] = None, audienceConditions: Optional[Sequence[str | list[str]]] = None, **kwargs: Any ): @@ -234,8 +232,6 @@ def __init__( self.trafficAllocation = trafficAllocation self.audienceIds = audienceIds self.audienceConditions = audienceConditions - self.includedFlags = includedFlags or [] - self.excludedFlags = excludedFlags or [] def get_audience_conditions_or_ids(self) -> Sequence[str | list[str]]: """Returns audienceConditions if present, otherwise audienceIds. diff --git a/optimizely/helpers/types.py b/optimizely/helpers/types.py index 7076dc4d..6b704c36 100644 --- a/optimizely/helpers/types.py +++ b/optimizely/helpers/types.py @@ -130,5 +130,3 @@ class HoldoutDict(ExperimentDict): Extends ExperimentDict with holdout-specific properties. """ holdoutStatus: HoldoutStatus - includedFlags: list[str] - excludedFlags: list[str] diff --git a/optimizely/project_config.py b/optimizely/project_config.py index 5b752538..0fb0f35f 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -93,9 +93,6 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): holdouts_data: list[types.HoldoutDict] = config.get('holdouts', []) self.holdouts: list[entities.Holdout] = [] self.holdout_id_map: dict[str, entities.Holdout] = {} - self.global_holdouts: list[entities.Holdout] = [] - self.included_holdouts: dict[str, list[entities.Holdout]] = {} - self.excluded_holdouts: dict[str, list[entities.Holdout]] = {} self.flag_holdouts_map: dict[str, list[entities.Holdout]] = {} # Convert holdout dicts to Holdout entities @@ -104,33 +101,13 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): holdout = entities.Holdout(**holdout_data) self.holdouts.append(holdout) - # Only process Running holdouts but doing it here for efficiency like the original Python implementation) + # Only process Running holdouts if not holdout.is_activated: continue # Map by ID for quick lookup self.holdout_id_map[holdout.id] = holdout - # Categorize as global vs flag-specific - # Global holdouts: apply to all flags unless explicitly excluded - # Flag-specific holdouts: only apply to explicitly included flags - if not holdout.includedFlags: - # This is a global holdout - self.global_holdouts.append(holdout) - - # Track which flags this global holdout excludes - if holdout.excludedFlags: - for flag_id in holdout.excludedFlags: - if flag_id not in self.excluded_holdouts: - self.excluded_holdouts[flag_id] = [] - self.excluded_holdouts[flag_id].append(holdout) - else: - # This holdout applies to specific flags only - for flag_id in holdout.includedFlags: - if flag_id not in self.included_holdouts: - self.included_holdouts[flag_id] = [] - self.included_holdouts[flag_id].append(holdout) - # Utility maps for quick lookup self.group_id_map: dict[str, entities.Group] = self._generate_key_map(self.groups, 'id', entities.Group) self.experiment_id_map: dict[str, entities.Experiment] = self._generate_key_map( @@ -263,19 +240,8 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): everyone_else_variation.variables, 'id', entities.Variation.VariableUsage ) - flag_id = feature.id - applicable_holdouts: list[entities.Holdout] = [] - - # Add global holdouts first, excluding any that are explicitly excluded for this flag - excluded_holdouts = self.excluded_holdouts.get(flag_id, []) - for holdout in self.global_holdouts: - if holdout not in excluded_holdouts: - applicable_holdouts.append(holdout) - - # Add flag-specific local holdouts AFTER global holdouts - if flag_id in self.included_holdouts: - applicable_holdouts.extend(self.included_holdouts[flag_id]) - + # Map all running holdouts to this flag + applicable_holdouts = list(self.holdout_id_map.values()) if applicable_holdouts: self.flag_holdouts_map[feature.key] = applicable_holdouts diff --git a/tests/test_bucketing_holdout.py b/tests/test_bucketing_holdout.py index e3d9bcb3..8a5e7c78 100644 --- a/tests/test_bucketing_holdout.py +++ b/tests/test_bucketing_holdout.py @@ -62,8 +62,6 @@ def setUp(self): 'id': 'holdout_1', 'key': 'test_holdout', 'status': 'Running', - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -92,8 +90,6 @@ def setUp(self): 'id': 'holdout_empty_1', 'key': 'empty_holdout', 'status': 'Running', - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': [], 'variations': [], 'trafficAllocation': [] diff --git a/tests/test_config.py b/tests/test_config.py index c21f9b34..29b1c49e 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1384,10 +1384,6 @@ def setUp(self): # Create config with holdouts config_body_with_holdouts = self.config_dict_with_features.copy() - # Use correct feature flag IDs from the datafile - boolean_feature_id = '91111' # boolean_single_variable_feature - multi_variate_feature_id = '91114' # test_feature_in_experiment_and_rollout - config_body_with_holdouts['holdouts'] = [ { 'id': 'holdout_1', @@ -1395,9 +1391,7 @@ def setUp(self): 'status': 'Running', 'variations': [], 'trafficAllocation': [], - 'audienceIds': [], - 'includedFlags': [], - 'excludedFlags': [boolean_feature_id] + 'audienceIds': [] }, { 'id': 'holdout_2', @@ -1405,9 +1399,7 @@ def setUp(self): 'status': 'Running', 'variations': [], 'trafficAllocation': [], - 'audienceIds': [], - 'includedFlags': [multi_variate_feature_id], - 'excludedFlags': [] + 'audienceIds': [] }, { 'id': 'holdout_3', @@ -1415,9 +1407,7 @@ def setUp(self): 'status': 'Inactive', 'variations': [], 'trafficAllocation': [], - 'audienceIds': [], - 'includedFlags': [boolean_feature_id], - 'excludedFlags': [] + 'audienceIds': [] } ] @@ -1425,55 +1415,6 @@ def setUp(self): opt_obj = optimizely.Optimizely(self.config_json_with_holdouts) self.config_with_holdouts = opt_obj.config_manager.get_config() - def test_get_holdouts_for_flag__non_existent_flag(self): - """ Test that get_holdouts_for_flag returns empty array for non-existent flag. """ - - holdouts = self.config_with_holdouts.get_holdouts_for_flag('non_existent_flag') - self.assertEqual([], holdouts) - - def test_get_holdouts_for_flag__returns_global_and_specific_holdouts(self): - """ Test that get_holdouts_for_flag returns global holdouts that do not exclude the flag - and specific holdouts that include the flag. """ - - holdouts = self.config_with_holdouts.get_holdouts_for_flag('test_feature_in_experiment_and_rollout') - self.assertEqual(2, len(holdouts)) - - global_holdout = next((h for h in holdouts if h['key'] == 'global_holdout'), None) - self.assertIsNotNone(global_holdout) - self.assertEqual('holdout_1', global_holdout['id']) - - specific_holdout = next((h for h in holdouts if h['key'] == 'specific_holdout'), None) - self.assertIsNotNone(specific_holdout) - self.assertEqual('holdout_2', specific_holdout['id']) - - def test_get_holdouts_for_flag__excludes_global_holdouts_for_excluded_flags(self): - """ Test that get_holdouts_for_flag does not return global holdouts that exclude the flag. """ - - holdouts = self.config_with_holdouts.get_holdouts_for_flag('boolean_single_variable_feature') - self.assertEqual(0, len(holdouts)) - - global_holdout = next((h for h in holdouts if h['key'] == 'global_holdout'), None) - self.assertIsNone(global_holdout) - - def test_get_holdouts_for_flag__caches_results(self): - """ Test that get_holdouts_for_flag caches results for subsequent calls. """ - - holdouts1 = self.config_with_holdouts.get_holdouts_for_flag('test_feature_in_experiment_and_rollout') - holdouts2 = self.config_with_holdouts.get_holdouts_for_flag('test_feature_in_experiment_and_rollout') - - # Should be the same object (cached) - self.assertIs(holdouts1, holdouts2) - self.assertEqual(2, len(holdouts1)) - - def test_get_holdouts_for_flag__returns_only_global_for_non_targeted_flags(self): - """ Test that get_holdouts_for_flag returns only global holdouts for flags not specifically targeted. """ - - holdouts = self.config_with_holdouts.get_holdouts_for_flag('test_feature_in_rollout') - - # Should only include global holdout (not excluded and no specific targeting) - self.assertEqual(1, len(holdouts)) - self.assertEqual('global_holdout', holdouts[0]['key']) - def test_get_holdout__returns_holdout_for_valid_id(self): """ Test that get_holdout returns holdout when valid ID is provided. """ @@ -1516,36 +1457,6 @@ def test_get_holdout__does_not_log_when_found(self): self.assertIsNotNone(result) mock_logger.error.assert_not_called() - def test_holdout_initialization__categorizes_holdouts_properly(self): - """ Test that holdouts are properly categorized during initialization. """ - - self.assertIn('holdout_1', self.config_with_holdouts.holdout_id_map) - self.assertIn('holdout_2', self.config_with_holdouts.holdout_id_map) - # Check if a holdout with ID 'holdout_1' exists in global_holdouts - holdout_ids_in_global = [h.id for h in self.config_with_holdouts.global_holdouts] - self.assertIn('holdout_1', holdout_ids_in_global) - - # Use correct feature flag IDs - boolean_feature_id = '91111' - multi_variate_feature_id = '91114' - - self.assertIn(multi_variate_feature_id, self.config_with_holdouts.included_holdouts) - self.assertTrue(len(self.config_with_holdouts.included_holdouts[multi_variate_feature_id]) > 0) - self.assertNotIn(boolean_feature_id, self.config_with_holdouts.included_holdouts) - - self.assertIn(boolean_feature_id, self.config_with_holdouts.excluded_holdouts) - self.assertTrue(len(self.config_with_holdouts.excluded_holdouts[boolean_feature_id]) > 0) - - def test_holdout_initialization__only_processes_running_holdouts(self): - """ Test that only running holdouts are processed during initialization. """ - - self.assertNotIn('holdout_3', self.config_with_holdouts.holdout_id_map) - self.assertNotIn('holdout_3', self.config_with_holdouts.global_holdouts) - - boolean_feature_id = '91111' - included_for_boolean = self.config_with_holdouts.included_holdouts.get(boolean_feature_id) - self.assertIsNone(included_for_boolean) - class FeatureRolloutConfigTest(base.BaseTest): """Tests for Feature Rollout support in ProjectConfig parsing.""" diff --git a/tests/test_decision_service_holdout.py b/tests/test_decision_service_holdout.py index e286ba5c..b5584a37 100644 --- a/tests/test_decision_service_holdout.py +++ b/tests/test_decision_service_holdout.py @@ -38,16 +38,11 @@ def setUp(self): # Create a config dict with holdouts and feature flags config_dict_with_holdouts = self.config_dict_with_features.copy() - # Get feature flag ID for test_feature_in_experiment - test_feature_id = '91111' - config_dict_with_holdouts['holdouts'] = [ { 'id': 'holdout_1', 'key': 'test_holdout', 'status': 'Running', - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -76,8 +71,6 @@ def setUp(self): 'id': 'holdout_2', 'key': 'excluded_holdout', 'status': 'Running', - 'includedFlags': [], - 'excludedFlags': [test_feature_id], 'audienceIds': [], 'variations': [ { @@ -268,60 +261,6 @@ def test_holdout_populates_decision_reasons(self): # get_variation_for_feature with holdouts tests - def test_user_bucketed_into_holdout_returns_before_experiments(self): - """ - When user is bucketed into holdout, should return holdout decision - before checking experiments or rollouts. - """ - feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') - self.assertIsNotNone(feature_flag) - - user_context = self.opt_obj.create_user_context('testUserId', {}) - - # Mock get_holdouts_for_flag to return holdouts - holdout = self.config_with_holdouts.holdouts[0] if self.config_with_holdouts.holdouts else None - self.assertIsNotNone(holdout) - - holdout_variation = holdout.variations[0] - - # Create a holdout decision - mock_holdout_decision = decision_service.Decision( - experiment=holdout, - variation=holdout_variation, - source=enums.DecisionSources.HOLDOUT, - cmab_uuid=None - ) - - mock_holdout_result = { - 'decision': mock_holdout_decision, - 'error': False, - 'reasons': [] - } - - # Mock get_holdouts_for_flag to return holdouts so the holdout path is taken - with mock.patch.object( - self.config_with_holdouts, - 'get_holdouts_for_flag', - return_value=[holdout] - ): - with mock.patch.object( - self.opt_obj.decision_service, - 'get_variation_for_holdout', - return_value=mock_holdout_result - ): - decision_result = self.opt_obj.decision_service.get_variation_for_feature( - self.config_with_holdouts, - feature_flag, - user_context - ) - - self.assertIsNotNone(decision_result) - - # Decision should be valid and from holdout - decision = decision_result['decision'] - self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) - self.assertIsNotNone(decision.variation) - def test_no_holdout_decision_falls_through_to_experiment_and_rollout(self): """When holdout returns no decision, should fall through to experiment and rollout evaluation.""" feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') @@ -376,42 +315,6 @@ def test_evaluates_holdouts_before_experiments(self): self.assertIsNotNone(decision_result) - def test_evaluates_global_holdouts_for_all_flags(self): - """Should evaluate global holdouts for all flags.""" - feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') - self.assertIsNotNone(feature_flag) - - # Get global holdouts - global_holdouts = [ - h for h in self.config_with_holdouts.holdouts - if not h.includedFlags or len(h.includedFlags) == 0 - ] - - if global_holdouts: - user_context = self.opt_obj.create_user_context('testUserId', {}) - - result = self.decision_service_with_holdouts.get_variations_for_feature_list( - self.config_with_holdouts, - [feature_flag], - user_context, - [] - ) - - self.assertIsNotNone(result) - self.assertIsInstance(result, list) - - def test_respects_included_and_excluded_flags_configuration(self): - """Should respect included and excluded flags configuration.""" - feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') - - if feature_flag: - # Get holdouts for this flag - holdouts_for_flag = self.config_with_holdouts.get_holdouts_for_flag('test_feature_in_experiment') - - # Should not include holdouts that exclude this flag - excluded_holdout = next((h for h in holdouts_for_flag if h.key == 'excluded_holdout'), None) - self.assertIsNone(excluded_holdout) - # Holdout logging and error handling tests def test_logs_when_holdout_evaluation_starts(self): @@ -618,8 +521,6 @@ def test_decide_with_holdout_sends_impression_event(self): 'id': 'holdout_1', 'key': 'test_holdout', 'status': 'Running', - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -727,8 +628,6 @@ def test_decide_with_holdout_does_not_send_impression_when_disabled(self): 'id': 'holdout_1', 'key': 'test_holdout', 'status': 'Running', - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -801,8 +700,6 @@ def notification_callback(notification_type, user_id, user_attributes, decision_ 'id': 'holdout_1', 'key': 'test_holdout', 'status': 'Running', - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -930,8 +827,6 @@ def test_decide_all_with_global_holdout(self): 'id': 'global_holdout', 'key': 'global_test_holdout', 'status': 'Running', - 'includedFlags': [], # Global - applies to all flags - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -968,17 +863,12 @@ def test_decide_all_with_global_holdout(self): def test_decide_all_with_included_flags(self): """Should apply holdout only to included flags in decide_all.""" - # Get feature flag IDs - feature1_id = '91111' # test_feature_in_experiment - config_dict_with_holdouts = self.config_dict_with_features.copy() config_dict_with_holdouts['holdouts'] = [ { 'id': 'included_holdout', 'key': 'specific_holdout', 'status': 'Running', - 'includedFlags': [feature1_id], # Only applies to feature1 - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -1014,16 +904,12 @@ def test_decide_all_with_included_flags(self): def test_decide_all_with_excluded_flags(self): """Should exclude holdout from excluded flags in decide_all.""" - feature1_id = '91111' # test_feature_in_experiment - config_dict_with_holdouts = self.config_dict_with_features.copy() config_dict_with_holdouts['holdouts'] = [ { 'id': 'excluded_holdout', 'key': 'global_except_one', 'status': 'Running', - 'includedFlags': [], # Global - 'excludedFlags': [feature1_id], # Except feature1 'audienceIds': [], 'variations': [ { @@ -1058,9 +944,6 @@ def test_decide_all_with_excluded_flags(self): def test_decide_all_with_multiple_holdouts(self): """Should handle multiple holdouts with correct priority.""" - feature1_id = '91111' - feature2_id = '91112' - config_dict_with_holdouts = self.config_dict_with_features.copy() config_dict_with_holdouts['holdouts'] = [ # Global holdout (applies to all) @@ -1068,8 +951,6 @@ def test_decide_all_with_multiple_holdouts(self): 'id': 'global_holdout', 'key': 'global_holdout', 'status': 'Running', - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -1091,8 +972,6 @@ def test_decide_all_with_multiple_holdouts(self): 'id': 'specific_holdout', 'key': 'specific_holdout', 'status': 'Running', - 'includedFlags': [feature2_id], - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -1114,8 +993,6 @@ def test_decide_all_with_multiple_holdouts(self): 'id': 'excluded_holdout', 'key': 'excluded_holdout', 'status': 'Running', - 'includedFlags': [], - 'excludedFlags': [feature1_id], 'audienceIds': [], 'variations': [ { @@ -1154,8 +1031,6 @@ def test_decide_all_with_enabled_flags_only_option(self): 'id': 'disabled_holdout', 'key': 'disabled_holdout', 'status': 'Running', - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -1203,8 +1078,6 @@ def test_holdout_impression_event_has_correct_metadata(self): 'id': 'metadata_holdout', 'key': 'metadata_test_holdout', 'status': 'Running', - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -1257,8 +1130,6 @@ def test_holdout_impression_respects_send_flag_decisions_false(self): 'id': 'send_flag_holdout', 'key': 'send_flag_test_holdout', 'status': 'Running', - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -1307,8 +1178,6 @@ def test_holdout_not_running_does_not_apply(self): 'id': 'draft_holdout', 'key': 'draft_holdout', 'status': 'Draft', # Not Running - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -1347,8 +1216,6 @@ def test_holdout_concluded_status_does_not_apply(self): 'id': 'concluded_holdout', 'key': 'concluded_holdout', 'status': 'Concluded', - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -1387,8 +1254,6 @@ def test_holdout_archived_status_does_not_apply(self): 'id': 'archived_holdout', 'key': 'archived_holdout', 'status': 'Archived', - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -1431,8 +1296,6 @@ def test_holdout_with_audience_match(self): 'id': 'audience_holdout', 'key': 'audience_test_holdout', 'status': 'Running', - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': ['11154'], # Requires browser_type=chrome 'variations': [ { From 807d75b10fbe71bad9dc75c002a0fd222a6cc956 Mon Sep 17 00:00:00 2001 From: Farhan Anjum Date: Thu, 30 Apr 2026 17:38:08 +0600 Subject: [PATCH 10/12] [FSSDK-12521] chore: preparing for release v5.5.0 (#508) * [FSSDK-12521] chore: preparing for release v5.5.0 Co-Authored-By: Claude Opus 4.6 (1M context) * chore: remove internal tooling entries from changelog Removed ruff linter migration and Arnica code risk warning entries from the v5.5.0 changelog as they are internal CI/tooling changes not relevant to SDK users. Co-Authored-By: Claude Opus 4.6 (1M context) * Update CHANGELOG.md Co-authored-by: Muzahidul Islam <129880873+muzahidul-opti@users.noreply.github.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) Co-authored-by: Muzahidul Islam <129880873+muzahidul-opti@users.noreply.github.com> --- CHANGELOG.md | 28 ++++++++++++++++++++++++++++ optimizely/version.py | 2 +- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f5091a0e..715ee391 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,33 @@ # Optimizely Python SDK Changelog +## 5.5.0 +April 30th, 2026 + +### New Features + +#### **Feature Rollout Support** + +- Added support for Feature Rollouts, a new rule type that combines Targeted Delivery simplicity with A/B test measurement capabilities. During project config parsing, the "everyone else" variation from the flag's rollout is automatically injected into feature rollout experiments, enabling correct evaluation without changes to decision logic. ([#499](https://github.com/optimizely/python-sdk/pull/499)) +- Removed experiment type validation from config parsing to support flexible rule types. ([#500](https://github.com/optimizely/python-sdk/pull/500)) + +--- + +### Enhancements + +- Added event retry support for reliable event dispatching. ([#475](https://github.com/optimizely/python-sdk/pull/475)) +- Excluded CMAB experiments from UserProfileService to prevent stale cached decisions. ([#474](https://github.com/optimizely/python-sdk/pull/474)) +- Cleaned up flag-level holdout fields for simplified configuration. ([#507](https://github.com/optimizely/python-sdk/pull/507)) + +--- + +### Bug Fixes + +- Fixed `return` in `finally` block silently swallowing exceptions. ([#505](https://github.com/optimizely/python-sdk/pull/505)) +- Fixed `PollingConfigManager` rejecting valid URL-only configuration. ([#497](https://github.com/optimizely/python-sdk/pull/497)) + +--- + + ## 5.4.0 December 19th, 2025 diff --git a/optimizely/version.py b/optimizely/version.py index aef5b367..85bb307e 100644 --- a/optimizely/version.py +++ b/optimizely/version.py @@ -11,5 +11,5 @@ # See the License for the specific language governing permissions and # limitations under the License. -version_info = (5, 4, 0) +version_info = (5, 5, 0) __version__ = '.'.join(str(v) for v in version_info) From 013ddf06e9e9bf0f3946908aa5a7c0e2294a9c85 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Mon, 1 Jun 2026 09:03:46 -0700 Subject: [PATCH 11/12] [AI-FSSDK] [FSSDK-12369] Add local holdouts support to Python SDK (#512) * [AI-FSSDK] [FSSDK-12369] Add local holdouts support to Python SDK * [FSSDK-12369] Fix copyright year from 2025 to 2026 in new test files Co-Authored-By: Claude Sonnet 4.6 * [FSSDK-12369] Add mandatory forced-decision-beats-local-holdout enforcement test * trigger CI * trigger CI Co-Authored-By: Claude Opus 4.6 (1M context) --------- Co-authored-by: Claude Sonnet 4.6 Co-authored-by: Farhan Anjum --- optimizely/decision_service.py | 48 +++- optimizely/entities.py | 15 ++ optimizely/helpers/types.py | 1 + optimizely/project_config.py | 47 +++- tests/test_decision_service_holdout.py | 355 ++++++++++++++++++++++++- tests/test_holdout_config.py | 312 ++++++++++++++++++++++ 6 files changed, 759 insertions(+), 19 deletions(-) create mode 100644 tests/test_holdout_config.py diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index be2be2c5..11188908 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -610,6 +610,24 @@ def get_variation_for_rollout( return Decision(experiment=rule, variation=forced_decision_variation, source=enums.DecisionSources.ROLLOUT, cmab_uuid=None), decide_reasons + # Check local holdouts targeting this specific delivery rule (FSSDK-12369) + local_holdouts = project_config.get_holdouts_for_rule(rule.id) + for holdout in local_holdouts: + local_holdout_decision = self.get_variation_for_holdout( + holdout, user_context, project_config + ) + decide_reasons.extend(local_holdout_decision['reasons']) + + local_decision = local_holdout_decision['decision'] + if local_decision.variation is not None: + message = ( + f"The user '{user_id}' is bucketed into local holdout '{holdout.key}' " + f"for delivery rule '{rule.key}'." + ) + self.logger.info(message) + decide_reasons.append(message) + return local_decision, decide_reasons + bucketing_id, bucket_reasons = self._get_bucketing_id(user_id, attributes) decide_reasons += bucket_reasons @@ -733,9 +751,9 @@ def get_decision_for_flag( reasons = decide_reasons.copy() if decide_reasons else [] user_id = user_context.user_id - # Check holdouts - holdouts = project_config.get_holdouts_for_flag(feature_flag.key) - for holdout in holdouts: + # Check global holdouts (flag level — before any rules are evaluated) + global_holdouts = project_config.get_global_holdouts() + for holdout in global_holdouts: holdout_decision = self.get_variation_for_holdout(holdout, user_context, project_config) reasons.extend(holdout_decision['reasons']) @@ -756,7 +774,7 @@ def get_decision_for_flag( 'reasons': reasons } - # If no holdout decision, check experiments then rollouts + # If no global holdout decision, check experiments then rollouts if feature_flag.experimentIds: for experiment_id in feature_flag.experimentIds: experiment = project_config.get_experiment_from_id(experiment_id) @@ -778,6 +796,28 @@ def get_decision_for_flag( 'reasons': reasons } + # Check local holdouts targeting this specific experiment rule (FSSDK-12369) + local_holdouts = project_config.get_holdouts_for_rule(experiment.id) + for holdout in local_holdouts: + local_holdout_decision = self.get_variation_for_holdout( + holdout, user_context, project_config + ) + reasons.extend(local_holdout_decision['reasons']) + + local_decision = local_holdout_decision['decision'] + if local_decision.variation is not None: + message = ( + f"The user '{user_id}' is bucketed into local holdout '{holdout.key}' " + f"for experiment rule '{experiment.key}'." + ) + self.logger.info(message) + reasons.append(message) + return { + 'decision': local_holdout_decision['decision'], + 'error': False, + 'reasons': reasons + } + # Get variation for experiment variation_result = self.get_variation( project_config, experiment, user_context, user_profile_tracker, reasons, decide_options diff --git a/optimizely/entities.py b/optimizely/entities.py index f67dee89..e15595b4 100644 --- a/optimizely/entities.py +++ b/optimizely/entities.py @@ -223,6 +223,7 @@ def __init__( trafficAllocation: list[TrafficAllocation], audienceIds: list[str], audienceConditions: Optional[Sequence[str | list[str]]] = None, + includedRules: Optional[list[str]] = None, **kwargs: Any ): self.id = id @@ -232,6 +233,8 @@ def __init__( self.trafficAllocation = trafficAllocation self.audienceIds = audienceIds self.audienceConditions = audienceConditions + # None = global holdout (applies to all rules); list of rule IDs = local holdout + self.included_rules: Optional[list[str]] = includedRules def get_audience_conditions_or_ids(self) -> Sequence[str | list[str]]: """Returns audienceConditions if present, otherwise audienceIds. @@ -241,6 +244,18 @@ def get_audience_conditions_or_ids(self) -> Sequence[str | list[str]]: """ return self.audienceConditions if self.audienceConditions is not None else self.audienceIds + @property + def is_global(self) -> bool: + """Check if this is a global holdout (applies to all rules across all flags). + + A holdout is global when includedRules is None (absent from datafile). + An empty list [] is a local holdout that targets no rules (different from global). + + Returns: + True if included_rules is None (global), False otherwise (local). + """ + return self.included_rules is None + @property def is_activated(self) -> bool: """Check if the holdout is activated (running). diff --git a/optimizely/helpers/types.py b/optimizely/helpers/types.py index 6b704c36..7ee51373 100644 --- a/optimizely/helpers/types.py +++ b/optimizely/helpers/types.py @@ -130,3 +130,4 @@ class HoldoutDict(ExperimentDict): Extends ExperimentDict with holdout-specific properties. """ holdoutStatus: HoldoutStatus + includedRules: Optional[list[str]] # None = global holdout; list of rule IDs = local holdout diff --git a/optimizely/project_config.py b/optimizely/project_config.py index 0fb0f35f..5ec5f85b 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -93,7 +93,10 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): holdouts_data: list[types.HoldoutDict] = config.get('holdouts', []) self.holdouts: list[entities.Holdout] = [] self.holdout_id_map: dict[str, entities.Holdout] = {} - self.flag_holdouts_map: dict[str, list[entities.Holdout]] = {} + # Global holdouts (includedRules is None) — evaluated at flag level before any rule + self.global_holdouts: list[entities.Holdout] = [] + # Rule-level holdouts — map from rule ID to holdouts targeting that rule + self.rule_holdouts_map: dict[str, list[entities.Holdout]] = {} # Convert holdout dicts to Holdout entities for holdout_data in holdouts_data: @@ -108,6 +111,16 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): # Map by ID for quick lookup self.holdout_id_map[holdout.id] = holdout + # Classify holdout as global or local based on includedRules + if holdout.is_global: + self.global_holdouts.append(holdout) + else: + # Local holdout — register for each targeted rule ID + for rule_id in (holdout.included_rules or []): + if rule_id not in self.rule_holdouts_map: + self.rule_holdouts_map[rule_id] = [] + self.rule_holdouts_map[rule_id].append(holdout) + # Utility maps for quick lookup self.group_id_map: dict[str, entities.Group] = self._generate_key_map(self.groups, 'id', entities.Group) self.experiment_id_map: dict[str, entities.Experiment] = self._generate_key_map( @@ -240,11 +253,6 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): everyone_else_variation.variables, 'id', entities.Variation.VariableUsage ) - # Map all running holdouts to this flag - applicable_holdouts = list(self.holdout_id_map.values()) - if applicable_holdouts: - self.flag_holdouts_map[feature.key] = applicable_holdouts - rollout = None if len(feature.rolloutId) == 0 else self.rollout_id_map[feature.rolloutId] if rollout: for exp in rollout.experiments: @@ -878,19 +886,30 @@ def get_flag_variation( return None - def get_holdouts_for_flag(self, flag_key: str) -> list[entities.Holdout]: - """ Helper method to get holdouts from an applied feature flag. + def get_global_holdouts(self) -> list[entities.Holdout]: + """Return all global holdouts (includedRules is None). - Args: - flag_key: Key of the feature flag. + Global holdouts are evaluated at flag level before any rule is checked. Returns: - The holdouts that apply for a specific flag as Holdout entity objects. + List of global Holdout entities that are currently running. """ - if not self.holdouts: - return [] + return self.global_holdouts + + def get_holdouts_for_rule(self, rule_id: str) -> list[entities.Holdout]: + """Return local holdouts that target a specific rule. - return self.flag_holdouts_map.get(flag_key, []) + Local holdouts are evaluated per-rule, before the rule's audience and + traffic allocation checks. A rule ID not present in any holdout's + includedRules simply returns an empty list — silently skipped. + + Args: + rule_id: The experiment or delivery rule ID to look up. + + Returns: + List of local Holdout entities targeting the given rule ID. + """ + return self.rule_holdouts_map.get(rule_id, []) def get_holdout(self, holdout_id: str) -> Optional[entities.Holdout]: """ Helper method to get holdout from holdout ID. diff --git a/tests/test_decision_service_holdout.py b/tests/test_decision_service_holdout.py index b5584a37..1bf81c93 100644 --- a/tests/test_decision_service_holdout.py +++ b/tests/test_decision_service_holdout.py @@ -1,4 +1,4 @@ -# Copyright 2025 Optimizely and contributors +# Copyright 2026 Optimizely and contributors # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -1331,3 +1331,356 @@ def test_holdout_with_audience_match(self): self.assertIsNotNone(decision_no_match) finally: opt.close() + + +# --------------------------------------------------------------------------- +# Level 2: Local Holdouts Decision Service Tests (FSSDK-12369) +# --------------------------------------------------------------------------- + +_HOLDOUT_VARIATION = [{'id': 'hvar_1', 'key': 'holdout_control', 'variables': []}] +_FULL_TRAFFIC = [{'entityId': 'hvar_1', 'endOfRange': 10000}] +_NO_TRAFFIC = [{'entityId': 'hvar_1', 'endOfRange': 0}] + + +def _holdout(holdout_id, key, included_rules=None, traffic=None, status='Running'): + """Build a minimal holdout datafile dict.""" + h = { + 'id': holdout_id, + 'key': key, + 'status': status, + 'audienceIds': [], + 'variations': _HOLDOUT_VARIATION, + 'trafficAllocation': traffic or _FULL_TRAFFIC, + } + if included_rules is not None: + h['includedRules'] = included_rules + return h + + +class LocalHoldoutDecisionServiceTest(base.BaseTest): + """Level 2 decision service tests for local holdouts (FSSDK-12369). + + These tests exercise the decision flow branches introduced by local holdouts + and must live in the decision service test file per [UNITTEST] spec. + + Experiment '111127' is the experiment rule for feature 'test_feature_in_experiment'. + Rollout '211111' with rule '211147' (no audience, 60% traffic) is the delivery rule + for 'test_feature_in_rollout'. + """ + + def setUp(self): + base.BaseTest.setUp(self) + self.error_handler = error_handler.NoOpErrorHandler() + self.spy_logger = mock.MagicMock(spec=logger.SimpleLogger) + self.spy_logger.logger = self.spy_logger + self.spy_user_profile_service = mock.MagicMock() + self.spy_cmab_service = mock.MagicMock() + + def tearDown(self): + if hasattr(self, 'opt_obj'): + self.opt_obj.close() + + def _make_opt(self, holdouts): + cfg = self.config_dict_with_features.copy() + cfg['holdouts'] = holdouts + self.opt_obj = optimizely_module.Optimizely(json.dumps(cfg)) + return self.opt_obj + + def _decision_svc(self): + return decision_service.DecisionService( + self.spy_logger, + self.spy_user_profile_service, + self.spy_cmab_service, + ) + + # ------------------------------------------------------------------ + # Branch 1: Global holdout evaluated at flag level + # ------------------------------------------------------------------ + + def test_global_holdout_evaluated_before_any_rule(self): + """User in a global holdout returns holdout decision before any rule is evaluated. + + The global holdout has full traffic allocation and no audience restriction, + so the user must be caught at flag level before experiments are checked. + """ + opt = self._make_opt([_holdout('gh1', 'global_full', traffic=_FULL_TRAFFIC)]) + config = opt.config_manager.get_config() + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + self.assertIsNotNone(feature_flag) + + ds = self._decision_svc() + + # Mock get_variation so we can assert it was NOT called (holdout short-circuits) + with mock.patch.object(ds, 'get_variation', wraps=ds.get_variation) as mock_get_variation: + user_ctx = opt.create_user_context('user_in_global_holdout', {}) + result = ds.get_decision_for_flag(feature_flag, user_ctx, config) + + decision = result['decision'] + self.assertIsNotNone(decision) + # Source must be HOLDOUT + self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + # Experiment-level get_variation should not have been called + mock_get_variation.assert_not_called() + + def test_global_holdout_miss_falls_through_to_experiment(self): + """User not in the global holdout falls through to experiment evaluation.""" + # Zero-traffic global holdout — nobody bucketed in + opt = self._make_opt([_holdout('gh1', 'global_zero', traffic=_NO_TRAFFIC)]) + config = opt.config_manager.get_config() + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + self.assertIsNotNone(feature_flag) + + ds = self._decision_svc() + user_ctx = opt.create_user_context('testUserId', {}) + result = ds.get_decision_for_flag(feature_flag, user_ctx, config) + + self.assertIsNotNone(result) + # Source must NOT be HOLDOUT — user fell through to experiment or rollout + decision = result['decision'] + self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) + + # ------------------------------------------------------------------ + # Branch 2: Local holdout hit — user bucketed into local holdout for rule X + # ------------------------------------------------------------------ + + def test_local_holdout_hit_returns_holdout_decision_for_experiment_rule(self): + """User bucketed into local holdout for experiment rule X returns holdout decision. + + The experiment rule ID is '111127' (test_experiment linked to test_feature_in_experiment). + The local holdout targets only this rule with full traffic. + Audience and traffic allocation for the experiment itself must NOT be evaluated. + """ + experiment_rule_id = '111127' + opt = self._make_opt([ + _holdout('lh1', 'local_for_exp', included_rules=[experiment_rule_id], traffic=_FULL_TRAFFIC) + ]) + config = opt.config_manager.get_config() + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + self.assertIsNotNone(feature_flag) + + ds = self._decision_svc() + + with mock.patch.object(ds, 'get_variation', wraps=ds.get_variation) as mock_get_var: + user_ctx = opt.create_user_context('user_in_local_holdout', {}) + result = ds.get_decision_for_flag(feature_flag, user_ctx, config) + + decision = result['decision'] + # User must be caught by local holdout + self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + # Regular experiment evaluation (get_variation) must not have run + mock_get_var.assert_not_called() + + def test_local_holdout_hit_returns_holdout_decision_for_delivery_rule(self): + """User bucketed into local holdout for delivery/rollout rule returns holdout decision. + + Rule '211147' is the everyone-else rollout rule for 'test_feature_in_rollout'. + The local holdout targets this delivery rule with full traffic. + """ + delivery_rule_id = '211147' + opt = self._make_opt([ + _holdout('lh2', 'local_for_delivery', included_rules=[delivery_rule_id], traffic=_FULL_TRAFFIC) + ]) + config = opt.config_manager.get_config() + feature_flag = config.get_feature_from_key('test_feature_in_rollout') + self.assertIsNotNone(feature_flag) + + ds = self._decision_svc() + user_ctx = opt.create_user_context('user_delivery_holdout', {}) + result = ds.get_decision_for_flag(feature_flag, user_ctx, config) + + self.assertIsNotNone(result) + decision = result['decision'] + self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + + # ------------------------------------------------------------------ + # Branch 3: Local holdout miss — user falls through to regular rule evaluation + # ------------------------------------------------------------------ + + def test_local_holdout_miss_falls_through_to_regular_rule_evaluation(self): + """User not bucketed into local holdout falls through to normal experiment evaluation. + + Zero-traffic local holdout — nobody bucketed in — user proceeds to experiment. + """ + experiment_rule_id = '111127' + opt = self._make_opt([ + _holdout('lh_miss', 'local_no_traffic', included_rules=[experiment_rule_id], traffic=_NO_TRAFFIC) + ]) + config = opt.config_manager.get_config() + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + self.assertIsNotNone(feature_flag) + + ds = self._decision_svc() + user_ctx = opt.create_user_context('testUserId', {}) + result = ds.get_decision_for_flag(feature_flag, user_ctx, config) + + self.assertIsNotNone(result) + decision = result['decision'] + # Source must NOT be HOLDOUT — missed the local holdout + self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) + + # ------------------------------------------------------------------ + # Branch 4: Rule specificity — local holdout for rule X does NOT affect rule Y + # ------------------------------------------------------------------ + + def test_local_holdout_for_rule_x_does_not_affect_rule_y(self): + """Local holdout targeting experiment rule '111127' must not apply to rollout rule '211147'. + + get_holdouts_for_rule('211147') should return empty — the holdout only targets '111127'. + """ + experiment_rule_id = '111127' + delivery_rule_id = '211147' + + opt = self._make_opt([ + _holdout('lh_x', 'local_for_exp_only', included_rules=[experiment_rule_id], traffic=_FULL_TRAFFIC) + ]) + config = opt.config_manager.get_config() + + # Verify rule map specificity via project config API + holdouts_for_exp = config.get_holdouts_for_rule(experiment_rule_id) + holdouts_for_delivery = config.get_holdouts_for_rule(delivery_rule_id) + + self.assertEqual(len(holdouts_for_exp), 1) + self.assertEqual(holdouts_for_exp[0].id, 'lh_x') + self.assertEqual(holdouts_for_delivery, []) + + def test_two_rules_each_with_own_local_holdout_are_independent(self): + """Each rule's local holdout evaluation is independent of other rules.""" + exp_rule_id = '111127' + delivery_rule_id = '211147' + + opt = self._make_opt([ + _holdout('lh_exp', 'for_exp', included_rules=[exp_rule_id], traffic=_FULL_TRAFFIC), + _holdout('lh_del', 'for_del', included_rules=[delivery_rule_id], traffic=_FULL_TRAFFIC), + ]) + config = opt.config_manager.get_config() + + exp_holdouts = config.get_holdouts_for_rule(exp_rule_id) + del_holdouts = config.get_holdouts_for_rule(delivery_rule_id) + + exp_ids = {h.id for h in exp_holdouts} + del_ids = {h.id for h in del_holdouts} + + self.assertIn('lh_exp', exp_ids) + self.assertNotIn('lh_del', exp_ids) + + self.assertIn('lh_del', del_ids) + self.assertNotIn('lh_exp', del_ids) + + # ------------------------------------------------------------------ + # Branch 5: Experiment vs delivery rules — local holdout applies to both + # ------------------------------------------------------------------ + + def test_local_holdout_applies_to_experiment_rule(self): + """Local holdout check applies to experiment rules in get_decision_for_flag.""" + experiment_rule_id = '111127' + opt = self._make_opt([ + _holdout('lh_exp', 'for_experiment', included_rules=[experiment_rule_id], traffic=_FULL_TRAFFIC) + ]) + config = opt.config_manager.get_config() + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + + ds = self._decision_svc() + user_ctx = opt.create_user_context('user_exp_holdout', {}) + result = ds.get_decision_for_flag(feature_flag, user_ctx, config) + + self.assertEqual(result['decision'].source, enums.DecisionSources.HOLDOUT) + + def test_local_holdout_applies_to_rollout_delivery_rule(self): + """Local holdout check applies to rollout/delivery rules in get_decision_for_flag.""" + delivery_rule_id = '211147' + opt = self._make_opt([ + _holdout('lh_del', 'for_delivery', included_rules=[delivery_rule_id], traffic=_FULL_TRAFFIC) + ]) + config = opt.config_manager.get_config() + # test_feature_in_rollout only has rollout rules, no experiment rules + feature_flag = config.get_feature_from_key('test_feature_in_rollout') + self.assertIsNotNone(feature_flag) + + ds = self._decision_svc() + user_ctx = opt.create_user_context('user_delivery_holdout', {}) + result = ds.get_decision_for_flag(feature_flag, user_ctx, config) + + self.assertIsNotNone(result) + decision = result['decision'] + self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + + # ------------------------------------------------------------------ + # Precedence: Global → Forced → Local → Regular rule + # ------------------------------------------------------------------ + + def test_global_holdout_takes_precedence_over_local_holdout(self): + """Global holdout evaluated first — user caught before local holdout check.""" + experiment_rule_id = '111127' + opt = self._make_opt([ + _holdout('gh1', 'global_full', traffic=_FULL_TRAFFIC), # global, full traffic + _holdout('lh1', 'local_full', included_rules=[experiment_rule_id], traffic=_FULL_TRAFFIC), + ]) + config = opt.config_manager.get_config() + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + + ds = self._decision_svc() + + # Mock local holdout evaluation to verify it is never reached + with mock.patch.object(config, 'get_holdouts_for_rule', wraps=config.get_holdouts_for_rule) as mock_local: + user_ctx = opt.create_user_context('user_global_first', {}) + result = ds.get_decision_for_flag(feature_flag, user_ctx, config) + + self.assertEqual(result['decision'].source, enums.DecisionSources.HOLDOUT) + # get_holdouts_for_rule should NOT be called — global holdout short-circuited + mock_local.assert_not_called() + + def test_forced_decision_beats_100_percent_local_holdout(self): + """Forced decision MUST win over a 100% traffic local holdout targeting the same rule. + + MANDATORY ENFORCEMENT TEST (cross-sdk guideline): + Setup: User has a forced decision set for rule 'test_experiment'. A local holdout + targets rule '111127' (same rule) with 100% traffic allocation. + Assert: The forced decision variation is returned — NOT the holdout variation. + If this test fails, the per-rule ordering (forced → local holdout → regular) is wrong. + """ + from optimizely.optimizely_user_context import OptimizelyUserContext + + experiment_rule_id = '111127' # id of 'test_experiment' + forced_variation_key = 'control' # a valid variation key in test_experiment + + opt = self._make_opt([ + _holdout('lh_full', 'local_full_traffic', included_rules=[experiment_rule_id], traffic=_FULL_TRAFFIC) + ]) + config = opt.config_manager.get_config() + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + self.assertIsNotNone(feature_flag) + + ds = self._decision_svc() + user_ctx = opt.create_user_context('forced_user', {}) + + # Set forced decision for 'test_feature_in_experiment' with rule 'test_experiment' + decision_context = OptimizelyUserContext.OptimizelyDecisionContext( + flag_key='test_feature_in_experiment', + rule_key='test_experiment' + ) + forced_decision = OptimizelyUserContext.OptimizelyForcedDecision( + variation_key=forced_variation_key + ) + user_ctx.set_forced_decision(decision_context, forced_decision) + + result = ds.get_decision_for_flag(feature_flag, user_ctx, config) + + decision = result['decision'] + self.assertIsNotNone(decision) + # The forced decision variation must be returned, NOT a holdout variation + self.assertEqual(decision.variation.key, forced_variation_key) + self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) + + def test_no_holdouts_at_all_falls_through_to_experiment(self): + """When there are no holdouts, decision falls through to experiment evaluation.""" + opt = self._make_opt([]) + config = opt.config_manager.get_config() + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + + ds = self._decision_svc() + user_ctx = opt.create_user_context('testUserId', {}) + result = ds.get_decision_for_flag(feature_flag, user_ctx, config) + + self.assertIsNotNone(result) + decision = result['decision'] + self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) diff --git a/tests/test_holdout_config.py b/tests/test_holdout_config.py new file mode 100644 index 00000000..3d8cd08c --- /dev/null +++ b/tests/test_holdout_config.py @@ -0,0 +1,312 @@ +# Copyright 2026 Optimizely and contributors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Level 1 tests for holdout config parsing and data model (FSSDK-12369). + +Tests cover: +- isGlobal classification (includedRules == None vs list) +- get_global_holdouts() returns only global holdouts +- get_holdouts_for_rule() returns local holdouts for a specific rule +- Backward compatibility with old datafiles (no includedRules field) +- Edge cases: empty array vs None, unknown rule IDs, cross-flag targeting +""" + +import json +import unittest + +from optimizely import entities +from optimizely import optimizely as optimizely_module +from tests import base + + +HOLDOUT_VARIATION = [ + {'id': 'holdout_var_1', 'key': 'holdout_control', 'variables': []} +] + +FULL_TRAFFIC = [{'entityId': 'holdout_var_1', 'endOfRange': 10000}] +NO_TRAFFIC = [{'entityId': 'holdout_var_1', 'endOfRange': 0}] + + +def _make_holdout( + holdout_id: str, + key: str, + included_rules: object = 'MISSING', + status: str = 'Running', +) -> dict: + """Build a holdout datafile dict. Pass included_rules=None for global, + a list for local, or leave as 'MISSING' to omit the field entirely.""" + h: dict = { + 'id': holdout_id, + 'key': key, + 'status': status, + 'audienceIds': [], + 'variations': HOLDOUT_VARIATION, + 'trafficAllocation': FULL_TRAFFIC, + } + if included_rules != 'MISSING': + h['includedRules'] = included_rules + return h + + +def _build_config(holdouts: list, base_test: 'base.BaseTest') -> 'optimizely_module.Optimizely': + """Create an Optimizely instance with the given holdouts list.""" + config_dict = base_test.config_dict_with_features.copy() + config_dict['holdouts'] = holdouts + return optimizely_module.Optimizely(json.dumps(config_dict)) + + +class HoldoutEntityIsGlobalTest(unittest.TestCase): + """Tests for the Holdout.is_global computed property.""" + + def test_is_global_returns_true_when_included_rules_is_none(self): + """Holdout with includedRules=None should be classified as global.""" + holdout = entities.Holdout( + id='h1', key='global_holdout', status='Running', + variations=HOLDOUT_VARIATION, trafficAllocation=FULL_TRAFFIC, + audienceIds=[], includedRules=None + ) + self.assertTrue(holdout.is_global) + + def test_is_global_returns_true_when_included_rules_not_provided(self): + """Holdout with no includedRules kwarg (default None) is global.""" + holdout = entities.Holdout( + id='h1', key='global_holdout', status='Running', + variations=HOLDOUT_VARIATION, trafficAllocation=FULL_TRAFFIC, + audienceIds=[] + ) + self.assertTrue(holdout.is_global) + + def test_is_global_returns_false_when_included_rules_is_empty_list(self): + """Empty list [] is a local holdout targeting no rules — NOT global.""" + holdout = entities.Holdout( + id='h1', key='local_holdout_no_rules', status='Running', + variations=HOLDOUT_VARIATION, trafficAllocation=FULL_TRAFFIC, + audienceIds=[], includedRules=[] + ) + self.assertFalse(holdout.is_global) + + def test_is_global_returns_false_when_included_rules_has_rule_ids(self): + """Holdout with rule IDs is local — is_global should be False.""" + holdout = entities.Holdout( + id='h1', key='local_holdout', status='Running', + variations=HOLDOUT_VARIATION, trafficAllocation=FULL_TRAFFIC, + audienceIds=[], includedRules=['rule_1', 'rule_2'] + ) + self.assertFalse(holdout.is_global) + + def test_included_rules_stored_correctly(self): + """included_rules attribute should match the passed includedRules value.""" + holdout_global = entities.Holdout( + id='h1', key='g', status='Running', + variations=HOLDOUT_VARIATION, trafficAllocation=FULL_TRAFFIC, + audienceIds=[], includedRules=None + ) + holdout_local = entities.Holdout( + id='h2', key='l', status='Running', + variations=HOLDOUT_VARIATION, trafficAllocation=FULL_TRAFFIC, + audienceIds=[], includedRules=['rule_a', 'rule_b'] + ) + self.assertIsNone(holdout_global.included_rules) + self.assertEqual(holdout_local.included_rules, ['rule_a', 'rule_b']) + + +class HoldoutConfigGetGlobalHoldoutsTest(unittest.TestCase): + """Tests for ProjectConfig.get_global_holdouts().""" + + def setUp(self): + base.BaseTest.setUp(self) + + def tearDown(self): + if hasattr(self, 'opt_obj'): + self.opt_obj.close() + + def test_get_global_holdouts_returns_all_global_holdouts(self): + """All holdouts without includedRules should be returned.""" + self.opt_obj = _build_config([ + _make_holdout('h1', 'global_1'), # field absent → global + _make_holdout('h2', 'global_2', None), # explicitly None → global + _make_holdout('h3', 'local_1', ['rule_x']), # local + ], self) + config = self.opt_obj.config_manager.get_config() + global_holdouts = config.get_global_holdouts() + global_ids = {h.id for h in global_holdouts} + self.assertIn('h1', global_ids) + self.assertIn('h2', global_ids) + self.assertNotIn('h3', global_ids) + + def test_get_global_holdouts_returns_empty_when_no_holdouts(self): + """No holdouts in datafile → empty list.""" + self.opt_obj = _build_config([], self) + config = self.opt_obj.config_manager.get_config() + self.assertEqual(config.get_global_holdouts(), []) + + def test_get_global_holdouts_excludes_non_running_holdouts(self): + """Draft holdouts are not activated and should not appear.""" + self.opt_obj = _build_config([ + _make_holdout('h_running', 'running_global', status='Running'), + _make_holdout('h_draft', 'draft_global', status='Draft'), + ], self) + config = self.opt_obj.config_manager.get_config() + global_holdouts = config.get_global_holdouts() + ids = {h.id for h in global_holdouts} + self.assertIn('h_running', ids) + self.assertNotIn('h_draft', ids) + + def test_get_global_holdouts_returns_empty_when_all_local(self): + """If all holdouts are local, global list is empty.""" + self.opt_obj = _build_config([ + _make_holdout('h1', 'local_a', ['rule_1']), + _make_holdout('h2', 'local_b', ['rule_2']), + ], self) + config = self.opt_obj.config_manager.get_config() + self.assertEqual(config.get_global_holdouts(), []) + + +class HoldoutConfigGetHoldoutsForRuleTest(unittest.TestCase): + """Tests for ProjectConfig.get_holdouts_for_rule().""" + + def setUp(self): + base.BaseTest.setUp(self) + + def tearDown(self): + if hasattr(self, 'opt_obj'): + self.opt_obj.close() + + def test_get_holdouts_for_rule_returns_matching_local_holdouts(self): + """Holdout targeting rule_x should be returned for rule_x.""" + self.opt_obj = _build_config([ + _make_holdout('h1', 'local_h', ['rule_x', 'rule_y']), + ], self) + config = self.opt_obj.config_manager.get_config() + holdouts = config.get_holdouts_for_rule('rule_x') + self.assertEqual(len(holdouts), 1) + self.assertEqual(holdouts[0].id, 'h1') + + def test_get_holdouts_for_rule_returns_empty_for_unknown_rule(self): + """Rule ID not found in any holdout's includedRules returns empty list.""" + self.opt_obj = _build_config([ + _make_holdout('h1', 'local_h', ['rule_a']), + ], self) + config = self.opt_obj.config_manager.get_config() + # Silently skip unknown rule IDs + holdouts = config.get_holdouts_for_rule('nonexistent_rule') + self.assertEqual(holdouts, []) + + def test_get_holdouts_for_rule_returns_empty_when_no_holdouts(self): + """No holdouts defined → always returns empty list.""" + self.opt_obj = _build_config([], self) + config = self.opt_obj.config_manager.get_config() + self.assertEqual(config.get_holdouts_for_rule('any_rule'), []) + + def test_get_holdouts_for_rule_multiple_holdouts_for_same_rule(self): + """Multiple holdouts can target the same rule.""" + self.opt_obj = _build_config([ + _make_holdout('h1', 'local_h1', ['rule_x']), + _make_holdout('h2', 'local_h2', ['rule_x', 'rule_y']), + ], self) + config = self.opt_obj.config_manager.get_config() + holdouts = config.get_holdouts_for_rule('rule_x') + ids = {h.id for h in holdouts} + self.assertIn('h1', ids) + self.assertIn('h2', ids) + + def test_get_holdouts_for_rule_does_not_return_global_holdouts(self): + """Global holdouts should not appear in get_holdouts_for_rule results.""" + self.opt_obj = _build_config([ + _make_holdout('global', 'global_holdout'), # field absent → global + _make_holdout('local', 'local_holdout', ['rule_x']), + ], self) + config = self.opt_obj.config_manager.get_config() + holdouts = config.get_holdouts_for_rule('rule_x') + ids = {h.id for h in holdouts} + self.assertNotIn('global', ids) + self.assertIn('local', ids) + + def test_get_holdouts_for_rule_rule_specificity(self): + """A holdout targeting rule_x must not appear for rule_y.""" + self.opt_obj = _build_config([ + _make_holdout('h1', 'local_for_x', ['rule_x']), + ], self) + config = self.opt_obj.config_manager.get_config() + self.assertEqual(config.get_holdouts_for_rule('rule_y'), []) + + def test_get_holdouts_for_rule_cross_flag_targeting(self): + """One holdout can target rules from multiple different flags.""" + self.opt_obj = _build_config([ + _make_holdout('h1', 'cross_flag_holdout', ['rule_flag_a', 'rule_flag_b']), + ], self) + config = self.opt_obj.config_manager.get_config() + for rule_id in ['rule_flag_a', 'rule_flag_b']: + holdouts = config.get_holdouts_for_rule(rule_id) + self.assertEqual(len(holdouts), 1) + self.assertEqual(holdouts[0].id, 'h1') + + +class HoldoutBackwardCompatibilityTest(unittest.TestCase): + """Tests for backward compatibility with old datafiles (no includedRules field).""" + + def setUp(self): + base.BaseTest.setUp(self) + + def tearDown(self): + if hasattr(self, 'opt_obj'): + self.opt_obj.close() + + def test_old_datafile_without_included_rules_treated_as_global(self): + """Datafiles without includedRules field must default to global holdout.""" + # Simulate old datafile — no 'includedRules' key at all + old_holdout = { + 'id': 'old_h', + 'key': 'legacy_holdout', + 'status': 'Running', + 'audienceIds': [], + 'variations': HOLDOUT_VARIATION, + 'trafficAllocation': FULL_TRAFFIC, + } + self.opt_obj = _build_config([old_holdout], self) + config = self.opt_obj.config_manager.get_config() + + global_holdouts = config.get_global_holdouts() + self.assertEqual(len(global_holdouts), 1) + self.assertEqual(global_holdouts[0].id, 'old_h') + self.assertTrue(global_holdouts[0].is_global) + + def test_old_datafile_holdout_does_not_appear_in_rule_map(self): + """Legacy holdouts with no includedRules field must not pollute rule map.""" + old_holdout = { + 'id': 'old_h', + 'key': 'legacy_holdout', + 'status': 'Running', + 'audienceIds': [], + 'variations': HOLDOUT_VARIATION, + 'trafficAllocation': FULL_TRAFFIC, + } + self.opt_obj = _build_config([old_holdout], self) + config = self.opt_obj.config_manager.get_config() + self.assertEqual(config.get_holdouts_for_rule('any_rule'), []) + + def test_empty_included_rules_is_local_not_global(self): + """[] (empty array) is a local holdout targeting no rules — not the same as None.""" + holdout_empty = _make_holdout('h_empty', 'empty_local', included_rules=[]) + holdout_none = _make_holdout('h_none', 'global_none', included_rules=None) + + self.opt_obj = _build_config([holdout_empty, holdout_none], self) + config = self.opt_obj.config_manager.get_config() + + global_holdouts = config.get_global_holdouts() + global_ids = {h.id for h in global_holdouts} + + # None → global; [] → local (does not appear in global list) + self.assertIn('h_none', global_ids) + self.assertNotIn('h_empty', global_ids) From 9bf1c311119967cb07457fb7d6ce4d62ca36db75 Mon Sep 17 00:00:00 2001 From: Jae Kim <45045038+jaeopt@users.noreply.github.com> Date: Wed, 3 Jun 2026 09:09:27 -0700 Subject: [PATCH 12/12] [AI-FSSDK] [FSSDK-12670] Block ODP identify event for single identifier (#513) * [AI-FSSDK] [FSSDK-12670] Block ODP identify event for single identifier * [FSSDK-12670] Address review feedback: fix log message * [FSSDK-12670] Retrigger CI * [FSSDK-12670] Add comment explaining the < 2 identifiers guard Co-Authored-By: Claude Opus 4.6 * [FSSDK-12670] retrigger CI --------- Co-authored-by: Claude Opus 4.6 Co-authored-by: Farhan Anjum --- optimizely/odp/odp_event_manager.py | 25 ++++++++++- optimizely/odp/odp_manager.py | 4 +- optimizely/optimizely.py | 4 +- optimizely/optimizely_user_context.py | 4 +- tests/test_odp_manager.py | 60 +++++++++++++++++++++++---- tests/test_optimizely.py | 2 +- tests/test_user_context.py | 4 +- 7 files changed, 86 insertions(+), 17 deletions(-) diff --git a/optimizely/odp/odp_event_manager.py b/optimizely/odp/odp_event_manager.py index 3fb961ac..a7bd0906 100644 --- a/optimizely/odp/odp_event_manager.py +++ b/optimizely/odp/odp_event_manager.py @@ -267,9 +267,30 @@ def dispatch(self, event: OdpEvent) -> None: except Full: self.logger.warning(Errors.ODP_EVENT_FAILED.format("Queue is full")) - def identify_user(self, user_id: str) -> None: + def identify_user(self, identifiers: dict[str, str]) -> None: + """Send an identify event to ODP if there are multiple valid identifiers. + + An identify event is only useful when there are two or more identifiers + to link together in the ODP user profile. Sending a single identifier + provides no linking value and wastes a network call. + + Args: + identifiers: A dictionary of identifier key-value pairs + (e.g. {"fs_user_id": "abc", "email": "a@b.com"}). + """ + # Filter out identifiers with None or empty string values. + valid_identifiers = {k: v for k, v in identifiers.items() if v} + + # Identify requires 2+ identifiers to link (e.g., vuid + fs_user_id). + # A single identifier has no cross-reference value and generates unnecessary traffic. + if len(valid_identifiers) < 2: + self.logger.debug( + 'ODP identify event is not dispatched (fewer than 2 valid identifiers).' + ) + return + self.send_event(OdpManagerConfig.EVENT_TYPE, 'identified', - {OdpManagerConfig.KEY_FOR_USER_ID: user_id}, {}) + valid_identifiers, {}) def update_config(self) -> None: """Adds update config signal to event_queue.""" diff --git a/optimizely/odp/odp_manager.py b/optimizely/odp/odp_manager.py index a6e26253..10c160e0 100644 --- a/optimizely/odp/odp_manager.py +++ b/optimizely/odp/odp_manager.py @@ -73,7 +73,7 @@ def fetch_qualified_segments(self, user_id: str, options: list[str]) -> Optional return self.segment_manager.fetch_qualified_segments(user_key, user_value, options) - def identify_user(self, user_id: str) -> None: + def identify_user(self, identifiers: dict[str, str]) -> None: if not self.enabled or not self.event_manager: self.logger.debug('ODP identify event is not dispatched (ODP disabled).') return @@ -81,7 +81,7 @@ def identify_user(self, user_id: str) -> None: self.logger.debug('ODP identify event is not dispatched (ODP not integrated).') return - self.event_manager.identify_user(user_id) + self.event_manager.identify_user(identifiers) def send_event(self, type: str, action: str, identifiers: dict[str, str], data: dict[str, Any]) -> None: """ diff --git a/optimizely/optimizely.py b/optimizely/optimizely.py index 4b014e7f..04162823 100644 --- a/optimizely/optimizely.py +++ b/optimizely/optimizely.py @@ -1541,7 +1541,7 @@ def _update_odp_config_on_datafile_update(self) -> None: config.all_segments ) - def _identify_user(self, user_id: str) -> None: + def _identify_user(self, identifiers: dict[str, str]) -> None: if not self.is_valid: self.logger.error(enums.Errors.INVALID_OPTIMIZELY.format('identify_user')) return @@ -1551,7 +1551,7 @@ def _identify_user(self, user_id: str) -> None: self.logger.error(enums.Errors.INVALID_PROJECT_CONFIG.format('identify_user')) return - self.odp_manager.identify_user(user_id) + self.odp_manager.identify_user(identifiers) def _fetch_qualified_segments(self, user_id: str, options: Optional[list[str]] = None) -> Optional[list[str]]: if not self.is_valid: diff --git a/optimizely/optimizely_user_context.py b/optimizely/optimizely_user_context.py index e88c0f52..379dfb0e 100644 --- a/optimizely/optimizely_user_context.py +++ b/optimizely/optimizely_user_context.py @@ -19,6 +19,7 @@ from typing import TYPE_CHECKING, Any, Callable, Optional, NewType, Dict from optimizely.decision import optimizely_decision +from optimizely.helpers.enums import OdpManagerConfig if TYPE_CHECKING: # prevent circular dependency by skipping import at runtime @@ -73,7 +74,8 @@ def __init__( ] = {} if self.client and identify: - self.client._identify_user(user_id) + identifiers = {OdpManagerConfig.KEY_FOR_USER_ID: user_id} + self.client._identify_user(identifiers) class OptimizelyDecisionContext: """ Using class with attributes here instead of namedtuple because diff --git a/tests/test_odp_manager.py b/tests/test_odp_manager.py index ae0e4a1a..c9483dc0 100644 --- a/tests/test_odp_manager.py +++ b/tests/test_odp_manager.py @@ -48,7 +48,7 @@ def test_configurations_disable_odp(self): mock_logger.reset_mock() # these call should be dropped gracefully with None - manager.identify_user('user1') + manager.identify_user({'fs_user_id': 'user1'}) manager.send_event('t1', 'a1', {}, {}) mock_logger.error.assert_called_once_with('ODP is not enabled.') @@ -126,10 +126,11 @@ def test_identify_user_datafile_not_ready(self): manager = OdpManager(False, OptimizelySegmentsCache, event_manager=event_manager, logger=mock_logger) + identifiers = {'fs_user_id': 'user1', 'email': 'test@example.com'} with mock.patch.object(event_manager, 'identify_user') as mock_identify_user: - manager.identify_user('user1') + manager.identify_user(identifiers) - mock_identify_user.assert_called_once_with('user1') + mock_identify_user.assert_called_once_with(identifiers) mock_logger.error.assert_not_called() def test_identify_user_odp_integrated(self): @@ -139,13 +140,14 @@ def test_identify_user_odp_integrated(self): manager = OdpManager(False, LRUCache(10, 20), event_manager=event_manager, logger=mock_logger) manager.update_odp_config('key1', 'host1', []) + identifiers = {'fs_user_id': 'user1', 'email': 'test@example.com'} with mock.patch.object(event_manager, 'dispatch') as mock_dispatch_event: - manager.identify_user('user1') + manager.identify_user(identifiers) mock_dispatch_event.assert_called_once_with({ 'type': 'fullstack', 'action': 'identified', - 'identifiers': {'fs_user_id': 'user1'}, + 'identifiers': {'fs_user_id': 'user1', 'email': 'test@example.com'}, 'data': { 'idempotence_id': mock.ANY, 'data_source_type': 'sdk', @@ -161,8 +163,9 @@ def test_identify_user_odp_not_integrated(self): manager = OdpManager(False, CustomCache(), event_manager=event_manager, logger=mock_logger) manager.update_odp_config(None, None, []) + identifiers = {'fs_user_id': 'user1', 'email': 'test@example.com'} with mock.patch.object(event_manager, 'dispatch') as mock_dispatch_event: - manager.identify_user('user1') + manager.identify_user(identifiers) mock_dispatch_event.assert_not_called() mock_logger.error.assert_not_called() @@ -175,13 +178,56 @@ def test_identify_user_odp_disabled(self): manager = OdpManager(False, OptimizelySegmentsCache, event_manager=event_manager, logger=mock_logger) manager.enabled = False + identifiers = {'fs_user_id': 'user1', 'email': 'test@example.com'} with mock.patch.object(event_manager, 'identify_user') as mock_identify_user: - manager.identify_user('user1') + manager.identify_user(identifiers) mock_identify_user.assert_not_called() mock_logger.error.assert_not_called() mock_logger.debug.assert_called_with('ODP identify event is not dispatched (ODP disabled).') + def test_identify_user_single_identifier_skipped(self): + """Identify event should not be sent when only one identifier is provided.""" + mock_logger = mock.MagicMock() + event_manager = OdpEventManager(mock_logger, OdpEventApiManager()) + + manager = OdpManager(False, LRUCache(10, 20), event_manager=event_manager, logger=mock_logger) + manager.update_odp_config('key1', 'host1', []) + + with mock.patch.object(event_manager, 'dispatch') as mock_dispatch_event: + manager.identify_user({'fs_user_id': 'user1'}) + + mock_dispatch_event.assert_not_called() + + def test_identify_user_empty_values_not_counted(self): + """Identifiers with empty or None values should not count toward the minimum.""" + mock_logger = mock.MagicMock() + event_manager = OdpEventManager(mock_logger, OdpEventApiManager()) + + manager = OdpManager(False, LRUCache(10, 20), event_manager=event_manager, logger=mock_logger) + manager.update_odp_config('key1', 'host1', []) + + with mock.patch.object(event_manager, 'dispatch') as mock_dispatch_event: + manager.identify_user({'fs_user_id': 'user1', 'email': '', 'vuid': None}) + + mock_dispatch_event.assert_not_called() + + def test_identify_user_multiple_identifiers_sent(self): + """Identify event should be sent when two or more valid identifiers are provided.""" + mock_logger = mock.MagicMock() + event_manager = OdpEventManager(mock_logger, OdpEventApiManager()) + + manager = OdpManager(False, LRUCache(10, 20), event_manager=event_manager, logger=mock_logger) + manager.update_odp_config('key1', 'host1', []) + + identifiers = {'fs_user_id': 'user1', 'vuid': 'vuid123', 'email': 'a@b.com'} + with mock.patch.object(event_manager, 'dispatch') as mock_dispatch_event: + manager.identify_user(identifiers) + + mock_dispatch_event.assert_called_once() + event = mock_dispatch_event.call_args[0][0] + self.assertEqual(event.identifiers, identifiers) + def test_send_event_datafile_not_ready(self): mock_logger = mock.MagicMock() event_manager = OdpEventManager(mock_logger, OdpEventApiManager()) diff --git a/tests/test_optimizely.py b/tests/test_optimizely.py index c2de186f..0340ef72 100644 --- a/tests/test_optimizely.py +++ b/tests/test_optimizely.py @@ -5343,7 +5343,7 @@ def test_send_identify_event__when_called_with_odp_enabled(self): with mock.patch.object(client, '_identify_user') as identify: client.create_user_context('user-id') - identify.assert_called_once_with('user-id') + identify.assert_called_once_with({'fs_user_id': 'user-id'}) mock_logger.error.assert_not_called() client.close() diff --git a/tests/test_user_context.py b/tests/test_user_context.py index 3ae9be0d..3edb072c 100644 --- a/tests/test_user_context.py +++ b/tests/test_user_context.py @@ -2094,7 +2094,7 @@ def test_send_identify_event_when_user_context_created(self): with mock.patch.object(client, '_identify_user') as identify: OptimizelyUserContext(client, mock_logger, 'user-id') - identify.assert_called_once_with('user-id') + identify.assert_called_once_with({'fs_user_id': 'user-id'}) mock_logger.error.assert_not_called() client.close() @@ -2104,7 +2104,7 @@ def test_identify_is_skipped_with_decisions(self): with mock.patch.object(client, '_identify_user') as identify: user_context = OptimizelyUserContext(client, mock_logger, 'user-id') - identify.assert_called_once_with('user-id') + identify.assert_called_once_with({'fs_user_id': 'user-id'}) mock_logger.error.assert_not_called() with mock.patch.object(client, '_identify_user') as identify: