From ed02842756a16d3a2caa2d85c0ffc0931490e084 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Tue, 10 Jan 2023 07:48:28 -0800 Subject: [PATCH 1/3] Fix Configuration to ensure calling the property setters. --- .../src/ConfigurationBinder.cs | 8 +++ .../tests/ConfigurationBinderTests.cs | 57 +++++++++++++++++++ ...tensions.Configuration.Binder.Tests.csproj | 3 +- 3 files changed, 67 insertions(+), 1 deletion(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs index 9266f9325ad696..a1a6d8a73d0fc4 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs @@ -382,6 +382,14 @@ private static void BindInstance( } } } + + if (!bindingPoint.HasNewValue && !bindingPoint.IsReadOnly && bindingPoint.Value is not null) + { + // For property binding, there are some cases when HasNewValue is not set in BindingPoint while a non-null Value inside that object can be retrieved + // from the property getter. As example, when binding a property which not having a configuration entry matching this property and the getter can initialize the Value. + // It is important to set the HasNewValue to true at this time to force the property setter to run. The reason is the setters can have a logic adjusting the Value. + bindingPoint.SetValue(bindingPoint.Value); + } } [RequiresDynamicCode(DynamicCodeWarningMessage)] diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs index 521501d5938de3..7c2792780ea263 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs @@ -1,6 +1,8 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.Configuration.Test; using System; using System.Collections; using System.Collections.Generic; @@ -2578,6 +2580,61 @@ public void CanBindPrivatePropertiesFromBaseClass() Assert.Equal("a", test.ExposePrivatePropertyValue()); } + [Fact] + public void EnsureCallingThePropertySetter() + { + var json = @"{ + ""IPFiltering"": { + ""HttpStatusCode"": 401, + ""Blacklist"": [ ""192.168.0.10-192.168.10.20"", ""fe80::/10"" ] + } + }"; + + var configuration = new ConfigurationBuilder() + .AddJsonStream(TestStreamHelpers.StringToStream(json)) + .Build(); + + OptionWithCollectionProperties options = configuration.GetSection("IPFiltering").Get(); + + Assert.NotNull(options); + Assert.Equal(2, options.Blacklist.Count); + Assert.Equal("192.168.0.10-192.168.10.20", options.Blacklist.ElementAt(0)); + Assert.Equal("fe80::/10", options.Blacklist.ElementAt(1)); + + Assert.Equal(2, options.ParsedBlacklist.Count); // should be initialized when calling the options.Blacklist setter. + + Assert.Equal(401, options.HttpStatusCode); // exists in configuration and properly sets the property + Assert.Equal(2, options.OtherCode); // doesn't exist in configuration. the setter sets default value '2' + } + + public class OptionWithCollectionProperties + { + private int _otherCode; + private ICollection blacklist = new HashSet(); + + public ICollection Blacklist + { + get => this.blacklist; + set + { + this.blacklist = value ?? new HashSet(); + this.ParsedBlacklist = this.blacklist.Select(b => b).ToList(); + } + } + + public int HttpStatusCode { get; set; } = 0; + + // ParsedBlacklist initialized using the setter of Blacklist. + public ICollection ParsedBlacklist { get; private set; } = new HashSet(); + + // This property not having any match in the configuration. Still the setter need to be called during the binding. + public int OtherCode + { + get => _otherCode; + set => _otherCode = value == 0 ? 2 : value; + } + } + private interface ISomeInterface { } diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Microsoft.Extensions.Configuration.Binder.Tests.csproj b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Microsoft.Extensions.Configuration.Binder.Tests.csproj index a408e921ec89e6..198d6cbfae86b3 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Microsoft.Extensions.Configuration.Binder.Tests.csproj +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Microsoft.Extensions.Configuration.Binder.Tests.csproj @@ -10,13 +10,14 @@ Link="Common\ConfigurationProviderExtensions.cs" /> - + + From 843ba20d950aecdaf81f270ec89a935000f5348d Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Wed, 11 Jan 2023 08:25:22 -0800 Subject: [PATCH 2/3] Address the feedback --- .../src/ConfigurationBinder.cs | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs index a1a6d8a73d0fc4..0d2fa560a4e037 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs @@ -262,7 +262,10 @@ private static void BindProperty(PropertyInfo property, object instance, IConfig config.GetSection(GetPropertyName(property)), options); - if (propertyBindingPoint.HasNewValue) + // For property binding, there are some cases when HasNewValue is not set in BindingPoint while a non-null Value inside that object can be retrieved from the property getter. + // As example, when binding a property which not having a configuration entry matching this property and the getter can initialize the Value. + // It is important to call the property setter as the setters can have a logic adjusting the Value. + if (!propertyBindingPoint.IsReadOnly && propertyBindingPoint.Value is not null) { property.SetValue(instance, propertyBindingPoint.Value); } @@ -382,14 +385,6 @@ private static void BindInstance( } } } - - if (!bindingPoint.HasNewValue && !bindingPoint.IsReadOnly && bindingPoint.Value is not null) - { - // For property binding, there are some cases when HasNewValue is not set in BindingPoint while a non-null Value inside that object can be retrieved - // from the property getter. As example, when binding a property which not having a configuration entry matching this property and the getter can initialize the Value. - // It is important to set the HasNewValue to true at this time to force the property setter to run. The reason is the setters can have a logic adjusting the Value. - bindingPoint.SetValue(bindingPoint.Value); - } } [RequiresDynamicCode(DynamicCodeWarningMessage)] From ec72255ed76399e358098bd7e0efe74611083108 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Thu, 12 Jan 2023 11:03:22 -0800 Subject: [PATCH 3/3] Package authoring --- .../src/Microsoft.Extensions.Configuration.Binder.csproj | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/Microsoft.Extensions.Configuration.Binder.csproj b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/Microsoft.Extensions.Configuration.Binder.csproj index 76207aa9c34981..c8834b6b4b3d5b 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/Microsoft.Extensions.Configuration.Binder.csproj +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/Microsoft.Extensions.Configuration.Binder.csproj @@ -5,8 +5,8 @@ true true true - 2 - false + 3 + true Functionality to bind an object to data in configuration providers for Microsoft.Extensions.Configuration.