LoggingConfiguration - Redirect AddRuleForOneLevel and AddRuleForAllLevels to AddRule #6010

snakefoot · 2025-09-28T18:53:02Z

No description provided.

coderabbitai · 2025-09-28T18:53:09Z

Warning

Rate limit exceeded

@snakefoot has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 22 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between cf31dc7 and 0b78087.

📒 Files selected for processing (2)

src/NLog/Config/LoggingConfiguration.cs (9 hunks)
src/NLog/Config/LoggingRule.cs (3 hunks)

Walkthrough

LoggingConfiguration now ensures targets are registered before registering LoggingRule, centralizes AddRule overloads to funnel through common AddRule paths, and changes one target-not-found exception type. LoggingRule adds null-checks and conditional enabling for level ranges. No public API signatures changed.

Changes

Cohort / File(s)	Summary of Changes
Rule registration & target resolution `src/NLog/Config/LoggingConfiguration.cs`	- Ensure targets are registered (AddTargetThreadSafe) before creating/registering a LoggingRule. - AddRule(LoggingRule) now registers any Targets contained in the rule prior to adding the rule. - AddRule(minLevel,maxLevel, string targetName, ...) now throws NLogConfigurationException when a named target is not found (replacing NLogRuntimeException). - Removed inline null-guards for Target in one overload to defer handling downstream.
Overload consolidation & delegation `src/NLog/Config/LoggingConfiguration.cs`	- AddRuleForOneLevel and AddRuleForAllLevels overloads now delegate to the centralized AddRule(...) paths instead of constructing LoggingRule instances inline. - Centralized target-resolution and rule-creation flow to reduce duplicated guards and constructions.
Input validation & conditional enabling `src/NLog/Config/LoggingRule.cs`	- Constructors now call EnableLoggingForLevels conditionally (only when provided levels are not Off/null). - EnableLoggingForLevels now validates minLevel and maxLevel for null before updating filters. No public signatures changed.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor App
  participant LC as LoggingConfiguration
  participant T as TargetRegistry
  participant LR as LoggingRule

  rect rgba(200,230,255,0.25)
    note over App,LC: Overload delegation
    App->>LC: AddRuleForOneLevel/AllLevels(...)
    LC->>LC: Delegate to AddRule(minLevel,maxLevel, target|targetName, pattern, [final])
  end

  rect rgba(220,255,220,0.25)
    note over LC,T: Target resolution & registration
    LC->>T: Resolve target(s) (by name or object)
    LC->>T: AddTargetThreadSafe(target)    /* ensure registered */
    LC->>LR: Create LoggingRule(...)       /* using resolved targets */
    LC->>LC: AddRule(LoggingRule) -> registers rule targets then adds rule
  end

  rect rgba(255,240,200,0.25)
    note over App,LR: Level enabling validation
    App->>LR: EnableLoggingForLevels(minLevel, maxLevel)
    LR->>LR: Validate non-null and conditionally enable levels
  end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

I twitch my whiskers, tidy and bright,
Targets hop in line, registered right.
Levels checked gently, no null in sight,
Rules march forward, neat and light.
A rabbit nods — logging feels just right. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The pull request description is empty and does not provide any information about the changes or their purpose, making it impossible to understand the intent from the description alone.	Please add a descriptive summary that outlines the purpose of the refactoring, the main changes introduced, and any relevant context or motivation to help reviewers understand the pull request.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title accurately and succinctly identifies the primary change by referencing the LoggingConfiguration class and the redirection of AddRuleForOneLevel and AddRuleForAllLevels methods to the AddRule implementation, clearly summarizing the core refactoring intent.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (2)

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

242-244: Consistent null‑guards added

Good alignment with other range APIs and central validation via Guard.ThrowIfNull. Consider also clamping Off here as a defensive measure (e.g., treat maxLevel==Off as MaxLevel) to make the API more forgiving to callers, though the primary Off handling fix belongs at the call site in LoggingConfiguration (see comment there).
src/NLog/Config/LoggingConfiguration.cs (1)
346-350: Defensive null check when registering rule.Targets

If a consumer accidentally inserts null into LoggingRule.Targets, AddTargetThreadSafe will dereference it. Guard inside the loop to prevent NREs.

Apply this diff:
-            if (rule.Targets?.Count > 0)
-            {
-                foreach (var target in rule.Targets)
-                    AddTargetThreadSafe(target);
-            }
+            if (rule.Targets?.Count > 0)
+            {
+                foreach (var target in rule.Targets)
+                {
+                    if (target is null)
+                        continue; // Ignore invalid entries defensively
+                    AddTargetThreadSafe(target);
+                }
+            }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b8888ac and 13c5ddd.

📒 Files selected for processing (2)

src/NLog/Config/LoggingConfiguration.cs (9 hunks)
src/NLog/Config/LoggingRule.cs (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

src/NLog/Config/LoggingConfiguration.cs (3)

src/NLog/NLogConfigurationException.cs (4)

NLogConfigurationException (51-53)

NLogConfigurationException (59-62)

NLogConfigurationException (98-101)

NLogConfigurationException (115-118)

src/NLog/Config/LoggingRule.cs (5)

LoggingRule (59-61)

LoggingRule (66-69)

LoggingRule (78-84)

LoggingRule (92-98)

LoggingRule (105-110)

src/NLog/Internal/Guard.cs (1)

Guard (56-78)

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

src/NLog/Internal/Guard.cs (1)

Guard (56-78)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: task-list-completed

🔇 Additional comments (3)

src/NLog/Config/LoggingConfiguration.cs (3)

306-306: Exception type switch to NLogConfigurationException is appropriate

Configuration error surfaced with a clearer exception type. Please confirm no external code relies on catching NLogRuntimeException here.

334-337: Registering target before adding rule improves consistency

Target is now ensured to be known before the rule is stored, which helps later lookups and diagnostics. LGTM.

398-399: Centralizing AllLevels helpers through AddRule is cleaner

Reduces duplication and ensures consistent target registration and exception behavior. Nice refactor.

Also applies to: 408-409, 419-420

src/NLog/Config/LoggingConfiguration.cs

…evels to AddRule

sonarqubecloud · 2025-09-28T19:46:27Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
85.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

snakefoot added the refactoring label Sep 28, 2025

pull-request-size bot added the size/M label Sep 28, 2025

snakefoot force-pushed the dev branch from b8888ac to 13c5ddd Compare September 28, 2025 19:14

coderabbitai bot reviewed Sep 28, 2025

View reviewed changes

src/NLog/Config/LoggingConfiguration.cs Show resolved Hide resolved

snakefoot force-pushed the dev branch 3 times, most recently from cf31dc7 to e807900 Compare September 28, 2025 19:32

LoggingConfiguration - Redirect AddRuleForOneLevel and AddRuleForAllL…

0b78087

…evels to AddRule

snakefoot force-pushed the dev branch from e807900 to 0b78087 Compare September 28, 2025 19:37

snakefoot enabled auto-merge (squash) September 28, 2025 19:43

snakefoot merged commit f0c9bcc into NLog:dev Sep 28, 2025
6 checks passed

snakefoot added this to the 6.0.5 milestone Sep 30, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

LoggingConfiguration - Redirect AddRuleForOneLevel and AddRuleForAllLevels to AddRule #6010

LoggingConfiguration - Redirect AddRuleForOneLevel and AddRuleForAllLevels to AddRule #6010

Uh oh!

snakefoot commented Sep 28, 2025

Uh oh!

coderabbitai bot commented Sep 28, 2025 •

edited

Loading

Rate limit exceeded

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

sonarqubecloud bot commented Sep 28, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

LoggingConfiguration - Redirect AddRuleForOneLevel and AddRuleForAllLevels to AddRule #6010

LoggingConfiguration - Redirect AddRuleForOneLevel and AddRuleForAllLevels to AddRule #6010

Uh oh!

Conversation

snakefoot commented Sep 28, 2025

Uh oh!

coderabbitai bot commented Sep 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rate limit exceeded

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sonarqubecloud bot commented Sep 28, 2025

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai bot commented Sep 28, 2025 •

edited

Loading