Open MattKotsenas opened 2 years ago
Tagging subscribers to this area: @dotnet/area-extensions-configuration See info in area-owners.md if you want to be subscribed.
Author: | MattKotsenas |
---|---|
Assignees: | - |
Labels: | `api-suggestion`, `untriaged`, `area-Extensions-Configuration` |
Milestone: | - |
What should the function return or do if key is found but the value cannot be parsed/bound to the introduced type?
What should the function return or do if key is found but the value cannot be parsed/bound to the introduced type?
Today GetValue<T>
throws an InvalidOperationException
with a message of the format Failed to convert configuration value at '{key}' to type '{type}'.
So I would suggest doing the same. I'll update the proposal to make that explicit.
After looking at how I would implement this code a bit more, it seems that the Abstractions package doesn't expose GetValue
, it's exposed from Binder, so I've updated the proposal to have the new method on ConfigurationBinder
.
Also here's a sample implementation on my fork: https://github.com/dotnet/runtime/compare/main...MattKotsenas:feature/get-required-value-69163?expand=0
Hi there! I believe the proposal is ready for feedback, and the sample implementation has been updated with tests. Any suggestions on how to proceed is greatly appreciated. Thanks!
Would it make sense to go ahead and open a PR, so the .NET team can review it that way?
(We'd also be interested in a feature like this 🙂)
In .NET 7 nullable reference types have been enabled in ConfigurationBinder
so GetValue
method is now marked that it can return null. After migration to .NET 7 I get a warning almost everywhere I use GetValue<T>
because of the possible null. The solution to this is to add !
null forgiving operator everywhere I use GetValue
but it's not an elegant solution.
This change should've been shipped with .NET 7 :(
@maryamariyan , @terrajobst , this proposal seems to have stalled out; is there anything we can do to get an approval / rejection and move forward?
If you have any questions, please let me know. Thanks!
@maryamariyan didn't we just add validation to configuration? Could this be achieved more generally with that?
@MattKotsenas how blocking this scenario for you? This is not a high priority for us which we have marked for the future, and we can consider it later.
@MattKotsenas I remember you had a fork open with a sample implementation. Would it make sense to open a PR for the .NET team to review?
We cannot accept a PR before design review any proposal. The issue is not about the implementation. I need to figure out the priority for that request to allocate time for that to investigate and analyze the proposal.
Ah, got it 🙂
What is the currently the recommended usage pattern for required configuration? e.g. for HttpClient base addresses.
{
"CoolService": {
"BaseAddress": "https://cool-stuff/"
}
}
What is the pattern for turning this into a not-null System.Uri
, or throw when the config is missing (or invalid format)?
// a
Uri a = new Uri(Configuration.GetRequiredSection("CoolService:BaseAddress").Value ?? throw new Exception());
// b
Uri b = Configuration.GetRequiredSection("CoolService").GetValue<Uri>("BaseAddress") ?? throw new Exception();
// c
Uri c = new Uri(Configuration.GetRequiredSection("CoolService")["BaseAddress"] ?? throw new Exception());
// d
Uri d = new Uri(Configuration.GetSection("CoolService")["BaseAddress"] ?? throw new Exception());
// e
Uri e = Configuration.GetSection("CoolService").GetValue<Uri>("BaseAddress") ?? throw new Exception();
// f
Uri f = new Uri(Configuration.GetSection("CoolService:BaseAddress").Value ?? throw new Exception());
// g
Uri g = new Uri(Configuration["CoolService:BaseAddress"] ?? throw new Exception());
I think the proposed design looks good, it would simplify my code.
Uri future = Configuration.GetRequiredValue<Uri>("CoolService:BaseAddress");
I think this is a good suggestion that aligns well with other APIs such as IServiceProvider.GetService
vs IServiceProvider.GetRequiredService
.
It is particularly annoying how it plays out with NRT and the added runtime protection against typos and misconfiguration is nice.
The only aspect I find annoying with this is the indexer access that IConfiguration
exposes, which cannot have an equivalent "required" version:
public void Method(IConfiguration configuration)
{
var option = configuration["MySection:MyOption"]; // returns `null` if option is not present
...
}
Would it be a best practice then, moving forward, to never use the indexer access for "normal" mandatory options and prefer GetRequiredValue<string>
instead?
Background
ConfigurationBinder
has a methodGetValue<T>
, which is a simple way of getting a single value from configuration and converting it to the specified type (docs). The API optionally accepts a default value if the configuration key cannot be found. If the key is not found and no default is specified,default(T)
is returned. If the value is found but cannot be converted to the target type, an exception is thrown.Motivation
In some cases, silently returning
default(T)
can lead to subtle bugs. For example, consider an example app that requires a timeout value, which should always be specified inappsettings.json
, and is retrieved like thisvar timeout = configuration.GetValue<TimeSpan>("Timeout")
. In our example the config value is missing. Since no default is specified, the defaultTimeSpan
is returned, which is...00:00:00
! As a result, all operations time out instantly rather that what we might expect, which is some type of exception that the configuration key wasn't found..NET 6 introduced a GetRequiredSection extension that performs similar validation for configuration sections, so I thought it may be appropriate to extend that convenience to the single value case as well.
Note that the proposed APIs would be on the
ConfigurationBinder
class inMicrosoft.Extensions.Configuration.Binder
and notConfigurationExtensions
inMicrosoft.Extensions.Configuration.Abstractions
sinceGetValue
is exposed through Binder and isn't part of the Abstractions layer.API Proposal
I propose that the exception if the configuration key isn't found is an
InvalidOperationException
, since that's whatGetRequiredSection
throws, but am open to other suggestions.API Usage
Configuration:
Comparison of usage (generic):
Comparison of usage (non-generic):
Alternative Designs
The non-generic case can be simulated with a one-liner like this:
which could then be made generic with another cast, but seems a bit unwieldy, especially if used in multiple places.
The generic version is more important in my opinion, since it's the case that can introduce confusion by coercing a missing value to the default (especially for value types). The non-generic version is proposed mostly for symmetry and reflection scenarios, and thus if it's decided that the non-generic version is not wanted, I wouldn't be opposed to dropping it.
Risks
Low as far as I can see. It's a new, opt-in API that follows an existing naming convention, and that increases type safety, so the likelihood of misuse or abuse seems low.