From f920f96479dea4a7d567068fb290ccd24fb39253 Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Fri, 12 Jun 2026 13:18:18 -0700 Subject: [PATCH 1/3] [AI-FSSDK] [FSSDK-12760] add localHoldouts to datafile for backward compatibility --- pkg/config/datafileprojectconfig/config.go | 18 +- .../datafileprojectconfig/config_test.go | 128 +++++ .../entities/entities.go | 11 +- .../datafileprojectconfig/mappers/holdout.go | 82 +++- .../mappers/holdout_test.go | 446 +++++++++++++----- pkg/entities/experiment.go | 13 +- 6 files changed, 535 insertions(+), 163 deletions(-) diff --git a/pkg/config/datafileprojectconfig/config.go b/pkg/config/datafileprojectconfig/config.go index 53186340..a4d5f54e 100644 --- a/pkg/config/datafileprojectconfig/config.go +++ b/pkg/config/datafileprojectconfig/config.go @@ -62,9 +62,9 @@ type DatafileProjectConfig struct { flagVariationsMap map[string][]entities.Variation holdouts []entities.Holdout holdoutIDMap map[string]entities.Holdout - // ruleHoldoutsMap maps rule IDs to local holdouts targeting those rules + // ruleHoldoutsMap maps rule IDs to local holdouts (from the `localHoldouts` datafile section) targeting those rules. ruleHoldoutsMap map[string][]entities.Holdout - // globalHoldouts holds only global holdouts (IncludedRules == nil) + // globalHoldouts holds running entries from the `holdouts` datafile section. globalHoldouts []entities.Holdout } @@ -287,14 +287,14 @@ func (c DatafileProjectConfig) GetRegion() string { return c.region } -// GetGlobalHoldouts returns all global holdouts (those with IncludedRules == nil). -// These are evaluated at flag level, before any per-rule evaluation. +// GetGlobalHoldouts returns all running global holdouts from the `holdouts` datafile section. +// Evaluated at flag level before any per-rule evaluation; section membership is the sole signal for scope. func (c DatafileProjectConfig) GetGlobalHoldouts() []entities.Holdout { return c.globalHoldouts } -// GetHoldoutsForRule returns all local holdouts that target the given rule ID. -// These are evaluated per-rule, after forced decisions, before audience/traffic checks. +// GetHoldoutsForRule returns running local holdouts (from the `localHoldouts` datafile section) +// that target the given rule ID. Evaluated per-rule, after forced decisions, before audience/traffic checks. func (c DatafileProjectConfig) GetHoldoutsForRule(ruleID string) []entities.Holdout { if holdouts, exists := c.ruleHoldoutsMap[ruleID]; exists { return holdouts @@ -348,7 +348,11 @@ func NewDatafileProjectConfig(jsonDatafile []byte, logger logging.OptimizelyLogP audienceMap, audienceSegmentList := mappers.MapAudiences(append(datafile.TypedAudiences, datafile.Audiences...)) flagVariationsMap := mappers.MapFlagVariations(featureMap) - holdouts, holdoutIDMap, globalHoldouts, ruleHoldoutsMap := mappers.MapHoldouts(datafile.Holdouts) + // Two top-level holdout sections drive scope (FSSDK-12760): + // `holdouts` → global holdouts (every entry; `includedRules` stripped) + // `localHoldouts` → local holdouts (must carry `includedRules`; otherwise logged and skipped) + // Older datafiles without `localHoldouts` continue to work unchanged. + holdouts, holdoutIDMap, globalHoldouts, ruleHoldoutsMap := mappers.MapHoldouts(datafile.Holdouts, datafile.LocalHoldouts, logger) attributeKeyMap := make(map[string]entities.Attribute) attributeIDToKeyMap := make(map[string]string) diff --git a/pkg/config/datafileprojectconfig/config_test.go b/pkg/config/datafileprojectconfig/config_test.go index b69fe106..50dde946 100644 --- a/pkg/config/datafileprojectconfig/config_test.go +++ b/pkg/config/datafileprojectconfig/config_test.go @@ -767,3 +767,131 @@ func TestGetHoldoutsForRuleWithNoHoldouts(t *testing.T) { assert.Len(t, actual, 0) assert.Equal(t, []entities.Holdout{}, actual) } + +// --------------------------------------------------------------------------- +// FSSDK-12760 — `localHoldouts` JSON section end-to-end parsing +// --------------------------------------------------------------------------- + +// TestNewDatafileProjectConfigPartitionsHoldoutsByDatafileSection exercises the +// full datafile → entity pipeline to confirm the `holdouts` and `localHoldouts` +// top-level keys are partitioned strictly by section membership. +func TestNewDatafileProjectConfigPartitionsHoldoutsByDatafileSection(t *testing.T) { + datafile := []byte(`{ + "version": "4", + "accountId": "123", + "projectId": "456", + "revision": "1", + "holdouts": [ + { + "id": "g1", + "key": "global_holdout", + "status": "Running", + "audienceIds": [], + "variations": [{"id": "vg1", "key": "v"}], + "trafficAllocation": [{"entityId": "vg1", "endOfRange": 10000}] + } + ], + "localHoldouts": [ + { + "id": "l1", + "key": "local_holdout_rule_x", + "status": "Running", + "audienceIds": [], + "includedRules": ["rule_x"], + "variations": [{"id": "vl1", "key": "v"}], + "trafficAllocation": [{"entityId": "vl1", "endOfRange": 5000}] + } + ] + }`) + + config, err := NewDatafileProjectConfig(datafile, logging.GetLogger("", "DatafileProjectConfig")) + assert.NoError(t, err) + assert.NotNil(t, config) + + // Global section → global holdouts; never registers against any rule. + globals := config.GetGlobalHoldouts() + assert.Len(t, globals, 1) + assert.Equal(t, "g1", globals[0].ID) + assert.True(t, globals[0].IsGlobal()) + + // Local section → per-rule map; never appears in global list. + ruleX := config.GetHoldoutsForRule("rule_x") + assert.Len(t, ruleX, 1) + assert.Equal(t, "l1", ruleX[0].ID) + assert.False(t, ruleX[0].IsGlobal()) + + // Global id must not have leaked into the per-rule map. + for _, h := range ruleX { + assert.NotEqual(t, "g1", h.ID) + } +} + +// TestNewDatafileProjectConfigBackwardCompatNoLocalHoldoutsSection verifies that +// older datafiles emitted before FSSDK-12760 (no `localHoldouts` key) are parsed +// exactly like before — every entry in `holdouts` is global, no errors. +func TestNewDatafileProjectConfigBackwardCompatNoLocalHoldoutsSection(t *testing.T) { + datafile := []byte(`{ + "version": "4", + "accountId": "123", + "projectId": "456", + "revision": "1", + "holdouts": [ + { + "id": "old1", + "key": "old_global_holdout", + "status": "Running", + "audienceIds": [], + "variations": [{"id": "v1", "key": "v"}], + "trafficAllocation": [{"entityId": "v1", "endOfRange": 10000}] + } + ] + }`) + + config, err := NewDatafileProjectConfig(datafile, logging.GetLogger("", "DatafileProjectConfig")) + assert.NoError(t, err) + assert.NotNil(t, config) + + globals := config.GetGlobalHoldouts() + assert.Len(t, globals, 1) + assert.Equal(t, "old1", globals[0].ID) + assert.True(t, globals[0].IsGlobal()) + + // No localHoldouts section means no per-rule entries. + assert.Empty(t, config.GetHoldoutsForRule("any_rule")) +} + +// TestNewDatafileProjectConfigGlobalSectionStripsIncludedRules verifies that any +// `includedRules` field accidentally present on an entry in the `holdouts` (global) +// section is stripped during parsing — section membership is the sole signal for scope. +func TestNewDatafileProjectConfigGlobalSectionStripsIncludedRules(t *testing.T) { + datafile := []byte(`{ + "version": "4", + "accountId": "123", + "projectId": "456", + "revision": "1", + "holdouts": [ + { + "id": "g_with_rules", + "key": "global_with_stray_rules", + "status": "Running", + "audienceIds": [], + "includedRules": ["rule_should_be_ignored"], + "variations": [{"id": "v1", "key": "v"}], + "trafficAllocation": [{"entityId": "v1", "endOfRange": 10000}] + } + ] + }`) + + config, err := NewDatafileProjectConfig(datafile, logging.GetLogger("", "DatafileProjectConfig")) + assert.NoError(t, err) + assert.NotNil(t, config) + + // Still classified as global; IncludedRules was stripped on the entity. + globals := config.GetGlobalHoldouts() + assert.Len(t, globals, 1) + assert.True(t, globals[0].IsGlobal()) + assert.Nil(t, globals[0].IncludedRules) + + // The stray rule id must NOT show up in any per-rule lookup. + assert.Empty(t, config.GetHoldoutsForRule("rule_should_be_ignored")) +} diff --git a/pkg/config/datafileprojectconfig/entities/entities.go b/pkg/config/datafileprojectconfig/entities/entities.go index 581fb1b6..57d2e46a 100644 --- a/pkg/config/datafileprojectconfig/entities/entities.go +++ b/pkg/config/datafileprojectconfig/entities/entities.go @@ -114,7 +114,10 @@ type Rollout struct { Experiments []Experiment `json:"experiments"` } -// Holdout represents a holdout from the Optimizely datafile +// Holdout represents a holdout from the Optimizely datafile. +// Scope (global vs local) is set by the datafile section the entry came from +// (`holdouts` vs `localHoldouts`); `IncludedRules` is stripped on `holdouts` entries +// and required on `localHoldouts` entries. type Holdout struct { ID string `json:"id"` Key string `json:"key"` @@ -123,9 +126,8 @@ type Holdout struct { AudienceConditions interface{} `json:"audienceConditions"` Variations []Variation `json:"variations"` TrafficAllocation []TrafficAllocation `json:"trafficAllocation"` - // IncludedRules is optional. nil = global holdout (applies to all rules across all flags). - // Non-nil array = local holdout (applies only to the specified rule IDs). - // An empty non-nil array means a local holdout that targets no rules. + // IncludedRules carries per-rule targeting for local holdouts. Required on + // `localHoldouts` entries; ignored/stripped on `holdouts` entries at parse time. IncludedRules *[]string `json:"includedRules,omitempty"` } @@ -146,6 +148,7 @@ type Datafile struct { Events []Event `json:"events"` Rollouts []Rollout `json:"rollouts"` Holdouts []Holdout `json:"holdouts,omitempty"` + LocalHoldouts []Holdout `json:"localHoldouts,omitempty"` Integrations []Integration `json:"integrations"` TypedAudiences []Audience `json:"typedAudiences"` Variables []string `json:"variables"` diff --git a/pkg/config/datafileprojectconfig/mappers/holdout.go b/pkg/config/datafileprojectconfig/mappers/holdout.go index be804db6..3f8116f3 100644 --- a/pkg/config/datafileprojectconfig/mappers/holdout.go +++ b/pkg/config/datafileprojectconfig/mappers/holdout.go @@ -18,14 +18,27 @@ package mappers import ( + "fmt" + datafileEntities "github.com/optimizely/go-sdk/v2/pkg/config/datafileprojectconfig/entities" "github.com/optimizely/go-sdk/v2/pkg/entities" + "github.com/optimizely/go-sdk/v2/pkg/logging" ) -// MapHoldouts maps the raw datafile holdout entities to SDK Holdout entities. -// Global holdouts (IncludedRules == nil) are returned in globalHoldouts for flag-level evaluation. -// Local holdouts (IncludedRules != nil) are indexed by rule ID in ruleHoldoutsMap. -func MapHoldouts(holdouts []datafileEntities.Holdout) ( +// MapHoldouts maps the two top-level datafile holdout sections to SDK Holdout entities. +// +// Section membership is the sole signal for scope (FSSDK-12760): +// - `holdouts` entries are global; any `includedRules` field on them is stripped. +// - `localHoldouts` entries are local and MUST carry a non-nil `includedRules` list; +// entries missing it are logged via `logger` and excluded (no fallback to global). +// +// Returns the combined entity list, an id->entity map, the global-only list, and a +// per-rule map for local holdouts. +func MapHoldouts( + globalHoldoutsRaw []datafileEntities.Holdout, + localHoldoutsRaw []datafileEntities.Holdout, + logger logging.OptimizelyLogProducer, +) ( holdoutList []entities.Holdout, holdoutIDMap map[string]entities.Holdout, globalHoldouts []entities.Holdout, @@ -36,29 +49,68 @@ func MapHoldouts(holdouts []datafileEntities.Holdout) ( globalHoldouts = []entities.Holdout{} ruleHoldoutsMap = make(map[string][]entities.Holdout) - for _, holdout := range holdouts { - // Only process running holdouts + // Process global holdouts: drop any `IncludedRules` so section membership alone + // determines scope, even if the datafile incorrectly includes one. + for _, holdout := range globalHoldoutsRaw { if holdout.Status != string(entities.HoldoutStatusRunning) { continue } - mappedHoldout := mapHoldout(holdout) + sanitized := holdout + sanitized.IncludedRules = nil + + mappedHoldout := mapHoldout(sanitized) holdoutList = append(holdoutList, mappedHoldout) - holdoutIDMap[holdout.ID] = mappedHoldout - - if mappedHoldout.IsGlobal() { - globalHoldouts = append(globalHoldouts, mappedHoldout) - } else { - // Local holdout: applies only to the specified rule IDs - for _, ruleID := range *mappedHoldout.IncludedRules { - ruleHoldoutsMap[ruleID] = append(ruleHoldoutsMap[ruleID], mappedHoldout) + holdoutIDMap[mappedHoldout.ID] = mappedHoldout + globalHoldouts = append(globalHoldouts, mappedHoldout) + } + + // Process local holdouts: `IncludedRules` is REQUIRED. Entries missing it (nil + // pointer) are invalid per spec — log and skip, never promote to global. + for _, holdout := range localHoldoutsRaw { + if holdout.Status != string(entities.HoldoutStatusRunning) { + continue + } + + if holdout.IncludedRules == nil { + if logger != nil { + logger.Error( + fmt.Sprintf( + "Local holdout %q is missing required \"includedRules\" field and will be excluded from evaluation.", + holdoutLabel(holdout), + ), + nil, + ) } + continue + } + + mappedHoldout := mapHoldout(holdout) + holdoutList = append(holdoutList, mappedHoldout) + holdoutIDMap[mappedHoldout.ID] = mappedHoldout + + // Register the local holdout for each rule it targets. An empty IncludedRules + // slice is valid (matches no rules) but is not promoted to global. + for _, ruleID := range *mappedHoldout.IncludedRules { + ruleHoldoutsMap[ruleID] = append(ruleHoldoutsMap[ruleID], mappedHoldout) } } return holdoutList, holdoutIDMap, globalHoldouts, ruleHoldoutsMap } +// holdoutLabel returns a stable, user-facing label for an invalid local holdout +// log message. Prefers the holdout key (human-readable) and falls back to the id. +func holdoutLabel(h datafileEntities.Holdout) string { + if h.Key != "" { + return h.Key + } + if h.ID != "" { + return h.ID + } + return "" +} + func mapHoldout(datafileHoldout datafileEntities.Holdout) entities.Holdout { var audienceConditionTree *entities.TreeNode var err error diff --git a/pkg/config/datafileprojectconfig/mappers/holdout_test.go b/pkg/config/datafileprojectconfig/mappers/holdout_test.go index 4d153533..36552c82 100644 --- a/pkg/config/datafileprojectconfig/mappers/holdout_test.go +++ b/pkg/config/datafileprojectconfig/mappers/holdout_test.go @@ -17,26 +17,57 @@ package mappers import ( + "strings" + "sync" "testing" datafileEntities "github.com/optimizely/go-sdk/v2/pkg/config/datafileprojectconfig/entities" "github.com/optimizely/go-sdk/v2/pkg/entities" + "github.com/optimizely/go-sdk/v2/pkg/logging" "github.com/stretchr/testify/assert" ) -func TestMapHoldoutsEmpty(t *testing.T) { - rawHoldouts := []datafileEntities.Holdout{} +// captureLogger is a minimal OptimizelyLogProducer used by the tests to capture +// log messages without depending on the real logger plumbing. +type captureLogger struct { + mu sync.Mutex + errors []string + warnings []string +} + +func (c *captureLogger) Debug(_ string) {} +func (c *captureLogger) Info(_ string) {} +func (c *captureLogger) Warning(message string) { c.mu.Lock(); defer c.mu.Unlock(); c.warnings = append(c.warnings, message) } +func (c *captureLogger) Error(message string, _ interface{}) { + c.mu.Lock() + defer c.mu.Unlock() + c.errors = append(c.errors, message) +} + +// errorMessages returns a snapshot of captured error messages. +func (c *captureLogger) errorMessages() []string { + c.mu.Lock() + defer c.mu.Unlock() + out := make([]string, len(c.errors)) + copy(out, c.errors) + return out +} + +// Verify captureLogger satisfies the interface at compile time. +var _ logging.OptimizelyLogProducer = (*captureLogger)(nil) - holdoutList, holdoutIDMap, globalHoldouts, _ := MapHoldouts(rawHoldouts) +func TestMapHoldoutsEmpty(t *testing.T) { + holdoutList, holdoutIDMap, globalHoldouts, ruleHoldoutsMap := MapHoldouts(nil, nil, &captureLogger{}) assert.Empty(t, holdoutList) assert.Empty(t, holdoutIDMap) assert.Empty(t, globalHoldouts) + assert.Empty(t, ruleHoldoutsMap) } -func TestMapHoldoutsAppliestoAllFlags(t *testing.T) { - // Running holdouts with nil IncludedRules are global and returned in globalHoldouts - rawHoldouts := []datafileEntities.Holdout{ +func TestMapHoldoutsGlobalSectionAppliesToAllFlags(t *testing.T) { + // Running entries in the `holdouts` (global) section are returned in globalHoldouts. + rawGlobal := []datafileEntities.Holdout{ { ID: "holdout_1", Key: "running_holdout", @@ -50,21 +81,19 @@ func TestMapHoldoutsAppliestoAllFlags(t *testing.T) { }, } - holdoutList, holdoutIDMap, globalHoldouts, _ := MapHoldouts(rawHoldouts) + holdoutList, holdoutIDMap, globalHoldouts, _ := MapHoldouts(rawGlobal, nil, &captureLogger{}) assert.Len(t, holdoutList, 1) assert.Len(t, holdoutIDMap, 1) assert.Equal(t, "holdout_1", holdoutList[0].ID) assert.Equal(t, "running_holdout", holdoutList[0].Key) - - // Global holdout should appear in globalHoldouts assert.Len(t, globalHoldouts, 1) assert.Equal(t, "running_holdout", globalHoldouts[0].Key) + assert.True(t, globalHoldouts[0].IsGlobal()) } -func TestMapHoldoutsNotRunning(t *testing.T) { - // Holdout with non-Running status should be filtered out - rawHoldouts := []datafileEntities.Holdout{ +func TestMapHoldoutsNotRunningInGlobalSection(t *testing.T) { + rawGlobal := []datafileEntities.Holdout{ { ID: "holdout_1", Key: "paused_holdout", @@ -75,16 +104,15 @@ func TestMapHoldoutsNotRunning(t *testing.T) { }, } - holdoutList, holdoutIDMap, globalHoldouts, _ := MapHoldouts(rawHoldouts) + holdoutList, holdoutIDMap, globalHoldouts, _ := MapHoldouts(rawGlobal, nil, &captureLogger{}) assert.Empty(t, holdoutList) assert.Empty(t, holdoutIDMap) assert.Empty(t, globalHoldouts) } -func TestMapHoldoutsMultipleHoldouts(t *testing.T) { - // Multiple running global holdouts all appear in globalHoldouts - rawHoldouts := []datafileEntities.Holdout{ +func TestMapHoldoutsMultipleGlobalHoldouts(t *testing.T) { + rawGlobal := []datafileEntities.Holdout{ { ID: "holdout_1", Key: "holdout_1", @@ -109,19 +137,17 @@ func TestMapHoldoutsMultipleHoldouts(t *testing.T) { }, } - holdoutList, holdoutIDMap, globalHoldouts, _ := MapHoldouts(rawHoldouts) + holdoutList, holdoutIDMap, globalHoldouts, _ := MapHoldouts(rawGlobal, nil, &captureLogger{}) assert.Len(t, holdoutList, 2) assert.Len(t, holdoutIDMap, 2) - - // Both global holdouts appear in globalHoldouts assert.Len(t, globalHoldouts, 2) assert.Equal(t, "holdout_1", globalHoldouts[0].Key) assert.Equal(t, "holdout_2", globalHoldouts[1].Key) } func TestMapHoldoutsWithAudienceConditions(t *testing.T) { - rawHoldouts := []datafileEntities.Holdout{ + rawGlobal := []datafileEntities.Holdout{ { ID: "holdout_1", Key: "holdout_with_audience", @@ -137,16 +163,15 @@ func TestMapHoldoutsWithAudienceConditions(t *testing.T) { }, } - holdoutList, _, _, _ := MapHoldouts(rawHoldouts) + holdoutList, _, _, _ := MapHoldouts(rawGlobal, nil, &captureLogger{}) - // Verify audience conditions are mapped assert.Len(t, holdoutList, 1) assert.Equal(t, []string{"audience_1", "audience_2"}, holdoutList[0].AudienceIds) assert.NotNil(t, holdoutList[0].AudienceConditionTree) } func TestMapHoldoutsVariationsMapping(t *testing.T) { - rawHoldouts := []datafileEntities.Holdout{ + rawGlobal := []datafileEntities.Holdout{ { ID: "holdout_1", Key: "holdout_variations", @@ -173,15 +198,13 @@ func TestMapHoldoutsVariationsMapping(t *testing.T) { }, } - holdoutList, _, _, _ := MapHoldouts(rawHoldouts) + holdoutList, _, _, _ := MapHoldouts(rawGlobal, nil, &captureLogger{}) - // Verify variations are mapped correctly assert.Len(t, holdoutList, 1) assert.Len(t, holdoutList[0].Variations, 2) assert.Contains(t, holdoutList[0].Variations, "var_1") assert.Contains(t, holdoutList[0].Variations, "var_2") - // Verify traffic allocation assert.Len(t, holdoutList[0].TrafficAllocation, 2) assert.Equal(t, "var_1", holdoutList[0].TrafficAllocation[0].EntityID) assert.Equal(t, 5000, holdoutList[0].TrafficAllocation[0].EndOfRange) @@ -189,42 +212,14 @@ func TestMapHoldoutsVariationsMapping(t *testing.T) { assert.Equal(t, 10000, holdoutList[0].TrafficAllocation[1].EndOfRange) } -// Level 1 tests for local holdout support (FSSDK-12369) +// --------------------------------------------------------------------------- +// FSSDK-12760 — backward-compatible localHoldouts section tests +// --------------------------------------------------------------------------- -func TestMapHoldoutsIsGlobalNilIncludedRules(t *testing.T) { - // A holdout with no IncludedRules field (nil pointer) should be global - rawHoldouts := []datafileEntities.Holdout{ - { - ID: "holdout_global", - Key: "global_holdout", - Status: "Running", - IncludedRules: nil, // nil = global - Variations: []datafileEntities.Variation{ - {ID: "var_1", Key: "variation_1"}, - }, - TrafficAllocation: []datafileEntities.TrafficAllocation{ - {EntityID: "var_1", EndOfRange: 10000}, - }, - }, - } - - holdoutList, _, globalHoldouts, ruleHoldoutsMap := MapHoldouts(rawHoldouts) - - assert.Len(t, holdoutList, 1) - // nil IncludedRules should be classified as global - assert.True(t, holdoutList[0].IsGlobal()) - assert.Nil(t, holdoutList[0].IncludedRules) - // Global holdout should appear in globalHoldouts - assert.Len(t, globalHoldouts, 1) - assert.Equal(t, "global_holdout", globalHoldouts[0].Key) - // ruleHoldoutsMap should be empty for a global holdout - assert.Empty(t, ruleHoldoutsMap) -} - -func TestMapHoldoutsLocalHoldoutWithIncludedRules(t *testing.T) { - // A holdout with IncludedRules pointing to specific rule IDs should be local +func TestMapHoldoutsLocalSectionRegistersPerRule(t *testing.T) { + // Local section entries must be registered under each rule in IncludedRules. includedRules := []string{"rule_id_1", "rule_id_2"} - rawHoldouts := []datafileEntities.Holdout{ + rawLocal := []datafileEntities.Holdout{ { ID: "holdout_local", Key: "local_holdout", @@ -239,32 +234,29 @@ func TestMapHoldoutsLocalHoldoutWithIncludedRules(t *testing.T) { }, } - holdoutList, _, globalHoldouts, ruleHoldoutsMap := MapHoldouts(rawHoldouts) + holdoutList, _, globalHoldouts, ruleHoldoutsMap := MapHoldouts(nil, rawLocal, &captureLogger{}) assert.Len(t, holdoutList, 1) - // Non-nil IncludedRules should be classified as local (not global) assert.False(t, holdoutList[0].IsGlobal()) assert.NotNil(t, holdoutList[0].IncludedRules) - // Local holdout must NOT appear in globalHoldouts + // Local section entry must NOT appear in globalHoldouts. assert.Empty(t, globalHoldouts) - // Local holdout should appear in ruleHoldoutsMap for each rule it targets assert.Contains(t, ruleHoldoutsMap, "rule_id_1") assert.Contains(t, ruleHoldoutsMap, "rule_id_2") assert.Len(t, ruleHoldoutsMap["rule_id_1"], 1) assert.Len(t, ruleHoldoutsMap["rule_id_2"], 1) - assert.Equal(t, "local_holdout", ruleHoldoutsMap["rule_id_1"][0].Key) - assert.Equal(t, "local_holdout", ruleHoldoutsMap["rule_id_2"][0].Key) } -func TestMapHoldoutsEmptyIncludedRulesIsLocalNotGlobal(t *testing.T) { - // An empty (non-nil) IncludedRules slice is local (targets no rules), NOT global +func TestMapHoldoutsLocalSectionEmptyIncludedRulesIsValidButTargetsNoRules(t *testing.T) { + // An empty (non-nil) IncludedRules slice is valid: the entity is created + // and tracked, but does not register against any rule. Not invalid, not global. emptyRules := []string{} - rawHoldouts := []datafileEntities.Holdout{ + rawLocal := []datafileEntities.Holdout{ { ID: "holdout_empty_local", Key: "empty_local_holdout", Status: "Running", - IncludedRules: &emptyRules, // non-nil, but empty + IncludedRules: &emptyRules, Variations: []datafileEntities.Variation{ {ID: "var_1", Key: "variation_1"}, }, @@ -274,113 +266,209 @@ func TestMapHoldoutsEmptyIncludedRulesIsLocalNotGlobal(t *testing.T) { }, } - holdoutList, _, globalHoldouts, ruleHoldoutsMap := MapHoldouts(rawHoldouts) + logger := &captureLogger{} + holdoutList, holdoutIDMap, globalHoldouts, ruleHoldoutsMap := MapHoldouts(nil, rawLocal, logger) assert.Len(t, holdoutList, 1) - // Empty non-nil IncludedRules should be local (not global) assert.False(t, holdoutList[0].IsGlobal()) - // Empty local holdout must NOT appear in globalHoldouts assert.Empty(t, globalHoldouts) - // ruleHoldoutsMap should also be empty (no rules to target) assert.Empty(t, ruleHoldoutsMap) + // Entity is tracked in the id map (not invalid) + assert.Contains(t, holdoutIDMap, "holdout_empty_local") + // And no error was logged. + assert.Empty(t, logger.errorMessages()) +} + +func TestMapHoldoutsLocalSectionMissingIncludedRulesIsInvalid(t *testing.T) { + // Per FSSDK-12760: local entries WITHOUT IncludedRules are invalid — logged and skipped. + rawLocal := []datafileEntities.Holdout{ + { + ID: "holdout_invalid", + Key: "invalid_local", + Status: "Running", + // IncludedRules: nil — invalid in the localHoldouts section. + Variations: []datafileEntities.Variation{ + {ID: "var_1", Key: "variation_1"}, + }, + TrafficAllocation: []datafileEntities.TrafficAllocation{ + {EntityID: "var_1", EndOfRange: 10000}, + }, + }, + } + + logger := &captureLogger{} + holdoutList, holdoutIDMap, globalHoldouts, ruleHoldoutsMap := MapHoldouts(nil, rawLocal, logger) + + // Invalid entry must be excluded across all outputs — NEVER promoted to global. + assert.Empty(t, holdoutList) + assert.Empty(t, holdoutIDMap) + assert.Empty(t, globalHoldouts) + assert.Empty(t, ruleHoldoutsMap) + + // Error must be logged and reference both the holdout key and "includedRules". + errors := logger.errorMessages() + assert.Len(t, errors, 1) + assert.Contains(t, errors[0], "invalid_local") + assert.Contains(t, errors[0], "includedRules") } -func TestMapHoldoutsMixedGlobalAndLocal(t *testing.T) { - // Mix of global and local holdouts - includedRules := []string{"rule_1"} - rawHoldouts := []datafileEntities.Holdout{ +func TestMapHoldoutsLocalSectionInvalidEntryDoesNotAffectValidEntries(t *testing.T) { + // Mixing one invalid local entry with a valid one: invalid is skipped, valid is processed. + validRules := []string{"rule_x"} + rawLocal := []datafileEntities.Holdout{ + { + ID: "holdout_invalid", + Key: "invalid_local", + Status: "Running", + // nil IncludedRules — invalid. + }, { - ID: "holdout_global", - Key: "global_holdout", + ID: "holdout_valid", + Key: "valid_local", Status: "Running", - IncludedRules: nil, + IncludedRules: &validRules, Variations: []datafileEntities.Variation{ - {ID: "var_g", Key: "var_global"}, + {ID: "var_v", Key: "v"}, }, TrafficAllocation: []datafileEntities.TrafficAllocation{ - {EntityID: "var_g", EndOfRange: 10000}, + {EntityID: "var_v", EndOfRange: 10000}, }, }, + } + + logger := &captureLogger{} + holdoutList, holdoutIDMap, _, ruleHoldoutsMap := MapHoldouts(nil, rawLocal, logger) + + assert.Len(t, holdoutList, 1) + assert.Equal(t, "holdout_valid", holdoutList[0].ID) + assert.Contains(t, holdoutIDMap, "holdout_valid") + assert.NotContains(t, holdoutIDMap, "holdout_invalid") + assert.Contains(t, ruleHoldoutsMap, "rule_x") + assert.Len(t, logger.errorMessages(), 1) +} + +func TestMapHoldoutsGlobalSectionStripsIncludedRules(t *testing.T) { + // If a global-section entry accidentally has IncludedRules, it must be stripped + // at parse time and the entity must still be classified as global. The stray + // rule id must NOT appear in the per-rule map. + strayRules := []string{"rule_should_be_ignored"} + rawGlobal := []datafileEntities.Holdout{ { - ID: "holdout_local", - Key: "local_holdout", + ID: "holdout_stray", + Key: "stray_global", Status: "Running", - IncludedRules: &includedRules, + IncludedRules: &strayRules, Variations: []datafileEntities.Variation{ - {ID: "var_l", Key: "var_local"}, + {ID: "var_1", Key: "variation_1"}, }, TrafficAllocation: []datafileEntities.TrafficAllocation{ - {EntityID: "var_l", EndOfRange: 10000}, + {EntityID: "var_1", EndOfRange: 10000}, }, }, } - holdoutList, _, globalHoldouts, ruleHoldoutsMap := MapHoldouts(rawHoldouts) + holdoutList, _, globalHoldouts, ruleHoldoutsMap := MapHoldouts(rawGlobal, nil, &captureLogger{}) - assert.Len(t, holdoutList, 2) + assert.Len(t, holdoutList, 1) + assert.True(t, holdoutList[0].IsGlobal(), "entry from global section must always be global") + assert.Nil(t, holdoutList[0].IncludedRules, "IncludedRules must be stripped on global entries") - // Only global holdout in globalHoldouts assert.Len(t, globalHoldouts, 1) - assert.Equal(t, "global_holdout", globalHoldouts[0].Key) + assert.True(t, globalHoldouts[0].IsGlobal()) - // Local holdout in ruleHoldoutsMap - assert.Contains(t, ruleHoldoutsMap, "rule_1") - assert.Len(t, ruleHoldoutsMap["rule_1"], 1) - assert.Equal(t, "local_holdout", ruleHoldoutsMap["rule_1"][0].Key) + // The stray rule id must not have leaked into the per-rule map. + assert.NotContains(t, ruleHoldoutsMap, "rule_should_be_ignored") + assert.Empty(t, ruleHoldoutsMap) } -func TestMapHoldoutsLocalHoldoutCrossRuleTargeting(t *testing.T) { - // A single local holdout can target rules from multiple flags - includedRules := []string{"rule_a", "rule_b", "rule_c"} - rawHoldouts := []datafileEntities.Holdout{ +func TestMapHoldoutsBothSectionsPartitionCorrectly(t *testing.T) { + // When both sections are present, scope is enforced strictly by section + // membership — entries never cross over. + localRulesA := []string{"rule_a"} + localRulesB := []string{"rule_b"} + rawGlobal := []datafileEntities.Holdout{ { - ID: "holdout_cross", - Key: "cross_rule_holdout", + ID: "g1", + Key: "global_1", + Status: "Running", + Variations: []datafileEntities.Variation{ + {ID: "var_g1", Key: "var_g1"}, + }, + TrafficAllocation: []datafileEntities.TrafficAllocation{ + {EntityID: "var_g1", EndOfRange: 10000}, + }, + }, + { + ID: "g2", + Key: "global_2", + Status: "Running", + Variations: []datafileEntities.Variation{ + {ID: "var_g2", Key: "var_g2"}, + }, + TrafficAllocation: []datafileEntities.TrafficAllocation{ + {EntityID: "var_g2", EndOfRange: 10000}, + }, + }, + } + rawLocal := []datafileEntities.Holdout{ + { + ID: "l1", + Key: "local_1", Status: "Running", - IncludedRules: &includedRules, + IncludedRules: &localRulesA, Variations: []datafileEntities.Variation{ - {ID: "var_1", Key: "variation_1"}, + {ID: "var_l1", Key: "var_l1"}, }, TrafficAllocation: []datafileEntities.TrafficAllocation{ - {EntityID: "var_1", EndOfRange: 10000}, + {EntityID: "var_l1", EndOfRange: 10000}, + }, + }, + { + ID: "l2", + Key: "local_2", + Status: "Running", + IncludedRules: &localRulesB, + Variations: []datafileEntities.Variation{ + {ID: "var_l2", Key: "var_l2"}, + }, + TrafficAllocation: []datafileEntities.TrafficAllocation{ + {EntityID: "var_l2", EndOfRange: 10000}, }, }, } - _, _, _, ruleHoldoutsMap := MapHoldouts(rawHoldouts) + holdoutList, holdoutIDMap, globalHoldouts, ruleHoldoutsMap := MapHoldouts(rawGlobal, rawLocal, &captureLogger{}) - // Each rule should have the holdout mapped - assert.Contains(t, ruleHoldoutsMap, "rule_a") - assert.Contains(t, ruleHoldoutsMap, "rule_b") - assert.Contains(t, ruleHoldoutsMap, "rule_c") - assert.Len(t, ruleHoldoutsMap["rule_a"], 1) - assert.Len(t, ruleHoldoutsMap["rule_b"], 1) - assert.Len(t, ruleHoldoutsMap["rule_c"], 1) -} + assert.Len(t, holdoutList, 4) + assert.Len(t, holdoutIDMap, 4) -func TestMapHoldoutsIsGlobalProperty(t *testing.T) { - // Verify IsGlobal() works correctly for both global and local holdouts - nilRules := (*[]string)(nil) - emptyRules := []string{} - ruleIDs := []string{"rule_1"} + globalIDs := map[string]bool{} + for _, h := range globalHoldouts { + globalIDs[h.ID] = true + } + assert.Equal(t, map[string]bool{"g1": true, "g2": true}, globalIDs) - globalHoldout := entities.Holdout{IncludedRules: nilRules} - localHoldoutEmpty := entities.Holdout{IncludedRules: &emptyRules} - localHoldoutWithRules := entities.Holdout{IncludedRules: &ruleIDs} + assert.Len(t, ruleHoldoutsMap["rule_a"], 1) + assert.Equal(t, "l1", ruleHoldoutsMap["rule_a"][0].ID) + assert.Len(t, ruleHoldoutsMap["rule_b"], 1) + assert.Equal(t, "l2", ruleHoldoutsMap["rule_b"][0].ID) - assert.True(t, globalHoldout.IsGlobal(), "nil IncludedRules should be global") - assert.False(t, localHoldoutEmpty.IsGlobal(), "empty non-nil IncludedRules should NOT be global") - assert.False(t, localHoldoutWithRules.IsGlobal(), "non-nil IncludedRules with rules should NOT be global") + // Global ids must NOT show up in any per-rule entry. + for _, ruleHoldouts := range ruleHoldoutsMap { + for _, h := range ruleHoldouts { + assert.NotContains(t, []string{"g1", "g2"}, h.ID) + } + } } -func TestMapHoldoutsBackwardCompatibilityOldDatafile(t *testing.T) { - // Old datafiles without IncludedRules field should be treated as global - rawHoldouts := []datafileEntities.Holdout{ +func TestMapHoldoutsBackwardCompatNoLocalSection(t *testing.T) { + // Old datafiles without a `localHoldouts` section continue to work unchanged: + // every entry in `holdouts` is global, no errors, no log noise. + rawGlobal := []datafileEntities.Holdout{ { ID: "holdout_old", Key: "old_global_holdout", Status: "Running", - // No IncludedRules field — simulates old datafile format Variations: []datafileEntities.Variation{ {ID: "var_1", Key: "variation_1"}, }, @@ -390,12 +478,108 @@ func TestMapHoldoutsBackwardCompatibilityOldDatafile(t *testing.T) { }, } - holdoutList, _, globalHoldouts, ruleHoldoutsMap := MapHoldouts(rawHoldouts) + logger := &captureLogger{} + // localHoldoutsRaw is nil — simulates a pre-FSSDK-12760 datafile. + holdoutList, _, globalHoldouts, ruleHoldoutsMap := MapHoldouts(rawGlobal, nil, logger) assert.Len(t, holdoutList, 1) - // Old datafile holdout with no IncludedRules should default to global assert.True(t, holdoutList[0].IsGlobal()) assert.Len(t, globalHoldouts, 1) assert.Equal(t, "old_global_holdout", globalHoldouts[0].Key) assert.Empty(t, ruleHoldoutsMap) + assert.Empty(t, logger.errorMessages()) +} + +func TestMapHoldoutsLocalSectionNonRunningExcluded(t *testing.T) { + // Non-Running entries in `localHoldouts` are filtered out before validation — + // they must not be evaluated and must not produce error logs. + includedRules := []string{"rule_x"} + rawLocal := []datafileEntities.Holdout{ + { + ID: "holdout_draft", + Key: "draft_local", + Status: "Draft", + IncludedRules: &includedRules, + }, + } + + logger := &captureLogger{} + holdoutList, holdoutIDMap, _, ruleHoldoutsMap := MapHoldouts(nil, rawLocal, logger) + + assert.Empty(t, holdoutList) + assert.Empty(t, holdoutIDMap) + assert.Empty(t, ruleHoldoutsMap) + assert.Empty(t, logger.errorMessages()) +} + +func TestMapHoldoutsLocalSectionMissingIncludedRulesUsesIDWhenKeyAbsent(t *testing.T) { + // When an invalid local holdout has no Key, the log message must still + // identify the entry — fall back to the ID. + rawLocal := []datafileEntities.Holdout{ + { + ID: "id_only_holdout", + Status: "Running", + // Key empty, IncludedRules nil — invalid. + }, + } + + logger := &captureLogger{} + MapHoldouts(nil, rawLocal, logger) + + errors := logger.errorMessages() + assert.Len(t, errors, 1) + assert.Contains(t, errors[0], "id_only_holdout") +} + +func TestMapHoldoutsNilLoggerWithInvalidLocalEntryDoesNotPanic(t *testing.T) { + // Defensive: callers may pass a nil logger; the mapper must not panic. + rawLocal := []datafileEntities.Holdout{ + { + ID: "holdout_invalid", + Key: "invalid_local", + Status: "Running", + // IncludedRules: nil — invalid; would normally produce a log. + }, + } + + assert.NotPanics(t, func() { + holdoutList, _, _, _ := MapHoldouts(nil, rawLocal, nil) + assert.Empty(t, holdoutList) + }) +} + +func TestMapHoldoutsIsGlobalProperty(t *testing.T) { + // Verify the entity-level IsGlobal property is unchanged by the section split. + nilRules := (*[]string)(nil) + emptyRules := []string{} + ruleIDs := []string{"rule_1"} + + globalHoldout := entities.Holdout{IncludedRules: nilRules} + localHoldoutEmpty := entities.Holdout{IncludedRules: &emptyRules} + localHoldoutWithRules := entities.Holdout{IncludedRules: &ruleIDs} + + assert.True(t, globalHoldout.IsGlobal(), "nil IncludedRules should be global") + assert.False(t, localHoldoutEmpty.IsGlobal(), "empty non-nil IncludedRules should NOT be global") + assert.False(t, localHoldoutWithRules.IsGlobal(), "non-nil IncludedRules with rules should NOT be global") +} + +func TestMapHoldoutsErrorMessageWording(t *testing.T) { + // The error message wording is part of the API surface for operators — ensure + // it remains user-actionable across refactors. + rawLocal := []datafileEntities.Holdout{ + { + ID: "h_x", + Key: "x_local", + Status: "Running", + }, + } + + logger := &captureLogger{} + MapHoldouts(nil, rawLocal, logger) + + errors := logger.errorMessages() + assert.Len(t, errors, 1) + // Message must say what is missing and what the consequence is. + assert.True(t, strings.Contains(errors[0], "missing"), "message should mention 'missing'") + assert.True(t, strings.Contains(errors[0], "excluded"), "message should mention 'excluded'") } diff --git a/pkg/entities/experiment.go b/pkg/entities/experiment.go index 426e6b21..2798b02f 100644 --- a/pkg/entities/experiment.go +++ b/pkg/entities/experiment.go @@ -90,15 +90,16 @@ type Holdout struct { Variations map[string]Variation // keyed by variation ID TrafficAllocation []Range AudienceConditionTree *TreeNode - // IncludedRules is nil for global holdouts (applies to all rules across all flags). - // Non-nil for local holdouts (applies only to the specified rule IDs). - // An empty non-nil slice means a local holdout that targets no rules. + // IncludedRules carries per-rule targeting for local holdouts. The datafile + // section determines scope; `DatafileProjectConfig` strips `IncludedRules` on + // entries from the `holdouts` section, so nil here is equivalent to global. IncludedRules *[]string } -// IsGlobal returns true if this holdout is a global holdout (applies to all rules across all flags). -// A holdout is global when IncludedRules is nil. -// An empty non-nil IncludedRules slice is NOT global — it is a local holdout that targets no rules. +// IsGlobal returns true if this holdout is global (applies to all rules across all flags). +// Scope is set by the datafile section (`holdouts` vs `localHoldouts`); since +// `IncludedRules` is stripped on `holdouts` entries at parse time, nil here matches +// section membership. func (h *Holdout) IsGlobal() bool { return h.IncludedRules == nil } From d10400c8bb8ce9ead7367923bd14e46074f5511c Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Mon, 15 Jun 2026 11:59:22 -0700 Subject: [PATCH 2/3] [FSSDK-12760] Fix duplicate ID collision, log level, and brittle test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Log a warning when the same holdout ID appears in both datafile sections (the later entry overwrites the earlier one in the ID map) - Downgrade missing-includedRules log from Error to Warning — this is a data quality issue, not a runtime failure - Replace brittle TestMapHoldoutsErrorMessageWording (asserted on specific words) with TestMapHoldoutsDuplicateIDsAcrossSectionsLogsWarning Co-Authored-By: Claude Opus 4.6 --- .../datafileprojectconfig/mappers/holdout.go | 19 +++- .../mappers/holdout_test.go | 92 +++++++++++-------- 2 files changed, 73 insertions(+), 38 deletions(-) diff --git a/pkg/config/datafileprojectconfig/mappers/holdout.go b/pkg/config/datafileprojectconfig/mappers/holdout.go index 3f8116f3..c0050ff9 100644 --- a/pkg/config/datafileprojectconfig/mappers/holdout.go +++ b/pkg/config/datafileprojectconfig/mappers/holdout.go @@ -61,6 +61,14 @@ func MapHoldouts( mappedHoldout := mapHoldout(sanitized) holdoutList = append(holdoutList, mappedHoldout) + if _, exists := holdoutIDMap[mappedHoldout.ID]; exists && logger != nil { + logger.Warning( + fmt.Sprintf( + "Duplicate holdout ID %q encountered across datafile sections; later entry overwrites earlier one.", + mappedHoldout.ID, + ), + ) + } holdoutIDMap[mappedHoldout.ID] = mappedHoldout globalHoldouts = append(globalHoldouts, mappedHoldout) } @@ -74,12 +82,11 @@ func MapHoldouts( if holdout.IncludedRules == nil { if logger != nil { - logger.Error( + logger.Warning( fmt.Sprintf( "Local holdout %q is missing required \"includedRules\" field and will be excluded from evaluation.", holdoutLabel(holdout), ), - nil, ) } continue @@ -87,6 +94,14 @@ func MapHoldouts( mappedHoldout := mapHoldout(holdout) holdoutList = append(holdoutList, mappedHoldout) + if _, exists := holdoutIDMap[mappedHoldout.ID]; exists && logger != nil { + logger.Warning( + fmt.Sprintf( + "Duplicate holdout ID %q encountered across datafile sections; later entry overwrites earlier one.", + mappedHoldout.ID, + ), + ) + } holdoutIDMap[mappedHoldout.ID] = mappedHoldout // Register the local holdout for each rule it targets. An empty IncludedRules diff --git a/pkg/config/datafileprojectconfig/mappers/holdout_test.go b/pkg/config/datafileprojectconfig/mappers/holdout_test.go index 36552c82..a44b0125 100644 --- a/pkg/config/datafileprojectconfig/mappers/holdout_test.go +++ b/pkg/config/datafileprojectconfig/mappers/holdout_test.go @@ -17,7 +17,6 @@ package mappers import ( - "strings" "sync" "testing" @@ -31,25 +30,24 @@ import ( // log messages without depending on the real logger plumbing. type captureLogger struct { mu sync.Mutex - errors []string warnings []string } -func (c *captureLogger) Debug(_ string) {} -func (c *captureLogger) Info(_ string) {} -func (c *captureLogger) Warning(message string) { c.mu.Lock(); defer c.mu.Unlock(); c.warnings = append(c.warnings, message) } -func (c *captureLogger) Error(message string, _ interface{}) { +func (c *captureLogger) Debug(_ string) {} +func (c *captureLogger) Info(_ string) {} +func (c *captureLogger) Error(_ string, _ interface{}) {} +func (c *captureLogger) Warning(message string) { c.mu.Lock() defer c.mu.Unlock() - c.errors = append(c.errors, message) + c.warnings = append(c.warnings, message) } -// errorMessages returns a snapshot of captured error messages. -func (c *captureLogger) errorMessages() []string { +// warningMessages returns a snapshot of captured warning messages. +func (c *captureLogger) warningMessages() []string { c.mu.Lock() defer c.mu.Unlock() - out := make([]string, len(c.errors)) - copy(out, c.errors) + out := make([]string, len(c.warnings)) + copy(out, c.warnings) return out } @@ -275,8 +273,8 @@ func TestMapHoldoutsLocalSectionEmptyIncludedRulesIsValidButTargetsNoRules(t *te assert.Empty(t, ruleHoldoutsMap) // Entity is tracked in the id map (not invalid) assert.Contains(t, holdoutIDMap, "holdout_empty_local") - // And no error was logged. - assert.Empty(t, logger.errorMessages()) + // And no warning was logged. + assert.Empty(t, logger.warningMessages()) } func TestMapHoldoutsLocalSectionMissingIncludedRulesIsInvalid(t *testing.T) { @@ -305,11 +303,11 @@ func TestMapHoldoutsLocalSectionMissingIncludedRulesIsInvalid(t *testing.T) { assert.Empty(t, globalHoldouts) assert.Empty(t, ruleHoldoutsMap) - // Error must be logged and reference both the holdout key and "includedRules". - errors := logger.errorMessages() - assert.Len(t, errors, 1) - assert.Contains(t, errors[0], "invalid_local") - assert.Contains(t, errors[0], "includedRules") + // Warning must be logged and reference both the holdout key and "includedRules". + warnings := logger.warningMessages() + assert.Len(t, warnings, 1) + assert.Contains(t, warnings[0], "invalid_local") + assert.Contains(t, warnings[0], "includedRules") } func TestMapHoldoutsLocalSectionInvalidEntryDoesNotAffectValidEntries(t *testing.T) { @@ -344,7 +342,7 @@ func TestMapHoldoutsLocalSectionInvalidEntryDoesNotAffectValidEntries(t *testing assert.Contains(t, holdoutIDMap, "holdout_valid") assert.NotContains(t, holdoutIDMap, "holdout_invalid") assert.Contains(t, ruleHoldoutsMap, "rule_x") - assert.Len(t, logger.errorMessages(), 1) + assert.Len(t, logger.warningMessages(), 1) } func TestMapHoldoutsGlobalSectionStripsIncludedRules(t *testing.T) { @@ -487,7 +485,7 @@ func TestMapHoldoutsBackwardCompatNoLocalSection(t *testing.T) { assert.Len(t, globalHoldouts, 1) assert.Equal(t, "old_global_holdout", globalHoldouts[0].Key) assert.Empty(t, ruleHoldoutsMap) - assert.Empty(t, logger.errorMessages()) + assert.Empty(t, logger.warningMessages()) } func TestMapHoldoutsLocalSectionNonRunningExcluded(t *testing.T) { @@ -509,7 +507,7 @@ func TestMapHoldoutsLocalSectionNonRunningExcluded(t *testing.T) { assert.Empty(t, holdoutList) assert.Empty(t, holdoutIDMap) assert.Empty(t, ruleHoldoutsMap) - assert.Empty(t, logger.errorMessages()) + assert.Empty(t, logger.warningMessages()) } func TestMapHoldoutsLocalSectionMissingIncludedRulesUsesIDWhenKeyAbsent(t *testing.T) { @@ -526,9 +524,9 @@ func TestMapHoldoutsLocalSectionMissingIncludedRulesUsesIDWhenKeyAbsent(t *testi logger := &captureLogger{} MapHoldouts(nil, rawLocal, logger) - errors := logger.errorMessages() - assert.Len(t, errors, 1) - assert.Contains(t, errors[0], "id_only_holdout") + warnings := logger.warningMessages() + assert.Len(t, warnings, 1) + assert.Contains(t, warnings[0], "id_only_holdout") } func TestMapHoldoutsNilLoggerWithInvalidLocalEntryDoesNotPanic(t *testing.T) { @@ -563,23 +561,45 @@ func TestMapHoldoutsIsGlobalProperty(t *testing.T) { assert.False(t, localHoldoutWithRules.IsGlobal(), "non-nil IncludedRules with rules should NOT be global") } -func TestMapHoldoutsErrorMessageWording(t *testing.T) { - // The error message wording is part of the API surface for operators — ensure - // it remains user-actionable across refactors. - rawLocal := []datafileEntities.Holdout{ +func TestMapHoldoutsDuplicateIDsAcrossSectionsLogsWarning(t *testing.T) { + // If the same holdout ID appears in both sections, a warning must be logged + // and the later (local) entry overwrites the earlier (global) one in the ID map. + localRules := []string{"rule_x"} + rawGlobal := []datafileEntities.Holdout{ { - ID: "h_x", - Key: "x_local", + ID: "dup_id", + Key: "global_version", Status: "Running", + Variations: []datafileEntities.Variation{ + {ID: "var_g", Key: "var_g"}, + }, + TrafficAllocation: []datafileEntities.TrafficAllocation{ + {EntityID: "var_g", EndOfRange: 10000}, + }, + }, + } + rawLocal := []datafileEntities.Holdout{ + { + ID: "dup_id", + Key: "local_version", + Status: "Running", + IncludedRules: &localRules, + Variations: []datafileEntities.Variation{ + {ID: "var_l", Key: "var_l"}, + }, + TrafficAllocation: []datafileEntities.TrafficAllocation{ + {EntityID: "var_l", EndOfRange: 10000}, + }, }, } logger := &captureLogger{} - MapHoldouts(nil, rawLocal, logger) + _, holdoutIDMap, _, _ := MapHoldouts(rawGlobal, rawLocal, logger) + + warnings := logger.warningMessages() + assert.Len(t, warnings, 1) + assert.Contains(t, warnings[0], "dup_id") - errors := logger.errorMessages() - assert.Len(t, errors, 1) - // Message must say what is missing and what the consequence is. - assert.True(t, strings.Contains(errors[0], "missing"), "message should mention 'missing'") - assert.True(t, strings.Contains(errors[0], "excluded"), "message should mention 'excluded'") + // The local entry should have overwritten the global one in the ID map. + assert.Equal(t, "local_version", holdoutIDMap["dup_id"].Key) } From 25053b2161d73541ddfb14b3bc1324ae9b831da3 Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Mon, 15 Jun 2026 15:21:33 -0700 Subject: [PATCH 3/3] [FSSDK-12760] Empty commit to re-trigger CI