Skip to content

EventProperties - Format Dictionary Properties correctly #6054

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
Merged
snakefoot merged 1 commit into from
Nov 30, 2025
Merged

EventProperties - Format Dictionary Properties correctly #6054

snakefoot merged 1 commit into from
Nov 30, 2025

Conversation

@snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Nov 30, 2025 •
edited
Loading

Fix bug introduced with the change to nullable API. See also: #5809

@snakefoot snakefoot added the bug Bug report / Bug fix label Nov 30, 2025
@coderabbitai
Copy link

coderabbitai bot commented Nov 30, 2025 •
edited
Loading

Walkthrough

TryFormatToString now returns nullable string? and returns null for non-formattable, non-null values; several callers switch local declarations to var; Layout added a fallback to objectValue?.ToString() then empty string; a unit test for Dictionary event property was added.

Changes

Cohort / File(s) Summary
FormatHelper core change
src/NLog/Internal/FormatHelper.cs		 Method signature changed to return string?; removed IEnumerable branch and now returns null for non-formattable, non-null values; retains string.Empty for null input.
Layout renderers (type inference only)
src/NLog/LayoutRenderers/EventPropertiesLayoutRenderer.cs, src/NLog/LayoutRenderers/FuncLayoutRenderer.cs, src/NLog/LayoutRenderers/GdcLayoutRenderer.cs, src/NLog/LayoutRenderers/ScopeContextPropertyLayoutRenderer.cs		 Local variable declarations changed from explicit string to var when capturing TryFormatToString result; no behavior or control-flow changes.
Layout typed fallback handling
src/NLog/Layouts/Typed/Layout.cs		 GetFormattedMessage now falls back to objectValue?.ToString() and then string.Empty when TryFormatToString returns null.
Tests — event properties
tests/NLog.UnitTests/LayoutRenderers/EventPropertiesTests.cs		 Added using System.Collections.Generic; and new test TestDictionaryProperty asserting dictionary event property renders as key1=value1.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Areas to review closely:
    • src/NLog/Internal/FormatHelper.cs — verify removal of IEnumerable handling and null-return semantics are intentional.
    • All callers of TryFormatToString — confirm they properly handle nullable returns (including the new fallback in Layout.cs).
    • tests/NLog.UnitTests/LayoutRenderers/EventPropertiesTests.cs — ensure the added test reflects intended rendering for complex types.

Poem

🐇 I hopped through types and found a null,
I left the lists, kept logic full.
A fallback nibble, a dictionary cheer,
Tiny changes, but the path is clear.
Hop on, code — let's run without fear! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main focus of the changeset: formatting Dictionary properties in EventProperties layout renderer, which is the core objective of this bug fix.
Description check ✅ Passed The PR description references a specific issue (#5809) and clearly indicates this is a bug fix related to a nullable API change, which aligns with the changeset's modifications to method return types and null handling.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

@snakefoot snakefoot mentioned this pull request Nov 30, 2025
Merged
coderabbitai[bot]
coderabbitai bot reviewed Nov 30, 2025
View reviewed changes
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e37aabb and c43e61f.

📒 Files selected for processing (7)
  • src/NLog/Internal/FormatHelper.cs (2 hunks)
  • src/NLog/LayoutRenderers/EventPropertiesLayoutRenderer.cs (1 hunks)
  • src/NLog/LayoutRenderers/FuncLayoutRenderer.cs (1 hunks)
  • src/NLog/LayoutRenderers/GdcLayoutRenderer.cs (1 hunks)
  • src/NLog/LayoutRenderers/ScopeContextPropertyLayoutRenderer.cs (1 hunks)
  • src/NLog/Layouts/Typed/Layout.cs (1 hunks)
  • tests/NLog.UnitTests/LayoutRenderers/EventPropertiesTests.cs (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/NLog/LayoutRenderers/ScopeContextPropertyLayoutRenderer.cs (1)
src/NLog/Internal/FormatHelper.cs (2)
  • FormatHelper (38-87)
  • TryFormatToString (68-86)
src/NLog/LayoutRenderers/EventPropertiesLayoutRenderer.cs (1)
src/NLog/Internal/FormatHelper.cs (2)
  • FormatHelper (38-87)
  • TryFormatToString (68-86)
src/NLog/LayoutRenderers/FuncLayoutRenderer.cs (1)
src/NLog/Internal/FormatHelper.cs (2)
  • FormatHelper (38-87)
  • TryFormatToString (68-86)
src/NLog/Layouts/Typed/Layout.cs (1)
src/NLog/Internal/FormatHelper.cs (2)
  • FormatHelper (38-87)
  • TryFormatToString (68-86)
src/NLog/LayoutRenderers/GdcLayoutRenderer.cs (1)
src/NLog/Internal/FormatHelper.cs (2)
  • FormatHelper (38-87)
  • TryFormatToString (68-86)
src/NLog/Internal/FormatHelper.cs (2)
src/NLog/LayoutRenderers/LayoutRenderer.cs (1)
  • IFormatProvider (218-221)
src/NLog/Internal/ExceptionMessageFormatProvider.cs (1)
  • IFormatProvider (80-83)
🔇 Additional comments (7)
src/NLog/LayoutRenderers/GdcLayoutRenderer.cs (1)

106-106: LGTM - Variable declaration updated for nullable return type.

The change from explicit string to var accommodates the nullable string? return type from TryFormatToString and maintains consistency with similar changes across other layout renderers.

src/NLog/LayoutRenderers/EventPropertiesLayoutRenderer.cs (1)

142-142: LGTM - Consistent with nullable return type handling.

The variable declaration change from explicit string to var properly handles the nullable string? returned by TryFormatToString.

src/NLog/Layouts/Typed/Layout.cs (1)

193-193: Essential fallback logic for non-formattable types.

The added fallback chain (?? objectValue?.ToString() ?? string.Empty) correctly handles the scenario where TryFormatToString returns null for complex objects that aren't IFormattable or IConvertible (such as Dictionary<string, string>). This ensures such objects still render via their ToString() implementation rather than being silently omitted.

src/NLog/LayoutRenderers/FuncLayoutRenderer.cs (1)

109-109: LGTM - Consistent refactor across layout renderers.

Type inference usage aligns with the updated TryFormatToString returning string? and maintains consistency with other layout renderer changes in this PR.

src/NLog/LayoutRenderers/ScopeContextPropertyLayoutRenderer.cs (1)

96-96: LGTM - Aligned with nullable return type.

The change to var is consistent with similar updates across all layout renderers and correctly handles the nullable string? return type.

tests/NLog.UnitTests/LayoutRenderers/EventPropertiesTests.cs (1)

86-98: Good test coverage for dictionary property rendering.

The test correctly validates that Dictionary<string, string> properties render as key=value pairs when using the event-properties layout renderer, providing coverage for the bug fix where TryFormatToString returns null for non-formattable complex types.

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

68-86: Core fix: Return null for non-formattable objects to signal complex formatting requirement.

The updated logic correctly returns null for objects that are neither IFormattable nor IConvertible, with IFormattable and IConvertible handled directly, and null inputs returning string.Empty. Verify that all callers of TryFormatToString throughout the codebase properly handle the nullable return type and that Layout.cs line 193 provides appropriate fallback handling.

@snakefoot snakefoot force-pushed the StringValueRendererFix branch from c43e61f to d143227 Compare November 30, 2025 17:26
@snakefoot snakefoot closed this Nov 30, 2025
@snakefoot snakefoot reopened this Nov 30, 2025
@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 30, 2025

Quality Gate Passed 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

@snakefoot snakefoot merged commit aa5566c into NLog:dev Nov 30, 2025
5 of 6 checks passed
@snakefoot snakefoot added this to the 6.0.7 milestone Nov 30, 2025
@snakefoot snakefoot changed the title EventProperties - Format Dictionary Properties EventProperties - Format Dictionary Properties correctly Dec 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

Assignees

No one assigned

Labels

bug Bug report / Bug fix size/M

Projects

None yet

Milestone

6.0.7

Development

Successfully merging this pull request may close these issues.

1 participant

@snakefoot