dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.47k stars 4.76k forks source link

Configuration binding does not throw when it should #110147

Open 0xced opened 6 days ago

0xced commented 6 days ago

Description

Binding a configuration using Microsoft.Extensions.Configuration.Binder 9.0.0 with ErrorOnUnknownConfiguration set to true should throw an InvalidOperationException as documented in .NET 8 breaking changes - ConfigurationBinder throws for mismatched value.

Previously, BinderOptions.ErrorOnUnknownConfiguration was used solely to raise an exception if a value existed in the configuration but not in the model being bound to. Now, if this property is set to true, an exception is also thrown if the value in the configuration can't be converted to the type of value in the model.

Unfortunately, it doesn't throw that exception and binding succeeds when it should fail.

Reproduction Steps

BindConfiguration.csproj

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net9.0</TargetFramework>
    <Nullable>enable</Nullable>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.Extensions.Configuration" Version="9.0.0" />
    <PackageReference Include="Microsoft.Extensions.Configuration.Binder" Version="9.0.0" />
  </ItemGroup>

</Project>

Program.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
using Microsoft.Extensions.Configuration;

var hasSourceGenerator = typeof(Program).Assembly.GetTypes().Any(e => e.FullName?.StartsWith("Microsoft.Extensions.Configuration.Binder.SourceGeneration") == true);
Console.WriteLine($"Using configuration binding source generator: {hasSourceGenerator}");
WriteAssemblyInfo(typeof(ConfigurationBuilder).Assembly);
WriteAssemblyInfo(typeof(ConfigurationBinder).Assembly);

var configuration = new ConfigurationBuilder().AddInMemoryCollection(new Dictionary<string, string?> { ["Values:Monday"] = "not-an-array-of-string" }).Build();
try
{
    var testSettings = configuration.Get<TestSettings>(options => options.ErrorOnUnknownConfiguration = true);
    Console.WriteLine($"❌ Should have thrown InvalidOperationException but bound {testSettings?.Values.Count} values");
    foreach (var (key, value) in testSettings?.Values ?? [])
    {
        Console.WriteLine($"{key}: {string.Join(", ", value)}");
    }
    return 1;
}
catch (InvalidOperationException exception)
{
    Console.Error.WriteLine($"✅ Expected exception: {exception.Message}");
    return 0;
}

static void WriteAssemblyInfo(Assembly assembly)
{
    Console.WriteLine($"{assembly.GetName().Name} {assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()?.InformationalVersion}");
}

internal class TestSettings
{
    public Dictionary<DayOfWeek, string[]> Values { get; init; } = [];
}

Expected behavior

Running dotnet run should print

✅ Expected exception: 'ErrorOnUnknownConfiguration' was set and binding has failed. The likely cause is an invalid configuration value.

Actual behavior

Running dotnet run prints

❌ Should have thrown InvalidOperationException but bound 0 values

Regression?

This is a regression from Microsoft.Extensions.Configuration.Binder version 8.0.2 where it properly throws an InvalidOperationException.

Note that the regression was introduced between version 9.0.0-preview.1.24080.9 (1d1bf92fcf43aa6981804dc53c5174445069c9e4) where it still throws as expected and version 9.0.0-preview.2.24128.5 (8e5e748c5c06d3e40244c725bd2124f06998f6c1) where it doesn't throw anymore.

Known Workarounds

Since it's a regression the workaround is to stay on Microsoft.Extensions.Configuration.Binder version 8.0.2. ☹️

Configuration

dotnet --info
.NET SDK:
 Version:           9.0.100
 Commit:            59db016f11
 Workload version:  9.0.100-manifests.3068a692
 MSBuild version:   17.12.7+5b8665660

Runtime Environment:
 OS Name:     Mac OS X
 OS Version:  13.6
 OS Platform: Darwin
 RID:         osx-arm64
 Base Path:   /usr/local/share/dotnet/sdk/9.0.100/

.NET workloads installed:
There are no installed workloads to display.
Configured to use loose manifests when installing new manifests.

Host:
  Version:      9.0.0
  Architecture: arm64
  Commit:       9d5a6a9aa4

.NET SDKs installed:
  6.0.428 [/usr/local/share/dotnet/sdk]
  7.0.410 [/usr/local/share/dotnet/sdk]
  8.0.404 [/usr/local/share/dotnet/sdk]
  9.0.100 [/usr/local/share/dotnet/sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.App 6.0.36 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 7.0.20 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 8.0.11 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 9.0.0 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 6.0.36 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 7.0.20 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 8.0.11 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 9.0.0 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]

Other architectures found:
  x64   [/usr/local/share/dotnet/x64]
    registered at [/etc/dotnet/install_location_x64]

Environment variables:
  Not set

global.json file:
  Not found

Learn more:
  https://aka.ms/dotnet/info

Download .NET:
  https://aka.ms/dotnet/download

Other information

Note that setting <EnableConfigurationBindingGenerator>true</EnableConfigurationBindingGenerator> in the project to enable the binder source generator never works as expected (i.e. never throws), even with version 8.0.2. So a separate issue should probably be opened about it.

Also it's "funny" that this issue is basically the inverse of #98231 where it throws even though ErrorOnUnknownConfiguration is set to false.

Finally, I still stand by my point that repurposing the ErrorOnUnknownConfiguration property to control whether an exception must be thrown when a conversion fails was not a good idea and that it's never too late to introduce a new property to the options to control the exception behaviour separately from unknown/missing configuration keys.

dotnet-policy-service[bot] commented 6 days ago

Tagging subscribers to this area: @dotnet/area-extensions-configuration See info in area-owners.md if you want to be subscribed.

tarekgh commented 5 days ago

The change caused this regression is https://github.com/dotnet/runtime/pull/97777.