Replace static parameters in route pattern for telemetry

[tomerqodo]

Benchmark PR from qodo-benchmark#20

Summary by CodeRabbit

• Refactor

  • Improved route pattern diagnostic output formatting with centralized formatter logic.
  • Enhanced constraint representation in debug strings, including better regex constraint display.
  • Optimized required value substitution in route pattern debug information.

• Tests

  • Added comprehensive unit tests for route pattern debug string formatting.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

This PR refactors route pattern debug string formatting by extracting logic from RoutePattern into a new centralized RoutePatternDebugStringFormatter class. Additionally, constraint rendering is enhanced to properly format regex constraints, and comprehensive tests are added to validate the formatter behavior.

Changes

Cohort / File(s)	Summary
Route Pattern Debug Formatting src/Http/Routing/src/Patterns/RoutePattern.cs, src/Http/Routing/src/Patterns/RoutePatternDebugStringFormatter.cs	Removed LINQ dependency and delegated DebuggerToString to new RoutePatternDebugStringFormatter class. Formatter handles required value substitution, segment joining, literal preservation, and slash normalization with multi-segment path support.
Constraint Rendering Enhancement src/Http/Routing/src/Patterns/RoutePatternParameterPart.cs	Enhanced constraint rendering logic to handle regex constraints specially (rendering as "regex(...)"), prioritize constraint.Content over ParameterPolicy, and modernized null-checking syntax.
Formatter Unit Tests src/Http/Routing/test/UnitTests/Patterns/RoutePatternDebugStringFormatterTest.cs	Added 12 test methods covering required value replacement, catch-all parameters, complex segments, literal preservation, constraint rendering, and theory-based comprehensive validation with theory test and parsing helper.
Test Assertion Update src/Mvc/Mvc.Core/test/ApplicationModels/EndpointMetadataProviderTest.cs	Updated assertion to expect new debug string format "/Test/ActionWithParameterMetadata/{id?}" for IRouteDiagnosticsMetadata.

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 Hop, hop! New formatters now align,
Debug strings rendered, constraints divine!
Regex routes styled in regex array,
Refactored cleanly in every way,
Tests verify each code review play! 🐇

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description is minimal and incomplete. It only references a benchmark PR without providing substantive details about the changes, rationale, or impact.	Add a detailed description explaining the changes to route pattern formatting, why they improve telemetry, and how the new formatter works. Include the specific issue number this fixes.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Replace static parameters in route pattern for telemetry' accurately describes the main change: refactoring route pattern formatting to support replacing static parameter values in debug strings for telemetry purposes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

🧹 Recent nitpick comments

src/Http/Routing/test/UnitTests/Patterns/RoutePatternDebugStringFormatterTest.cs (1)

231-242: Consider adding bounds check in test helper.

The ParseRequiredValues helper assumes well-formed input. If pair.IndexOf('=') returns -1 (no = found), Substring(0, -1) would throw. While all current test data is valid, consider a defensive check for test robustness.

🔧 Optional: Add defensive check

 private static RouteValueDictionary ParseRequiredValues(string requiredValuesText)
 {
     var requiredValues = new RouteValueDictionary();
     foreach (var pair in requiredValuesText.Split(','))
     {
         var eqIndex = pair.IndexOf('=');
+        if (eqIndex < 0)
+        {
+            throw new ArgumentException($"Invalid required value format: '{pair}'");
+        }
         var key = pair.Substring(0, eqIndex);
         var value = pair.Substring(eqIndex + 1);
         requiredValues[key] = value;
     }
     return requiredValues;
 }

---

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4718204 and 4404a33.

📒 Files selected for processing (5)

• src/Http/Routing/src/Patterns/RoutePattern.cs
• src/Http/Routing/src/Patterns/RoutePatternDebugStringFormatter.cs
• src/Http/Routing/src/Patterns/RoutePatternParameterPart.cs
• src/Http/Routing/test/UnitTests/Patterns/RoutePatternDebugStringFormatterTest.cs
• src/Mvc/Mvc.Core/test/ApplicationModels/EndpointMetadataProviderTest.cs

🧰 Additional context used

🧬 Code graph analysis (2)

src/Mvc/Mvc.Core/test/ApplicationModels/EndpointMetadataProviderTest.cs (1)

src/Shared/Http2cat/Http2Utilities.cs (1)

• Assert (1003-1044)

src/Http/Routing/src/Patterns/RoutePattern.cs (2)

src/Http/Routing/src/Patterns/RoutePatternParameterPart.cs (1)

• DebuggerToString (83-131)

src/Http/Routing/src/Patterns/RoutePatternDebugStringFormatter.cs (2)

• RoutePatternDebugStringFormatter (8-103)
• Format (13-45)

🔇 Additional comments (12)

src/Mvc/Mvc.Core/test/ApplicationModels/EndpointMetadataProviderTest.cs (1)

129-129: LGTM!

The updated assertion correctly reflects the new RoutePatternDebugStringFormatter behavior, which replaces route parameters with their required values. The route /Test/ActionWithParameterMetadata/{id?} is the expected output when the formatter substitutes {controller} → Test and {action} → ActionWithParameterMetadata from the action descriptor, while preserving the unmatched {id?} parameter.

src/Http/Routing/src/Patterns/RoutePattern.cs (1)

154-158: LGTM!

Clean refactoring that extracts the debug string formatting logic into a dedicated RoutePatternDebugStringFormatter class. This follows the Single Responsibility Principle and centralizes the formatting logic that now handles required value substitution. The comment accurately documents the three usage contexts for this method.

src/Http/Routing/src/Patterns/RoutePatternParameterPart.cs (2)

118-122: LGTM!

The change from Default != null to Default is not null is a consistent stylistic update using modern C# pattern matching syntax. Semantically equivalent behavior.

---

99-116: This edge case cannot occur. The RoutePatternParameterPolicyReference class has two constructors: one accepts a string content parameter, the other accepts an IParameterPolicy. Each constructor sets exactly one property while leaving the other null. By design, every instance has either Content or ParameterPolicy set—never both null. The code's if/else chain is therefore guaranteed to append something after the colon in every iteration.

Likely an incorrect or invalid review comment.

src/Http/Routing/src/Patterns/RoutePatternDebugStringFormatter.cs (4)

13-45: LGTM - Well-structured formatting logic.

The Format method handles key edge cases appropriately:

• Fast path for patterns without matching required values
• Leading slash preservation from RawText
• Empty result normalization to "/"

The implementation is clean and readable.

---

47-63: LGTM!

Efficient helper that short-circuits as soon as a matching required value is found. Reuses TryGetRequiredValue for consistent validation logic.

---

65-88: LGTM!

The segment handling correctly distinguishes between:

1. Simple segments with a single parameter → direct substitution or fallback to DebuggerToString()
2. Complex segments or simple literal segments → iterates parts and substitutes parameters with required values

The fallthrough for simple literal segments (where Parts[0] is not a RoutePatternParameterPart) to the complex segment path is intentional and correct.

---

90-102: LGTM!

Clean implementation with proper [NotNullWhen(true)] annotation. The validation correctly rejects:

• Missing keys
• RequiredValueAny sentinel values
• Null values
• Empty strings (via Length: > 0 check)

src/Http/Routing/test/UnitTests/Patterns/RoutePatternDebugStringFormatterTest.cs (4)

10-25: LGTM!

Good foundational test case validating the core behavior of replacing matching required values while preserving optional parameters.

---

44-105: LGTM!

Excellent edge case coverage testing that parameters are preserved when required values are:

• RoutePattern.RequiredValueAny (sentinel)
• null
• Empty string ""

These tests align with the validation logic in TryGetRequiredValue.

---

175-195: LGTM!

Excellent test for the complex constraint rendering behavior. The comments on lines 190-193 clearly document the expected behavior:

• Constraints from parameterPolicies come first, then inline constraints
• RegexRouteConstraint renders as regex(pattern) format
• String constraints are converted to regex patterns like ^(fizz)$

---

197-229: LGTM!

Comprehensive theory-based test covering a wide range of scenarios:

• Default values and constraints
• Single/double star catch-all parameters ({*path}, {**path})
• Leading slash preservation
• Empty templates and root paths
• Partial replacements and complex segments

The inline data provides excellent coverage matrix.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

---

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