jamesnetherton commented on code in PR #9368:
URL: https://github.com/apache/camel-quarkus/pull/9368#discussion_r4334785534
##########
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:
`ResourceManager.removeResource(String uri)` is the server-agnostic
overload: it removes every non-method resource with that URI across all server
configurations, not just the one this engine registered on the default server.
If an application uses a named server (`quarkus.mcp.server."other".*`) and
registers a resource with the same URI there, stopping the `ai-resource` route
also removes the application's resource. This contradicts the claim that the
engine removes only resources it registered. Suggest `removeResource(uri,
<default server name>)` to mirror how the resource was registered.
##########
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:
`resourceTimeout` is a timeout but lives in the `BUILD_AND_RUN_TIME_FIXED`
config root, so it cannot be tuned on a built application. It is only consumed
by a `RUNTIME_INIT` recorder, so nothing needs it at build time. Could it move
to a runtime config root?
##########
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:
`McpServerBridge` puts the spec into `publishedResources` before calling
`engine.resourceAdded`, and `resourceAdded` is `void`, so a resource refused
here is still tracked as published by the bridge. Consequences:
1. A later `ai-resource` route with the refused URI is rejected with
"collides with an already exposed resource", although nothing actually serves
that URI.
2. When the route holding the clashing name stops, the refused resource is
never retried, so it stays unexposed even though the name is now free.
The root fix probably belongs in Camel: let the SPI report whether the
resource was accepted, or enforce name uniqueness in the bridge, rather than a
Quarkus-only refusal here.
##########
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:
With a 4-byte payload the Base64 output never reaches the 76-char line
length, so this test would still pass if the engine switched to the MIME
encoder (`BlobResourceContents.create(uri, byte[])`) that the engine comment
deliberately avoids. A payload over 57 bytes would make the test guard that
choice.
##########
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:
`delayed_resource` delays 3000 ms against a 4000 ms `resource-timeout`, and
`slow_tool` was shortened from 6000 to 3000 ms against a 2000 ms
`tool-timeout`. Both now have only about 1 s of margin. On a loaded CI runner
or in native mode (executor scheduling, first-read warm-up, the pool still busy
with the timed-out `slow_resource` read),
`testReadResourceWithinResourceTimeout` could flake. Maybe widen the gap, e.g.
a much shorter delay for the within-timeout case.
--
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]
