Skip to content

Multiple value exception, fixes #103 #105

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Apr 30, 2018
Merged

Multiple value exception, fixes #103 #105

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7b18eef
added exception
MV10 Apr 28, 2018
99cb4af
unit test for multiple conflicting argument values
MV10 Apr 28, 2018
36df0a6
code formatting
MV10 Apr 28, 2018
5e6d67d
verbiage and naming feedback
MV10 Apr 28, 2018
67e7ca8
more detailed test assertion and change text to match new exception
MV10 Apr 28, 2018
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
4 changes: 4 additions & 0 deletions src/Serilog.Settings.Configuration/Settings/Configuration/ConfigurationReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,10 @@ internal ILookup<string, Dictionary<string, IConfigurationArgumentValue>> GetMet
IConfigurationArgumentValue GetArgumentValue(IConfigurationSection argumentSection)
{
IConfigurationArgumentValue argumentValue;

if(argumentSection.Value != null && argumentSection.GetChildren().Any())
throw new InvalidOperationException($"Combined configuration sources must result in a discrete value (string, int, etc.) or complex value (section, list, etc.), not both. Argument: {argumentSection.Path}");
Copy link
Member

Choose a reason for hiding this comment

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

rest of code uses redundant braces ;)

Copy link
Member

Choose a reason for hiding this comment

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

please use standard formatting (ctrl K D) can do it - e.g. if ( should have a space

Copy link
Member

Choose a reason for hiding this comment

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

Given this has the same precondition, can move inside the other if to L230

Copy link
Member

Choose a reason for hiding this comment

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

On reflection - the rest of this file is light on redundant braces; please disregard. would be nice if the exception message can be split across lines (example from further down:

  throw new InvalidOperationException(
                            "A zero-length or whitespace assembly name was supplied to a Serilog.Using configuration statement.");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed about the if-space -- not my style but definitely seems to be the Serilog way.

On if/else I don't think braces are redundant. I eliminate on stand-alone one-liner ifs, but plenty of other if/else statements elsewhere in the code with braces that could be eliminated.

I think it's much clearer to perform the test separately (my guess is the compiler will optimize anyway, not that it would be measurable).

Copy link
Member

Choose a reason for hiding this comment

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

Fair point re it being subjective whether moving it inside there makes sense; I'll leave it to your judgement.

Copy link
Member

Choose a reason for hiding this comment

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

Suggest comment
// Reject cases where incoming config is mangled such that there are both scalar and aggregate values for this section as invalid
Suggest some elements of following message
"The configuration element: '{argumentSection.Path}', which is the result of combining multiple configuration inputs has an ambiguous set of values.\nPlease ensure inputs consistently supply either a scalar (int, string etc...) or a complex (section, list etc.) value for this argument section.");
"argument section" / "configuration element" / "scalar" are my terms and should only be used if they map to domain terms in serilog.settings.configuration or dotnetcore config stuff.

Copy link
Contributor Author

@MV10 MV10 Apr 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Ah! A fellow comment-junkie. I was just thinking, I had to explain this in more detail in our discussion, the check in the code probably warrants a blurb.

Sometimes the configuration represents POCOs, for example, so I think I'll stick with "complex types" rather than aggregates. I do like scalar better, though technically MEC treats all discrete values as string -- if your JSON has an int, by the time you get it from config, it'll be the ToString representation.

Thinking about this for a comment:

Reject configurations where an element has both scalar and complex values as a result of reading multiple configuration sources.

Revised exception message, considering these are always argument values being processed at this point in the code. I also realized there are only three valid scalar types in JSON:

The value for the argument {argumentSection.Path} is assigned different value types in more than one configuration source. Ensure all configurations consistently use either a scalar (int, string, boolean) or a complex (array, section, list, POCO, etc.) type for this argument value.

Of course, now it occurs to me that the very clear exception message probably makes the code comment redundant.


if (argumentSection.Value != null)
{
argumentValue = new StringArgumentValue(() => argumentSection.Value, argumentSection.GetReloadToken);
Expand Down
43 changes: 41 additions & 2 deletions test/Serilog.Settings.Configuration.Tests/ConfigurationSettingsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,11 @@ namespace Serilog.Settings.Configuration.Tests
{
public class ConfigurationSettingsTests
{
static LoggerConfiguration ConfigFromJson(string jsonString)
static LoggerConfiguration ConfigFromJson(string jsonString, string secondJsonSource = null)
Copy link
Member

Choose a reason for hiding this comment

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

could possibly make this params array, but rule of threee

Copy link
Contributor Author

@MV10 MV10 Apr 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

And YAGNI... I can't think of a test scenario where n config sources would do anything two sources can't represent.

{
var config = new ConfigurationBuilder().AddJsonString(jsonString).Build();
var builder = new ConfigurationBuilder().AddJsonString(jsonString);
if(secondJsonSource != null) builder.AddJsonString(secondJsonSource);
Copy link
Member

Choose a reason for hiding this comment

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

please autoformat

var config = builder.Build();
return new LoggerConfiguration()
.ReadFrom.Configuration(config);
}
Expand Down Expand Up @@ -556,5 +558,42 @@ public void WriteToSubLoggerWithLevelSwitchIsSupported()

Assert.Equal(1, DummyRollingFileSink.Emitted.Count);
}

[Trait("Bugfix", "#103")]
[Fact]
public void MultipleArgumentValuesThrowsInvalidOperationException()
Copy link
Member

Choose a reason for hiding this comment

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

InconsistentComplexVsScalarStateForAConfigurationNodeThrowsIOE

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 thinking:
InconsistentComplexVsScalarArgumentValuesThrowsIOE

{
var jsonDiscreteValue = @"{
""Serilog"": {
""Using"": [""TestDummies""],
""WriteTo"": [{
""Name"": ""DummyRollingFile"",
Copy link
Member

Choose a reason for hiding this comment

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

rolling file is deprecated, maybe example can start using File ?

Copy link
Member

Choose a reason for hiding this comment

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

hm, I see its used in the rest of the test codebase here; prob leave it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thinking as well.

""Args"": {""pathFormat"" : ""C:\\""}
}]
}
}";

var jsonComplexValue = @"{
""Serilog"": {
""Using"": [""TestDummies""],
""WriteTo"": [{
""Name"": ""DummyRollingFile"",
""Args"": {""pathFormat"" : { ""foo"" : ""bar"" } }
}]
}
}";

// These will combine into a ConfigurationSection object that has both
// Value == "C:\" and GetChildren() == List<string>. No configuration
// extension matching this exists (in theory an "object" argument could
// accept either value). ConfigurationReader should throw as soon as
// the multiple values are recognized; it will never attempt to locate
// a matching argument.

var ex = Assert.Throws<InvalidOperationException>(() => ConfigFromJson(jsonDiscreteValue, jsonComplexValue));
Copy link
Member

Choose a reason for hiding this comment

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

can you split the line please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't match other Assert.Throws in the tests or many other lambdas throughout the project, but it doesn't bother me either way, I'll update.


Assert.Contains("Combined configuration sources", ex.Message);
Assert.Contains("pathFormat", ex.Message);
Copy link
Member

Choose a reason for hiding this comment

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

use the ' bounding in the message I added and assert on the full path (can you pin nesting/context from parent too, or is the as good as it gets) ?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean about ' bounding? But the Path property does provide some additional context. I'll have to do some checking to see exactly how much, though, I think it changes depending on how you retrieve the section. On the other hand, pathFormat is as "deep" as you can go in this test data and still be processing anything related to the test target, so as-written it is arguably already a valid assertion.

}
}
}