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.

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

Feature names with colon in name are not parsed correctly #90

Closed
peterwurzinger opened this issue Aug 5, 2020 · 5 comments
Closed

Feature names with colon in name are not parsed correctly #90

peterwurzinger opened this issue Aug 5, 2020 · 5 comments
Assignees

Comments

@peterwurzinger
Copy link

peterwurzinger commented Aug 5, 2020

Given the following configuration:

"FeatureManagement": {
    "Feature1": true,
    "Prefix:Feature2": true
}

Querying the features was a bit surprising at first:

featureManager.IsEnabledAsync("Feature1") //returns true
featureManager.IsEnabledAsync("Prefix:Feature2") //returns false

Listing the feature names is also quite misleading:

featureManager.GetFeatureNamesAsync(); //returns "Feature1", "Prefix"

By taking a closer look, the colon is of course not only a gimmick, but more like a common separator for config sections. So when using the JSON file provider, the abovementioned configuration could also be semantically identical rephrased as

"FeatureManagement": {
    "Feature1": true,
    "Prefix": {
        "Feature2": true
    }
}

And that explains the weird behavior.

To solve that, would it be a possible to tokenize given feature names by : , treating them as config-sections and only the last segment as the name? Imho that would fix the "Prefix":Feature" : true case, as well as enable feature grouping (see above).

@peterwurzinger peterwurzinger changed the title Feature names with colon in name are not queried Feature names with colon in name are not parsed correctly Aug 5, 2020
@jimmyca15
Copy link
Member

In general, the ':' character should be avoided in setting names in json settings files in .NET Core. The ':' character is treated specially as you have called out. I don't think it would be a good idea to go against that concept. It may be a good idea to call this out in the readme though.

@peterwurzinger
Copy link
Author

Well, since IsEnabledAsync("Prefix:Feature") will never behave as expected, would it then be an option to throw an exception, if the input string contains a colon?

@jimmyca15
Copy link
Member

That's a great suggestion. Technically if someone is using a custom IFeatureDefinitionProvider then colon might be fine for them, but if the default ConfigurationFeatureDefinitionProvider is being used then we can throw an ArgumentException to prevent confusion in a case like this.

@cwe1ss
Copy link

cwe1ss commented Oct 18, 2022

I just tried Azure App Configuration and was surprised by this behavior as well. The docs use a colon for regular key/value pairs and this works as expected with ASP.NET Core ( https://learn.microsoft.com/en-us/azure/azure-app-configuration/quickstart-aspnet-core-app?tabs=core6x#create-an-app-configuration-store ) so I expected this to work with Feature Management as well.

It was even more unexpected that I had to use FeatureManagement:MyPrefix instead of just MyPrefix to get this working.

builder.Services.AddFeatureManagement(builder.Configuration.GetSection("FeatureManagement:MyPrefix"));

I think it would be important that we could use the same separator for regular keys and for flags when using Feature Management with Azure App Configuration.

@zhiyuanliang-ms zhiyuanliang-ms self-assigned this Oct 23, 2023
@zhiyuanliang-ms
Copy link
Contributor

#288
Resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants