From 0d1769cfcf7f2edf751be41acf7e907c4f1f9960 Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Thu, 14 Jul 2022 09:05:21 -0400 Subject: [PATCH 1/5] Handle null, empty, or whitespaces for rolloutId Includes unit test covering existing logic --- OptimizelySDK.Tests/ProjectConfigTest.cs | 13 +++++++++++++ OptimizelySDK.sln.DotSettings | 7 ++++++- OptimizelySDK/Config/DatafileProjectConfig.cs | 5 +++++ 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/OptimizelySDK.Tests/ProjectConfigTest.cs b/OptimizelySDK.Tests/ProjectConfigTest.cs index 03faf85d..68105489 100644 --- a/OptimizelySDK.Tests/ProjectConfigTest.cs +++ b/OptimizelySDK.Tests/ProjectConfigTest.cs @@ -822,5 +822,18 @@ public void TestIsFeatureExperimentReturnsTrueForExperimentThatBelongsToAFeature Assert.True(typedConfig.IsFeatureExperiment(experiment.Id)); } + + [Test] + public void TestRolloutWithEmptyStringRolloutId() + { + var rolloutId = string.Empty; + + var rollout = Config.GetRolloutFromId(rolloutId); + + // OptlyConfig.OptimizelyConfigService.GetDeliveryRules L362 + Assert.IsNull(rollout.Experiments); + // Bucketing.DecisionService.GetVariationForFeatureRollout L439 + Assert.IsNull(rollout.Id); + } } } diff --git a/OptimizelySDK.sln.DotSettings b/OptimizelySDK.sln.DotSettings index c1724b1d..7cb4f57e 100644 --- a/OptimizelySDK.sln.DotSettings +++ b/OptimizelySDK.sln.DotSettings @@ -13,6 +13,7 @@ END_OF_LINE TOGETHER_SAME_LINE False + False 0 END_OF_LINE NEVER @@ -40,4 +41,8 @@ <Policy Inspect="True" Prefix="" Suffix="" Style="AaBb"><ExtraRule Prefix="" Suffix="" Style="AaBb" /></Policy> <Policy Inspect="True" Prefix="I" Suffix="" Style="AaBb" /> <Policy Inspect="True" Prefix="" Suffix="" Style="AA_BB" /> - <Policy Inspect="True" Prefix="" Suffix="" Style="aaBb" /> \ No newline at end of file + <Policy Inspect="True" Prefix="" Suffix="" Style="aaBb" /> + True + True + True + True \ No newline at end of file diff --git a/OptimizelySDK/Config/DatafileProjectConfig.cs b/OptimizelySDK/Config/DatafileProjectConfig.cs index 07045ae3..9334bb3b 100644 --- a/OptimizelySDK/Config/DatafileProjectConfig.cs +++ b/OptimizelySDK/Config/DatafileProjectConfig.cs @@ -636,6 +636,11 @@ public Variation GetFlagVariationByKey(string flagKey, string variationKey) /// Rollout Entity corresponding to the rollout ID or a dummy entity if ID is invalid public Rollout GetRolloutFromId(string rolloutId) { + if (string.IsNullOrWhiteSpace(rolloutId)) + { + return new Rollout(); + } + if (_RolloutIdMap.ContainsKey(rolloutId)) return _RolloutIdMap[rolloutId]; From a3271c6abd7e29f242f76c2b505af320b2fc51da Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Thu, 14 Jul 2022 09:30:28 -0400 Subject: [PATCH 2/5] Add e2e test using retrieved config from datafile --- OptimizelySDK.Tests/EmptyRolloutRule.json | 14 ++++++++++++++ OptimizelySDK.Tests/ProjectConfigTest.cs | 15 +++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/OptimizelySDK.Tests/EmptyRolloutRule.json b/OptimizelySDK.Tests/EmptyRolloutRule.json index 70913612..8aa8e559 100644 --- a/OptimizelySDK.Tests/EmptyRolloutRule.json +++ b/OptimizelySDK.Tests/EmptyRolloutRule.json @@ -25,6 +25,20 @@ ], "id": "18244658520", "key": "empty_rollout" + }, + { + "experimentIds": [], + "rolloutId": "", + "variables": [ + { + "defaultValue": "", + "type": "string", + "id": "2832355113", + "key": "var_2" + } + ], + "id": "24246538512", + "key": "empty_rollout_id" } ], "experiments": [], diff --git a/OptimizelySDK.Tests/ProjectConfigTest.cs b/OptimizelySDK.Tests/ProjectConfigTest.cs index 68105489..f4dde7d2 100644 --- a/OptimizelySDK.Tests/ProjectConfigTest.cs +++ b/OptimizelySDK.Tests/ProjectConfigTest.cs @@ -823,6 +823,21 @@ public void TestIsFeatureExperimentReturnsTrueForExperimentThatBelongsToAFeature Assert.True(typedConfig.IsFeatureExperiment(experiment.Id)); } + [Test] + public void TestRolloutWithEmptyStringRolloutIdFromConfigFile() + { + var projectConfig = DatafileProjectConfig.Create(TestData.EmptyRolloutDatafile, new NoOpLogger(), new NoOpErrorHandler()); + Assert.IsNotNull(projectConfig); + var featureFlag = projectConfig.FeatureKeyMap["empty_rollout_id"]; + + var rollout = projectConfig.GetRolloutFromId(featureFlag.RolloutId); + + // OptlyConfig.OptimizelyConfigService.GetDeliveryRules L362 + Assert.IsNull(rollout.Experiments); + // Bucketing.DecisionService.GetVariationForFeatureRollout L439 + Assert.IsNull(rollout.Id); + } + [Test] public void TestRolloutWithEmptyStringRolloutId() { From b95a053b5bd2e42e139a929bfd057bdfba003067 Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Thu, 14 Jul 2022 13:36:05 -0400 Subject: [PATCH 3/5] Satisfy .NET 3.5 lack of string.IsNullOrWhiteSpace() Grrr we have to put up with this until 2029 unless we check w/ our clients to see if we can remove it. --- OptimizelySDK.Tests/ProjectConfigTest.cs | 13 +++++++++++++ OptimizelySDK/Config/DatafileProjectConfig.cs | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/OptimizelySDK.Tests/ProjectConfigTest.cs b/OptimizelySDK.Tests/ProjectConfigTest.cs index f4dde7d2..f61aebdc 100644 --- a/OptimizelySDK.Tests/ProjectConfigTest.cs +++ b/OptimizelySDK.Tests/ProjectConfigTest.cs @@ -850,5 +850,18 @@ public void TestRolloutWithEmptyStringRolloutId() // Bucketing.DecisionService.GetVariationForFeatureRollout L439 Assert.IsNull(rollout.Id); } + + [Test] + public void TestRolloutWithConsistingOfASingleSpaceRolloutId() + { + var rolloutId = " "; // single space + + var rollout = Config.GetRolloutFromId(rolloutId); + + // OptlyConfig.OptimizelyConfigService.GetDeliveryRules L362 + Assert.IsNull(rollout.Experiments); + // Bucketing.DecisionService.GetVariationForFeatureRollout L439 + Assert.IsNull(rollout.Id); + } } } diff --git a/OptimizelySDK/Config/DatafileProjectConfig.cs b/OptimizelySDK/Config/DatafileProjectConfig.cs index 9334bb3b..01e05ba2 100644 --- a/OptimizelySDK/Config/DatafileProjectConfig.cs +++ b/OptimizelySDK/Config/DatafileProjectConfig.cs @@ -636,7 +636,7 @@ public Variation GetFlagVariationByKey(string flagKey, string variationKey) /// Rollout Entity corresponding to the rollout ID or a dummy entity if ID is invalid public Rollout GetRolloutFromId(string rolloutId) { - if (string.IsNullOrWhiteSpace(rolloutId)) + if (string.IsNullOrEmpty(rolloutId.Trim())) { return new Rollout(); } From adc0dab681047d580c3592da486c96467cf28be7 Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Thu, 14 Jul 2022 13:58:05 -0400 Subject: [PATCH 4/5] Copyright updates I need to get those pre-commit hooks running --- OptimizelySDK.Tests/ProjectConfigTest.cs | 2 +- OptimizelySDK/Config/DatafileProjectConfig.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/OptimizelySDK.Tests/ProjectConfigTest.cs b/OptimizelySDK.Tests/ProjectConfigTest.cs index f61aebdc..fc06d454 100644 --- a/OptimizelySDK.Tests/ProjectConfigTest.cs +++ b/OptimizelySDK.Tests/ProjectConfigTest.cs @@ -1,5 +1,5 @@ /* - * Copyright 2017-2021, Optimizely + * Copyright 2017-2022, Optimizely * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. diff --git a/OptimizelySDK/Config/DatafileProjectConfig.cs b/OptimizelySDK/Config/DatafileProjectConfig.cs index 01e05ba2..01abb1a2 100644 --- a/OptimizelySDK/Config/DatafileProjectConfig.cs +++ b/OptimizelySDK/Config/DatafileProjectConfig.cs @@ -1,5 +1,5 @@ /* - * Copyright 2019-2021, Optimizely + * Copyright 2019-2022, Optimizely * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. From 395225472586575fbce8ca09709460fb705958bb Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Fri, 15 Jul 2022 17:04:35 -0400 Subject: [PATCH 5/5] Code review corrections. --- OptimizelySDK.Tests/ProjectConfigTest.cs | 19 ++++++++++++------- OptimizelySDK/Config/DatafileProjectConfig.cs | 6 +++++- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/OptimizelySDK.Tests/ProjectConfigTest.cs b/OptimizelySDK.Tests/ProjectConfigTest.cs index fc06d454..7cf69c54 100644 --- a/OptimizelySDK.Tests/ProjectConfigTest.cs +++ b/OptimizelySDK.Tests/ProjectConfigTest.cs @@ -826,15 +826,13 @@ public void TestIsFeatureExperimentReturnsTrueForExperimentThatBelongsToAFeature [Test] public void TestRolloutWithEmptyStringRolloutIdFromConfigFile() { - var projectConfig = DatafileProjectConfig.Create(TestData.EmptyRolloutDatafile, new NoOpLogger(), new NoOpErrorHandler()); + var projectConfig = DatafileProjectConfig.Create(TestData.EmptyRolloutDatafile, null, null); Assert.IsNotNull(projectConfig); var featureFlag = projectConfig.FeatureKeyMap["empty_rollout_id"]; var rollout = projectConfig.GetRolloutFromId(featureFlag.RolloutId); - // OptlyConfig.OptimizelyConfigService.GetDeliveryRules L362 Assert.IsNull(rollout.Experiments); - // Bucketing.DecisionService.GetVariationForFeatureRollout L439 Assert.IsNull(rollout.Id); } @@ -845,9 +843,7 @@ public void TestRolloutWithEmptyStringRolloutId() var rollout = Config.GetRolloutFromId(rolloutId); - // OptlyConfig.OptimizelyConfigService.GetDeliveryRules L362 Assert.IsNull(rollout.Experiments); - // Bucketing.DecisionService.GetVariationForFeatureRollout L439 Assert.IsNull(rollout.Id); } @@ -858,9 +854,18 @@ public void TestRolloutWithConsistingOfASingleSpaceRolloutId() var rollout = Config.GetRolloutFromId(rolloutId); - // OptlyConfig.OptimizelyConfigService.GetDeliveryRules L362 Assert.IsNull(rollout.Experiments); - // Bucketing.DecisionService.GetVariationForFeatureRollout L439 + Assert.IsNull(rollout.Id); + } + + [Test] + public void TestRolloutWithConsistingOfANullRolloutId() + { + string nullRolloutId = null; + + var rollout = Config.GetRolloutFromId(nullRolloutId); + + Assert.IsNull(rollout.Experiments); Assert.IsNull(rollout.Id); } } diff --git a/OptimizelySDK/Config/DatafileProjectConfig.cs b/OptimizelySDK/Config/DatafileProjectConfig.cs index 01abb1a2..93a0aceb 100644 --- a/OptimizelySDK/Config/DatafileProjectConfig.cs +++ b/OptimizelySDK/Config/DatafileProjectConfig.cs @@ -636,7 +636,11 @@ public Variation GetFlagVariationByKey(string flagKey, string variationKey) /// Rollout Entity corresponding to the rollout ID or a dummy entity if ID is invalid public Rollout GetRolloutFromId(string rolloutId) { - if (string.IsNullOrEmpty(rolloutId.Trim())) +#if NET35 || NET40 + if (string.IsNullOrEmpty(rolloutId) || string.IsNullOrEmpty(rolloutId.Trim())) +#else + if (string.IsNullOrWhiteSpace(rolloutId)) +#endif { return new Rollout(); }