-
Notifications
You must be signed in to change notification settings - Fork 678
Fix: Correct URI query parameter matching and class name typo (Fixes #596) #599
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
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.
Thank you @sheegansrigm
LGTM
…ers (#599) - Enhanced DefaultMcpUriTemplateManager to use Pattern.quote() for escaping special regex characters like '?' - Replaced simple string replacement regex generation with robust pattern building - Added test case to verify URI matching with query parameters (e.g., "file://name/search?={search}") - Fixed typo: renamed DeafaultMcpUriTemplateManagerFactory to DefaultMcpUriTemplateManagerFactory - Updated all references across server classes and tests Signed-off-by: Christian Tzolov <[email protected]>
…ers (#599) - Enhanced DefaultMcpUriTemplateManager to use Pattern.quote() for escaping special regex characters like '?' - Replaced simple string replacement regex generation with robust pattern building - Added test case to verify URI matching with query parameters (e.g., "file://name/search?={search}") - Fixed typo: renamed DeafaultMcpUriTemplateManagerFactory to DefaultMcpUriTemplateManagerFactory - Updated all references across server classes and tests Signed-off-by: Christian Tzolov <[email protected]>
Thanks for the review @tzolov |
Hi @tzolov I just pushed a new commit to address the CI build failure. It looks like my changes had some minor formatting violations that the spring-javaformat check caught. I've fixed this by running mvn spring-javaformat:apply as the log suggested. The automated checks should all pass now. |
@sheegansrigm I have tested this and it is working. Another issue I found with the query parameter is if it is not defined in the request, it will fail to match the template. According to the specs, if query parameter is undefined, then it should not be set. Example Template:
Failed URI match:
|
Motivation and Context
This pull request addresses two separate issues, with the primary goal of resolving issue #596.
Fixes: #596
URI Matching Bug: A critical bug was identified in
DefaultMcpUriTemplateManager.matches()
where it would fail to correctly match any URI template containing a query parameter (e.g.,file://name/search?={search}
). The root cause was the unescaped?
character being misinterpreted as a special character by the regex engine.Class Name Typo: Incorporates a pending change to correct a persistent spelling mistake in a core factory class name (
DefaultMcpUriTepmlateManagerFactory
).Description of Changes
Bug Fix: The
matches
method inDefaultMcpUriTemplateManager
has been rewritten to use a robust regex-building approach withPattern.quote()
. This ensures that all literal parts of the URI template, including special characters like?
, are properly escaped before the matching pattern is compiled. The logic now mirrors the safer implementation already used in theextractVariableValues
method.Typo Fix: The class
DefaultMcpUriTepmlateManagerFactory
has been renamed toDefaultMcpUriTemplateManagerFactory
, and all references to it have been updated throughout the codebase.How Has This Been Tested?
shouldMatchUriWithQueryParameters
andshouldExtractVariableValuesFromUriWithQueryParameters
) were added to specifically reproduce the URI matching bug.Breaking Changes
[x] Yes. Any user directly referencing the misspelled class
DefaultMcpUriTepmlateManagerFactory
will need to update their code to use the corrected class nameDefaultMcpUriTemplateManagerFactory
after pulling this change.Types of changes
Checklist
Additional context
This is a straightforward find-and-replace action to correct a typo. No other logic or functionality has been altered.