[FSSDK-12813] Relax campaign_id/entity_id validation to non-empty string per updated spec

Author: jaeopt

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

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')

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()