NLog / NLog.Extensions.Logging

NLog as Logging Provider for Microsoft Extension Logging
https://nlog-project.org
BSD 2-Clause "Simplified" License
392 stars 151 forks source link

Skipping empty config sections without throwing exceptions #724

Closed RodionovDmitry closed 8 months ago

RodionovDmitry commented 9 months ago

This fixes https://github.com/NLog/NLog/issues/5469

When config file has empty sections like this

{
  "NLog": {
    "throwConfigExceptions": true,
    "variables": {},
    "extensions": [],
    "targets": {},
    "rules": []
  }
}

Those sections are read as key-value pairs with null as the value. This causes an exception when LoggingConfigurationParser is trying to parse it. There is a whitelist of properties allowed in the config. The exception message is also a bit misleading Unrecognized value 'targets'='' for element 'NLog'

In this PR I am checking if the "property" (which is actually an empty section) has no value. And if so - I am skipping it.

Added a test that blows up with exact same exception as the one described in the linked issue (before my changes). The test passes with the changes. I had to add a dependency to Microsoft.Extensions.Configuration.Json to be able to pass JSON config as a string. But this is for the test project only, so I hope it is OK.

codecov-commenter commented 9 months ago

Codecov Report

Attention: Patch coverage is 94.73684% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 81.18%. Comparing base (ce05f3b) to head (c0c2f78). Report is 2 commits behind head on master.

Files Patch % Lines
...ensions.Logging/Config/NLogLoggingConfiguration.cs 94.73% 0 Missing and 1 partial :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #724 +/- ## ========================================== - Coverage 81.21% 81.18% -0.03% ========================================== Files 19 19 Lines 1735 1743 +8 Branches 304 311 +7 ========================================== + Hits 1409 1415 +6 + Misses 189 188 -1 - Partials 137 140 +3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

snakefoot commented 9 months ago

Thank you for the pull-request, and starting the idea-race.

I guess it would work against the validation of unrecognized configuration-keys, as users might get confused why settings goes completely under the radar (ignored). Also curious what would happen with {myValue: null}, will it assign null or just skip the value?

Maybe the handling of simple-values assigned to null from JSON-config also needs to be tested (Unrelated to this bug).

RodionovDmitry commented 9 months ago

Thank you for the pull-request, and starting the idea-race.

I guess it would work against the validation of unrecognized configuration-keys, as users might get confused why settings goes completely under the radar (ignored). Also curious what would happen with {myValue: null}, will it assign null or just skip the value?

Maybe the handling of simple-values assigned to null from JSON-config also needs to be tested (Unrelated to this bug).

You have a valid concern :) I added a separate test that proves that unknown simple keys still cause an exception if they are initialized with an explicit null value.

snakefoot commented 8 months ago

Are you in a hurry and need a release right now? or are you fine with waiting ?

RodionovDmitry commented 8 months ago

Are you in a hurry and need a release right now? or are you fine with waiting ?

No hurry at all 😄

This PR was a coding interview task for me 😄

Thanks for you time and guidance here @snakefoot 🙏

snakefoot commented 6 months ago

NLog.Extensions.Logging v5.3.9 has been released, that allows official sections like targets and variables to be empty.

Thanks again for the contribution.