From 620014a0c41754db486947b04fb71e9c4dcb993e Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Fri, 26 Jun 2026 19:08:43 -0700 Subject: [PATCH 1/2] [FSSDK-12839] Normalize impression event campaign_id, variation_id, entity_id --- optimizely/event/event_factory.py | 15 +++- optimizely/event/payload.py | 10 ++- optimizely/event_builder.py | 19 +++-- optimizely/helpers/validator.py | 21 +++++ tests/test_event_builder.py | 123 +++++++++++++++++++++++++++++- tests/test_event_factory.py | 91 +++++++++++++++++++++- 6 files changed, 266 insertions(+), 13 deletions(-) diff --git a/optimizely/event/event_factory.py b/optimizely/event/event_factory.py index f66c1d59..5ed1a5c3 100644 --- a/optimizely/event/event_factory.py +++ b/optimizely/event/event_factory.py @@ -1,4 +1,4 @@ -# Copyright 2019, 2022, Optimizely +# Copyright 2019, 2022, 2026, Optimizely # 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 @@ -134,12 +134,21 @@ def _create_visitor(cls, event: Optional[user_event.UserEvent], logger: Logger) if isinstance(event.experiment, entities.Experiment): experiment_layerId = event.experiment.layerId + campaign_id: str = ( + experiment_layerId + if validator.is_numeric_string_id(experiment_layerId) + else experiment_id + ) + normalized_variation_id: Optional[str] = ( + variation_id if validator.is_numeric_string_id(variation_id) else None + ) + metadata = payload.Metadata(event.flag_key, event.rule_key, event.rule_type, variation_key, event.enabled, event.cmab_uuid) - decision = payload.Decision(experiment_layerId, experiment_id, variation_id, metadata) + decision = payload.Decision(campaign_id, experiment_id, normalized_variation_id, metadata) snapshot_event = payload.SnapshotEvent( - experiment_layerId, event.uuid, cls.ACTIVATE_EVENT_KEY, event.timestamp, + campaign_id, event.uuid, cls.ACTIVATE_EVENT_KEY, event.timestamp, ) snapshot = payload.Snapshot([snapshot_event], [decision]) diff --git a/optimizely/event/payload.py b/optimizely/event/payload.py index e352dd10..a87f97e2 100644 --- a/optimizely/event/payload.py +++ b/optimizely/event/payload.py @@ -1,4 +1,4 @@ -# Copyright 2019, 2022, Optimizely +# Copyright 2019, 2022, 2026, Optimizely # 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 @@ -71,7 +71,13 @@ def get_event_params(self) -> dict[str, Any]: class Decision: """ Class respresenting Decision. """ - def __init__(self, campaign_id: str, experiment_id: str, variation_id: str, metadata: Metadata): + def __init__( + self, + campaign_id: str, + experiment_id: str, + variation_id: Optional[str], + metadata: Metadata, + ): self.campaign_id = campaign_id self.experiment_id = experiment_id self.variation_id = variation_id diff --git a/optimizely/event_builder.py b/optimizely/event_builder.py index e9c9fd44..b4d65f5b 100644 --- a/optimizely/event_builder.py +++ b/optimizely/event_builder.py @@ -1,4 +1,4 @@ -# Copyright 2016-2019, 2022, Optimizely +# Copyright 2016-2019, 2022, 2026, Optimizely # 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 @@ -178,7 +178,7 @@ def _get_common_params( def _get_required_params_for_impression( self, experiment: Experiment, variation_id: str - ) -> dict[str, list[dict[str, str | int]]]: + ) -> dict[str, list[dict[str, Any]]]: """ Get parameters that are required for the impression event to register. Args: @@ -188,19 +188,26 @@ def _get_required_params_for_impression( Returns: Dict consisting of decisions and events info for impression event. """ - snapshot: dict[str, list[dict[str, str | int]]] = {} + campaign_id: str = ( + experiment.layerId if validator.is_numeric_string_id(experiment.layerId) else experiment.id + ) + normalized_variation_id: Optional[str] = ( + variation_id if validator.is_numeric_string_id(variation_id) else None + ) + + snapshot: dict[str, list[dict[str, Any]]] = {} snapshot[self.EventParams.DECISIONS] = [ { self.EventParams.EXPERIMENT_ID: experiment.id, - self.EventParams.VARIATION_ID: variation_id, - self.EventParams.CAMPAIGN_ID: experiment.layerId, + self.EventParams.VARIATION_ID: normalized_variation_id, + self.EventParams.CAMPAIGN_ID: campaign_id, } ] snapshot[self.EventParams.EVENTS] = [ { - self.EventParams.EVENT_ID: experiment.layerId, + self.EventParams.EVENT_ID: campaign_id, self.EventParams.TIME: self._get_time(), self.EventParams.KEY: 'campaign_activated', self.EventParams.UUID: str(uuid.uuid4()), diff --git a/optimizely/helpers/validator.py b/optimizely/helpers/validator.py index 59328331..6c1245b3 100644 --- a/optimizely/helpers/validator.py +++ b/optimizely/helpers/validator.py @@ -228,6 +228,27 @@ def is_non_empty_string(input_id_key: str) -> bool: return False +def is_numeric_string_id(value: Any) -> bool: + """ Determine if value is a non-empty string consisting entirely of decimal digits. + + Booleans are rejected even though they are technically instances of int in Python; the + field contract is "string of decimal digits", and a bool is neither a string nor a digit. + + Args: + value: Variable which needs to be validated. + + Returns: + True if value is a non-empty string of decimal digits [0-9]. False otherwise + (including for None, empty string, whitespace, non-string types, or strings + containing any non-digit character such as a sign, decimal point, or letter). + """ + if not isinstance(value, str) or isinstance(value, bool): + return False + if not value: + return False + return value.isdigit() and value.isascii() + + def is_attribute_valid(attribute_key: str, attribute_value: Any) -> bool: """ Determine if given attribute is valid. diff --git a/tests/test_event_builder.py b/tests/test_event_builder.py index 237e17a4..f74e7061 100644 --- a/tests/test_event_builder.py +++ b/tests/test_event_builder.py @@ -1,4 +1,4 @@ -# Copyright 2016-2019, Optimizely +# Copyright 2016-2019, 2026, Optimizely # 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 @@ -11,6 +11,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import copy from unittest import mock import unittest from operator import itemgetter @@ -1015,3 +1016,123 @@ def test_create_conversion_event__when_event_is_used_in_multiple_experiments(sel event_builder.EventBuilder.HTTP_VERB, event_builder.EventBuilder.HTTP_HEADERS, ) + + +class ImpressionEventIdNormalizationTest(base.BaseTest): + """Impression-event normalization rules for campaign_id, variation_id, and entity_id.""" + + def setUp(self, *args, **kwargs): + base.BaseTest.setUp(self, 'config_dict_with_multiple_experiments') + self.event_builder = self.optimizely.event_builder + self.experiment = self.project_config.get_experiment_from_key('test_experiment') + + def _build_impression(self, experiment, variation_id): + return self.event_builder._get_required_params_for_impression(experiment, variation_id) + + def _with_layer_id(self, layer_id): + experiment = copy.deepcopy(self.experiment) + experiment.layerId = layer_id + return experiment + + def _decision(self, snapshot): + return snapshot[event_builder.EventBuilder.EventParams.DECISIONS][0] + + def _event(self, snapshot): + return snapshot[event_builder.EventBuilder.EventParams.EVENTS][0] + + # campaign_id normalization (US1) ---------------------------------------------- + + def test_campaign_id_valid_numeric_layer_id_passes_through(self): + experiment = self._with_layer_id('111182') + snapshot = self._build_impression(experiment, '111129') + self.assertEqual(self._decision(snapshot)['campaign_id'], '111182') + + def test_campaign_id_empty_string_falls_back_to_experiment_id(self): + experiment = self._with_layer_id('') + snapshot = self._build_impression(experiment, '111129') + self.assertEqual(self._decision(snapshot)['campaign_id'], experiment.id) + + def test_campaign_id_none_falls_back_to_experiment_id(self): + experiment = self._with_layer_id(None) + snapshot = self._build_impression(experiment, '111129') + self.assertEqual(self._decision(snapshot)['campaign_id'], experiment.id) + + def test_campaign_id_non_numeric_string_falls_back_to_experiment_id(self): + experiment = self._with_layer_id('abc') + snapshot = self._build_impression(experiment, '111129') + self.assertEqual(self._decision(snapshot)['campaign_id'], experiment.id) + + def test_campaign_id_whitespace_falls_back_to_experiment_id(self): + experiment = self._with_layer_id(' ') + snapshot = self._build_impression(experiment, '111129') + self.assertEqual(self._decision(snapshot)['campaign_id'], experiment.id) + + def test_campaign_id_integer_value_falls_back_to_experiment_id(self): + experiment = self._with_layer_id(111182) + snapshot = self._build_impression(experiment, '111129') + self.assertEqual(self._decision(snapshot)['campaign_id'], experiment.id) + + # variation_id normalization (US2) --------------------------------------------- + + def test_variation_id_valid_numeric_passes_through(self): + snapshot = self._build_impression(self.experiment, '111129') + self.assertEqual(self._decision(snapshot)['variation_id'], '111129') + + def test_variation_id_empty_string_becomes_none(self): + snapshot = self._build_impression(self.experiment, '') + self.assertIsNone(self._decision(snapshot)['variation_id']) + + def test_variation_id_non_numeric_string_becomes_none(self): + snapshot = self._build_impression(self.experiment, 'variation_a') + self.assertIsNone(self._decision(snapshot)['variation_id']) + + def test_variation_id_none_stays_none(self): + snapshot = self._build_impression(self.experiment, None) + self.assertIsNone(self._decision(snapshot)['variation_id']) + + # entity_id normalization (US3) and US3 acceptance #5: byte-equality with campaign_id + + def test_entity_id_valid_layer_id_passes_through(self): + experiment = self._with_layer_id('111182') + snapshot = self._build_impression(experiment, '111129') + self.assertEqual(self._event(snapshot)['entity_id'], '111182') + + def test_entity_id_empty_falls_back_to_experiment_id(self): + experiment = self._with_layer_id('') + snapshot = self._build_impression(experiment, '111129') + self.assertEqual(self._event(snapshot)['entity_id'], experiment.id) + + def test_entity_id_non_numeric_falls_back_to_experiment_id(self): + experiment = self._with_layer_id('abc') + snapshot = self._build_impression(experiment, '111129') + self.assertEqual(self._event(snapshot)['entity_id'], experiment.id) + + def test_entity_id_equals_campaign_id_when_layer_invalid(self): + experiment = self._with_layer_id('') + snapshot = self._build_impression(experiment, '111129') + self.assertEqual( + self._event(snapshot)['entity_id'], + self._decision(snapshot)['campaign_id'], + ) + + def test_entity_id_equals_campaign_id_when_layer_valid(self): + experiment = self._with_layer_id('111182') + snapshot = self._build_impression(experiment, '111129') + self.assertEqual( + self._event(snapshot)['entity_id'], + self._decision(snapshot)['campaign_id'], + ) + + # Negative regression: conversion events are out of scope (FR-010). + + def test_conversion_event_entity_id_uses_event_id_unchanged(self): + with mock.patch('time.time', return_value=42.123), mock.patch( + 'uuid.uuid4', return_value='a68cf1ad-0393-4e18-af87-efe8f01a7c9c' + ): + event_obj = self.event_builder.create_conversion_event( + self.project_config, 'test_event', 'test_user', None, None, + ) + + snapshot = event_obj.params['visitors'][0]['snapshots'][0] + event = snapshot['events'][0] + self.assertEqual(event['entity_id'], self.project_config.get_event('test_event').id) diff --git a/tests/test_event_factory.py b/tests/test_event_factory.py index 6d70c713..41be2d3b 100644 --- a/tests/test_event_factory.py +++ b/tests/test_event_factory.py @@ -1,4 +1,4 @@ -# Copyright 2019, Optimizely +# Copyright 2019, 2026, Optimizely # 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 @@ -11,6 +11,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import copy from unittest import mock import time import unittest @@ -1237,3 +1238,91 @@ def test_create_impression_event_without_cmab_uuid(self): EventFactory.HTTP_VERB, EventFactory.HTTP_HEADERS, ) + + +class ImpressionEventIdNormalizationFactoryTest(base.BaseTest): + """Impression-event normalization rules in EventFactory._create_visitor.""" + + def setUp(self, *args, **kwargs): + base.BaseTest.setUp(self, 'config_dict_with_multiple_experiments') + self.logger = logger.NoOpLogger() + self.experiment = self.project_config.get_experiment_from_key('test_experiment') + + def _build_visitor(self, layer_id, variation_id): + from optimizely.event import user_event + + experiment = copy.deepcopy(self.experiment) + experiment.layerId = layer_id + + variation_payload = {'id': variation_id, 'key': 'whatever'} if variation_id is not None else None + + ctx = user_event.EventContext( + self.project_config.get_account_id(), + self.project_config.get_project_id(), + self.project_config.get_revision(), + self.project_config.get_anonymize_ip_value(), + 'US', + ) + impression = user_event.ImpressionEvent( + event_context=ctx, + user_id='test_user', + experiment=experiment, + visitor_attributes=[], + variation=variation_payload, + flag_key='flag_a', + rule_key=experiment.key, + rule_type='experiment', + enabled=True, + ) + visitor = EventFactory._create_visitor(impression, self.logger) + self.assertIsNotNone(visitor) + snapshot = visitor.snapshots[0] + decision = snapshot.decisions[0] + snapshot_event = snapshot.events[0] + return decision, snapshot_event, experiment + + def test_campaign_id_valid_layer_id_passes_through(self): + decision, _event, _exp = self._build_visitor('111182', '111129') + self.assertEqual(decision.campaign_id, '111182') + + def test_campaign_id_empty_layer_id_falls_back_to_experiment_id(self): + decision, _event, experiment = self._build_visitor('', '111129') + self.assertEqual(decision.campaign_id, experiment.id) + + def test_campaign_id_non_numeric_layer_id_falls_back_to_experiment_id(self): + decision, _event, experiment = self._build_visitor('abc', '111129') + self.assertEqual(decision.campaign_id, experiment.id) + + def test_variation_id_empty_becomes_none(self): + decision, _event, _exp = self._build_visitor('111182', '') + self.assertIsNone(decision.variation_id) + + def test_variation_id_non_numeric_becomes_none(self): + decision, _event, _exp = self._build_visitor('111182', 'variation_a') + self.assertIsNone(decision.variation_id) + + def test_variation_id_valid_passes_through(self): + decision, _event, _exp = self._build_visitor('111182', '111129') + self.assertEqual(decision.variation_id, '111129') + + def test_entity_id_matches_campaign_id_when_layer_invalid(self): + decision, snapshot_event, _exp = self._build_visitor('', '111129') + self.assertEqual(snapshot_event.entity_id, decision.campaign_id) + + def test_entity_id_matches_campaign_id_when_layer_valid(self): + decision, snapshot_event, _exp = self._build_visitor('111182', '111129') + self.assertEqual(snapshot_event.entity_id, decision.campaign_id) + + def test_variation_id_serializes_to_json_null(self): + decision, _event, _exp = self._build_visitor('111182', 'not_numeric') + # The EventBatch serializer renders the variation_id field as JSON null, + # which is the wire contract this fix exists to honor. + from optimizely.event.payload import EventBatch, Snapshot, Visitor + batch = EventBatch('acc', 'proj', 'rev', 'python-sdk', '1.0', False, True) + snapshot = Snapshot([], [decision]) + visitor = Visitor([snapshot], [], 'u') + batch.visitors = [visitor] + params = batch.get_event_params() + rendered = params['visitors'][0]['snapshots'][0]['decisions'][0] + self.assertIn('variation_id', rendered) + self.assertIsNone(rendered['variation_id']) From f59444faa3f36e48bc88449891a1901aa7373b02 Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Fri, 26 Jun 2026 20:03:58 -0700 Subject: [PATCH 2/2] [FSSDK-12839] Relax campaign_id/entity_id to non-empty string per spec --- optimizely/event/event_factory.py | 2 +- optimizely/event_builder.py | 2 +- tests/test_event_builder.py | 26 ++++++++++++++++++-------- tests/test_event_factory.py | 10 +++++++--- 4 files changed, 27 insertions(+), 13 deletions(-) diff --git a/optimizely/event/event_factory.py b/optimizely/event/event_factory.py index 5ed1a5c3..ff21d198 100644 --- a/optimizely/event/event_factory.py +++ b/optimizely/event/event_factory.py @@ -136,7 +136,7 @@ def _create_visitor(cls, event: Optional[user_event.UserEvent], logger: Logger) campaign_id: str = ( experiment_layerId - if validator.is_numeric_string_id(experiment_layerId) + if validator.is_non_empty_string(experiment_layerId) else experiment_id ) normalized_variation_id: Optional[str] = ( diff --git a/optimizely/event_builder.py b/optimizely/event_builder.py index b4d65f5b..e84d1733 100644 --- a/optimizely/event_builder.py +++ b/optimizely/event_builder.py @@ -189,7 +189,7 @@ def _get_required_params_for_impression( Dict consisting of decisions and events info for impression event. """ campaign_id: str = ( - experiment.layerId if validator.is_numeric_string_id(experiment.layerId) else experiment.id + experiment.layerId if validator.is_non_empty_string(experiment.layerId) else experiment.id ) normalized_variation_id: Optional[str] = ( variation_id if validator.is_numeric_string_id(variation_id) else None diff --git a/tests/test_event_builder.py b/tests/test_event_builder.py index f74e7061..fb0b7efd 100644 --- a/tests/test_event_builder.py +++ b/tests/test_event_builder.py @@ -1057,17 +1057,24 @@ def test_campaign_id_none_falls_back_to_experiment_id(self): snapshot = self._build_impression(experiment, '111129') self.assertEqual(self._decision(snapshot)['campaign_id'], experiment.id) - def test_campaign_id_non_numeric_string_falls_back_to_experiment_id(self): - experiment = self._with_layer_id('abc') + def test_campaign_id_opaque_string_passes_through(self): + # Per FR-001: any non-empty string (numeric or opaque like "default-12345" + # or "layer_abc") is valid and passes through unchanged. + experiment = self._with_layer_id('default-12345') snapshot = self._build_impression(experiment, '111129') - self.assertEqual(self._decision(snapshot)['campaign_id'], experiment.id) + self.assertEqual(self._decision(snapshot)['campaign_id'], 'default-12345') - def test_campaign_id_whitespace_falls_back_to_experiment_id(self): + def test_campaign_id_whitespace_string_passes_through(self): + # Per FR-001 / spec Assumptions: a non-empty string of any character + # content is valid; whitespace-only strings are non-empty and accepted. experiment = self._with_layer_id(' ') snapshot = self._build_impression(experiment, '111129') - self.assertEqual(self._decision(snapshot)['campaign_id'], experiment.id) + self.assertEqual(self._decision(snapshot)['campaign_id'], ' ') def test_campaign_id_integer_value_falls_back_to_experiment_id(self): + # Non-string types are out of scope per spec Assumptions; the + # is_non_empty_string predicate returns False for them, so the + # fallback to experiment_id fires. experiment = self._with_layer_id(111182) snapshot = self._build_impression(experiment, '111129') self.assertEqual(self._decision(snapshot)['campaign_id'], experiment.id) @@ -1102,10 +1109,13 @@ def test_entity_id_empty_falls_back_to_experiment_id(self): snapshot = self._build_impression(experiment, '111129') self.assertEqual(self._event(snapshot)['entity_id'], experiment.id) - def test_entity_id_non_numeric_falls_back_to_experiment_id(self): - experiment = self._with_layer_id('abc') + def test_entity_id_opaque_layer_id_passes_through(self): + # Per FR-009: entity_id accepts any non-empty string (numeric or opaque + # like "layer_abc") and passes through unchanged. The fallback to + # experiment_id fires only when the value is empty, null, or missing. + experiment = self._with_layer_id('layer_abc') snapshot = self._build_impression(experiment, '111129') - self.assertEqual(self._event(snapshot)['entity_id'], experiment.id) + self.assertEqual(self._event(snapshot)['entity_id'], 'layer_abc') def test_entity_id_equals_campaign_id_when_layer_invalid(self): experiment = self._with_layer_id('') diff --git a/tests/test_event_factory.py b/tests/test_event_factory.py index 41be2d3b..d36f0e89 100644 --- a/tests/test_event_factory.py +++ b/tests/test_event_factory.py @@ -1289,9 +1289,13 @@ def test_campaign_id_empty_layer_id_falls_back_to_experiment_id(self): decision, _event, experiment = self._build_visitor('', '111129') self.assertEqual(decision.campaign_id, experiment.id) - def test_campaign_id_non_numeric_layer_id_falls_back_to_experiment_id(self): - decision, _event, experiment = self._build_visitor('abc', '111129') - self.assertEqual(decision.campaign_id, experiment.id) + def test_campaign_id_opaque_layer_id_passes_through(self): + # Per FR-001: any non-empty string (numeric or opaque like + # "default-12345" / "layer_abc") is valid and passes through unchanged. + decision, snapshot_event, _exp = self._build_visitor('layer_abc', '111129') + self.assertEqual(decision.campaign_id, 'layer_abc') + # FR-009: entity_id MUST stay byte-equal to campaign_id even for opaque IDs. + self.assertEqual(snapshot_event.entity_id, 'layer_abc') def test_variation_id_empty_becomes_none(self): decision, _event, _exp = self._build_visitor('111182', '')