Extensibility for Configuration Binding Source Generator

@eerhardt

Initial discussion - #44493 (comment)
cc @eerhardt @ericstj @davidfowl

Extensibility for Configuration Binding Source Generator

Background

The configuration binding source generator provides AOT and trim-friendly configuration in ASP.NET Core. It is an alternative to the pre-exising reflection-based implementation. For each type to bind, the reflection implementation checks whether a TypeConverter instance exists and uses it if so. This was mainly applicable as a convenient abstraction to bind to built-in primitives such as int and string which are parsable from string. However, the TypeConverter mechanism is not friendly for AOT usage. As a result, the binding generator does not look up TypeConverter usage.

TypeConverter inadvertently provided a way for developers to convert binding behavior. We've seen partner teams use it to parse non-primitive types such as security certificates. This begs the question of whether we do need to support it, or provide a replacement mechanism to cover customization scenarios.

Proposed customization strategies

We don't want to honor TypeConverter in the generator implementation. References to it would largely undo the major benefit of the generator, which is AOT and linking friendliness. Since the generator is new, we have an opportunity to provide a better customization experience.

Check and honor `IParsable<T>` implementations.

This only works for design-time customization. Does not work for non-owned types.

API proposal: new API for runtime configuration

We would add a new converters dictionary to register converter instances. This would be last one wins.

namespace Microsoft.Extensions.Configuration
{
    public class BinderOptions
    {
        public bool BindNonPublicProperties { get; set; }
        public bool ErrorOnUnknownConfiguration { get; set; }

        // New
        // Note: last one wins, just like with current `TypeConverter` look up behavior.
        // Note: boxing for custom structs. Should be okay since binding generally isn't in hot path.
        public IDictionary<Type, Func<string, object>> Converters { get; }
    }
}

Customization restrictions

Like the reflection implementation, and with IParsable<T>, we would only support types directly parsable from string.
We would throw an exception if we detect a converter for a type that is included in the list of "intrinsic" types that TypeConverter supports & the generator now handles with hand-written logic.

Order of customization preference

Honor runtime converter.
Use IParsable<T> implementation if detected (replacing generators handwritten logic for intrinsic types).
Fallback to built-in binding logic.

@eerhardt

From @eerhardt in #83533 (comment):

One option for the "opt in" model of TypeConverters is to respect the TypeConverterAttribute if either:

Directly applied to the property

Directly applied to the Type of the property

So, for example:
[TypeConverter(typeof(PointConverter))]
public struct Point
{
   public int X;
   public int Y;
}

public class CustomOptions
{
    public Point CurrentPoint { get; set }
}
or
public struct Point
{
   public int X;
   public int Y;
}

public class CustomOptions
{
    [TypeConverter(typeof(PointConverter))]
    public Point CurrentPoint { get; set }
}
See also:

Developers can safely trim apps which need Configuration Binder #44493 (comment)

Support TypeConverterAttribute on bound properties #36545

One option for the "opt in" model of TypeConverters is to respect the TypeConverterAttribute if either:

Directly applied to the property

Directly applied to the Type of the property

This makes sense to me. Just that it doesn't account for converters registered at runtime i.e. with TypeDescriptor. We could document that as an unsupported scenario.

The "registered at runtime" may be interesting to a subset of customers. But the way to enable that would mean all the other customers would have to pay for it. My preference would be to only enable it if we get feedback it is required. And if we do, we make it opt-in, somehow.

I don't feel great about making any bets on the TypeConverter infrastructure. It's pretty old and never designed with AOT and Link-ability in mind.

Directly applied to the property

This wasn't previously honored by ConfigurationBinder. It only looked up the converter for the type.

runtime/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

Line 855 in 6ef9d10

TypeConverter converter = TypeDescriptor.GetConverter(type);

Directly applied to the Type of the property

While this would provide a way for folks to extend the system and be consistent with what the binder used to do, it might still be too heavy. Converters handle more than just String->T and the linker might not be able to shake out a small enough code-path to make this viable.

An alternative would be to explicitly design some hook for folks to register a lightweight Func<string,T> conversion that we could honor at runtime. Heck - if they wanted to, they could even decide to add back in converter that's driven off the TypeConverter infrastructure.

Given all these considerations, this whole scenario seems like something we should await customer/stakeholder feedback for before implementing a solution.

@geeknoid

cc @geeknoid. I believe you have used this functionality in the past. Would not supporting customized converting of string => custom Type be a blocker for you using the ConfigurationBinder source generator?

This wasn't previously honored by ConfigurationBinder. It only looked up the converter for the type.

Correct. But this has been an ask from customers. See my "see also" above: #36545.

We're asking about existing types we don't own right? Why wouldn't we support IParseable<T>?

We only use type converters for this purpose, ie. configuration binding. As mentioned in the other issue, IParseable support would cover most of our use cases (assuming the interface made it to some inbox types as is currently planned)--but not all.

A mechanism to provide a Func<string, T>, particularly for types we don't own, would work for us. However those cases mostly revolve around HashSet<> (ie. value: "a,b,c" => HashSet<string>.Count == 3). If I recall correctly from reviewing the PRs, I believe the sourcegen is already special-casing target types of HashSet<>/ISet<>. Assuming some mechanism for customization existed, would that special casing take precedence?

We could always create a HashSet<> subclass that implemented IParseable. It'd be kind of weird, but not terrible if it's the only workaround. I'd have the same question though: would special-casing of ISet<>/HashSet<>--or any types for that matter--supercede checks for IParseable?

@pinkfloydx33

@pinkfloydx33 AFAIK the goal for the generator in .NET 8 is parity (to the degree possible) with the reflection implementation. Thus IParseable<T> based parsing won't be included now or it will be considered a stretch goal. If you have scenarios that work with reflection that the generator doesn't support, please file an issue.

On a related note IParseable<T> might seem like an expedient converter implementation for primitives (#83533) but it is not available in .NET Framework or Standard which the generator aims to support.

If feedback indicates that there are crucial dependences on TypeDescriptor being used at runtime, we could add API (either in source or say an MSBuild property) to determine whether the generator should include code that does the relevant look ups.

@layomia

@layomia Fair enough, I'm sure you understand the constraints and concerns much better than I do. It sounds like a couple weeks ago you were looking for some signal that people were blocked on this sort of extensibility. It sounds like we're not going to make .NET 8 but put me down as part of that signal for the next release.

@adamsitnik

@adamsitnik had a scenario described in #91324 where he binds to an abstract type and data helps discriminate which derived type to create.

@eerhardt

We are too late in 9.0 to be adding new extensibility features to Configuration. I do want us to reconsider this in 10.0. Would be interested to hear from others like @eerhardt @tarekgh @davidfowl what is most valuable to tackle here.

AOT support would be very much appreciated for something like this.

@eerhardt

Found this when looking at how to customize the deserialization logic. I have a configuration model with a DirectoryInfo property which ends up as null currently, despite being populated with a string value.

Unfortunately I'm going to need to set this to a string and then set up an alternative property to get the DirectoryInfo type. However, if we can implement IParsable and, ideally, observing JsonConverterAttribute as suggested by @eerhardt, that would be fantastic!

➕ Plus one from my side that being able to control the parsing logic would help tremendously. Right now, if we enable the binding source generator it will cause many breaking changes to existing customers due to how complex types are bound which prevents many of our packages from being AOT ready.

It would also be great to have the ability to see the property's key. A common use case is when you don’t want the app to crash due to an improper config value, and in your situation, it’s acceptable to simply log and skip the bad settings key.

Triage: we may consider offering more extensibility.

Navigation Menu

Repository navigation

Extensibility for Configuration Binding Source Generator #83599