[AI-FSSDK] [FSSDK-12834] Replace non-numeric holdout placeholder IDs in event-construction tests (#646)

jaeopt · web-flow · commit 7babd8c27396 · 2026-07-01T18:20:47.000+06:00
* [AI-FSSDK] [FSSDK-12834] Replace non-numeric holdout placeholder IDs in event-construction tests

* [AI-FSSDK] [FSSDK-12834] Refine FR-011 inline comments for relaxed campaign_id contract

* [AI-FSSDK] [FSSDK-12834] Relax campaign_id/entity_id validation to any non-empty string

* [FSSDK-12834] Strip ticket refs and tighten comments

Drop FSSDK-12813 / FSSDK-12834 references from production and test
code (ticket IDs belong in commit messages and PR titles, not source).
Tighten the verbose multi-line docstrings down to one short line each
or remove them where the code is self-explanatory.

No behavior change.
diff --git a/Sources/Implementation/Events/BatchEventBuilder.swift b/Sources/Implementation/Events/BatchEventBuilder.swift
@@ -62,30 +62,23 @@ class BatchEventBuilder {
                                 dispatchEvents: [dispatchEvent])
     }
 
-    // MARK: - Decision Event ID Normalization (FSSDK-12813)
+    // MARK: - Decision Event ID Normalization
 
-    /// Normalizes `campaign_id` and `variation_id` so every dispatched decision
-    /// event carries pipeline-valid values, regardless of decision type
-    /// (experiment, feature test, rollout, or holdout). See spec FR-001–FR-005.
+    /// Normalizes `campaign_id` and `variation_id` for every dispatched
+    /// decision event (experiment, feature test, rollout, holdout).
     ///
-    /// - `campaign_id` falls back to `experiment_id` when the raw value is not a
-    ///   non-empty decimal-digit string. For well-formed datafiles this is a
-    ///   no-op for non-holdout decisions; the fallback fires on holdout events
-    ///   (which legitimately may lack `layerId`) and acts as a safety net for
-    ///   any future malformed input.
-    /// - `variation_id` becomes `nil` (emitted as JSON `null`) when the raw
-    ///   value is not a non-empty decimal-digit string.
+    /// - `campaign_id` (mirrored as `events[].entity_id`) accepts any non-empty
+    ///   string and falls back to `experiment_id` only on empty/whitespace
+    ///   input. Opaque IDs (e.g. `"layer_abc"`) pass through.
+    /// - `variation_id` must be a non-empty decimal-digit string; anything
+    ///   else becomes `nil` (encoded as JSON `null`).
+    /// - `experiment_id` is never validated here — it is always passed
+    ///   through so the event is never dropped.
     static func normalizeDecisionIds(rawCampaignId: String,
                                      rawVariationId: String?,
                                      experimentId: String) -> (campaignId: String, variationId: String?) {
-        // campaign_id must be a numeric string, otherwise fall back to
-        // experiment_id. The upstream experiment_id is passed through unchanged
-        // even if it is itself invalid (FR-006 — never drop the event).
-        let campaignId = isValidNumericIdString(rawCampaignId) ? rawCampaignId : experimentId
+        let campaignId = isValidStringId(rawCampaignId) ? rawCampaignId : experimentId
 
-        // variation_id must be a numeric string or JSON null. Anything else
-        // (empty, whitespace, non-numeric) becomes nil so the encoder emits
-        // JSON `null`.
         let variationId: String?
         if let raw = rawVariationId, isValidNumericIdString(raw) {
             variationId = raw
@@ -96,16 +89,19 @@ class BatchEventBuilder {
         return (campaignId, variationId)
     }
 
-    /// Returns true iff `value` is a non-empty string consisting entirely of
-    /// decimal digits `[0-9]`. Empty strings, whitespace-only strings, and any
-    /// string containing non-digit characters return false. Leading zeros are
-    /// allowed because Optimizely IDs are opaque identifiers (see spec).
+    /// True iff `value` is non-empty and contains at least one non-whitespace
+    /// character. Used for the relaxed `campaign_id` / `entity_id` contract.
+    static func isValidStringId(_ value: String) -> Bool {
+        guard !value.isEmpty else { return false }
+        return value.contains { !$0.isWhitespace }
+    }
+
+    /// True iff `value` is a non-empty ASCII decimal-digit string `[0-9]+`.
+    /// Used for the strict `variation_id` contract. We walk unicode scalars
+    /// directly because `CharacterSet.decimalDigits` would also accept
+    /// non-ASCII digit forms (e.g. Arabic-Indic).
     static func isValidNumericIdString(_ value: String) -> Bool {
         guard !value.isEmpty else { return false }
-        // `CharacterSet.decimalDigits` includes non-ASCII digit forms (e.g.
-        // Arabic-Indic digits). Spec restricts validity to ASCII `[0-9]`, so we
-        // walk the unicode scalars explicitly instead of using
-        // `rangeOfCharacter(from: CharacterSet.decimalDigits.inverted)`.
         for scalar in value.unicodeScalars {
             if scalar.value < 0x30 || scalar.value > 0x39 {
                 return false
diff --git a/Tests/OptimizelyTests-Common/BatchEventBuilderTests_Events.swift b/Tests/OptimizelyTests-Common/BatchEventBuilderTests_Events.swift
@@ -1,5 +1,5 @@
 //
-// Copyright 2019-2021, 2023 Optimizely, Inc. and contributors
+// Copyright 2019-2021, 2023, 2026 Optimizely, Inc. and contributors
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -27,19 +27,22 @@ class BatchEventBuilderTests_Events: XCTestCase {
     var project: Project!
     let datafile = OTUtils.loadJSONDatafile("api_datafile")!
     
+    // Numeric strings used for both id fields: campaign_id accepts any
+    // non-empty string, variation_id requires digits-only. Numeric satisfies
+    // both and keeps fixtures uniform.
     var sampleHoldout: [String: Any] {
         return [
             "status": "Running",
-            "id": "holdout_4444444",
+            "id": "4444444",
             "key": "holdout_key",
             "trafficAllocation": [
-                ["entityId": "holdout_variation_a11", "endOfRange": 10000] // 100% traffic allocation
+                ["entityId": "4444411", "endOfRange": 10000] // 100% traffic allocation
             ],
             "audienceIds": [],
             "variations": [
                 [
                     "variables": [],
-                    "id": "holdout_variation_a11",
+                    "id": "4444411",
                     "key": "holdout_a"
                 ]
             ]
diff --git a/Tests/OptimizelyTests-Common/BatchEventBuilderTests_HoldoutIds.swift b/Tests/OptimizelyTests-Common/BatchEventBuilderTests_HoldoutIds.swift
@@ -14,21 +14,43 @@
 // limitations under the License.
 //
 
-// Tests for FSSDK-12813: every decision event (experiment, feature test,
-// rollout, or holdout) must carry a valid numeric `campaign_id` (falling back
-// to `experiment_id` if invalid) and either a valid numeric `variation_id` or
-// JSON `null`. The normalization is uniform across decision types per FR-005.
+// Tests for decision-event id normalization:
+//   campaign_id / events[].entity_id: any non-empty string; fallback to
+//     experiment_id on empty / whitespace / null.
+//   variation_id: decimal-digit string only; otherwise JSON null.
 
 import XCTest
 
 class BatchEventBuilderTests_HoldoutIds: XCTestCase {
 
-    // MARK: - isValidNumericIdString
+    // MARK: - isValidStringId (relaxed contract for campaign_id / entity_id)
+
+    func testIsValidStringId_nonEmptyContent() {
+        XCTAssertTrue(BatchEventBuilder.isValidStringId("12345"))
+        XCTAssertTrue(BatchEventBuilder.isValidStringId("default-12345"))
+        XCTAssertTrue(BatchEventBuilder.isValidStringId("layer_abc"))
+        XCTAssertTrue(BatchEventBuilder.isValidStringId("a"))
+        XCTAssertTrue(BatchEventBuilder.isValidStringId("0"))
+        XCTAssertTrue(BatchEventBuilder.isValidStringId("not_a_number"))
+        // Non-ASCII content is accepted; no character-set restriction.
+        XCTAssertTrue(BatchEventBuilder.isValidStringId("\u{0660}\u{0661}"))
+    }
+
+    func testIsValidStringId_emptyOrWhitespaceOnly() {
+        XCTAssertFalse(BatchEventBuilder.isValidStringId(""))
+        XCTAssertFalse(BatchEventBuilder.isValidStringId(" "))
+        XCTAssertFalse(BatchEventBuilder.isValidStringId("   "))
+        XCTAssertFalse(BatchEventBuilder.isValidStringId("\t"))
+        XCTAssertFalse(BatchEventBuilder.isValidStringId("\n"))
+        XCTAssertFalse(BatchEventBuilder.isValidStringId(" \t\n "))
+    }
+
+    // MARK: - isValidNumericIdString (strict contract for variation_id)
 
     func testIsValidNumericIdString_validDigits() {
         XCTAssertTrue(BatchEventBuilder.isValidNumericIdString("12345"))
         XCTAssertTrue(BatchEventBuilder.isValidNumericIdString("0"))
-        // Leading zeros are allowed because IDs are opaque.
+        // Leading zeros allowed: IDs are opaque.
         XCTAssertTrue(BatchEventBuilder.isValidNumericIdString("007"))
         XCTAssertTrue(BatchEventBuilder.isValidNumericIdString("10390977714"))
     }
@@ -49,14 +71,13 @@ class BatchEventBuilderTests_HoldoutIds: XCTestCase {
         XCTAssertFalse(BatchEventBuilder.isValidNumericIdString("+12345"))
         XCTAssertFalse(BatchEventBuilder.isValidNumericIdString("12 45"))
         XCTAssertFalse(BatchEventBuilder.isValidNumericIdString("1e10"))
-        // Non-ASCII digit forms (e.g. Arabic-Indic) are NOT valid per spec.
+        // Non-ASCII digit forms (e.g. Arabic-Indic) are NOT valid.
         XCTAssertFalse(BatchEventBuilder.isValidNumericIdString("\u{0660}\u{0661}"))
     }
 
-    // MARK: - normalizeDecisionIds — valid IDs pass through unchanged (SC-003)
+    // MARK: - normalizeDecisionIds — happy path
 
     func testNormalize_validCampaignAndVariation_passThrough() {
-        // The dominant production case: both IDs already valid. No change.
         let (campaign, variation) = BatchEventBuilder.normalizeDecisionIds(
             rawCampaignId: "555",
             rawVariationId: "777",
@@ -66,8 +87,6 @@ class BatchEventBuilderTests_HoldoutIds: XCTestCase {
     }
 
     func testNormalize_validCampaignAndNilVariation_variationStaysNil() {
-        // Valid campaign, no variation supplied (e.g. holdout with no variation
-        // assigned). variation_id must be JSON null, not empty string.
         let (campaign, variation) = BatchEventBuilder.normalizeDecisionIds(
             rawCampaignId: "555",
             rawVariationId: nil,
@@ -76,30 +95,47 @@ class BatchEventBuilderTests_HoldoutIds: XCTestCase {
         XCTAssertNil(variation)
     }
 
-    // MARK: - normalizeDecisionIds — campaign_id (FR-001/FR-002) applied uniformly
+    // MARK: - normalizeDecisionIds — campaign_id
 
     func testNormalize_emptyCampaignFallsBackToExperimentId() {
+        // Canonical holdout case: layerId defaults to "".
         let (campaign, _) = BatchEventBuilder.normalizeDecisionIds(
             rawCampaignId: "",
             rawVariationId: "777",
             experimentId: "exp_42")
         XCTAssertEqual(campaign, "exp_42")
     }
 
-    func testNormalize_whitespaceCampaignFallsBackToExperimentId() {
+    func testNormalize_whitespaceOnlyCampaignFallsBackToExperimentId() {
         let (campaign, _) = BatchEventBuilder.normalizeDecisionIds(
             rawCampaignId: "   ",
             rawVariationId: "777",
             experimentId: "exp_42")
         XCTAssertEqual(campaign, "exp_42")
     }
 
-    func testNormalize_nonNumericCampaignFallsBackToExperimentId() {
+    func testNormalize_opaqueCampaignPassesThroughUnchanged() {
+        let (campaign, _) = BatchEventBuilder.normalizeDecisionIds(
+            rawCampaignId: "default-12345",
+            rawVariationId: "777",
+            experimentId: "exp_42")
+        XCTAssertEqual(campaign, "default-12345")
+    }
+
+    func testNormalize_nonNumericCampaignPassesThroughUnchanged() {
         let (campaign, _) = BatchEventBuilder.normalizeDecisionIds(
             rawCampaignId: "not_a_number",
             rawVariationId: "777",
             experimentId: "exp_42")
-        XCTAssertEqual(campaign, "exp_42")
+        XCTAssertEqual(campaign, "not_a_number")
+    }
+
+    func testNormalize_opaquePrefixedLayerIdPassesThroughUnchanged() {
+        let (campaign, _) = BatchEventBuilder.normalizeDecisionIds(
+            rawCampaignId: "layer_abc123",
+            rawVariationId: "777",
+            experimentId: "exp_42")
+        XCTAssertEqual(campaign, "layer_abc123")
     }
 
     func testNormalize_validNumericCampaignKept() {
@@ -111,16 +147,15 @@ class BatchEventBuilderTests_HoldoutIds: XCTestCase {
     }
 
     func testNormalize_invalidExperimentIdStillPassedThrough() {
-        // FR-006: never drop the event; pass invalid experiment_id through if
-        // that is all we have for the fallback.
+        // Never drop the event — pass the invalid fallback through.
         let (campaign, _) = BatchEventBuilder.normalizeDecisionIds(
             rawCampaignId: "",
             rawVariationId: "777",
             experimentId: "")
         XCTAssertEqual(campaign, "")
     }
 
-    // MARK: - normalizeDecisionIds — variation_id (FR-003/FR-004) applied uniformly
+    // MARK: - normalizeDecisionIds — variation_id
 
     func testNormalize_emptyVariationBecomesNil() {
         let (_, variation) = BatchEventBuilder.normalizeDecisionIds(
@@ -180,14 +215,13 @@ class BatchEventBuilderTests_HoldoutIds: XCTestCase {
         let data = try JSONEncoder().encode(decision)
         let json = try JSONSerialization.jsonObject(with: data, options: .allowFragments) as! [String: Any]
 
-        // JSONSerialization surfaces JSON `null` as `NSNull`. Verify the key is
-        // PRESENT with an explicit null (not omitted by encodeIfPresent).
+        // JSONSerialization surfaces JSON `null` as `NSNull`. The key must be
+        // present with explicit null (not omitted).
         XCTAssertTrue(json.keys.contains("variation_id"),
                       "variation_id must be present in the JSON payload")
         XCTAssertTrue(json["variation_id"] is NSNull,
                       "variation_id must serialize as explicit JSON null when nil")
 
-        // Sanity-check other fields are unchanged.
         XCTAssertEqual(json["campaign_id"] as? String, "12345")
         XCTAssertEqual(json["experiment_id"] as? String, "67890")
     }
@@ -212,25 +246,23 @@ class BatchEventBuilderTests_HoldoutIds: XCTestCase {
         XCTAssertFalse(json["variation_id"] is NSNull)
     }
 
-    // MARK: - Impression event entity_id (FR-009 / SC-006)
+    // MARK: - Impression event entity_id
 
     /// End-to-end: a holdout impression event has an empty source `layerId`,
     /// so both `decisions[0].campaign_id` AND `events[0].entity_id` must fall
     /// back to the holdout's `experiment_id`, and they must be byte-equal.
     func testImpressionEvent_holdout_entityIdMirrorsNormalizedCampaignId() throws {
         let datafile = OTUtils.loadJSONDatafile("api_datafile")!
         let eventDispatcher = MockEventDispatcher()
-        var optimizely: OptimizelyClient! = OptimizelyClient(sdkKey: "fssdk_12813_entity_id",
+        var optimizely: OptimizelyClient! = OptimizelyClient(sdkKey: "holdout_entity_id_test",
                                                              eventDispatcher: eventDispatcher)
         defer {
             optimizely?.close()
             optimizely = nil
         }
         try optimizely.start(datafile: datafile)
 
-        // Holdout struct defaults layerId to "" (see Holdout.swift) — the
-        // canonical case this fix targets. id is the fallback used by both
-        // campaign_id (FR-002) and entity_id (FR-009).
+        // Holdout defaults layerId to "" — the canonical case this targets.
         let holdoutJSON: [String: Any] = [
             "status": "Running",
             "id": "holdout_4444444",
@@ -265,14 +297,28 @@ class BatchEventBuilderTests_HoldoutIds: XCTestCase {
         let campaignId = decision["campaign_id"] as? String
         let entityId = dispatchEvent["entity_id"] as? String
 
-        // FR-002: campaign_id falls back to experiment_id (holdout.id).
         XCTAssertEqual(campaignId, holdout.id,
                        "campaign_id must fall back to holdout.id when layerId is empty")
-        // FR-009: entity_id falls back the same way.
         XCTAssertEqual(entityId, holdout.id,
                        "entity_id must fall back to holdout.id when layerId is empty")
-        // SC-006: the two fields share source + fallback; they must never diverge.
+        // entity_id mirrors normalized campaign_id; they must never diverge.
         XCTAssertEqual(campaignId, entityId,
                        "campaign_id and entity_id must hold the same normalized value")
     }
+
+    /// Opaque non-numeric source passes through normalization unchanged.
+    /// entity_id is assigned from the normalized campaign_id in
+    /// createImpressionEvent, so proving campaign_id passthrough also proves
+    /// entity_id passthrough on the wire.
+    func testNormalize_opaqueLayerIdPassesThroughForBothCampaignAndEntity() {
+        let opaqueLayerId = "layer_abc123"
+        let (campaignId, variationId) = BatchEventBuilder.normalizeDecisionIds(
+            rawCampaignId: opaqueLayerId,
+            rawVariationId: "777",
+            experimentId: "exp_999")
+        XCTAssertEqual(campaignId, opaqueLayerId,
+                       "opaque layerId must pass through to campaign_id unchanged")
+        XCTAssertEqual(variationId, "777",
+                       "valid numeric variation_id must pass through unchanged")
+    }
 }
diff --git a/Tests/OptimizelyTests-Common/DecisionListenerTest_Holdouts.swift b/Tests/OptimizelyTests-Common/DecisionListenerTest_Holdouts.swift
@@ -1,5 +1,5 @@
 //
-// Copyright 2022, Optimizely, Inc. and contributors 
+// Copyright 2022, 2026, Optimizely, Inc. and contributors
 // 
 // Licensed under the Apache License, Version 2.0 (the "License");  
 // you may not use this file except in compliance with the License.
@@ -39,20 +39,23 @@ class DecisionListenerTests_Holdouts: XCTestCase {
     let kVariableValueDouble = 4.2
     let kVariableValueBool = true
     
+    // variation id and trafficAllocation entityId must be numeric strings
+    // (variation_id contract). Holdout id uses numeric too for fixture
+    // uniformity, though any non-empty string would satisfy campaign_id.
     var sampleHoldout: [String: Any] {
         return [
             "status": "Running",
-            "id": "id_holdout",
+            "id": "9999900001",
             "key": "key_holdout",
             "layerId": "10420273888",
             "trafficAllocation": [
-                ["entityId": "id_holdout_variation", "endOfRange": 500]
+                ["entityId": "9999900002", "endOfRange": 500]
             ],
             "audienceIds": [],
             "variations": [
                 [
                     "variables": [],
-                    "id": "id_holdout_variation",
+                    "id": "9999900002",
                     "key": "key_holdout_variation"
                 ]
             ]
diff --git a/Tests/OptimizelyTests-Common/DecisionServiceTests_LocalHoldouts.swift b/Tests/OptimizelyTests-Common/DecisionServiceTests_LocalHoldouts.swift
@@ -30,19 +30,21 @@ class DecisionServiceTests_LocalHoldouts: XCTestCase {
     let experimentRuleId = "10390977673"  // From decide_datafile
     let deliveryRuleId = "3332020515"     // From decide_datafile rollout
 
+    // variation id and trafficAllocation entityId must be numeric strings.
+    // Holdout id uses numeric too for fixture uniformity. Keys are arbitrary.
     var sampleHoldout: [String: Any] {
         return [
             "status": "Running",
-            "id": "holdout_test_id",
+            "id": "9999900010",
             "key": "holdout_test_key",
             "trafficAllocation": [
-                ["entityId": "holdout_variation_id", "endOfRange": 5000] // 50% traffic
+                ["entityId": "9999900020", "endOfRange": 5000] // 50% traffic
             ],
             "audienceIds": [],
             "variations": [
                 [
                     "variables": [],
-                    "id": "holdout_variation_id",
+                    "id": "9999900020",
                     "key": "holdout_variation_key",
                     "featureEnabled": false
                 ]
diff --git a/Tests/OptimizelyTests-Common/OptimizelyUserContextTests_Decide_Holdouts.swift b/Tests/OptimizelyTests-Common/OptimizelyUserContextTests_Decide_Holdouts.swift
diff --git a/Tests/OptimizelyTests-Common/OptimizelyUserContextTests_Decide_With_Holdouts_Reasons.swift b/Tests/OptimizelyTests-Common/OptimizelyUserContextTests_Decide_With_Holdouts_Reasons.swift
