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.

Sign up for GitHub

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

Jump to bottom

LoggingConfigurationParser - Moved attribute validation from Xml-Config #3466

Merged
merged 2 commits into from
Jul 2, 2019
Merged

LoggingConfigurationParser - Moved attribute validation from Xml-Config #3466

merged 2 commits into from
Jul 2, 2019

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Jun 9, 2019 •
edited
Loading

Improves the validation for NLogLoggingConfiguration (appsettings-json)

Removing Value from ILoggingConfigurationElement. Unless full support for checking the Value in all locations where possible (And not just for variables). And also avoid too much diversion from XML and JSON-config (Suddenly supporting unnamed-properties).

Instead one can do this, where one assigns the value-property (and allow <layout> as alias):

<variable name="mailBodyLayout">
<layout>
<![CDATA[${onexception:inner=${exceptionMailBodyLayout}:whenempty=${standardMailBodyLayout}}

---
${application-name} v${application-version}

Server: ${machinename}
Date: ${longdate}
${when:when=length('${iis-site-name}') > 0:inner=${iisApplicationInformation}}
Base Directory: ${basedir}
Identity: ${windows-identity}
Process Time: ${processtime}]]>
</layout>
</variable>

@snakefoot snakefoot mentioned this pull request Jun 9, 2019
Merged
@snakefoot snakefoot force-pushed the LoggingConfigurationParseValidation branch from 26298fc to 966e30a Compare June 10, 2019 00:03
@snakefoot snakefoot force-pushed the LoggingConfigurationParseValidation branch 2 times, most recently from 172d851 to 009c32a Compare June 10, 2019 00:19
@304NotModified 304NotModified self-requested a review June 10, 2019 00:41
@snakefoot snakefoot force-pushed the LoggingConfigurationParseValidation branch from 009c32a to d7845ab Compare June 10, 2019 11:54
@codecov-io
Copy link

codecov-io commented Jun 10, 2019 •
edited
Loading

Codecov Report

Merging #3466 into dev will decrease coverage by <1%.
The diff coverage is 85%.

@@          Coverage Diff           @@
##             dev   #3466    +/-   ##
======================================
- Coverage     81%     81%   -<1%     
======================================
  Files        345     345            
  Lines      27886   27917    +31     
  Branches    3778    3798    +20     
======================================
+ Hits       22483   22497    +14     
- Misses      4305    4322    +17     
  Partials    1098    1098

@snakefoot snakefoot force-pushed the LoggingConfigurationParseValidation branch 3 times, most recently from 0b7d9dd to d8efb55 Compare June 10, 2019 12:26
304NotModified
304NotModified reviewed Jun 10, 2019
View reviewed changes
Copy link
Member

@304NotModified 304NotModified left a comment

Choose a reason for hiding this comment

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

cool, so no breaking change in the interface 👍

src/NLog/Config/LoggingConfigurationParser.cs
_validChildren.Add(validChild);
yield return validChild;
}
_validChildren = _validChildren ?? ArrayHelper.Empty<ValidatedConfigurationElement>();
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit unexpected (according to the method name)

could we move this out of this method, or rename the method (YieldAndCacheValidChildren or something like that)

Copy link
Contributor Author

@snakefoot snakefoot Jun 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Will rename the method as suggested. But the reason is to perform lazy validation of children (But only once)

src/NLog/Config/LoggingConfigurationParser.cs
@@ -1295,9 +1278,122 @@ private static string GetName(Target target)
return string.IsNullOrEmpty(target.Name) ? target.GetType().Name : target.Name;
}

private class ValidatedConfigurationElement : ILoggingConfigurationElement
Copy link
Member

Choose a reason for hiding this comment

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

I which level this element is "Validated"? I don't fully understand the class name

Copy link
Contributor Author

@snakefoot snakefoot Jun 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It currently performs validation of attribute-name-uniqueness., But more validation can be added later (Ex. valid element-Name)

@304NotModified
Copy link
Member

PS:

And also avoid too much diversion from XML and JSON-config (Suddenly supporting unnamed-properties).

The unnamed property wasn't suddenly for me, and please note it's not unique for XML (yes, it isn't in JSON).

@snakefoot
Copy link
Contributor Author

snakefoot commented Jun 10, 2019 •
edited
Loading

The unnamed property wasn't suddenly for me, and please note it's not unique for XML (yes, it isn't in JSON).

Lets say config-item in Nlog has 2 attributes (or more). How do you specify which attribute is configured by the unnamed-property-value?

If you want Xml-config to automatically update the "value"-property (on whatever config-item in question) when using xml-element-value. Then you should implement this magic in the xml-element-node, so it yields the unnamed xml-element-value as a bonus attribute with name=value value=xml-element-value.

@snakefoot snakefoot force-pushed the LoggingConfigurationParseValidation branch from d8efb55 to 85d58c7 Compare June 10, 2019 22:13
@304NotModified 304NotModified modified the milestones: 5.6, 5.0 (new) Jun 13, 2019
@304NotModified
Copy link
Member

@snakefoot now it's my time for holiday :)
I'm a short week in Rome, so I'm limited with response and this PR will be probably after that week.

@snakefoot
Copy link
Contributor Author

snakefoot commented Jun 14, 2019 via email •
edited
Loading

@304NotModified
Copy link
Member

-> snakefoot#9

@snakefoot
Copy link
Contributor Author

snakefoot commented Jun 25, 2019 •
edited
Loading

-> snakefoot#9

Seems your removal of my explicit cast has now broken the build:

https://ci.appveyor.com/project/nlog/nlog/builds/25516652

And Xml_configuration_variableWithInnerAndAttribute_attributeHasPrecedence is now failing:

https://travis-ci.org/NLog/NLog/builds/550093878?utm_source=github_status&utm_medium=notification

@304NotModified
Copy link
Member

Seems your removal of my explicit cast has now broken the build:

https://ci.appveyor.com/project/nlog/nlog/builds/25516652

OK, I will check that. Locally it works without cast.

@304NotModified 304NotModified self-assigned this Jun 25, 2019
@304NotModified
Copy link
Member

Unfortunately the logs are always a bit unclear. I forgot .NET 3.5 doesn't support covariance interfaces :(

@304NotModified
Copy link
Member

did I broke this?

image

@snakefoot
Copy link
Contributor Author

snakefoot commented Jun 25, 2019 •
edited
Loading

did I broke this?

No the validation introduced with this PR just makes sure you don't have invalid configuration. You have created the invalid test config after this PR was introduced.

@304NotModified 304NotModified removed their assignment Jun 25, 2019
@snakefoot snakefoot force-pushed the LoggingConfigurationParseValidation branch from cbf669f to ba2e4de Compare June 30, 2019 19:19
@snakefoot
Copy link
Contributor Author

snakefoot commented Jun 30, 2019 •
edited
Loading

Have now fixed Xml_configuration_variableWithInnerAndAttribute_attributeHasPrecedence so the unit-test ignores (the improved) validation of invalid config.

@304NotModified
Copy link
Member

Thanks!

@304NotModified 304NotModified merged commit ad850f8 into NLog:dev Jul 2, 2019
@snakefoot snakefoot deleted the LoggingConfigurationParseValidation branch April 4, 2020 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@304NotModified 304NotModified 304NotModified left review comments

Assignees
No one assigned
Labels
refactoring size/XL
Projects
None yet
Milestone
5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@snakefoot @codecov-io @304NotModified