ramu11 commented on PR #29722:
URL: https://github.com/apache/camel/pull/29722#issuecomment-6845466098
> Thanks @ramu11, this is a solid piece of work. The matcher is small and
easy to follow: literals are `Pattern.quote`d, variables must fill a whole
segment, and the parser rejects query and fragment expressions. Checked against
the MCP Java SDK 2.1.0 `DefaultMcpUriTemplateManager`: both accept the same
URIs for whole-segment templates, so the bridge's overlap refusal really does
give one template per URI. Fixed-before-template precedence and not-found for
unmatched URIs come out right, and the explicit `notifyResourcesListChanged()`
after `removeResourceTemplate` is a good catch (the SDK does not send it
there). The conformance tests cover listing, reads, precedence, unmatched URIs
and lifecycle nicely.
>
> A few things to address in this PR:
>
> * **Generated DSL:** `uriTemplate` is new and `resourceUri` is no longer
required, but `AiResourceEndpointBuilderFactory` (still "Required: true" at
line 89, no `uriTemplate`) and `AiResourceComponentBuilderFactory` were not
regenerated. CI's uncommitted-changes check will likely flag them.
> * **mcp-server docs:** `mcp-server.adoc` line 279 still says "Stdio
exposes tools and prompts", but this PR also makes stdio serve fixed resources
(worth a line in the PR description too). Could you also document templates in
the Resources section (~line 124) and `supportsResourceTemplates()` in the SPI
paragraph (~line 65), and regenerate the catalog copy? `design/aiResource.adoc`
still lists templates as a non-goal and planned a separate opt-in conformance
class for them; a short update there would help.
> * **Variable matching:** `AiResourceUriTemplate.java:113` matches
`([^/]+)`, so `crm://customers/42?x=1` yields `id=42?x=1`, and `.`, `..` and
control characters match too. A correctly expanded RFC 6570 level-1 URI would
have percent-encoded those. What do you think about restricting the variable to
RFC 3986 pchar characters and rejecting `.`/`..` segments? The docs warning is
good, but a safer default here protects route authors.
> * **Headers in the catalog:** `CamelAiResourceUri` and the
`CamelAiResourceTemplateVariable.` prefix (`AiResource.java:31`, `:34`) need
`@Metadata`, plus `headersClass` on `@UriEndpoint`, so they show up in the
generated docs as `design/headers.adoc` asks. The `@BodyTypes` `consumerOut`
text at `AiResourceEndpoint.java:49` ("No body and no headers") no longer holds
for template reads.
> * **Imports (CLAUDE.md rule):** fully qualified
`io.modelcontextprotocol.spec.McpError` in `StdioMcpServerEngine.java:284` and
`:350`, and `java.util.Map.of` in `AiResourceEndpointLifecycleTest.java:65`,
`AiResourceRegistryTest.java:152` and `StdioMcpServerEngineTest.java:304`;
please import them.
>
> Smaller things, optional:
>
> * `putTemplate` accepts two routes with an identical `uriTemplate` under
the same tag, while `put` fails startup for a duplicate fixed URI. Should
templates behave the same? Also, the Javadoc of `putDefaultTemplate` (line 202)
says it is checked against every tagged pool, but nothing is checked.
> * The resource and template read handlers are copied into both engines; a
small shared helper like `McpPromptSdkSupport` would keep them in sync. The
stdio `resourceAdded` also drops `title`.
> * The re-evaluation in `unpublishResourceTemplate`
(`McpServerBridge.java:455`) logs the overlap ERROR again for every
still-refused template on each template-route stop.
> * [Question] Keeping percent-encoding raw is the safer choice, but ai-tool
and ai-prompt arguments arrive decoded. Is raw the contract we want for route
authors?
> * A few more matcher tests would pin the behaviour: query or fragment in
the request URI, `..` segments, identical templates, and an invalid
`uriTemplate` failing route start.
>
> _Claude Code on behalf of davsclaus. This review was generated by an AI
agent and may contain inaccuracies; please verify all suggestions before
applying._
Thanks @davsclaus for the detailed review and for checking the matcher
against MCP Java SDK 2.1.0. I appreciate the thorough feedback.
I've addressed the main points in the latest commits:
1. **Generated DSL:** Regenerated `AiResourceEndpointBuilderFactory` and
`AiResourceComponentBuilderFactory`. The new `uriTemplate` option is included,
and `resourceUri` is no longer marked as required.
2. **Documentation:** Updated the MCP server documentation with
resource-template usage and the `supportsResourceTemplates()` SPI capability.
Also corrected the stdio description to include resources and templates,
regenerated the catalog documentation, and updated `design/aiResource.adoc` to
include URI templates in the implemented scope.
3. **Variable matching and security:** Restricted template variables to RFC
3986 `pchar` characters and valid `%HH` escapes. Query strings, fragments,
control characters, malformed percent escapes, and `.` / `..` segments are
rejected. Percent-encoded values are intentionally preserved rather than
decoded, so an encoded slash remains within the original path segment. Added
matcher regression tests for these cases.
I've also hardened the error paths: `AiResourceExecutor` now uses the
stable resource name in `ExecutionError` messages instead of including the
concrete client-supplied URI. The server-side read-failure logging also uses
stable resource identifiers.
4. **Headers and body metadata:** Added `@Metadata` to `CamelAiResourceUri`
and `CamelAiResourceTemplateVariable.`, configured `headersClass =
AiResource.class`, and updated the `@BodyTypes` description to document the
headers supplied for template reads.
5. **Imports:** Replaced the fully qualified `McpError` and `Map.of`
references with imports, following the project convention.
Regarding the smaller points:
- **Duplicate templates:** Exact duplicate templates are now rejected during
registration, and overlapping tagged templates are rejected rather than relying
on registration order. The default untagged pool remains separate because it is
never exposed by the MCP bridge. I've also corrected the `putDefaultTemplate`
Javadoc to reflect that behavior.
- **Stdio resource title:** The stdio engine now preserves the resource
title when building the MCP resource.
- **Shared read-handler helper:** I left the engine-specific handler
implementations separate in this PR to avoid expanding the scope with an
additional refactoring.
- **Repeated overlap logging:** The bridge re-evaluates remaining templates
after a removal so a previously refused template can become publishable. The
possibility of repeated conflict logs during that re-evaluation remains a minor
logging improvement.
- **Percent-encoding contract:** Values intentionally remain raw, including
percent-encoded octets. This is documented for route authors; decoding could
change path-segment semantics, particularly for encoded `/` characters.
The focused `AiResourceExecutorTest` passes after the latest error-message
changes: 9 tests, zero failures and zero errors. The earlier affected-module
verification also completed successfully.
Thanks again for the careful review and useful suggestions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
