From 9e39ee25adcba3820f3e19b8f68c0ee0b6abf7c6 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Thu, 14 May 2026 15:06:09 -0700 Subject: [PATCH] [AI-FSSDK] [FSSDK-12369] Add local holdouts support to Go SDK --- pkg/cmab/service_test.go | 10 + pkg/config/datafileprojectconfig/config.go | 35 ++- .../entities/entities.go | 4 + .../datafileprojectconfig/mappers/holdout.go | 29 ++- .../mappers/holdout_test.go | 240 +++++++++++++++++- pkg/config/interface.go | 6 + pkg/decision/composite_feature_service.go | 5 +- .../evaluator/audience_evaluator_test.go | 10 + pkg/decision/feature_experiment_service.go | 15 +- .../feature_experiment_service_test.go | 4 +- pkg/decision/helpers_test.go | 10 + pkg/decision/holdout_service.go | 72 ++++++ pkg/decision/holdout_service_test.go | 207 +++++++++++++++ pkg/decision/rollout_service.go | 27 ++ pkg/entities/experiment.go | 11 + 15 files changed, 665 insertions(+), 20 deletions(-) diff --git a/pkg/cmab/service_test.go b/pkg/cmab/service_test.go index 4675d773..8ea9f90b 100644 --- a/pkg/cmab/service_test.go +++ b/pkg/cmab/service_test.go @@ -235,6 +235,16 @@ func (m *MockProjectConfig) GetHoldoutsForFlag(featureKey string) []entities.Hol return args.Get(0).([]entities.Holdout) } +func (m *MockProjectConfig) GetGlobalHoldouts() []entities.Holdout { + args := m.Called() + return args.Get(0).([]entities.Holdout) +} + +func (m *MockProjectConfig) GetHoldoutsForRule(ruleID string) []entities.Holdout { + args := m.Called(ruleID) + return args.Get(0).([]entities.Holdout) +} + type CmabServiceTestSuite struct { suite.Suite mockClient *MockCmabClient diff --git a/pkg/config/datafileprojectconfig/config.go b/pkg/config/datafileprojectconfig/config.go index eeeb341a..4f07b2ef 100644 --- a/pkg/config/datafileprojectconfig/config.go +++ b/pkg/config/datafileprojectconfig/config.go @@ -63,6 +63,10 @@ type DatafileProjectConfig struct { holdouts []entities.Holdout holdoutIDMap map[string]entities.Holdout flagHoldoutsMap map[string][]entities.Holdout + // ruleHoldoutsMap maps rule IDs to local holdouts targeting those rules + ruleHoldoutsMap map[string][]entities.Holdout + // globalHoldouts holds only global holdouts (IncludedRules == nil) + globalHoldouts []entities.Holdout } // GetDatafile returns a string representation of the environment's datafile @@ -284,7 +288,9 @@ func (c DatafileProjectConfig) GetRegion() string { return c.region } -// GetHoldoutsForFlag returns all holdouts applicable to the given feature flag +// GetHoldoutsForFlag returns all global holdouts applicable to the given feature flag. +// Only global holdouts (those with IncludedRules == nil) are returned here. +// Local holdouts are retrieved per rule via GetHoldoutsForRule. func (c DatafileProjectConfig) GetHoldoutsForFlag(featureKey string) []entities.Holdout { if holdouts, exists := c.flagHoldoutsMap[featureKey]; exists { return holdouts @@ -292,6 +298,21 @@ func (c DatafileProjectConfig) GetHoldoutsForFlag(featureKey string) []entities. return []entities.Holdout{} } +// GetGlobalHoldouts returns all global holdouts (those with IncludedRules == nil). +// These are evaluated at flag level, before any per-rule evaluation. +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. +func (c DatafileProjectConfig) GetHoldoutsForRule(ruleID string) []entities.Holdout { + if holdouts, exists := c.ruleHoldoutsMap[ruleID]; exists { + return holdouts + } + return []entities.Holdout{} +} + // NewDatafileProjectConfig initializes a new datafile from a json byte array using the default JSON datafile parser func NewDatafileProjectConfig(jsonDatafile []byte, logger logging.OptimizelyLogProducer) (*DatafileProjectConfig, error) { datafile, err := Parse(jsonDatafile) @@ -338,7 +359,15 @@ func NewDatafileProjectConfig(jsonDatafile []byte, logger logging.OptimizelyLogP audienceMap, audienceSegmentList := mappers.MapAudiences(append(datafile.TypedAudiences, datafile.Audiences...)) flagVariationsMap := mappers.MapFlagVariations(featureMap) - holdouts, holdoutIDMap, flagHoldoutsMap := mappers.MapHoldouts(datafile.Holdouts, featureMap) + holdouts, holdoutIDMap, flagHoldoutsMap, ruleHoldoutsMap := mappers.MapHoldouts(datafile.Holdouts, featureMap) + + // Collect global holdouts (IncludedRules == nil) for direct access + globalHoldouts := []entities.Holdout{} + for i := range holdouts { + if holdouts[i].IsGlobal() { + globalHoldouts = append(globalHoldouts, holdouts[i]) + } + } attributeKeyMap := make(map[string]entities.Attribute) attributeIDToKeyMap := make(map[string]string) @@ -384,6 +413,8 @@ func NewDatafileProjectConfig(jsonDatafile []byte, logger logging.OptimizelyLogP holdouts: holdouts, holdoutIDMap: holdoutIDMap, flagHoldoutsMap: flagHoldoutsMap, + ruleHoldoutsMap: ruleHoldoutsMap, + globalHoldouts: globalHoldouts, } logger.Info("Datafile is valid.") diff --git a/pkg/config/datafileprojectconfig/entities/entities.go b/pkg/config/datafileprojectconfig/entities/entities.go index 0e68966a..581fb1b6 100644 --- a/pkg/config/datafileprojectconfig/entities/entities.go +++ b/pkg/config/datafileprojectconfig/entities/entities.go @@ -123,6 +123,10 @@ 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 *[]string `json:"includedRules,omitempty"` } // Integration represents a integration from the Optimizely datafile diff --git a/pkg/config/datafileprojectconfig/mappers/holdout.go b/pkg/config/datafileprojectconfig/mappers/holdout.go index 4a51d8a6..5f18eb11 100644 --- a/pkg/config/datafileprojectconfig/mappers/holdout.go +++ b/pkg/config/datafileprojectconfig/mappers/holdout.go @@ -22,18 +22,21 @@ import ( "github.com/optimizely/go-sdk/v2/pkg/entities" ) -// MapHoldouts maps the raw datafile holdout entities to SDK Holdout entities -// All running holdouts apply to all flags +// MapHoldouts maps the raw datafile holdout entities to SDK Holdout entities. +// Global holdouts (IncludedRules == nil) apply to all flags via flagHoldoutsMap. +// Local holdouts (IncludedRules != nil) are indexed by rule ID in ruleHoldoutsMap. func MapHoldouts(holdouts []datafileEntities.Holdout, featureMap map[string]entities.Feature) ( holdoutList []entities.Holdout, holdoutIDMap map[string]entities.Holdout, flagHoldoutsMap map[string][]entities.Holdout, + ruleHoldoutsMap map[string][]entities.Holdout, ) { holdoutList = []entities.Holdout{} holdoutIDMap = make(map[string]entities.Holdout) flagHoldoutsMap = make(map[string][]entities.Holdout) + ruleHoldoutsMap = make(map[string][]entities.Holdout) - runningHoldouts := []entities.Holdout{} + globalHoldouts := []entities.Holdout{} for _, holdout := range holdouts { // Only process running holdouts @@ -44,17 +47,26 @@ func MapHoldouts(holdouts []datafileEntities.Holdout, featureMap map[string]enti mappedHoldout := mapHoldout(holdout) holdoutList = append(holdoutList, mappedHoldout) holdoutIDMap[holdout.ID] = mappedHoldout - runningHoldouts = append(runningHoldouts, mappedHoldout) + + if mappedHoldout.IsGlobal() { + // Global holdout: applies to all rules across all flags + 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) + } + } } - // All running holdouts apply to all flags + // Global holdouts apply to all flags (flag-level evaluation) for _, feature := range featureMap { - if len(runningHoldouts) > 0 { - flagHoldoutsMap[feature.Key] = runningHoldouts + if len(globalHoldouts) > 0 { + flagHoldoutsMap[feature.Key] = globalHoldouts } } - return holdoutList, holdoutIDMap, flagHoldoutsMap + return holdoutList, holdoutIDMap, flagHoldoutsMap, ruleHoldoutsMap } func mapHoldout(datafileHoldout datafileEntities.Holdout) entities.Holdout { @@ -107,5 +119,6 @@ func mapHoldout(datafileHoldout datafileEntities.Holdout) entities.Holdout { Variations: variations, TrafficAllocation: trafficAllocation, AudienceConditionTree: audienceConditionTree, + IncludedRules: datafileHoldout.IncludedRules, } } diff --git a/pkg/config/datafileprojectconfig/mappers/holdout_test.go b/pkg/config/datafileprojectconfig/mappers/holdout_test.go index 36df6245..19d755fb 100644 --- a/pkg/config/datafileprojectconfig/mappers/holdout_test.go +++ b/pkg/config/datafileprojectconfig/mappers/holdout_test.go @@ -28,7 +28,7 @@ func TestMapHoldoutsEmpty(t *testing.T) { rawHoldouts := []datafileEntities.Holdout{} featureMap := map[string]entities.Feature{} - holdoutList, holdoutIDMap, flagHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) + holdoutList, holdoutIDMap, flagHoldoutsMap, _ := MapHoldouts(rawHoldouts, featureMap) assert.Empty(t, holdoutList) assert.Empty(t, holdoutIDMap) @@ -57,7 +57,7 @@ func TestMapHoldoutsAppliestoAllFlags(t *testing.T) { "feature_3": {ID: "feature_3", Key: "feature_3"}, } - holdoutList, holdoutIDMap, flagHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) + holdoutList, holdoutIDMap, flagHoldoutsMap, _ := MapHoldouts(rawHoldouts, featureMap) // Verify holdout list and ID map assert.Len(t, holdoutList, 1) @@ -93,7 +93,7 @@ func TestMapHoldoutsNotRunning(t *testing.T) { "feature_1": {ID: "feature_1", Key: "feature_1"}, } - holdoutList, holdoutIDMap, flagHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) + holdoutList, holdoutIDMap, flagHoldoutsMap, _ := MapHoldouts(rawHoldouts, featureMap) // Non-running holdouts should be filtered out assert.Empty(t, holdoutList) @@ -133,7 +133,7 @@ func TestMapHoldoutsMultipleHoldouts(t *testing.T) { "feature_2": {ID: "feature_2", Key: "feature_2"}, } - holdoutList, holdoutIDMap, flagHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) + holdoutList, holdoutIDMap, flagHoldoutsMap, _ := MapHoldouts(rawHoldouts, featureMap) // Verify both holdouts are in the list assert.Len(t, holdoutList, 2) @@ -174,7 +174,7 @@ func TestMapHoldoutsWithAudienceConditions(t *testing.T) { "feature_1": {ID: "feature_1", Key: "feature_1"}, } - holdoutList, _, _ := MapHoldouts(rawHoldouts, featureMap) + holdoutList, _, _, _ := MapHoldouts(rawHoldouts, featureMap) // Verify audience conditions are mapped assert.Len(t, holdoutList, 1) @@ -214,7 +214,7 @@ func TestMapHoldoutsVariationsMapping(t *testing.T) { "feature_1": {ID: "feature_1", Key: "feature_1"}, } - holdoutList, _, _ := MapHoldouts(rawHoldouts, featureMap) + holdoutList, _, _, _ := MapHoldouts(rawHoldouts, featureMap) // Verify variations are mapped correctly assert.Len(t, holdoutList, 1) @@ -229,3 +229,231 @@ func TestMapHoldoutsVariationsMapping(t *testing.T) { assert.Equal(t, "var_2", holdoutList[0].TrafficAllocation[1].EntityID) assert.Equal(t, 10000, holdoutList[0].TrafficAllocation[1].EndOfRange) } + +// Level 1 tests for local holdout support (FSSDK-12369) + +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}, + }, + }, + } + featureMap := map[string]entities.Feature{ + "feature_1": {ID: "feature_1", Key: "feature_1"}, + } + + holdoutList, _, flagHoldoutsMap, ruleHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) + + 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 flagHoldoutsMap for every feature + assert.Contains(t, flagHoldoutsMap, "feature_1") + assert.Len(t, flagHoldoutsMap["feature_1"], 1) + // 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 + includedRules := []string{"rule_id_1", "rule_id_2"} + rawHoldouts := []datafileEntities.Holdout{ + { + ID: "holdout_local", + Key: "local_holdout", + Status: "Running", + IncludedRules: &includedRules, + Variations: []datafileEntities.Variation{ + {ID: "var_1", Key: "variation_1"}, + }, + TrafficAllocation: []datafileEntities.TrafficAllocation{ + {EntityID: "var_1", EndOfRange: 10000}, + }, + }, + } + featureMap := map[string]entities.Feature{ + "feature_1": {ID: "feature_1", Key: "feature_1"}, + } + + holdoutList, _, flagHoldoutsMap, ruleHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) + + 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 flagHoldoutsMap + assert.Empty(t, flagHoldoutsMap) + // 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 + emptyRules := []string{} + rawHoldouts := []datafileEntities.Holdout{ + { + ID: "holdout_empty_local", + Key: "empty_local_holdout", + Status: "Running", + IncludedRules: &emptyRules, // non-nil, but empty + Variations: []datafileEntities.Variation{ + {ID: "var_1", Key: "variation_1"}, + }, + TrafficAllocation: []datafileEntities.TrafficAllocation{ + {EntityID: "var_1", EndOfRange: 10000}, + }, + }, + } + featureMap := map[string]entities.Feature{ + "feature_1": {ID: "feature_1", Key: "feature_1"}, + } + + holdoutList, _, flagHoldoutsMap, ruleHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) + + 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 flagHoldoutsMap + assert.Empty(t, flagHoldoutsMap) + // ruleHoldoutsMap should also be empty (no rules to target) + assert.Empty(t, ruleHoldoutsMap) +} + +func TestMapHoldoutsMixedGlobalAndLocal(t *testing.T) { + // Mix of global and local holdouts + includedRules := []string{"rule_1"} + rawHoldouts := []datafileEntities.Holdout{ + { + ID: "holdout_global", + Key: "global_holdout", + Status: "Running", + IncludedRules: nil, + Variations: []datafileEntities.Variation{ + {ID: "var_g", Key: "var_global"}, + }, + TrafficAllocation: []datafileEntities.TrafficAllocation{ + {EntityID: "var_g", EndOfRange: 10000}, + }, + }, + { + ID: "holdout_local", + Key: "local_holdout", + Status: "Running", + IncludedRules: &includedRules, + Variations: []datafileEntities.Variation{ + {ID: "var_l", Key: "var_local"}, + }, + TrafficAllocation: []datafileEntities.TrafficAllocation{ + {EntityID: "var_l", EndOfRange: 10000}, + }, + }, + } + featureMap := map[string]entities.Feature{ + "feature_1": {ID: "feature_1", Key: "feature_1"}, + } + + holdoutList, _, flagHoldoutsMap, ruleHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) + + assert.Len(t, holdoutList, 2) + + // Global holdout in flagHoldoutsMap (only the global one) + assert.Contains(t, flagHoldoutsMap, "feature_1") + assert.Len(t, flagHoldoutsMap["feature_1"], 1) + assert.Equal(t, "global_holdout", flagHoldoutsMap["feature_1"][0].Key) + + // 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) +} + +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{ + { + ID: "holdout_cross", + Key: "cross_rule_holdout", + Status: "Running", + IncludedRules: &includedRules, + Variations: []datafileEntities.Variation{ + {ID: "var_1", Key: "variation_1"}, + }, + TrafficAllocation: []datafileEntities.TrafficAllocation{ + {EntityID: "var_1", EndOfRange: 10000}, + }, + }, + } + featureMap := map[string]entities.Feature{} + + _, _, _, ruleHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) + + // 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) +} + +func TestMapHoldoutsIsGlobalProperty(t *testing.T) { + // Verify IsGlobal() works correctly for both global and local holdouts + 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 TestMapHoldoutsBackwardCompatibilityOldDatafile(t *testing.T) { + // Old datafiles without IncludedRules field should be treated as global + rawHoldouts := []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"}, + }, + TrafficAllocation: []datafileEntities.TrafficAllocation{ + {EntityID: "var_1", EndOfRange: 10000}, + }, + }, + } + featureMap := map[string]entities.Feature{ + "my_feature": {ID: "my_feature", Key: "my_feature"}, + } + + holdoutList, _, flagHoldoutsMap, ruleHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) + + assert.Len(t, holdoutList, 1) + // Old datafile holdout with no IncludedRules should default to global + assert.True(t, holdoutList[0].IsGlobal()) + assert.Contains(t, flagHoldoutsMap, "my_feature") + assert.Len(t, flagHoldoutsMap["my_feature"], 1) + assert.Empty(t, ruleHoldoutsMap) +} diff --git a/pkg/config/interface.go b/pkg/config/interface.go index 6d45c14a..59f7a380 100644 --- a/pkg/config/interface.go +++ b/pkg/config/interface.go @@ -57,6 +57,12 @@ type ProjectConfig interface { GetFlagVariationsMap() map[string][]entities.Variation GetRegion() string GetHoldoutsForFlag(featureKey string) []entities.Holdout + // GetGlobalHoldouts returns all global holdouts (those with IncludedRules == nil). + // These are evaluated at flag level before any per-rule evaluation. + GetGlobalHoldouts() []entities.Holdout + // GetHoldoutsForRule returns all local holdouts targeting the given rule ID. + // These are evaluated per-rule, after forced decisions, before audience/traffic checks. + GetHoldoutsForRule(ruleID string) []entities.Holdout } // ProjectConfigManager maintains an instance of the ProjectConfig diff --git a/pkg/decision/composite_feature_service.go b/pkg/decision/composite_feature_service.go index 2c1b2b54..a5e7c496 100644 --- a/pkg/decision/composite_feature_service.go +++ b/pkg/decision/composite_feature_service.go @@ -31,11 +31,12 @@ type CompositeFeatureService struct { // NewCompositeFeatureService returns a new instance of the CompositeFeatureService func NewCompositeFeatureService(sdkKey string, compositeExperimentService ExperimentService) *CompositeFeatureService { + holdoutService := NewHoldoutService(sdkKey) return &CompositeFeatureService{ logger: logging.GetLogger(sdkKey, "CompositeFeatureService"), featureServices: []FeatureService{ - NewHoldoutService(sdkKey), - NewFeatureExperimentService(logging.GetLogger(sdkKey, "FeatureExperimentService"), compositeExperimentService), + holdoutService, + NewFeatureExperimentService(logging.GetLogger(sdkKey, "FeatureExperimentService"), compositeExperimentService, holdoutService), NewRolloutService(sdkKey), }, } diff --git a/pkg/decision/evaluator/audience_evaluator_test.go b/pkg/decision/evaluator/audience_evaluator_test.go index 75897aed..e23dd86a 100644 --- a/pkg/decision/evaluator/audience_evaluator_test.go +++ b/pkg/decision/evaluator/audience_evaluator_test.go @@ -205,6 +205,16 @@ func (m *MockProjectConfig) GetHoldoutsForFlag(featureKey string) []entities.Hol return args.Get(0).([]entities.Holdout) } +func (m *MockProjectConfig) GetGlobalHoldouts() []entities.Holdout { + args := m.Called() + return args.Get(0).([]entities.Holdout) +} + +func (m *MockProjectConfig) GetHoldoutsForRule(ruleID string) []entities.Holdout { + args := m.Called(ruleID) + return args.Get(0).([]entities.Holdout) +} + // MockLogger is a mock implementation of OptimizelyLogProducer // (This declaration has been removed to resolve the redeclaration error) diff --git a/pkg/decision/feature_experiment_service.go b/pkg/decision/feature_experiment_service.go index f2bc3689..c01a09e4 100644 --- a/pkg/decision/feature_experiment_service.go +++ b/pkg/decision/feature_experiment_service.go @@ -28,14 +28,16 @@ import ( // FeatureExperimentService helps evaluate feature test associated with the feature type FeatureExperimentService struct { compositeExperimentService ExperimentService + holdoutService *HoldoutService logger logging.OptimizelyLogProducer } // NewFeatureExperimentService returns a new instance of the FeatureExperimentService -func NewFeatureExperimentService(logger logging.OptimizelyLogProducer, compositeExperimentService ExperimentService) *FeatureExperimentService { +func NewFeatureExperimentService(logger logging.OptimizelyLogProducer, compositeExperimentService ExperimentService, holdoutService *HoldoutService) *FeatureExperimentService { return &FeatureExperimentService{ logger: logger, compositeExperimentService: compositeExperimentService, + holdoutService: holdoutService, } } @@ -60,6 +62,17 @@ func (f FeatureExperimentService) GetDecision(decisionContext FeatureDecisionCon } } + // [FSSDK-12369] Check local holdouts targeting this experiment rule. + // Local holdouts are evaluated after forced decisions but before audience/traffic checks. + if f.holdoutService != nil { + holdoutDecision, holdoutReasons, _ := f.holdoutService.GetDecisionForRule(featureExperiment.ID, decisionContext.ProjectConfig, userContext, options) + reasons.Append(holdoutReasons) + if holdoutDecision.Variation != nil { + // User is in a local holdout — return holdout decision, skip rule evaluation + return holdoutDecision, reasons, nil + } + } + experiment := featureExperiment experimentDecisionContext := ExperimentDecisionContext{ Experiment: &experiment, diff --git a/pkg/decision/feature_experiment_service_test.go b/pkg/decision/feature_experiment_service_test.go index 7d1c55c1..08fd0ac5 100644 --- a/pkg/decision/feature_experiment_service_test.go +++ b/pkg/decision/feature_experiment_service_test.go @@ -174,8 +174,10 @@ func (s *FeatureExperimentServiceTestSuite) TestGetDecisionMutex() { func (s *FeatureExperimentServiceTestSuite) TestNewFeatureExperimentService() { compositeExperimentService := &CompositeExperimentService{logger: logging.GetLogger("sdkKey", "CompositeExperimentService")} - featureExperimentService := NewFeatureExperimentService(logging.GetLogger("", ""), compositeExperimentService) + holdoutService := NewHoldoutService("sdkKey") + featureExperimentService := NewFeatureExperimentService(logging.GetLogger("", ""), compositeExperimentService, holdoutService) s.IsType(compositeExperimentService, featureExperimentService.compositeExperimentService) + s.NotNil(featureExperimentService.holdoutService) } func (s *FeatureExperimentServiceTestSuite) TestGetDecisionWithCmabUUID() { diff --git a/pkg/decision/helpers_test.go b/pkg/decision/helpers_test.go index 8c7a94cc..7f308dbd 100644 --- a/pkg/decision/helpers_test.go +++ b/pkg/decision/helpers_test.go @@ -64,6 +64,16 @@ func (c *mockProjectConfig) GetHoldoutsForFlag(featureKey string) []entities.Hol return args.Get(0).([]entities.Holdout) } +func (c *mockProjectConfig) GetGlobalHoldouts() []entities.Holdout { + args := c.Called() + return args.Get(0).([]entities.Holdout) +} + +func (c *mockProjectConfig) GetHoldoutsForRule(ruleID string) []entities.Holdout { + args := c.Called(ruleID) + return args.Get(0).([]entities.Holdout) +} + type MockExperimentDecisionService struct { mock.Mock } diff --git a/pkg/decision/holdout_service.go b/pkg/decision/holdout_service.go index 48fae2ad..d2838497 100644 --- a/pkg/decision/holdout_service.go +++ b/pkg/decision/holdout_service.go @@ -120,6 +120,78 @@ func (h HoldoutService) GetDecision(decisionContext FeatureDecisionContext, user return FeatureDecision{}, reasons, nil } +// GetDecisionForRule evaluates local holdouts targeting the given rule ID. +// It returns a non-empty FeatureDecision if the user is bucketed into a local holdout for this rule. +// Local holdouts are evaluated per-rule, after forced decisions, before audience/traffic checks. +func (h HoldoutService) GetDecisionForRule(ruleID string, projectConfig config.ProjectConfig, userContext entities.UserContext, options *decide.Options) (FeatureDecision, decide.DecisionReasons, error) { + reasons := decide.NewDecisionReasons(options) + + holdouts := projectConfig.GetHoldoutsForRule(ruleID) + + for i := range holdouts { + holdout := &holdouts[i] + h.logger.Debug(fmt.Sprintf("Evaluating local holdout %s for rule %s", holdout.Key, ruleID)) + + // Check if holdout is running + if holdout.Status != entities.HoldoutStatusRunning { + reason := reasons.AddInfo("Local holdout %s is not running.", holdout.Key) + h.logger.Info(reason) + continue + } + + // Check audience conditions + inAudience := h.checkIfUserInHoldoutAudience(holdout, userContext, projectConfig, options) + reasons.Append(inAudience.reasons) + + if !inAudience.result { + reason := reasons.AddInfo("User %s does not meet conditions for local holdout %s.", userContext.ID, holdout.Key) + h.logger.Info(reason) + continue + } + + reason := reasons.AddInfo("User %s meets conditions for local holdout %s.", userContext.ID, holdout.Key) + h.logger.Info(reason) + + // Get bucketing ID + bucketingID, err := userContext.GetBucketingID() + if err != nil { + errorMessage := reasons.AddInfo("Error computing bucketing ID for local holdout %q: %q", holdout.Key, err.Error()) + h.logger.Debug(errorMessage) + } + + // Convert holdout to experiment structure for bucketing + experimentForBucketing := entities.Experiment{ + ID: holdout.ID, + Key: holdout.Key, + Variations: holdout.Variations, + TrafficAllocation: holdout.TrafficAllocation, + AudienceIds: holdout.AudienceIds, + AudienceConditions: holdout.AudienceConditions, + AudienceConditionTree: holdout.AudienceConditionTree, + } + + // Bucket user into holdout variation + variation, _, _ := h.bucketer.Bucket(bucketingID, experimentForBucketing, entities.Group{}) + + if variation != nil { + reason = reasons.AddInfo("User %s is in variation %s of local holdout %s for rule %s.", userContext.ID, variation.Key, holdout.Key, ruleID) + h.logger.Info(reason) + + featureDecision := FeatureDecision{ + Experiment: experimentForBucketing, + Variation: variation, + Source: Holdout, + } + return featureDecision, reasons, nil + } + + reason = reasons.AddInfo("User %s is not bucketed into local holdout %s for rule %s.", userContext.ID, holdout.Key, ruleID) + h.logger.Info(reason) + } + + return FeatureDecision{}, reasons, nil +} + // checkIfUserInHoldoutAudience evaluates if user meets holdout audience conditions func (h HoldoutService) checkIfUserInHoldoutAudience(holdout *entities.Holdout, userContext entities.UserContext, projectConfig config.ProjectConfig, options *decide.Options) decisionResult { decisionReasons := decide.NewDecisionReasons(options) diff --git a/pkg/decision/holdout_service_test.go b/pkg/decision/holdout_service_test.go index 9a5e4a71..55099802 100644 --- a/pkg/decision/holdout_service_test.go +++ b/pkg/decision/holdout_service_test.go @@ -408,3 +408,210 @@ func TestHoldoutServiceIntegration(t *testing.T) { assert.Equal(t, holdoutVar.ID, decision.Variation.ID) assert.Equal(t, Holdout, decision.Source) } + +// Level 2 — GetDecisionForRule (local holdout decision service) tests (FSSDK-12369) + +// TestGetDecisionForRuleNoLocalHoldouts verifies that when there are no local holdouts for a rule, +// the function returns an empty decision (the rule is evaluated normally). +func TestGetDecisionForRuleNoLocalHoldouts(t *testing.T) { + mockConfig := new(mockProjectConfig) + mockBucketer := new(MockExperimentBucketer) + mockAudienceEval := new(MockAudienceTreeEvaluator) + mockLogger := new(MockLogger) + + mockConfig.On("GetHoldoutsForRule", "rule_id_1").Return([]entities.Holdout{}) + + service := HoldoutService{ + audienceTreeEvaluator: mockAudienceEval, + bucketer: mockBucketer, + logger: mockLogger, + } + + userCtx := entities.UserContext{ID: "test_user"} + options := &decide.Options{} + + decision, _, err := service.GetDecisionForRule("rule_id_1", mockConfig, userCtx, options) + + assert.NoError(t, err) + assert.Nil(t, decision.Variation, "No local holdouts means no holdout decision") + mockConfig.AssertExpectations(t) +} + +// TestGetDecisionForRuleUserBucketedIntoLocalHoldout verifies that when a user is bucketed into a +// local holdout for a specific rule, the holdout variation is returned and rule evaluation is skipped. +func TestGetDecisionForRuleUserBucketedIntoLocalHoldout(t *testing.T) { + mockConfig := new(mockProjectConfig) + mockBucketer := new(MockExperimentBucketer) + mockAudienceEval := new(MockAudienceTreeEvaluator) + mockLogger := new(MockLogger) + + localHoldoutVar := entities.Variation{ID: "local_var_1", Key: "local_variation_1"} + localHoldout := entities.Holdout{ + ID: "local_holdout_1", + Key: "test_local_holdout", + Status: entities.HoldoutStatusRunning, + Variations: map[string]entities.Variation{ + "local_var_1": localHoldoutVar, + }, + TrafficAllocation: []entities.Range{ + {EntityID: "local_var_1", EndOfRange: 10000}, // 100% traffic + }, + } + + mockConfig.On("GetHoldoutsForRule", "rule_x").Return([]entities.Holdout{localHoldout}) + mockBucketer.On("Bucket", "test_user", mock.AnythingOfType("entities.Experiment"), entities.Group{}).Return(&localHoldoutVar, reasons.Reason(""), nil) + mockLogger.On("Debug", mock.Anything).Return() + mockLogger.On("Info", mock.Anything).Return() + + service := HoldoutService{ + audienceTreeEvaluator: mockAudienceEval, + bucketer: mockBucketer, + logger: mockLogger, + } + + userCtx := entities.UserContext{ID: "test_user"} + options := &decide.Options{} + + decision, _, err := service.GetDecisionForRule("rule_x", mockConfig, userCtx, options) + + assert.NoError(t, err) + assert.NotNil(t, decision.Variation, "User bucketed into local holdout should return holdout variation") + assert.Equal(t, localHoldoutVar.ID, decision.Variation.ID) + assert.Equal(t, Holdout, decision.Source) + mockConfig.AssertExpectations(t) + mockBucketer.AssertExpectations(t) +} + +// TestGetDecisionForRuleUserMissesLocalHoldout verifies that when a user is NOT bucketed into a +// local holdout, an empty decision is returned so that regular rule evaluation proceeds. +func TestGetDecisionForRuleUserMissesLocalHoldout(t *testing.T) { + mockConfig := new(mockProjectConfig) + mockBucketer := new(MockExperimentBucketer) + mockAudienceEval := new(MockAudienceTreeEvaluator) + mockLogger := new(MockLogger) + + localHoldout := entities.Holdout{ + ID: "local_holdout_miss", + Key: "test_local_holdout_miss", + Status: entities.HoldoutStatusRunning, + Variations: map[string]entities.Variation{ + "local_var_1": {ID: "local_var_1", Key: "local_variation_1"}, + }, + TrafficAllocation: []entities.Range{ + {EntityID: "local_var_1", EndOfRange: 0}, // 0% traffic — no user will bucket in + }, + } + + mockConfig.On("GetHoldoutsForRule", "rule_y").Return([]entities.Holdout{localHoldout}) + // Bucketer returns nil (user not bucketed) + mockBucketer.On("Bucket", "test_user_miss", mock.AnythingOfType("entities.Experiment"), entities.Group{}).Return(nil, reasons.Reason(""), nil) + mockLogger.On("Debug", mock.Anything).Return() + mockLogger.On("Info", mock.Anything).Return() + + service := HoldoutService{ + audienceTreeEvaluator: mockAudienceEval, + bucketer: mockBucketer, + logger: mockLogger, + } + + userCtx := entities.UserContext{ID: "test_user_miss"} + options := &decide.Options{} + + decision, _, err := service.GetDecisionForRule("rule_y", mockConfig, userCtx, options) + + assert.NoError(t, err) + assert.Nil(t, decision.Variation, "User not bucketed into local holdout should fall through to regular evaluation") + mockConfig.AssertExpectations(t) + mockBucketer.AssertExpectations(t) +} + +// TestGetDecisionForRuleRuleSpecificity verifies that a local holdout targeting rule X +// does NOT affect rule Y. +func TestGetDecisionForRuleRuleSpecificity(t *testing.T) { + mockConfig := new(mockProjectConfig) + mockBucketer := new(MockExperimentBucketer) + mockAudienceEval := new(MockAudienceTreeEvaluator) + mockLogger := new(MockLogger) + + localHoldoutVar := entities.Variation{ID: "local_var_1", Key: "local_variation_1"} + localHoldout := entities.Holdout{ + ID: "local_holdout_for_rule_x", + Key: "holdout_for_rule_x", + Status: entities.HoldoutStatusRunning, + Variations: map[string]entities.Variation{ + "local_var_1": localHoldoutVar, + }, + TrafficAllocation: []entities.Range{ + {EntityID: "local_var_1", EndOfRange: 10000}, // 100% traffic + }, + } + + // rule_x has the holdout; rule_y has none + mockConfig.On("GetHoldoutsForRule", "rule_x").Return([]entities.Holdout{localHoldout}) + mockConfig.On("GetHoldoutsForRule", "rule_y").Return([]entities.Holdout{}) + mockConfig.On("GetAudienceMap").Return(map[string]entities.Audience{}) + mockBucketer.On("Bucket", "test_user", mock.AnythingOfType("entities.Experiment"), entities.Group{}).Return(&localHoldoutVar, reasons.Reason(""), nil) + mockLogger.On("Debug", mock.Anything).Return() + mockLogger.On("Info", mock.Anything).Return() + + service := HoldoutService{ + audienceTreeEvaluator: mockAudienceEval, + bucketer: mockBucketer, + logger: mockLogger, + } + + userCtx := entities.UserContext{ID: "test_user"} + options := &decide.Options{} + + // rule_x: user should be in holdout + decisionX, _, errX := service.GetDecisionForRule("rule_x", mockConfig, userCtx, options) + assert.NoError(t, errX) + assert.NotNil(t, decisionX.Variation, "Local holdout for rule_x must apply to rule_x") + + // rule_y: user should NOT be in any holdout (holdout doesn't target rule_y) + decisionY, _, errY := service.GetDecisionForRule("rule_y", mockConfig, userCtx, options) + assert.NoError(t, errY) + assert.Nil(t, decisionY.Variation, "Local holdout for rule_x must NOT apply to rule_y") +} + +// TestGetDecisionForRuleLocalHoldoutSkippedIfNotRunning verifies that a non-running local holdout +// is skipped and no holdout decision is returned. +func TestGetDecisionForRuleLocalHoldoutSkippedIfNotRunning(t *testing.T) { + mockConfig := new(mockProjectConfig) + mockBucketer := new(MockExperimentBucketer) + mockAudienceEval := new(MockAudienceTreeEvaluator) + mockLogger := new(MockLogger) + + pausedLocalHoldout := entities.Holdout{ + ID: "paused_local_holdout", + Key: "paused_holdout", + Status: entities.HoldoutStatus("Paused"), // not running + Variations: map[string]entities.Variation{ + "var_1": {ID: "var_1", Key: "variation_1"}, + }, + TrafficAllocation: []entities.Range{ + {EntityID: "var_1", EndOfRange: 10000}, + }, + } + + mockConfig.On("GetHoldoutsForRule", "rule_z").Return([]entities.Holdout{pausedLocalHoldout}) + mockConfig.On("GetAudienceMap").Return(map[string]entities.Audience{}) + mockLogger.On("Debug", mock.Anything).Return() + mockLogger.On("Info", mock.Anything).Return() + + service := HoldoutService{ + audienceTreeEvaluator: mockAudienceEval, + bucketer: mockBucketer, + logger: mockLogger, + } + + userCtx := entities.UserContext{ID: "test_user"} + options := &decide.Options{} + + decision, _, err := service.GetDecisionForRule("rule_z", mockConfig, userCtx, options) + + assert.NoError(t, err) + assert.Nil(t, decision.Variation, "Non-running local holdout should be skipped") + // Bucketer should never be called for a non-running holdout + mockBucketer.AssertNotCalled(t, "Bucket") +} diff --git a/pkg/decision/rollout_service.go b/pkg/decision/rollout_service.go index 13c1402c..69f8a5f4 100644 --- a/pkg/decision/rollout_service.go +++ b/pkg/decision/rollout_service.go @@ -32,6 +32,7 @@ import ( type RolloutService struct { audienceTreeEvaluator evaluator.TreeEvaluator experimentBucketerService ExperimentService + holdoutService *HoldoutService logger logging.OptimizelyLogProducer } @@ -42,6 +43,7 @@ func NewRolloutService(sdkKey string) *RolloutService { logger: logger, audienceTreeEvaluator: evaluator.NewMixedTreeEvaluator(logger), experimentBucketerService: NewExperimentBucketerService(logging.GetLogger(sdkKey, "ExperimentBucketerService")), + holdoutService: NewHoldoutService(sdkKey), } } @@ -98,6 +100,20 @@ func (r RolloutService) GetDecision(decisionContext FeatureDecisionContext, user return nil } + // checkForLocalHoldout checks if a user is in a local holdout for the given rollout rule. + // Returns a FeatureDecision if the user is in a holdout, nil otherwise. + checkForLocalHoldout := func(exp *entities.Experiment) *FeatureDecision { + if r.holdoutService == nil { + return nil + } + holdoutDecision, holdoutReasons, _ := r.holdoutService.GetDecisionForRule(exp.ID, decisionContext.ProjectConfig, userContext, options) + reasons.Append(holdoutReasons) + if holdoutDecision.Variation != nil { + return &holdoutDecision + } + return nil + } + for index := 0; index < numberOfExperiments-1; index++ { loggingKey := strconv.Itoa(index + 1) experiment := &rollout.Experiments[index] @@ -107,6 +123,12 @@ func (r RolloutService) GetDecision(decisionContext FeatureDecisionContext, user return *forcedDecision, reasons, nil } + // [FSSDK-12369] Check local holdouts targeting this rollout rule. + // Local holdouts are evaluated after forced decisions but before audience/traffic checks. + if holdoutDecision := checkForLocalHoldout(experiment); holdoutDecision != nil { + return *holdoutDecision, reasons, nil + } + experimentDecisionContext := getExperimentDecisionContext(experiment) // Move to next evaluation if condition tree is available and evaluation fails @@ -137,6 +159,11 @@ func (r RolloutService) GetDecision(decisionContext FeatureDecisionContext, user return *forcedDecision, reasons, nil } + // [FSSDK-12369] Check local holdouts for the fallback/everyone else rule + if holdoutDecision := checkForLocalHoldout(experiment); holdoutDecision != nil { + return *holdoutDecision, reasons, nil + } + experimentDecisionContext := getExperimentDecisionContext(experiment) // Move to bucketing if conditionTree is unavailable or evaluation passes evaluationResult := experiment.AudienceConditionTree == nil || evaluateConditionTree(experiment, "Everyone Else") diff --git a/pkg/entities/experiment.go b/pkg/entities/experiment.go index 0de662b6..426e6b21 100644 --- a/pkg/entities/experiment.go +++ b/pkg/entities/experiment.go @@ -90,4 +90,15 @@ 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 *[]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. +func (h *Holdout) IsGlobal() bool { + return h.IncludedRules == nil }