Repository navigation
Split comma-separated variables when matching resource URI templates - #1900
Laurianti wants to merge 2 commits into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused parser correction matches the reported behavior and is covered by representative regression tests.
Review effort: Balanced
Findings: None
Fixes resource URI parsing for comma-separated variables in simple, reserved, and fragment expressions.
Changes:
- Prevents greedy capture of commas in multi-variable expressions.
- Adds regression coverage for all affected expression types.
| File | Description |
|---|---|
| src/ModelContextProtocol.Core/UriTemplate.cs | Splits multi-variable expression values at commas. |
| tests/ModelContextProtocol.Tests/Server/McpServerResourceTests.cs | Verifies correct resource argument binding. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
|
There is an ambiguity here for the reserved (+) and fragment (#) operators that I think this matcher should handle explicitly rather than silently treating comma as always structural. RFC 6570 reserved expansion leaves reserved characters unescaped, and comma itself is a reserved character. So a value containing a literal comma is a valid input for {+lat,lon} / {#lat,lon}. For example, expanding values where the first variable contains a comma can legitimately produce a URI with more commas than variables. This PR changes both variables to a [^,]*-style match, which means literal commas can no longer belong to either captured value. That fixes the common 45.4,9.1 case, but it also turns some valid reserved/fragment expansions into non-matches (and reverse matching is inherently ambiguous once a value itself may contain the separator). Could we either:
A regression with a reserved value containing a literal comma would make the chosen contract explicit. AI-assisted review; checked the current PR head against RFC 6570 reserved/fragment expansion semantics before posting. |
Sorry, something went wrong.
|
Thanks. Option 2: the split stays, and the contract is now explicit in 358dd43. With more than one variable, a literal comma is the separator, so a comma inside a value has to be percent-encoded as %2C, which reserved and fragment expansion leave as is and the matcher decodes. A literal comma inside a value can't be told apart from the separator, so that URI does not match instead of being split wrongly. The comment in AppendExpression says so, and two tests pin it down: UriTemplate_CommaInValue_ReachesTheMethod and UriTemplate_LiteralCommaInMultiVariableValue_DoesNotMatch.
{#lat,lon} gives the same results. Option 1 would leave {+lat,lon} with 45.4,9.1 as it is on main. I also ran the matcher of main and of this PR against the matching cases of other implementations and the RFC 6570 examples:
The cases that still fail also fail on main and are outside this change, for example exploded lists and the order of query parameters. Note This reply was drafted with AI assistance and reviewed before posting. |
Sorry, something went wrong.
Closes #1896
When a simple, reserved or fragment expression holds more than one variable, UriTemplate.CreateParser now excludes the comma from the value character class, so each variable matches up to the next comma instead of the first one taking every value. Expressions with a single variable produce the same regex as before, and label expansion keeps its greedy matching.
Before, resource://coords/{lat,lon} matched resource://coords/45.4,9.1 with lat = "45.4,9.1" and lon = ""; now lat = "45.4" and lon = "9.1". The same applies to {+lat,lon} and {#lat,lon}.
With more than one variable a literal comma is always the separator, so a comma inside a value has to be percent-encoded (%2C): {+lat,lon} reads a%2Cb,c as lat = "a,b" and lon = "c", and does not match a,b,c, where the split can't be told. A reserved or fragment expression with a single variable keeps literal commas, as before.
The regression tests read a resource through each of the three expression kinds and check that both values reach the method, that an encoded comma stays in its value, that a literal comma inside a multi-variable reserved or fragment value does not match, and that a single reserved or fragment variable keeps literal commas.
Note
This pull request was drafted with AI assistance and reviewed before posting.