Fix handling invalid collection items in configuration #97777
Conversation
ghost
assigned 
|
Tagging subscribers to this area: @dotnet/area-extensions-configuration Issue DetailsIn .NET 8.0, a breaking change was introduced through PR. This modification aimed to enable the recognition of empty or null collection items in configuration. However, it inadvertently led to the acceptance of invalid non-null or non-empty items as well. This fix addresses the issue both in the runtime binder and the source generator.
|
src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs
Show resolved
Hide resolved
...crosoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.Collections.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs
Show resolved
Hide resolved
| @@ -119,6 +119,10 @@ namespace Microsoft.Extensions.Configuration.Binder.SourceGeneration | |||
| { | |||
| foreach (IConfigurationSection section in configuration.GetChildren()) | |||
| { | |||
| if (!string.IsNullOrEmpty(section.Value) && !section.GetChildren().GetEnumerator().MoveNext()) | |||
There was a problem hiding this comment.
Couldn't this be generated using !section.GetChildren().Any() instead? It could theoretically avoid the enumerator allocation provided that the underlying enumerable implements ICollection<T>.
There was a problem hiding this comment.
This will need to add a dependency on System.Linq which comes with its own complication if the project not referencing it.
There was a problem hiding this comment.
There are plenty of existing spots in this generated code that might benefit from Linq, or even this API: #83692
Not sure we should tackle introducing new dependencies to this code at this point for a regression bugfix. Instead we should think about minimizing this change and reducing risk as we'll likely want to backport it. Codebase refactoring can be captured in a separate issue.
There was a problem hiding this comment.
It's not technically adding a dependency that wasn't there before though, right? All supported TFMs include System.Linq.
There was a problem hiding this comment.
We'll address this in a different PR.
|
@tarekgh , my bad. Thanks for fix |
|
@Maximys no worries. I tagged you only for awareness and if you have any feedback. Thanks for all help you have provided all the way. |
#97558
In .NET 8.0, a breaking change was introduced through PR. This modification aimed to enable the recognition of empty or null collection items in configuration. However, it inadvertently led to the acceptance of invalid non-null or non-empty items as well. This fix addresses the issue both in the runtime binder and the source generator.