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

Issue 36015 new BinderOption to throw on missing configuration #53852

Merged
Show file tree
Hide file tree
Changes from 8 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
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ public partial class BinderOptions
{
public BinderOptions() { }
public bool BindNonPublicProperties { get { throw null; } set { } }
public bool ErrorOnUnknownConfiguration { get { throw null; } set { } }
}
public static partial class ConfigurationBinder
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,13 @@ public class BinderOptions
/// If true, the binder will attempt to set all non read-only properties.
/// </summary>
public bool BindNonPublicProperties { get; set; }

/// <summary>
/// When false (the default), no exceptions are thrown when a configuration key is found for which the
/// provided model object does not have an appropriate property which matches the key's name.
/// When true, an <see cref="System.InvalidOperationException"/> is thrown with a description
/// of the missing properties.
/// </summary>
public bool ErrorOnUnknownConfiguration { get; set; }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,27 @@ private static void BindNonScalar(this IConfiguration configuration, object inst
{
if (instance != null)
{
foreach (PropertyInfo property in GetAllProperties(instance.GetType()))
List<PropertyInfo> modelProperties = GetAllProperties(instance.GetType()).ToList();
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need the ToList()?

Copy link
Member

Choose a reason for hiding this comment

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

I see it is to enumerate the list twice. Maybe we can void it and check this as we bind the properties, aggregate the missing names on a list if the flag to throw is enabled and then throw?

Copy link
Contributor Author

@SteveDunn SteveDunn Jun 15, 2021

Choose a reason for hiding this comment

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

If performance is the driving force behind this suggestion, then enumerating just once would be the best thing to do. I'm guessing though, that binding configuration won't be on a hot path. Hence, in my opinion, it is more beneficial this way as:

  • the list of properties has already been gathered (we're just enumerating twice, not building it twice)
  • it is logically self-contained; a distinct step before any binding is attempted
  • it fails fast: if we check as we bind we could end up with an object which has some properties that are set and some which aren't set
  • if we check as we bind, then the enumeration of property names in config would happen on BindProperty, which would result in more enumerations than we currently have

Copy link
Member

Choose a reason for hiding this comment

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

Those are valid points. Sounds good. I think since this is opt-in, I don't mind an extra enumeration, also Binding usually just happens once in a lifetime of the application.

What do you think of removing Linq usages? We have been trying to do that on these libraries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to remove Linq usages if that fits in with the plan for libraries. May I ask why though? I've certainly seen, in my day job, overuse of Linq especially in tight loops, which cause a lot of overhead for the GC with temporary objects. But outside of tight loops or hot paths, I think the declarative nature of Linq makes the code easier to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just looked into doing this imperatively by creating an empty List to hold the missing property names and found that we could end up with a sparse array, which I estimate would counter any gains achieved by switching from Linq. One the one hand, we create a zero-sized List, in which case the size doubles for every addition potentially leading to a sparse array, or we could create the List with the same size as the amount of children and assume everything is missing, in which case it's likely that not everything is missing and we'll again end up with a sparse array.

I don't think these scenarios are tragic though; whether it's the overhead of a Linq enumerator, or a few dozen empty slots in an array, this usually happens just once.

Bearing this in mind, I think it's no less efficient to use Linq, but If you'd like me to switch, then I'm happy to do so.

Copy link
Member

Choose a reason for hiding this comment

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

I don't mind an extra enumeration, also Binding usually just happens once in a lifetime of the application.

I think if you use IOptionsSnapshot for example then binding could happen once per request

Copy link
Member

Choose a reason for hiding this comment

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

Also, Microsoft.Extensions.Configuration has already used Linq to build the collection that we further 'compose'

The reason reducing Linq was brought up, has to do with this issue #44598, for example https://github.com/dotnet/runtime/pull/44825/files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found this post by Andrew Lock:
https://andrewlock.net/the-dangers-and-gotchas-of-using-scoped-services-when-configuring-options-in-asp-net-core/

It says:

Note: As the strongly-typed settings are re-built with every request, and the binding relies on reflection under the hood, you should bear performance in mind. There is currently an open issue on GitHub to investigate performance.

It links to aspnet/Configuration#604

Seeing as Reflection is the slowest part of the process, I think the extra enumeration is fairly insignificant.

Copy link
Member

Choose a reason for hiding this comment

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

GetAllProperties is a private method that's typed as returning IEnumerable<PropertyInfo>, but its implementation is actually constructing and returning a List<PropertyInfo>. The right change here then is to just change that private method to return a List<PropertyInfo> and remove the ToList call from the call site.


if (options.ErrorOnUnknownConfiguration)
{
HashSet<string> propertyNames = new(modelProperties.Select(mp => mp.Name),
StringComparer.OrdinalIgnoreCase);

List<string> missingPropertyNames = configuration.GetChildren()
.Where(cs => !propertyNames.Contains(cs.Key))
.Select(mp => $"'{mp.Key}'")
.ToList();

if (missingPropertyNames.Count > 0)
{
throw new InvalidOperationException(SR.Format(SR.Error_MissingConfig,
nameof(options.ErrorOnUnknownConfiguration), nameof(BinderOptions), instance.GetType(),
string.Join(", ", missingPropertyNames)));
}
}

foreach (PropertyInfo property in modelProperties)
{
BindProperty(property, instance, configuration, options);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
<?xml version="1.0" encoding="utf-8"?>
maryamariyan marked this conversation as resolved.
Show resolved Hide resolved
<root>
<!--
Microsoft ResX Schema

<!--
Microsoft ResX Schema
Version 2.0

The primary goals of this format is to allow a simple XML format
that is mostly human readable. The generation and parsing of the
various data types are done through the TypeConverter classes
The primary goals of this format is to allow a simple XML format
that is mostly human readable. The generation and parsing of the
various data types are done through the TypeConverter classes
associated with the data types.

Example:

... ado.net/XML headers & schema ...
<resheader name="resmimetype">text/microsoft-resx</resheader>
<resheader name="version">2.0</resheader>
Expand All @@ -26,36 +26,36 @@
<value>[base64 mime encoded string representing a byte array form of the .NET Framework object]</value>
<comment>This is a comment</comment>
</data>

There are any number of "resheader" rows that contain simple
There are any number of "resheader" rows that contain simple
name/value pairs.

Each data row contains a name, and value. The row also contains a
type or mimetype. Type corresponds to a .NET class that support
text/value conversion through the TypeConverter architecture.
Classes that don't support this are serialized and stored with the
Each data row contains a name, and value. The row also contains a
type or mimetype. Type corresponds to a .NET class that support
text/value conversion through the TypeConverter architecture.
Classes that don't support this are serialized and stored with the
mimetype set.

The mimetype is used for serialized objects, and tells the
ResXResourceReader how to depersist the object. This is currently not
The mimetype is used for serialized objects, and tells the
ResXResourceReader how to depersist the object. This is currently not
extensible. For a given mimetype the value must be set accordingly:

Note - application/x-microsoft.net.object.binary.base64 is the format
that the ResXResourceWriter will generate, however the reader can
Note - application/x-microsoft.net.object.binary.base64 is the format
that the ResXResourceWriter will generate, however the reader can
read any of the formats listed below.

mimetype: application/x-microsoft.net.object.binary.base64
value : The object must be serialized with
value : The object must be serialized with
: System.Runtime.Serialization.Formatters.Binary.BinaryFormatter
: and then encoded with base64 encoding.

mimetype: application/x-microsoft.net.object.soap.base64
value : The object must be serialized with
value : The object must be serialized with
: System.Runtime.Serialization.Formatters.Soap.SoapFormatter
: and then encoded with base64 encoding.

mimetype: application/x-microsoft.net.object.bytearray.base64
value : The object must be serialized into a byte array
value : The object must be serialized into a byte array
: using a System.ComponentModel.TypeConverter
: and then encoded with base64 encoding.
-->
Expand Down Expand Up @@ -126,10 +126,13 @@
<data name="Error_FailedToActivate" xml:space="preserve">
<value>Failed to create instance of type '{0}'.</value>
</data>
<data name="Error_MissingConfig" xml:space="preserve">
<value>'{0}' was set on the provided {1}, but the following properties were not found on the instance of {2}: {3}</value>
</data>
<data name="Error_MissingParameterlessConstructor" xml:space="preserve">
<value>Cannot create instance of type '{0}' because it is missing a public parameterless constructor.</value>
</data>
<data name="Error_UnsupportedMultidimensionalArray" xml:space="preserve">
<value>Cannot create instance of type '{0}' because multidimensional arrays are not supported.</value>
</data>
</root>
</root>
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,56 @@ public void GetNullValue()
Assert.Null(config.GetSection("Object").Get<ComplexOptions>());
}

[Fact]
public void ThrowsIfPropertyInConfigMissingInModel()
{
var dic = new Dictionary<string, string>
{
{"ThisDoesNotExistInTheModel", "42"},
{"Integer", "-2"},
{"Boolean", "TRUe"},
{"Nested:Integer", "11"}
};
var configurationBuilder = new ConfigurationBuilder();
configurationBuilder.AddInMemoryCollection(dic);
var config = configurationBuilder.Build();

var instance = new ComplexOptions();

var ex = Assert.Throws<InvalidOperationException>(
() => config.Bind(instance, o => o.ErrorOnUnknownConfiguration = true));

string expectedMessage = SR.Format(SR.Error_MissingConfig,
nameof(BinderOptions.ErrorOnUnknownConfiguration), nameof(BinderOptions), typeof(ComplexOptions), "'ThisDoesNotExistInTheModel'");

Assert.Equal(expectedMessage, ex.Message);
}
[Fact]
public void ThrowsIfPropertyInConfigMissingInNestedModel()
{
var dic = new Dictionary<string, string>
{
{"Nested:ThisDoesNotExistInTheModel", "42"},
{"Integer", "-2"},
{"Boolean", "TRUe"},
{"Nested:Integer", "11"}
};
var configurationBuilder = new ConfigurationBuilder();
configurationBuilder.AddInMemoryCollection(dic);
var config = configurationBuilder.Build();

var instance = new ComplexOptions();

string expectedMessage = SR.Format(SR.Error_MissingConfig,
nameof(BinderOptions.ErrorOnUnknownConfiguration), nameof(BinderOptions), typeof(NestedOptions), "'ThisDoesNotExistInTheModel'");

var ex = Assert.Throws<InvalidOperationException>(
() => config.Bind(instance, o => o.ErrorOnUnknownConfiguration = true));


maryamariyan marked this conversation as resolved.
Show resolved Hide resolved
Assert.Equal(expectedMessage, ex.Message);
}

[Fact]
public void GetDefaultsWhenDataDoesNotExist()
{
Expand Down