From 46bfc6e53637be3d764d54c766500b528323eb24 Mon Sep 17 00:00:00 2001 From: Md Junaed Hossain <169046794+junaed-optimizely@users.noreply.github.com> Date: Mon, 22 Jun 2026 20:37:50 +0600 Subject: [PATCH] [AI-FSSDK] [FSSDK-12792] Add localHoldouts section to datafile for backward-compatible local holdout support --- OptimizelySDK.Tests/ProjectConfigTest.cs | 133 ++++++++++++++++++ .../TestData/HoldoutTestData.json | 4 +- OptimizelySDK/Bucketing/DecisionService.cs | 2 +- OptimizelySDK/Config/DatafileProjectConfig.cs | 67 +++++++-- OptimizelySDK/Entity/Holdout.cs | 9 +- OptimizelySDK/ProjectConfig.cs | 15 +- OptimizelySDK/Utils/HoldoutConfig.cs | 22 ++- 7 files changed, 217 insertions(+), 35 deletions(-) diff --git a/OptimizelySDK.Tests/ProjectConfigTest.cs b/OptimizelySDK.Tests/ProjectConfigTest.cs index 83f114c9..9388093f 100644 --- a/OptimizelySDK.Tests/ProjectConfigTest.cs +++ b/OptimizelySDK.Tests/ProjectConfigTest.cs @@ -1418,11 +1418,144 @@ public void TestMissingHoldoutsField_BackwardCompatibility() Assert.IsNotNull(datafileProjectConfig.Holdouts); Assert.AreEqual(0, datafileProjectConfig.Holdouts.Length); + Assert.IsNotNull(datafileProjectConfig.LocalHoldouts); + Assert.AreEqual(0, datafileProjectConfig.LocalHoldouts.Length); + // Methods should still work with empty holdouts var holdout = datafileProjectConfig.GetHoldout("any_id"); Assert.IsNull(holdout); } + [Test] + public void TestLocalHoldoutsSection_ParsedSeparatelyFromHoldouts() + { + // Test that localHoldouts are parsed from the new datafile section + var testDataPath = Path.Combine(TestContext.CurrentContext.TestDirectory, + "TestData", "HoldoutTestData.json"); + var jsonContent = File.ReadAllText(testDataPath); + var testData = JObject.Parse(jsonContent); + + var datafileJson = testData["datafileWithLocalHoldouts"].ToString(); + var datafileProjectConfig = DatafileProjectConfig.Create(datafileJson, + new NoOpLogger(), new NoOpErrorHandler()) as DatafileProjectConfig; + + // holdouts section has 1 global holdout + Assert.AreEqual(1, datafileProjectConfig.Holdouts.Length, + "holdouts section should contain only global holdouts"); + Assert.AreEqual("holdout_global_2", datafileProjectConfig.Holdouts[0].Id); + + // Global holdout should have IncludedRules stripped to null + Assert.IsNull(datafileProjectConfig.Holdouts[0].IncludedRules, + "Global holdout should have IncludedRules stripped to null"); + Assert.IsTrue(datafileProjectConfig.Holdouts[0].IsGlobal); + + // localHoldouts section has 4 local holdouts (all with valid includedRules) + Assert.AreEqual(4, datafileProjectConfig.LocalHoldouts.Length, + "localHoldouts section should contain local holdouts with valid includedRules"); + + // Verify each local holdout has IncludedRules set + foreach (var localHoldout in datafileProjectConfig.LocalHoldouts) + { + Assert.IsNotNull(localHoldout.IncludedRules, + $"Local holdout {localHoldout.Key} should have IncludedRules set"); + Assert.IsFalse(localHoldout.IsGlobal, + $"Local holdout {localHoldout.Key} should not be global"); + } + } + + [Test] + public void TestHoldoutsSection_IncludedRulesStrippedAtParseTime() + { + // Test that includedRules on entries in holdouts section is stripped. + // Even if the backend accidentally puts includedRules on a global holdout, + // the SDK ignores it because section membership is the sole signal. + var datafileWithIncludedRulesOnGlobal = @"{ + ""version"": ""4"", + ""rollouts"": [], + ""projectId"": ""test_project"", + ""experiments"": [], + ""groups"": [], + ""attributes"": [], + ""audiences"": [], + ""layers"": [], + ""events"": [], + ""revision"": ""1"", + ""featureFlags"": [], + ""holdouts"": [ + { + ""id"": ""ho_1"", + ""key"": ""holdout_with_stale_rules"", + ""status"": ""Running"", + ""variations"": [], + ""trafficAllocation"": [], + ""audienceIds"": [], + ""includedRules"": [""rule_123""] + } + ] + }"; + + var config = DatafileProjectConfig.Create(datafileWithIncludedRulesOnGlobal, + new NoOpLogger(), new NoOpErrorHandler()) as DatafileProjectConfig; + + Assert.AreEqual(1, config.Holdouts.Length); + Assert.IsNull(config.Holdouts[0].IncludedRules, + "includedRules on holdouts-section entries must be stripped at parse time"); + Assert.IsTrue(config.Holdouts[0].IsGlobal, + "Holdout from holdouts section must be treated as global"); + } + + [Test] + public void TestLocalHoldoutsWithoutIncludedRules_ExcludedAndLogged() + { + // Test that localHoldouts entries without includedRules are excluded + var datafileWithInvalidLocal = @"{ + ""version"": ""4"", + ""rollouts"": [], + ""projectId"": ""test_project"", + ""experiments"": [], + ""groups"": [], + ""attributes"": [], + ""audiences"": [], + ""layers"": [], + ""events"": [], + ""revision"": ""1"", + ""featureFlags"": [], + ""localHoldouts"": [ + { + ""id"": ""ho_valid"", + ""key"": ""valid_local"", + ""status"": ""Running"", + ""variations"": [], + ""trafficAllocation"": [], + ""audienceIds"": [], + ""includedRules"": [""rule_1""] + }, + { + ""id"": ""ho_invalid"", + ""key"": ""invalid_local_no_rules"", + ""status"": ""Running"", + ""variations"": [], + ""trafficAllocation"": [], + ""audienceIds"": [] + } + ] + }"; + + var loggerMock = new Mock(); + var config = DatafileProjectConfig.Create(datafileWithInvalidLocal, + loggerMock.Object, new NoOpErrorHandler()) as DatafileProjectConfig; + + // Only the valid local holdout should remain + Assert.AreEqual(1, config.LocalHoldouts.Length, + "Only valid local holdouts with includedRules should be kept"); + Assert.AreEqual("ho_valid", config.LocalHoldouts[0].Id); + + // Verify error was logged for the invalid entry + loggerMock.Verify(l => l.Log(LogLevel.ERROR, + It.Is(s => s.Contains("invalid_local_no_rules") && s.Contains("no includedRules"))), + Times.Once); + } + #endregion [Test] diff --git a/OptimizelySDK.Tests/TestData/HoldoutTestData.json b/OptimizelySDK.Tests/TestData/HoldoutTestData.json index eb65ccc6..41a02e00 100644 --- a/OptimizelySDK.Tests/TestData/HoldoutTestData.json +++ b/OptimizelySDK.Tests/TestData/HoldoutTestData.json @@ -391,7 +391,9 @@ ], "audienceIds": [], "audienceConditions": [] - }, + } + ], + "localHoldouts": [ { "id": "holdout_local_rule1", "key": "local_holdout_rule1", diff --git a/OptimizelySDK/Bucketing/DecisionService.cs b/OptimizelySDK/Bucketing/DecisionService.cs index a287bc59..e5bd05fe 100644 --- a/OptimizelySDK/Bucketing/DecisionService.cs +++ b/OptimizelySDK/Bucketing/DecisionService.cs @@ -916,7 +916,7 @@ public virtual Result GetDecisionForFlag( var userId = user.GetUserId(); - // Check global holdouts first (highest priority — evaluated at flag level, before any rules) + // Check global holdouts first (from holdouts section — evaluated at flag level, before any rules) var globalHoldouts = projectConfig.GetGlobalHoldouts(); foreach (var holdout in globalHoldouts) { diff --git a/OptimizelySDK/Config/DatafileProjectConfig.cs b/OptimizelySDK/Config/DatafileProjectConfig.cs index 4bf9a00c..e4d02b8f 100644 --- a/OptimizelySDK/Config/DatafileProjectConfig.cs +++ b/OptimizelySDK/Config/DatafileProjectConfig.cs @@ -299,10 +299,17 @@ private Dictionary> _VariationIdMap public Rollout[] Rollouts { get; set; } /// - /// Associative list of Holdouts. + /// Global holdouts from the datafile "holdouts" section. + /// All entries are treated as global; any includedRules is stripped at parse time. /// public Holdout[] Holdouts { get; set; } + /// + /// Local holdouts from the datafile "localHoldouts" section. + /// Each entry must have includedRules; entries without it are invalid and excluded at parse time. + /// + public Holdout[] LocalHoldouts { get; set; } + /// /// Associative list of Integrations. /// @@ -327,6 +334,7 @@ private void Initialize() FeatureFlags = FeatureFlags ?? new FeatureFlag[0]; Rollouts = Rollouts ?? new Rollout[0]; Holdouts = Holdouts ?? new Holdout[0]; + LocalHoldouts = LocalHoldouts ?? new Holdout[0]; Integrations = Integrations ?? new Integration[0]; _ExperimentKeyMap = new Dictionary(); @@ -422,11 +430,16 @@ private void Initialize() } } - // Adding Holdout variations in variation id and key maps. + // Process holdouts section: entries are global by section membership. + // Strip any includedRules so they are treated as global holdouts. if (Holdouts != null) { foreach (var holdout in Holdouts) { + // Section membership is the sole signal for scope. + // Strip includedRules from holdouts-section entries at parse time. + holdout.IncludedRules = null; + _VariationKeyMap[holdout.Key] = new Dictionary(); _VariationIdMap[holdout.Key] = new Dictionary(); _VariationIdMapByExperimentId[holdout.Id] = new Dictionary(); @@ -445,6 +458,43 @@ private void Initialize() } } + // Process localHoldouts section: entries must have includedRules. + // Validate and skip entries without includedRules (log error). + var validLocalHoldouts = new List(); + if (LocalHoldouts != null) + { + foreach (var localHoldout in LocalHoldouts) + { + if (localHoldout.IncludedRules == null) + { + Logger.Log(LogLevel.ERROR, + $"Local holdout \"{localHoldout.Key}\" (id: {localHoldout.Id}) in localHoldouts section has no includedRules. Excluding from evaluation."); + continue; + } + + validLocalHoldouts.Add(localHoldout); + + _VariationKeyMap[localHoldout.Key] = new Dictionary(); + _VariationIdMap[localHoldout.Key] = new Dictionary(); + _VariationIdMapByExperimentId[localHoldout.Id] = new Dictionary(); + _VariationKeyMapByExperimentId[localHoldout.Id] = new Dictionary(); + + if (localHoldout.Variations != null) + { + foreach (var variation in localHoldout.Variations) + { + _VariationKeyMap[localHoldout.Key][variation.Key] = variation; + _VariationIdMap[localHoldout.Key][variation.Id] = variation; + _VariationKeyMapByExperimentId[localHoldout.Id][variation.Key] = variation; + _VariationIdMapByExperimentId[localHoldout.Id][variation.Id] = variation; + } + } + } + } + + // Store only valid local holdouts + LocalHoldouts = validLocalHoldouts.ToArray(); + // Inject "everyone else" variation into feature_rollout experiments foreach (var feature in FeatureFlags) { @@ -561,8 +611,11 @@ private void Initialize() _FlagVariationMap = flagToVariationsMap; - // Initialize HoldoutConfig for managing flag-to-holdout relationships - _holdoutConfig = new HoldoutConfig(Holdouts ?? new Holdout[0]); + // Initialize HoldoutConfig with global holdouts (from holdouts section) + // and local holdouts (from localHoldouts section, validated above). + var allHoldoutsForConfig = (Holdouts ?? new Holdout[0]) + .Concat(LocalHoldouts ?? new Holdout[0]).ToArray(); + _holdoutConfig = new HoldoutConfig(allHoldoutsForConfig); } /// @@ -910,8 +963,7 @@ public Holdout GetHoldout(string holdoutId) } /// - /// Returns all global holdouts (holdouts where IncludedRules is null). - /// Global holdouts apply to all rules across all flags and are evaluated at flag level. + /// Returns all global holdouts (from holdouts section, evaluated at flag level). /// /// Read-only list of global holdouts public List GetGlobalHoldouts() @@ -920,8 +972,7 @@ public List GetGlobalHoldouts() } /// - /// Returns local holdouts that target a specific rule ID. - /// Local holdouts are evaluated per-rule, after forced decisions but before regular rule evaluation. + /// Returns local holdouts (from localHoldouts section) targeting a specific rule ID. /// /// The rule ID to look up holdouts for /// Read-only list of local holdouts targeting the given rule, or empty list if none diff --git a/OptimizelySDK/Entity/Holdout.cs b/OptimizelySDK/Entity/Holdout.cs index 7ed44835..b8bfba31 100644 --- a/OptimizelySDK/Entity/Holdout.cs +++ b/OptimizelySDK/Entity/Holdout.cs @@ -47,16 +47,13 @@ public override string LayerId } /// - /// Optional array of rule IDs that this holdout targets (local holdout). - /// When null, the holdout applies to all rules across all flags (global holdout). - /// When set to an array (even empty), the holdout only applies to the specified rules. - /// Rule IDs in this array are experiment/delivery rule IDs from the datafile, NOT flag IDs. + /// Rule IDs this holdout targets. Scope is determined by datafile section membership: + /// holdouts section entries have this stripped to null; localHoldouts entries must have it set. /// public string[] IncludedRules { get; set; } /// - /// Returns true if this is a global holdout (IncludedRules is null), - /// false if this is a local holdout (IncludedRules is a non-null array). + /// True if global (from holdouts section, IncludedRules is null at parse time). /// public bool IsGlobal => IncludedRules == null; } diff --git a/OptimizelySDK/ProjectConfig.cs b/OptimizelySDK/ProjectConfig.cs index e0321190..5528f990 100644 --- a/OptimizelySDK/ProjectConfig.cs +++ b/OptimizelySDK/ProjectConfig.cs @@ -181,10 +181,17 @@ public interface ProjectConfig Rollout[] Rollouts { get; set; } /// - /// Associative list of Holdouts. + /// Global holdouts from the datafile "holdouts" section. + /// All entries are treated as global; any includedRules on these entries is stripped at parse time. /// Holdout[] Holdouts { get; set; } + /// + /// Local holdouts from the datafile "localHoldouts" section. + /// Each entry must have includedRules; entries without it are invalid and excluded at parse time. + /// + Holdout[] LocalHoldouts { get; set; } + /// /// Associative list of Integrations. /// @@ -333,15 +340,13 @@ public interface ProjectConfig Holdout GetHoldout(string holdoutId); /// - /// Returns all global holdouts (holdouts where IncludedRules is null). - /// Global holdouts apply to all rules across all flags and are evaluated at flag level. + /// Returns all global holdouts (from holdouts section, evaluated at flag level). /// /// Read-only list of global holdouts List GetGlobalHoldouts(); /// - /// Returns local holdouts that target a specific rule ID. - /// Local holdouts are evaluated per-rule, after forced decisions but before regular rule evaluation. + /// Returns local holdouts (from localHoldouts section) targeting a specific rule ID. /// /// The rule ID to look up holdouts for /// Read-only list of local holdouts targeting the given rule, or empty list if none diff --git a/OptimizelySDK/Utils/HoldoutConfig.cs b/OptimizelySDK/Utils/HoldoutConfig.cs index a8c888fe..641cc90d 100644 --- a/OptimizelySDK/Utils/HoldoutConfig.cs +++ b/OptimizelySDK/Utils/HoldoutConfig.cs @@ -21,8 +21,8 @@ namespace OptimizelySDK.Utils { /// - /// Configuration manager for holdouts, providing holdout ID mapping, - /// global holdout access, and rule-level local holdout lookups. + /// Manages holdout ID mapping, global holdout access, and rule-level local holdout lookups. + /// Scope is determined by datafile section: holdouts = global, localHoldouts = rule-scoped. /// public class HoldoutConfig { @@ -30,14 +30,12 @@ public class HoldoutConfig private readonly Dictionary _holdoutIdMap; /// - /// Global holdouts — holdouts where IncludedRules is null. - /// These are evaluated at flag level, before any per-rule logic. + /// Global holdouts (from holdouts section; IncludedRules is null). /// private readonly List _globalHoldouts; /// - /// Maps rule IDs to the local holdouts that target those rules. - /// A local holdout has IncludedRules set to a non-null array of rule IDs. + /// Maps rule IDs to the local holdouts (from localHoldouts section) that target those rules. /// private readonly Dictionary> _ruleHoldoutsMap; @@ -75,16 +73,14 @@ private void UpdateHoldoutMapping() // Build ID mapping _holdoutIdMap[holdout.Id] = holdout; - // Classify as global or local based on IncludedRules + // Classify by IncludedRules: null = global (holdouts section), + // non-null = local (localHoldouts section). if (holdout.IsGlobal) { - // IncludedRules == null → global holdout (applies to all rules across all flags) _globalHoldouts.Add(holdout); } else { - // IncludedRules != null → local holdout (applies only to the specified rules) - // An empty IncludedRules array means local but matches no rules (intentional). foreach (var ruleId in holdout.IncludedRules) { if (!_ruleHoldoutsMap.ContainsKey(ruleId)) @@ -116,8 +112,7 @@ public Holdout GetHoldout(string holdoutId) } /// - /// Returns all global holdouts (holdouts where IncludedRules is null). - /// These apply to all rules across all flags and are evaluated at flag level. + /// Returns all global holdouts (from holdouts section, evaluated at flag level). /// /// List of global holdouts public List GetGlobalHoldouts() @@ -126,8 +121,7 @@ public List GetGlobalHoldouts() } /// - /// Returns local holdouts that target a specific rule ID. - /// These are evaluated per-rule, after forced decisions but before regular rule evaluation. + /// Returns local holdouts (from localHoldouts section) targeting a specific rule ID. /// /// The rule ID to look up holdouts for /// List of holdouts targeting the specified rule, or an empty list if none