Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explore options immutability #43359

Open
Tracked by #47191 ...
davidfowl opened this issue Oct 13, 2020 · 16 comments
Open
Tracked by #47191 ...

Explore options immutability #43359

davidfowl opened this issue Oct 13, 2020 · 16 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-Extensions-Options feature-request
Milestone

Comments

@davidfowl
Copy link
Member

davidfowl commented Oct 13, 2020

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

)

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
Copy link

ghost commented Oct 13, 2020

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

@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Oct 14, 2020
@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Oct 22, 2020
@maryamariyan maryamariyan added this to Uncommitted in ML, Extensions, Globalization, etc, POD. via automation Oct 22, 2020
@maryamariyan maryamariyan added this to the 6.0.0 milestone Oct 22, 2020
@maryamariyan maryamariyan added User Story A single user-facing feature. Can be grouped under an epic. Team Epic labels Jan 19, 2021
@maryamariyan maryamariyan changed the title Explore options an immutability Explore options immutability Jan 19, 2021
@maryamariyan maryamariyan removed Team Epic User Story A single user-facing feature. Can be grouped under an epic. labels Jan 19, 2021
@skyoxZ
Copy link
Contributor

skyoxZ commented Mar 1, 2021

I find a workaround for #46996:

public record MyImmutableOption
{
    public string Name { get; init; } = "[NoName]";
    public int Size { get; init; }
}

@TorreyGarland
Copy link

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.

@eerhardt eerhardt modified the milestones: 6.0.0, 7.0.0 Jul 14, 2021
@ghost ghost moved this from 6.0.0 to Untriaged in ML, Extensions, Globalization, etc, POD. Jul 14, 2021
@maryamariyan maryamariyan moved this from Untriaged to 7.0.0 in ML, Extensions, Globalization, etc, POD. Aug 4, 2021
@Nihlus
Copy link

Nihlus commented Aug 8, 2021

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

@davidfowl
Copy link
Member Author

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
Copy link

Nihlus commented Aug 8, 2021

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.

  • Immutable option types must be given an initial state
  • Configuration steps do not mutate in-place, but instead return an entirely new instance which becomes the new option instance

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.

@maryamariyan maryamariyan added the api-needs-work API needs work before it is approved, it is NOT ready for implementation label Apr 7, 2022
@SteveDunn
Copy link
Contributor

SteveDunn commented Jun 28, 2022

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

@eerhardt
Copy link
Member

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

@ohadschn
Copy link

ohadschn commented Nov 19, 2022

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:

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 (#29387).

Opened an issue suggesting immutable collection binding: : #78592

@krwq krwq modified the milestones: 8.0.0, Future Jan 30, 2023
@mu88
Copy link

mu88 commented Jul 15, 2023

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
Copy link

wertzui commented Jul 15, 2023

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

What is the error, you are seeing?

@mu88
Copy link

mu88 commented Jul 15, 2023

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
Copy link

Nihlus commented Jul 15, 2023

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
Copy link

mu88 commented Jul 16, 2023

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

@davidfowl
Copy link
Member Author

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

@mykhailok01
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-Extensions-Options feature-request
Projects
None yet
Development

No branches or pull requests