dotnet / runtime

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

Developers can safely trim apps which need Configuration Binder #44493

Closed eerhardt closed 1 year ago

eerhardt commented 3 years ago

Edited by @layomia.

Background

Application configuration in ASP.NET Core is performed using one or more configuration providers. Configuration providers read data (as key-value pairs) using a variety of sources such as settings files (e.g. appsettings.json), environment variables, Azure Key Vault etc.

At the core of this mechanism is ConfigurationBinder, an extension class in platform extensions that provides Bind and Get methods that maps configuration values (IConfiguration instances) to strongly-typed objects. Bind takes an instance, while Get creates one on behalf of the caller. The current approach currently uses reflection which causes issues for trimming and Native AOT.

Here's example of code that users write to invoke the binder:

using Microsoft.AspNetCore.Builder;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;

// ASP.NET Core apps configure and launch a host, responsible for app startup and lifetime management.
// Templates create a WebApplicationBuilder which contains the host and provides default configuration
// for the app that may be specified by users through the mechanisms shown above.
// `args` contains configuration values specified through command line
WebApplicationBuilder builder = WebApplication.CreateBuilder(args);

// Represents a section of application configuration values (key-value pairs).
// Implements `IConfiguration`.
IConfigurationSection section = builder.Configuration.GetSection("MyOptions");

// !! Configure call - to be replaced with source-gen'd implementation
// `builder.Services`: a collection of user or framework-provided services for the application to compose
// `Configure<MyOptions>(section)`: adds the configuration section to the service collection for use in their operations
// `MyOptions`: configuration shape/type that maps to command-line specified config values
builder.Services.Configure<MyOptions>(section);

// !! Get call - to be replaced with source-gen'd implementation
// Get a new instance of your options using values from the config section
MyOptions options0 = section.Get<MyOptions>();

// Bind config options to an existing instance
MyOptions options1 = new MyOptions();
section.Bind(myOptions1);

// Build the application.
WebApplication app = builder.Build();
// Specify a http GET operation
app.MapGet("/", () => "Hello World!");
// Run the app
app.Run();

// Configuration classes mapping to user-provided values
public class MyOptions
{
    public int A { get; set; }
    public string S { get; set; }
    public byte[] Data { get; set; }
    public Dictionary<string, string> Values { get; set; }
    public List<MyClass> Values2 { get; set; }
}

public class MyClass
{
    public int SomethingElse { get; set; }
}

Configuration binding source generator in .NET 8

In .NET 8, as a replacement to reflection, we wish to ship a source generator that generates static code to bind config values to target user types. We are proposing an implicit mechanism where we probe for Configure, Bind, and Get calls that we can retrieve type info from.

Given the sample user code above, here's the code that would be generated:

using System.Collections.Generic;
using Microsoft.AspNetCore.Builder;
using Microsoft.Extensison.DependencyInjection;

public static class GeneratedConfigurationBinder
{
    public static IServiceCollection Configure<T>(this IServiceCollection services, IConfiguration configuration)
    {
        if (typeof(T) == typeof(MyOptions))
        {
            // Redirect to generated Bind method.
            return services.Configure<MyOptions>(obj => Bind(configuration, obj));
        }

        // Repeat if-check for all input types.

        throw new NotSupportedException("The source generator did not detect this type as input.");
    }

    public static T? Get<T>(this global::Microsoft.Extensions.Configuration.IConfiguration configuration)
    {
        if (typeof(T) == typeof(MyOptions))
        {
            MyOptions obj = new();
            Bind(configuration, obj);
            return (T)(object)obj;
        }

        // Repeat if-check for all input types.

        throw new NotSupportedException("The source generator did not detect this type as input.");
    }

    internal static void Bind(this IConfiguration configuration, MyOptions obj)
    {
        if (obj is null)
        {
            throw new ArgumentNullException(nameof(obj));
        }

        // Preliminary code to validate input configuration.

        obj.A = configuration["A"] is String stringValue0 ? int.Parse(stringValue4) : default;
        obj.S = configuration["S"] is String stringValue1 ? bool.Parse(stringValue5) : default;
        obj.Values ??= new();
        Bind(configuration.GetSection("Values"), obj.Values);
        obj.Values2 ?? = new();
        Bind(configuration.GetSection("Values2"), obj.Values2);
    }

    private static void Bind(this IConfiguration configuration, Dictionary<string, string> obj)
    {
        if (obj is null)
        {
            throw new global::System.ArgumentNullException(nameof(obj));
        }

        string key;
        string element;

        foreach (Microsoft.Extensions.Configuration.IConfigurationSection section in configuration.GetChildren())
        {
            key = section["Key"] is string stringValue2 ? stringVal2 : default;
            element = section["Value"] is string stringValue3 ? stringValue3 : default;
            obj[key] = element;
        }
    }

    private static void Bind(this IConfiguration configuration, List<MyClass> obj)
    {
        throw new NotSupportedException("This type is not supported.");
    }
}

Generator kick-off gesture

We want the user experience to be easy and minimal. Starting off, the generator will simply probe for a compiler visible property called UseConfigurationBinderSourceGenerator to determine whether to run. This allows folks that don't want the generator or for which it doesn't work for (yet) to skip using it.

<ItemGroup>
    <CompilerVisibleProperty Include="UseConfigurationBinderSourceGenerator" />
</ItemGroup>

We'll add this as an SDK property later.

Generation strategy:

The operation is to convert IConfiguration instances into target config types. We'll do this by generate bespoke parsing/mapping logic for each target type. This is similar to a "fast-path" deserialization feature that the System.Text.Json source generator could implement (https://github.com/dotnet/runtime/issues/55043).

PR: https://github.com/dotnet/runtime/pull/82179

Integration with user apps

The code is generated into the global namespace and the methods are picked over the reflection by the compiler.

Technical details

What's next

https://github.com/dotnet/runtime/issues/79527


Related docs

Initial design

ghost commented 3 years ago

Tagging subscribers to this area: @maryamariyan See info in area-owners.md if you want to be subscribed.


Issue meta data

Issue content: Source Generators are a new technology that was added to Roslyn in the .NET 5 timeframe, but we have not yet utilized them by creating generators to assist with runtime and framework features. We will work to define customer scenarios in more detail here with customer development. Code that uses runtime reflection and reflection.emit causes issues for Trimming and Native AOT. Source generators can be used to generate alternative code at build time that is static so can be used in conjunction with Trimming and AOT. Work on trimming ASP.NET apps has revealed where there are current limitations that can be solved by source generators. This item tracks creating a Source Generator for [ConfigurationBinder](https://github.com/dotnet/runtime/blob/a79df14b3cc62ade39382a6e08d3b25871d8ebb6/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs#L12-L15), which binds strongly-typed objects to configuration values. cc @maryamariyan @davidfowl @ericstj @samsp-msft
Issue author: eerhardt
Assignees: -
Milestone: -

eerhardt commented 3 years ago

@danroth27 @davidfowl - I don't see ConfigurationBinder being used on the WASM side of a Blazor application. Would a source generator only be useful for a server side ASP.NET app? Or is it useful for Blazor WASM as well?

I see this work item as being of the same importance as #44432, so I marked it as Priority 2 as well. Any thoughts there?

danroth27 commented 3 years ago

@eerhardt Users could decide to use ConfigurationBinder themselves, but as far as I know it isn't commonly used. So I agree that this is really more about ASP.NET Core than Blazor.

maryamariyan commented 3 years ago

Linking this issue to https://github.com/dotnet/runtime/issues/43930 and https://github.com/dotnet/runtime/issues/36130

7sharp9 commented 3 years ago

How would this be consumed by non C# languages?

allantargino commented 2 years ago

I was investigating this issue and end up finding a limitation (by design) of source generators on a particular case.

Currently, the following piece of code that sets init properties of constructed instances works well, giving the usage of reflection by the ConfigurationBinder:

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

var configuration = new ConfigurationBuilder()
    .AddInMemoryCollection(new Dictionary<string, string>
    {
        { "Name", "Allan" }
    })
    .Build();

var p = new Person();
configuration.Bind(p);

Console.WriteLine(p.Name);

public class Person
{
    public string Name { get; init; }
}

But if we generate code that tries to set an init property of an existing constructed instance (without reflection), by design, the compiler won't allow it.

Any ideas of how to support init properties with source generators? Should we generate code with reflection on it as a fallback case?

From a Trimming and Native AOT perspective, that I believe may be the end goal here, I am not sure how this mix of reflection/non-reflection setters would be useful. Thoughts?

Just trying to think on the use case without breaking the current API :) Thank you.

allantargino commented 2 years ago

Another interesting case of the API that we need to consider is the BinderOptions.BindNonPublicProperties option. Should we also generate code with reflection to set non-public properties?

eerhardt commented 2 years ago

My thinking in both of these cases is we would generate code that doesn't compile, and we let the developer fix the code. Or we add diagnostic warnings, like we do in the LoggerMessage source generator saying "couldn't bind to Person.Name because it doesn't have a public setter".

Falling back to reflection defeats the purpose of having a source generator.

maryamariyan commented 1 year ago

@tarekgh @davidfowl we can use this issue to track the epic for adding source generator for configuration binder.

davidfowl commented 1 year ago

Here's the prototype of a source generator https://github.com/davidfowl/ConfigSourceGenerator

layomia commented 1 year ago

I updated the description with a spec and action items. I discussed these with @eerhardt and I'm working through the items.

eerhardt commented 1 year ago

Approach 2: Use JsonSerializer to deserialize into target type

I don't think this approach is feasible for the following reasons:

  1. Performance.
    • Going through an intermediate "JSON" layer would not be fast.
    • It would bloat the code, since we would also need the JSON source generated code.
    • There can and will be more values in the IConfiguration than what needs to be bound to the object. It would be wasteful to serialize these values to JSON if they aren't being bound to the object.
  2. The ConfigurationBinder can bind an IConfiguration to an existing object. This takes an existing object, with some values already defaulted on the object, and then overwrites those values with data coming from the IConfiguration. JsonSerializer (AFAIK) can only create new objects from the JSON payload. It can't "merge" an existing object with a JSON payload.
layomia commented 1 year ago

I don't think this approach is feasible for the following reasons

I'll update the notes to indicate that this was just an early thought experiment.

Going through an intermediate "JSON" layer would not be fast.

Were it not for other issues cited, I'd at least want to get perf numbers for this approach compared against the current reflection-based implementation, and also the approach we've agreed to take (approach 1).

It would bloat the code, since we would also need the JSON source generated code

We could add a new knob for "deserialization-only" metadata generation to reduce the cost. This might be enough to make the code footprint comparable to generating deserialization logic for each target config type and other bootstrapping.

There can and will be more values in the IConfiguration than what needs to be bound to the object. It would be wasteful to serialize these values to JSON if they aren't being bound to the object.

I haven't determined this for sure, but if there's a reliable pattern to identify these values, a custom converter or other configuration might be able to efficiently filter them out.

JsonSerializer (AFAIK) can only create new objects from the JSON payload. It can't "merge" an existing object with a JSON payload.

A feature to merge/populate existing objects is currently being designed (@krwq; as part of https://github.com/dotnet/runtime/issues/78556).

davidfowl commented 1 year ago

I also think we should consider moving away from TypeConverters as part of this change.

eerhardt commented 1 year ago

moving away from TypeConverters

Can you elaborate why? It seems like a reasonable way of converting a string to an unknown type.

davidfowl commented 1 year ago
  1. Performance reasons - TypeConverter is old technology, and it shows. There's no way to avoid boxing value types
  2. It's full of trimmer unsafe APIs
  3. We have IParsable\<T> now

My source generator currently uses IParsable\<T> as a replacement for type converters. Maybe it would be a reasonable compromise to fall back to type converters based on an attribute-based check (since there's no registration system for TypeConverters outside of the type itself) so we can avoid using the APIs at runtime unless we knew the type had a type converter.

davidfowl commented 1 year ago

I just looked to convince myself again, the runtime overhead of type descriptor and I'm 98% sure we should avoid it unless we can't find a TryParse and the type doesn't have the TypeConverter attribute. So much runtime overhead to convert simple types to strings 😄.

layomia commented 1 year ago

I also think we should consider moving away from TypeConverters as part of this change

Can't yet comment on whether we should do this but tracking with https://github.com/dotnet/runtime/issues/79527.

layomia commented 1 year ago

Hello folks, I'm OOF starting tomorrow so work here will resume in January. The generator is close to MVP but still needs some work to generate good code for all the existing reflection based tests, which will be to throw Not(Yet)SupportedException in a few cases (a reasonable baseline/starting point IMO). If anyone is interested in what the implementation looks like atm, check out my ⁠branch.

The next step would be to document what isn't supported yet (e.g. deserializing into parameterized ctors), why, and the path to support. I'll add them as task items in https://github.com/dotnet/runtime/issues/79527. There'll likely be a few behavioral differences vs the reflection implementation, largely for the same reasons as the System.Text.Json generator. This work sequence follows the "Action Items" I laid out in the description above.

pinkfloydx33 commented 1 year ago

While I like the idea of leveraging IParseable<>, you don't always have control over the source type and/or it doesn't make sense outside of supporting configuration values.

For example, we have a custom CsvToHashSetTypeConverter<> that we register for configuration binding purposes only. This converter takes a delimited string of the forms "some;values;here" and "1,2,3" and converts them into a HashSet<string>/HashSet<int> respectively. Some of our configuration comes from external systems where we don't have control over the format. For the rest, a delimited string is often simpler to work with over a json-like array syntax, especially when the current behavior of overlayed collection/array values is unwanted (or surprising to a non-technical user).

In any case, adding IParseable<> to HashSet<> (or any collection types) generally doesn't make any sense. Even if it did, we don't own the type so it wouldn't be a viable option. We'd still need a way to either register our type converter or have it replaced with something more akin to a strongly typed JSON custom converter.

eerhardt commented 1 year ago

@pinkfloydx33 - Can you show/describe how you are registering your TypeConverter and how that TypeConverter's CanConvertFrom/ConvertFrom is implemented? A big issue with TypeDescriptor.GetConverter is that there can be only 1 in the app for a given type. So if some other part of the app wanted to convert strings to HashSets, they couldn't co-exist.

I think you could still enable your scenario, but instead using a custom collection Type, for example derive from HashSet<T>, or wrap a HashSet<T>. Then make that collection Type IParseable.

Another option is that the source generator could still respect TypeConverterAttribute applied to the Type (or even optionally applied to the property, which ConfigurationBinder doesn't do today).

One other reason to not use TypeDescriptor.GetConverter is what I discovered yesterday, using TypeDescriptor has a considerable impact on application size when publishing for NativeAOT. See https://github.com/dotnet/runtime/issues/81931.

davidfowl commented 1 year ago

Supporting type converters can be opt-in but we should have an extensibility model decoupled from them. We don't want to incur the cost when it's not needed.

pinkfloydx33 commented 1 year ago

To register the type converters we add the following at application startup for any app we know requires it:

TypeDescriptor.AddAttributes(typeof(HashSet<string>), new TypeConverterAttribute(typeof(OurConverter)));

We had considered module initializers but scrapped that idea after reading some issues regarding the feature and decided to be explicit. I'm on mobile only for the next few days so grabbing the implementations is a bit difficult but I can report back in a few days if you still want to see it.

We also register converters (where applicable) for HashSet<int> as well as for the Aspnet IPNetwork and the BCL IPAddress classes.

The latter two are to support IP Address and IP Network/subnet (ie. 10.0.0.0/16) in configuration for things like forwarded headers and IP and subnet whitelisting on our YARP gateway.

If IParsable were supported it would still not cover IPAddress (though I believe there's an issue in this repo to add the interface) nor IPNetwork (which I know there's plans to add to the BCL but I can't recall if parse/format are included). Structural/Pattern-matched TryParse similar to aspnet would be interesting and could also cover the former.

That said, we only use the type converters for this explicit purpose. IParsable support could get us the rest of the way if it made it to the IP classes. I like your wrapper collection idea for the HashSet where IParseable doesn't make sense, but would much prefer the interface made it to the networking classes.

In any case, an opt-in configuration-converter system, divorced from TypeConverters, with sourcegen would be really interesting.

eerhardt commented 1 year ago

It sounds like any solution here needs to support the scenario where the "app dev" doesn't own the Type being parsed (i.e. can't add an interface implementation to the Type). Or at least have a workable solution for that scenario.

A thing I dislike about the existing TypeConverter solution is that it can't be statically discerned at compile time. Source generators work best when they know what behavior they are coding for, and can make the appropriate optimizations. Maybe if we recognized a TypeConverterAttribute on the property, as well as support IParseable, that might be enough to support any scenario.

bartonjs commented 1 year ago

Marking as needs-work because a lot of this is contingent on a non-released compiler feature; but that doesn't block experimenting with it as-adjusted in previews.

chsienki commented 1 year ago

Generator kick-off gesture

Want to call this out as probably not the right approach. It's pretty difficult to 'disable' a generator based on a property it reads in, and you end up with a lot of complicated logic (we do this in Razor for other reasons and it's not pleasant). From a performance POV you're also still doing a (small) amount of work even when it doesn't run.

Instead, you should adjust the logic in ResolveTargetingPackAssets (or add another target that runs after it) to add/remove the generator being passed as part of the Analyzers collection altogether, based on the property provided by the user. This keeps the generator simpler, and ensure the least amount of work is done when it's not in use.

layomia commented 1 year ago

Thanks @chsienki I'll look into this approach. FYI @ericerhardt

Want to call this out as probably not the right approach

Does the source generator cookbook need to be updated then?

we do this in Razor for other reasons and it's not pleasant

Can you pls point me to where this is done? (or cc @captainsafia if you know...)

chsienki commented 1 year ago

@layomia It's totally valid to use CompilerVisibleProperty as described in the cookbook, I'm just saying for this scenario it's not very useful. It's better to just not pass the generator in, instead of trying to supress it in code.

The razor generator does suppression via CompilerVisibleProperty but it can't switch to the other mechanism due to some hot reload weirdness that we're fixing. When thats done, we'll use the 'don't pass it' strategy too.

@captainsafia has been working on a different generator which uses the 'don't pass it' strategy that you can probably re-use almost wholesale.

captainsafia commented 1 year ago

@captainsafia has been working on a different generator which uses the 'don't pass it' strategy that you can probably re-use almost wholesale.

You can find a sample of the strategy used for the Minimal APIs generator at this PR: https://github.com/dotnet/sdk/pull/29982

ericstj commented 1 year ago

I think one of the challenges faced with this one is which targets should get the removal: https://github.com/dotnet/sdk/blob/45a14239002eb0a6f8ea6096d756d882a0cb8208/src/WebSdk/Web/Sdk/Sdk.props#L71-L73. This generator is not part of an SDK. It's associated with a library that's in the ASP.NET shared framework and also part of a NuGet package (to be used outside of ASP.NET). Do you recommend we put them in the .NET SDK and the Microsoft.Extensions.Configuration.Binder nuget package?

andrewlock commented 1 year ago

One potential issue I can foresee with the design of the generated code (because I've run into a similar issue with my own generators) is around internal classes and InternalsVisibleTo.

The fact that the generated code is always a fixed type (GeneratedConfigurationBinder) in the global namespace, means that something like this will no longer compile:

Project 1:

[assembly:InternalsVisible2(Project2)]
var config = new ConfigurationBuilder().Build();
var settings = new SomeOptions();
section.Bind(settings);

Project 2: (has a <ProjectReference> to Project1)

var config = new ConfigurationBuilder().Build();
var settings = new MoreOptions();
section.Bind(settings); // 👈 error

Gives

[CS0121] The call is ambiguous between the following methods or properties: 'GeneratedConfigurationBinder.Configure<T>(Microsoft.Extensions.DependencyInjection.IServiceCollection, Microsoft.Extensions.Configuration.IConfiguration)' and 'GeneratedConfigurationBinder.Configure<T>(Microsoft.Extensions.DependencyInjection.IServiceCollection, Microsoft.Extensions.Configuration.IConfiguration)'

You might consider it an edge case, but it was one of the first issues I ran into when building my first source generator.

Note: I've just seen that preview 4 bits are going onto NuGet, so apologies if this is no longer relevant! :D (Edit: looks like the preview 3 package is broken for me)

layomia commented 1 year ago

Thanks @andrewlock - I've opened https://github.com/dotnet/runtime/issues/86363 to address this.

davidfowl commented 1 year ago

This was the only edge case I found and it does feel like an edge case. That said, we can figure out a cleaner way to handle it.

layomia commented 1 year ago

Closing this issue given the feature is largely implemented and is getting integrated up-stack. Remaining work is tracked in https://github.com/dotnet/runtime/issues/79527 and this query.