Do not access "Serilog" sub-section when calling ReadFrom.ConfigSection / properly populate IConfiguration in configuration methods

[tsimbalar]

fixes #143 where we are trying to read a subsection of config section passed as parameter

This is a regression introduced in v3.0.0

The ConfigurationReader had several constructor overloads, one of them accepting a IConfigurationSection instead of a IConfiguration... but the IConfigurationSection one was not being invoked by the ReadFrom.ConfigSection() extension methods.

Making that constructor public allows the section to be passed, but it also means the ConfigurationReader no longer has access to the full Configuration when that overload is used, so the behavior of Serilog.Settings.Configuration with config methods accepting IConfiguration parameters may be subtly broken.

I am wondering if to properly solve that we should :

• mark ReadFrom.ConfigSection as obsolete
• add a new ReadFrom.ConfigSection(Configuration, sectionName:string)

that is, we would still pass the full Configuration object, but look for a given section within ...

Any lights on this topic are welcome, I'm not 100% up-to-date with how Microsoft.Extensions.Configuration works :)

[MV10]

It was probably an oversight to leave public off this, but the comment mentions that IConfiguration parameters won't be populated in target methods, and populating _configuration isn't likely to help. The only place this matters that I'm aware of (so far) is in the SQL sink where the full config is used to look for named connection strings (which would be a config section external to the Serilog section, whatever name it might use).

I'd have to study this and think about the issue more but offhand I can't think of any other problems fixing it this way. I'd just leave _configuration null (without looking at the code, I assume the package would treat a null as not having an IConfiguration available to populate in the target method).

[tsimbalar]

There is a bit of ambiguity considering that IConfigSection implements/inherits IConfiguration ... I did set _configuration to the provided configSection thinking that this would at least allow to access sub-sections under the Serilog one ... but now I think of it, this definitely does not make any sense :P

I think the best approach for now would therefore be indeed to let _configuration undefined. It would probably be better to throw an explicit exception when we notice that the user is trying to use a configuration method that accepts an IConfiguration, but no ambient Configuration is available (typically : a call to ReadFrom.ConfigSection) ...

We could provide an overload of ReadFrom.ConfigSection that accepts both an IConfiguration and a IConfigSection so that the functionality is still available somehow ...

[tsimbalar]

Ok, so it turns out that, while implementing an explicit error message for cases where calling ReadFrom.ConfigSection and trying to call configuration methods expecting an IConfiguration, I could reproduce #142 ...

This PR therefore fixes both #143 and #142 , and introduces tests for those cases.

Unfortunately, because we rely on a static ConfigurationReader._configuration field, our tests are no longer deterministic (the test LoggerConfigurationExtensionsTests.ReadFromConfigurationSectionThrowsWhenTryingToCallConfigurationMethodWithIConfigurationParam passes when run alone, but not anymore where the whole test suite is run).

I think the long term fix would be to get rid of that static state, but it may be better to release a v3.0.1 bugfix first ?

@nblumhardt @MV10 any opinion on the subject ?

[nblumhardt]

Love the added test coverage :-) 👍

+1 from me, seems like getting this to dev and asking for some confirmation on the original report would be the best way to move it forward. @MV10 ?