Skip to content

[AI-FSSDK] [FSSDK-12792] Add localHoldouts section to datafile #413

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
junaed-optimizely wants to merge 1 commit into from
Closed

[AI-FSSDK] [FSSDK-12792] Add localHoldouts section to datafile #413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 133 additions & 0 deletions OptimizelySDK.Tests/ProjectConfigTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<ILogger>();
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<string>(s => s.Contains("invalid_local_no_rules") && s.Contains("no includedRules"))),
Times.Once);
}

#endregion

[Test]
Expand Down
4 changes: 3 additions & 1 deletion OptimizelySDK.Tests/TestData/HoldoutTestData.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,9 @@
],
"audienceIds": [],
"audienceConditions": []
},
}
],
"localHoldouts": [
{
"id": "holdout_local_rule1",
"key": "local_holdout_rule1",
Expand Down
2 changes: 1 addition & 1 deletion OptimizelySDK/Bucketing/DecisionService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -916,7 +916,7 @@ public virtual Result<FeatureDecision> 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)
{
Expand Down
67 changes: 59 additions & 8 deletions OptimizelySDK/Config/DatafileProjectConfig.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,10 +299,17 @@ private Dictionary<string, Dictionary<string, Variation>> _VariationIdMap
public Rollout[] Rollouts { get; set; }

/// <summary>
/// Associative list of Holdouts.
/// Global holdouts from the datafile "holdouts" section.
/// All entries are treated as global; any includedRules is stripped at parse time.
/// </summary>
public Holdout[] Holdouts { get; set; }

/// <summary>
/// Local holdouts from the datafile "localHoldouts" section.
/// Each entry must have includedRules; entries without it are invalid and excluded at parse time.
/// </summary>
public Holdout[] LocalHoldouts { get; set; }

/// <summary>
/// Associative list of Integrations.
/// </summary>
Expand All @@ -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<string, Experiment>();

Expand Down Expand Up @@ -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<string, Variation>();
_VariationIdMap[holdout.Key] = new Dictionary<string, Variation>();
_VariationIdMapByExperimentId[holdout.Id] = new Dictionary<string, Variation>();
Expand All @@ -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<Holdout>();
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<string, Variation>();
_VariationIdMap[localHoldout.Key] = new Dictionary<string, Variation>();
_VariationIdMapByExperimentId[localHoldout.Id] = new Dictionary<string, Variation>();
_VariationKeyMapByExperimentId[localHoldout.Id] = new Dictionary<string, Variation>();

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)
{
Expand Down Expand Up @@ -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);
}

/// <summary>
Expand Down Expand Up @@ -910,8 +963,7 @@ public Holdout GetHoldout(string holdoutId)
}

/// <summary>
/// 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).
/// </summary>
/// <returns>Read-only list of global holdouts</returns>
public List<Holdout> GetGlobalHoldouts()
Expand All @@ -920,8 +972,7 @@ public List<Holdout> GetGlobalHoldouts()
}

/// <summary>
/// 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.
/// </summary>
/// <param name="ruleId">The rule ID to look up holdouts for</param>
/// <returns>Read-only list of local holdouts targeting the given rule, or empty list if none</returns>
Expand Down
9 changes: 3 additions & 6 deletions OptimizelySDK/Entity/Holdout.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,16 +47,13 @@ public override string LayerId
}

/// <summary>
/// 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.
/// </summary>
public string[] IncludedRules { get; set; }

/// <summary>
/// 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).
/// </summary>
public bool IsGlobal => IncludedRules == null;
}
Expand Down
15 changes: 10 additions & 5 deletions OptimizelySDK/ProjectConfig.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,10 +181,17 @@ public interface ProjectConfig
Rollout[] Rollouts { get; set; }

/// <summary>
/// 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.
/// </summary>
Holdout[] Holdouts { get; set; }

/// <summary>
/// Local holdouts from the datafile "localHoldouts" section.
/// Each entry must have includedRules; entries without it are invalid and excluded at parse time.
/// </summary>
Holdout[] LocalHoldouts { get; set; }

/// <summary>
/// Associative list of Integrations.
/// </summary>
Expand Down Expand Up @@ -333,15 +340,13 @@ public interface ProjectConfig
Holdout GetHoldout(string holdoutId);

/// <summary>
/// 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).
/// </summary>
/// <returns>Read-only list of global holdouts</returns>
List<Holdout> GetGlobalHoldouts();

/// <summary>
/// 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.
/// </summary>
/// <param name="ruleId">The rule ID to look up holdouts for</param>
/// <returns>Read-only list of local holdouts targeting the given rule, or empty list if none</returns>
Expand Down
Loading
Loading