Implement manual configuration binding for proxy config #521
Conversation
davidfowl
requested review from
alnikola,
halter73 and
Tratcher
as code owners
| { | ||
| // This method is protected so we use reflection to trigger it. The alternative is to wrap or implement | ||
| // a custom configuration provider and expose OnReload as a public method | ||
| var reload = typeof(ConfigurationProvider).GetMethod("OnReload", BindingFlags.NonPublic | BindingFlags.Instance); |
There was a problem hiding this comment.
This is slightly hacky but it let me skip writing a custom configuration provider that replicates the in memory provider 😄. I can if this feels too hacky though...
There was a problem hiding this comment.
Since it's in tests, I think it's fine as long as it saves redundant code lines.
- Removed the intermediate *Data types. This makes it clear that there are really 2 models, the input configuration and the runtime model the proxy uses. - Removed use of IOptionMonitor<T> and bind directly to configuration due to inability to handle parsing or validation errors. Fixes #481
davidfowl
force-pushed
the
davidfowl/manual-config-binding
branch
from
ca10922
to
7f99a25
Compare
|
Updated |
| internal static int ReadInt32(this IConfiguration configuration, string name) | ||
| { | ||
| return configuration[name] is string value ? int.Parse(value, NumberStyles.None, CultureInfo.InvariantCulture) : default; | ||
| } | ||
|
|
||
| internal static double ReadDouble(this IConfiguration configuration, string name) | ||
| { | ||
| return configuration[name] is string value ? double.Parse(value, CultureInfo.InvariantCulture) : default; | ||
| } | ||
|
|
||
| internal static TimeSpan ReadTimeSpan(this IConfiguration configuration, string name) | ||
| { | ||
| return configuration[name] is string value ? TimeSpan.Parse(value, CultureInfo.InvariantCulture) : default; | ||
| } | ||
|
|
||
| internal static TEnum ReadEnum<TEnum>(this IConfiguration configuration, string name) where TEnum : struct | ||
| { | ||
| return configuration[name] is string value ? Enum.Parse<TEnum>(value, ignoreCase: true) : default; | ||
| } | ||
|
|
||
| internal static bool ReadBool(this IConfiguration configuration, string name) | ||
| { | ||
| return configuration[name] is string value ? bool.Parse(value) : default; | ||
| } |
There was a problem hiding this comment.
Many of the cases where you're parsing primitives should be returning nullables rater than returning the default (0, false, etc.). In fact, I think we should clarify our policy so that all options properties should be nullable and not set by default. That lets the implementation decide what default to apply if none is specified, especially since many of the implementations will replaceable.
E.g. this should be nullable and not set. Let the factory choose the default.
There was a problem hiding this comment.
I can do this in a different PR.
- Be more explicit about defaults - Move defaults to configuration reading
Fixes #481
PS: This change lead me to file this issue dotnet/runtime#44381