Fix handling invalid collection items in configuration (#97777)

* Fix handling invalid collection items in configuration * fix the test * add comment
dotnet · Feb 1, 2024 · 0fff4ef · 0fff4ef
1 parent 765090f
commit 0fff4ef
Show file tree

Hide file tree

Showing 13 changed files with 109 additions and 9 deletions.
diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs
@@ -328,7 +328,7 @@ private void EmitInitializeMethod(ObjectSpec type)
                 IEnumerable<PropertySpec> initOnlyProps = type.Properties.Where(prop => prop is { SetOnInit: true });
                 List<string> ctorArgList = new();
 
-                EmitStartBlock($"public static {type.TypeRef.FullyQualifiedName} {GetInitalizeMethodDisplayString(type)}({Identifier.IConfiguration} {Identifier.configuration}, {Identifier.BinderOptions}? {Identifier.binderOptions})");
+                EmitStartBlock($"public static {type.TypeRef.FullyQualifiedName} {GetInitializeMethodDisplayString(type)}({Identifier.IConfiguration} {Identifier.configuration}, {Identifier.BinderOptions}? {Identifier.binderOptions})");
                 _emitBlankLineBeforeNextStatement = false;
 
                 foreach (ParameterSpec parameter in type.ConstructorParameters)
@@ -709,6 +709,11 @@ private void EmitBindingLogicForEnumerableWithAdd(TypeRef elementTypeRef, string
                         break;
                     case ComplexTypeSpec complexType when _typeIndex.CanInstantiate(complexType):
                         {
+                            // If a section possesses a null or empty string value and lacks any children, we bind to the default value of the type.
+                            // In the case of a non-null or non-empty string value without any section children, binding cannot be performed at that moment,
+                            // and this section should be skipped.
+                            EmitBindCheckForSectionValue();
+
                             EmitBindingLogic(complexType, Identifier.value, Identifier.section, InitializationKind.Declaration, ValueDefaulting.None);
                             _writer.WriteLine($"{addExpr}({Identifier.value});");
                         }
@@ -718,6 +723,20 @@ private void EmitBindingLogicForEnumerableWithAdd(TypeRef elementTypeRef, string
                 EmitEndBlock();
             }
 
+            // EmitBindCheckForSectionValue produce the following code:
+            // if (!string.IsNullOrEmpty(section.Value) && !section.GetChildren().GetEnumerator().MoveNext()) { continue; }
+            //
+            // If a section possesses a null or empty string value and lacks any children, we bind to the default value of the type.
+            // In the case of a non-null or non-empty string value without any section children, binding cannot be performed at that moment,
+            // and this section should be skipped.
+            private void EmitBindCheckForSectionValue()
+            {
+                // We utilize GetEnumerator().MoveNext() instead of employing Linq's Any() since there is no assurance that the System.Linq reference is included.
+                EmitStartBlock($"if (!string.IsNullOrEmpty({Expression.sectionValue}) && !{Identifier.section}.{Identifier.GetChildren}().{Identifier.GetEnumerator}().{Identifier.MoveNext}())");
+                _writer.WriteLine($@"continue;");
+                EmitEndBlock();
+            }
+
             private void EmitBindCoreImplForDictionary(DictionarySpec type)
             {
                 EmitCollectionCastIfRequired(type, out string instanceIdentifier);
@@ -1132,7 +1151,7 @@ private bool EmitObjectInit(ComplexTypeSpec type, string memberAccessExpr, Initi
                     else
                     {
                         Debug.Assert(strategy is ObjectInstantiationStrategy.ParameterizedConstructor);
-                        string initMethodIdentifier = GetInitalizeMethodDisplayString(((ObjectSpec)type));
+                        string initMethodIdentifier = GetInitializeMethodDisplayString(((ObjectSpec)type));
                         initExpr = $"{initMethodIdentifier}({configArgExpr}, {Identifier.binderOptions})";
                     }
                 }
@@ -1175,7 +1194,7 @@ private bool EmitObjectInit(ComplexTypeSpec type, string memberAccessExpr, Initi
                         break;
                     default:
                         {
-                            Debug.Fail($"Invaild initialization kind: {initKind}");
+                            Debug.Fail($"Invalid initialization kind: {initKind}");
                         }
                         break;
                 }

diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/Helpers.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/Helpers.cs
@@ -107,6 +107,7 @@ private static class Identifier
                 public const string Get = nameof(Get);
                 public const string GetBinderOptions = nameof(GetBinderOptions);
                 public const string GetChildren = nameof(GetChildren);
+                public const string GetEnumerator = nameof(GetEnumerator);
                 public const string GetSection = nameof(GetSection);
                 public const string GetValue = nameof(GetValue);
                 public const string HasConfig = nameof(HasConfig);
@@ -121,6 +122,7 @@ private static class Identifier
                 public const string IOptionsChangeTokenSource = nameof(IOptionsChangeTokenSource);
                 public const string IServiceCollection = nameof(IServiceCollection);
                 public const string Length = nameof(Length);
+                public const string MoveNext = nameof(MoveNext);
                 public const string Name = nameof(Name);
                 public const string NumberStyles = nameof(NumberStyles);
                 public const string Parse = nameof(Parse);
@@ -261,7 +263,7 @@ private void EmitCheckForNullArgument_WithBlankLine(string paramName, bool useTh
 
             private string GetIncrementalIdentifier(string prefix) => $"{prefix}{_valueSuffixIndex++}";
 
-            private static string GetInitalizeMethodDisplayString(ObjectSpec type) =>
+            private static string GetInitializeMethodDisplayString(ObjectSpec type) =>
                 $"{nameof(MethodsToGen_CoreBindingHelper.Initialize)}{type.IdentifierCompatibleSubstring}";
         }
     }

diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs
@@ -428,7 +428,7 @@ private static void BindInstance(
             }
             else
             {
-                if (isParentCollection && bindingPoint.Value is null)
+                if (isParentCollection && bindingPoint.Value is null && string.IsNullOrEmpty(configValue))
                 {
                     // If we don't have an instance, try to create one
                     bindingPoint.TrySetValue(CreateInstance(type, config, options));

diff --git a/...soft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.Collections.cs b/...soft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.Collections.cs
@@ -3,7 +3,9 @@
 
 using System;
 using System.Collections.Generic;
+using System.IO;
 using System.Linq;
+using System.Text;
 #if BUILDING_SOURCE_GENERATOR_TESTS
 using Microsoft.Extensions.Configuration;
 #endif
@@ -286,7 +288,7 @@ private void GetIntDictionaryT<T>(T k1, T k2, T k3)
             var config = configurationBuilder.Build();
 
             var options = new Dictionary<T, string>();
-#pragma warning disable SYSLIB1104 
+#pragma warning disable SYSLIB1104
             config.GetSection("IntegerKeyDictionary").Bind(options);
 #pragma warning restore SYSLIB1104
 
@@ -724,13 +726,13 @@ public void ObjectDictionary()
         }
 
         [Fact]
-        public void ObjectDictionaryWithHarcodedElements()
+        public void ObjectDictionaryWithHardcodedElements()
         {
             var input = new Dictionary<string, string>
             {
                 {"ObjectDictionary:abc:Integer", "1"},
-                {"ObjectDictionary:def", "null"},
-                {"ObjectDictionary:ghi", "null"}
+                {"ObjectDictionary:def", null },
+                {"ObjectDictionary:ghi", null }
             };
 
             var configurationBuilder = new ConfigurationBuilder();
@@ -2332,6 +2334,40 @@ public void TestMutatingDictionaryValues()
             Assert.Equal("NewValue", dict["Key"][1]);
         }
 
+        [Fact]
+        public void TestCollectionWithNullOrEmptyItems()
+        {
+            string json = @"
+                {
+                    ""CollectionContainer"": [
+                    {
+                        ""Elements"":
+                        {
+                            ""Typdde"": ""UserCredentials"",
+                            ""foo"": ""00"",
+                            ""111"": """",
+                            ""BaseUrl"": ""cccccc"",
+                            ""Valid"": {
+                                ""Type"": ""System.Boolean""
+                            },
+                        }
+                    }
+                    ]
+                }
+            ";
+
+            var builder = new ConfigurationBuilder();
+            Stream stream = new MemoryStream(Encoding.UTF8.GetBytes(json));
+            builder.AddJsonStream(stream);
+            IConfigurationRoot config = builder.Build();
+
+            List<CollectionContainer> result = config.GetSection("CollectionContainer").Get<List<CollectionContainer>>();
+            Assert.Equal(1, result.Count);
+            Assert.Equal(2, result[0].Elements.Count);
+            Assert.Null(result[0].Elements[0].Type);
+            Assert.Equal("System.Boolean", result[0].Elements[1].Type);
+        }
+
         // Test behavior for root level arrays.
 
         // Tests for TypeConverter usage.

diff --git a/...ons.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.Collections.cs b/...ons.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.Collections.cs
@@ -377,5 +377,16 @@ public class OptionsWithDifferentCollectionInterfaces
             public IList<string> UnInstantiatedIList { get; set; }
             public IReadOnlyList<string> UnInstantiatedIReadOnlyList { get; set; }
         }
+
+        public class CollectionContainer
+        {
+            public string Name { get; set; }
+            public List<Element> Elements { get; set; }
+        }
+
+        public class Element
+        {
+            public string Type { get; set; }
+        }
     }
 }
diff --git a/.../tests/SourceGenerationTests/Baselines/net462/ServiceCollection/Configure_T.generated.txt b/.../tests/SourceGenerationTests/Baselines/net462/ServiceCollection/Configure_T.generated.txt
@@ -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())
+                {
+                    continue;
+                }
                 var value = new global::Program.MyClass2();
                 BindCore(section, ref value, defaultValueIfNotFound: false, binderOptions);
                 instance.Add(value);

diff --git a/...enerationTests/Baselines/net462/ServiceCollection/Configure_T_BinderOptions.generated.txt b/...enerationTests/Baselines/net462/ServiceCollection/Configure_T_BinderOptions.generated.txt
@@ -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())
+                {
+                    continue;
+                }
                 var value = new global::Program.MyClass2();
                 BindCore(section, ref value, defaultValueIfNotFound: false, binderOptions);
                 instance.Add(value);

diff --git a/...s/SourceGenerationTests/Baselines/net462/ServiceCollection/Configure_T_name.generated.txt b/...s/SourceGenerationTests/Baselines/net462/ServiceCollection/Configure_T_name.generated.txt
@@ -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())
+                {
+                    continue;
+                }
                 var value = new global::Program.MyClass2();
                 BindCore(section, ref value, defaultValueIfNotFound: false, binderOptions);
                 instance.Add(value);

diff --git a/...tionTests/Baselines/net462/ServiceCollection/Configure_T_name_BinderOptions.generated.txt b/...tionTests/Baselines/net462/ServiceCollection/Configure_T_name_BinderOptions.generated.txt
@@ -113,6 +113,10 @@ namespace Microsoft.Extensions.Configuration.Binder.SourceGeneration
         {
             foreach (IConfigurationSection section in configuration.GetChildren())
             {
+                if (!string.IsNullOrEmpty(section.Value) && !section.GetChildren().GetEnumerator().MoveNext())
+                {
+                    continue;
+                }
                 var value = new global::Program.MyClass2();
                 BindCore(section, ref value, defaultValueIfNotFound: false, binderOptions);
                 instance.Add(value);

diff --git a/...ts/SourceGenerationTests/Baselines/netcoreapp/ServiceCollection/Configure_T.generated.txt b/...ts/SourceGenerationTests/Baselines/netcoreapp/ServiceCollection/Configure_T.generated.txt
@@ -113,6 +113,10 @@ namespace Microsoft.Extensions.Configuration.Binder.SourceGeneration
         {
             foreach (IConfigurationSection section in configuration.GetChildren())
             {
+                if (!string.IsNullOrEmpty(section.Value) && !section.GetChildren().GetEnumerator().MoveNext())
+                {
+                    continue;
+                }
                 var value = new global::Program.MyClass2();
                 BindCore(section, ref value, defaultValueIfNotFound: false, binderOptions);
                 instance.Add(value);

diff --git a/...ationTests/Baselines/netcoreapp/ServiceCollection/Configure_T_BinderOptions.generated.txt b/...ationTests/Baselines/netcoreapp/ServiceCollection/Configure_T_BinderOptions.generated.txt
@@ -113,6 +113,10 @@ namespace Microsoft.Extensions.Configuration.Binder.SourceGeneration
         {
             foreach (IConfigurationSection section in configuration.GetChildren())
             {
+                if (!string.IsNullOrEmpty(section.Value) && !section.GetChildren().GetEnumerator().MoveNext())
+                {
+                    continue;
+                }
                 var value = new global::Program.MyClass2();
                 BindCore(section, ref value, defaultValueIfNotFound: false, binderOptions);
                 instance.Add(value);

diff --git a/...urceGenerationTests/Baselines/netcoreapp/ServiceCollection/Configure_T_name.generated.txt b/...urceGenerationTests/Baselines/netcoreapp/ServiceCollection/Configure_T_name.generated.txt
@@ -113,6 +113,10 @@ namespace Microsoft.Extensions.Configuration.Binder.SourceGeneration
         {
             foreach (IConfigurationSection section in configuration.GetChildren())
             {
+                if (!string.IsNullOrEmpty(section.Value) && !section.GetChildren().GetEnumerator().MoveNext())
+                {
+                    continue;
+                }
                 var value = new global::Program.MyClass2();
                 BindCore(section, ref value, defaultValueIfNotFound: false, binderOptions);
                 instance.Add(value);

diff --git a/...Tests/Baselines/netcoreapp/ServiceCollection/Configure_T_name_BinderOptions.generated.txt b/...Tests/Baselines/netcoreapp/ServiceCollection/Configure_T_name_BinderOptions.generated.txt
@@ -107,6 +107,10 @@ namespace Microsoft.Extensions.Configuration.Binder.SourceGeneration
         {
             foreach (IConfigurationSection section in configuration.GetChildren())
             {
+                if (!string.IsNullOrEmpty(section.Value) && !section.GetChildren().GetEnumerator().MoveNext())
+                {
+                    continue;
+                }
                 var value = new global::Program.MyClass2();
                 BindCore(section, ref value, defaultValueIfNotFound: false, binderOptions);
                 instance.Add(value);