[FSSDK-11544] Parsing holdout configuration from datafile + project config holdout impl. #384

junaed-optimizely · 2025-08-12T16:04:20Z

Summary

Datafile Parsing to support holdout + ProjectConfig holdout change

Test plan

Tests have been added to support the changes

Issues

FSSDK-11544
FSSDK-11545

Copilot

Pull Request Overview

Implements holdout functionality for the Optimizely SDK by adding datafile parsing support and project configuration integration. This enables holdout experiments that can be applied globally or targeted to specific feature flags.

Added comprehensive holdout entity model with flag inclusion/exclusion logic
Introduced holdout configuration manager for efficient flag-to-holdout relationship mapping
Extended project configuration interface to support holdout operations and queries

Reviewed Changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
OptimizelySDK/Utils/HoldoutConfig.cs	Core holdout configuration manager with caching and flag relationship logic
OptimizelySDK/Entity/Holdout.cs	Complete holdout entity implementation with audience and variation support
OptimizelySDK/Entity/IExperimentCore.cs	Shared interface for common experiment/holdout functionality
OptimizelySDK/Entity/ExperimentCoreExtensions.cs	Extension methods for shared experiment/holdout operations
OptimizelySDK/Entity/Experiment.cs	Updated to implement IExperimentCore interface
OptimizelySDK/ProjectConfig.cs	Added holdout-related interface methods
OptimizelySDK/Config/DatafileProjectConfig.cs	Integrated holdout parsing and configuration management
Various project files	Added new entity and utility files to compilation
Test files	Comprehensive test coverage for holdout functionality

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-08-14T17:23:46Z

OptimizelySDK/Entity/Holdout.cs

+        /// <summary>
+        /// Flags excluded from this holdout
+        /// </summary>
+        public string[] ExcludedFlags { get; set; } = new string[0];


Using new string[0] is less efficient than Array.Empty<string>() in .NET Core 2.1+ or declaring a static readonly empty array. Consider using a more efficient approach for empty array initialization.

Suggested change

public string[] ExcludedFlags { get; set; } = new string[0];

public string[] IncludedFlags { get; set; } = Array.Empty<string>();

/// <summary>

/// Flags excluded from this holdout

/// </summary>

public string[] ExcludedFlags { get; set; } = Array.Empty<string>();

I think this is a good suggestion.

Copilot · 2025-08-14T17:23:46Z

OptimizelySDK/Entity/Holdout.cs

+        /// <summary>
+        /// Flags excluded from this holdout
+        /// </summary>
+        public string[] ExcludedFlags { get; set; } = new string[0];


Using new string[0] is less efficient than Array.Empty<string>() in .NET Core 2.1+ or declaring a static readonly empty array. Consider using a more efficient approach for empty array initialization.

Suggested change

public string[] ExcludedFlags { get; set; } = new string[0];

public string[] ExcludedFlags { get; set; } = Array.Empty<string>();

Copilot · 2025-08-14T17:23:47Z

OptimizelySDK/Config/DatafileProjectConfig.cs

+            var message = $@"Holdout ID ""{holdoutId}"" is not in datafile.";
+            Logger.Log(LogLevel.ERROR, message);
+            ErrorHandler.HandleError(
+                new InvalidExperimentException("Provided holdout is not in datafile."));


The error message uses InvalidExperimentException for a holdout operation, which is misleading. Consider creating a more specific exception type like InvalidHoldoutException or using a more generic exception type.

Suggested change

new InvalidExperimentException("Provided holdout is not in datafile."));

new InvalidHoldoutException("Provided holdout is not in datafile."));

It's a good idea.

muzahidul-opti · 2025-08-14T17:02:03Z

OptimizelySDK/Entity/Holdout.cs

+        /// </summary>
+        public Holdout()
+        {
+            Id = "";


Why do we need this?

Yeah. This should not be here. I guess I put this while implementing tests, and forget to adjust later!

muzahidul-opti · 2025-08-14T17:06:41Z

OptimizelySDK/ProjectConfig.cs

+        /// Get the holdout from the ID
+        /// </summary>
+        /// <param name="holdoutId">ID for holdout</param>
+        /// <returns>Holdout Entity corresponding to the holdout ID or a dummy entity if ID is invalid</returns>


Curious to know csharp doesn't support null value?

My bad. I guess I lost track of the default case. Will update promptly!

muzahidul-opti · 2025-08-14T17:26:33Z

OptimizelySDK/Utils/HoldoutConfig.cs

+                    activeHoldouts.Add(globalHoldout);
+                }
+            }
+


I think we could improve this logic. If excluded list is empty we don't need to iterate the global ho list.
if (excludefForFlag.Count > 0) {
/ / do iteration
} else {
activeHoldouts = globalHoldout
}

mikechu-optimizely

I've reviewed and provided comments around performance and consistency. I do not see any security problems. I'm providing a non-blocking approval but suggest consideration of the comments from me and Copilot. Good work. Welcome to C#. 😁

mikechu-optimizely · 2025-08-14T20:16:03Z

OptimizelySDK/Config/DatafileProjectConfig.cs

+        /// <summary>
+        /// Associative array of Holdout ID to Holdout(s) in the datafile
+        /// </summary>
+        private Dictionary<string, Holdout> _HoldoutIdMap;
+
+        public Dictionary<string, Holdout> HoldoutIdMap => _HoldoutIdMap;
+


Thanks for following existing convention. I hope we'll go to more modern syntax public Dictionary<string, Holdout> HoldoutIdMap { get; private set; }

mikechu-optimizely · 2025-08-14T20:19:42Z

OptimizelySDK/Config/DatafileProjectConfig.cs

+            var message = $@"Holdout ID ""{holdoutId}"" is not in datafile.";
+            Logger.Log(LogLevel.ERROR, message);
+            ErrorHandler.HandleError(
+                new InvalidExperimentException("Provided holdout is not in datafile."));


It's a good idea.

mikechu-optimizely · 2025-08-14T20:21:50Z

OptimizelySDK/Config/DatafileProjectConfig.cs

+        public Holdout[] GetHoldoutsForFlag(string flagKey)
+        {
+            var holdouts = _holdoutConfig?.GetHoldoutsForFlag(flagKey);
+            return holdouts?.ToArray() ?? new Holdout[0];


Suggested change

return holdouts?.ToArray() ?? new Holdout[0];

return holdouts?.ToArray() ?? Array.Empty<Holdout>();

Consider what Copilot pointed out too.

mikechu-optimizely · 2025-08-14T20:28:27Z

OptimizelySDK/Entity/ExperimentCoreExtensions.cs

+            var collect = false;
+            var replaced = string.Empty;
+
+            foreach (var ch in conditionString)


Would this accomplish the goal in lieu of looping?

return Regex.Replace( conditionString, @"AUDIENCE$""([^""]+)""$", match => { var id = match.Groups[1].Value; var name = audiencesMap.TryGetValue(id, out var audienceName) ? audienceName : id; return $"\"{name}\""; });

mikechu-optimizely · 2025-08-14T20:29:04Z

OptimizelySDK/Entity/Holdout.cs

+        /// <summary>
+        /// Flags excluded from this holdout
+        /// </summary>
+        public string[] ExcludedFlags { get; set; } = new string[0];


I think this is a good suggestion.

mikechu-optimizely · 2025-08-14T20:34:29Z

OptimizelySDK/Entity/Holdout.cs

+        /// <summary>
+        /// Generate variation key maps for performance optimization
+        /// </summary>
+        public void GenerateVariationKeyMap()


NIT: Does GenerateVariationMaps sound more aligned with the method generating an id map and a key map?

mikechu-optimizely · 2025-08-14T20:35:58Z

OptimizelySDK/Entity/Holdout.cs

+        /// </summary>
+        /// <param name="audiencesMap">Map of audience ID to audience name</param>
+        /// <returns>Serialized audience string with names</returns>
+        public string SerializeAudiences(Dictionary<string, string> audiencesMap)


Is this method being used? (just double-checking)

...or maybe it will be? …or perhaps we're following a reference implementation for consistency across other SDKs. 😄

mikechu-optimizely · 2025-08-14T20:41:39Z

OptimizelySDK/Entity/Holdout.cs

+            {
+                if (AudienceConditions == null)
+                {
+                    return null;


Should we also set the backing field to null to ensure the value is always consistent with the source data?

Suggested change

return null;

_audienceConditionsString = null;

return null;

mikechu-optimizely · 2025-08-14T20:49:15Z

OptimizelySDK/Utils/HoldoutConfig.cs

+        /// <returns>The Holdout object if found, null otherwise</returns>
+        public Holdout GetHoldout(string holdoutId)
+        {
+            return _holdoutIdMap.ContainsKey(holdoutId) ? _holdoutIdMap[holdoutId] : null;


Suggested change

return _holdoutIdMap.ContainsKey(holdoutId) ? _holdoutIdMap[holdoutId] : null;

if (string.IsNullOrEmpty(holdoutId))

{

return null;

}

_holdoutIdMap.TryGetValue(holdoutId, out var holdout);

return holdout;

...for better performance and null/empty input check

[FSSDK-11544] experiment core and holdout introduction

e9e0df5

optimizely-codereview-bot assigned junaed-optimizely Aug 12, 2025

junaed-optimizely added 4 commits August 12, 2025 22:08

[FSSDK-11544] copyright fix

3f3f4c0

[FSSDK-11544] Experiment adjustment

73d261a

[FSSDK-11544] holdout parsing

6f7d2c8

[FSSDK-11544] test coverage

b3b3a6e

junaed-optimizely marked this pull request as ready for review August 13, 2025 18:02

junaed-optimizely added 3 commits August 14, 2025 19:38

[FSSDK-11544] lint fix

4be2173

[FSSDK-11544] lint fix test for one file

702db9d

[FSSDK-11544] lint fix

f77205d

junaed-optimizely requested review from muzahidul-opti and mikechu-optimizely August 14, 2025 16:18

junaed-optimizely changed the title ~~[FSSDK-11544] experiment core and holdout introduction~~ [FSSDK-11544] Parsing holdout configuration from datafile + project config holdout impl. Aug 14, 2025

mikechu-optimizely requested a review from Copilot August 14, 2025 17:22

Copilot AI reviewed Aug 14, 2025

View reviewed changes

muzahidul-opti requested changes Aug 14, 2025

View reviewed changes

[FSSDK-11544] review update

12d8d1b

junaed-optimizely requested a review from muzahidul-opti August 14, 2025 17:58

mikechu-optimizely approved these changes Aug 14, 2025

View reviewed changes

	public string[] ExcludedFlags { get; set; } = new string[0];
	public string[] ExcludedFlags { get; set; } = Array.Empty<string>();

	new InvalidExperimentException("Provided holdout is not in datafile."));
	new InvalidHoldoutException("Provided holdout is not in datafile."));

	return holdouts?.ToArray() ?? new Holdout[0];
	return holdouts?.ToArray() ?? Array.Empty<Holdout>();

-            return _holdoutIdMap.ContainsKey(holdoutId) ? _holdoutIdMap[holdoutId] : null;
+            if (string.IsNullOrEmpty(holdoutId))
+            {
+                 return null;
+             }
+            _holdoutIdMap.TryGetValue(holdoutId, out var holdout);
+            return holdout;

[FSSDK-11544] Parsing holdout configuration from datafile + project config holdout impl. #384

Are you sure you want to change the base?

[FSSDK-11544] Parsing holdout configuration from datafile + project config holdout impl. #384

Uh oh!

Conversation

junaed-optimizely commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Issues

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikechu-optimizely left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

junaed-optimizely commented Aug 12, 2025 •

edited

Loading