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 immutable collections (System.Collections.Immutable) in options binding (ConfigurationBinder) #78592

Open
ohadschn opened this issue Nov 19, 2022 · 4 comments
Labels
area-Extensions-Configuration enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@ohadschn
Copy link

ohadschn commented Nov 19, 2022

Related issue: dotnet/dotnet-api-docs#8658 (Possible threading issues with non thread safe collections in Options classes)

Consider the following app settings and corresponding options class:

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

Following a ConfigurationBinder.Bind call, the immutable array above is not bound (its IsDefault is true).
Indeed, this does not seem to be supported, where the closest thing I could find was read-only collection binding:

However, even assuming the client does not cast away the immutability, it's not as good as Immutable without any guarantee on the generated underlying type. This is specifically problematic when considering thread safety (which a readonly interface does not guarantee). Plus these is no ReadOnlySet<T> AFAIK (#29387).

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 19, 2022
@ghost
Copy link

ghost commented Nov 19, 2022

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

Issue Details

Consider the following app settings and corresponding options class:

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

Following a ConfigurationBinder.Bind call, the immutable array above is not bound (its IsDefault is true).
Indeed, this does not seem to be supported, where the closest thing I could find was read-only collection binding:

However, even assuming the client does not cast away the immutability, it's not as good as Immutable without any guarantee on the generated underlying type. This is specifically problematic when considering thread safety (which a readonly interface does not guarantee). Plus these is no ReadOnlySet<T> AFAIK (#29387).

Author: ohadschn
Assignees: -
Labels:

area-System.Collections

Milestone: -

@ghost
Copy link

ghost commented Nov 22, 2022

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

Issue Details

Related issue: dotnet/dotnet-api-docs#8658 (Possible threading issues with non thread safe collections in Options classes)

Consider the following app settings and corresponding options class:

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

Following a ConfigurationBinder.Bind call, the immutable array above is not bound (its IsDefault is true).
Indeed, this does not seem to be supported, where the closest thing I could find was read-only collection binding:

However, even assuming the client does not cast away the immutability, it's not as good as Immutable without any guarantee on the generated underlying type. This is specifically problematic when considering thread safety (which a readonly interface does not guarantee). Plus these is no ReadOnlySet<T> AFAIK (#29387).

Author: ohadschn
Assignees: -
Labels:

area-System.Collections, untriaged, area-Extensions-Configuration

Milestone: -

@layomia
Copy link
Contributor

layomia commented Nov 22, 2022

Related to #44493.

This seems like an enhancement request. @dotnet/area-extensions-configuration @davidfowl @ericerhardt it looks like a plethora of collection types are supported for configuration binding. How do we determine what types to support? Do we want collection support in the .NET 8 version of the ConfigurationBinder source generator?

@layomia layomia removed the untriaged New issue has not been triaged by the area owner label Nov 22, 2022
@layomia layomia added this to the Future milestone Nov 22, 2022
@layomia layomia added the enhancement Product code improvement that does NOT require public API changes/additions label Nov 22, 2022
@ohads-MSFT
Copy link

How do we determine what types to support?

If I could offer my 2 cents, considering the nature of options classes in general and their ASP.NET usage specifically, I think the concurrent readonly scenario is extremely common.

Recall that options classes are cached which means virtually all DI consumers would get the same options class instance (for example, each controller instance created for a request). This basically makes it a concurrent scenario automatically (where readonly is virtually assumed).

While it is true that IReadOnlyXXX binding works and serves this purpose the gold standard would be immutable collections for sure (for example see https://stackoverflow.com/questions/30165810/why-use-immutablelist-over-readonlycollection).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Extensions-Configuration enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

No branches or pull requests

4 participants