Add NuGetFeatureFlags feature switch #7313
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| // Copyright (c) .NET Foundation. All rights reserved. | ||
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
| #if !NET9_0_OR_GREATER | ||
| namespace System.Diagnostics.CodeAnalysis | ||
| { | ||
| [AttributeUsage(AttributeTargets.Property, Inherited = false)] | ||
| internal sealed class FeatureSwitchDefinitionAttribute : Attribute | ||
| { | ||
| public FeatureSwitchDefinitionAttribute(string switchName) => SwitchName = switchName; | ||
| public string SwitchName { get; } | ||
| } | ||
| } | ||
| #endif |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| // Copyright (c) .NET Foundation. All rights reserved. | ||
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
| using System; | ||
| using System.Diagnostics.CodeAnalysis; | ||
| using NuGet.Common; | ||
|
|
||
| namespace NuGet.Shared | ||
| { | ||
| internal static class NuGetFeatureFlags | ||
| { | ||
| internal const string UsesNSJDeserializationSwitchName = "NuGet.UsesNSJDeserialization"; | ||
| internal const string UsesNSJDeserializationEnvVar = "NUGET_USES_NSJ_DESERIALIZATION"; | ||
|
|
||
| private static readonly Lazy<bool> _isNSJDeserializationEnabledByEnvironment = | ||
| new Lazy<bool>(() => IsNSJDeserializationEnabledByEnvironment(EnvironmentVariableWrapper.Instance)); | ||
|
|
||
| /// <summary>Feature switch for NSJ deserialization. Defaults to <see langword="true"/>.</summary> | ||
| [FeatureSwitchDefinition(UsesNSJDeserializationSwitchName)] | ||
| internal static bool NSJDeserializationFeatureSwitch { get; } = | ||
| !AppContext.TryGetSwitch(UsesNSJDeserializationSwitchName, out bool value) || value; | ||
|
|
||
| /// <summary>Returns <see langword="false"/> when env var <c>NUGET_USES_NSJ_DESERIALIZATION</c> is <c>false</c>.</summary> | ||
| internal static bool IsNSJDeserializationEnabledByEnvironment(IEnvironmentVariableReader? env = null) | ||
| { | ||
| if (env is null) | ||
| { | ||
| return _isNSJDeserializationEnabledByEnvironment.Value; | ||
| } | ||
|
|
||
| string? envValue = env.GetEnvironmentVariable(UsesNSJDeserializationEnvVar); | ||
| return !string.Equals(envValue, "false", StringComparison.OrdinalIgnoreCase); | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| // Copyright (c) .NET Foundation. All rights reserved. | ||
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
| using System.Collections.Generic; | ||
| using NuGet.Shared; | ||
| using Test.Utility; | ||
| using Xunit; | ||
|
|
||
| namespace NuGet.Protocol.Tests | ||
| { | ||
| public class NuGetFeatureFlagsTests | ||
| { | ||
| [Fact] | ||
| public void NSJDeserializationFeatureSwitch_Default_ReturnsTrue() | ||
| { | ||
| Assert.True(NuGetFeatureFlags.NSJDeserializationFeatureSwitch); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void IsNSJDeserializationEnabledByEnvironment_WhenEnvVarNotSet_ReturnsTrue() | ||
| { | ||
| Assert.True(NuGetFeatureFlags.IsNSJDeserializationEnabledByEnvironment(TestEnvironmentVariableReader.EmptyInstance)); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData("false")] | ||
| [InlineData("False")] | ||
| [InlineData("FALSE")] | ||
| public void IsNSJDeserializationEnabledByEnvironment_WhenEnvVarSetToFalse_ReturnsFalse(string value) | ||
| { | ||
| var env = new TestEnvironmentVariableReader( | ||
| new Dictionary<string, string> { [NuGetFeatureFlags.UsesNSJDeserializationEnvVar] = value }); | ||
|
|
||
| Assert.False(NuGetFeatureFlags.IsNSJDeserializationEnabledByEnvironment(env)); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData("true")] | ||
| [InlineData("True")] | ||
| [InlineData("0")] | ||
| [InlineData("1")] | ||
| [InlineData("anything")] | ||
|
Member
There was a problem hiding this comment. I haven't seen this pattern much. I'd rather people have to explicitly set something to opt out of it. For example https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Clients/NuGet.VisualStudio.Common/Services/NuGetFeatureFlagService.cs follows a simpler logic. Granted that one is opt in for the most part, and this is opt-out. |
||
| public void IsNSJDeserializationEnabledByEnvironment_WhenEnvVarSetToTrueOrUnrecognized_ReturnsTrue(string value) | ||
| { | ||
| var env = new TestEnvironmentVariableReader( | ||
| new Dictionary<string, string> { [NuGetFeatureFlags.UsesNSJDeserializationEnvVar] = value }); | ||
|
|
||
| Assert.True(NuGetFeatureFlags.IsNSJDeserializationEnabledByEnvironment(env)); | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I was doing a thought exercise in how we can use this value, and I think I need help.
Assume we are doing #7299.
In src/NuGet.Core/NuGet.Protocol/Providers/RepositorySignatureResourceProvider.cs, qwe decided how to deserialize based on the feature flag.
We'd want to have unit tests that run in the same process that exercise the codepath for switching via an env var.
The caching here would make that hard.
I might be missing something, but I don't think the service itself should be caching the env var values for testability purposes.
Also the fact that we're using a static class also means that we can't actually test things because we're not really composing anything.
I think this needs to be a long lived object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Callers will be using
internal static bool IsNSJDeserializationEnabledByEnvironment(IEnvironmentVariableReader? env = null)For example, say I have a method
MyMethod(env = null).If it has a section that does IsNSJDeserialzatioEnabledByEnvitronemtn(env)
if env is null we utilize the cached value
However if it is set we don't utilize the cached value. This allows for both a cached implementation and testable switch too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed that, thanks.
Honestly, I'd rather the code force people to pass a env var reader from a more general testability perspective, but 2 others have reviewed this, and haven't necessarily shared that feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move the contents of this PR into the first PR that's ready to merge that actually uses it? I understand splitting big work into smaller PRs that are easier to review, but in this case it's harder for me personally to visualise how tests will test both NJ and STJ codepaths in the same process. So, putting this static feature flag code in the same PR that demonstrates how the tests work will make it easier for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok will do. Closing PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#7298