Fix reserved expansion (e.g. "{+var}") when matching resources #1142
Conversation
- Add more test cases
There was a problem hiding this comment.
Pull request overview
This pull request fixes URI template parsing for reserved expansion ({+var}), label expansion ({.var}), and path-style parameters ({;var}). The bug was causing templates like samples://{dependency}/{+path} to fail when matching URIs with nested paths.
Changes:
- Fixed reserved expansion to properly match slashes and other reserved characters per RFC 6570
- Fixed label expansion to handle dot-prefixed values with correct separator logic
- Added support for path-style parameter expansion with semicolon-prefixed name=value pairs
- Added 757 lines of comprehensive tests covering all URI template operators and edge cases
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/ModelContextProtocol.Core/UriTemplate.cs |
Fixed operator handling in CreateParser switch statement, added AppendPathParameterExpression method, updated separator logic for multi-value expansions, and made UriTemplate class public |
tests/ModelContextProtocol.Tests/UriTemplateCreateParserTests.cs |
Added comprehensive test suite with 757 lines covering all RFC 6570 operators, edge cases, and regression tests for the fixed bugs |
tests/ModelContextProtocol.Tests/Configuration/McpServerResourceRoutingTests.cs |
Added integration test for the reserved expansion bug fix with nested path matching |
|
|
||
| switch (m.Groups["operator"].Value) | ||
| { | ||
| case "+": AppendExpression(ref pattern, paramNames, null, "[^?&#]+"); break; |
There was a problem hiding this comment.
This new case is the primary fix for the "{+path}" issue reported to me by @heaths.
There was a problem hiding this comment.
I might not have enough context for the full call tree here, but an empty expansion should patch as well, so "*" instead of "+" in the regex. From https://modelcontextprotocol.io/specification/2025-03-26/server/resources#resource-templates, see the test cases in https://datatracker.ietf.org/doc/html/rfc6570#page-22 e.g., O{+empty}X matches OX.
| /// e.g. it may treat portions of invalid templates as literals rather than throwing. | ||
| /// </remarks> | ||
| internal static partial class UriTemplate | ||
| public static partial class UriTemplate |
There was a problem hiding this comment.
@stephentoub @eiriktsarpalis I made UriTemplate.CreateParser and UriTemplate.FormatUri public mostly for testing purposes (although our tests don't directly call FormatUri yet). I'm happy to change this to IVT or reflection. Or we could put all these tests cases through a more complicated harness similar to what I have in McpServerResourceRoutingTests.
Another option might be labeling the API experimental.
There was a problem hiding this comment.
Why can't we just extend the test cases at
I would like to avoid making these types public. ModelContextProtocol is not the library that should be exposing a UriTemplate implementation publicly.
There was a problem hiding this comment.
We can. This was just a little more straightforward. I understand not wanting to support this API going forward though.
I'll convert these tests cases over to McpServerResourceRoutingTests.
There was a problem hiding this comment.
Assuming the dependency graph is not too complex, couldn't we just link UriTemplate.cs in the test project and test there?
halter73
changed the title
@heaths