Skip to content
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.

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

Skipping empty config sections without throwing exceptions #724

Merged
merged 4 commits into from
Feb 29, 2024

Conversation

RodionovDmitry
Copy link
Contributor

@RodionovDmitry RodionovDmitry commented Feb 9, 2024

This fixes NLog/NLog#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
Copy link

codecov-commenter commented Feb 9, 2024

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 ⚠️

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

Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@snakefoot
Copy link
Contributor

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
Copy link
Contributor Author

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.

Copy link
Contributor

@snakefoot snakefoot left a comment

Choose a reason for hiding this comment

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

Think it looks great. And thank you for the unit-tests.

@snakefoot snakefoot merged commit 6abd7d8 into NLog:master Feb 29, 2024
3 checks passed
@snakefoot
Copy link
Contributor

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

@RodionovDmitry RodionovDmitry deleted the empty-sections branch March 3, 2024 00:16
@RodionovDmitry
Copy link
Contributor Author

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Unrecognized value" exception on empty arrays and objects
3 participants