diff --git a/OptimizelySDK.Tests/LocalHoldoutsSectionTest.cs b/OptimizelySDK.Tests/LocalHoldoutsSectionTest.cs new file mode 100644 index 00000000..1f79aaeb --- /dev/null +++ b/OptimizelySDK.Tests/LocalHoldoutsSectionTest.cs @@ -0,0 +1,342 @@ +/* + * Copyright 2026, Optimizely + * + * 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 + * + * https://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. + */ + +using System.IO; +using System.Linq; +using Moq; +using Newtonsoft.Json.Linq; +using NUnit.Framework; +using OptimizelySDK.Config; +using OptimizelySDK.Entity; +using OptimizelySDK.ErrorHandler; +using OptimizelySDK.Logger; + +namespace OptimizelySDK.Tests +{ + /// + /// Tests for the 'localHoldouts' datafile section: section-based scoping, validation, and backward compat. + /// + [TestFixture] + public class LocalHoldoutsSectionTest + { + private Mock LoggerMock; + private JObject TestData; + + [SetUp] + public void Initialize() + { + LoggerMock = new Mock(); + + var testDataPath = Path.Combine(TestContext.CurrentContext.TestDirectory, + "TestData", "HoldoutTestData.json"); + var jsonContent = File.ReadAllText(testDataPath); + TestData = JObject.Parse(jsonContent); + } + + private DatafileProjectConfig BuildConfig(string fixtureName) + { + var datafile = TestData[fixtureName].ToString(); + return DatafileProjectConfig.Create(datafile, LoggerMock.Object, + new NoOpErrorHandler()) as DatafileProjectConfig; + } + + // ----------------------------------------------------------------------- + // localHoldouts section parsing — happy path + // ----------------------------------------------------------------------- + + [Test] + public void TestLocalHoldoutsSection_ExposedAsTopLevelProperty() + { + var config = BuildConfig("datafileWithLocalHoldoutsSection"); + + Assert.IsNotNull(config.LocalHoldouts, + "LocalHoldouts property should be exposed on DatafileProjectConfig"); + Assert.AreEqual(1, config.LocalHoldouts.Length, + "LocalHoldouts should contain only valid entries (invalid ones are excluded at parse time)"); + } + + [Test] + public void TestLocalHoldoutsSection_RegistersInRuleMap() + { + // Entries in 'localHoldouts' must be registered under each rule in IncludedRules. + var config = BuildConfig("datafileWithLocalHoldoutsSection"); + + var ruleAHoldouts = config.GetHoldoutsForRule("rule_a"); + Assert.AreEqual(1, ruleAHoldouts.Count, + "rule_a should be targeted by exactly one local holdout"); + Assert.AreEqual("section_local_1", ruleAHoldouts[0].Id, + "rule_a should be targeted by section_local_1"); + } + + [Test] + public void TestLocalHoldoutsSection_EntriesExcludedFromGlobalList() + { + // Entries in 'localHoldouts' must NOT appear in GetGlobalHoldouts(). + var config = BuildConfig("datafileWithLocalHoldoutsSection"); + + var globals = config.GetGlobalHoldouts(); + Assert.IsFalse(globals.Any(h => h.Id == "section_local_1"), + "Local holdout must not appear in the global holdouts list"); + } + + [Test] + public void TestLocalHoldoutsSection_LocalEntryIsRetrievableById() + { + // Entries from both sections must be tracked in the unified holdout id map. + var config = BuildConfig("datafileWithLocalHoldoutsSection"); + + Assert.IsNotNull(config.GetHoldout("section_global_1"), + "Global-section holdout should be retrievable by ID"); + Assert.IsNotNull(config.GetHoldout("section_local_1"), + "Local-section holdout should be retrievable by ID"); + } + + // ----------------------------------------------------------------------- + // localHoldouts section — variation maps must include local holdouts + // ----------------------------------------------------------------------- + + [Test] + public void TestLocalHoldoutsSection_VariationsRegisteredInVariationMaps() + { + // The decision service resolves variations by holdout key, so local holdouts' + // variations must be present in the variation maps. + var config = BuildConfig("datafileWithLocalHoldoutsSection"); + + Assert.IsTrue(config.VariationKeyMap.ContainsKey("section_local_holdout_1"), + "Local holdout key should be present in VariationKeyMap"); + Assert.IsTrue(config.VariationKeyMap["section_local_holdout_1"] + .ContainsKey("section_local_off"), + "Local holdout's variation key should be registered"); + } + + // ----------------------------------------------------------------------- + // Section partition — IncludedRules on global-section entries is ignored + // ----------------------------------------------------------------------- + + [Test] + public void TestGlobalSection_IncludedRulesOnEntryIsStrippedAtParseTime() + { + // Any IncludedRules field on a 'holdouts' entry must NOT narrow its scope. + // Section membership in 'holdouts' alone determines global scope. + var config = BuildConfig("datafileWithLocalHoldoutsSection"); + + var strayHoldout = config.GetHoldout("section_global_with_stray_rules"); + Assert.IsNotNull(strayHoldout, + "Global-section holdout with stray includedRules should still be loaded"); + + Assert.IsNull(strayHoldout.IncludedRules, + "IncludedRules on a global-section entry must be stripped at parse time"); + Assert.IsTrue(strayHoldout.IsGlobal, + "Global-section entry must always classify as global, regardless of source IncludedRules"); + + // Must NOT appear under the rule it spuriously listed + var holdoutsForStrayRule = config.GetHoldoutsForRule("should_be_ignored_rule"); + Assert.AreEqual(0, holdoutsForStrayRule.Count, + "Stray includedRules on a global-section entry must not place it in the rule map"); + + // Must appear in the global list + var globals = config.GetGlobalHoldouts(); + Assert.IsTrue(globals.Any(h => h.Id == "section_global_with_stray_rules"), + "Global-section entry with stray includedRules must still be in the global list"); + } + + // ----------------------------------------------------------------------- + // Invalid local holdouts — missing IncludedRules is logged and excluded + // ----------------------------------------------------------------------- + + [Test] + public void TestLocalHoldoutsSection_MissingIncludedRulesIsExcluded() + { + // Entries in 'localHoldouts' without IncludedRules are invalid per spec. + // SDK must exclude them from evaluation. It must NOT fall back to global application. + var config = BuildConfig("datafileWithLocalHoldoutsSection"); + + // Invalid entry must not be retrievable by ID (excluded from holdout map). + Assert.IsNull(config.GetHoldout("section_local_invalid"), + "Invalid local holdout (missing IncludedRules) must be excluded from holdout map"); + + // Invalid entry must not be applied as global. + var globals = config.GetGlobalHoldouts(); + Assert.IsFalse(globals.Any(h => h.Id == "section_local_invalid"), + "Invalid local holdout must NOT fall back to global application"); + } + + [Test] + public void TestLocalHoldoutsSection_MissingIncludedRulesLogsError() + { + // Verify an error log is emitted for an invalid local holdout entry. + BuildConfig("datafileWithLocalHoldoutsSection"); + + LoggerMock.Verify( + l => l.Log(LogLevel.ERROR, + It.Is(msg => + msg.Contains("section_local_invalid_key") && + msg.Contains("includedRules"))), + Times.AtLeastOnce(), + "Expected an error log mentioning the invalid local holdout's key and includedRules"); + } + + // ----------------------------------------------------------------------- + // Backward compatibility — datafiles without 'localHoldouts' section + // ----------------------------------------------------------------------- + + [Test] + public void TestBackwardCompat_DatafileWithoutLocalHoldoutsSection() + { + // Old datafiles that only emit the 'holdouts' section must continue to work. + // Every entry is global; LocalHoldouts defaults to empty array. + var config = BuildConfig("datafileWithHoldouts"); + + Assert.IsNotNull(config.LocalHoldouts, + "LocalHoldouts must default to an empty array when the section is absent"); + Assert.AreEqual(0, config.LocalHoldouts.Length, + "Missing 'localHoldouts' key should result in an empty LocalHoldouts array"); + + // The 'holdouts' entries are still loaded as global, and no errors are produced. + Assert.IsTrue(config.GetGlobalHoldouts().Count > 0, + "Global holdouts from the 'holdouts' section should still be loaded"); + } + + // ----------------------------------------------------------------------- + // Direct DatafileProjectConfig.Create with minimal inline datafiles + // ----------------------------------------------------------------------- + + [Test] + public void TestBackwardCompat_DatafileMissingBothHoldoutSections() + { + // Datafile that emits neither 'holdouts' nor 'localHoldouts' must produce empty lists. + var datafile = @"{ + ""version"": ""4"", + ""projectId"": ""p1"", + ""accountId"": ""a1"", + ""revision"": ""1"", + ""experiments"": [], + ""groups"": [], + ""attributes"": [], + ""audiences"": [], + ""events"": [], + ""featureFlags"": [], + ""rollouts"": [], + ""anonymizeIP"": false + }"; + + var config = DatafileProjectConfig.Create(datafile, LoggerMock.Object, + new NoOpErrorHandler()) as DatafileProjectConfig; + + Assert.IsNotNull(config); + Assert.IsNotNull(config.Holdouts); + Assert.AreEqual(0, config.Holdouts.Length); + Assert.IsNotNull(config.LocalHoldouts); + Assert.AreEqual(0, config.LocalHoldouts.Length); + Assert.AreEqual(0, config.GetGlobalHoldouts().Count); + Assert.AreEqual(0, config.GetHoldoutsForRule("any_rule").Count); + } + + [Test] + public void TestBothSectionsPartitionCorrectly() + { + // When both 'holdouts' and 'localHoldouts' are present, scope is enforced by + // section membership — entries never cross over. + var datafile = @"{ + ""version"": ""4"", + ""projectId"": ""p1"", + ""accountId"": ""a1"", + ""revision"": ""1"", + ""experiments"": [], + ""groups"": [], + ""attributes"": [], + ""audiences"": [], + ""events"": [], + ""featureFlags"": [], + ""rollouts"": [], + ""anonymizeIP"": false, + ""holdouts"": [ + {""id"": ""g1"", ""key"": ""g1k"", ""status"": ""Running"", + ""variations"": [], ""trafficAllocation"": [], + ""audienceIds"": [], ""audienceConditions"": []}, + {""id"": ""g2"", ""key"": ""g2k"", ""status"": ""Running"", + ""variations"": [], ""trafficAllocation"": [], + ""audienceIds"": [], ""audienceConditions"": []} + ], + ""localHoldouts"": [ + {""id"": ""l1"", ""key"": ""l1k"", ""status"": ""Running"", + ""variations"": [], ""trafficAllocation"": [], + ""audienceIds"": [], ""audienceConditions"": [], + ""includedRules"": [""rule_a""]}, + {""id"": ""l2"", ""key"": ""l2k"", ""status"": ""Running"", + ""variations"": [], ""trafficAllocation"": [], + ""audienceIds"": [], ""audienceConditions"": [], + ""includedRules"": [""rule_b""]} + ] + }"; + + var config = DatafileProjectConfig.Create(datafile, LoggerMock.Object, + new NoOpErrorHandler()) as DatafileProjectConfig; + + var globalIds = config.GetGlobalHoldouts().Select(h => h.Id).OrderBy(s => s).ToArray(); + CollectionAssert.AreEqual(new[] { "g1", "g2" }, globalIds, + "Global section entries must be exactly the 'holdouts' entries"); + + Assert.AreEqual(1, config.GetHoldoutsForRule("rule_a").Count); + Assert.AreEqual("l1", config.GetHoldoutsForRule("rule_a")[0].Id); + Assert.AreEqual(1, config.GetHoldoutsForRule("rule_b").Count); + Assert.AreEqual("l2", config.GetHoldoutsForRule("rule_b")[0].Id); + } + + [Test] + public void TestLocalSection_EmptyIncludedRulesIsValid_TargetsNoRules() + { + // IncludedRules == [] is valid (entity is stored), but targets no rules. + // Not invalid, not global — matches existing entity-level semantics where [] != null. + var datafile = @"{ + ""version"": ""4"", + ""projectId"": ""p1"", + ""accountId"": ""a1"", + ""revision"": ""1"", + ""experiments"": [], + ""groups"": [], + ""attributes"": [], + ""audiences"": [], + ""events"": [], + ""featureFlags"": [], + ""rollouts"": [], + ""anonymizeIP"": false, + ""localHoldouts"": [ + {""id"": ""l_empty"", ""key"": ""l_empty_k"", ""status"": ""Running"", + ""variations"": [], ""trafficAllocation"": [], + ""audienceIds"": [], ""audienceConditions"": [], + ""includedRules"": []} + ] + }"; + + var config = DatafileProjectConfig.Create(datafile, LoggerMock.Object, + new NoOpErrorHandler()) as DatafileProjectConfig; + + // Stored (valid) — retrievable by id + var stored = config.GetHoldout("l_empty"); + Assert.IsNotNull(stored, "Local holdout with empty IncludedRules must still be stored"); + Assert.IsFalse(stored.IsGlobal, "Empty IncludedRules is local, not global"); + + // Not in any rule map + Assert.AreEqual(0, config.GetHoldoutsForRule("any_rule").Count, + "Empty IncludedRules must match no rules"); + + // Not global + Assert.AreEqual(0, config.GetGlobalHoldouts().Count, + "Local holdout with empty IncludedRules must not be promoted to global"); + } + } +} diff --git a/OptimizelySDK.Tests/OptimizelySDK.Tests.csproj b/OptimizelySDK.Tests/OptimizelySDK.Tests.csproj index 8aed9432..98db5acf 100644 --- a/OptimizelySDK.Tests/OptimizelySDK.Tests.csproj +++ b/OptimizelySDK.Tests/OptimizelySDK.Tests.csproj @@ -115,6 +115,7 @@ + diff --git a/OptimizelySDK.Tests/TestData/HoldoutTestData.json b/OptimizelySDK.Tests/TestData/HoldoutTestData.json index eb65ccc6..2c440ed8 100644 --- a/OptimizelySDK.Tests/TestData/HoldoutTestData.json +++ b/OptimizelySDK.Tests/TestData/HoldoutTestData.json @@ -211,6 +211,123 @@ } ] }, + "datafileWithLocalHoldoutsSection": { + "version": "4", + "rollouts": [], + "projectId": "test_project", + "experiments": [], + "groups": [], + "attributes": [], + "audiences": [], + "layers": [], + "events": [], + "revision": "1", + "accountId": "12345", + "anonymizeIP": false, + "featureFlags": [ + { + "id": "flag_1", + "key": "test_flag_1", + "experimentIds": [], + "rolloutId": "", + "variables": [] + } + ], + "holdouts": [ + { + "id": "section_global_1", + "key": "section_global_holdout_1", + "status": "Running", + "layerId": "layer_sg1", + "variations": [ + { + "id": "sgvar_1", + "key": "section_global_off", + "featureEnabled": false, + "variables": [] + } + ], + "trafficAllocation": [ + { + "entityId": "sgvar_1", + "endOfRange": 10000 + } + ], + "audienceIds": [], + "audienceConditions": [] + }, + { + "id": "section_global_with_stray_rules", + "key": "section_global_with_stray_rules_key", + "status": "Running", + "layerId": "layer_sg_stray", + "variations": [ + { + "id": "sgstray_var_1", + "key": "section_global_stray_off", + "featureEnabled": false, + "variables": [] + } + ], + "trafficAllocation": [ + { + "entityId": "sgstray_var_1", + "endOfRange": 10000 + } + ], + "audienceIds": [], + "audienceConditions": [], + "includedRules": ["should_be_ignored_rule"] + } + ], + "localHoldouts": [ + { + "id": "section_local_1", + "key": "section_local_holdout_1", + "status": "Running", + "layerId": "layer_sl1", + "variations": [ + { + "id": "slvar_1", + "key": "section_local_off", + "featureEnabled": false, + "variables": [] + } + ], + "trafficAllocation": [ + { + "entityId": "slvar_1", + "endOfRange": 10000 + } + ], + "audienceIds": [], + "audienceConditions": [], + "includedRules": ["rule_a"] + }, + { + "id": "section_local_invalid", + "key": "section_local_invalid_key", + "status": "Running", + "layerId": "layer_sl_invalid", + "variations": [ + { + "id": "slinvalid_var_1", + "key": "section_local_invalid_off", + "featureEnabled": false, + "variables": [] + } + ], + "trafficAllocation": [ + { + "entityId": "slinvalid_var_1", + "endOfRange": 10000 + } + ], + "audienceIds": [], + "audienceConditions": [] + } + ] + }, "datafileWithLocalHoldouts": { "version": "4", "rollouts": [ @@ -391,7 +508,9 @@ ], "audienceIds": [], "audienceConditions": [] - }, + } + ], + "localHoldouts": [ { "id": "holdout_local_rule1", "key": "local_holdout_rule1", diff --git a/OptimizelySDK/Config/DatafileProjectConfig.cs b/OptimizelySDK/Config/DatafileProjectConfig.cs index 4bf9a00c..18591a14 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 'holdouts' datafile section). IncludedRules is stripped at parse time. /// public Holdout[] Holdouts { get; set; } + /// + /// Local holdouts (from the 'localHoldouts' datafile section). Rule-scoped via IncludedRules. + /// Entries missing IncludedRules are excluded at parse time. Filtered to valid-only after parsing. + /// + [JsonProperty("localHoldouts")] + 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,25 +430,24 @@ private void Initialize() } } - // Adding Holdout variations in variation id and key maps. - if (Holdouts != null) + // Holdout variations from both sections need variation maps for the decision service. + var allHoldoutsForVariationMaps = (Holdouts ?? new Holdout[0]) + .Concat(LocalHoldouts ?? new Holdout[0]); + foreach (var holdout in allHoldoutsForVariationMaps) { - foreach (var holdout in Holdouts) - { - _VariationKeyMap[holdout.Key] = new Dictionary(); - _VariationIdMap[holdout.Key] = new Dictionary(); - _VariationIdMapByExperimentId[holdout.Id] = new Dictionary(); - _VariationKeyMapByExperimentId[holdout.Id] = new Dictionary(); + _VariationKeyMap[holdout.Key] = new Dictionary(); + _VariationIdMap[holdout.Key] = new Dictionary(); + _VariationIdMapByExperimentId[holdout.Id] = new Dictionary(); + _VariationKeyMapByExperimentId[holdout.Id] = new Dictionary(); - if (holdout.Variations != null) + if (holdout.Variations != null) + { + foreach (var variation in holdout.Variations) { - foreach (var variation in holdout.Variations) - { - _VariationKeyMap[holdout.Key][variation.Key] = variation; - _VariationIdMap[holdout.Key][variation.Id] = variation; - _VariationKeyMapByExperimentId[holdout.Id][variation.Key] = variation; - _VariationIdMapByExperimentId[holdout.Id][variation.Id] = variation; - } + _VariationKeyMap[holdout.Key][variation.Key] = variation; + _VariationIdMap[holdout.Key][variation.Id] = variation; + _VariationKeyMapByExperimentId[holdout.Id][variation.Key] = variation; + _VariationIdMapByExperimentId[holdout.Id][variation.Id] = variation; } } } @@ -561,8 +568,50 @@ private void Initialize() _FlagVariationMap = flagToVariationsMap; - // Initialize HoldoutConfig for managing flag-to-holdout relationships - _holdoutConfig = new HoldoutConfig(Holdouts ?? new Holdout[0]); + _holdoutConfig = new HoldoutConfig(BuildCombinedHoldouts()); + } + + /// + /// Merges global and local holdout sections. Strips IncludedRules from globals, + /// validates locals (null IncludedRules → error + exclude), updates LocalHoldouts to valid-only. + /// + private Holdout[] BuildCombinedHoldouts() + { + var combined = new List(); + + foreach (var holdout in Holdouts ?? new Holdout[0]) + { + if (holdout == null) + { + continue; + } + + holdout.IncludedRules = null; + combined.Add(holdout); + } + + var validLocal = new List(); + foreach (var holdout in LocalHoldouts ?? new Holdout[0]) + { + if (holdout == null) + { + continue; + } + + if (holdout.IncludedRules == null) + { + var identifier = !string.IsNullOrEmpty(holdout.Key) ? holdout.Key : holdout.Id; + Logger?.Log(LogLevel.ERROR, + $"Local holdout \"{identifier}\" is missing required \"includedRules\" field and will be excluded from evaluation."); + continue; + } + + validLocal.Add(holdout); + combined.Add(holdout); + } + + LocalHoldouts = validLocal.ToArray(); + return combined.ToArray(); } /// @@ -910,8 +959,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 the 'holdouts' datafile section). /// /// Read-only list of global holdouts public List GetGlobalHoldouts() @@ -920,8 +968,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 the given 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..96a33922 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. Null for global holdouts (stripped at parse time). /// 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 when global (IncludedRules is null). Consistent with section membership + /// because the config parser strips IncludedRules on 'holdouts'-section entries. /// public bool IsGlobal => IncludedRules == null; } diff --git a/OptimizelySDK/ProjectConfig.cs b/OptimizelySDK/ProjectConfig.cs index e0321190..143f368d 100644 --- a/OptimizelySDK/ProjectConfig.cs +++ b/OptimizelySDK/ProjectConfig.cs @@ -181,10 +181,15 @@ public interface ProjectConfig Rollout[] Rollouts { get; set; } /// - /// Associative list of Holdouts. + /// Global holdouts (from the 'holdouts' datafile section). /// Holdout[] Holdouts { get; set; } + /// + /// Local holdouts (from the 'localHoldouts' datafile section). Rule-scoped via IncludedRules. + /// + Holdout[] LocalHoldouts { get; set; } + /// /// Associative list of Integrations. ///