[AI-FSSDK] [FSSDK-12813] normalize campaign_id, variation_id, and entity_id on decision events#642
Merged
Merged
Conversation
Apply the campaign_id / variation_id normalization uniformly across every decision type (experiment, feature test, rollout, holdout) instead of scoping it to holdouts. The contract is the same for every dispatched event, so removing the isHoldout branch simplifies the code and provides forward-compatibility for future decision types. For well-formed datafiles this is a no-op for non-holdout decisions (their layerId values are already valid numeric strings); the normalization continues to fire on holdout events and acts as a safety net for any future malformed input. - Drop isHoldout parameter from normalizeDecisionIds - Rename test names from holdout_* to apply uniformly - 17 unit tests pass; 40 regression tests in sibling suites pass
jaeopt
changed the title
…009) events[].entity_id on impression events shares the same source (experiment.layerId) and the same fallback contract as decisions[].campaign_id, so it has the same bug for holdouts. Use the normalized campaign_id value for entity_id too, guaranteeing the two fields never diverge on the wire. - BatchEventBuilder.swift: DispatchEvent constructor now passes campaignId (normalized) instead of rawCampaignId - BatchEventBuilderTests_HoldoutIds.swift: add end-to-end test asserting both campaign_id and entity_id fall back to holdout.id when layerId is empty, and that the two fields are byte-equal (SC-006) - 18/18 unit tests pass; 40/40 regression tests pass
jaeopt
changed the title
Two impression tests still asserted the pre-normalization variation_id: - testDecide_sendImpressionForNullVariation expected "" for a rollout with no variation; new code emits JSON null. - Holdouts testDecide_sendImpression expected "id_holdout_variation", which is non-numeric and now normalizes to null. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Coverage is 93.849% — ai/jaeopt/FSSDK-12813-holdout-event into master. No base build found for |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Normalizes the three ID fields on outgoing decision impression events so the event-processing pipeline always receives pipeline-valid values:
decisions[].campaign_iddecisions[].variation_idevents[].entity_idApplies uniformly across decision types (experiment, feature test, rollout, holdout) — there is no per-type branching in the normalization path.
Per the updated spec (FSSDK-12813):
campaign_idandentity_idMUST be non-empty decimal-digit strings; when invalid (empty, whitespace, null, or non-numeric), the SDK falls back to the decision'sexperiment_id. Because both fields share the same source (experiment.layerId) and the same fallback, they MUST always hold the same normalized value on the wire (SC-006).variation_idMUST be a non-empty decimal-digit string OR explicit JSONnull; when invalid (empty, whitespace, non-numeric), the SDK emitsnull.entity_idfrom a different upstream source (the conversion event's own datafileid) and are out of scope (FR-010).Why generalize beyond holdouts?
The bug's loudest manifestation today is on holdout impression events (holdouts may legitimately lack
layerId, so the invalid-campaign_id/entity_idpath fires on every holdout event). Audited the SDK fleet and confirmed validcampaign_id/entity_idvalues are always non-empty numeric strings for non-holdout decisions in well-formed datafiles — so applying the same normalization uniformly is a true no-op for those paths today (verified by sibling regression suites, see below). The uniform rule simplifies the code and provides forward-compatibility for any future decision type or future datafile edge case.Changes
Sources/Implementation/Events/BatchEventBuilder.swiftcreateImpressionEventroutescampaign_id/variation_idthroughnormalizeDecisionIds(...)for every decision (noisHoldoutbranch).DispatchEvent.entityIDnow receives the normalizedcampaignIdinstead ofrawCampaignId, soevents[].entity_idanddecisions[].campaign_idshare the same fallback (FR-009 / SC-006).isValidNumericIdString(_:)helper restricted to ASCII[0-9].Sources/Data Model/DispatchEvents/BatchEvent.swift—Decision.variationIDisString?with a customencode(to:)that emits explicit JSONnull(notencodeIfPresent) when nil, so the on-the-wire JSON carries"variation_id": nullfor the no-variation case.Tests/OptimizelyTests-Common/BatchEventBuilderTests_HoldoutIds.swift— 18 tests covering numeric-string validation, uniformcampaign_id/variation_idnormalization, FR-006 passthrough, JSONnullserialization, and an end-to-end impression event assertion thatcampaign_idandentity_idboth fall back to the holdout'sidand are byte-equal. (Filename retained to keepproject.pbxprojreferences stable; contents cover all decision types and all three ID fields.)OptimizelySwiftSDK.xcodeproj/project.pbxproj— registers the test file withOptimizelyTests-Common-iOSandOptimizelyTests-Common-tvOS.Verification
BatchEventBuilderTests_HoldoutIds)BatchEventBuilderTests_Events,BatchEventBuilderTests_Attributes,BatchEventBuilderTests_EventTags,BatchEventBuilderTests_Region,DecisionServiceTests_Holdouts) — confirms the generalized normalization is a true no-op for existing valid non-holdout events (SC-003) and that existing impression-event assertions (entity_id == campaign_id) continue to hold.xcodebuild -workspace OptimizelySwiftSDK.xcworkspace -scheme OptimizelySwiftSDK-iOS -destination 'platform=iOS Simulator,name=iPhone 17' testJira Ticket
FSSDK-12813