Extensibility for Configuration Binding Source Generator

[layomia]

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

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

[ghost]

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

Issue Details

---

Initial discussion - #44493 (comment)

cc @eerhardt @ericstj @davidfowl

Author:	layomia
Assignees:	-
Labels:	area-Extensions-Configuration, source-generator
Milestone:	8.0.0

[layomia]

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

[layomia]

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.

[eerhardt]

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.

[ericstj]

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.

[layomia]

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

[eerhardt]

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?

[eerhardt]

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.

[davidfowl]

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

[pinkfloydx33]

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?

[layomia]

@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.

[ericstj]

Probably you don't need to have the IParseable interface to permit types to define their own bind logic. We could probe IParseable first, then for the method signature that would have been there if it had implemented IParseable (to support frameworks that don't have IParseable).

I think the scenario of folks defining bind logic for types they own (maybe IParseable), and folks defining bind logic for types they don't own are legitimate use cases and things the reflection-based binder supported. We need to do it in a different way than TypeConverter since that has a large code size and uses a significant amount of refection.

[davidfowl]

Probably you don't need to have the IParseable interface to permit types to define their own bind logic. We could probe IParseable first, then for the method signature that would have been there if it had implemented IParseable (to support frameworks that don't have IParseable).

This is what minimal APIs does.

I think the scenario of folks defining bind logic for types they own (maybe IParseable), and folks defining bind logic for types they don't own are legitimate use cases and things the reflection-based binder supported. We need to do it in a different way than TypeConverter since that has a large code size and uses a significant amount of refection

Agreed. There are also language features that will help here (implicit extensions).

[layomia]

Triage: given we're in the later stages of the .NET 8 dev time, we've decided to pause on implementing an extensibility feature for the source generator in .NET 8. Effectively, the generator won't add code to check for TypeConverter or IParsable<T> implementations for the type. Built-in primitives are already handled with handwritten logic which inlines the core parsing logic that these mechanisms provide.

We don't have a strong signal that these features are needed right now, but we'll be following feedback here closely and can revisit the decision.

[ericstj]

@pinkfloydx33 I noticed your down-vote. Would you like to share your scenario with us to see if we're missing something? Do you have an application that uses custom bind logic today?

[pinkfloydx33]

I believe I shared it above and in the linked issue.

Particularly we have a bunch of settings that we pass in as a delimited list rather than an actual array of values. We then use a type converter to convert this to a HashSet of a specific type. We've gone this route in some cases as we've found it easier to manage layered settings if we are completely overwriting them, versus trying to layer array indexed positions which have non-obvious semantics.

Say we have a Setting array of [ 1, 2, 3 ] in our appsettings.json that we bind to a HashSet<int>. We then overlay that with Setting:0=2 and Setting:1=4 in Azure AppConfig, thus becoming [ 2, 4, 3 ]. Our Configuration Managers (non developers) don't realize there was a third position in the base layer and now we've got a rogue value that they'd have to be aware of in the first place and can't easily null out.

Array semantics in the overlays can be confusing to a non-dev. They also must make sense of the array index syntax in general. So instead, we require them to specify the entire desired value as a comma-delimited string: Setting=2,4. We then use a type converter registered at application startup to enable binding this string to a HashSet<int>. This allows our Config Managers to see/set the full value in a single entry without being aware of the actual implementation and allows us to treat it as a collection in the application itself.

Without support for type converters (or IParseable along with our own implementing HashSet subclass) the source gen'd binder is worthless to us and we'll have to stick with the reflection-based binder.

[ericstj]

Could you do the same thing with a poco that had a string value and an accessor that lazily initialized the HashSet from the string?

using Microsoft.Extensions.Configuration;

var config = new ConfigurationBuilder()
    .AddInMemoryCollection(new Dictionary<string,string?>
    {
        ["MyOptions:Value"] = "2,3"

    }).Build();

var converter = config.GetSection("MyOptions").Get<MyHashSetConverter>()!;

Console.WriteLine(converter.Converted.Count);

public class MyHashSetConverter
{
    private HashSet<int>? _converted;

    public string? Value { get; set; }

    public HashSet<int> Converted { get => _converted ??= Parse(Value!); }
    private HashSet<int> Parse(string value)
    {
        var parts = value.Split(',');
        return new HashSet<int>(parts.Select(x => int.Parse(x)));
    }
}

[davidfowl]

@ericstj Are we out of time to implement IParesable<T>?

[pinkfloydx33]

@ericstj I hadn't considered doing that; it should/could work. It's feels a little unnecessary but is definitely a valid workaround that would work with source gen. Thanks!

However, I believe that another scenario I have won't work. We bind to IPAddress and IPNetwork (from aspnetcore though to be replaced with inbox class with net8) in a couple places. The first is some customizations over forwarded headers, but the second and most important is in our YARP gateway where we use addresses/CIDR blocks for whitelisting access with the help of Firewall. Both of these classes (will) support IParsable whereas right now we are using a custom type converter to support both of them.

I suppose we can use a similar trick and cache the parsed address/network object. I'm not crazy about that--less so than the case cited above. This would be covered with IParsable support and it feels like a big gap that it's not included. I'm sure I can't have the only two use-cases. I realize any feature starts at -100 points and needs design/dev/testing/doc/etc, but given that there's prior art around IParsable--particularly in aspnet--it feels like it's something that could've been adapted in a manner that offset some of that cost.

Just my 2cp. We've got the work around demonstrated above so I'm not bound to reflection... though I may just wait until it's supported before trying to switch.

[ericstj]

@ericstj Are we out of time to implement IParesable<T>?

We discussed with @tarekgh, @eerhardt, and @layomia last week. The cut-off for feature work for 8.0 is effectively today. We don't have a strong signal that folks are blocked on this extensibility, and we didn't want to rush something. We can always add this later, but it's much more difficult to remove it if we got it wrong. IParseable is less risky than a larger extensibility model with public API - @layomia already has a prototype - but adding support is still not small. We wouldn't want to add it to just the source generator but also the runtime binder. If IParseable is must have for v1 I think it can still be prioritized - but we'll have less time to do other things important for the quality of the product.

[christopherbahr]

I'm the author of #89254 which proposed a mechanism to control binding of types not owned by the application (In my case TimeSpan).

The API described in the opening of this PR would solve my case perfectly.

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; }
    }
}

Is there any chance to get just that into .NET 8? That ignores the source generator AOT problem but is still an improvement in customization for configuration binding. It seems relatively simple to implement, we just need to thread the options object down through the binder to where it's currently finding the TypeConverter and check the Converters dictionary.

That doesn't feel like it adds a lot of risk that we've gotten something wrong. A generic override seems compatible with a later, smarter way to handle IParseable objects or any other sort of smarter automatic way to do the right thing. Any such system would want to account for the consumer preferring a different parsing method for classes they don't own for whatever reason.

[layomia]

@christopherbahr right now we're focused on getting the quality of the release right. Better not to rush this.

Often times the devil is in the details with features like this -- e.g., we need to get the order of precedence right; would folks need other inputs such as CultureInfo?; what's the nature of any behavior difference between source-gen & reflection impl? We wouldn't want to rush such considerations through API review.

[christopherbahr]

@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.

[ericstj]

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

[ericstj]

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.

[leidegre]

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