Do not access "Serilog" sub-section when calling ReadFrom.ConfigSection / properly populate IConfiguration in configuration methods
#144
Conversation
fixes serilog#143 where we are trying to read a subsection of config section passed as parameter
| public ConfigurationReader(IConfigurationSection configSection, DependencyContext dependencyContext) | ||
| { | ||
| _section = configSection ?? throw new ArgumentNullException(nameof(configSection)); | ||
| _configuration = configSection ?? throw new ArgumentNullException(nameof(configSection)); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 ...
but user tries to invoke a config method accepting a `IConfiguration`so that it is obvious that something is wrong. Probably accidentally fixes another issue
|
Ok, so it turns out that, while implementing an explicit error message for cases where calling This PR therefore fixes both #143 and #142 , and introduces tests for those cases. Unfortunately, because we rely on a I think the long term fix would be to get rid of that @nblumhardt @MV10 any opinion on the subject ? |
tsimbalar
changed the title
IConfiguration in configuration methods
|
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 ? |
3 participants
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
ConfigurationReaderhad several constructor overloads, one of them accepting aIConfigurationSectioninstead of aIConfiguration... but theIConfigurationSectionone was not being invoked by theReadFrom.ConfigSection()extension methods.Making that constructor
publicallows the section to be passed, but it also means theConfigurationReaderno longer has access to the fullConfigurationwhen that overload is used, so the behavior of Serilog.Settings.Configuration with config methods acceptingIConfigurationparameters may be subtly broken.I am wondering if to properly solve that we should :
ReadFrom.ConfigSectionas obsoleteReadFrom.ConfigSection(Configuration, sectionName:string)that is, we would still pass the full
Configurationobject, 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 :)