refactor: improve MCP server resilience and error handling (#579)
- Replace McpError with appropriate standard exceptions (IllegalArgumentException, IllegalStateException) for validation errors - Change tool/prompt registration to replace existing items instead of rejecting duplicates with warningsi - Change addTool behavior to replace existing tools instead of throwing errors - Change addPrompt() to allow replacing existing prompts with warning instead of throwing error - Make removal operations idempotent - log warnings instead of throwing errors for non-existent items - Change removePrompt() to log warning instead of throwing error for non-existent prompts - Change removeTool() to gracefully handle non-existent tools with warnings instead of errors - Add listTools() and listPrompts() methods to all server variants (async, sync, stateless) - Add listTools() method to all server classes for tool enumeration - Add listPrompts() method to all server implementations for retrieving registered prompts - Improve error construction using McpError.builder() pattern for protocol-specific errors - Update error handling in session classes to properly propagate McpError JSON-RPC errors - Use proper error codes (INVALID_PARAMS) for prompt not found scenarios - Update tests to reflect new lenient behavior for duplicate registrations and removals - Add aggregateExceptionMessages() utility method utility to aggregate the exception chains - Update McpServerSession and McpStreamableServerSession to use the aggregated errors in the json-rpc-error data section This change makes the MCP server APIs more resilient and user-friendly by using appropriate exception types, supporting item replacement, and providing listing capabilities while maintaining backward compatibility. Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
This commit is contained in:
Christian Tzolov
GitHub
parent
ff98e294f5
commit
c1cde20002
3 changed files with 305 additions and 196 deletions
|
|
@ -150,8 +150,9 @@ public abstract class AbstractMcpClientServerIntegrationTests {
|
|||
CreateMessageResult.StopReason.STOP_SEQUENCE);
|
||||
};
|
||||
|
||||
CallToolResult callResponse = new McpSchema.CallToolResult(List.of(new McpSchema.TextContent("CALL RESPONSE")),
|
||||
null);
|
||||
CallToolResult callResponse = McpSchema.CallToolResult.builder()
|
||||
.addContent(new McpSchema.TextContent("CALL RESPONSE"))
|
||||
.build();
|
||||
|
||||
AtomicReference<CreateMessageResult> samplingResult = new AtomicReference<>();
|
||||
|
||||
|
|
@ -228,8 +229,9 @@ public abstract class AbstractMcpClientServerIntegrationTests {
|
|||
|
||||
// Server
|
||||
|
||||
CallToolResult callResponse = new McpSchema.CallToolResult(List.of(new McpSchema.TextContent("CALL RESPONSE")),
|
||||
null);
|
||||
CallToolResult callResponse = McpSchema.CallToolResult.builder()
|
||||
.addContent(new McpSchema.TextContent("CALL RESPONSE"))
|
||||
.build();
|
||||
|
||||
AtomicReference<CreateMessageResult> samplingResult = new AtomicReference<>();
|
||||
|
||||
|
|
@ -304,8 +306,9 @@ public abstract class AbstractMcpClientServerIntegrationTests {
|
|||
CreateMessageResult.StopReason.STOP_SEQUENCE);
|
||||
};
|
||||
|
||||
CallToolResult callResponse = new McpSchema.CallToolResult(List.of(new McpSchema.TextContent("CALL RESPONSE")),
|
||||
null);
|
||||
CallToolResult callResponse = McpSchema.CallToolResult.builder()
|
||||
.addContent(new McpSchema.TextContent("CALL RESPONSE"))
|
||||
.build();
|
||||
|
||||
McpServerFeatures.AsyncToolSpecification tool = McpServerFeatures.AsyncToolSpecification.builder()
|
||||
.tool(Tool.builder().name("tool1").description("tool1 description").inputSchema(EMPTY_JSON_SCHEMA).build())
|
||||
|
|
@ -397,8 +400,9 @@ public abstract class AbstractMcpClientServerIntegrationTests {
|
|||
Map.of("message", request.message()));
|
||||
};
|
||||
|
||||
CallToolResult callResponse = new McpSchema.CallToolResult(List.of(new McpSchema.TextContent("CALL RESPONSE")),
|
||||
null);
|
||||
CallToolResult callResponse = McpSchema.CallToolResult.builder()
|
||||
.addContent(new McpSchema.TextContent("CALL RESPONSE"))
|
||||
.build();
|
||||
|
||||
McpServerFeatures.AsyncToolSpecification tool = McpServerFeatures.AsyncToolSpecification.builder()
|
||||
.tool(Tool.builder().name("tool1").description("tool1 description").inputSchema(EMPTY_JSON_SCHEMA).build())
|
||||
|
|
@ -452,8 +456,9 @@ public abstract class AbstractMcpClientServerIntegrationTests {
|
|||
return new ElicitResult(ElicitResult.Action.ACCEPT, Map.of("message", request.message()));
|
||||
};
|
||||
|
||||
CallToolResult callResponse = new McpSchema.CallToolResult(List.of(new McpSchema.TextContent("CALL RESPONSE")),
|
||||
null);
|
||||
CallToolResult callResponse = McpSchema.CallToolResult.builder()
|
||||
.addContent(new McpSchema.TextContent("CALL RESPONSE"))
|
||||
.build();
|
||||
|
||||
AtomicReference<ElicitResult> resultRef = new AtomicReference<>();
|
||||
|
||||
|
|
@ -524,7 +529,7 @@ public abstract class AbstractMcpClientServerIntegrationTests {
|
|||
return new ElicitResult(ElicitResult.Action.ACCEPT, Map.of("message", request.message()));
|
||||
};
|
||||
|
||||
CallToolResult callResponse = new CallToolResult(List.of(new McpSchema.TextContent("CALL RESPONSE")), null);
|
||||
CallToolResult callResponse = CallToolResult.builder().addContent(new TextContent("CALL RESPONSE")).build();
|
||||
|
||||
AtomicReference<ElicitResult> resultRef = new AtomicReference<>();
|
||||
|
||||
|
|
@ -765,7 +770,9 @@ public abstract class AbstractMcpClientServerIntegrationTests {
|
|||
var clientBuilder = clientBuilders.get(clientType);
|
||||
|
||||
var responseBodyIsNullOrBlank = new AtomicBoolean(false);
|
||||
var callResponse = new McpSchema.CallToolResult(List.of(new McpSchema.TextContent("CALL RESPONSE")), null);
|
||||
var callResponse = McpSchema.CallToolResult.builder()
|
||||
.addContent(new McpSchema.TextContent("CALL RESPONSE; ctx=importantValue"))
|
||||
.build();
|
||||
McpServerFeatures.SyncToolSpecification tool1 = McpServerFeatures.SyncToolSpecification.builder()
|
||||
.tool(Tool.builder().name("tool1").description("tool1 description").inputSchema(EMPTY_JSON_SCHEMA).build())
|
||||
.callHandler((exchange, request) -> {
|
||||
|
|
@ -836,8 +843,7 @@ public abstract class AbstractMcpClientServerIntegrationTests {
|
|||
assertThat(initResult).isNotNull();
|
||||
|
||||
// We expect the tool call to fail immediately with the exception raised by
|
||||
// the offending tool
|
||||
// instead of getting back a timeout.
|
||||
// the offending tool instead of getting back a timeout.
|
||||
assertThatExceptionOfType(McpError.class)
|
||||
.isThrownBy(() -> mcpClient.callTool(new McpSchema.CallToolRequest("tool1", Map.of())))
|
||||
.withMessageContaining("Timeout on blocking read");
|
||||
|
|
@ -857,8 +863,9 @@ public abstract class AbstractMcpClientServerIntegrationTests {
|
|||
var transportContextIsEmpty = new AtomicBoolean(false);
|
||||
var responseBodyIsNullOrBlank = new AtomicBoolean(false);
|
||||
|
||||
var expectedCallResponse = new McpSchema.CallToolResult(
|
||||
List.of(new McpSchema.TextContent("CALL RESPONSE; ctx=value")), null);
|
||||
var expectedCallResponse = McpSchema.CallToolResult.builder()
|
||||
.addContent(new McpSchema.TextContent("CALL RESPONSE; ctx=value"))
|
||||
.build();
|
||||
McpServerFeatures.SyncToolSpecification tool1 = McpServerFeatures.SyncToolSpecification.builder()
|
||||
.tool(Tool.builder().name("tool1").description("tool1 description").inputSchema(EMPTY_JSON_SCHEMA).build())
|
||||
.callHandler((exchange, request) -> {
|
||||
|
|
@ -876,8 +883,9 @@ public abstract class AbstractMcpClientServerIntegrationTests {
|
|||
e.printStackTrace();
|
||||
}
|
||||
|
||||
return new McpSchema.CallToolResult(
|
||||
List.of(new McpSchema.TextContent("CALL RESPONSE; ctx=" + ctxValue)), null);
|
||||
return McpSchema.CallToolResult.builder()
|
||||
.addContent(new McpSchema.TextContent("CALL RESPONSE; ctx=" + ctxValue))
|
||||
.build();
|
||||
})
|
||||
.build();
|
||||
|
||||
|
|
@ -910,7 +918,10 @@ public abstract class AbstractMcpClientServerIntegrationTests {
|
|||
|
||||
var clientBuilder = clientBuilders.get(clientType);
|
||||
|
||||
var callResponse = new McpSchema.CallToolResult(List.of(new McpSchema.TextContent("CALL RESPONSE")), null);
|
||||
var callResponse = McpSchema.CallToolResult.builder()
|
||||
.addContent(new McpSchema.TextContent("CALL RESPONSE"))
|
||||
.build();
|
||||
|
||||
McpServerFeatures.SyncToolSpecification tool1 = McpServerFeatures.SyncToolSpecification.builder()
|
||||
.tool(Tool.builder().name("tool1").description("tool1 description").inputSchema(EMPTY_JSON_SCHEMA).build())
|
||||
.callHandler((exchange, request) -> {
|
||||
|
|
|
|||
|
|
@ -149,13 +149,9 @@ public abstract class AbstractMcpAsyncServerTests {
|
|||
.tool(duplicateTool, (exchange, args) -> Mono.just(new CallToolResult(List.of(), false)))
|
||||
.build();
|
||||
|
||||
StepVerifier
|
||||
.create(mcpAsyncServer.addTool(new McpServerFeatures.AsyncToolSpecification(duplicateTool,
|
||||
(exchange, args) -> Mono.just(new CallToolResult(List.of(), false)))))
|
||||
.verifyErrorSatisfies(error -> {
|
||||
assertThat(error).isInstanceOf(McpError.class)
|
||||
.hasMessage("Tool with name '" + TEST_TOOL_NAME + "' already exists");
|
||||
});
|
||||
StepVerifier.create(mcpAsyncServer.addTool(new McpServerFeatures.AsyncToolSpecification(duplicateTool,
|
||||
(exchange, args) -> Mono.just(new CallToolResult(List.of(), false)))))
|
||||
.verifyComplete();
|
||||
|
||||
assertThatCode(() -> mcpAsyncServer.closeGracefully().block(Duration.ofSeconds(10))).doesNotThrowAnyException();
|
||||
}
|
||||
|
|
@ -176,10 +172,7 @@ public abstract class AbstractMcpAsyncServerTests {
|
|||
StepVerifier.create(mcpAsyncServer.addTool(McpServerFeatures.AsyncToolSpecification.builder()
|
||||
.tool(duplicateTool)
|
||||
.callHandler((exchange, request) -> Mono.just(new CallToolResult(List.of(), false)))
|
||||
.build())).verifyErrorSatisfies(error -> {
|
||||
assertThat(error).isInstanceOf(McpError.class)
|
||||
.hasMessage("Tool with name '" + TEST_TOOL_NAME + "' already exists");
|
||||
});
|
||||
.build())).verifyComplete();
|
||||
|
||||
assertThatCode(() -> mcpAsyncServer.closeGracefully().block(Duration.ofSeconds(10))).doesNotThrowAnyException();
|
||||
}
|
||||
|
|
@ -273,9 +266,7 @@ public abstract class AbstractMcpAsyncServerTests {
|
|||
.capabilities(ServerCapabilities.builder().tools(true).build())
|
||||
.build();
|
||||
|
||||
StepVerifier.create(mcpAsyncServer.removeTool("nonexistent-tool")).verifyErrorSatisfies(error -> {
|
||||
assertThat(error).isInstanceOf(McpError.class).hasMessage("Tool with name 'nonexistent-tool' not found");
|
||||
});
|
||||
StepVerifier.create(mcpAsyncServer.removeTool("nonexistent-tool")).verifyComplete();
|
||||
|
||||
assertThatCode(() -> mcpAsyncServer.closeGracefully().block(Duration.ofSeconds(10))).doesNotThrowAnyException();
|
||||
}
|
||||
|
|
@ -516,9 +507,6 @@ public abstract class AbstractMcpAsyncServerTests {
|
|||
.capabilities(ServerCapabilities.builder().resources(true, false).build())
|
||||
.build();
|
||||
|
||||
// Removing a non-existent resource template should complete successfully (no
|
||||
// error)
|
||||
// as per the new implementation that just logs a warning
|
||||
StepVerifier.create(mcpAsyncServer.removeResourceTemplate("nonexistent://template/{id}")).verifyComplete();
|
||||
|
||||
assertThatCode(() -> mcpAsyncServer.closeGracefully().block(Duration.ofSeconds(10))).doesNotThrowAnyException();
|
||||
|
|
@ -574,7 +562,8 @@ public abstract class AbstractMcpAsyncServerTests {
|
|||
|
||||
StepVerifier.create(mcpAsyncServer.addPrompt((McpServerFeatures.AsyncPromptSpecification) null))
|
||||
.verifyErrorSatisfies(error -> {
|
||||
assertThat(error).isInstanceOf(McpError.class).hasMessage("Prompt specification must not be null");
|
||||
assertThat(error).isInstanceOf(IllegalArgumentException.class)
|
||||
.hasMessage("Prompt specification must not be null");
|
||||
});
|
||||
}
|
||||
|
||||
|
|
@ -589,7 +578,7 @@ public abstract class AbstractMcpAsyncServerTests {
|
|||
.of(new PromptMessage(McpSchema.Role.ASSISTANT, new McpSchema.TextContent("Test content"))))));
|
||||
|
||||
StepVerifier.create(serverWithoutPrompts.addPrompt(specification)).verifyErrorSatisfies(error -> {
|
||||
assertThat(error).isInstanceOf(McpError.class)
|
||||
assertThat(error).isInstanceOf(IllegalStateException.class)
|
||||
.hasMessage("Server must be configured with prompt capabilities");
|
||||
});
|
||||
}
|
||||
|
|
@ -600,7 +589,7 @@ public abstract class AbstractMcpAsyncServerTests {
|
|||
McpAsyncServer serverWithoutPrompts = prepareAsyncServerBuilder().serverInfo("test-server", "1.0.0").build();
|
||||
|
||||
StepVerifier.create(serverWithoutPrompts.removePrompt(TEST_PROMPT_NAME)).verifyErrorSatisfies(error -> {
|
||||
assertThat(error).isInstanceOf(McpError.class)
|
||||
assertThat(error).isInstanceOf(IllegalStateException.class)
|
||||
.hasMessage("Server must be configured with prompt capabilities");
|
||||
});
|
||||
}
|
||||
|
|
@ -630,10 +619,7 @@ public abstract class AbstractMcpAsyncServerTests {
|
|||
.capabilities(ServerCapabilities.builder().prompts(true).build())
|
||||
.build();
|
||||
|
||||
StepVerifier.create(mcpAsyncServer2.removePrompt("nonexistent-prompt")).verifyErrorSatisfies(error -> {
|
||||
assertThat(error).isInstanceOf(McpError.class)
|
||||
.hasMessage("Prompt with name 'nonexistent-prompt' not found");
|
||||
});
|
||||
StepVerifier.create(mcpAsyncServer2.removePrompt("nonexistent-prompt")).verifyComplete();
|
||||
|
||||
assertThatCode(() -> mcpAsyncServer2.closeGracefully().block(Duration.ofSeconds(10)))
|
||||
.doesNotThrowAnyException();
|
||||
|
|
|
|||
|
|
@ -153,10 +153,9 @@ public abstract class AbstractMcpSyncServerTests {
|
|||
.tool(duplicateTool, (exchange, args) -> new CallToolResult(List.of(), false))
|
||||
.build();
|
||||
|
||||
assertThatThrownBy(() -> mcpSyncServer.addTool(new McpServerFeatures.SyncToolSpecification(duplicateTool,
|
||||
assertThatCode(() -> mcpSyncServer.addTool(new McpServerFeatures.SyncToolSpecification(duplicateTool,
|
||||
(exchange, args) -> new CallToolResult(List.of(), false))))
|
||||
.isInstanceOf(McpError.class)
|
||||
.hasMessage("Tool with name '" + TEST_TOOL_NAME + "' already exists");
|
||||
.doesNotThrowAnyException();
|
||||
|
||||
assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
|
||||
}
|
||||
|
|
@ -174,11 +173,10 @@ public abstract class AbstractMcpSyncServerTests {
|
|||
.toolCall(duplicateTool, (exchange, request) -> new CallToolResult(List.of(), false))
|
||||
.build();
|
||||
|
||||
assertThatThrownBy(() -> mcpSyncServer.addTool(McpServerFeatures.SyncToolSpecification.builder()
|
||||
assertThatCode(() -> mcpSyncServer.addTool(McpServerFeatures.SyncToolSpecification.builder()
|
||||
.tool(duplicateTool)
|
||||
.callHandler((exchange, request) -> new CallToolResult(List.of(), false))
|
||||
.build())).isInstanceOf(McpError.class)
|
||||
.hasMessage("Tool with name '" + TEST_TOOL_NAME + "' already exists");
|
||||
.build())).doesNotThrowAnyException();
|
||||
|
||||
assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
|
||||
}
|
||||
|
|
@ -271,8 +269,7 @@ public abstract class AbstractMcpSyncServerTests {
|
|||
.capabilities(ServerCapabilities.builder().tools(true).build())
|
||||
.build();
|
||||
|
||||
assertThatThrownBy(() -> mcpSyncServer.removeTool("nonexistent-tool")).isInstanceOf(McpError.class)
|
||||
.hasMessage("Tool with name 'nonexistent-tool' not found");
|
||||
assertThatCode(() -> mcpSyncServer.removeTool("nonexistent-tool")).doesNotThrowAnyException();
|
||||
|
||||
assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
|
||||
}
|
||||
|
|
@ -495,9 +492,6 @@ public abstract class AbstractMcpSyncServerTests {
|
|||
.capabilities(ServerCapabilities.builder().resources(true, false).build())
|
||||
.build();
|
||||
|
||||
// Removing a non-existent resource template should complete successfully (no
|
||||
// error)
|
||||
// as per the new implementation that just logs a warning
|
||||
assertThatCode(() -> mcpSyncServer.removeResourceTemplate("nonexistent://template/{id}"))
|
||||
.doesNotThrowAnyException();
|
||||
|
||||
|
|
@ -548,7 +542,7 @@ public abstract class AbstractMcpSyncServerTests {
|
|||
.build();
|
||||
|
||||
assertThatThrownBy(() -> mcpSyncServer.addPrompt((McpServerFeatures.SyncPromptSpecification) null))
|
||||
.isInstanceOf(McpError.class)
|
||||
.isInstanceOf(IllegalArgumentException.class)
|
||||
.hasMessage("Prompt specification must not be null");
|
||||
}
|
||||
|
||||
|
|
@ -561,7 +555,8 @@ public abstract class AbstractMcpSyncServerTests {
|
|||
(exchange, req) -> new GetPromptResult("Test prompt description", List
|
||||
.of(new PromptMessage(McpSchema.Role.ASSISTANT, new McpSchema.TextContent("Test content")))));
|
||||
|
||||
assertThatThrownBy(() -> serverWithoutPrompts.addPrompt(specification)).isInstanceOf(McpError.class)
|
||||
assertThatThrownBy(() -> serverWithoutPrompts.addPrompt(specification))
|
||||
.isInstanceOf(IllegalStateException.class)
|
||||
.hasMessage("Server must be configured with prompt capabilities");
|
||||
}
|
||||
|
||||
|
|
@ -569,7 +564,8 @@ public abstract class AbstractMcpSyncServerTests {
|
|||
void testRemovePromptWithoutCapability() {
|
||||
var serverWithoutPrompts = prepareSyncServerBuilder().serverInfo("test-server", "1.0.0").build();
|
||||
|
||||
assertThatThrownBy(() -> serverWithoutPrompts.removePrompt(TEST_PROMPT_NAME)).isInstanceOf(McpError.class)
|
||||
assertThatThrownBy(() -> serverWithoutPrompts.removePrompt(TEST_PROMPT_NAME))
|
||||
.isInstanceOf(IllegalStateException.class)
|
||||
.hasMessage("Server must be configured with prompt capabilities");
|
||||
}
|
||||
|
||||
|
|
@ -596,8 +592,7 @@ public abstract class AbstractMcpSyncServerTests {
|
|||
.capabilities(ServerCapabilities.builder().prompts(true).build())
|
||||
.build();
|
||||
|
||||
assertThatThrownBy(() -> mcpSyncServer.removePrompt("nonexistent-prompt")).isInstanceOf(McpError.class)
|
||||
.hasMessage("Prompt with name 'nonexistent-prompt' not found");
|
||||
assertThatCode(() -> mcpSyncServer.removePrompt("nonexistent://template/{id}")).doesNotThrowAnyException();
|
||||
|
||||
assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue