Allow ConfigBinder to bind arrays to Singular elements

[vidommet]

When trying to bind a collection of type T, ConfigBinder will now try to bind to the current object instead of children objects when the provided configuration doesn't contain an array.

This change allows elements that are not arrays to be bound to arrays. For example, a type string[] can now be bound to "key" in case "key:0" doesn't exist.

[ghost]

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

Issue Details

---

When trying to bind a collection of type T, ConfigBinder will now try to bind to the current object instead of children objects when the provided configuration doesn't contain an array.

This change allows elements that are not arrays to be bound to arrays. For example, a type string[] can now be bound to "key" in case "key:0" doesn't exist.

Author:	vidommet
Assignees:	-
Labels:	area-Extensions-Configuration
Milestone:	-

[dnfadmin]

All CLA requirements met.

[eerhardt]

Can we extract this into a helper method and use it in both places? Also it would be nice to write a comment or two about what this is doing, and why.

[safern]

To provide more context this is to be able to bind xml represented arrays as a single element array. Unlike JSON where you express when an object is an array in XML you can't represent that, so this would not bind correctly and create an array with a single null element, i.e this would return a Metadata array with a null element on it.

<TvShows>
  <Metadata>
    <Name>Prison Break</Name>
  </Metadata>
</TvShows>

public class TvShows
{
  public Metadata[] Metadata { get; set; }
}

public class Metadata
{
  public string Name { get; set; }
}

The reason why we can't fix this in the provider is because we don't have a way to know when the current node is expected to bind to an array, so we can't just append a 0 index to the key when we have a single child node.

cc: @davidfowl @maryamariyan for thoughts on taking this change now as it kind of changes the expected behavior, i.e if I now have a JSON like:

{
  "elements": "element1"
}

elements could be bound to a string[] even though it is not represented as an array on the JSON. However I'm supportive of this change since XML is broken for this scenario. Another option would be to put this behind a BinderOption flag.

[safern]

Also, @HaoK for his thoughts.

[HaoK]

This doesn't sound unreasonable, but I'd put it behind a option flag for sure, since this seems reasonable safe to enable by default

[safern]

Thanks, @HaoK, I agree with you. I've opened: #57325 to review the flag addition.

@vidommet will close this PR and once that flag is approved we can reopen it and add the flag and put the new behavior behind the flag.

[maryamariyan]

@HaoK let us know if you know of a better way for this condition

[vidommet]

If any of the children is an integer (key:999), this assumes the actual object is an array.

In real world, either all children are integers or all are not. But doing "Any" prevents usages in weird cases where someone used both things
having key:a, key:0 doesn't make sense but i am assuming that it is an array (as in the old behavior)

[HaoK]

So the old behavior always used the children correct? Why does this need to check if any children are a number? What doesn't work if this is just returning the children if there are any, otherwise the config.

[vidommet]

This used to always return children for arrays/collections. But as part of my change, I might have to return the parent object or children object (when it contains array as a children vs non-array).
If children are a number, that means the children are an array and we select that. Otherwise, we select the parent object as a single-element array.
When there are nested objects (keys - "a:b", "a:c"), there are children but they cannot be bound. You have to select the parent object ("a" itself) for the single-element array to be bound.

[maryamariyan]

LGTM aside from comments

[eerhardt]

In a perfect world, this should use RemoteExecutor. Since this switch is process-wide, setting this switch can affect other tests running in the process.

[davidfowl]

Did we really add a global switch for configuration behavior. This makes me sad 😢

[eerhardt]

We discussed it at length on in API Review. The decision was that we didn't think people need/want the old behavior. Thus we didn't want to add a public API for it. But since this is a behavior change, we wanted a way to turn the behavior off, just in case someone needed it.

See #57325 (comment)

[eerhardt]

Does this need to be cached? Reading from AppContext each time in a loop might be a perf issue.

[ghost]

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

[maryamariyan]

I'll apply the remaining feedback on a follow up PR.

[mapogolions]

@maryamariyan Could you please take a look at the problem caused by the changes #58330

[maryamariyan]

We reverted this PR in #59716, so I'll remove the breaking change labels rather than opening issue in dotnet/docs