-
Notifications
You must be signed in to change notification settings - Fork 601
Fix reserved expansion (e.g. "{+var}") when matching resources #1142
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.
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
base: main
Are you sure you want to change the base?
Conversation
- Add more test cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
| pattern.AppendFormatted("(?:;"); | ||
| pattern.AppendFormatted(paramName); | ||
| pattern.AppendFormatted("(?:=(?<"); | ||
| pattern.AppendFormatted(paramName); | ||
| pattern.AppendFormatted(">[^;/?&]+))?)?"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These can all be AppendLiteral
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Should I make the same change to the AppendParameter inside of AppendQueryExpression?
@heaths