dotnet / runtime

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

Explore options immutability #43359

Open davidfowl opened 3 years ago

davidfowl commented 3 years ago

Today the IOptions<TOptions> pattern is built around the mutability of the TOptions until first use (though nothing prevents later mutation). We should investigate what it would mean to consume immutable TOptions in the options system. Today, the system constructs TOptions using Activator.CreateInstance then executes a series of delegates on top of that instead to produce the "final" instance (see https://github.com/dotnet/runtime/blob/546115d17880b6dc97bf3f0a0c846c760e53b11e/src/libraries/Microsoft.Extensions.Options/src/OptionsFactory.cs#L46)

An alternative model would be to do the same thing but execute delegates that return a new instance of the options object instead of mutating the current instance. We would need to figure out how to support both side by side but it would allow designing immutable options objects.

Here's an example of what we what it would look like:

public record MyImmutableOption(string Name, int Size);

public void ConfigureServices(IServiceCollection services)
{
    services.Configure<MyImmutableOption>(o =>
    {
        return o with { Name = "Foo" };
    });
}

cc @ericstj @eerhardt @maryamariyan

ghost commented 3 years ago

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

skyoxZ commented 3 years ago

I find a workaround for #46996:

public record MyImmutableOption
{
    public string Name { get; init; } = "[NoName]";
    public int Size { get; init; }
}
TorreyGarland commented 3 years ago

Records with all "init" property setters partially works. I am running into an issue with deserializing immutable collection types. IOptions is not picking up any data from the app.settings files.

Nihlus commented 3 years ago

I have a suggested API surface + implementation - is there an appropriate place to share this? PR, here in this issue...?

davidfowl commented 3 years ago

At this point it would be for .NET 7. It's a bit late to take such a big change for .NET 6. Would love to see the proposed design in this issue though!

Nihlus commented 3 years ago

Sure!

So, essentialy, I've tried to go for the most straightforward approach to supporting immutable option types (and indirectly, records), while maintaining a familiar and unsurprising API. The short explanation is that a set of extension methods are added alongside the existing Configure family of methods, which instead of accepting an Action<TOptions>, accept a Func<TOptions, TOptions>.

Cumulative configurations of an immutable options type is done by simply returning a new copy of the options type, instead of mutating it in-place as is done today.

The full source code for this proposal can be viewed here, and since I have a need for this right away, I intend to release it on nuget as an experimental package as soon as I've got some unit tests set up :)

The proposed surface would do the following (with nullable reference types enabled).

New Methods

The following methods are defined as extension methods to IServiceCollection.

IServiceCollection Configure<TOptions>(this IServiceCollection services, Func<TOptions> creator);
IServiceCollection Configure<TOptions>(this IServiceCollection services, string? name, Func<TOptions> creator);
IServiceCollection Configure<TOptions>(this IServiceCollection services, Func<TOptions,TOptions> configureOptions);
IServiceCollection Configure<TOptions>(this IServiceCollection services, string? name, Func<TOptions,TOptions> configureOptions);
IServiceCollection ConfigureAll<TOptions>(this IServiceCollection services, Func<TOptions,TOptions> configureOptions);
IServiceCollection PostConfigure<TOptions>(this IServiceCollection services, Func<TOptions,TOptions> configureOptions);
IServiceCollection PostConfigure<TOptions>(this IServiceCollection services, string name, Func<TOptions,TOptions> configureOptions);
IServiceCollection PostConfigureAll<TOptions>(this IServiceCollection services, Action<TOptions> configureOptions);

They behave effectively identical to their existing counterparts, save for operating on immutable types.

New Types

The following new types are added.

Interfaces

The following new interfaces are added, generally matching the existing interfaces.

public interface ICreateOptions<out TOptions> where TOptions : class
{
    string? Name { get; }
    TOptions Create();
}

public interface IReadOnlyConfigureNamedOptions<TOptions> : IReadOnlyConfigureOptions<TOptions> where TOptions : class
{
    TOptions Configure(string name, TOptions options);
}

public interface IReadOnlyConfigureOptions<TOptions> where TOptions : class
{
    TOptions Configure(TOptions options);
}

public interface IReadOnlyPostConfigureOptions<TOptions> where TOptions : class
{
    TOptions PostConfigure(string name, TOptions options);
}

Records

The following new records are added, generally matching the existing classes but utilizing new language features. It would of course be trivial to implement these as classes instead, but records are used here for brevity's sake. The full implementations are omitted from this section; see the linked experimental implementation for the complete source code.

public record CreateOptions<TOptions>(string? Name, Func<TOptions> Creator) : ICreateOptions<TOptions> where TOptions : class;

public record ReadOnlyConfigureNamedOptions<TOptions>(string? Name, Func<TOptions, TOptions> Function) : IReadOnlyConfigureNamedOptions<TOptions> where TOptions : class;

public record ReadOnlyPostConfigureOptions<TOptions>(string? Name, Func<TOptions, TOptions> Function) : IReadOnlyPostConfigureOptions<TOptions> where TOptions : class;

Classes

The following new classes are added.

public class ReadOnlyOptionsFactory<TOptions> : IOptionsFactory<TOptions> where TOptions : class

Usage

Usage of the new API differs in two major ways.

The second of the two points is solved by using Func<TOptions, TOptions> as described above. The first, however, is slightly more interesting. As you've seen, I've added an ICreateOptions interface with a corresponding implementation, which is used to create the initial state for an options instance. This is utilized in ReadOnlyOptionsFactory in one of three ways.

Explicit initialization

Given the following options type,

public record MyOptions(string Value, bool Flag);

it could be utilized in the following way:

var services = new ServiceCollection()
    .Configure<MyOptions>(() => new MyOptions("Initial", false))
    .Configure<MyOptions>(opts => opts with { Value = "Something else" })
    .BuildServiceProvider();

Using a Configure call with a parameterless lambda lets the user specify the initial state explicitly, and it may then be configured normally.

Parameterless constructor

Given the following options type,

public record MyOptions()
{
    public string Value { get; init; }
    public bool Flag { get; init; }
}

it could be utilized in the following way:

var services = new ServiceCollection()
    .Configure<MyOptions>(opts => opts with { Value = "Something else" })
    .BuildServiceProvider();

In this case, the parameterless constructor is used to create the initial state, and the user does not need to specify anything. This is the most similar to the current API.

All-optional constructor

Given the following options type,

public record MyOptions(string Value = "Initial", bool Flag = true);

it could be utilized in the following way:

var services = new ServiceCollection()
    .Configure<MyOptions>(opts => opts with { Value = "Something else" })
    .BuildServiceProvider();

In this case, a constructor with all parameters defined as optional is detected and used to create the initial state. This is a best-of-both-worlds approach, which allows both terse usage and an appropriate initial state.

SteveDunn commented 2 years ago

Records with all "init" property setters partially works. I am running into an issue with deserializing immutable collection types. IOptions is not picking up any data from the app.settings files.

@TorreyGarland - this sounds like https://github.com/dotnet/runtime/issues/61547 which was fixed in https://github.com/dotnet/runtime/pull/52514, which was part of .NET 6

Actually, scrap that - it's not part of .NET 6 as the PR was closed so that further thought could be given to the problem.

A new PR was created and merged to main in March, so it'll be part of .NET 7: https://github.com/dotnet/runtime/pull/66131

eerhardt commented 2 years ago

We will consider this in a future release. Moving this issue out of the 7.0 milestone.

ohadschn commented 1 year ago

Records with all "init" property setters partially works. I am running into an issue with deserializing immutable collection types. IOptions is not picking up any data from the app.settings files.

@TorreyGarland - this sounds like #61547 which was fixed in #52514, which was part of .NET 6

Actually, scrap that - it's not part of .NET 6 as the PR was closed so that further thought could be given to the problem.

A new PR was created and merged to main in March, so it'll be part of .NET 7: #66131

@SteveDunn By "immutable collection types" are we talking about System.Collections.Immutable? Are you saying for example that the following should be possible in .NET 7 (but not 6)?

// app.settings
{
    "MyStrings": ["a", "b", "c"]
}
public class MyOptions
{
    public ImmutableArray<string> MyStrings { get; set; }
   //or ImmutableHashSet<string>, or ImmutableDictionary<string,string>, etc.
}

Because I just attempted this and neither .NET 6 nor .NET 7 worked - the immutable array above was not bound (its IsDefault was true), while a sibling regular array I added for control was bound without issue.

I tried binding to the concrete ReadOnlyCollection<T> class but that failed with Cannot create instance ... missing a public parameterless constructor. However Binding to IReadOnlyCollection<T>/IReadOnlyDictionary<T>/IReadOnlyList<T> did work: https://github.com/dotnet/runtime/blob/0f3a88b479ddab4899954ba1c2cbdbceb5c23385/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs#L1272

Nevertheless, without any guarantee on the generated underlying type, it's not as good as Immutable - especially considering thread safety (which a readonly interface does not guarantee). Plus these is no ReadOnlySet<T> AFAIK (https://github.com/dotnet/runtime/issues/29387).

Opened an issue suggesting immutable collection binding: : https://github.com/dotnet/runtime/issues/78592

mu88 commented 1 year ago

Will this be shipped with .NET 8 or is there any workaround how immutable options can be used? Because at the moment with .NET 7, the following code doesn't compile:

internal class WebApplicationFactoryForAny : WebApplicationFactory<Program>
{
    protected override void ConfigureWebHost(IWebHostBuilder builder) =>
        builder.ConfigureTestServices(services => services.Configure<ScreenshotOptions>(options => options with { Url = "" }));
}

public record ScreenshotOptions
{
    public const string SectionName = nameof(ScreenshotOptions);

    public string Url { get; init; } = string.Empty;

    public UrlType UrlType { get; init; }

    public string Username { get; init; } = string.Empty;

    public string Password { get; init; } = string.Empty;

    public string ScreenshotFileName { get; init; } = "Screenshot.png";

    public uint Width { get; init; }

    public uint Height { get; init; }

    public uint TimeBetweenHttpCallsInSeconds { get; init; }

    public uint RefreshIntervalInSeconds { get; init; }

    public bool BackgroundProcessingEnabled { get; init; }

    public Activity? Activity { get; init; }

    public string CalculateSleepBetweenUpdates() =>
        Activity.DisplayShouldBeActive()
            ? RefreshIntervalInSeconds.ToString()
            : Activity.RefreshIntervalWhenInactiveInSeconds.ToString();
}

public enum UrlType
{
    Any,
    OpenHab
}

public record Activity(TimeOnly ActiveFrom, TimeOnly ActiveTo, uint RefreshIntervalWhenInactiveInSeconds);
wertzui commented 1 year ago

It doesn't compile, or are you getting an Exception at startup?

What is the error, you are seeing?

mu88 commented 1 year ago

It doesn't compile, I'm getting a [CS0201] Only assignment, call, increment, decrement, await, and new object expressions can be used as a statement

Nihlus commented 1 year ago

That is expected, as the discussed functionality is not in .NET as of yet. The API surface I suggested above is available as a nuget package if you want to start using it right away - it's linked in the comment.

mu88 commented 1 year ago

And will the support for immutable options be part of the upcoming .NET 8 release?

davidfowl commented 1 year ago

@mu88 No, it's in the "Future" milestone, which means it was deprioritized.

mykhailok01 commented 10 months ago

Is there a chance it will be added in .NET 9? :)