From 459409b26ba32b5257979c23e2e77bc8e769a879 Mon Sep 17 00:00:00 2001 From: Guy Ludvig Date: Sun, 12 Jul 2026 10:08:07 +0300 Subject: [PATCH] Fix five correctness bugs in the binding engine + add regression tests Phase 1 of the review fix plan: self-contained, non-breaking correctness fixes, each verified against the suite (88 tests green on net8.0 + net10.0). - Register EnumTypeConverter so enum properties bind from strings instead of throwing InvalidCastException. The converter existed but was never added to the converter chain, so enums fell through to Convert.ChangeType. - Parse scalar values with InvariantCulture in DefaultTypeConverter. Values arrive as strings, so double/decimal previously parsed under the ambient culture (e.g. "1.5" bound to 15 on a de-DE host). - BindingContext.PropertyType now returns the property's type instead of its declaring interface (it was set to propertyInfo.DeclaringType). Also drop two dead, always-false null checks in the ctor. - SettingsOptionsValidator no longer throws when AttributeType is null while interface/suffix indication is set; the Attribute check is now guarded. - Fix two broken error-message templates in Resources (a missing $ that emitted a literal {typeName}, and a stray $ before {type.FullName}). Tests: add Conversion/DefaultTypeConverterTests (culture-invariant parsing) and Conversion/EnumConversionTests (enum-from-string + attribute default). Both were written to fail against the pre-fix code, then confirmed green after the fixes. --- .../BindingContext.cs | 4 +- .../Conversion/DefaultTypeConverter.cs | 3 +- .../Conversion/TypeConvertersCollections.cs | 1 + .../Core/SettingsOptionsValidator.cs | 5 +- .../ExistForAll.SimpleSettings/Resources.cs | 4 +- .../Conversion/DefaultTypeConverterTests.cs | 75 +++++++++++++++++++ .../Conversion/EnumConversionTests.cs | 49 ++++++++++++ 7 files changed, 133 insertions(+), 8 deletions(-) create mode 100644 src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/DefaultTypeConverterTests.cs create mode 100644 src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/EnumConversionTests.cs diff --git a/src/Core/ExistForAll.SimpleSettings/BindingContext.cs b/src/Core/ExistForAll.SimpleSettings/BindingContext.cs index 8a6c4c8..1b960e0 100644 --- a/src/Core/ExistForAll.SimpleSettings/BindingContext.cs +++ b/src/Core/ExistForAll.SimpleSettings/BindingContext.cs @@ -28,14 +28,12 @@ public BindingContext(string section, { if (section == null) throw new ArgumentNullException(nameof(section)); if (key == null) throw new ArgumentNullException(nameof(key)); - if (string.Equals(section, null, StringComparison.Ordinal)) throw new ArgumentNullException(nameof(section)); - if (string.Equals(key, null, StringComparison.Ordinal)) throw new ArgumentNullException(nameof(key)); Section = section; Key = key; SettingsType = settingsType ?? throw new ArgumentNullException(nameof(settingsType)); PropertyInfo = propertyInfo ?? throw new ArgumentNullException(nameof(propertyInfo)); - PropertyType = propertyInfo.DeclaringType!; + PropertyType = propertyInfo.PropertyType; CurrentValue = currentValue; } diff --git a/src/Core/ExistForAll.SimpleSettings/Conversion/DefaultTypeConverter.cs b/src/Core/ExistForAll.SimpleSettings/Conversion/DefaultTypeConverter.cs index 5bbed8d..6beab48 100644 --- a/src/Core/ExistForAll.SimpleSettings/Conversion/DefaultTypeConverter.cs +++ b/src/Core/ExistForAll.SimpleSettings/Conversion/DefaultTypeConverter.cs @@ -1,4 +1,5 @@ using System; +using System.Globalization; namespace ExistForAll.SimpleSettings.Conversion { @@ -11,7 +12,7 @@ public bool CanConvert(Type settingsType) public object Convert(object value, Type settingsType) { - return System.Convert.ChangeType(value, settingsType); + return System.Convert.ChangeType(value, settingsType, CultureInfo.InvariantCulture); } } } \ No newline at end of file diff --git a/src/Core/ExistForAll.SimpleSettings/Conversion/TypeConvertersCollections.cs b/src/Core/ExistForAll.SimpleSettings/Conversion/TypeConvertersCollections.cs index a97cab2..67986a1 100644 --- a/src/Core/ExistForAll.SimpleSettings/Conversion/TypeConvertersCollections.cs +++ b/src/Core/ExistForAll.SimpleSettings/Conversion/TypeConvertersCollections.cs @@ -10,6 +10,7 @@ public TypeConvertersCollections(SettingsOptions settingsOptions) AddLast(new UriTypeConvertor()); AddLast(new ArrayTypeConverter(settingsOptions, this)); AddLast(new EnumerableTypeConverter(settingsOptions, this)); + AddLast(new EnumTypeConverter()); AddLast(new DefaultTypeConverter()); } } diff --git a/src/Core/ExistForAll.SimpleSettings/Core/SettingsOptionsValidator.cs b/src/Core/ExistForAll.SimpleSettings/Core/SettingsOptionsValidator.cs index 1c1a913..8da767c 100644 --- a/src/Core/ExistForAll.SimpleSettings/Core/SettingsOptionsValidator.cs +++ b/src/Core/ExistForAll.SimpleSettings/Core/SettingsOptionsValidator.cs @@ -14,8 +14,9 @@ public void ValidateOptions(SettingsOptions settingsOptions) throw new SettingsOptionsArgumentNullException(); } - if (!typeof(Attribute).GetTypeInfo().IsAssignableFrom(settingsOptions.AttributeType)) - throw new SettingsOptionNonAttributeException(settingsOptions.AttributeType!); + if (settingsOptions.AttributeType != null && + !typeof(Attribute).GetTypeInfo().IsAssignableFrom(settingsOptions.AttributeType)) + throw new SettingsOptionNonAttributeException(settingsOptions.AttributeType); if (string.IsNullOrWhiteSpace(settingsOptions.ArraySplitDelimiter)) diff --git a/src/Core/ExistForAll.SimpleSettings/Resources.cs b/src/Core/ExistForAll.SimpleSettings/Resources.cs index 57434c0..ca51ac6 100644 --- a/src/Core/ExistForAll.SimpleSettings/Resources.cs +++ b/src/Core/ExistForAll.SimpleSettings/Resources.cs @@ -43,10 +43,10 @@ public static string PropertyNotAllowNullMessage(string propertyName) => $@"[{propertyName}] is marked as Null not allowed, yet the value is null. please provide value via binder or attribute"; public static string TypeIsNotInterface(string typeName) => - @"[{typeName}] is not an interface, SimpleSettings supports only interfaces"; + $@"[{typeName}] is not an interface, SimpleSettings supports only interfaces"; public static string SettingsOptionAttributeTypeMessage(Type type) => - $"SimpleSettings support Attribute indication of interfaces, the type provided [${type.FullName}] is not an attribute."; + $"SimpleSettings support Attribute indication of interfaces, the type provided [{type.FullName}] is not an attribute."; } } \ No newline at end of file diff --git a/src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/DefaultTypeConverterTests.cs b/src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/DefaultTypeConverterTests.cs new file mode 100644 index 0000000..65f2c37 --- /dev/null +++ b/src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/DefaultTypeConverterTests.cs @@ -0,0 +1,75 @@ +using System.Globalization; +using ExistForAll.SimpleSettings.Binder; + +namespace ExistForAll.SimpleSettings.UnitTests.Conversion +{ + public class DefaultTypeConverterTests + { + // The default SectionNameFormatter strips the leading "I": INumericSettings -> "NumericSettings". + private const string Section = "NumericSettings"; + + [Test] + [NotInParallel] + public async Task Build_DoubleFromString_UnderGermanCulture_ParsesInvariant() + { + var original = CultureInfo.CurrentCulture; + try + { + // In de-DE '.' is the group separator, so a culture-sensitive parse of "1.5" yields 15. + CultureInfo.CurrentCulture = new CultureInfo("de-DE"); + + var settings = BuildWith(nameof(INumericSettings.Value), "1.5") + .GetSettings(); + + await Assert.That(settings.Value).IsEqualTo(1.5d); + } + finally + { + CultureInfo.CurrentCulture = original; + } + } + + [Test] + [NotInParallel] + public async Task Build_DecimalFromString_UnderGermanCulture_ParsesInvariant() + { + var original = CultureInfo.CurrentCulture; + try + { + CultureInfo.CurrentCulture = new CultureInfo("de-DE"); + + var settings = BuildWith(nameof(INumericSettings.Amount), "1234.56") + .GetSettings(); + + await Assert.That(settings.Amount).IsEqualTo(1234.56m); + } + finally + { + CultureInfo.CurrentCulture = original; + } + } + + [Test] + public async Task Build_IntFromString_BindsValue() + { + var settings = BuildWith(nameof(INumericSettings.Count), "42") + .GetSettings(); + + await Assert.That(settings.Count).IsEqualTo(42); + } + + private static SettingsBuilder BuildWith(string key, string value) + { + var collection = new InMemoryCollection(); + collection.Add(Section, key, value); + return SettingsBuilder.CreateBuilder(x => x.AddSectionBinder(new InMemoryBinder(collection))); + } + + public interface INumericSettings + { + double Value { get; set; } + decimal Amount { get; set; } + int Count { get; set; } + } + } +} diff --git a/src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/EnumConversionTests.cs b/src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/EnumConversionTests.cs new file mode 100644 index 0000000..b41de55 --- /dev/null +++ b/src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/EnumConversionTests.cs @@ -0,0 +1,49 @@ +using System; +using ExistForAll.SimpleSettings.Binder; + +namespace ExistForAll.SimpleSettings.UnitTests.Conversion +{ + public class EnumConversionTests + { + // The default SectionNameFormatter strips the leading "I": IEnumSettings -> "EnumSettings". + private const string Section = "EnumSettings"; + + [Test] + public async Task Build_EnumPropertyFromStringValue_BindsEnum() + { + var settings = BuildWith(nameof(IEnumSettings.Day), "Monday") + .GetSettings(); + + await Assert.That(settings.Day).IsEqualTo(DayOfWeek.Monday); + } + + [Test] + public async Task Build_EnumPropertyFromDefaultValue_BindsEnum() + { + // No binder: the value comes from the attribute default and is already the target type, + // so this path works even without the EnumTypeConverter registered — it guards the fix. + var settings = SettingsBuilder.CreateBuilder() + .GetSettings(); + + await Assert.That(settings.Day).IsEqualTo(DayOfWeek.Friday); + } + + private static SettingsBuilder BuildWith(string key, string value) + { + var collection = new InMemoryCollection(); + collection.Add(Section, key, value); + return SettingsBuilder.CreateBuilder(x => x.AddSectionBinder(new InMemoryBinder(collection))); + } + + public interface IEnumSettings + { + DayOfWeek Day { get; set; } + } + + public interface IEnumWithDefault + { + [SettingsProperty(DefaultValue = DayOfWeek.Friday)] + DayOfWeek Day { get; set; } + } + } +}