Skip to content
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

Jump to bottom

bug: Handle null, empty, or whitespaces for RolloutId #304

Merged
merged 5 commits into from
Jul 18, 2022
Merged

bug: Handle null, empty, or whitespaces for RolloutId #304

merged 5 commits into from
Jul 18, 2022

Conversation

mikechu-optimizely
Copy link
Contributor

Summary

  • Handle possible empty string rolloutId in datafile

An empty string rolloutId should be ignored and not raise and exception

Test plan

Unit test added to OptimizelySDK.Tests/ProjectConfigTest.cs

Issues

  • OASIS-8370

@mikechu-optimizely mikechu-optimizely requested a review from a team as a code owner July 14, 2022 13:33
@mikechu-optimizely mikechu-optimizely self-assigned this Jul 14, 2022
@mikechu-optimizely
Satisfy .NET 3.5 lack of string.IsNullOrWhiteSpace()
b95a053 
Grrr we have to put up with this until 2029 unless we check w/ our clients to see if we can remove it.
@mikechu-optimizely
Copy link
Contributor Author

@msohailhussain I don't seem to have access to edit the CI GitHub Action where I can figure out how to correct this error.

2022-07-14 17:38:01 [ERROR]   Failed to get [GITHUB_TOKEN]!
2022-07-14 17:38:01 [ERROR]   []
2022-07-14 17:38:01 [ERROR]   Please set a [GITHUB_TOKEN] from the main workflow environment to take advantage of multiple status reports!

I see that it's not a required Check, but I like all ✔️ 🤷

@mikechu-optimizely
Copyright updates
adc0dab 
I need to get those pre-commit hooks running
msohailhussain
msohailhussain suggested changes Jul 15, 2022
View reviewed changes
Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please address

OptimizelySDK.Tests/ProjectConfigTest.cs Outdated
[Test]
public void TestRolloutWithEmptyStringRolloutIdFromConfigFile()
{
var projectConfig = DatafileProjectConfig.Create(TestData.EmptyRolloutDatafile, new NoOpLogger(), new NoOpErrorHandler());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of NoOpErrorHandler, can you please use ErrorHandler with the the flag, that exception should be raised when error is raised.

OptimizelySDK.Tests/ProjectConfigTest.cs Outdated

var rollout = projectConfig.GetRolloutFromId(featureFlag.RolloutId);

// OptlyConfig.OptimizelyConfigService.GetDeliveryRules L362
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this comment specifically Line number, that can be change in future.

OptimizelySDK.Tests/ProjectConfigTest.cs Outdated

var rollout = Config.GetRolloutFromId(rolloutId);

// OptlyConfig.OptimizelyConfigService.GetDeliveryRules L362
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

OptimizelySDK.Tests/ProjectConfigTest.cs Outdated

var rollout = Config.GetRolloutFromId(rolloutId);

// OptlyConfig.OptimizelyConfigService.GetDeliveryRules L362
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

OptimizelySDK.sln.DotSettings
@@ -13,6 +13,7 @@
<s:String x:Key="/Default/CodeStyle/CodeFormatting/CSharpFormat/CASE_BLOCK_BRACES/@EntryValue">END_OF_LINE</s:String>
<s:String x:Key="/Default/CodeStyle/CodeFormatting/CSharpFormat/EMPTY_BLOCK_STYLE/@EntryValue">TOGETHER_SAME_LINE</s:String>
<s:Boolean x:Key="/Default/CodeStyle/CodeFormatting/CSharpFormat/KEEP_EXISTING_EXPR_MEMBER_ARRANGEMENT/@EntryValue">False</s:Boolean>
<s:Boolean x:Key="/Default/CodeStyle/CodeFormatting/CSharpFormat/KEEP_EXISTING_INITIALIZER_ARRANGEMENT/@EntryValue">False</s:Boolean>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is causing linter to cause errors. not sure.

OptimizelySDK.sln.DotSettings
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ECSharpPlaceEmbeddedOnSameLineMigration/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ECSharpUseContinuousIndentInsideBracesMigration/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ESettingsUpgrade_002EMigrateBlankLinesAroundFieldToBlankLinesAroundProperty/@EntryIndexedValue">True</s:Boolean></wpf:ResourceDictionary>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eol

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If .DotSettings is causing problems should I add JetBrains to .gitignore?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, not problematic, just end of line is better to have at the end of file.

OptimizelySDK/Config/DatafileProjectConfig.cs Outdated
@@ -636,6 +636,11 @@ public Variation GetFlagVariationByKey(string flagKey, string variationKey)
/// <returns>Rollout Entity corresponding to the rollout ID or a dummy entity if ID is invalid</returns>
public Rollout GetRolloutFromId(string rolloutId)
{
if (string.IsNullOrEmpty(rolloutId.Trim()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will cause exception in case rolloutId is missing and eventually null is passed. Check this method
string.IsNullOrWhiteSpace

@msohailhussain
Copy link
Contributor

@mikechu-optimizely in unit tests add one more case for passing null

msohailhussain
msohailhussain approved these changes Jul 18, 2022
View reviewed changes
Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mikechu-optimizely mikechu-optimizely merged commit 96a253d into master Jul 18, 2022
@mikechu-optimizely mikechu-optimizely deleted the mike/empty-string-rolloutid branch July 18, 2022 13:13
@mikechu-optimizely mikechu-optimizely changed the title [OASIS-8370] Handle null, empty, or whitespaces for RolloutId bug: Handle null, empty, or whitespaces for RolloutId Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msohailhussain msohailhussain msohailhussain approved these changes

Assignees

@mikechu-optimizely mikechu-optimizely

Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mikechu-optimizely @msohailhussain