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

ConfigurationBuilder add elements to initialized array instead of replacing it #36569

Closed
Eilon opened this issue Oct 18, 2018 · 19 comments
Closed
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-Configuration
Milestone

Comments

@Eilon
Copy link
Member

Eilon commented Oct 18, 2018

From @LordXaosa on Thursday, 18 October 2018 09:39:00

Code:

class TestClass
        {
            public TestClass()
            {
                TestArray = new[] { "123", "test1" };
            }
            public string[] TestArray { get; set; }
        }
class MyConfig
        {
            public TestClass Test{ get; set; }
        }
MyConfig cfg = new MyConfig();
IConfiguration config = new ConfigurationBuilder()
                 .SetBasePath(AppDomain.CurrentDomain.BaseDirectory)
                .AddJsonFile("appsettings.json")
                .Build();
            config.Bind(cfg);

in appsettings.json:

{
  "Test": {
    "TestArray": [ "3", "4" ]
  }
}

I expect after config loading there would be only 2 items: "3" and "4"
But there would be all 4. "123","test1","3","4"
Consider this is a bug or there should be an option for replacing array values.

Copied from original issue: dotnet/aspnetcore#3666

@poke
Copy link
Contributor

poke commented Oct 18, 2018

Judging by the existence of tests that verify exactly that the binder behaves that way, I would assume that this behavior is fully intended.

@LordXaosa
Copy link

Such behavior doesn't seem to be obvious... I really expected 100% equal objects in config and in runtime. May be there should be an option? Count this as suggestion.

@thviQit
Copy link

thviQit commented Oct 26, 2018

This is really bothering me as well. If I write something in my config.json, I expect it to overwrite default values, not add to it.
Basically it means you cannot have default values for collections/arrays, but always have to define them in your configuration. That is not the behavior I would expect.
Right now it giving me some headaches as I validate my option POCO with fluentvalidation, and that doesn't allow null properties.

@aspnet-hello aspnet-hello transferred this issue from aspnet/Configuration Dec 17, 2018
@chuanboz
Copy link

+1 to fix this behavior.
Have some find some workaround before this is fixed?

@chuanboz
Copy link

As this would be a breaking change, can we consider it in asp.net core 3.0 that is expected to ship this fall?

@roblapp
Copy link

roblapp commented Jul 10, 2019

This is causing me a lot of grief. What is the plan for this?

@tvanbaak
Copy link

tvanbaak commented Mar 6, 2020

I am also having trouble because of this behavior. A fairly simple fix should be to add a BinderOptions flag that, if set, calls .Clear() on the collection before adding to it.

For anybody else having trouble with this in the meantime, creating a shim class inheriting from List<T> that calls .Clear() upon the first call to .Add() solves this for the list case, and there should be a similar solution for the Dictionary case.

@analogrelay analogrelay transferred this issue from dotnet/extensions May 15, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label May 15, 2020
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this issue. Please help me learn by adding exactly one area label.

@analogrelay analogrelay added this to the Future milestone May 15, 2020
@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Jul 1, 2020
@maryamariyan maryamariyan added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Oct 20, 2020
@maryamariyan maryamariyan added this to Uncommitted in ML, Extensions, Globalization, etc, POD. via automation Oct 20, 2020
@maryamariyan
Copy link
Member

I am also having trouble because of this behavior. A fairly simple fix should be to add a BinderOptions flag that, if set, calls .Clear() on the collection before adding to it.

@tvanbaak would you be interested in providing API proposal with sample usage for this API addition? Guideline for API proposal is here

@tvanbaak
Copy link

@maryamariyan Sure, I was thinking of forking the configuration library repo and checking if the solution is as easy as I think it is. I can put an official API proposal in when I have the time to do that in the next month or so.

@maryamariyan
Copy link
Member

maryamariyan commented Oct 27, 2020

I can put an official API proposal in when I have the time to do that in the next month or so.

Sure. Sounds good. The API proposal would be there to cover motivation for change, sample usage, and any risk involved with making the new changes. If you wanted to get more guidance check out api-review-process.md.

I was thinking of forking the configuration library repo and checking if the solution is as easy as I think it is.

@tvanbaak please note, this issue was transferred from dotnet/extensions and some of extensions source code moved to dotnet/runtime earlier in May.


The note below is just FYI in case you are keen on testing things out or plan on making future contributions in this repo.

In case you are new to this repo and interested to contribute:

If you wanted to play around with (e.g. Microsoft.Extensions.Configuration library) extensions source code or any other libraries in runtime, your first stop is to check workflow/requirements for dotnet/runtime.

Once you've made sure your machine has the requirements to build, then perhaps keep the building instructions with you as cheatsheet. That would help you while you do some prototypes on e.g. Microsoft.Extensions.Configuration,

And then in a sample console or web application point to your local copy of Microsoft.Extensions.Configuration by:

    <Reference Include="C:\runtimepath\artifacts\bin\Microsoft.Extensions.Configuration\netstandard2.0-Debug\Microsoft.Extensions.Configuration.dll" />

If the work involves API addition, then follow updating-ref-source.md section For most assemblies within libraries.


@MrAntix
Copy link

MrAntix commented Oct 13, 2021

as a proposal ...

public class TestSettings
{
    [DoNotMerge]
    public string[] TestArray { get; set; } = new[] { "123", "test1" };
}

@fairking
Copy link

fairking commented Oct 22, 2021

as a proposal (alternative way) ...

var appModules = new List<string>();
cfg.GetSection("Modules").Bind(appModules, o => o.ArrayMergeMode = ConfigurationArrayMergeMode.Override);
public enum ConfigurationArrayMergeMode
{
    Override,
    Merge,
    Append
}

However I deem that the option Override must be a default option in the same way as other types work (strings, objects).

Update 30/03/2022

I'd like to clarify things how I see it, based on my experience.

1. Override (default)

The default behaviour (IMHO) should be to replace the existing array. For example:
appsettings.json:

"Modules": [
    "Module1",
    "Module2",
    "Module3"
]

appsettings.development.json:

"Modules": [
    "Module4",
    "Module5"
]

Results in [ "Module4", "Module5" ]. Missing Modules section in appsettings.development.json does not override/replace, so the original array remains untouched. Empty or nullable array in appsettings.development.json should replace the original array accordingly (clear or set to null).

Please consider include other methods:

2. Merge

This is the current (wrong) behavior I never used and I probably will never ever use it. For example:
appsettings.json:

"Modules": [
    "Module1",
    "Module2",
    "Module3"
]

appsettings.development.json:

"Modules": [
    "Module4",
    "Module5"
]

Results in [ "Module4", "Module5", "Module3" ].

3. Append

This one is interesting behavior can be useful sometimes. For example:
appsettings.json:

"Modules": [
    "Module1",
    "Module2",
    "Module3"
]

appsettings.development.json:

"Modules": [
    "Module4",
    "Module5"
]

Results in [ "Module1", "Module2", "Module3", "Module4", "Module5", ].

People are asking too: https://stackoverflow.com/questions/51614792/how-to-override-an-asp-net-core-configuration-array-setting-reducing-length-of-t/69676630

@fairking
Copy link

@Eilon FYI

@maryamariyan
Copy link
Member

This merge/additive behavior is by design.

Closing as for the moment we don't have immediate plans to add this feature.

@marcopelegrini
Copy link

marcopelegrini commented Apr 22, 2022

This is very counter intuitive and very easily lead to errors and wasted time.
Please consider at least give developers the option to explicitly define how sections should be bound.

@poke
Copy link
Contributor

poke commented Apr 22, 2022

Please consider at least give developers the option to explicitly define how sections should be bound.

I understand the frustration here but you have to understand that this issue does not happen at bind time. Configuration and binding are actually two separate concepts which operate separately from each other. So even if the binder was able to control whether to append or replace items (which wouldn’t be too difficult to implement, e.g. with a custom collection type), the issue is that the underlying configuration does not support this.

I explain this issue also in this Stack Overflow answer but the basic idea is that configuration is loaded as a list of key/value pairs. This happens regardless of what your input format is. So a JSON configuration file is actually flattened into a format where you have flat keys and values. And the hierarchy within JSON is just represented by a key which has a “path” with a colon as the “path separator“.

So taking one of the examples above, a JSON "Modules": [ "Module1", "Module2", "Module3" ] would result in the following configuration values:

Modules:0 = Module1
Modules:1 = Module2
Modules:2 = Module3

And if there’s now another JSON file that contains "Modules": ["Module4", "Module5"], then this would result in these values:

Modules:0 = Module4
Modules:1 = Module5

At that time, the information that this is JSON is gone. So when the configuration system now overlays the configuration providers on top of each other, you will get these values:

Modules:0 = Module4
Modules:1 = Module5
Modules:2 = Module3

And of course, if you now run the configuration binder on this, then it shouldn’t be a surprise how the result comes to be.


So the underlying format of the configuration system simply does not allow a different behavior since the providers overwrite each other on the key/value items. What theoretically could work is if the binder would bind each configuration provider individually and then apply its own logic in merging this. But the system currently does not allow for this.

And changing it now would be a major breaking change considering that this is how the system has been behaving (by design!) for a little over six years.

@fairking
Copy link

fairking commented Apr 23, 2022

@poke

the basic idea is that configuration is loaded as a list of key/value pairs.

I see. Thanks for the explanation.

Bellow is my quick test:
image

So, the Build() is merging values. The Bind() has nothing to do with that.

The only workaround I can see today is to use a single string with comma separated values.

appsettings.json:

{
    "Modules": "Module1,Module2,Module3",
}

appsettings.development.json:

{
    "Modules": "Module4,Module5",
}

This is the result:
image

@maryamariyan maryamariyan reopened this Apr 25, 2022
@maryamariyan
Copy link
Member

Thanks for the engagement, closing,

@ghost ghost locked as resolved and limited conversation to collaborators May 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-Configuration
Projects
None yet
Development

No branches or pull requests