jiriOndrusek-agent commented on code in PR #9368:
URL: https://github.com/apache/camel-quarkus/pull/9368#discussion_r4359049082
##########
extensions/mcp-server/runtime/src/main/java/org/apache/camel/quarkus/component/mcp/server/QuarkusMcpServerEngine.java:
##########
@@ -107,6 +123,59 @@ public void toolRemoved(String toolName) {
}
}
+ @Override
+ public void resourceAdded(McpServerResource resource) {
+ ResourceManager.ResourceDefinition definition =
resourceManager.newResource(resource.name())
+ .setUri(resource.uri())
+ .setMimeType(resource.mimeType())
+ .setDescription(resource.description());
+ if (resource.title() != null && !resource.title().isBlank()) {
+ definition.setTitle(resource.title());
+ }
+ definition.setHandler(arguments -> {
+ McpResourceReadResult result = resource.handler().read();
+ if (result.isError()) {
+ // resources/read has no in-band error flag; the message is
pre-sanitized by the bridge
+ throw new McpException(result.errorMessage(),
JsonRpcErrorCodes.INTERNAL_ERROR);
+ }
+ // plain Base64 as in the Vert.x engine:
BlobResourceContents.create(String, byte[]) adds MIME line breaks
+ ResourceContents contents = result.blob() != null
+ ? new BlobResourceContents(resource.uri(),
Base64.getEncoder().encodeToString(result.blob()),
+ resource.mimeType())
+ : new TextResourceContents(resource.uri(), result.text(),
resource.mimeType());
+ return new ResourceResponse(List.of(contents));
+ });
+ try {
+ definition.register();
+ } catch (IllegalArgumentException e) {
+ // quarkus-mcp-server also requires unique names: refuse the
resource, like the bridge does on a uri clash
+ LOG.error("Refusing to expose MCP resource '{}': {}. Resource
names and uris must be unique.",
+ resource.uri(), e.getMessage());
+ return;
+ }
+ registeredResourceUris.add(resource.uri());
+ LOG.debug("MCP resource added: {}", resource.uri());
+ }
+
+ @Override
+ public void resourceRemoved(String resourceUri) {
+ if (!registeredResourceUris.remove(resourceUri)) {
+ // refused in resourceAdded: removing by uri would delete another
resource with the same uri
+ return;
+ }
+ try {
+ resourceManager.removeResource(resourceUri);
Review Comment:
Good catch, thanks. It now calls `removeResource(uri, McpServer.DEFAULT)`,
mirroring the registration (71fbb8776c). The new
`testStoppingResourceRouteKeepsResourceOfOtherServer` registers a resource with
the same uri on a named server; it failed before the fix.
_Generated with [Claude Code](https://claude.com/claude-code) on behalf of
@JiriOndrusek_
##########
extensions/mcp-server/runtime/src/main/java/org/apache/camel/quarkus/component/mcp/server/QuarkusMcpServerEngine.java:
##########
@@ -107,6 +123,59 @@ public void toolRemoved(String toolName) {
}
}
+ @Override
+ public void resourceAdded(McpServerResource resource) {
+ ResourceManager.ResourceDefinition definition =
resourceManager.newResource(resource.name())
+ .setUri(resource.uri())
+ .setMimeType(resource.mimeType())
+ .setDescription(resource.description());
+ if (resource.title() != null && !resource.title().isBlank()) {
+ definition.setTitle(resource.title());
+ }
+ definition.setHandler(arguments -> {
+ McpResourceReadResult result = resource.handler().read();
+ if (result.isError()) {
+ // resources/read has no in-band error flag; the message is
pre-sanitized by the bridge
+ throw new McpException(result.errorMessage(),
JsonRpcErrorCodes.INTERNAL_ERROR);
+ }
+ // plain Base64 as in the Vert.x engine:
BlobResourceContents.create(String, byte[]) adds MIME line breaks
+ ResourceContents contents = result.blob() != null
+ ? new BlobResourceContents(resource.uri(),
Base64.getEncoder().encodeToString(result.blob()),
+ resource.mimeType())
+ : new TextResourceContents(resource.uri(), result.text(),
resource.mimeType());
+ return new ResourceResponse(List.of(contents));
+ });
+ try {
+ definition.register();
+ } catch (IllegalArgumentException e) {
Review Comment:
Agreed on the analysis, and that the root fix belongs in Camel:
`McpServerBridge.publishResource` stores the spec before the void
`resourceAdded`, so the engine cannot report the refusal. The catch here keeps
the app starting, as auto-started routes are published from the bridge's start
snapshot; point 2 is also how the bridge treats its own refusals. I'd keep the
refusal for Camel 4.23. I opened
[CAMEL-26102](https://issues.apache.org/jira/browse/CAMEL-26102) to let the SPI
report whether a tool or resource was accepted.
_Generated with [Claude Code](https://claude.com/claude-code) on behalf of
@JiriOndrusek_
##########
extensions/mcp-server/runtime/src/main/java/org/apache/camel/quarkus/component/mcp/server/CamelMcpServerConfig.java:
##########
@@ -55,4 +57,13 @@ public interface CamelMcpServerConfig {
*/
@WithDefault("20000")
long toolTimeout();
+
+ /**
+ * Per-read resource execution timeout in milliseconds. A read exceeding
the timeout returns an error to the MCP
+ * client; the underlying route keeps running until it completes on its
own.
+ *
+ * @asciidoclet
+ */
+ @WithDefault("20000")
+ long resourceTimeout();
Review Comment:
Agreed. `resource-timeout`, and `tool-timeout` with the same issue, moved to
a new `RUN_TIME` root, `CamelMcpServerRunTimeConfig`, read by the recorder at
runtime (71fbb8776c). Checked on the built app:
`-Dquarkus.camel.mcp-server.resource-timeout=1` now takes effect.
_Generated with [Claude Code](https://claude.com/claude-code) on behalf of
@JiriOndrusek_
##########
integration-tests/mcp-server/src/main/java/org/apache/camel/quarkus/component/mcp/server/it/McpServerRoutes.java:
##########
@@ -69,5 +75,63 @@ public void configure() {
+
"&outputParameter.temperature=number&outputParameter.temperature.required=true"
+
"&outputParameter.unit=string&outputParameter.unit.enum=celsius,fahrenheit")
.setBody(constant("{\"temperature\":21.5,\"unit\":\"celsius\"}"));
+
+
from("ai-resource:app_config?resourceUri=camel:///config/app.json&tags=conformance"
+ + "&description=Application
configuration&mimeType=application/json&title=App config")
+ .routeId("app-config-route")
+ .setBody(constant("{\"env\":\"test\"}"));
+
+
from("ai-resource:latest_report?resourceUri=camel:///reports/latest.pdf&tags=conformance"
+ + "&description=Latest report&mimeType=application/pdf")
+ .setBody(constant(new byte[] { 0x25, 0x50, 0x44, 0x46 }));
+
+ // an InputStream body, as returned by producers such as aws2-s3
getObject; the file is opened on every read
+
from("ai-resource:notes?resourceUri=camel:///notes.txt&tags=conformance"
+ + "&description=Notes read from a file&mimeType=text/plain")
+ .process(e ->
e.getMessage().setBody(Files.newInputStream(Paths.get(NOTES_DIR,
"notes.txt"))));
+
+
from("ai-resource:latest_order?resourceUri=camel:///orders/latest.json&tags=conformance"
+ + "&description=Latest order&mimeType=application/json")
+ .setBody(constant(new Order("A-1", 2)))
+ .marshal().json(JsonLibrary.Jackson);
+
+
from("ai-resource:fail_resource?resourceUri=camel:///fail&tags=conformance&description=Always
fails")
+ .process(e -> {
+ throw new IllegalStateException("secret internal detail");
+ });
+
+
from("ai-resource:slow_resource?resourceUri=camel:///slow&tags=conformance"
+ + "&description=Exceeds the resource timeout")
+ .delay(6000)
+ .setBody(constant("done"));
+
+ // between tool-timeout and resource-timeout, like slow_tool
+
from("ai-resource:delayed_resource?resourceUri=camel:///delayed&tags=conformance"
+ + "&description=Exceeds the tool timeout, not the resource
timeout")
+ .delay(3000)
Review Comment:
Agreed. The within-timeout delay must stay above `tool-timeout` to catch a
mix-up of the two options, so I raised `resource-timeout` instead: 2000/6000
ms, with delays 4000 (`slow_tool`), 3000 (`delayed_resource`) and 8000
(`slow_resource`), so 2 s or more to each timeout a test relies on
(71fbb8776c). Swapping the two options still fails both tests.
_Generated with [Claude Code](https://claude.com/claude-code) on behalf of
@JiriOndrusek_
##########
integration-tests/mcp-server/src/test/java/org/apache/camel/quarkus/component/mcp/server/it/McpServerTest.java:
##########
@@ -216,31 +224,207 @@ void testToolsListReflectsRouteStopAndStart() {
.toolsList(page ->
assertThat(toolNames(page)).contains("say_hello"))
.thenAssertResults();
- controlRoute("stop");
- assertThat(routeStatus()).isEqualTo("Stopped");
+ controlRoute("say-hello-route", "stop");
+ assertThat(routeStatus("say-hello-route")).isEqualTo("Stopped");
await().atMost(10, TimeUnit.SECONDS).untilAsserted(() ->
client().when()
.toolsList(page ->
assertThat(toolNames(page)).doesNotContain("say_hello"))
.thenAssertResults());
- controlRoute("start");
- assertThat(routeStatus()).isEqualTo("Started");
+ controlRoute("say-hello-route", "start");
+ assertThat(routeStatus("say-hello-route")).isEqualTo("Started");
await().atMost(10, TimeUnit.SECONDS).untilAsserted(() ->
client().when()
.toolsList(page ->
assertThat(toolNames(page)).contains("say_hello"))
.thenAssertResults());
}
- private static void controlRoute(String action) {
+ // The resource scenarios below mirror
McpServerResourceConformanceTestSupport from camel-mcp-server-api
+
+ @Test
+ void testServerAdvertisesResourcesCapability() {
+
assertThat(client().initResult().capabilities()).extracting(ServerCapability::name).contains("resources");
+ }
+
+ @Test
+ void testServerAdvertisesListChangedForToolsAndResources() {
+ // both lists follow the route lifecycle
+ assertThat(client().initResult().capabilities())
+ .filteredOn(capability -> List.of("tools",
"resources").contains(capability.name()))
+ .hasSize(2)
+ .allSatisfy(capability ->
assertThat(capability.properties()).containsEntry("listChanged", true));
+ }
+
+ @Test
+ void testListResourcesExposesOnlySelectedTags() {
+ client().when()
+ .resourcesList(page -> {
+ assertThat(resourceUris(page))
+ .contains("camel:///config/app.json",
"camel:///reports/latest.pdf", "camel:///fail",
+ "camel:///slow")
+ .doesNotContain("camel:///hidden",
"camel:///other");
+
+ ResourceInfo config =
page.findByUri("camel:///config/app.json");
+ assertThat(config.name()).isEqualTo("app_config");
+ assertThat(config.title()).isEqualTo("App config");
+ assertThat(config.description()).isEqualTo("Application
configuration");
+
assertThat(config.mimeType()).isEqualTo("application/json");
+ })
+ .thenAssertResults();
+ }
+
+ @Test
+ void testReadTextResource() {
+ client().when()
+ .resourcesRead("camel:///config/app.json", response -> {
+ assertThat(response.contents()).hasSize(1);
+ TextResourceContents contents =
response.contents().get(0).asText();
+
assertThat(contents.uri()).isEqualTo("camel:///config/app.json");
+
assertThat(contents.mimeType()).isEqualTo("application/json");
+
assertThat(contents.text()).isEqualTo("{\"env\":\"test\"}");
+ })
+ .thenAssertResults();
+ }
+
+ @Test
+ void testReadBinaryResource() {
+ client().when()
+ .resourcesRead("camel:///reports/latest.pdf", response -> {
+ assertThat(response.contents()).hasSize(1);
+ BlobResourceContents contents =
response.contents().get(0).asBlob();
+
assertThat(contents.mimeType()).isEqualTo("application/pdf");
+
assertThat(Base64.getDecoder().decode(contents.blob())).isEqualTo(new byte[] {
0x25, 0x50, 0x44, 0x46 });
Review Comment:
Agreed, thanks. The payload is now 74 bytes, so a MIME encoder would add a
line break, which `Base64.getDecoder()` rejects (71fbb8776c). With the engine
switched to the MIME encoder, the test now fails.
_Generated with [Claude Code](https://claude.com/claude-code) on behalf of
@JiriOndrusek_
--
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]
