-
Notifications
You must be signed in to change notification settings - Fork 119
Add support for the ':' character in feature flags when using the default feature flag definition provider. #155
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.
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
Conversation
…ault feature flag definition provider.
|
cc @MaryanneNjeri I wasn't able to add you as a reviewer. |
| } | ||
| } | ||
|
|
||
| configuration = section; |
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.
If featureName is Feature:With:Colons, after the foreach loop ends, we'll set configuration to the section named Colons . Then we call ReadFeatureDefinition(configuration), and that method will return the FeatureDefinition object where FeatureDefinition.Name will be set to Colons instead of Feature:With:Colons (code).
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.
Thanks. This made me realize a bigger problem which is how this works with GetFeatureDefinitionAsync. We may not be able to handle that
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.
GetFeatureDefinitionAsync can't detect features that have :. That's a glaring hole in supporting :. We may just want to block : because of that.
| sections = section == null ? | ||
| GetFeatureDefinitionSections() : | ||
| section.GetChildren(); |
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.
section == null will only be true for the first iteration in the foreach loop. Instead of doing this check inside the loop every time, can we initialize sections to GetFeatureDefinitionSections() before the loop?
| { | ||
| // | ||
| // Feature names with configuration path delimiters require traversing children sections for resolution | ||
| IEnumerable<string> sectionNames = featureName.Split(SectionSeparator, StringSplitOptions.None); |
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.
Could we define SectionSeparator as a string instead of string[]? Another option is to directly use ConfigurationPath.KeyDelimiter because this is a special delimiter that's unlikely to change in .NET.
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 just stored it that way since Split accepts string[] and ConfigurationPath.KeyDelimiter is a string instead of a char. It's an optimization instead of creating a new string array every time.
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.
FWIW, Split also accepts string delimiter.
|
Hi guys, is there an update for this feature? Why this was closed? "EndpointEnabledStatus": {
"Path": {
"Subpath": {
"Create": false,
"Read" : true,
"Update" : false,
"Delete" : true
}
}
}I've been doing some testing and seems the GetChildren is the main issue because it just takes the children of the root. FeatureManagement-Dotnet/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs Line 189 in e3eb4da
|
|
@fpaganetto We abandoned this due to the complexity of supporting the ':' character in .NET Core. This character separates configuration sections and as you mentioned breaks the way the feature manager works today. It's not possible to deterministically enumerate all defined features if the |
|
Just to be clear, I want to have subsections on the I don't understand if this is because of allowing the An example of what I want: [FeatureGate("Authentication:User:Read")]
public async Task<IActionResult> ReadUser(int id){}
[FeatureGate("Authentication:User:Create")]
public async Task<IActionResult> CreateUser(User user){}On my appsettings.json: "EndpointEnabledStatus": {
"Authentication": {
"User": {
"Create": false,
"Read" : true,
}
}
} |
|
@fpaganetto Thanks for the clarification. That is not going to be possible. The PR that you linked does need to be completed to make it clear that the ':' character is not supported. I will bump it for review. |
This PR adds support for feature flags containing the ':' character.
Currently a feature with the name "Feature:With:Colons" cannot be found by the feature manager. With this change, the feature will now be found and evaluated as expected.