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

Support for constructor binding #36389

Closed
divega opened this issue Feb 17, 2016 · 10 comments
Closed

Support for constructor binding #36389

divega opened this issue Feb 17, 2016 · 10 comments
Labels
area-Extensions-Configuration feature-request help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@divega
Copy link
Contributor

divega commented Feb 17, 2016

Edit: We're not going to do most of what is here, but allowing configuration to be bound to options that are immutable through exposing a constructor and read-only properties.

In some scenarios it might be good to have either an interface that doesn't expose mutability or an implementation of the configuration interfaces that throws when changes are attempted. The application could pass instances of these to components that it trusts enough to read configuration but not to modify it.

Note that this is a separate feature from persistence covered in dotnet/extensions#386. Currently configurations are not persistable but they can be modified in memory so they are not read-only either.

@nbarbettini
Copy link
Member

👍 It would be great to see more strict immutability, plus the option for persistence (depending on the outcome of dotnet/extensions#386).

@HaoK
Copy link
Member

HaoK commented Jul 19, 2016

Clearing milestone for visibility

@lukas-lansky
Copy link
Contributor

I think it should not be like "in some scenarios, someone might need to enforce readonlyness", but "the overwhelming majority of configuration access is readonly, so this should be enforced statically to prevent mistakes unless opted-out to support complicated RW scenarios".

I was kind of disappointed when I saw that your design is mutable-everything because otherwise it's such a great solution. It unfortunately sets bad example for object oriented design and it's difficult to evangelize developers to not promise stuff they don't mean to fulfill in their interfaces when the standard library requires them to write useless setters.

So, it would be great if you allow this:

    public interface ISomeKindOfNetworkConfig
    {
        int TimeoutInMs { get; }

        int Port { get; }
    }

    static void Main(string[] args)
    {
        var c = new ConfigurationBuilder()
            .AddInMemoryCollection(new List<KeyValuePair<string, string>> {
                new KeyValuePair<string, string>("Port", "8080"),
                new KeyValuePair<string, string>("Timeout", "60000")})
           .Build();

        var networkConfig = c.Get<ISomeKindOfNetworkConfig>();

        Console.WriteLine(networkConfig.Port);
    }

@MartinJohns
Copy link

@divega I was bothered about this too, so I wrote a little library that works for my use case.

NuGet: Microsoft.Extensions.Configuration.ImmutableBinder
GitHub: MartinJohns/ConfigurationContrib

It adds a new extension method ImmutableBind<T> which will create an instance and bind the configuration using a constructor. This allows for your option types to have only getter-only auto-properties. Also the library supports binding to IReadOnlyCollection<> and IReadOnlyDictionary<,> (actually it does not currently support other collection or dictionary values). It's first rough version.

Note: I'm not certain if I'm allowed to name my package this way. If there will be any feedback from Microsoft or NuGet I will rename the package.

@ajcvickers ajcvickers transferred this issue from aspnet/Configuration Dec 9, 2018
@ajcvickers ajcvickers changed the title Support for readonlyness Support for constructor binding Dec 9, 2018
@halforbit
Copy link

Had dotnet/extensions#593 closed as a dupe of this, so I will comment here. In dotnet/extensions#593 I asked for binding support to basic immutable POCOs via constructor parameters.

It sounds like what is being suggested/discussed here is instead to make a mutable configuration class and an immutable interface that it implements. This seems over-patterned, and not DRY.

I made an extension method that does what I am describing, and published it at:

Github: halforbit/extensions-configuration

Nuget: Halforbit.Extensions.Configuration

Internally it interprets the enumerated key/values from an IConfigurationSection into a JObject, then uses the JObject.ToObject<>() method to construct the result POCO. This technique has very stable and mature support for creating constructor-based immutable POCOs, as well as anything else that Json.Net supports, which is quite a lot.

I can use my extension method to do what I want to do today, but I know the prescribed approach tends to include Options<>. I am speculating here, but I expect the .Get() method here is probably what is underpinning Options<> functionality, so that would mean it doesn't support immutable POCO either, and at that point my custom extension method would not be used.

Edit: We're not going to do most of what is here

@divega I'm not sure what you mean by this. I hope it doesn't mean this feature won't be implemented.

I would be willing to put together a PR applying the above technique against the Microsoft.Extensions.Configuration source if we can agree it is worthwhile and can be admitted.

@divega
Copy link
Contributor Author

divega commented Jan 9, 2019

@halforbit it was @ajcvickers who added this paragraph at the top the initial comment:

Edit: We're not going to do most of what is here, but allowing configuration to be bound to options that are immutable through exposing a constructor and read-only properties.

So the issue now is being used to track binding to options with read-only properties and constructor parameters.

@frabe1579
Copy link

This is a practical example of this issue:
The issue #IdentityServer/IdentityServer4#2573 is related to this one.
In IdentityServer4 when the client configuration is stored in appsettings.json, the claims of the client cannot be read because the class System.Security.Claims.Claim is immutable and does not have a parameterless constructor.

@OnurGumus
Copy link

I think this is pretty important now considering C# 8 will complain about nulls with getters and setters.

@analogrelay analogrelay transferred this issue from dotnet/extensions May 13, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-Extensions-Configuration untriaged New issue has not been triaged by the area owner labels May 13, 2020
@analogrelay analogrelay added this to the Future milestone May 13, 2020
@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Jul 1, 2020
@maryamariyan maryamariyan modified the milestones: Future, 6.0.0 Oct 8, 2020
@maryamariyan
Copy link
Member

Next step is to prepare API proposal and examples using guidelines in https://github.com/dotnet/runtime/blob/master/docs/project/api-review-process.md

@maryamariyan maryamariyan added the help wanted [up-for-grabs] Good issue for external contributors label Oct 8, 2020
@maryamariyan maryamariyan modified the milestones: 6.0.0, Future Oct 26, 2020
@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Jun 11, 2022

ConfigurationBinder is already able to initialize an init-only property like public int Port { get; init; }.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Configuration feature-request help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests