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

ConfigurationValidation - wrapper for IConfiguration nullable checks #526

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions src/Abstractions/ConfigurationValidation.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT license.

using System;
using Microsoft.Extensions.Configuration;

namespace Microsoft.Omex.Extensions.Abstractions
{
/// <summary>
/// Class provides common validation methods for Microsoft.Extensions.IConfiguration
/// </summary>
public static class ConfigurationValidation
{
/// <summary>
/// Omex workaround around IConfigurationSection.Get() returning null variables.
/// </summary>
/// <typeparam name="T">Type needed from the configuration section</typeparam>
/// <param name="configuration">IConfiguration object</param>
/// <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)
Copy link
Contributor

@AndreyTretyak AndreyTretyak Nov 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
return configuration.GetSection(sectionName).Get<T>()
?? throw new InvalidOperationException($"{nameof(T)} missing in the {sectionName} section of the configuration");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the result of nameof(T), does it help in troubleshooting?

}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
<Content Include="BuildTransitive\*" Pack="true" PackagePath="buildTransitive" />
</ItemGroup>
<ItemGroup>
<PackageReference Include="Microsoft.Extensions.Configuration.Binder" />
<PackageReference Include="Microsoft.Extensions.Hosting.Abstractions" />
<PackageReference Include="Microsoft.Extensions.Logging.Abstractions" />
<PackageReference Include="System.Diagnostics.DiagnosticSource" />
Expand Down
76 changes: 76 additions & 0 deletions tests/Abstractions.UnitTests/ConfigurationValidationTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT license.

using System;
using System.Collections.Generic;
using System.ComponentModel.DataAnnotations;
using Microsoft.Extensions.Configuration;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using Moq;
using Newtonsoft.Json.Linq;

namespace Microsoft.Omex.Extensions.Abstractions.UnitTests
{
[TestClass]
public class ConfigurationValidationTests
{
[TestMethod]
public void GetNonNullableOrThrow_WhenValueNotNull()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In most cases our tests are named Method_Scenario_ExpectedResult.

{
// prepare mocks
string sectionKeyInt = "SectionKeyInt";
string sectionKeyString = "SectionKeyString";
string configValueInt = "5";
string configValueString = "asdf";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you accounting for a case where bound value is an object, not a primitive type?


Mock<IConfigurationSection> mockSectionInt = new Mock<IConfigurationSection>();
Mock<IConfigurationSection> mockSectionString = new Mock<IConfigurationSection>();
Mock<IConfiguration> mockConfig = new Mock<IConfiguration>();

mockSectionInt.Setup(x => x.Value).Returns(configValueInt);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

mockSectionString.Setup(x => x.Value).Returns(configValueString);
mockConfig.Setup(x => x.GetSection(It.Is<string>(k => k == sectionKeyInt))).Returns(mockSectionInt.Object);
mockConfig.Setup(x => x.GetSection(It.Is<string>(k => k == sectionKeyString))).Returns(mockSectionString.Object);

// assert
int resultInt = ConfigurationValidation.GetNonNullableOrThrow<int>(mockConfig.Object, sectionKeyInt);
string resultString = ConfigurationValidation.GetNonNullableOrThrow<string>(mockConfig.Object, sectionKeyString);
Assert.AreEqual(resultInt, int.Parse(configValueInt));
Assert.AreEqual(resultString, configValueString);
}


[TestMethod]
public void GetNonNullableOrThrow_WhenValueNull()
{
// prepare mocks
string sectionKey = "SectionKey";

Mock<IConfigurationSection> mockSection = new Mock<IConfigurationSection>();
Mock<IConfiguration> mockConfig = new Mock<IConfiguration>();

mockSection.Setup(x => x.Value).Returns((string?)null);
mockConfig.Setup(x => x.GetSection(It.Is<string>(k => k == sectionKey))).Returns(mockSection.Object);

// assert
Assert.ThrowsException<InvalidOperationException>(() => ConfigurationValidation.GetNonNullableOrThrow<string>(mockConfig.Object, sectionKey));
}

[TestMethod]
public void GetNonNullableOrThrow_WhenWrongType()
{
// prepare mocks
string sectionKey = "SectionKey";
string sectionKeyString = "SectionKeyString";

Mock<IConfigurationSection> mockSection = new Mock<IConfigurationSection>();
Mock<IConfiguration> mockConfig = new Mock<IConfiguration>();

mockSection.Setup(x => x.Value).Returns(sectionKeyString);
mockConfig.Setup(x => x.GetSection(It.Is<string>(k => k == sectionKey))).Returns(mockSection.Object);

// assert
Assert.ThrowsException<InvalidOperationException>(() => ConfigurationValidation.GetNonNullableOrThrow<int>(mockConfig.Object, sectionKey));
}
}
}