From 1c9172d476d628ac88cbdaacbbacc74342db53c7 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Fri, 10 Oct 2025 11:54:38 -0700 Subject: [PATCH 1/5] add holdouts service and logic --- pkg/cmab/service_test.go | 5 + pkg/config/datafileprojectconfig/config.go | 7 + pkg/config/interface.go | 1 + pkg/decision/composite_feature_service.go | 1 + .../composite_feature_service_test.go | 7 +- pkg/decision/entities.go | 2 + .../evaluator/audience_evaluator_test.go | 5 + pkg/decision/holdout_service.go | 154 ++++++++++++++++++ pkg/entities/experiment.go | 20 +++ 9 files changed, 199 insertions(+), 3 deletions(-) create mode 100644 pkg/decision/holdout_service.go diff --git a/pkg/cmab/service_test.go b/pkg/cmab/service_test.go index 7c13edf8..4675d773 100644 --- a/pkg/cmab/service_test.go +++ b/pkg/cmab/service_test.go @@ -230,6 +230,11 @@ func (m *MockProjectConfig) GetRegion() string { return args.String(0) } +func (m *MockProjectConfig) GetHoldoutsForFlag(featureKey string) []entities.Holdout { + args := m.Called(featureKey) + 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 759b79ea..0c53400f 100644 --- a/pkg/config/datafileprojectconfig/config.go +++ b/pkg/config/datafileprojectconfig/config.go @@ -281,6 +281,13 @@ func (c DatafileProjectConfig) GetRegion() string { return c.region } +// GetHoldoutsForFlag returns all holdouts applicable to the given feature flag +// TODO: Implementation will be added in holdout parsing PR +func (c DatafileProjectConfig) GetHoldoutsForFlag(featureKey string) []entities.Holdout { + // Stub implementation - will be replaced with actual holdout logic + 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) diff --git a/pkg/config/interface.go b/pkg/config/interface.go index 04ee0d05..6d45c14a 100644 --- a/pkg/config/interface.go +++ b/pkg/config/interface.go @@ -56,6 +56,7 @@ type ProjectConfig interface { GetAttributes() []entities.Attribute GetFlagVariationsMap() map[string][]entities.Variation GetRegion() string + GetHoldoutsForFlag(featureKey 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 b92ded53..2c1b2b54 100644 --- a/pkg/decision/composite_feature_service.go +++ b/pkg/decision/composite_feature_service.go @@ -34,6 +34,7 @@ func NewCompositeFeatureService(sdkKey string, compositeExperimentService Experi return &CompositeFeatureService{ logger: logging.GetLogger(sdkKey, "CompositeFeatureService"), featureServices: []FeatureService{ + NewHoldoutService(sdkKey), NewFeatureExperimentService(logging.GetLogger(sdkKey, "FeatureExperimentService"), compositeExperimentService), NewRolloutService(sdkKey), }, diff --git a/pkg/decision/composite_feature_service_test.go b/pkg/decision/composite_feature_service_test.go index 412a810c..5a75b5eb 100644 --- a/pkg/decision/composite_feature_service_test.go +++ b/pkg/decision/composite_feature_service_test.go @@ -202,9 +202,10 @@ func (s *CompositeFeatureServiceTestSuite) TestNewCompositeFeatureService() { // Assert that the service is instantiated with the correct child services in the right order compositeExperimentService := NewCompositeExperimentService("") compositeFeatureService := NewCompositeFeatureService("", compositeExperimentService) - s.Equal(2, len(compositeFeatureService.featureServices)) - s.IsType(&FeatureExperimentService{compositeExperimentService: compositeExperimentService}, compositeFeatureService.featureServices[0]) - s.IsType(&RolloutService{}, compositeFeatureService.featureServices[1]) + s.Equal(3, len(compositeFeatureService.featureServices)) + s.IsType(&HoldoutService{}, compositeFeatureService.featureServices[0]) + s.IsType(&FeatureExperimentService{compositeExperimentService: compositeExperimentService}, compositeFeatureService.featureServices[1]) + s.IsType(&RolloutService{}, compositeFeatureService.featureServices[2]) } func TestCompositeFeatureTestSuite(t *testing.T) { diff --git a/pkg/decision/entities.go b/pkg/decision/entities.go index 621b892a..fec60903 100644 --- a/pkg/decision/entities.go +++ b/pkg/decision/entities.go @@ -55,6 +55,8 @@ const ( Rollout Source = "rollout" // FeatureTest - the decision came from a feature test FeatureTest Source = "feature-test" + // Holdout - the decision came from a holdout + Holdout Source = "holdout" ) // Decision contains base information about a decision diff --git a/pkg/decision/evaluator/audience_evaluator_test.go b/pkg/decision/evaluator/audience_evaluator_test.go index 4d57a912..75897aed 100644 --- a/pkg/decision/evaluator/audience_evaluator_test.go +++ b/pkg/decision/evaluator/audience_evaluator_test.go @@ -200,6 +200,11 @@ func (m *MockProjectConfig) GetRegion() string { return args.String(0) } +func (m *MockProjectConfig) GetHoldoutsForFlag(featureKey string) []entities.Holdout { + args := m.Called(featureKey) + 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/holdout_service.go b/pkg/decision/holdout_service.go new file mode 100644 index 00000000..4a19d975 --- /dev/null +++ b/pkg/decision/holdout_service.go @@ -0,0 +1,154 @@ +/**************************************************************************** + * Copyright 2025, Optimizely, Inc. and contributors * + * * + * Licensed under the Apache License, Version 2.0 (the "License"); * + * you may not use this file except in compliance with the License. * + * You may obtain a copy of the License at * + * * + * http://www.apache.org/licenses/LICENSE-2.0 * + * * + * Unless required by applicable law or agreed to in writing, software * + * distributed under the License is distributed on an "AS IS" BASIS, * + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * + * See the License for the specific language governing permissions and * + * limitations under the License. * + ***************************************************************************/ + +// Package decision // +package decision + +import ( + "fmt" + + "github.com/optimizely/go-sdk/v2/pkg/config" + "github.com/optimizely/go-sdk/v2/pkg/decide" + "github.com/optimizely/go-sdk/v2/pkg/decision/bucketer" + "github.com/optimizely/go-sdk/v2/pkg/decision/evaluator" + "github.com/optimizely/go-sdk/v2/pkg/entities" + "github.com/optimizely/go-sdk/v2/pkg/logging" +) + +// HoldoutService evaluates holdout groups for feature flags +type HoldoutService struct { + audienceTreeEvaluator evaluator.TreeEvaluator + bucketer bucketer.ExperimentBucketer + logger logging.OptimizelyLogProducer +} + +// NewHoldoutService returns a new instance of the HoldoutService +func NewHoldoutService(sdkKey string) *HoldoutService { + logger := logging.GetLogger(sdkKey, "HoldoutService") + return &HoldoutService{ + audienceTreeEvaluator: evaluator.NewMixedTreeEvaluator(logger), + bucketer: *bucketer.NewMurmurhashExperimentBucketer(logger, bucketer.DefaultHashSeed), + logger: logger, + } +} + +// GetDecision returns a decision for holdouts associated with the feature +func (h HoldoutService) GetDecision(decisionContext FeatureDecisionContext, userContext entities.UserContext, options *decide.Options) (FeatureDecision, decide.DecisionReasons, error) { + feature := decisionContext.Feature + reasons := decide.NewDecisionReasons(options) + + holdouts := decisionContext.ProjectConfig.GetHoldoutsForFlag(feature.Key) + + for _, holdout := range holdouts { + h.logger.Debug(fmt.Sprintf("Evaluating holdout %s for feature %s", holdout.Key, feature.Key)) + + // Check if holdout is running + if holdout.Status != entities.HoldoutStatusRunning { + reason := reasons.AddInfo("Holdout %s is not running.", holdout.Key) + h.logger.Info(reason) + continue + } + + // Check audience conditions + inAudience := h.checkIfUserInHoldoutAudience(&holdout, userContext, decisionContext.ProjectConfig, options) + reasons.Append(inAudience.reasons) + + if !inAudience.result { + reason := reasons.AddInfo("User %s does not meet conditions for holdout %s.", userContext.ID, holdout.Key) + h.logger.Info(reason) + continue + } + + reason := reasons.AddInfo("User %s meets conditions for 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 holdout %q: %q", holdout.Key, err.Error()) + h.logger.Debug(errorMessage) + } + + if bucketingID != userContext.ID { + h.logger.Debug(fmt.Sprintf("Using bucketing ID: %q for user %q", bucketingID, userContext.ID)) + } + + // 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 holdout %s.", userContext.ID, variation.Key, holdout.Key) + h.logger.Info(reason) + + featureDecision := FeatureDecision{ + Experiment: experimentForBucketing, + Variation: variation, + Source: Holdout, + } + return featureDecision, reasons, nil + } + + reason = reasons.AddInfo("User %s is in no holdout variation.", userContext.ID) + 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) + + if holdout == nil { + logMessage := decisionReasons.AddInfo("Holdout is nil, defaulting to false") + h.logger.Debug(logMessage) + return decisionResult{result: false, reasons: decisionReasons} + } + + if holdout.AudienceConditionTree != nil { + condTreeParams := entities.NewTreeParameters(&userContext, projectConfig.GetAudienceMap()) + h.logger.Debug(fmt.Sprintf("Evaluating audiences for holdout %q.", holdout.Key)) + + evalResult, _, audienceReasons := h.audienceTreeEvaluator.Evaluate(holdout.AudienceConditionTree, condTreeParams, options) + decisionReasons.Append(audienceReasons) + + logMessage := decisionReasons.AddInfo("Audiences for holdout %s collectively evaluated to %v.", holdout.Key, evalResult) + h.logger.Debug(logMessage) + + return decisionResult{result: evalResult, reasons: decisionReasons} + } + + logMessage := decisionReasons.AddInfo("Audiences for holdout %s collectively evaluated to true.", holdout.Key) + h.logger.Debug(logMessage) + return decisionResult{result: true, reasons: decisionReasons} +} + +// decisionResult is a helper struct to return both result and reasons +type decisionResult struct { + result bool + reasons decide.DecisionReasons +} diff --git a/pkg/entities/experiment.go b/pkg/entities/experiment.go index 50001cc2..6d04e581 100644 --- a/pkg/entities/experiment.go +++ b/pkg/entities/experiment.go @@ -59,3 +59,23 @@ type VariationVariable struct { ID string Value string } + +// HoldoutStatus represents the status of a holdout +type HoldoutStatus string + +const ( + // HoldoutStatusRunning - the holdout status is running + HoldoutStatusRunning HoldoutStatus = "Running" +) + +// Holdout represents a holdout that can be applied to feature flags +type Holdout struct { + ID string + Key string + Status HoldoutStatus + AudienceIds []string + AudienceConditions interface{} + Variations map[string]Variation // keyed by variation ID + TrafficAllocation []Range + AudienceConditionTree *TreeNode +} From 8c19ff7cc5515c906d82ea23a2fc7736270e4d21 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Thu, 13 Nov 2025 14:17:22 -0800 Subject: [PATCH 2/5] fix: resolve range variable address bug in holdout loop (CWE-118) - Changed loop from 'for _, holdout := range holdouts' to 'for i := range holdouts' - Created proper pointer 'holdout := &holdouts[i]' to avoid address-of-iteration-variable issue - This fixes the security warning where all iterations would point to the same memory location - All tests passing Resolves Prisma Cloud security scan: Incorrect access of indexable resource --- pkg/decision/holdout_service.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/decision/holdout_service.go b/pkg/decision/holdout_service.go index 4a19d975..64bb1980 100644 --- a/pkg/decision/holdout_service.go +++ b/pkg/decision/holdout_service.go @@ -52,7 +52,8 @@ func (h HoldoutService) GetDecision(decisionContext FeatureDecisionContext, user holdouts := decisionContext.ProjectConfig.GetHoldoutsForFlag(feature.Key) - for _, holdout := range holdouts { + for i := range holdouts { + holdout := &holdouts[i] h.logger.Debug(fmt.Sprintf("Evaluating holdout %s for feature %s", holdout.Key, feature.Key)) // Check if holdout is running @@ -63,7 +64,7 @@ func (h HoldoutService) GetDecision(decisionContext FeatureDecisionContext, user } // Check audience conditions - inAudience := h.checkIfUserInHoldoutAudience(&holdout, userContext, decisionContext.ProjectConfig, options) + inAudience := h.checkIfUserInHoldoutAudience(holdout, userContext, decisionContext.ProjectConfig, options) reasons.Append(inAudience.reasons) if !inAudience.result { From 79642d70fa1b08f348c81016ea3227a728900b44 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Thu, 13 Nov 2025 14:42:46 -0800 Subject: [PATCH 3/5] feat: Complete holdout implementation with parsing, mapping, and tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Implemented holdout parsing from datafile with global/specific/exclusion logic - Added MapHoldouts() to process holdout relationships with feature flags - Implemented GetHoldoutsForFlag() to retrieve applicable holdouts per flag - Fixed bucketer initialization to use pointer (interface value) - Created comprehensive unit tests for HoldoutService (11 test cases) - Added integration test with real bucketer and evaluator - Added GetHoldoutsForFlag() mock method to helpers_test.go All tests passing (decision and config packages) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- pkg/config/datafileprojectconfig/config.go | 12 +- .../entities/entities.go | 14 + .../datafileprojectconfig/mappers/holdout.go | 147 +++++++ pkg/decision/helpers_test.go | 5 + pkg/decision/holdout_service.go | 2 +- pkg/decision/holdout_service_test.go | 410 ++++++++++++++++++ 6 files changed, 587 insertions(+), 3 deletions(-) create mode 100644 pkg/config/datafileprojectconfig/mappers/holdout.go create mode 100644 pkg/decision/holdout_service_test.go diff --git a/pkg/config/datafileprojectconfig/config.go b/pkg/config/datafileprojectconfig/config.go index 0c53400f..13526875 100644 --- a/pkg/config/datafileprojectconfig/config.go +++ b/pkg/config/datafileprojectconfig/config.go @@ -60,6 +60,9 @@ type DatafileProjectConfig struct { region string flagVariationsMap map[string][]entities.Variation + holdouts []entities.Holdout + holdoutIDMap map[string]entities.Holdout + flagHoldoutsMap map[string][]entities.Holdout } // GetDatafile returns a string representation of the environment's datafile @@ -282,9 +285,10 @@ func (c DatafileProjectConfig) GetRegion() string { } // GetHoldoutsForFlag returns all holdouts applicable to the given feature flag -// TODO: Implementation will be added in holdout parsing PR func (c DatafileProjectConfig) GetHoldoutsForFlag(featureKey string) []entities.Holdout { - // Stub implementation - will be replaced with actual holdout logic + if holdouts, exists := c.flagHoldoutsMap[featureKey]; exists { + return holdouts + } return []entities.Holdout{} } @@ -330,6 +334,7 @@ func NewDatafileProjectConfig(jsonDatafile []byte, logger logging.OptimizelyLogP featureMap := mappers.MapFeatures(datafile.FeatureFlags, rolloutMap, experimentIDMap) audienceMap, audienceSegmentList := mappers.MapAudiences(append(datafile.TypedAudiences, datafile.Audiences...)) flagVariationsMap := mappers.MapFlagVariations(featureMap) + holdouts, holdoutIDMap, flagHoldoutsMap := mappers.MapHoldouts(datafile.Holdouts, featureMap) attributeKeyMap := make(map[string]entities.Attribute) attributeIDToKeyMap := make(map[string]string) @@ -372,6 +377,9 @@ func NewDatafileProjectConfig(jsonDatafile []byte, logger logging.OptimizelyLogP attributeKeyMap: attributeKeyMap, attributeIDToKeyMap: attributeIDToKeyMap, region: region, + holdouts: holdouts, + holdoutIDMap: holdoutIDMap, + flagHoldoutsMap: flagHoldoutsMap, } logger.Info("Datafile is valid.") diff --git a/pkg/config/datafileprojectconfig/entities/entities.go b/pkg/config/datafileprojectconfig/entities/entities.go index 6b7f2ab8..76edd48d 100644 --- a/pkg/config/datafileprojectconfig/entities/entities.go +++ b/pkg/config/datafileprojectconfig/entities/entities.go @@ -113,6 +113,19 @@ type Rollout struct { Experiments []Experiment `json:"experiments"` } +// Holdout represents a holdout from the Optimizely datafile +type Holdout struct { + ID string `json:"id"` + Key string `json:"key"` + Status string `json:"status"` + AudienceIds []string `json:"audienceIds"` + AudienceConditions interface{} `json:"audienceConditions"` + Variations []Variation `json:"variations"` + TrafficAllocation []TrafficAllocation `json:"trafficAllocation"` + IncludedFlags []string `json:"includedFlags,omitempty"` + ExcludedFlags []string `json:"excludedFlags,omitempty"` +} + // Integration represents a integration from the Optimizely datafile type Integration struct { Key *string `json:"key"` @@ -129,6 +142,7 @@ type Datafile struct { FeatureFlags []FeatureFlag `json:"featureFlags"` Events []Event `json:"events"` Rollouts []Rollout `json:"rollouts"` + Holdouts []Holdout `json:"holdouts,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 new file mode 100644 index 00000000..0a47bcf3 --- /dev/null +++ b/pkg/config/datafileprojectconfig/mappers/holdout.go @@ -0,0 +1,147 @@ +/**************************************************************************** + * Copyright 2025, Optimizely, Inc. and contributors * + * * + * Licensed under the Apache License, Version 2.0 (the "License"); * + * you may not use this file except in compliance with the License. * + * You may obtain a copy of the License at * + * * + * http://www.apache.org/licenses/LICENSE-2.0 * + * * + * Unless required by applicable law or agreed to in writing, software * + * distributed under the License is distributed on an "AS IS" BASIS, * + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * + * See the License for the specific language governing permissions and * + * limitations under the License. * + ***************************************************************************/ + +// Package mappers ... +package mappers + +import ( + datafileEntities "github.com/optimizely/go-sdk/v2/pkg/config/datafileprojectconfig/entities" + "github.com/optimizely/go-sdk/v2/pkg/entities" +) + +// MapHoldouts maps the raw datafile holdout entities to SDK Holdout entities +// and organizes them by flag relationships +func MapHoldouts(holdouts []datafileEntities.Holdout, featureMap map[string]entities.Feature) ( + holdoutList []entities.Holdout, + holdoutIDMap map[string]entities.Holdout, + flagHoldoutsMap map[string][]entities.Holdout, +) { + holdoutList = []entities.Holdout{} + holdoutIDMap = make(map[string]entities.Holdout) + flagHoldoutsMap = make(map[string][]entities.Holdout) + + globalHoldouts := []entities.Holdout{} + includedHoldouts := make(map[string][]entities.Holdout) + excludedHoldouts := make(map[string][]entities.Holdout) + + for _, holdout := range holdouts { + // Only process running holdouts + if holdout.Status != string(entities.HoldoutStatusRunning) { + continue + } + + mappedHoldout := mapHoldout(holdout) + holdoutList = append(holdoutList, mappedHoldout) + holdoutIDMap[holdout.ID] = mappedHoldout + + // Classify holdout by flag relationships + if len(holdout.IncludedFlags) == 0 { + // Global holdout - applies to all flags except excluded + globalHoldouts = append(globalHoldouts, mappedHoldout) + + // Track exclusions + for _, flagID := range holdout.ExcludedFlags { + excludedHoldouts[flagID] = append(excludedHoldouts[flagID], mappedHoldout) + } + } else { + // Specific holdout - applies only to included flags + for _, flagID := range holdout.IncludedFlags { + includedHoldouts[flagID] = append(includedHoldouts[flagID], mappedHoldout) + } + } + } + + // Build flagHoldoutsMap by combining global and specific holdouts + for _, feature := range featureMap { + flagKey := feature.Key + flagID := feature.ID + applicableHoldouts := []entities.Holdout{} + + // Add specifically included holdouts + if included, exists := includedHoldouts[flagID]; exists { + applicableHoldouts = append(applicableHoldouts, included...) + } + + // Add global holdouts (if not excluded) + isExcluded := false + if _, exists := excludedHoldouts[flagID]; exists { + isExcluded = true + } + + if !isExcluded { + applicableHoldouts = append(applicableHoldouts, globalHoldouts...) + } + + if len(applicableHoldouts) > 0 { + flagHoldoutsMap[flagKey] = applicableHoldouts + } + } + + return holdoutList, holdoutIDMap, flagHoldoutsMap +} + +func mapHoldout(datafileHoldout datafileEntities.Holdout) entities.Holdout { + var audienceConditionTree *entities.TreeNode + var err error + + // Build audience condition tree similar to experiments + if datafileHoldout.AudienceConditions == nil && len(datafileHoldout.AudienceIds) > 0 { + audienceConditionTree, err = buildAudienceConditionTree(datafileHoldout.AudienceIds) + } else { + switch audienceConditions := datafileHoldout.AudienceConditions.(type) { + case []interface{}: + if len(audienceConditions) > 0 { + audienceConditionTree, err = buildAudienceConditionTree(audienceConditions) + } + case string: + if audienceConditions != "" { + audienceConditionTree, err = buildAudienceConditionTree([]string{audienceConditions}) + } + default: + } + } + if err != nil { + // @TODO: handle error + func() {}() // cheat the linters + } + + // Map variations + variations := make(map[string]entities.Variation) + for _, datafileVariation := range datafileHoldout.Variations { + variation := mapVariation(datafileVariation) + variations[variation.ID] = variation + } + + // Map traffic allocations + trafficAllocation := make([]entities.Range, len(datafileHoldout.TrafficAllocation)) + for i, allocation := range datafileHoldout.TrafficAllocation { + trafficAllocation[i] = entities.Range{ + EntityID: allocation.EntityID, + EndOfRange: allocation.EndOfRange, + } + } + + return entities.Holdout{ + ID: datafileHoldout.ID, + Key: datafileHoldout.Key, + Status: entities.HoldoutStatus(datafileHoldout.Status), + AudienceIds: datafileHoldout.AudienceIds, + AudienceConditions: datafileHoldout.AudienceConditions, + Variations: variations, + TrafficAllocation: trafficAllocation, + AudienceConditionTree: audienceConditionTree, + } +} diff --git a/pkg/decision/helpers_test.go b/pkg/decision/helpers_test.go index 56a7e168..8c7a94cc 100644 --- a/pkg/decision/helpers_test.go +++ b/pkg/decision/helpers_test.go @@ -59,6 +59,11 @@ func (c *mockProjectConfig) GetFlagVariationsMap() map[string][]entities.Variati return args.Get(0).(map[string][]entities.Variation) } +func (c *mockProjectConfig) GetHoldoutsForFlag(featureKey string) []entities.Holdout { + args := c.Called(featureKey) + 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 64bb1980..b7da9a68 100644 --- a/pkg/decision/holdout_service.go +++ b/pkg/decision/holdout_service.go @@ -40,7 +40,7 @@ func NewHoldoutService(sdkKey string) *HoldoutService { logger := logging.GetLogger(sdkKey, "HoldoutService") return &HoldoutService{ audienceTreeEvaluator: evaluator.NewMixedTreeEvaluator(logger), - bucketer: *bucketer.NewMurmurhashExperimentBucketer(logger, bucketer.DefaultHashSeed), + bucketer: bucketer.NewMurmurhashExperimentBucketer(logger, bucketer.DefaultHashSeed), logger: logger, } } diff --git a/pkg/decision/holdout_service_test.go b/pkg/decision/holdout_service_test.go new file mode 100644 index 00000000..9a5e4a71 --- /dev/null +++ b/pkg/decision/holdout_service_test.go @@ -0,0 +1,410 @@ +/**************************************************************************** + * Copyright 2025, Optimizely, Inc. and contributors * + * * + * Licensed under the Apache License, Version 2.0 (the "License"); * + * you may not use this file except in compliance with the License. * + * You may obtain a copy of the License at * + * * + * http://www.apache.org/licenses/LICENSE-2.0 * + * * + * Unless required by applicable law or agreed to in writing, software * + * distributed under the License is distributed on an "AS IS" BASIS, * + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * + * See the License for the specific language governing permissions and * + * limitations under the License. * + ***************************************************************************/ + +package decision + +import ( + "testing" + + "github.com/optimizely/go-sdk/v2/pkg/decide" + "github.com/optimizely/go-sdk/v2/pkg/decision/bucketer" + "github.com/optimizely/go-sdk/v2/pkg/decision/evaluator" + "github.com/optimizely/go-sdk/v2/pkg/decision/reasons" + "github.com/optimizely/go-sdk/v2/pkg/entities" + "github.com/optimizely/go-sdk/v2/pkg/logging" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/suite" +) + +// Test holdout data +var testHoldoutVar1 = entities.Variation{ID: "holdout_var_1", Key: "holdout_variation_1"} +var testHoldoutVar2 = entities.Variation{ID: "holdout_var_2", Key: "holdout_variation_2"} + +var testAudience7777 = entities.Audience{ID: "7777"} +var testAudience7778 = entities.Audience{ID: "7778"} + +var testHoldout1 = entities.Holdout{ + ID: "holdout_1", + Key: "test_holdout_1", + Status: entities.HoldoutStatusRunning, + AudienceConditionTree: &entities.TreeNode{ + Operator: "and", + Nodes: []*entities.TreeNode{ + {Item: "7777"}, + }, + }, + Variations: map[string]entities.Variation{ + "holdout_var_1": testHoldoutVar1, + "holdout_var_2": testHoldoutVar2, + }, + TrafficAllocation: []entities.Range{ + {EntityID: "holdout_var_1", EndOfRange: 5000}, + {EntityID: "holdout_var_2", EndOfRange: 10000}, + }, +} + +var testHoldout2NoAudience = entities.Holdout{ + ID: "holdout_2", + Key: "test_holdout_2_no_audience", + Status: entities.HoldoutStatusRunning, + AudienceConditionTree: nil, // No audience targeting + Variations: map[string]entities.Variation{ + "holdout_var_1": testHoldoutVar1, + }, + TrafficAllocation: []entities.Range{ + {EntityID: "holdout_var_1", EndOfRange: 10000}, + }, +} + +var testHoldout3NotRunning = entities.Holdout{ + ID: "holdout_3", + Key: "test_holdout_3_not_running", + Status: entities.HoldoutStatus("Paused"), +} + +type HoldoutServiceTestSuite struct { + suite.Suite + mockConfig *mockProjectConfig + mockAudienceTreeEvaluator *MockAudienceTreeEvaluator + mockBucketer *MockExperimentBucketer + testFeatureDecisionContext FeatureDecisionContext + testUserContext entities.UserContext + mockLogger *MockLogger + options *decide.Options + decisionReasons decide.DecisionReasons +} + +func (s *HoldoutServiceTestSuite) SetupTest() { + s.mockConfig = new(mockProjectConfig) + s.mockAudienceTreeEvaluator = new(MockAudienceTreeEvaluator) + s.mockBucketer = new(MockExperimentBucketer) + + testAudienceMap := map[string]entities.Audience{ + "7777": testAudience7777, + "7778": testAudience7778, + } + + s.testUserContext = entities.UserContext{ + ID: "test_user_holdout", + } + + testFeature := entities.Feature{ + ID: "feature_1", + Key: "test_feature_with_holdout", + } + + s.testFeatureDecisionContext = FeatureDecisionContext{ + Feature: &testFeature, + ProjectConfig: s.mockConfig, + } + + s.mockConfig.On("GetAudienceMap").Return(testAudienceMap) + s.mockLogger = new(MockLogger) + s.options = &decide.Options{} + s.decisionReasons = decide.NewDecisionReasons(s.options) +} + +func (s *HoldoutServiceTestSuite) TestGetDecisionWithNoHoldouts() { + // Setup: No holdouts for the feature + s.mockConfig.On("GetHoldoutsForFlag", "test_feature_with_holdout").Return([]entities.Holdout{}) + + testHoldoutService := HoldoutService{ + audienceTreeEvaluator: s.mockAudienceTreeEvaluator, + bucketer: s.mockBucketer, + logger: s.mockLogger, + } + + decision, _, err := testHoldoutService.GetDecision(s.testFeatureDecisionContext, s.testUserContext, s.options) + + s.NoError(err) + s.Equal(FeatureDecision{}, decision) +} + +func (s *HoldoutServiceTestSuite) TestGetDecisionWithHoldoutNotRunning() { + // Setup: Holdout exists but is not running + s.mockConfig.On("GetHoldoutsForFlag", "test_feature_with_holdout").Return([]entities.Holdout{testHoldout3NotRunning}) + s.mockLogger.On("Debug", mock.Anything).Return() + s.mockLogger.On("Info", mock.MatchedBy(func(msg string) bool { + return true // Accept any info log message + })).Return() + + testHoldoutService := HoldoutService{ + audienceTreeEvaluator: s.mockAudienceTreeEvaluator, + bucketer: s.mockBucketer, + logger: s.mockLogger, + } + + decision, _, err := testHoldoutService.GetDecision(s.testFeatureDecisionContext, s.testUserContext, s.options) + + s.NoError(err) + s.Equal(FeatureDecision{}, decision) +} + +func (s *HoldoutServiceTestSuite) TestGetDecisionUserNotInAudience() { + // Setup: User doesn't meet audience conditions + s.mockConfig.On("GetHoldoutsForFlag", "test_feature_with_holdout").Return([]entities.Holdout{testHoldout1}) + s.mockAudienceTreeEvaluator.On("Evaluate", testHoldout1.AudienceConditionTree, mock.Anything, s.options).Return(false, true, s.decisionReasons) + s.mockLogger.On("Debug", mock.Anything).Return() + s.mockLogger.On("Info", mock.Anything).Return() + + testHoldoutService := HoldoutService{ + audienceTreeEvaluator: s.mockAudienceTreeEvaluator, + bucketer: s.mockBucketer, + logger: s.mockLogger, + } + + decision, _, err := testHoldoutService.GetDecision(s.testFeatureDecisionContext, s.testUserContext, s.options) + + s.NoError(err) + s.Equal(FeatureDecision{}, decision) + s.mockAudienceTreeEvaluator.AssertExpectations(s.T()) +} + +func (s *HoldoutServiceTestSuite) TestGetDecisionUserInAudienceButNotBucketed() { + // Setup: User meets audience conditions but doesn't get bucketed into a variation + s.mockConfig.On("GetHoldoutsForFlag", "test_feature_with_holdout").Return([]entities.Holdout{testHoldout1}) + s.mockAudienceTreeEvaluator.On("Evaluate", testHoldout1.AudienceConditionTree, mock.Anything, s.options).Return(true, true, s.decisionReasons) + s.mockBucketer.On("Bucket", "test_user_holdout", mock.AnythingOfType("entities.Experiment"), entities.Group{}).Return(nil, reasons.Reason(""), nil) + s.mockLogger.On("Debug", mock.Anything).Return() + s.mockLogger.On("Info", mock.Anything).Return() + + testHoldoutService := HoldoutService{ + audienceTreeEvaluator: s.mockAudienceTreeEvaluator, + bucketer: s.mockBucketer, + logger: s.mockLogger, + } + + decision, _, err := testHoldoutService.GetDecision(s.testFeatureDecisionContext, s.testUserContext, s.options) + + s.NoError(err) + s.Equal(FeatureDecision{}, decision) + s.mockAudienceTreeEvaluator.AssertExpectations(s.T()) + s.mockBucketer.AssertExpectations(s.T()) +} + +func (s *HoldoutServiceTestSuite) TestGetDecisionHappyPath() { + // Setup: User meets audience conditions and gets bucketed into a variation + s.mockConfig.On("GetHoldoutsForFlag", "test_feature_with_holdout").Return([]entities.Holdout{testHoldout1}) + s.mockAudienceTreeEvaluator.On("Evaluate", testHoldout1.AudienceConditionTree, mock.Anything, s.options).Return(true, true, s.decisionReasons) + s.mockBucketer.On("Bucket", "test_user_holdout", mock.AnythingOfType("entities.Experiment"), entities.Group{}).Return(&testHoldoutVar1, reasons.Reason(""), nil) + s.mockLogger.On("Debug", mock.Anything).Return() + s.mockLogger.On("Info", mock.Anything).Return() + + testHoldoutService := HoldoutService{ + audienceTreeEvaluator: s.mockAudienceTreeEvaluator, + bucketer: s.mockBucketer, + logger: s.mockLogger, + } + + decision, _, err := testHoldoutService.GetDecision(s.testFeatureDecisionContext, s.testUserContext, s.options) + + s.NoError(err) + s.NotNil(decision.Variation) + s.Equal(testHoldoutVar1.ID, decision.Variation.ID) + s.Equal(Holdout, decision.Source) + s.mockAudienceTreeEvaluator.AssertExpectations(s.T()) + s.mockBucketer.AssertExpectations(s.T()) +} + +func (s *HoldoutServiceTestSuite) TestGetDecisionNoAudienceTargeting() { + // Setup: Holdout with no audience targeting (applies to everyone) + s.mockConfig.On("GetHoldoutsForFlag", "test_feature_with_holdout").Return([]entities.Holdout{testHoldout2NoAudience}) + s.mockBucketer.On("Bucket", "test_user_holdout", mock.AnythingOfType("entities.Experiment"), entities.Group{}).Return(&testHoldoutVar1, reasons.Reason(""), nil) + s.mockLogger.On("Debug", mock.Anything).Return() + s.mockLogger.On("Info", mock.Anything).Return() + + testHoldoutService := HoldoutService{ + audienceTreeEvaluator: s.mockAudienceTreeEvaluator, + bucketer: s.mockBucketer, + logger: s.mockLogger, + } + + decision, _, err := testHoldoutService.GetDecision(s.testFeatureDecisionContext, s.testUserContext, s.options) + + s.NoError(err) + s.NotNil(decision.Variation) + s.Equal(testHoldoutVar1.ID, decision.Variation.ID) + s.Equal(Holdout, decision.Source) + s.mockBucketer.AssertExpectations(s.T()) +} + +func (s *HoldoutServiceTestSuite) TestGetDecisionMultipleHoldoutsFirstMatches() { + // Setup: Multiple holdouts, first one matches + holdouts := []entities.Holdout{testHoldout1, testHoldout2NoAudience} + s.mockConfig.On("GetHoldoutsForFlag", "test_feature_with_holdout").Return(holdouts) + s.mockAudienceTreeEvaluator.On("Evaluate", testHoldout1.AudienceConditionTree, mock.Anything, s.options).Return(true, true, s.decisionReasons) + s.mockBucketer.On("Bucket", "test_user_holdout", mock.AnythingOfType("entities.Experiment"), entities.Group{}).Return(&testHoldoutVar1, reasons.Reason(""), nil) + s.mockLogger.On("Debug", mock.Anything).Return() + s.mockLogger.On("Info", mock.Anything).Return() + + testHoldoutService := HoldoutService{ + audienceTreeEvaluator: s.mockAudienceTreeEvaluator, + bucketer: s.mockBucketer, + logger: s.mockLogger, + } + + decision, _, err := testHoldoutService.GetDecision(s.testFeatureDecisionContext, s.testUserContext, s.options) + + s.NoError(err) + s.NotNil(decision.Variation) + s.Equal(testHoldout1.ID, decision.Experiment.ID) + s.Equal(Holdout, decision.Source) + s.mockAudienceTreeEvaluator.AssertExpectations(s.T()) + s.mockBucketer.AssertExpectations(s.T()) +} + +func (s *HoldoutServiceTestSuite) TestGetDecisionMultipleHoldoutsSecondMatches() { + // Setup: Multiple holdouts, first doesn't match, second does + holdouts := []entities.Holdout{testHoldout1, testHoldout2NoAudience} + s.mockConfig.On("GetHoldoutsForFlag", "test_feature_with_holdout").Return(holdouts) + // First holdout: user not in audience + s.mockAudienceTreeEvaluator.On("Evaluate", testHoldout1.AudienceConditionTree, mock.Anything, s.options).Return(false, true, s.decisionReasons) + // Second holdout: no audience, user gets bucketed + s.mockBucketer.On("Bucket", "test_user_holdout", mock.AnythingOfType("entities.Experiment"), entities.Group{}).Return(&testHoldoutVar1, reasons.Reason(""), nil) + s.mockLogger.On("Debug", mock.Anything).Return() + s.mockLogger.On("Info", mock.Anything).Return() + + testHoldoutService := HoldoutService{ + audienceTreeEvaluator: s.mockAudienceTreeEvaluator, + bucketer: s.mockBucketer, + logger: s.mockLogger, + } + + decision, _, err := testHoldoutService.GetDecision(s.testFeatureDecisionContext, s.testUserContext, s.options) + + s.NoError(err) + s.NotNil(decision.Variation) + s.Equal(testHoldout2NoAudience.ID, decision.Experiment.ID) + s.Equal(Holdout, decision.Source) + s.mockAudienceTreeEvaluator.AssertExpectations(s.T()) + s.mockBucketer.AssertExpectations(s.T()) +} + +func (s *HoldoutServiceTestSuite) TestCheckIfUserInHoldoutAudienceNilHoldout() { + testHoldoutService := HoldoutService{ + audienceTreeEvaluator: s.mockAudienceTreeEvaluator, + logger: s.mockLogger, + } + s.mockLogger.On("Debug", mock.Anything).Return() + + result := testHoldoutService.checkIfUserInHoldoutAudience(nil, s.testUserContext, s.mockConfig, s.options) + + s.False(result.result) +} + +func (s *HoldoutServiceTestSuite) TestCheckIfUserInHoldoutAudienceNoConditionTree() { + holdout := testHoldout2NoAudience + testHoldoutService := HoldoutService{ + audienceTreeEvaluator: s.mockAudienceTreeEvaluator, + logger: s.mockLogger, + } + s.mockLogger.On("Debug", mock.Anything).Return() + + result := testHoldoutService.checkIfUserInHoldoutAudience(&holdout, s.testUserContext, s.mockConfig, s.options) + + s.True(result.result) +} + +func (s *HoldoutServiceTestSuite) TestCheckIfUserInHoldoutAudienceWithConditionTree() { + holdout := testHoldout1 + testHoldoutService := HoldoutService{ + audienceTreeEvaluator: s.mockAudienceTreeEvaluator, + logger: s.mockLogger, + } + s.mockAudienceTreeEvaluator.On("Evaluate", holdout.AudienceConditionTree, mock.Anything, s.options).Return(true, true, s.decisionReasons) + s.mockLogger.On("Debug", mock.Anything).Return() + + result := testHoldoutService.checkIfUserInHoldoutAudience(&holdout, s.testUserContext, s.mockConfig, s.options) + + s.True(result.result) + s.mockAudienceTreeEvaluator.AssertExpectations(s.T()) +} + +func TestHoldoutServiceTestSuite(t *testing.T) { + suite.Run(t, new(HoldoutServiceTestSuite)) +} + +func TestNewHoldoutService(t *testing.T) { + sdkKey := "test_sdk_key" + service := NewHoldoutService(sdkKey) + + assert.NotNil(t, service) + assert.NotNil(t, service.audienceTreeEvaluator) + assert.NotNil(t, service.bucketer) + assert.NotNil(t, service.logger) +} + +// Integration test with real bucketer and evaluator +func TestHoldoutServiceIntegration(t *testing.T) { + logger := logging.GetLogger("", "HoldoutService") + + // Create real dependencies + audienceEvaluator := evaluator.NewMixedTreeEvaluator(logger) + bucketer := bucketer.NewMurmurhashExperimentBucketer(logger, bucketer.DefaultHashSeed) + + service := &HoldoutService{ + audienceTreeEvaluator: audienceEvaluator, + bucketer: *bucketer, + logger: logger, + } + + // Create a simple holdout with no audience targeting + holdoutVar := entities.Variation{ID: "var_1", Key: "variation_1"} + holdout := entities.Holdout{ + ID: "holdout_integration", + Key: "test_holdout_integration", + Status: entities.HoldoutStatusRunning, + AudienceConditionTree: nil, + Variations: map[string]entities.Variation{ + "var_1": holdoutVar, + }, + TrafficAllocation: []entities.Range{ + {EntityID: "var_1", EndOfRange: 10000}, // 100% traffic + }, + } + + // Create mock config + mockConfig := new(mockProjectConfig) + mockConfig.On("GetHoldoutsForFlag", "test_feature").Return([]entities.Holdout{holdout}) + mockConfig.On("GetAudienceMap").Return(map[string]entities.Audience{}) + + feature := entities.Feature{ + ID: "feature_integration", + Key: "test_feature", + } + + decisionContext := FeatureDecisionContext{ + Feature: &feature, + ProjectConfig: mockConfig, + } + + userContext := entities.UserContext{ + ID: "user_123", + } + + options := &decide.Options{} + + // Execute decision + decision, _, err := service.GetDecision(decisionContext, userContext, options) + + // Verify + assert.NoError(t, err) + assert.NotNil(t, decision.Variation) + assert.Equal(t, holdoutVar.ID, decision.Variation.ID) + assert.Equal(t, Holdout, decision.Source) +} From c2aa044d4cfb648b6e4010b486ca3d84df608474 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Thu, 13 Nov 2025 15:00:12 -0800 Subject: [PATCH 4/5] test: Add comprehensive tests for holdout implementation and fix linting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Added 3 unit tests for GetHoldoutsForFlag() in config_test.go - Created holdout_test.go with 7 comprehensive mapper tests: - Empty holdouts - Global holdouts with exclusions - Specific holdouts with inclusions - Non-running holdouts filtering - Mixed global and specific holdouts - Audience conditions mapping - Variations and traffic allocation mapping - Fixed govet shadow linting error in holdout_service.go line 105 All tests passing. Improves code coverage for new holdout functionality. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../datafileprojectconfig/config_test.go | 61 ++++ .../mappers/holdout_test.go | 267 ++++++++++++++++++ pkg/decision/holdout_service.go | 2 +- 3 files changed, 329 insertions(+), 1 deletion(-) create mode 100644 pkg/config/datafileprojectconfig/mappers/holdout_test.go diff --git a/pkg/config/datafileprojectconfig/config_test.go b/pkg/config/datafileprojectconfig/config_test.go index ec14fbcb..96984729 100644 --- a/pkg/config/datafileprojectconfig/config_test.go +++ b/pkg/config/datafileprojectconfig/config_test.go @@ -728,3 +728,64 @@ func TestGetAttributeByKeyWithDirectMapping(t *testing.T) { assert.Nil(t, err) assert.Equal(t, attribute, actual) } + +func TestGetHoldoutsForFlagWithHoldouts(t *testing.T) { + flagKey := "test_flag" + holdout1 := entities.Holdout{ + ID: "holdout_1", + Key: "test_holdout_1", + Status: entities.HoldoutStatusRunning, + } + holdout2 := entities.Holdout{ + ID: "holdout_2", + Key: "test_holdout_2", + Status: entities.HoldoutStatusRunning, + } + + flagHoldoutsMap := make(map[string][]entities.Holdout) + flagHoldoutsMap[flagKey] = []entities.Holdout{holdout1, holdout2} + + config := &DatafileProjectConfig{ + flagHoldoutsMap: flagHoldoutsMap, + } + + actual := config.GetHoldoutsForFlag(flagKey) + assert.Len(t, actual, 2) + assert.Equal(t, holdout1, actual[0]) + assert.Equal(t, holdout2, actual[1]) +} + +func TestGetHoldoutsForFlagWithNoHoldouts(t *testing.T) { + flagKey := "test_flag" + flagHoldoutsMap := make(map[string][]entities.Holdout) + + config := &DatafileProjectConfig{ + flagHoldoutsMap: flagHoldoutsMap, + } + + actual := config.GetHoldoutsForFlag(flagKey) + assert.Len(t, actual, 0) + assert.Equal(t, []entities.Holdout{}, actual) +} + +func TestGetHoldoutsForFlagWithDifferentFlag(t *testing.T) { + flagKey := "test_flag" + otherFlagKey := "other_flag" + holdout := entities.Holdout{ + ID: "holdout_1", + Key: "test_holdout_1", + Status: entities.HoldoutStatusRunning, + } + + flagHoldoutsMap := make(map[string][]entities.Holdout) + flagHoldoutsMap[otherFlagKey] = []entities.Holdout{holdout} + + config := &DatafileProjectConfig{ + flagHoldoutsMap: flagHoldoutsMap, + } + + // Request different flag - should return empty + actual := config.GetHoldoutsForFlag(flagKey) + assert.Len(t, actual, 0) + assert.Equal(t, []entities.Holdout{}, actual) +} diff --git a/pkg/config/datafileprojectconfig/mappers/holdout_test.go b/pkg/config/datafileprojectconfig/mappers/holdout_test.go new file mode 100644 index 00000000..6e0e7c88 --- /dev/null +++ b/pkg/config/datafileprojectconfig/mappers/holdout_test.go @@ -0,0 +1,267 @@ +/**************************************************************************** + * Copyright 2025, Optimizely, Inc. and contributors * + * * + * Licensed under the Apache License, Version 2.0 (the "License"); * + * you may not use this file except in compliance with the License. * + * You may obtain a copy of the License at * + * * + * http://www.apache.org/licenses/LICENSE-2.0 * + * * + * Unless required by applicable law or agreed to in writing, software * + * distributed under the License is distributed on an "AS IS" BASIS, * + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * + * See the License for the specific language governing permissions and * + * limitations under the License. * + ***************************************************************************/ + +package mappers + +import ( + "testing" + + datafileEntities "github.com/optimizely/go-sdk/v2/pkg/config/datafileprojectconfig/entities" + "github.com/optimizely/go-sdk/v2/pkg/entities" + "github.com/stretchr/testify/assert" +) + +func TestMapHoldoutsEmpty(t *testing.T) { + rawHoldouts := []datafileEntities.Holdout{} + featureMap := map[string]entities.Feature{} + + holdoutList, holdoutIDMap, flagHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) + + assert.Empty(t, holdoutList) + assert.Empty(t, holdoutIDMap) + assert.Empty(t, flagHoldoutsMap) +} + +func TestMapHoldoutsGlobalHoldout(t *testing.T) { + // Global holdout: no includedFlags, applies to all flags except excluded + rawHoldouts := []datafileEntities.Holdout{ + { + ID: "holdout_1", + Key: "global_holdout", + Status: "Running", + ExcludedFlags: []string{"feature_2"}, + 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"}, + "feature_2": {ID: "feature_2", Key: "feature_2"}, + "feature_3": {ID: "feature_3", Key: "feature_3"}, + } + + holdoutList, holdoutIDMap, flagHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) + + // Verify holdout list and ID map + assert.Len(t, holdoutList, 1) + assert.Len(t, holdoutIDMap, 1) + assert.Equal(t, "holdout_1", holdoutList[0].ID) + assert.Equal(t, "global_holdout", holdoutList[0].Key) + + // Global holdout should apply to feature_1 and feature_3, but NOT feature_2 (excluded) + assert.Contains(t, flagHoldoutsMap, "feature_1") + assert.NotContains(t, flagHoldoutsMap, "feature_2") + assert.Contains(t, flagHoldoutsMap, "feature_3") + + assert.Len(t, flagHoldoutsMap["feature_1"], 1) + assert.Len(t, flagHoldoutsMap["feature_3"], 1) +} + +func TestMapHoldoutsSpecificHoldout(t *testing.T) { + // Specific holdout: has includedFlags, only applies to those flags + rawHoldouts := []datafileEntities.Holdout{ + { + ID: "holdout_1", + Key: "specific_holdout", + Status: "Running", + IncludedFlags: []string{"feature_1", "feature_2"}, + 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"}, + "feature_2": {ID: "feature_2", Key: "feature_2"}, + "feature_3": {ID: "feature_3", Key: "feature_3"}, + } + + holdoutList, holdoutIDMap, flagHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) + + // Verify holdout list and ID map + assert.Len(t, holdoutList, 1) + assert.Len(t, holdoutIDMap, 1) + + // Specific holdout should only apply to feature_1 and feature_2 + assert.Contains(t, flagHoldoutsMap, "feature_1") + assert.Contains(t, flagHoldoutsMap, "feature_2") + assert.NotContains(t, flagHoldoutsMap, "feature_3") + + assert.Len(t, flagHoldoutsMap["feature_1"], 1) + assert.Len(t, flagHoldoutsMap["feature_2"], 1) +} + +func TestMapHoldoutsNotRunning(t *testing.T) { + // Holdout with non-Running status should be filtered out + rawHoldouts := []datafileEntities.Holdout{ + { + ID: "holdout_1", + Key: "paused_holdout", + Status: "Paused", + Variations: []datafileEntities.Variation{ + {ID: "var_1", Key: "variation_1"}, + }, + }, + } + + featureMap := map[string]entities.Feature{ + "feature_1": {ID: "feature_1", Key: "feature_1"}, + } + + holdoutList, holdoutIDMap, flagHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) + + // Non-running holdouts should be filtered out + assert.Empty(t, holdoutList) + assert.Empty(t, holdoutIDMap) + assert.Empty(t, flagHoldoutsMap) +} + +func TestMapHoldoutsMixed(t *testing.T) { + // Mix of global and specific holdouts + rawHoldouts := []datafileEntities.Holdout{ + { + ID: "holdout_global", + Key: "global_holdout", + Status: "Running", + ExcludedFlags: []string{"feature_2"}, + Variations: []datafileEntities.Variation{ + {ID: "var_global", Key: "variation_global"}, + }, + TrafficAllocation: []datafileEntities.TrafficAllocation{ + {EntityID: "var_global", EndOfRange: 5000}, + }, + }, + { + ID: "holdout_specific", + Key: "specific_holdout", + Status: "Running", + IncludedFlags: []string{"feature_2"}, + Variations: []datafileEntities.Variation{ + {ID: "var_specific", Key: "variation_specific"}, + }, + TrafficAllocation: []datafileEntities.TrafficAllocation{ + {EntityID: "var_specific", EndOfRange: 10000}, + }, + }, + } + + featureMap := map[string]entities.Feature{ + "feature_1": {ID: "feature_1", Key: "feature_1"}, + "feature_2": {ID: "feature_2", Key: "feature_2"}, + } + + holdoutList, holdoutIDMap, flagHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) + + // Verify both holdouts are in the list + assert.Len(t, holdoutList, 2) + assert.Len(t, holdoutIDMap, 2) + + // feature_1: should get global holdout only (not excluded, not specifically included) + assert.Contains(t, flagHoldoutsMap, "feature_1") + assert.Len(t, flagHoldoutsMap["feature_1"], 1) + assert.Equal(t, "global_holdout", flagHoldoutsMap["feature_1"][0].Key) + + // feature_2: should get specific holdout only (excluded from global) + assert.Contains(t, flagHoldoutsMap, "feature_2") + assert.Len(t, flagHoldoutsMap["feature_2"], 1) + assert.Equal(t, "specific_holdout", flagHoldoutsMap["feature_2"][0].Key) +} + +func TestMapHoldoutsWithAudienceConditions(t *testing.T) { + rawHoldouts := []datafileEntities.Holdout{ + { + ID: "holdout_1", + Key: "holdout_with_audience", + Status: "Running", + AudienceIds: []string{"audience_1", "audience_2"}, + AudienceConditions: []interface{}{"or", "audience_1", "audience_2"}, + 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, _, _ := MapHoldouts(rawHoldouts, featureMap) + + // 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{ + { + ID: "holdout_1", + Key: "holdout_variations", + Status: "Running", + Variations: []datafileEntities.Variation{ + { + ID: "var_1", + Key: "variation_1", + FeatureEnabled: true, + Variables: []datafileEntities.VariationVariable{ + {ID: "var_var_1", Value: "value_1"}, + }, + }, + { + ID: "var_2", + Key: "variation_2", + FeatureEnabled: false, + }, + }, + TrafficAllocation: []datafileEntities.TrafficAllocation{ + {EntityID: "var_1", EndOfRange: 5000}, + {EntityID: "var_2", EndOfRange: 10000}, + }, + }, + } + + featureMap := map[string]entities.Feature{ + "feature_1": {ID: "feature_1", Key: "feature_1"}, + } + + holdoutList, _, _ := MapHoldouts(rawHoldouts, featureMap) + + // 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) + assert.Equal(t, "var_2", holdoutList[0].TrafficAllocation[1].EntityID) + assert.Equal(t, 10000, holdoutList[0].TrafficAllocation[1].EndOfRange) +} diff --git a/pkg/decision/holdout_service.go b/pkg/decision/holdout_service.go index b7da9a68..48fae2ad 100644 --- a/pkg/decision/holdout_service.go +++ b/pkg/decision/holdout_service.go @@ -102,7 +102,7 @@ func (h HoldoutService) GetDecision(decisionContext FeatureDecisionContext, user variation, _, _ := h.bucketer.Bucket(bucketingID, experimentForBucketing, entities.Group{}) if variation != nil { - reason := reasons.AddInfo("User %s is in variation %s of holdout %s.", userContext.ID, variation.Key, holdout.Key) + reason = reasons.AddInfo("User %s is in variation %s of holdout %s.", userContext.ID, variation.Key, holdout.Key) h.logger.Info(reason) featureDecision := FeatureDecision{ From 8a48ff56a0116c90cd66a9e8563a969cba733390 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Thu, 20 Nov 2025 22:01:14 -0800 Subject: [PATCH 5/5] fixed holdouts precendence order --- .../datafileprojectconfig/mappers/holdout.go | 18 ++--- .../mappers/holdout_test.go | 78 +++++++++++++++++++ 2 files changed, 85 insertions(+), 11 deletions(-) diff --git a/pkg/config/datafileprojectconfig/mappers/holdout.go b/pkg/config/datafileprojectconfig/mappers/holdout.go index 0a47bcf3..6108d04d 100644 --- a/pkg/config/datafileprojectconfig/mappers/holdout.go +++ b/pkg/config/datafileprojectconfig/mappers/holdout.go @@ -65,24 +65,20 @@ func MapHoldouts(holdouts []datafileEntities.Holdout, featureMap map[string]enti } // Build flagHoldoutsMap by combining global and specific holdouts + // Global holdouts take precedence (evaluated first), then specific holdouts for _, feature := range featureMap { flagKey := feature.Key flagID := feature.ID applicableHoldouts := []entities.Holdout{} - // Add specifically included holdouts - if included, exists := includedHoldouts[flagID]; exists { - applicableHoldouts = append(applicableHoldouts, included...) - } - - // Add global holdouts (if not excluded) - isExcluded := false - if _, exists := excludedHoldouts[flagID]; exists { - isExcluded = true + // Add global holdouts first (if not excluded) - they take precedence + if _, exists := excludedHoldouts[flagID]; !exists { + applicableHoldouts = append(applicableHoldouts, globalHoldouts...) } - if !isExcluded { - applicableHoldouts = append(applicableHoldouts, globalHoldouts...) + // Add specifically included holdouts second + if included, exists := includedHoldouts[flagID]; exists { + applicableHoldouts = append(applicableHoldouts, included...) } if len(applicableHoldouts) > 0 { diff --git a/pkg/config/datafileprojectconfig/mappers/holdout_test.go b/pkg/config/datafileprojectconfig/mappers/holdout_test.go index 6e0e7c88..7b192005 100644 --- a/pkg/config/datafileprojectconfig/mappers/holdout_test.go +++ b/pkg/config/datafileprojectconfig/mappers/holdout_test.go @@ -189,6 +189,84 @@ func TestMapHoldoutsMixed(t *testing.T) { assert.Equal(t, "specific_holdout", flagHoldoutsMap["feature_2"][0].Key) } +func TestMapHoldoutsPrecedence(t *testing.T) { + // Test that global holdouts take precedence over specific holdouts + // When both apply to the same flag, global should come first in the slice + rawHoldouts := []datafileEntities.Holdout{ + { + ID: "holdout_global", + Key: "global_holdout", + Status: "Running", + // No includedFlags = global, applies to all + Variations: []datafileEntities.Variation{ + {ID: "var_global", Key: "variation_global"}, + }, + TrafficAllocation: []datafileEntities.TrafficAllocation{ + {EntityID: "var_global", EndOfRange: 5000}, + }, + }, + { + ID: "holdout_specific", + Key: "specific_holdout", + Status: "Running", + IncludedFlags: []string{"feature_1"}, // Specific to feature_1 + Variations: []datafileEntities.Variation{ + {ID: "var_specific", Key: "variation_specific"}, + }, + TrafficAllocation: []datafileEntities.TrafficAllocation{ + {EntityID: "var_specific", EndOfRange: 10000}, + }, + }, + } + + featureMap := map[string]entities.Feature{ + "feature_1": {ID: "feature_1", Key: "feature_1"}, + } + + _, _, flagHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) + + // feature_1 should have BOTH holdouts, with global FIRST (precedence) + assert.Contains(t, flagHoldoutsMap, "feature_1") + assert.Len(t, flagHoldoutsMap["feature_1"], 2) + + // Global holdout should be first (takes precedence) + assert.Equal(t, "global_holdout", flagHoldoutsMap["feature_1"][0].Key, "Global holdout should take precedence (be first)") + // Specific holdout should be second + assert.Equal(t, "specific_holdout", flagHoldoutsMap["feature_1"][1].Key, "Specific holdout should be second") +} + +func TestMapHoldoutsExcludedFlagsNotInMap(t *testing.T) { + // Test that excluded flags do not get global holdouts + rawHoldouts := []datafileEntities.Holdout{ + { + ID: "holdout_global", + Key: "global_holdout", + Status: "Running", + ExcludedFlags: []string{"feature_excluded"}, + Variations: []datafileEntities.Variation{ + {ID: "var_1", Key: "variation_1"}, + }, + TrafficAllocation: []datafileEntities.TrafficAllocation{ + {EntityID: "var_1", EndOfRange: 10000}, + }, + }, + } + + featureMap := map[string]entities.Feature{ + "feature_included": {ID: "feature_included", Key: "feature_included"}, + "feature_excluded": {ID: "feature_excluded", Key: "feature_excluded"}, + } + + _, _, flagHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) + + // feature_included should have the global holdout + assert.Contains(t, flagHoldoutsMap, "feature_included") + assert.Len(t, flagHoldoutsMap["feature_included"], 1) + + // feature_excluded should NOT be in the map (no holdouts apply) + assert.NotContains(t, flagHoldoutsMap, "feature_excluded") +} + func TestMapHoldoutsWithAudienceConditions(t *testing.T) { rawHoldouts := []datafileEntities.Holdout{ {