From b11dd6fe54f570a02ababfe71b201fc392fab0e5 Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Thu, 25 Jun 2026 08:56:03 -0700 Subject: [PATCH 1/7] [FSSDK-12813] Normalize decision event campaign_id, variation_id, and entity_id --- optimizely/event/event_factory.py | 18 ++- optimizely/event/event_id_normalizer.py | 84 ++++++++++ optimizely/event/payload.py | 11 +- optimizely/event_builder.py | 20 ++- tests/test_event_factory.py | 200 ++++++++++++++++++++++++ tests/test_event_id_normalizer.py | 192 +++++++++++++++++++++++ 6 files changed, 517 insertions(+), 8 deletions(-) create mode 100644 optimizely/event/event_id_normalizer.py create mode 100644 tests/test_event_id_normalizer.py diff --git a/optimizely/event/event_factory.py b/optimizely/event/event_factory.py index f66c1d59..17f62b42 100644 --- a/optimizely/event/event_factory.py +++ b/optimizely/event/event_factory.py @@ -18,6 +18,7 @@ from optimizely.helpers import enums from optimizely.helpers import event_tag_utils from optimizely.helpers import validator +from . import event_id_normalizer from . import log_event from . import payload from . import user_event @@ -134,12 +135,25 @@ def _create_visitor(cls, event: Optional[user_event.UserEvent], logger: Logger) if isinstance(event.experiment, entities.Experiment): experiment_layerId = event.experiment.layerId + # FSSDK-12813: Normalize decision-event IDs uniformly across all + # decision types (experiment, feature test, rollout, holdout). + # campaign_id falls back to experiment_id when invalid. + # variation_id becomes None when invalid. + # entity_id on the impression event mirrors campaign_id (FR-009) + # so the two fields are byte-equivalent for the same event. + normalized_campaign_id = event_id_normalizer.normalize_campaign_id( + experiment_layerId, experiment_id + ) + normalized_variation_id = event_id_normalizer.normalize_variation_id(variation_id) + 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( + normalized_campaign_id, experiment_id, normalized_variation_id, metadata + ) snapshot_event = payload.SnapshotEvent( - experiment_layerId, event.uuid, cls.ACTIVATE_EVENT_KEY, event.timestamp, + normalized_campaign_id, event.uuid, cls.ACTIVATE_EVENT_KEY, event.timestamp, ) snapshot = payload.Snapshot([snapshot_event], [decision]) diff --git a/optimizely/event/event_id_normalizer.py b/optimizely/event/event_id_normalizer.py new file mode 100644 index 00000000..575f90ce --- /dev/null +++ b/optimizely/event/event_id_normalizer.py @@ -0,0 +1,84 @@ +# Copyright 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 +# +# 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. + +"""Normalization helpers for decision-event ID fields. + +This module provides byte-equivalent, cross-SDK normalization for the +``campaign_id``, ``variation_id``, and impression ``entity_id`` fields that +appear in dispatched decision events. See FSSDK-12813. + +Rules: + * A "numeric ID string" is a non-empty :class:`str` consisting entirely of + decimal digits ``0-9``. Leading zeros are allowed. Whitespace, negatives, + decimals, and exponents are INVALID. + * ``campaign_id`` -> when invalid, falls back to ``experiment_id`` (which is + itself passed through :func:`normalize_string_id`). + * ``variation_id`` -> when invalid, becomes ``None``. + * ``entity_id`` on impression events shares the campaign_id normalization + and is therefore byte-equivalent to the normalized campaign_id for the + same impression (FR-009). + +The normalization path MUST NOT log, warn, or raise. It must never drop or +defer event dispatch. +""" + +from __future__ import annotations + +from typing import Any, Optional + + +def is_numeric_id_string(value: Any) -> bool: + """Return ``True`` if ``value`` is a non-empty decimal-digit string. + + Whitespace, signs, decimal points, exponents, and non-string types all + return ``False``. Leading zeros are accepted. + """ + if not isinstance(value, str): + return False + if value == '': + return False + # ``str.isdigit`` rejects everything except [0-9] characters and the + # empty string. We've already excluded the empty case above. Note that + # ``isdigit`` also accepts some non-ASCII digit code points; ``isascii`` + # combined with ``isdigit`` restricts us to plain decimal digits. + return value.isascii() and value.isdigit() + + +def normalize_string_id(value: Any) -> Optional[str]: + """Return ``value`` if it's a numeric ID string, otherwise ``None``.""" + return value if is_numeric_id_string(value) else None + + +def normalize_campaign_id(campaign_id: Any, experiment_id: Any) -> str: + """Normalize a decision-event ``campaign_id`` (FR-001/FR-002, FR-009). + + If ``campaign_id`` is a valid numeric ID string it is returned unchanged. + Otherwise the function falls back to ``experiment_id`` (after applying + the same validation). If neither is a numeric ID string, an empty string + is returned so the event still dispatches (FR-006). + """ + if is_numeric_id_string(campaign_id): + return campaign_id # type: ignore[no-any-return] + if is_numeric_id_string(experiment_id): + return experiment_id # type: ignore[no-any-return] + return '' + + +def normalize_variation_id(variation_id: Any) -> Optional[str]: + """Normalize a decision-event ``variation_id`` (FR-003/FR-004). + + Returns the original value if it is a valid numeric ID string. Otherwise + returns ``None`` so the event payload omits/clears the field for the + downstream consumer. + """ + return variation_id if is_numeric_id_string(variation_id) else None diff --git a/optimizely/event/payload.py b/optimizely/event/payload.py index e352dd10..fa90b958 100644 --- a/optimizely/event/payload.py +++ b/optimizely/event/payload.py @@ -71,9 +71,18 @@ 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 + # FSSDK-12813: variation_id may be None when input is invalid / + # non-numeric (FR-003/FR-004). All other decision fields remain + # strings. self.variation_id = variation_id self.metadata = metadata diff --git a/optimizely/event_builder.py b/optimizely/event_builder.py index e9c9fd44..925c8e0b 100644 --- a/optimizely/event_builder.py +++ b/optimizely/event_builder.py @@ -18,6 +18,7 @@ from sys import version_info from . import version +from .event import event_id_normalizer from .helpers import enums from .helpers import event_tag_utils from .helpers import validator @@ -178,7 +179,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 +189,28 @@ 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]]] = {} + snapshot: dict[str, list[dict[str, Any]]] = {} + + # FSSDK-12813: Normalize decision-event IDs uniformly across all + # decision types. campaign_id falls back to experiment_id when + # invalid; variation_id becomes None when invalid; entity_id mirrors + # the normalized campaign_id (FR-009). + normalized_campaign_id = event_id_normalizer.normalize_campaign_id( + experiment.layerId, experiment.id + ) + normalized_variation_id = event_id_normalizer.normalize_variation_id(variation_id) 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: normalized_campaign_id, } ] snapshot[self.EventParams.EVENTS] = [ { - self.EventParams.EVENT_ID: experiment.layerId, + self.EventParams.EVENT_ID: normalized_campaign_id, self.EventParams.TIME: self._get_time(), self.EventParams.KEY: 'campaign_activated', self.EventParams.UUID: str(uuid.uuid4()), diff --git a/tests/test_event_factory.py b/tests/test_event_factory.py index 6d70c713..2376c316 100644 --- a/tests/test_event_factory.py +++ b/tests/test_event_factory.py @@ -1237,3 +1237,203 @@ def test_create_impression_event_without_cmab_uuid(self): EventFactory.HTTP_VERB, EventFactory.HTTP_HEADERS, ) + + +class EventFactoryIdNormalizationIntegrationTest(base.BaseTest): + """FSSDK-12813: end-to-end decision-event ID normalization. + + These tests build real ``ImpressionEvent`` instances using crafted + Experiment/Variation objects, then call ``EventFactory.create_log_event`` + and inspect the dispatched payload. They exercise FR-001..FR-009. + """ + + def setUp(self, *args, **kwargs): + base.BaseTest.setUp(self, 'config_dict_with_multiple_experiments') + self.logger = logger.NoOpLogger() + + def _build_impression( + self, + experiment_id, + layer_id, + variation_id, + rule_type='experiment', + ): + """Build an ImpressionEvent with the provided raw ID values. + + ``experiment_id``/``layer_id``/``variation_id`` are inserted verbatim + so tests can exercise empty/non-string/non-numeric inputs. + """ + from optimizely.entities import Experiment, Variation + from optimizely.event.user_event import EventContext, ImpressionEvent + + experiment = Experiment( + id=experiment_id, + key='exp_key', + status='Running', + audienceIds=[], + variations=[], + forcedVariations={}, + trafficAllocation=[], + layerId=layer_id, + ) + variation = Variation( + id=variation_id, + key='variation_key', + featureEnabled=True, + ) if isinstance(variation_id, str) else None + + event_context = EventContext( + account_id='12001', + project_id='111001', + revision='42', + anonymize_ip=False, + region='US', + ) + return ImpressionEvent( + event_context=event_context, + user_id='test_user', + experiment=experiment, + visitor_attributes=[], + variation=variation, + flag_key='flag_key', + rule_key='rule_key', + rule_type=rule_type, + enabled=True, + ) + + def _dispatched_decision(self, impression_event): + """Return (decision_dict, event_dict) for an impression event.""" + log_event = EventFactory.create_log_event(impression_event, self.logger) + snapshot = log_event.params['visitors'][0]['snapshots'][0] + return snapshot['decisions'][0], snapshot['events'][0] + + # ------------------------------------------------------------------ FR-001 + def test_valid_campaign_id_is_passed_through(self): + impression = self._build_impression('111127', '111182', '111129') + decision, event = self._dispatched_decision(impression) + self.assertEqual('111182', decision['campaign_id']) + # FR-009: entity_id mirrors campaign_id byte-for-byte. + self.assertEqual(decision['campaign_id'], event['entity_id']) + + # ------------------------------------------------------------------ FR-002 + def test_empty_campaign_id_falls_back_to_experiment_id(self): + impression = self._build_impression('111127', '', '111129') + decision, event = self._dispatched_decision(impression) + self.assertEqual('111127', decision['campaign_id']) + self.assertEqual('111127', event['entity_id']) + + def test_non_numeric_campaign_id_falls_back_to_experiment_id(self): + impression = self._build_impression('111127', 'campaign_a', '111129') + decision, event = self._dispatched_decision(impression) + self.assertEqual('111127', decision['campaign_id']) + self.assertEqual('111127', event['entity_id']) + + def test_whitespace_campaign_id_falls_back_to_experiment_id(self): + impression = self._build_impression('111127', ' ', '111129') + decision, event = self._dispatched_decision(impression) + self.assertEqual('111127', decision['campaign_id']) + self.assertEqual('111127', event['entity_id']) + + # ------------------------------------------------------------------ FR-003 + def test_valid_variation_id_is_passed_through(self): + impression = self._build_impression('111127', '111182', '111129') + decision, _ = self._dispatched_decision(impression) + self.assertEqual('111129', decision['variation_id']) + + # ------------------------------------------------------------------ FR-004 + def test_empty_variation_id_becomes_none(self): + impression = self._build_impression('111127', '111182', '') + decision, _ = self._dispatched_decision(impression) + self.assertIsNone(decision['variation_id']) + + def test_non_numeric_variation_id_becomes_none(self): + impression = self._build_impression('111127', '111182', 'variation_a') + decision, _ = self._dispatched_decision(impression) + self.assertIsNone(decision['variation_id']) + + def test_whitespace_variation_id_becomes_none(self): + impression = self._build_impression('111127', '111182', ' ') + decision, _ = self._dispatched_decision(impression) + self.assertIsNone(decision['variation_id']) + + # ------------------------------------------------------------------ FR-005 + def test_normalization_applies_to_rollout_decisions(self): + impression = self._build_impression( + '111127', 'bad_layer', 'bad_var', rule_type='rollout' + ) + decision, event = self._dispatched_decision(impression) + self.assertEqual('111127', decision['campaign_id']) + self.assertIsNone(decision['variation_id']) + self.assertEqual('111127', event['entity_id']) + + def test_normalization_applies_to_feature_test_decisions(self): + impression = self._build_impression( + '111127', '', '', rule_type='feature-test' + ) + decision, event = self._dispatched_decision(impression) + self.assertEqual('111127', decision['campaign_id']) + self.assertIsNone(decision['variation_id']) + self.assertEqual('111127', event['entity_id']) + + def test_normalization_applies_to_holdout_decisions(self): + impression = self._build_impression( + '111127', '', '', rule_type='holdout' + ) + decision, event = self._dispatched_decision(impression) + self.assertEqual('111127', decision['campaign_id']) + self.assertIsNone(decision['variation_id']) + self.assertEqual('111127', event['entity_id']) + + # ------------------------------------------------------------------ FR-006 + def test_event_still_dispatches_when_all_ids_invalid(self): + """FR-006: never drop / fail dispatch.""" + impression = self._build_impression('', '', '') + log_event = EventFactory.create_log_event(impression, self.logger) + self.assertIsNotNone(log_event) + decision, event = self._dispatched_decision(impression) + # campaign_id and entity_id end up as '' but the event still + # dispatches and the two fields remain byte-equivalent. + self.assertEqual('', decision['campaign_id']) + self.assertEqual('', event['entity_id']) + self.assertIsNone(decision['variation_id']) + + # ------------------------------------------------------------------ FR-009 + def test_entity_id_equals_campaign_id_byte_for_byte(self): + """FR-009: ``events[].entity_id`` must equal ``decisions[].campaign_id``.""" + for layer_id, exp_id, expected in [ + ('111182', '111127', '111182'), # campaign_id wins + ('', '111127', '111127'), # falls back to experiment_id + ('bad', '111127', '111127'), # fallback on invalid + ('007', '111127', '007'), # leading zeros preserved + ]: + with self.subTest(layer_id=layer_id, exp_id=exp_id): + impression = self._build_impression(exp_id, layer_id, '111129') + decision, event = self._dispatched_decision(impression) + self.assertEqual(expected, decision['campaign_id']) + self.assertEqual(decision['campaign_id'], event['entity_id']) + + # ----------------------------------------------------------------- FR-010 + def test_conversion_event_entity_id_unchanged(self): + """FR-010: conversion events derive entity_id from event.id, not the + normalizer. + """ + from optimizely.event.user_event_factory import UserEventFactory + + with mock.patch('time.time', return_value=42.123), mock.patch( + 'uuid.uuid4', return_value='a68cf1ad-0393-4e18-af87-efe8f01a7c9c' + ): + conversion_event = UserEventFactory.create_conversion_event( + self.project_config, + 'test_event', + 'test_user', + None, + None, + ) + log_event = EventFactory.create_log_event(conversion_event, self.logger) + snapshot = log_event.params['visitors'][0]['snapshots'][0] + # Conversion entity_id comes from the event.id of the conversion event + # and must NOT pass through the campaign_id normalizer. + self.assertEqual( + self.project_config.get_event('test_event').id, + snapshot['events'][0]['entity_id'], + ) diff --git a/tests/test_event_id_normalizer.py b/tests/test_event_id_normalizer.py new file mode 100644 index 00000000..83bd8a8d --- /dev/null +++ b/tests/test_event_id_normalizer.py @@ -0,0 +1,192 @@ +# Copyright 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 +# +# 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. + +"""Unit tests for :mod:`optimizely.event.event_id_normalizer` (FSSDK-12813).""" + +import unittest + +from optimizely.event import event_id_normalizer + + +class IsNumericIdStringTest(unittest.TestCase): + """Cover :func:`event_id_normalizer.is_numeric_id_string` edge cases.""" + + def test_returns_true_for_decimal_digit_string(self): + self.assertTrue(event_id_normalizer.is_numeric_id_string('12345')) + + def test_returns_true_for_single_digit(self): + self.assertTrue(event_id_normalizer.is_numeric_id_string('0')) + self.assertTrue(event_id_normalizer.is_numeric_id_string('9')) + + def test_returns_true_for_leading_zeros(self): + # FR-001 explicitly allows leading zeros. + self.assertTrue(event_id_normalizer.is_numeric_id_string('007')) + self.assertTrue(event_id_normalizer.is_numeric_id_string('00000')) + + def test_returns_false_for_empty_string(self): + self.assertFalse(event_id_normalizer.is_numeric_id_string('')) + + def test_returns_false_for_none(self): + self.assertFalse(event_id_normalizer.is_numeric_id_string(None)) + + def test_returns_false_for_int(self): + # FR-001 requires the value to be a string. + self.assertFalse(event_id_normalizer.is_numeric_id_string(12345)) + self.assertFalse(event_id_normalizer.is_numeric_id_string(0)) + + def test_returns_false_for_float(self): + self.assertFalse(event_id_normalizer.is_numeric_id_string(123.0)) + + def test_returns_false_for_bool(self): + # ``bool`` is a subclass of ``int`` but is still not a ``str``. + self.assertFalse(event_id_normalizer.is_numeric_id_string(True)) + self.assertFalse(event_id_normalizer.is_numeric_id_string(False)) + + def test_returns_false_for_whitespace(self): + self.assertFalse(event_id_normalizer.is_numeric_id_string(' ')) + self.assertFalse(event_id_normalizer.is_numeric_id_string(' 123')) + self.assertFalse(event_id_normalizer.is_numeric_id_string('123 ')) + self.assertFalse(event_id_normalizer.is_numeric_id_string('1 2')) + self.assertFalse(event_id_normalizer.is_numeric_id_string('\t')) + self.assertFalse(event_id_normalizer.is_numeric_id_string('\n')) + + def test_returns_false_for_signed_numbers(self): + self.assertFalse(event_id_normalizer.is_numeric_id_string('-1')) + self.assertFalse(event_id_normalizer.is_numeric_id_string('+1')) + + def test_returns_false_for_decimals(self): + self.assertFalse(event_id_normalizer.is_numeric_id_string('1.0')) + self.assertFalse(event_id_normalizer.is_numeric_id_string('.5')) + + def test_returns_false_for_exponents(self): + self.assertFalse(event_id_normalizer.is_numeric_id_string('1e5')) + self.assertFalse(event_id_normalizer.is_numeric_id_string('1E5')) + + def test_returns_false_for_hex(self): + self.assertFalse(event_id_normalizer.is_numeric_id_string('0x1A')) + self.assertFalse(event_id_normalizer.is_numeric_id_string('abc')) + + def test_returns_false_for_unicode_digits(self): + # ``str.isdigit`` is True for many non-ASCII digit code points; the + # normalizer must reject these because the wire format expects ASCII. + self.assertFalse(event_id_normalizer.is_numeric_id_string('٠١')) # Arabic-Indic 01 + self.assertFalse(event_id_normalizer.is_numeric_id_string('²')) # superscript 2 + + def test_returns_false_for_collections(self): + self.assertFalse(event_id_normalizer.is_numeric_id_string(['123'])) + self.assertFalse(event_id_normalizer.is_numeric_id_string({'id': '123'})) + self.assertFalse(event_id_normalizer.is_numeric_id_string(('1',))) + + +class NormalizeCampaignIdTest(unittest.TestCase): + """Cover :func:`event_id_normalizer.normalize_campaign_id` per FR-001/002, FR-009.""" + + def test_returns_campaign_id_when_valid(self): + self.assertEqual( + '111182', + event_id_normalizer.normalize_campaign_id('111182', '111127'), + ) + + def test_falls_back_to_experiment_id_when_campaign_id_empty(self): + self.assertEqual( + '111127', + event_id_normalizer.normalize_campaign_id('', '111127'), + ) + + def test_falls_back_to_experiment_id_when_campaign_id_none(self): + self.assertEqual( + '111127', + event_id_normalizer.normalize_campaign_id(None, '111127'), + ) + + def test_falls_back_to_experiment_id_when_campaign_id_non_numeric(self): + self.assertEqual( + '111127', + event_id_normalizer.normalize_campaign_id('abc', '111127'), + ) + + def test_falls_back_to_experiment_id_when_campaign_id_whitespace(self): + self.assertEqual( + '111127', + event_id_normalizer.normalize_campaign_id(' ', '111127'), + ) + + def test_falls_back_to_experiment_id_when_campaign_id_int(self): + # An int input is invalid (FR-001 requires a string). + self.assertEqual( + '111127', + event_id_normalizer.normalize_campaign_id(111182, '111127'), + ) + + def test_returns_empty_string_when_both_invalid(self): + # Do not drop / fail dispatch (FR-006); return ''. + self.assertEqual('', event_id_normalizer.normalize_campaign_id(None, None)) + self.assertEqual('', event_id_normalizer.normalize_campaign_id('', '')) + self.assertEqual('', event_id_normalizer.normalize_campaign_id('abc', 'xyz')) + + def test_preserves_leading_zeros(self): + self.assertEqual( + '007', + event_id_normalizer.normalize_campaign_id('007', '111127'), + ) + + +class NormalizeVariationIdTest(unittest.TestCase): + """Cover :func:`event_id_normalizer.normalize_variation_id` per FR-003/004.""" + + def test_returns_variation_id_when_valid(self): + self.assertEqual( + '111129', + event_id_normalizer.normalize_variation_id('111129'), + ) + + def test_returns_none_when_empty(self): + self.assertIsNone(event_id_normalizer.normalize_variation_id('')) + + def test_returns_none_when_none(self): + self.assertIsNone(event_id_normalizer.normalize_variation_id(None)) + + def test_returns_none_when_non_string(self): + self.assertIsNone(event_id_normalizer.normalize_variation_id(111129)) + self.assertIsNone(event_id_normalizer.normalize_variation_id(123.0)) + self.assertIsNone(event_id_normalizer.normalize_variation_id(True)) + + def test_returns_none_when_non_numeric(self): + self.assertIsNone(event_id_normalizer.normalize_variation_id('variation_a')) + self.assertIsNone(event_id_normalizer.normalize_variation_id('abc')) + + def test_returns_none_when_whitespace(self): + self.assertIsNone(event_id_normalizer.normalize_variation_id(' ')) + self.assertIsNone(event_id_normalizer.normalize_variation_id(' 111129')) + + def test_returns_none_when_signed(self): + self.assertIsNone(event_id_normalizer.normalize_variation_id('-111129')) + + def test_preserves_leading_zeros(self): + self.assertEqual('007', event_id_normalizer.normalize_variation_id('007')) + + +class NormalizeStringIdTest(unittest.TestCase): + """Cover the generic :func:`event_id_normalizer.normalize_string_id` helper.""" + + def test_returns_value_when_valid(self): + self.assertEqual('42', event_id_normalizer.normalize_string_id('42')) + + def test_returns_none_when_invalid(self): + self.assertIsNone(event_id_normalizer.normalize_string_id('')) + self.assertIsNone(event_id_normalizer.normalize_string_id(None)) + self.assertIsNone(event_id_normalizer.normalize_string_id('xx')) + + +if __name__ == '__main__': + unittest.main() From 31675bf0ddb6e4a8e2f77a99e4a129344e539557 Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Thu, 25 Jun 2026 11:09:22 -0700 Subject: [PATCH 2/7] [FSSDK-12813] Relax campaign_id/entity_id validation to non-empty string per updated spec --- optimizely/event/event_id_normalizer.py | 52 ++++++----- tests/test_event_factory.py | 48 ++++++++--- tests/test_event_id_normalizer.py | 110 ++++++++++++++++-------- 3 files changed, 145 insertions(+), 65 deletions(-) diff --git a/optimizely/event/event_id_normalizer.py b/optimizely/event/event_id_normalizer.py index 575f90ce..feeb4d34 100644 --- a/optimizely/event/event_id_normalizer.py +++ b/optimizely/event/event_id_normalizer.py @@ -18,12 +18,16 @@ appear in dispatched decision events. See FSSDK-12813. Rules: - * A "numeric ID string" is a non-empty :class:`str` consisting entirely of - decimal digits ``0-9``. Leading zeros are allowed. Whitespace, negatives, - decimals, and exponents are INVALID. - * ``campaign_id`` -> when invalid, falls back to ``experiment_id`` (which is - itself passed through :func:`normalize_string_id`). - * ``variation_id`` -> when invalid, becomes ``None``. + * ``campaign_id`` and impression ``entity_id`` accept **any non-empty + string** (numeric like ``"12345"`` or opaque like ``"default-12345"`` / + ``"layer_abc"``). The fallback to ``experiment_id`` fires ONLY when the + value is the empty string, ``None``, or missing. Non-string types are + out of scope for this normalization path (per spec assumptions; the + upstream datafile producer delivers string or null values). + * ``variation_id`` retains the stricter contract: it MUST be a non-empty + string of decimal digits ``0-9`` (leading zeros allowed). Empty, + whitespace, non-string, and non-numeric inputs are normalized to + ``None`` so the wire payload carries an explicit null. * ``entity_id`` on impression events shares the campaign_id normalization and is therefore byte-equivalent to the normalized campaign_id for the same impression (FR-009). @@ -37,11 +41,23 @@ from typing import Any, Optional +def is_non_empty_string(value: Any) -> bool: + """Return ``True`` if ``value`` is a non-empty :class:`str`. + + Used for ``campaign_id`` and ``entity_id`` validation per the relaxed + FR-001 / FR-009 contract: any non-empty string is accepted regardless of + character content (IDs may be opaque, e.g. ``"default-12345"``). + """ + return isinstance(value, str) and value != '' + + def is_numeric_id_string(value: Any) -> bool: """Return ``True`` if ``value`` is a non-empty decimal-digit string. - Whitespace, signs, decimal points, exponents, and non-string types all - return ``False``. Leading zeros are accepted. + Used for ``variation_id`` validation per FR-003 (the only field that + retains the strict numeric-string contract). Whitespace, signs, decimal + points, exponents, and non-string types all return ``False``. Leading + zeros are accepted. """ if not isinstance(value, str): return False @@ -54,22 +70,18 @@ def is_numeric_id_string(value: Any) -> bool: return value.isascii() and value.isdigit() -def normalize_string_id(value: Any) -> Optional[str]: - """Return ``value`` if it's a numeric ID string, otherwise ``None``.""" - return value if is_numeric_id_string(value) else None - - def normalize_campaign_id(campaign_id: Any, experiment_id: Any) -> str: """Normalize a decision-event ``campaign_id`` (FR-001/FR-002, FR-009). - If ``campaign_id`` is a valid numeric ID string it is returned unchanged. - Otherwise the function falls back to ``experiment_id`` (after applying - the same validation). If neither is a numeric ID string, an empty string - is returned so the event still dispatches (FR-006). + Returns ``campaign_id`` unchanged when it is a non-empty string (any + character content — numeric like ``"12345"`` or opaque like + ``"default-12345"``). Otherwise falls back to ``experiment_id`` (when it + is itself a non-empty string). If neither is a non-empty string, returns + an empty string so the event still dispatches (FR-006). """ - if is_numeric_id_string(campaign_id): + if is_non_empty_string(campaign_id): return campaign_id # type: ignore[no-any-return] - if is_numeric_id_string(experiment_id): + if is_non_empty_string(experiment_id): return experiment_id # type: ignore[no-any-return] return '' @@ -78,7 +90,7 @@ def normalize_variation_id(variation_id: Any) -> Optional[str]: """Normalize a decision-event ``variation_id`` (FR-003/FR-004). Returns the original value if it is a valid numeric ID string. Otherwise - returns ``None`` so the event payload omits/clears the field for the + returns ``None`` so the event payload carries an explicit null for the downstream consumer. """ return variation_id if is_numeric_id_string(variation_id) else None diff --git a/tests/test_event_factory.py b/tests/test_event_factory.py index 2376c316..f787c7a6 100644 --- a/tests/test_event_factory.py +++ b/tests/test_event_factory.py @@ -1322,17 +1322,29 @@ def test_empty_campaign_id_falls_back_to_experiment_id(self): self.assertEqual('111127', decision['campaign_id']) self.assertEqual('111127', event['entity_id']) - def test_non_numeric_campaign_id_falls_back_to_experiment_id(self): + def test_opaque_string_campaign_id_passes_through(self): + # FSSDK-12813: opaque (non-numeric) campaign_id values are valid and + # pass through unchanged per the relaxed FR-001 contract. impression = self._build_impression('111127', 'campaign_a', '111129') decision, event = self._dispatched_decision(impression) - self.assertEqual('111127', decision['campaign_id']) - self.assertEqual('111127', event['entity_id']) + self.assertEqual('campaign_a', decision['campaign_id']) + self.assertEqual('campaign_a', event['entity_id']) - def test_whitespace_campaign_id_falls_back_to_experiment_id(self): + def test_prefixed_opaque_campaign_id_passes_through(self): + # FSSDK-12813: e.g. holdout layer IDs like "default-12345". + impression = self._build_impression('111127', 'default-12345', '111129') + decision, event = self._dispatched_decision(impression) + self.assertEqual('default-12345', decision['campaign_id']) + self.assertEqual('default-12345', event['entity_id']) + + def test_whitespace_campaign_id_passes_through(self): + # FSSDK-12813: whitespace is a non-empty string and so is accepted; + # character-content validation is deferred to the upstream datafile + # producer. impression = self._build_impression('111127', ' ', '111129') decision, event = self._dispatched_decision(impression) - self.assertEqual('111127', decision['campaign_id']) - self.assertEqual('111127', event['entity_id']) + self.assertEqual(' ', decision['campaign_id']) + self.assertEqual(' ', event['entity_id']) # ------------------------------------------------------------------ FR-003 def test_valid_variation_id_is_passed_through(self): @@ -1358,8 +1370,10 @@ def test_whitespace_variation_id_becomes_none(self): # ------------------------------------------------------------------ FR-005 def test_normalization_applies_to_rollout_decisions(self): + # FSSDK-12813: empty campaign_id falls back to experiment_id; + # non-numeric variation_id normalizes to None. impression = self._build_impression( - '111127', 'bad_layer', 'bad_var', rule_type='rollout' + '111127', '', 'bad_var', rule_type='rollout' ) decision, event = self._dispatched_decision(impression) self.assertEqual('111127', decision['campaign_id']) @@ -1384,6 +1398,16 @@ def test_normalization_applies_to_holdout_decisions(self): self.assertIsNone(decision['variation_id']) self.assertEqual('111127', event['entity_id']) + def test_holdout_with_opaque_layer_id_passes_through(self): + # FSSDK-12813: the canonical holdout case — opaque layerId like + # "default-12345" is now a valid campaign_id and is NOT replaced. + impression = self._build_impression( + '111127', 'default-12345', '111129', rule_type='holdout' + ) + decision, event = self._dispatched_decision(impression) + self.assertEqual('default-12345', decision['campaign_id']) + self.assertEqual('default-12345', event['entity_id']) + # ------------------------------------------------------------------ FR-006 def test_event_still_dispatches_when_all_ids_invalid(self): """FR-006: never drop / fail dispatch.""" @@ -1401,10 +1425,12 @@ def test_event_still_dispatches_when_all_ids_invalid(self): def test_entity_id_equals_campaign_id_byte_for_byte(self): """FR-009: ``events[].entity_id`` must equal ``decisions[].campaign_id``.""" for layer_id, exp_id, expected in [ - ('111182', '111127', '111182'), # campaign_id wins - ('', '111127', '111127'), # falls back to experiment_id - ('bad', '111127', '111127'), # fallback on invalid - ('007', '111127', '007'), # leading zeros preserved + ('111182', '111127', '111182'), # numeric campaign_id wins + ('', '111127', '111127'), # empty falls back to experiment_id + # FSSDK-12813: opaque non-numeric IDs now pass through unchanged. + ('default-12345', '111127', 'default-12345'), + ('layer_abc', '111127', 'layer_abc'), + ('007', '111127', '007'), # leading zeros preserved ]: with self.subTest(layer_id=layer_id, exp_id=exp_id): impression = self._build_impression(exp_id, layer_id, '111129') diff --git a/tests/test_event_id_normalizer.py b/tests/test_event_id_normalizer.py index 83bd8a8d..65176612 100644 --- a/tests/test_event_id_normalizer.py +++ b/tests/test_event_id_normalizer.py @@ -18,8 +18,49 @@ from optimizely.event import event_id_normalizer +class IsNonEmptyStringTest(unittest.TestCase): + """Cover :func:`event_id_normalizer.is_non_empty_string` (FR-001/FR-009). + + Any non-empty string is valid for ``campaign_id`` / ``entity_id`` — IDs + may be numeric like ``"12345"`` or opaque like ``"default-12345"``. + """ + + def test_returns_true_for_numeric_string(self): + self.assertTrue(event_id_normalizer.is_non_empty_string('12345')) + + def test_returns_true_for_opaque_string(self): + # Opaque IDs are explicitly valid for campaign_id / entity_id. + self.assertTrue(event_id_normalizer.is_non_empty_string('default-12345')) + self.assertTrue(event_id_normalizer.is_non_empty_string('layer_abc')) + self.assertTrue(event_id_normalizer.is_non_empty_string('abc')) + + def test_returns_true_for_whitespace_string(self): + # Whitespace is a non-empty string and so is accepted; the spec + # explicitly defers any character-content validation upstream. + self.assertTrue(event_id_normalizer.is_non_empty_string(' ')) + + def test_returns_false_for_empty_string(self): + self.assertFalse(event_id_normalizer.is_non_empty_string('')) + + def test_returns_false_for_none(self): + self.assertFalse(event_id_normalizer.is_non_empty_string(None)) + + def test_returns_false_for_non_string_types(self): + # Non-string types are out of scope per the spec assumptions; the + # predicate rejects them so the fallback path fires. + self.assertFalse(event_id_normalizer.is_non_empty_string(12345)) + self.assertFalse(event_id_normalizer.is_non_empty_string(123.0)) + self.assertFalse(event_id_normalizer.is_non_empty_string(True)) + self.assertFalse(event_id_normalizer.is_non_empty_string(['123'])) + self.assertFalse(event_id_normalizer.is_non_empty_string({'id': '123'})) + + class IsNumericIdStringTest(unittest.TestCase): - """Cover :func:`event_id_normalizer.is_numeric_id_string` edge cases.""" + """Cover :func:`event_id_normalizer.is_numeric_id_string` edge cases. + + Used only for ``variation_id`` (FR-003), which retains the strict + decimal-digit contract. + """ def test_returns_true_for_decimal_digit_string(self): self.assertTrue(event_id_normalizer.is_numeric_id_string('12345')) @@ -29,7 +70,7 @@ def test_returns_true_for_single_digit(self): self.assertTrue(event_id_normalizer.is_numeric_id_string('9')) def test_returns_true_for_leading_zeros(self): - # FR-001 explicitly allows leading zeros. + # FR-003 explicitly allows leading zeros. self.assertTrue(event_id_normalizer.is_numeric_id_string('007')) self.assertTrue(event_id_normalizer.is_numeric_id_string('00000')) @@ -40,7 +81,7 @@ def test_returns_false_for_none(self): self.assertFalse(event_id_normalizer.is_numeric_id_string(None)) def test_returns_false_for_int(self): - # FR-001 requires the value to be a string. + # FR-003 requires the value to be a string. self.assertFalse(event_id_normalizer.is_numeric_id_string(12345)) self.assertFalse(event_id_normalizer.is_numeric_id_string(0)) @@ -89,50 +130,60 @@ def test_returns_false_for_collections(self): class NormalizeCampaignIdTest(unittest.TestCase): - """Cover :func:`event_id_normalizer.normalize_campaign_id` per FR-001/002, FR-009.""" + """Cover :func:`event_id_normalizer.normalize_campaign_id` per FR-001/002, FR-009. + + Per the relaxed spec, any non-empty string is valid for campaign_id — + fallback to ``experiment_id`` fires only on empty/None/missing. + """ - def test_returns_campaign_id_when_valid(self): + def test_returns_campaign_id_when_numeric(self): self.assertEqual( '111182', event_id_normalizer.normalize_campaign_id('111182', '111127'), ) - def test_falls_back_to_experiment_id_when_campaign_id_empty(self): + def test_returns_campaign_id_when_opaque_string(self): + # FSSDK-12813: opaque IDs (e.g. holdout layer IDs) pass through. self.assertEqual( - '111127', - event_id_normalizer.normalize_campaign_id('', '111127'), + 'default-12345', + event_id_normalizer.normalize_campaign_id('default-12345', '111127'), + ) + self.assertEqual( + 'layer_abc', + event_id_normalizer.normalize_campaign_id('layer_abc', '111127'), ) - def test_falls_back_to_experiment_id_when_campaign_id_none(self): + def test_returns_campaign_id_when_whitespace_string(self): + # Whitespace is non-empty; passes through (validation deferred upstream). self.assertEqual( - '111127', - event_id_normalizer.normalize_campaign_id(None, '111127'), + ' ', + event_id_normalizer.normalize_campaign_id(' ', '111127'), ) - def test_falls_back_to_experiment_id_when_campaign_id_non_numeric(self): + def test_falls_back_to_experiment_id_when_campaign_id_empty(self): self.assertEqual( '111127', - event_id_normalizer.normalize_campaign_id('abc', '111127'), + event_id_normalizer.normalize_campaign_id('', '111127'), ) - def test_falls_back_to_experiment_id_when_campaign_id_whitespace(self): + def test_falls_back_to_experiment_id_when_campaign_id_none(self): self.assertEqual( '111127', - event_id_normalizer.normalize_campaign_id(' ', '111127'), + event_id_normalizer.normalize_campaign_id(None, '111127'), ) - def test_falls_back_to_experiment_id_when_campaign_id_int(self): - # An int input is invalid (FR-001 requires a string). + def test_falls_back_to_opaque_experiment_id(self): + # Both fields may be opaque non-numeric strings. self.assertEqual( - '111127', - event_id_normalizer.normalize_campaign_id(111182, '111127'), + 'exp_42', + event_id_normalizer.normalize_campaign_id('', 'exp_42'), ) - def test_returns_empty_string_when_both_invalid(self): + def test_returns_empty_string_when_both_empty_or_none(self): # Do not drop / fail dispatch (FR-006); return ''. self.assertEqual('', event_id_normalizer.normalize_campaign_id(None, None)) self.assertEqual('', event_id_normalizer.normalize_campaign_id('', '')) - self.assertEqual('', event_id_normalizer.normalize_campaign_id('abc', 'xyz')) + self.assertEqual('', event_id_normalizer.normalize_campaign_id(None, '')) def test_preserves_leading_zeros(self): self.assertEqual( @@ -142,7 +193,10 @@ def test_preserves_leading_zeros(self): class NormalizeVariationIdTest(unittest.TestCase): - """Cover :func:`event_id_normalizer.normalize_variation_id` per FR-003/004.""" + """Cover :func:`event_id_normalizer.normalize_variation_id` per FR-003/004. + + ``variation_id`` retains the strict numeric-string contract. + """ def test_returns_variation_id_when_valid(self): self.assertEqual( @@ -176,17 +230,5 @@ def test_preserves_leading_zeros(self): self.assertEqual('007', event_id_normalizer.normalize_variation_id('007')) -class NormalizeStringIdTest(unittest.TestCase): - """Cover the generic :func:`event_id_normalizer.normalize_string_id` helper.""" - - def test_returns_value_when_valid(self): - self.assertEqual('42', event_id_normalizer.normalize_string_id('42')) - - def test_returns_none_when_invalid(self): - self.assertIsNone(event_id_normalizer.normalize_string_id('')) - self.assertIsNone(event_id_normalizer.normalize_string_id(None)) - self.assertIsNone(event_id_normalizer.normalize_string_id('xx')) - - if __name__ == '__main__': unittest.main() From 6903782690e81e7adb144156ba12fc4279eef835 Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Thu, 25 Jun 2026 14:14:22 -0700 Subject: [PATCH 3/7] [FSSDK-12813] Remove ticket references from code comments per cross-sdk guideline --- optimizely/event/event_factory.py | 6 ------ optimizely/event/event_id_normalizer.py | 2 +- optimizely/event/payload.py | 3 --- optimizely/event_builder.py | 4 ---- tests/test_event_factory.py | 19 +++++++------------ tests/test_event_id_normalizer.py | 4 ++-- 6 files changed, 10 insertions(+), 28 deletions(-) diff --git a/optimizely/event/event_factory.py b/optimizely/event/event_factory.py index 17f62b42..a52db94e 100644 --- a/optimizely/event/event_factory.py +++ b/optimizely/event/event_factory.py @@ -135,12 +135,6 @@ def _create_visitor(cls, event: Optional[user_event.UserEvent], logger: Logger) if isinstance(event.experiment, entities.Experiment): experiment_layerId = event.experiment.layerId - # FSSDK-12813: Normalize decision-event IDs uniformly across all - # decision types (experiment, feature test, rollout, holdout). - # campaign_id falls back to experiment_id when invalid. - # variation_id becomes None when invalid. - # entity_id on the impression event mirrors campaign_id (FR-009) - # so the two fields are byte-equivalent for the same event. normalized_campaign_id = event_id_normalizer.normalize_campaign_id( experiment_layerId, experiment_id ) diff --git a/optimizely/event/event_id_normalizer.py b/optimizely/event/event_id_normalizer.py index feeb4d34..969069a0 100644 --- a/optimizely/event/event_id_normalizer.py +++ b/optimizely/event/event_id_normalizer.py @@ -15,7 +15,7 @@ This module provides byte-equivalent, cross-SDK normalization for the ``campaign_id``, ``variation_id``, and impression ``entity_id`` fields that -appear in dispatched decision events. See FSSDK-12813. +appear in dispatched decision events. Rules: * ``campaign_id`` and impression ``entity_id`` accept **any non-empty diff --git a/optimizely/event/payload.py b/optimizely/event/payload.py index fa90b958..6d0e8970 100644 --- a/optimizely/event/payload.py +++ b/optimizely/event/payload.py @@ -80,9 +80,6 @@ def __init__( ): self.campaign_id = campaign_id self.experiment_id = experiment_id - # FSSDK-12813: variation_id may be None when input is invalid / - # non-numeric (FR-003/FR-004). All other decision fields remain - # strings. self.variation_id = variation_id self.metadata = metadata diff --git a/optimizely/event_builder.py b/optimizely/event_builder.py index 925c8e0b..96ceb937 100644 --- a/optimizely/event_builder.py +++ b/optimizely/event_builder.py @@ -191,10 +191,6 @@ def _get_required_params_for_impression( """ snapshot: dict[str, list[dict[str, Any]]] = {} - # FSSDK-12813: Normalize decision-event IDs uniformly across all - # decision types. campaign_id falls back to experiment_id when - # invalid; variation_id becomes None when invalid; entity_id mirrors - # the normalized campaign_id (FR-009). normalized_campaign_id = event_id_normalizer.normalize_campaign_id( experiment.layerId, experiment.id ) diff --git a/tests/test_event_factory.py b/tests/test_event_factory.py index f787c7a6..b038c090 100644 --- a/tests/test_event_factory.py +++ b/tests/test_event_factory.py @@ -1240,7 +1240,7 @@ def test_create_impression_event_without_cmab_uuid(self): class EventFactoryIdNormalizationIntegrationTest(base.BaseTest): - """FSSDK-12813: end-to-end decision-event ID normalization. + """End-to-end decision-event ID normalization. These tests build real ``ImpressionEvent`` instances using crafted Experiment/Variation objects, then call ``EventFactory.create_log_event`` @@ -1323,24 +1323,21 @@ def test_empty_campaign_id_falls_back_to_experiment_id(self): self.assertEqual('111127', event['entity_id']) def test_opaque_string_campaign_id_passes_through(self): - # FSSDK-12813: opaque (non-numeric) campaign_id values are valid and - # pass through unchanged per the relaxed FR-001 contract. impression = self._build_impression('111127', 'campaign_a', '111129') decision, event = self._dispatched_decision(impression) self.assertEqual('campaign_a', decision['campaign_id']) self.assertEqual('campaign_a', event['entity_id']) def test_prefixed_opaque_campaign_id_passes_through(self): - # FSSDK-12813: e.g. holdout layer IDs like "default-12345". + # Holdout layer IDs are opaque strings like "default-12345". impression = self._build_impression('111127', 'default-12345', '111129') decision, event = self._dispatched_decision(impression) self.assertEqual('default-12345', decision['campaign_id']) self.assertEqual('default-12345', event['entity_id']) def test_whitespace_campaign_id_passes_through(self): - # FSSDK-12813: whitespace is a non-empty string and so is accepted; - # character-content validation is deferred to the upstream datafile - # producer. + # Whitespace is a non-empty string; character-content validation is + # the upstream datafile producer's responsibility. impression = self._build_impression('111127', ' ', '111129') decision, event = self._dispatched_decision(impression) self.assertEqual(' ', decision['campaign_id']) @@ -1370,8 +1367,6 @@ def test_whitespace_variation_id_becomes_none(self): # ------------------------------------------------------------------ FR-005 def test_normalization_applies_to_rollout_decisions(self): - # FSSDK-12813: empty campaign_id falls back to experiment_id; - # non-numeric variation_id normalizes to None. impression = self._build_impression( '111127', '', 'bad_var', rule_type='rollout' ) @@ -1399,8 +1394,8 @@ def test_normalization_applies_to_holdout_decisions(self): self.assertEqual('111127', event['entity_id']) def test_holdout_with_opaque_layer_id_passes_through(self): - # FSSDK-12813: the canonical holdout case — opaque layerId like - # "default-12345" is now a valid campaign_id and is NOT replaced. + # Canonical holdout case: opaque layerId like "default-12345" is a + # valid campaign_id and must not be replaced. impression = self._build_impression( '111127', 'default-12345', '111129', rule_type='holdout' ) @@ -1427,7 +1422,7 @@ def test_entity_id_equals_campaign_id_byte_for_byte(self): for layer_id, exp_id, expected in [ ('111182', '111127', '111182'), # numeric campaign_id wins ('', '111127', '111127'), # empty falls back to experiment_id - # FSSDK-12813: opaque non-numeric IDs now pass through unchanged. + # Opaque non-numeric IDs pass through unchanged. ('default-12345', '111127', 'default-12345'), ('layer_abc', '111127', 'layer_abc'), ('007', '111127', '007'), # leading zeros preserved diff --git a/tests/test_event_id_normalizer.py b/tests/test_event_id_normalizer.py index 65176612..e478af64 100644 --- a/tests/test_event_id_normalizer.py +++ b/tests/test_event_id_normalizer.py @@ -11,7 +11,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -"""Unit tests for :mod:`optimizely.event.event_id_normalizer` (FSSDK-12813).""" +"""Unit tests for :mod:`optimizely.event.event_id_normalizer`.""" import unittest @@ -143,7 +143,7 @@ def test_returns_campaign_id_when_numeric(self): ) def test_returns_campaign_id_when_opaque_string(self): - # FSSDK-12813: opaque IDs (e.g. holdout layer IDs) pass through. + # Opaque IDs (e.g. holdout layer IDs) pass through. self.assertEqual( 'default-12345', event_id_normalizer.normalize_campaign_id('default-12345', '111127'), From db29893c3aa6e3ec7e3a1147458ae761ded325af Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Thu, 25 Jun 2026 17:45:09 -0700 Subject: [PATCH 4/7] [FSSDK-12813] Use TypeGuard for ID predicates to drop type: ignore --- optimizely/event/event_id_normalizer.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/optimizely/event/event_id_normalizer.py b/optimizely/event/event_id_normalizer.py index 969069a0..9486d5f2 100644 --- a/optimizely/event/event_id_normalizer.py +++ b/optimizely/event/event_id_normalizer.py @@ -38,10 +38,16 @@ from __future__ import annotations +from sys import version_info from typing import Any, Optional +if version_info < (3, 10): + from typing_extensions import TypeGuard +else: + from typing import TypeGuard -def is_non_empty_string(value: Any) -> bool: + +def is_non_empty_string(value: Any) -> TypeGuard[str]: """Return ``True`` if ``value`` is a non-empty :class:`str`. Used for ``campaign_id`` and ``entity_id`` validation per the relaxed @@ -51,7 +57,7 @@ def is_non_empty_string(value: Any) -> bool: return isinstance(value, str) and value != '' -def is_numeric_id_string(value: Any) -> bool: +def is_numeric_id_string(value: Any) -> TypeGuard[str]: """Return ``True`` if ``value`` is a non-empty decimal-digit string. Used for ``variation_id`` validation per FR-003 (the only field that @@ -80,9 +86,9 @@ def normalize_campaign_id(campaign_id: Any, experiment_id: Any) -> str: an empty string so the event still dispatches (FR-006). """ if is_non_empty_string(campaign_id): - return campaign_id # type: ignore[no-any-return] + return campaign_id if is_non_empty_string(experiment_id): - return experiment_id # type: ignore[no-any-return] + return experiment_id return '' From c70e58aa1cdbd0713d78cc337d9337598c12cd68 Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Thu, 25 Jun 2026 20:30:15 -0700 Subject: [PATCH 5/7] [FSSDK-12813] Add 2026 to copyright year in changed files --- optimizely/event/event_factory.py | 2 +- optimizely/event/payload.py | 2 +- optimizely/event_builder.py | 2 +- tests/test_event_factory.py | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/optimizely/event/event_factory.py b/optimizely/event/event_factory.py index a52db94e..b6a9c02a 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 diff --git a/optimizely/event/payload.py b/optimizely/event/payload.py index 6d0e8970..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 diff --git a/optimizely/event_builder.py b/optimizely/event_builder.py index 96ceb937..7ef6b347 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 diff --git a/tests/test_event_factory.py b/tests/test_event_factory.py index b038c090..70375ebb 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 From f79048b1c6af4ea0e88ae9f60afe8b2a95617f9b Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Fri, 26 Jun 2026 09:21:19 -0700 Subject: [PATCH 6/7] [FSSDK-12813] Trigger CI From 72ac0b2a5edd33496faa1e4b68931b4ce95c6709 Mon Sep 17 00:00:00 2001 From: Farhan Anjum Date: Thu, 2 Jul 2026 21:19:15 +0600 Subject: [PATCH 7/7] Refactor comments for clarity and remove spec references in event ID normalization and holdout configuration tests --- optimizely/event/event_id_normalizer.py | 27 +++++++++---------------- optimizely/project_config.py | 2 +- tests/test_event_factory.py | 20 +++++------------- tests/test_event_id_normalizer.py | 25 +++++++++++------------ tests/test_holdout_config.py | 2 +- 5 files changed, 28 insertions(+), 48 deletions(-) diff --git a/optimizely/event/event_id_normalizer.py b/optimizely/event/event_id_normalizer.py index 9486d5f2..f2d4235d 100644 --- a/optimizely/event/event_id_normalizer.py +++ b/optimizely/event/event_id_normalizer.py @@ -22,21 +22,20 @@ string** (numeric like ``"12345"`` or opaque like ``"default-12345"`` / ``"layer_abc"``). The fallback to ``experiment_id`` fires ONLY when the value is the empty string, ``None``, or missing. Non-string types are - out of scope for this normalization path (per spec assumptions; the - upstream datafile producer delivers string or null values). + out of scope for this normalization path (the upstream datafile + producer delivers string or null values). * ``variation_id`` retains the stricter contract: it MUST be a non-empty string of decimal digits ``0-9`` (leading zeros allowed). Empty, whitespace, non-string, and non-numeric inputs are normalized to ``None`` so the wire payload carries an explicit null. * ``entity_id`` on impression events shares the campaign_id normalization and is therefore byte-equivalent to the normalized campaign_id for the - same impression (FR-009). + same impression. The normalization path MUST NOT log, warn, or raise. It must never drop or defer event dispatch. """ -from __future__ import annotations from sys import version_info from typing import Any, Optional @@ -49,9 +48,7 @@ def is_non_empty_string(value: Any) -> TypeGuard[str]: """Return ``True`` if ``value`` is a non-empty :class:`str`. - - Used for ``campaign_id`` and ``entity_id`` validation per the relaxed - FR-001 / FR-009 contract: any non-empty string is accepted regardless of + Any non-empty string is accepted regardless of character content (IDs may be opaque, e.g. ``"default-12345"``). """ return isinstance(value, str) and value != '' @@ -59,31 +56,25 @@ def is_non_empty_string(value: Any) -> TypeGuard[str]: def is_numeric_id_string(value: Any) -> TypeGuard[str]: """Return ``True`` if ``value`` is a non-empty decimal-digit string. - - Used for ``variation_id`` validation per FR-003 (the only field that - retains the strict numeric-string contract). Whitespace, signs, decimal - points, exponents, and non-string types all return ``False``. Leading + Whitespace, signs, decimal points, exponents + and non-string types all return ``False``. Leading zeros are accepted. """ if not isinstance(value, str): return False if value == '': return False - # ``str.isdigit`` rejects everything except [0-9] characters and the - # empty string. We've already excluded the empty case above. Note that - # ``isdigit`` also accepts some non-ASCII digit code points; ``isascii`` - # combined with ``isdigit`` restricts us to plain decimal digits. return value.isascii() and value.isdigit() def normalize_campaign_id(campaign_id: Any, experiment_id: Any) -> str: - """Normalize a decision-event ``campaign_id`` (FR-001/FR-002, FR-009). + """Normalize a decision-event ``campaign_id``. Returns ``campaign_id`` unchanged when it is a non-empty string (any character content — numeric like ``"12345"`` or opaque like ``"default-12345"``). Otherwise falls back to ``experiment_id`` (when it is itself a non-empty string). If neither is a non-empty string, returns - an empty string so the event still dispatches (FR-006). + an empty string so the event still dispatches. """ if is_non_empty_string(campaign_id): return campaign_id @@ -93,7 +84,7 @@ def normalize_campaign_id(campaign_id: Any, experiment_id: Any) -> str: def normalize_variation_id(variation_id: Any) -> Optional[str]: - """Normalize a decision-event ``variation_id`` (FR-003/FR-004). + """Normalize a decision-event ``variation_id``. Returns the original value if it is a valid numeric ID string. Otherwise returns ``None`` so the event payload carries an explicit null for the diff --git a/optimizely/project_config.py b/optimizely/project_config.py index 728bf89a..a7152ae8 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -122,7 +122,7 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): self.global_holdouts.append(holdout) # Process local holdouts: every entry must carry 'includedRules' (list of rule IDs). - # Entries without 'includedRules' are invalid per spec — log an error and exclude + # Entries without 'includedRules' are invalid — log an error and exclude # them from evaluation (do NOT fall back to global application). for holdout_data in local_holdouts_data: if 'includedRules' not in holdout_data or holdout_data.get('includedRules') is None: diff --git a/tests/test_event_factory.py b/tests/test_event_factory.py index 70375ebb..b86a1544 100644 --- a/tests/test_event_factory.py +++ b/tests/test_event_factory.py @@ -1244,7 +1244,7 @@ class EventFactoryIdNormalizationIntegrationTest(base.BaseTest): These tests build real ``ImpressionEvent`` instances using crafted Experiment/Variation objects, then call ``EventFactory.create_log_event`` - and inspect the dispatched payload. They exercise FR-001..FR-009. + and inspect the dispatched payload. """ def setUp(self, *args, **kwargs): @@ -1307,15 +1307,13 @@ def _dispatched_decision(self, impression_event): snapshot = log_event.params['visitors'][0]['snapshots'][0] return snapshot['decisions'][0], snapshot['events'][0] - # ------------------------------------------------------------------ FR-001 def test_valid_campaign_id_is_passed_through(self): impression = self._build_impression('111127', '111182', '111129') decision, event = self._dispatched_decision(impression) self.assertEqual('111182', decision['campaign_id']) - # FR-009: entity_id mirrors campaign_id byte-for-byte. + # entity_id mirrors campaign_id byte-for-byte. self.assertEqual(decision['campaign_id'], event['entity_id']) - # ------------------------------------------------------------------ FR-002 def test_empty_campaign_id_falls_back_to_experiment_id(self): impression = self._build_impression('111127', '', '111129') decision, event = self._dispatched_decision(impression) @@ -1343,13 +1341,11 @@ def test_whitespace_campaign_id_passes_through(self): self.assertEqual(' ', decision['campaign_id']) self.assertEqual(' ', event['entity_id']) - # ------------------------------------------------------------------ FR-003 def test_valid_variation_id_is_passed_through(self): impression = self._build_impression('111127', '111182', '111129') decision, _ = self._dispatched_decision(impression) self.assertEqual('111129', decision['variation_id']) - # ------------------------------------------------------------------ FR-004 def test_empty_variation_id_becomes_none(self): impression = self._build_impression('111127', '111182', '') decision, _ = self._dispatched_decision(impression) @@ -1365,7 +1361,6 @@ def test_whitespace_variation_id_becomes_none(self): decision, _ = self._dispatched_decision(impression) self.assertIsNone(decision['variation_id']) - # ------------------------------------------------------------------ FR-005 def test_normalization_applies_to_rollout_decisions(self): impression = self._build_impression( '111127', '', 'bad_var', rule_type='rollout' @@ -1403,9 +1398,8 @@ def test_holdout_with_opaque_layer_id_passes_through(self): self.assertEqual('default-12345', decision['campaign_id']) self.assertEqual('default-12345', event['entity_id']) - # ------------------------------------------------------------------ FR-006 def test_event_still_dispatches_when_all_ids_invalid(self): - """FR-006: never drop / fail dispatch.""" + """Event must still dispatch even when all IDs are invalid.""" impression = self._build_impression('', '', '') log_event = EventFactory.create_log_event(impression, self.logger) self.assertIsNotNone(log_event) @@ -1416,9 +1410,8 @@ def test_event_still_dispatches_when_all_ids_invalid(self): self.assertEqual('', event['entity_id']) self.assertIsNone(decision['variation_id']) - # ------------------------------------------------------------------ FR-009 def test_entity_id_equals_campaign_id_byte_for_byte(self): - """FR-009: ``events[].entity_id`` must equal ``decisions[].campaign_id``.""" + """``events[].entity_id`` must equal ``decisions[].campaign_id``.""" for layer_id, exp_id, expected in [ ('111182', '111127', '111182'), # numeric campaign_id wins ('', '111127', '111127'), # empty falls back to experiment_id @@ -1433,11 +1426,8 @@ def test_entity_id_equals_campaign_id_byte_for_byte(self): self.assertEqual(expected, decision['campaign_id']) self.assertEqual(decision['campaign_id'], event['entity_id']) - # ----------------------------------------------------------------- FR-010 def test_conversion_event_entity_id_unchanged(self): - """FR-010: conversion events derive entity_id from event.id, not the - normalizer. - """ + """Conversion events derive entity_id from event.id, not the normalizer.""" from optimizely.event.user_event_factory import UserEventFactory with mock.patch('time.time', return_value=42.123), mock.patch( diff --git a/tests/test_event_id_normalizer.py b/tests/test_event_id_normalizer.py index e478af64..ca261482 100644 --- a/tests/test_event_id_normalizer.py +++ b/tests/test_event_id_normalizer.py @@ -19,7 +19,7 @@ class IsNonEmptyStringTest(unittest.TestCase): - """Cover :func:`event_id_normalizer.is_non_empty_string` (FR-001/FR-009). + """Cover :func:`event_id_normalizer.is_non_empty_string`. Any non-empty string is valid for ``campaign_id`` / ``entity_id`` — IDs may be numeric like ``"12345"`` or opaque like ``"default-12345"``. @@ -35,8 +35,8 @@ def test_returns_true_for_opaque_string(self): self.assertTrue(event_id_normalizer.is_non_empty_string('abc')) def test_returns_true_for_whitespace_string(self): - # Whitespace is a non-empty string and so is accepted; the spec - # explicitly defers any character-content validation upstream. + # Whitespace is a non-empty string and so is accepted; + # character-content validation is deferred upstream. self.assertTrue(event_id_normalizer.is_non_empty_string(' ')) def test_returns_false_for_empty_string(self): @@ -46,8 +46,7 @@ def test_returns_false_for_none(self): self.assertFalse(event_id_normalizer.is_non_empty_string(None)) def test_returns_false_for_non_string_types(self): - # Non-string types are out of scope per the spec assumptions; the - # predicate rejects them so the fallback path fires. + # Non-string types are rejected so the fallback path fires. self.assertFalse(event_id_normalizer.is_non_empty_string(12345)) self.assertFalse(event_id_normalizer.is_non_empty_string(123.0)) self.assertFalse(event_id_normalizer.is_non_empty_string(True)) @@ -58,7 +57,7 @@ def test_returns_false_for_non_string_types(self): class IsNumericIdStringTest(unittest.TestCase): """Cover :func:`event_id_normalizer.is_numeric_id_string` edge cases. - Used only for ``variation_id`` (FR-003), which retains the strict + Used only for ``variation_id``, which retains the strict decimal-digit contract. """ @@ -70,7 +69,7 @@ def test_returns_true_for_single_digit(self): self.assertTrue(event_id_normalizer.is_numeric_id_string('9')) def test_returns_true_for_leading_zeros(self): - # FR-003 explicitly allows leading zeros. + # Leading zeros are explicitly allowed. self.assertTrue(event_id_normalizer.is_numeric_id_string('007')) self.assertTrue(event_id_normalizer.is_numeric_id_string('00000')) @@ -81,7 +80,7 @@ def test_returns_false_for_none(self): self.assertFalse(event_id_normalizer.is_numeric_id_string(None)) def test_returns_false_for_int(self): - # FR-003 requires the value to be a string. + # The value must be a string. self.assertFalse(event_id_normalizer.is_numeric_id_string(12345)) self.assertFalse(event_id_normalizer.is_numeric_id_string(0)) @@ -130,10 +129,10 @@ def test_returns_false_for_collections(self): class NormalizeCampaignIdTest(unittest.TestCase): - """Cover :func:`event_id_normalizer.normalize_campaign_id` per FR-001/002, FR-009. + """Cover :func:`event_id_normalizer.normalize_campaign_id`. - Per the relaxed spec, any non-empty string is valid for campaign_id — - fallback to ``experiment_id`` fires only on empty/None/missing. + Any non-empty string is valid for campaign_id — fallback to + ``experiment_id`` fires only on empty/None/missing. """ def test_returns_campaign_id_when_numeric(self): @@ -180,7 +179,7 @@ def test_falls_back_to_opaque_experiment_id(self): ) def test_returns_empty_string_when_both_empty_or_none(self): - # Do not drop / fail dispatch (FR-006); return ''. + # Do not drop / fail dispatch; return ''. self.assertEqual('', event_id_normalizer.normalize_campaign_id(None, None)) self.assertEqual('', event_id_normalizer.normalize_campaign_id('', '')) self.assertEqual('', event_id_normalizer.normalize_campaign_id(None, '')) @@ -193,7 +192,7 @@ def test_preserves_leading_zeros(self): class NormalizeVariationIdTest(unittest.TestCase): - """Cover :func:`event_id_normalizer.normalize_variation_id` per FR-003/004. + """Cover :func:`event_id_normalizer.normalize_variation_id`. ``variation_id`` retains the strict numeric-string contract. """ diff --git a/tests/test_holdout_config.py b/tests/test_holdout_config.py index 6420e42c..e9416e11 100644 --- a/tests/test_holdout_config.py +++ b/tests/test_holdout_config.py @@ -471,7 +471,7 @@ def test_local_holdouts_section_entries_excluded_from_global_list(self): self.assertEqual(config.get_global_holdouts(), []) def test_local_holdouts_missing_included_rules_logged_and_excluded(self): - """Entries in 'localHoldouts' without 'includedRules' are invalid per spec. + """Entries in 'localHoldouts' without 'includedRules' are invalid. SDK must log an error and exclude the entry from evaluation. It must NOT fall back to global application (the partition between sections is hard).