Fix Configuration Binding with Instantiated Objects #81050

tarekgh · 2023-01-23T21:22:46Z

Un-mutate the settable collection instances during the binding.

ghost · 2023-01-23T21:23:02Z

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

Issue Details

#79766

When binding read-only collection interface against instantiated object, we shouldn't mutate this object or replace it with another created object. The reason is we cannot create a new collection instance behaving exactly as the instantiated one. For example, users can create a collection (e.g. Set) with specific string comparer behavior.
The change here is to revert to the .NET 6.0 behavior.

Author:	tarekgh
Assignees:	-
Labels:	`area-Extensions-Configuration`
Milestone:	-

tarekgh · 2023-01-23T21:24:54Z

CC @ericstj @SteveDunn @michasHL @layomia

halter73

This makes the issue described in #79766 worse when the collection type is immutable. If MySet was declared as an IReadOnlySet instead of an ISet, even the "// workaround" would break after this change, since the set will now contain no items after binding.

It's one thing to skip over unsettable collections, but completely skipping over settable collections that we would otherwise set if the initial value was null is going too far in my opinion. I know that this is the pre-7.0 behavior for IEnumerable, but this old behavior was reported as a bug by several customers in #36390. If you actually wanted the binder to skip over the property, you could remove the public setter.

I think the more targeted fix would be to prefer mutating the existing instance if it's declared as a mutable collection type like we did in .NET 6. This should be enough to fix the regression demonstrated in #79766 and be relatively safe to backport.

If we feel strongly that overwriting a non-null, immutable collection is too surprising because it could lose customizations from the original instance, we could throw instead of skip. The exception message could suggest removing the public setter in the unlikely case that the developer really did want to skip binding the collection. That wouldn't be a good candidate for backporting of course.

src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

tarekgh · 2023-01-24T18:07:56Z

@halter73 I have scoped the fix to settable collections only if you want to take a second look. Thanks!

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

tarekgh · 2023-01-26T23:20:19Z

/backport to release/7.0

github-actions · 2023-01-26T23:20:32Z

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/4020016056

github-actions · 2023-01-26T23:22:50Z

@tarekgh backporting to release/7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Fix Configuration Binding with Instantiated Objects
Using index info to reconstruct a base tree...
M	src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs
M	src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs
CONFLICT (content): Merge conflict in src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fix Configuration Binding with Instantiated Objects
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2023-01-26T23:22:51Z

@tarekgh an error occurred while backporting to release/7.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

eerhardt · 2023-01-30T15:53:47Z

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

@@ -315,12 +315,15 @@ private static void BindProperty(PropertyInfo property, object instance, IConfig
                }

                // for sets and read-only set interfaces, we clone what's there into a new collection, if we can


Is this comment now incorrect?

We need to remove the word sets from the comment.

I think the comment needs to be beefed up a bit to describe the whole behavior.

eerhardt · 2023-01-30T15:56:01Z

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

+                        if (!bindingPoint.IsReadOnly && newValue != null)
+                        {
+                            bindingPoint.SetValue(newValue);
+                        }
                    }

                    return;


Why isn't the if (TypeIsADictionaryInterface(type) section below symmetrical with the above if (TypeIsASetInterface(type)? It feels wrong that these two checks are different.

eerhardt · 2023-01-30T15:58:44Z

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

@@ -737,32 +748,38 @@ private static Array BindArray(Type type, IEnumerable? source, IConfiguration co
        {
            Type elementType = type.GetGenericArguments()[0];

-            Type keyType = type.GenericTypeArguments[0];
+            bool keyTypeIsEnum = elementType.IsEnum;


keyType is no longer used. Should this be changed to:

bool elementTypeIsEnum = elementType.IsEnum;

eerhardt · 2023-01-30T16:03:52Z

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

            {
-                foreach (object? item in orig)
+                dictionaryType = typeof(Dictionary<,>).MakeGenericType(keyType, valueType);
+                var dictionary = Activator.CreateInstance(dictionaryType);


object? dictionary = Activator.CreateInstance(dictionaryType);

yes, make sense.

eerhardt · 2023-01-30T16:05:04Z

src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs

@@ -812,7 +812,7 @@ public void CanBindISetNoSetter()

            var config = configurationBuilder.Build();

-            var options = config.Get<ComplexOptions>()!;
+            var options = config.Get<ComplexOptions>(o => o.ErrorOnUnknownConfiguration = true)!;


What was this change for?

I added this to get the exception thrown instead of skipping the configuration. Just to help in the case of the test failure.

Fix Configuration Binding with Instantiated Objects

8d7d920

ghost added the area-Extensions-Configuration label Jan 23, 2023

ghost assigned tarekgh Jan 23, 2023

tarekgh requested a review from eerhardt January 23, 2023 21:23

tarekgh added this to the 8.0.0 milestone Jan 23, 2023

tarekgh added 2 commits January 23, 2023 14:34

Add Extra test

1493935

Fix test build break

ea81eba

halter73 reviewed Jan 24, 2023

View reviewed changes

src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs Outdated Show resolved Hide resolved

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs Outdated Show resolved Hide resolved

Scoping the fix to settable collections only

a7761d5

halter73 approved these changes Jan 24, 2023

View reviewed changes

tarekgh added 3 commits January 24, 2023 15:55

Address the feedback

36c25ca

Add log messages to the test to get more info about CI failure

438a77c

Add logs to test

b8c73f9

tarekgh mentioned this pull request Jan 25, 2023

Improve performance of ConfigurationBinder (#81183) #81185

Closed

tarekgh added 2 commits January 25, 2023 12:14

Exclude ISET<T> from the test trimming

4f7a43c

Fix the trimming file to point at correct assembly

c047d04

build-analysis bot mentioned this pull request Jan 26, 2023

Tracking issue for CI build timeouts #76454

Closed

tarekgh merged commit 144dbcb into dotnet:main Jan 26, 2023

tarekgh deleted the FixConfigurationBindingWithInstantiatedObjects branch January 26, 2023 20:50

tarekgh mentioned this pull request Jan 27, 2023

[release/7.0] Fix Configuration Binding with Instantiated Objects #81250

Merged

eerhardt reviewed Jan 30, 2023

View reviewed changes

tarekgh mentioned this pull request Jan 30, 2023

Address ConfigurationBinder feedback #81384

Merged

ghost locked as resolved and limited conversation to collaborators Mar 1, 2023

		@@ -315,12 +315,15 @@ private static void BindProperty(PropertyInfo property, object instance, IConfig
		}

		// for sets and read-only set interfaces, we clone what's there into a new collection, if we can

Fix Configuration Binding with Instantiated Objects #81050

Fix Configuration Binding with Instantiated Objects #81050

Uh oh!

Conversation

tarekgh commented Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jan 23, 2023

Uh oh!

tarekgh commented Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

halter73 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tarekgh commented Jan 24, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tarekgh commented Jan 26, 2023

Uh oh!

github-actions bot commented Jan 26, 2023

Uh oh!

github-actions bot commented Jan 26, 2023

Uh oh!

github-actions bot commented Jan 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tarekgh commented Jan 23, 2023 •

edited

Loading

tarekgh commented Jan 23, 2023 •

edited

Loading