ConfigurationValidation - wrapper for IConfiguration nullable checks #526
Conversation
| /// <param name="sectionName">Name of configuration section to get the object from</param> | ||
| /// <returns>Non-nullable object of type T</returns> | ||
| /// <exception cref="ArgumentNullException">If the type T is not found in the given section of the configuration</exception> | ||
| public static T GetNonNullableOrThrow<T>(IConfiguration configuration, string sectionName) |
There was a problem hiding this comment.
It looks like GetRequiredSection doing the same thing.
And if we need for Get<T>() to throw we can probably set BinderOptions.ErrorOnUnknownConfiguration globally, that would provide a better exception with details about what properties missing.
There was a problem hiding this comment.
The problem with nullables was not on GetSection (returns always not-null) but on Get<T>.
Would the BinderOptions.ErrorOnUnknownConfiguration get us rid of the nullable warnings? That's why this PR - to have the validation in one place and not repeat the code.
There was a problem hiding this comment.
I don't know if Get<T> provides any options of ensuring compiler that return type is not nullable. If you won't be able to find one and decide to go ahead with adding custom wrapper I would suggest following:
- Use method name that aligns with other methods of the API, ex 'GetRequired`.
- In the implementation unitize methods that I've mentioned above to get proper explanation if what failed to bind instead of generic exception we are creating now.
- First of all create an API suggestion for ,NET explaining your use case and making sure that they don't have better suggestions with .NET 7 and possibly convincing them to add 'GetRequired` into next version.
| public class ConfigurationValidationTests | ||
| { | ||
| [TestMethod] | ||
| public void GetNonNullableOrThrow_WhenValueNotNull() |
There was a problem hiding this comment.
In most cases our tests are named Method_Scenario_ExpectedResult.
| Mock<IConfigurationSection> mockSectionString = new Mock<IConfigurationSection>(); | ||
| Mock<IConfiguration> mockConfig = new Mock<IConfiguration>(); | ||
|
|
||
| mockSectionInt.Setup(x => x.Value).Returns(configValueInt); |
There was a problem hiding this comment.
Are you sure your mocks behave the same way as the real configuration? It's better to just build a configuration object: https://stackoverflow.com/questions/64794219/how-to-mock-iconfiguration-getvalue.
| public static T GetNonNullableOrThrow<T>(IConfiguration configuration, string sectionName) | ||
| { | ||
| return configuration.GetSection(sectionName).Get<T>() | ||
| ?? throw new InvalidOperationException($"{nameof(T)} missing in the {sectionName} section of the configuration"); |
There was a problem hiding this comment.
What is the result of nameof(T), does it help in troubleshooting?
| string sectionKeyInt = "SectionKeyInt"; | ||
| string sectionKeyString = "SectionKeyString"; | ||
| string configValueInt = "5"; | ||
| string configValueString = "asdf"; |
There was a problem hiding this comment.
Are you accounting for a case where bound value is an object, not a primitive type?
After changes of nullability of some types in Microsoft.Extensions 7.0.0