src/NLog/Config/LoggingConfigurationParser.cs (1)

550-558:Consider renaming variable for clarity.

The variabletargetsOptions is used within the rules validation context, which is confusing. Consider renaming it toruleOption,option, orruleAttribute to better reflect its purpose.

Apply this diff:

         if (rulesElement.Values.Count > 0)         {-            foreach (var targetsOptions in rulesElement.Values)+            foreach (var ruleOption in rulesElement.Values)             {-                var configException = new NLogConfigurationException($"Unrecognized value '{targetsOptions.Key}'='{targetsOptions.Value}' in section '{rulesElement.Name}'");+                var configException = new NLogConfigurationException($"Unrecognized value '{ruleOption.Key}'='{ruleOption.Value}' in section '{rulesElement.Name}'");                 if (MustThrowConfigException(configException))                     throw configException;             }         }

tests/NLog.UnitTests/Config/XmlConfigTests.cs (1)

197-223:Good test coverage for graceful degradation.

The test appropriately validates that unknown attributes on<targets> and<rules> elements don't break logging functionality when exception throwing is disabled. This aligns well with the new validation logic inLoggingConfigurationParser.cs.

Optional enhancement: Consider asserting that warnings about the unknown attributes (option='unknown' andbingo='tada') are logged to the InternalLogger, similar to howXmlConfig_attributes_shouldNotLogWarningsToInternalLog captures internal log output. This would provide more complete validation that the configuration parser properly detects and reports these issues.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

tests/NLog.UnitTests/Config/XmlConfigTests.cs (1)

197-223:Consider adding test coverage for the exception-throwing scenario.

The current test validates that unknown attributes don't break logging when exceptions are suppressed. However, there's no corresponding test that verifies exceptions ARE thrown whenthrowExceptions='true' orthrowConfigExceptions='true' is set.

Following the pattern ofShoudThrowExceptionOnDuplicateAttributeWhenOptionIsEnabledTest (line 433), consider adding a similar test for unknown attributes on targets and rules elements.

Example test structure:

[Fact]publicvoidShouldThrowExceptionOnUnknownAttributeWhenOptionIsEnabledTest(){Assert.Throws<NLogConfigurationException>(()=>{newLogFactory().Setup().LoadConfigurationFromXml(@"        <nlog throwExceptions='true'>            <targets option='unknown'>                <target name='debug' type='Debug' layout='${message}' />            </targets>            <rules>                <logger name='*' minlevel='info' appendto='debug' />            </rules>        </nlog>");});Assert.Throws<NLogConfigurationException>(()=>{newLogFactory().Setup().LoadConfigurationFromXml(@"        <nlog throwConfigExceptions='true'>            <targets>                <target name='debug' type='Debug' layout='${message}' />            </targets>            <rules bingo='tada'>                <logger name='*' minlevel='info' appendto='debug' />            </rules>        </nlog>");});}

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro