Skip to content

Conversation

@mapogolions
Copy link
Contributor

At the moment, binding to dictionary has two different behaviors depending on the nesting level.

  1. Behaviour 1 - add | override | preserve. Please follow the links below for each case
    add new & preserve existing keys
    override existing keys

  2. Behaviour 2 - skip all existing keys & add new
    Meaning that new keys from a configuration will be picked up, but already existing dropped
    Please see the commit that reproduces the issue and shows in which cases this happens.

This PR attempts to correct this inconsistent behavior and use the first approach as baseline.

@ghost ghost added community-contribution Indicates that the PR has been added by a community member area-Extensions-Configuration labels Jan 15, 2022
@ghost
Copy link

ghost commented Jan 15, 2022

Tagging subscribers to this area: @dotnet/area-extensions-configuration
See info in area-owners.md if you want to be subscribed.

Issue Details

At the moment, binding to dictionary has two different behaviors depending on the nesting level.

  1. Behaviour 1 - add | override | preserve. Please follow the links below for each case
    add new & preserve existing keys
    override existing keys

  2. Behaviour 2 - skip all existing keys & add new
    Meaning that new keys from a configuration will be picked up, but already existing dropped
    Please see the commit that reproduces the issue and shows in which cases this happens.

This PR attempts to correct this inconsistent behavior and use the first approach as baseline.

Author: mapogolions
Assignees: -
Labels:

area-Extensions-Configuration, community-contribution

Milestone: -

@mapogolions
Copy link
Contributor Author

@eerhardt Thanks for review. I've added a unit test for a dictionary with an enum as key type

@eerhardt eerhardt self-assigned this Jan 18, 2022
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me. Thanks for the contribution!

@maryamariyan - any thoughts?

@eerhardt
Copy link
Member

OSX Failure is the same listed in #63439 (comment).

@maryamariyan
Copy link
Contributor

maryamariyan commented Jan 21, 2022

There's a conflict on the ConfigurationBinder file left to resolve. I think happened after merging your other PR.

Since Microsoft.Extensions.Configuration is widely used we're always very careful with taking risk on fixing/changing behaviors in it in case there are clients who already rely on the existing behavior, even if it is not correct.

For the very least we'd need to document the breaking change. @mapogolions do you see any use cases where we'd have regressing behavior after taking in this change? Good thing at least seems no existing tests had to change

cc @HaoK

@mapogolions
Copy link
Contributor Author

@maryamariyan
The conflict was probably caused by this PR that wraps each iteration in a try-catch block. In my opinion, this PR should not affect existing code.
Old approach:
drop existing keys & add new keys == (override if key exists | add key if not found in the origin) & remove all keys that are unique only to the origin
New approach:
(override if key exists | add key if not found in the origin) & do nothing
As you can see, only the tail has been changed

Copy link
Contributor

@maryamariyan maryamariyan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM aside from the merge conflict that needs to be resolved

@eerhardt
Copy link
Member

eerhardt commented Feb 7, 2022

Failure is unrelated. Merging.

@eerhardt eerhardt merged commit c70e282 into dotnet:main Feb 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Mar 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-Extensions-Configuration community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants