Multiple value exception, fixes #103 #105

MV10 · 2018-04-28T14:29:29Z

Throws InvalidOperationException if multiple configuration sources results in an ambiguous argument value. See #103 for details.

bartelink

Test looks good. I personally don't understand the problem well enough to say whether this is definitely the best solution

bartelink · 2018-04-28T15:18:45Z

src/Serilog.Settings.Configuration/Settings/Configuration/ConfigurationReader.cs

@@ -221,6 +221,10 @@ void ApplyEnrichment(LoggerConfiguration loggerConfiguration, IReadOnlyDictionar
            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}");


rest of code uses redundant braces ;)

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

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

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.");

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).

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

bartelink · 2018-04-28T15:25:18Z

test/Serilog.Settings.Configuration.Tests/ConfigurationSettingsTests.cs

+            // the multiple values are recognized; it will never attempt to locate
+            // a matching argument.
+
+            var ex = Assert.Throws<InvalidOperationException>(() => ConfigFromJson(jsonDiscreteValue, jsonComplexValue));


can you split the line please?

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.

bartelink · 2018-04-28T15:25:30Z

test/Serilog.Settings.Configuration.Tests/ConfigurationSettingsTests.cs

        {
-            var config = new ConfigurationBuilder().AddJsonString(jsonString).Build();
+            var builder = new ConfigurationBuilder().AddJsonString(jsonString);
+            if(secondJsonSource != null) builder.AddJsonString(secondJsonSource);


please autoformat

MV10 · 2018-04-28T16:06:04Z

@bartelink Thanks for the review. Are Serilog code standards posted somewhere?

I just realized after I push my edits, my replies to your review comments will go away, so copying here:

Agreed about space after if -- not my style but definitely the Serilog way.
Disagree about if/else redundant braces. Agree about one-liner if but there are plenty of other if/else blocks in the code where one could theoretically remove the braces.
Disagree about moving the exception test inside the following if -- I believe it is clearer to test for this separately, and I'm sure the compiler will optimize this anyway (not that the performance diff would be measurable).
I'll split the Assert.Throws lambda (that's my preferred style, too), but it doesn't match other Assert.Throws or other lambdas in general elsewhere in the code.
JSON utility function reformatted.

MV10 · 2018-04-28T16:07:38Z

@bartelink Maybe I can explain the issue more clearly.

Microsoft.Extensions.Configuration allows an application to load configuration information from multiple sources. The most common example is when an application has an appsettings.production.json file and an appsettings.development.json file. Both are loaded in debug and the dev file typically contains a subset of settings overriding the production settings (dev is loaded after the production version).

Consider these two simple bits of JSON (an extremely oversimplified example):

{
    "setting" : "discrete"
}

versus

{
    "setting" : [ "foo", "bar", "baz" ]
}

After reading the first one, the Value property is a string containing "discrete", but after reading the second one (independently), Value will be null -- you must handle the array using the Options.Abstractions package to deserialize via Get<List<string>>, or call GetChildren() to iterate over the contents (and in more complicated scenarios it may also be applicable to treat the value as a new IConfigurationSection). If you load both versions of the JSON during startup (such as overlaying debug settings on production settings), then Value is populated and the other data is also populated at the same time.

This package is expecting a single value to provide to a named argument, and there is no way to determine whether the contents of Value is the intended "final" value or the complex data is the final value.

bartelink · 2018-04-28T16:23:10Z

There is no serilog style. I worked in a different repo. I put in redundant braces that I hate to fit the style. There was 100% consistency in that repo about redundant braces.
The consistency is more important than that any one person is happy. I'd say in general though, like many projects, the general style is to do default IDE formatting - either that or people post a style guide and then lots of people waste ages nitpicking. I'm a nitpicker and enjoy it; but the only point of it all is to make code easy to read. The two points I made about line breaks were my main concern.
Re ifs inside/outside, as I responded - perf has nothing to do with it - if you feel this is a global precondition and are trying to stress that, that's plenty reasonable - the readability and maintaintabiliity trumps perf any time.

bartelink · 2018-04-28T16:25:00Z

I'm sure the readme and/or the Issues document all this stuff, but I really appreciate the detailed explanation. Will see if I can provide more useful comments given that context...

MV10 · 2018-04-28T16:34:34Z

Huh, just learned the "View Changes" button also shows the old comments.

I'm also doing work in the SQL sink and that repo is all over the place in terms of style, but I agree, readability is very important. I'm also tempted to add lots of comments but there doesn't seem to be much commenting in the Serilog codebase that I've seen.

I did consider splitting the line on the exception message, but even then it's still very long, I'm not sure much is to be gained there. I followed the example of a much longer exception earlier in that file.

And it fits just fine on my ridiculous 3200x1800 screen! 😁

bartelink

This looks great now - thanks for taking the time to explain.

I agree with your impl being exaclty as it is and only have some wording suggestions.

I'm not a reviewer on this repo or knowledgeable about the config system though, so probably best to not consider my approval to be final, no matter how picky I've been and patient you've been ;)

bartelink · 2018-04-28T16:34:04Z

test/Serilog.Settings.Configuration.Tests/ConfigurationSettingsTests.cs

+                ""Serilog"": {            
+                    ""Using"": [""TestDummies""],
+                    ""WriteTo"": [{
+                        ""Name"": ""DummyRollingFile"",


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

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

My thinking as well.

bartelink · 2018-04-28T16:35:14Z

test/Serilog.Settings.Configuration.Tests/ConfigurationSettingsTests.cs

+
+        [Trait("Bugfix", "#103")]
+        [Fact]
+        public void MultipleArgumentValuesThrowsInvalidOperationException()


InconsistentComplexVsScalarStateForAConfigurationNodeThrowsIOE

I'm thinking:
InconsistentComplexVsScalarArgumentValuesThrowsIOE

bartelink · 2018-04-28T16:36:36Z

test/Serilog.Settings.Configuration.Tests/ConfigurationSettingsTests.cs

+                => ConfigFromJson(jsonDiscreteValue, jsonComplexValue));
+
+            Assert.Contains("Combined configuration sources", ex.Message);
+            Assert.Contains("pathFormat", ex.Message);


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) ?)

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.

bartelink · 2018-04-28T16:37:39Z

test/Serilog.Settings.Configuration.Tests/ConfigurationSettingsTests.cs

@@ -12,9 +12,12 @@ namespace Serilog.Settings.Configuration.Tests
 {
    public class ConfigurationSettingsTests
    {
-        static LoggerConfiguration ConfigFromJson(string jsonString)
+        static LoggerConfiguration ConfigFromJson(string jsonString, string secondJsonSource = null)


could possibly make this params array, but rule of threee

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

bartelink · 2018-04-28T16:37:58Z

src/Serilog.Settings.Configuration/Settings/Configuration/ConfigurationReader.cs

@@ -221,6 +221,10 @@ void ApplyEnrichment(LoggerConfiguration loggerConfiguration, IReadOnlyDictionar
            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}");


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.

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.

bartelink · 2018-04-28T16:38:48Z

test/Serilog.Settings.Configuration.Tests/ConfigurationSettingsTests.cs

+            // a matching argument.
+
+            var ex = Assert.Throws<InvalidOperationException>(()
+                => ConfigFromJson(jsonDiscreteValue, jsonComplexValue));


think => shoudl go prev line ?

I think one line would be consistent with the rest of the code. Earlier you suggested the split (I tend to split them myself).

I meant that the => rarely starts a line in normal code and instead goes on prev line
i.e. the full () => on prev line

bartelink · 2018-04-28T16:40:16Z

src/Serilog.Settings.Configuration/Settings/Configuration/ConfigurationReader.cs

@@ -221,6 +221,10 @@ void ApplyEnrichment(LoggerConfiguration loggerConfiguration, IReadOnlyDictionar
            IConfigurationArgumentValue GetArgumentValue(IConfigurationSection argumentSection)
            {
                IConfigurationArgumentValue argumentValue;
+
+                if (argumentSection.Value != null && argumentSection.GetChildren().Any())


now you've explained it, this precondition check absolutely belongs here on its own and not nested

bartelink · 2018-04-28T16:48:59Z

I'm also doing work in the SQL sink and that repo is all over the place in terms of style, but I agree, readability is very important. I'm also tempted to add lots of comments but there doesn't seem to be much commenting in the Serilog codebase that I've seen.

One can def go overboard on comments, but for example the guard you're adding means nothing to me as I've never written code in the config system but for example can imagine myself ending up reading that code if I meet the exception. In the case of that exception message, if it can't just in time teach me by having a full article (brevity is important), then a comment can do half of the jon

I did consider splitting the line on the exception message, but even then it's still very long, I'm not sure much is to be gained there. I followed the example of a much longer exception earlier in that file.

I try to:

"sentence. "
+ "And another thing"

as its a good forcing function for confronting the fact that you havent taken the time to write a short letter ;)

And it fits just fine on my ridiculous 3200x1800 screen! 😁

:D I have a nice monitor here but 2 monitors in work are 1080 :( In general I look at github stuff in split view too as I need all the help I can get to remember how using short lines actually tends to cause better code in the end, even if it feels artificial!)

MV10 · 2018-04-28T17:37:33Z

Great feedback. Much happier with the exception message in particular.

MV10 · 2018-04-28T17:46:14Z

Realized what you meant about ' in the exception message and updated the test to assert against:

'Serilog:WriteTo:0:Args:pathFormat'

No ambiguity there!

bartelink · 2018-04-28T20:03:12Z

test/Serilog.Settings.Configuration.Tests/ConfigurationSettingsTests.cs

+            // a matching argument.
+
+            var ex = Assert.Throws<InvalidOperationException>(()
+                => ConfigFromJson(jsonDiscreteValue, jsonComplexValue));


I meant that the => rarely starts a line in normal code and instead goes on prev line
i.e. the full () => on prev line

bartelink · 2018-04-28T20:03:29Z

src/Serilog.Settings.Configuration/Settings/Configuration/ConfigurationReader.cs

+                // values as a result of reading multiple configuration sources.
+                if (argumentSection.Value != null && argumentSection.GetChildren().Any())
+                    throw new InvalidOperationException(
+                        $"The value for the argument '{argumentSection.Path}' is assigned different value " +


I keep thinking ArgumentException but I guess that's best reserved for actual method arguments.

That was my first inclination, too, but then I noticed IOE is used elsewhere in the code for similar "processing" errors.

nblumhardt · 2018-04-30T21:32:50Z

👍 great discussion here :-)

RE code style, unfortunately the Serilog org is pretty massive (a large number of sink repos) and consistency hasn't been our strong point. Mostly aiming for principle-of-least-surprise coding style; as much as I do love a good nitpick, there's a fair bit that comes down to taste and context. LGTM!

MV10 added 2 commits April 28, 2018 09:40

added exception

7b18eef

unit test for multiple conflicting argument values

99cb4af

bartelink reviewed Apr 28, 2018

View reviewed changes

code formatting

36df0a6

bartelink approved these changes Apr 28, 2018

View reviewed changes

verbiage and naming feedback

5e6d67d

more detailed test assertion and change text to match new exception

67e7ca8

bartelink reviewed Apr 28, 2018

View reviewed changes

nblumhardt merged commit e9dfb98 into serilog:dev Apr 30, 2018

MV10 deleted the multiple-value-exception branch May 1, 2018 21:53

merbla mentioned this pull request Oct 7, 2018

3.0 Release #112

Merged

Multiple value exception, fixes #103 #105

Multiple value exception, fixes #103 #105

Conversation

MV10 commented Apr 28, 2018

bartelink left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MV10 commented Apr 28, 2018

MV10 commented Apr 28, 2018 • edited Loading

bartelink commented Apr 28, 2018

bartelink commented Apr 28, 2018

MV10 commented Apr 28, 2018

bartelink left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MV10 Apr 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MV10 Apr 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartelink commented Apr 28, 2018

MV10 commented Apr 28, 2018

MV10 commented Apr 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nblumhardt commented Apr 30, 2018

MV10 commented Apr 28, 2018 •

edited

Loading

MV10 Apr 28, 2018 •

edited

Loading

MV10 Apr 28, 2018 •

edited

Loading

MV10 commented Apr 28, 2018 •

edited

Loading