JsonLayout - Reduce code complexity for rendering JsonAttribute by snakefoot · Pull Request #6026 · NLog/NLog

snakefoot · 2025-10-14T06:37:18Z

No description provided.

coderabbitai · 2025-10-14T06:37:44Z

Walkthrough

Refactors JSON layout value rendering by introducing a helper to centralize rendering and non-empty checks, adjusts control flow for encode/no-encode paths, simplifies escaping, and updates decision points based on StringBuilder length. Removes an obsolete escape helper and narrows a serializer method’s visibility without changing public APIs.

Changes

Cohort / File(s)	Summary
JSON value rendering refactor `src/NLog/Layouts/JSON/JsonAttribute.cs`, `src/NLog/Layouts/JSON/JsonLayout.cs`	Adds a private helper to render/verify non-empty values; updates encode and non-encode paths to use valueStart; bases property inclusion on StringBuilder length; simplifies escaping flow; removes the old PerformJsonEscapeIfNeeded; converts a static helper to an instance method and handles rollback on render failure.
Serializer escaping adjustments `src/NLog/Targets/DefaultJsonSerializer.cs`	Narrows RequiresJsonEscape to private; minor refactor in PerformJsonEscapeWhenNeeded using local length and direct AppendStringEscape call; updates length calculations. No public API changes beyond visibility.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Logger
  participant JsonLayout
  participant JsonAttribute
  participant Serializer as DefaultJsonSerializer

  Logger->>JsonLayout: RenderJsonFormattedMessage(logEvent)
  JsonLayout->>JsonLayout: AppendJsonPropertyValue(...)
  alt Encode = false
    JsonLayout->>JsonAttribute: RenderAppendJsonValue(logEvent, sb, valueStart)
    JsonAttribute-->>JsonLayout: success/failure via sb length
    opt Failure
      JsonLayout->>JsonLayout: Revert sb to original length
    end
  else Encode = true
    alt ValueType is null (string path)
      JsonLayout->>JsonAttribute: RenderAppendJsonValue(logEvent, sb, valueStart)
      JsonLayout->>Serializer: PerformJsonEscapeWhenNeeded(sb, startPos, escapeUnicode)
      Serializer-->>JsonLayout: Escaped (if needed)
      JsonLayout->>JsonLayout: Append quotes using valueStart
    else Object value present
      JsonLayout->>JsonLayout: Previous object handling preserved
      JsonLayout->>Serializer: PerformJsonEscapeWhenNeeded (if string-like)
    end
  end
  JsonLayout-->>Logger: JSON output (sb)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

A nibble of bytes, a hop through the night,
I trim the strings till they’re tidy and light.
Quotes in a row, escapes in a line,
I twitch my nose—now the JSON’s fine.
With helper in paw, I bound through the log,
Carrots for code, and a bug-free jog! 🥕🐇

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The pull request has no description text, so there is no contextual information or summary of the changes, leaving readers without guidance on the intent or scope of the refactor.	Please add a descriptive summary that outlines the purpose of the refactor, the key changes made to JSON rendering, and any impacts on existing functionality.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title succinctly captures the primary refactoring effort by indicating a reduction in code complexity for JSON layout attribute rendering and directly reflects the main focus of the changeset.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5df46fa and 77d891e.

📒 Files selected for processing (3)

src/NLog/Layouts/JSON/JsonAttribute.cs (1 hunks)
src/NLog/Layouts/JSON/JsonLayout.cs (2 hunks)
src/NLog/Targets/DefaultJsonSerializer.cs (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

src/NLog/Layouts/JSON/JsonAttribute.cs (2)

src/NLog/Targets/DefaultJsonSerializer.cs (8)

DefaultJsonSerializer (47-763)

DefaultJsonSerializer (70-73)

PerformJsonEscapeWhenNeeded (666-679)

SerializeObject (80-83)

SerializeObject (91-146)

SerializeObject (154-157)

SerializeObject (166-169)

SerializeObject (180-198)

src/NLog/Layouts/JSON/JsonLayout.cs (2)

SerializeObject (75-81)

Layout (50-484)

src/NLog/Layouts/JSON/JsonLayout.cs (2)

src/NLog/Targets/DefaultJsonSerializer.cs (3)

DefaultJsonSerializer (47-763)

DefaultJsonSerializer (70-73)

PerformJsonEscapeWhenNeeded (666-679)

src/NLog/Layouts/JSON/JsonAttribute.cs (5)

JsonAttribute (56-56)

JsonAttribute (63-63)

JsonAttribute (71-77)

RenderAppendJsonValue (163-192)

RenderAppendJsonValue (194-198)

🔇 Additional comments (7)

src/NLog/Targets/DefaultJsonSerializer.cs (2)

668-679: LGTM! Micro-optimization and simplified call site.

The local caching of builder.Length at line 668 is a valid micro-optimization since the length is accessed multiple times in the loop. The simplified call to AppendStringEscape (removing the Targets.DefaultJsonSerializer. prefix) is appropriate since we're already inside the class.

681-681: LGTM! Appropriate visibility restriction.

Narrowing the visibility from internal static to private static is a good practice as it reduces the API surface and clarifies that this method is only used within the class.

src/NLog/Layouts/JSON/JsonAttribute.cs (2)

165-181: LGTM! Well-structured refactoring with clear control flow.

The refactoring clearly separates three rendering paths:

No encoding: direct value rendering with empty-check

Encoding with no ValueType: string rendering with JSON escaping

ValueType specified: object serialization

The use of valueStart to track the beginning of content for escaping and empty-checks is a clean approach. The pattern where the method returns false on render failure and the caller handles rollback is consistent with the broader codebase (as seen in JsonLayout.cs).

194-198: LGTM! Effective helper method that reduces duplication.

The new helper centralizes the rendering logic and non-empty output check, eliminating code duplication between the encode/no-encode paths. The logic correctly honors the IncludeEmptyValue flag and determines whether meaningful content was rendered by comparing builder length to valueStart.

src/NLog/Layouts/JSON/JsonLayout.cs (3)

312-312: LGTM! Clearer condition for detecting the first property.

Using sb.Length == orgLength directly is more intuitive than the previous approach, making it immediately clear that we're checking if this is the first property being rendered.

453-455: LGTM! Simplified escaping logic.

Capturing valueStart after the opening quote and calling PerformJsonEscapeWhenNeeded directly is cleaner than the previous separate helper approach. The escaping correctly operates only on the value content, excluding the surrounding quotes.

464-472: LGTM! Consistent rollback pattern.

Changing from static to instance method aligns with the broader refactoring, and the rollback logic using initialLength correctly handles rendering failures by ensuring partial output is removed. This pattern is now consistent with how JsonAttribute.RenderAppendJsonValue is called.

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.}

sonarqubecloud · 2025-10-14T06:45:18Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

JsonLayout - Reduce code complexity for rendering JsonAttribute

77d891e

snakefoot added performance json / json-layout labels Oct 14, 2025

pull-request-size bot added the size/M label Oct 14, 2025

snakefoot merged commit 7b5c076 into NLog:dev Oct 14, 2025
5 of 6 checks passed

snakefoot added this to the 6.0.6 milestone Oct 24, 2025

coderabbitai bot mentioned this pull request Nov 8, 2025

Version 6.0.6 #6037

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JsonLayout - Reduce code complexity for rendering JsonAttribute#6026

JsonLayout - Reduce code complexity for rendering JsonAttribute#6026
snakefoot merged 1 commit intoNLog:devfrom
snakefoot:json_refactor

snakefoot commented Oct 14, 2025

Uh oh!

coderabbitai bot commented Oct 14, 2025 •

edited

Loading

Uh oh!

sonarqubecloud bot commented Oct 14, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

snakefoot commented Oct 14, 2025

Uh oh!

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

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

sonarqubecloud bot commented Oct 14, 2025

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai bot commented Oct 14, 2025 •

edited

Loading