ConfigurationBuilder - Configuration.Json: Binding a Dictionary<string,string> error when Key contains an string url like http://www.google.es

[ghost]

Describe the bug

Im reading a json file to a dictionary<string,string> like:

{
  "auths": {
    "https://www.google.es": {
      "uri": "https://www.google.es"
    }
  }
}

having the POCO class

public class MyClass
{
        [ConfigurationProperty("auths", IsRequired = false)]
        public Dictionary<string, OtherObject> Auths { get; set; }
}

When i load it using ConfigurationRoot like

public MyClass Read()
        {
            IConfigurationRoot configuration =
                new ConfigurationBuilder()
                    .AddJsonFile("path_to_json_file", optional: true, reloadOnChange: true)
                .Build();

            // Try Parse and return (if will exception in case of binding error)
            var _settings= new MyClass();
            configuration.Bind(_settings);

            return _settings;
        }

The issue is that the Dictionary item contains https only on the Key property. Looks like it's ignoring or having issues with https://.....

Not sure if it's an intended feature or a bug.

To Reproduce

Steps to reproduce the behavior:

1. Using version '3.1.8' of package 'Microsoft.Extensions.Configuration.Json'
2. Run this code 'with files in description'
3. See error in the screenshot attached

Expected behavior

Dictionary Key Item should contains https://www.google.es

Screenshots

[KalleOlaviNiemitalo]

That seems deliberate to me. The configuration system flattens the key hierarchy to a string with colons as delimiters so it becomes "auths:https://www.google.es:uri", and binding then cannot know where the boundaries were originally.

It is documented in Hierarchical configuration data.

[ghost]

That makes sense, even if i don't agree on using : as hierarchy... but, i will try to look into the code to see if the separator char can be changed / configured somewhere.

Thanks for the reply i completely missed that point !

[KalleOlaviNiemitalo]

The separator char does not look easy to change at run time: https://github.com/dotnet/extensions/blob/1774c15ac24c65513fa9fc1f4fbb69be9a2a4e25/src/Configuration/Config.Abstractions/src/ConfigurationPath.cs#L14-L17

[KalleOlaviNiemitalo]

By the way, I don't think the Microsoft.Extensions.Configuration.* packages will care about [ConfigurationProperty("auths", IsRequired = false)] in your POCO class.

[ghost]

Im using the ConfigProperty due other reasons.
About the limiter... i will need to use JsonSerializer :\

[KalleOlaviNiemitalo]

Can you use some other strings as keys? For example:

{
  "auths": {
    "es": {
      "uri": "https://www.google.es"
    }
  }
}

Then register an IPostConfigureOptions<MyClass> implementation that reads the Dictionary<string, OtherObject> Auths property that was populated from configuration binding, and copies the objects to a separate dictionary with the keys that you really want. The "es" key would be used only in configuration files or environment variables, not for looking up OtherObject instances later.

[ghost]

Well, not in this case, i need to use the Uri as key. But anyway, i can do exactly the same using the JsonSerializer with a custom serializer, and works well.

Without the need of : in the key, the classical approach works pretty well.

Thank you for your help, i think we can close the issue ?

[ghost]

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

[BrennanConroy]

Maybe we want to have an escape sequence for json config so this is allowed? Or a flag to disable the ':' behavior just for Json?

If any change is made, we should port it to the Newtonsoft.Json config provider in Extensions.

[JunTaoLuo]

Looks like this issue has been encountered previously, e.g. aspnet/Configuration#792 dotnet/extensions#782. Sounds like something we should address instead of our usual answer that : is a reserved character and this issue is by design.

[SteveDunn]

I'm happy to look at this one. PR here: #66886

[SteveDunn]

The separator char does not look easy to change at run time: https://github.com/dotnet/extensions/blob/1774c15ac24c65513fa9fc1f4fbb69be9a2a4e25/src/Configuration/Config.Abstractions/src/ConfigurationPath.cs#L14-L17

I've had a go here: #66886

[AraHaan]

I do not see the reason for :'s in json keys due to how everything gets messed up at runtime with them in the keys. Also it seems to make the json more verbose. I would think this should be by design.

[SteveDunn]

@AraHaan - I personally wouldn't have been able to see the use of keys with colons in, but the examples here are quite compelling.

[tebeco]

you're describing an array, but you're writing object.
Here is a configuration with array:

{
  "auths": [
    {
      "name": "http://google",
      "uri": "https://www.google.es"
    },
    {
      "name": "http://microsoft",
      "uri": "https://www.microsoft.es"
    }
  ]
}

public class AuthEntry
{
  public string Name {get;set;}
  public Uri Uri {get;set;}
}

public class AuthsOptions
{
  public const string SectionName = "auths";
  public AuthEntry[] Auths {get;set;}
}

public static class AuthsServiceCollectionExtensions
{
  public static IServiceCollection AddAuths(this IServiceCollection services)
  {
    services
      .AddOptions<AuthsOptions>()
      .Configure<IConfiguration>((options, configuration) => configuration.Bind(AuthsOptions.SectionName, options))
      //.Validate(...............)
    ;

    return services;
  }

  public static IServiceCollection AddAuths(this IServiceCollection services, Action<AuthsOptions> 
  configure) =>
    services
      .AddAuths()
      .Configure(configure)
    ;

public class MyService
{
  private readonly AuthsOptions _options;
  private readonly HttpClient _httpClient;

  public MyService(IOptions<AuthsOptions> options, HttpClient httpClient)
  {
    _options = options.Value; //.Value here because there's no reload on change, you could adapt it
    // build a dictionary from key ?
  }
  
  public async Task<string> GetDataFrom(string key)
  {
    var authEntry = options.Auths.First(authEntry => authEntry == key); // use stringcomparison ?
    
    await _httpClient.GetStringAsync(new Uri(authEntry.Uri, otherRelativeUri);
  }
}

[eerhardt]

We will consider this in a future release. Moving this issue out of the 7.0 milestone.

[SteveDunn]

This is the motivation behind #67616

[rjgotten]

you're describing an array, but you're writing object.

Many times you have to use objects by necessity as arrays are completely useless for everything but the most basic configuration needs when you use environment-specific configuration overrides. This is because arrays use positional (index) based merging across environment-specific config files which is an absolute pain to manage and slip-ups in it are many times impossible to detect.

Basically; you're telling people: "You're doing it wrong. You're supposed to step into this minefield here."

[rjgotten]

Looks like this issue has been encountered previously, e.g. aspnet/Configuration#792 dotnet/extensions#782. Sounds like something we should address instead of our usual answer that : is a reserved character and this issue is by design.

There was never any technical need for the : to be a reserved character to begin with.
That problem stems from the fact that the initial design for the internal storage of the configuration data in Microsoft.Extensions.Configuration was thought out poorly and stores everything in a flat dictionary by fully composed key, rather than use an appropriate tree-based data structure.

There are multiple other issues where this ill-thought out design is the root of the problem.
For instance; the performance problems with the ConfigurationBinder, when binding collection types. Because it has to loop over the entire collection of keys every time to determine which full keys are children of the collection key. (Which it does in GetChildKeys)

That was reported in #65885 as well, but ultimately wasn't acted upon (yet) because of fear of breaking existing implementations as well. At least that had a decent workaround. This one doesn't. You simply have to avoid using a dictionary keyed on anything that contains a : and that's it. No recourse.

Another problem with this internal storage of fully composed keys is that it's impossible to have a dictionary of empty objects.
That could be perfectly valid when bound to a Dictionary<string, Thing> where Thing is a POCO with default values assigned to properties. But because the config system stores fully composed keys only everything has to be a string->string key-value mapping. And as e.g. { "Items" : { "Foo" : {}, "Bar" : {}} does not have representable values in keys 'below' "Foo" or "Bar", that entire configuration is simply lost and you end up with an empty bound dictionary.

That would not be the case with a proper tree-based structure. Because there you'd have a string->node mapping where each node can optionally have a primitive value itself, and can have child nodes. Even if the collection of child nodes is empty; and even if the node has no primitive value itself; the node and its key are still representable in a sound way. And thus binding such empty objects would be feasible.

So...
Maybe, rather than trying to slap band-aid upon band-aid to try and fight symptoms of bad design, it's time to tackle the actual bad design?

[layomia]

Collapsing this into #67616 since that would be the fundamental change to enable this scenario.