From ae5c1c8f29c1bc0a73a211da22967ba98af42c59 Mon Sep 17 00:00:00 2001 From: Eric StJohn Date: Wed, 20 Sep 2023 15:43:50 -0700 Subject: [PATCH 1/3] Ensure Bind can handle null from GetSection IConfiguration instances may return a null value from GetSection. We were not handling this and would throw a NullReferenceException. --- .../src/ConfigurationBinder.cs | 5 ++++ .../tests/Common/ConfigurationBinderTests.cs | 23 +++++++++++++++++++ ...ation.Binder.SourceGeneration.Tests.csproj | 1 + ...tensions.Configuration.Binder.Tests.csproj | 1 + 4 files changed, 30 insertions(+) diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs index 74365d8084aa3..b06090138a8b3 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs @@ -298,6 +298,11 @@ private static void BindInstance( return; } + if (config == null) + { + return; + } + var section = config as IConfigurationSection; string? configValue = section?.Value; if (configValue != null && TryConvertValue(type, configValue, section?.Path, out object? convertedValue, out Exception? error)) diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs index 7c955e789184c..49ab44604eb4c 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs @@ -12,6 +12,7 @@ using Microsoft.Extensions.Configuration; #endif using Microsoft.Extensions.Configuration.Test; +using Moq; using Xunit; namespace Microsoft.Extensions @@ -2404,5 +2405,27 @@ public void SharedChildInstance() config.GetSection("A").Bind(instance); Assert.Equal("localhost", instance.ConnectionString); } + + [Fact] + public void CanBindToMockConfiugrationSection() + { + double expectedLatitude = 42.0002d; + var mockConfSection = new Mock(); + var latitudeSection = new Mock(); + latitudeSection.Setup(m => m.Value).Returns(expectedLatitude.ToString()); + + // only mock one of the two properties, the other will return a null section. + // runtime binder uses GetSection + mockConfSection.Setup(config => config.GetSection(nameof(GeolocationClass.Latitude))).Returns(latitudeSection.Object); + // source gen uses indexer + mockConfSection.Setup(config => config[nameof(GeolocationClass.Latitude)]).Returns(latitudeSection.Object?.Value); + mockConfSection.Setup(config => config.GetChildren()).Returns(new[] { latitudeSection.Object }); + + GeolocationClass result = new(); + mockConfSection.Object.Bind(result); + + Assert.Equal(expectedLatitude, result.Latitude); + Assert.Equal(default(double), result.Longitude); + } } } diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj index fc8db157eddee..5f8366c1e6836 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj @@ -43,6 +43,7 @@ + diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/UnitTests/Microsoft.Extensions.Configuration.Binder.Tests.csproj b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/UnitTests/Microsoft.Extensions.Configuration.Binder.Tests.csproj index fc859a6f71bbe..c81b5a1232c60 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/UnitTests/Microsoft.Extensions.Configuration.Binder.Tests.csproj +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/UnitTests/Microsoft.Extensions.Configuration.Binder.Tests.csproj @@ -25,6 +25,7 @@ + From 2a8cdd46ced1b339a2c30a4b4a7f5d2baa964926 Mon Sep 17 00:00:00 2001 From: Eric StJohn Date: Wed, 20 Sep 2023 18:30:28 -0700 Subject: [PATCH 2/3] Address feedback --- .../src/ConfigurationBinder.cs | 2 +- .../ConfigurationBinderTests.TestClasses.cs | 6 +++++ .../tests/Common/ConfigurationBinderTests.cs | 26 ++++++++++--------- 3 files changed, 21 insertions(+), 13 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs index b06090138a8b3..dfc35d8020855 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs @@ -298,7 +298,7 @@ private static void BindInstance( return; } - if (config == null) + if (config is null) { return; } diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs index f47cdbe6dbbb5..48474033ec1f8 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs @@ -888,5 +888,11 @@ public int MyIntProperty } } + public class SimplePoco + { + public string A { get; set; } + public string B { get; set; } + } + } } diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs index 49ab44604eb4c..bd3ee8392a0dd 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs @@ -1768,7 +1768,7 @@ public void EnsureCallingThePropertySetter() Assert.Equal(0, options.OtherCodeNullable); Assert.Equal("default", options.OtherCodeString); Assert.Null(options.OtherCodeNull); - Assert.Null(options.OtherCodeUri); + Assert.Null(options.OtherCodeUri); } [Fact] @@ -2239,7 +2239,7 @@ void TestUntypedOverloads(IConfiguration? configuration, string? key) Assert.Throws(() => configuration.GetValue(typeof(GeolocationClass), key, new GeolocationClass())); Assert.Throws(() => configuration.GetValue(typeof(Geolocation), key)); Assert.Throws(() => configuration.GetValue(typeof(Geolocation), key, defaultValue: null)); - Assert.Throws(() => configuration.GetValue(typeof(Geolocation), key, default(Geolocation))); + Assert.Throws(() => configuration.GetValue(typeof(Geolocation), key, default(Geolocation))); } } @@ -2406,26 +2406,28 @@ public void SharedChildInstance() Assert.Equal("localhost", instance.ConnectionString); } - [Fact] + // Moq heavily utilizes RefEmit, which does not work on most aot workloads + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))] public void CanBindToMockConfiugrationSection() { - double expectedLatitude = 42.0002d; + const string expectedA = "hello"; + var mockConfSection = new Mock(); - var latitudeSection = new Mock(); - latitudeSection.Setup(m => m.Value).Returns(expectedLatitude.ToString()); + var aSection = new Mock(); + aSection.Setup(m => m.Value).Returns(expectedA); // only mock one of the two properties, the other will return a null section. // runtime binder uses GetSection - mockConfSection.Setup(config => config.GetSection(nameof(GeolocationClass.Latitude))).Returns(latitudeSection.Object); + mockConfSection.Setup(config => config.GetSection(nameof(SimplePoco.A))).Returns(aSection.Object); // source gen uses indexer - mockConfSection.Setup(config => config[nameof(GeolocationClass.Latitude)]).Returns(latitudeSection.Object?.Value); - mockConfSection.Setup(config => config.GetChildren()).Returns(new[] { latitudeSection.Object }); + mockConfSection.Setup(config => config[nameof(SimplePoco.A)]).Returns(aSection.Object?.Value); + mockConfSection.Setup(config => config.GetChildren()).Returns(new[] { aSection.Object }); - GeolocationClass result = new(); + SimplePoco result = new(); mockConfSection.Object.Bind(result); - Assert.Equal(expectedLatitude, result.Latitude); - Assert.Equal(default(double), result.Longitude); + Assert.Equal(expectedA, result.A); + Assert.Equal(default(string), result.B); } } } From fdc4a25657439394a56e53721c431dc5776eda79 Mon Sep 17 00:00:00 2001 From: Eric StJohn Date: Thu, 21 Sep 2023 12:46:58 -0700 Subject: [PATCH 3/3] Remove Moq from ConfigBinder tests --- .../tests/Common/ConfigurationBinderTests.cs | 39 ++++++++++++------- ...ation.Binder.SourceGeneration.Tests.csproj | 1 - ...tensions.Configuration.Binder.Tests.csproj | 1 - 3 files changed, 24 insertions(+), 17 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs index bd3ee8392a0dd..7e635c1f0bdaa 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs @@ -11,8 +11,8 @@ #if BUILDING_SOURCE_GENERATOR_TESTS using Microsoft.Extensions.Configuration; #endif +using Microsoft.Extensions.Configuration.Memory; using Microsoft.Extensions.Configuration.Test; -using Moq; using Xunit; namespace Microsoft.Extensions @@ -2406,28 +2406,37 @@ public void SharedChildInstance() Assert.Equal("localhost", instance.ConnectionString); } - // Moq heavily utilizes RefEmit, which does not work on most aot workloads - [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))] - public void CanBindToMockConfiugrationSection() + [Fact] + public void CanBindToMockConfigurationSection() { const string expectedA = "hello"; - var mockConfSection = new Mock(); - var aSection = new Mock(); - aSection.Setup(m => m.Value).Returns(expectedA); - - // only mock one of the two properties, the other will return a null section. - // runtime binder uses GetSection - mockConfSection.Setup(config => config.GetSection(nameof(SimplePoco.A))).Returns(aSection.Object); - // source gen uses indexer - mockConfSection.Setup(config => config[nameof(SimplePoco.A)]).Returns(aSection.Object?.Value); - mockConfSection.Setup(config => config.GetChildren()).Returns(new[] { aSection.Object }); + var configSource = new MemoryConfigurationSource() + { + InitialData = new Dictionary() + { + [$":{nameof(SimplePoco.A)}"] = expectedA, + } + }; + var configRoot = new MockConfigurationRoot(new[] { configSource.Build(null) }); + var configSection = new ConfigurationSection(configRoot, string.Empty); SimplePoco result = new(); - mockConfSection.Object.Bind(result); + configSection.Bind(result); Assert.Equal(expectedA, result.A); Assert.Equal(default(string), result.B); } + + // a mock configuration root that will return null for undefined Sections, + // as is common when Configuration interfaces are mocked + class MockConfigurationRoot : ConfigurationRoot, IConfigurationRoot + { + public MockConfigurationRoot(IList providers) : base(providers) + { } + + IConfigurationSection IConfiguration.GetSection(string key) => + this[key] is null ? null : new ConfigurationSection(this, key); + } } } diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj index 5f8366c1e6836..fc8db157eddee 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj @@ -43,7 +43,6 @@ - diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/UnitTests/Microsoft.Extensions.Configuration.Binder.Tests.csproj b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/UnitTests/Microsoft.Extensions.Configuration.Binder.Tests.csproj index c81b5a1232c60..fc859a6f71bbe 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/UnitTests/Microsoft.Extensions.Configuration.Binder.Tests.csproj +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/UnitTests/Microsoft.Extensions.Configuration.Binder.Tests.csproj @@ -25,7 +25,6 @@ -