Skip to content

feat: enhance error handling with custom error code preservation #653

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 7, 2025
+160 −39
Merged

feat: enhance error handling with custom error code preservation #653

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
09d8df4
feat: enhance error handling with custom error code preservation
tzolov Nov 6, 2025
2f87aed
Address review comments and improve the test run isolation
tzolov Nov 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions mcp-core/src/main/java/io/modelcontextprotocol/spec/McpClientSession.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,9 +166,15 @@ private void handle(McpSchema.JSONRPCMessage message) {
else if (message instanceof McpSchema.JSONRPCRequest request) {
logger.debug("Received request: {}", request);
handleIncomingRequest(request).onErrorResume(error -> {

McpSchema.JSONRPCResponse.JSONRPCError jsonRpcError = (error instanceof McpError mcpError
&& mcpError.getJsonRpcError() != null) ? mcpError.getJsonRpcError()
// TODO: add error message through the data field
: new McpSchema.JSONRPCResponse.JSONRPCError(McpSchema.ErrorCodes.INTERNAL_ERROR,
error.getMessage(), McpError.aggregateExceptionMessages(error));

var errorResponse = new McpSchema.JSONRPCResponse(McpSchema.JSONRPC_VERSION, request.id(), null,
new McpSchema.JSONRPCResponse.JSONRPCError(McpSchema.ErrorCodes.INTERNAL_ERROR,
error.getMessage(), null));
jsonRpcError);
return Mono.just(errorResponse);
}).flatMap(this.transport::sendMessage).onErrorComplete(t -> {
logger.warn("Issue sending response to the client, ", t);
Expand Down
161 changes: 149 additions & 12 deletions mcp-core/src/test/java/io/modelcontextprotocol/spec/McpClientSessionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import reactor.test.StepVerifier;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

/**
* Test suite for {@link McpClientSession} that verifies its JSON-RPC message handling,
Expand Down Expand Up @@ -57,17 +56,6 @@ void tearDown() {
}
}

@Test
void testConstructorWithInvalidArguments() {
assertThatThrownBy(() -> new McpClientSession(null, transport, Map.of(), Map.of()))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("The requestTimeout can not be null");

assertThatThrownBy(() -> new McpClientSession(TIMEOUT, null, Map.of(), Map.of()))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("transport can not be null");
}

TypeRef<String> responseType = new TypeRef<>() {
};

Expand Down Expand Up @@ -190,6 +178,155 @@ void testUnknownMethodHandling() {
assertThat(response.error().code()).isEqualTo(McpSchema.ErrorCodes.METHOD_NOT_FOUND);
}

@Test
void testRequestHandlerThrowsMcpErrorWithJsonRpcError() {
// Setup: Create a request handler that throws McpError with custom error code and
// data
String testMethod = "test.customError";
Map<String, Object> errorData = Map.of("customField", "customValue");
McpClientSession.RequestHandler<?> failingHandler = params -> Mono
.error(McpError.builder(123).message("Custom error message").data(errorData).build());

transport = new MockMcpClientTransport();
session = new McpClientSession(TIMEOUT, transport, Map.of(testMethod, failingHandler), Map.of());
Copy link
Contributor

@Kehrlann Kehrlann Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the transport from the class, and make a new session instance.

Suggested change
transport = new MockMcpClientTransport();
session = new McpClientSession(TIMEOUT, transport, Map.of(testMethod, failingHandler), Map.of());
var session = new McpClientSession(TIMEOUT, transport, Map.of(testMethod, failingHandler), Map.of());

Goes for all tests.


// Simulate incoming request that will trigger the error
McpSchema.JSONRPCRequest request = new McpSchema.JSONRPCRequest(McpSchema.JSONRPC_VERSION, testMethod,
"test-id", null);
transport.simulateIncomingMessage(request);

// Verify: The response should contain the custom error from McpError
McpSchema.JSONRPCMessage sentMessage = transport.getLastSentMessage();
assertThat(sentMessage).isInstanceOf(McpSchema.JSONRPCResponse.class);
McpSchema.JSONRPCResponse response = (McpSchema.JSONRPCResponse) sentMessage;
assertThat(response.error()).isNotNull();
assertThat(response.error().code()).isEqualTo(123);
assertThat(response.error().message()).isEqualTo("Custom error message");
assertThat(response.error().data()).isEqualTo(errorData);
}

@Test
void testRequestHandlerThrowsGenericException() {
// Setup: Create a request handler that throws a generic RuntimeException
String testMethod = "test.genericError";
RuntimeException exception = new RuntimeException("Something went wrong");
McpClientSession.RequestHandler<?> failingHandler = params -> Mono.error(exception);

transport = new MockMcpClientTransport();
session = new McpClientSession(TIMEOUT, transport, Map.of(testMethod, failingHandler), Map.of());

// Simulate incoming request that will trigger the error
McpSchema.JSONRPCRequest request = new McpSchema.JSONRPCRequest(McpSchema.JSONRPC_VERSION, testMethod,
"test-id", null);
transport.simulateIncomingMessage(request);

// Verify: The response should contain INTERNAL_ERROR with aggregated exception
// messages in data field
McpSchema.JSONRPCMessage sentMessage = transport.getLastSentMessage();
assertThat(sentMessage).isInstanceOf(McpSchema.JSONRPCResponse.class);
McpSchema.JSONRPCResponse response = (McpSchema.JSONRPCResponse) sentMessage;
assertThat(response.error()).isNotNull();
assertThat(response.error().code()).isEqualTo(McpSchema.ErrorCodes.INTERNAL_ERROR);
assertThat(response.error().message()).isEqualTo("Something went wrong");
// Verify data field contains aggregated exception messages
assertThat(response.error().data()).isNotNull();
assertThat(response.error().data().toString()).contains("RuntimeException");
assertThat(response.error().data().toString()).contains("Something went wrong");
}

@Test
void testRequestHandlerThrowsExceptionWithCause() {
// Setup: Create a request handler that throws an exception with a cause chain
String testMethod = "test.chainedError";
RuntimeException rootCause = new IllegalArgumentException("Root cause message");
RuntimeException middleCause = new IllegalStateException("Middle cause message", rootCause);
RuntimeException topException = new RuntimeException("Top level message", middleCause);
McpClientSession.RequestHandler<?> failingHandler = params -> Mono.error(topException);

transport = new MockMcpClientTransport();
session = new McpClientSession(TIMEOUT, transport, Map.of(testMethod, failingHandler), Map.of());

// Simulate incoming request that will trigger the error
McpSchema.JSONRPCRequest request = new McpSchema.JSONRPCRequest(McpSchema.JSONRPC_VERSION, testMethod,
"test-id", null);
transport.simulateIncomingMessage(request);

// Verify: The response should contain INTERNAL_ERROR with full exception chain
// in data field
McpSchema.JSONRPCMessage sentMessage = transport.getLastSentMessage();
assertThat(sentMessage).isInstanceOf(McpSchema.JSONRPCResponse.class);
McpSchema.JSONRPCResponse response = (McpSchema.JSONRPCResponse) sentMessage;
assertThat(response.error()).isNotNull();
assertThat(response.error().code()).isEqualTo(McpSchema.ErrorCodes.INTERNAL_ERROR);
assertThat(response.error().message()).isEqualTo("Top level message");
// Verify data field contains the full exception chain
String dataString = response.error().data().toString();
assertThat(dataString).contains("RuntimeException");
assertThat(dataString).contains("Top level message");
assertThat(dataString).contains("IllegalStateException");
assertThat(dataString).contains("Middle cause message");
assertThat(dataString).contains("IllegalArgumentException");
assertThat(dataString).contains("Root cause message");
}

@Test
void testRequestHandlerThrowsMcpErrorWithoutJsonRpcError() {
Copy link
Contributor

@Kehrlann Kehrlann Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggesting] remove this tests

// Setup: Create a request handler that throws deprecated McpError without
// JSONRPCError
String testMethod = "test.deprecatedError";
@SuppressWarnings("deprecation")
McpError deprecatedError = new McpError("Deprecated error format");
McpClientSession.RequestHandler<?> failingHandler = params -> Mono.error(deprecatedError);

transport = new MockMcpClientTransport();
session = new McpClientSession(TIMEOUT, transport, Map.of(testMethod, failingHandler), Map.of());

// Simulate incoming request that will trigger the error
McpSchema.JSONRPCRequest request = new McpSchema.JSONRPCRequest(McpSchema.JSONRPC_VERSION, testMethod,
"test-id", null);
transport.simulateIncomingMessage(request);

// Verify: The response should create a new INTERNAL_ERROR with aggregated
// messages
McpSchema.JSONRPCMessage sentMessage = transport.getLastSentMessage();
assertThat(sentMessage).isInstanceOf(McpSchema.JSONRPCResponse.class);
McpSchema.JSONRPCResponse response = (McpSchema.JSONRPCResponse) sentMessage;
assertThat(response.error()).isNotNull();
assertThat(response.error().code()).isEqualTo(McpSchema.ErrorCodes.INTERNAL_ERROR);
assertThat(response.error().message()).isEqualTo("Deprecated error format");
// Verify data field contains aggregated exception messages
assertThat(response.error().data()).isNotNull();
assertThat(response.error().data().toString()).contains("McpError");
assertThat(response.error().data().toString()).contains("Deprecated error format");
}

@Test
void testRequestHandlerThrowsResourceNotFoundError() {
Copy link
Contributor

@Kehrlann Kehrlann Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] remove this test, it's a special case of testRequestHandlerThrowsMcpErrorWithJsonRpcError

// Setup: Create a request handler that throws RESOURCE_NOT_FOUND error
String testMethod = "test.resourceError";
String resourceUri = "file:///missing/resource.txt";
McpClientSession.RequestHandler<?> failingHandler = params -> Mono
.error(McpError.RESOURCE_NOT_FOUND.apply(resourceUri));

transport = new MockMcpClientTransport();
session = new McpClientSession(TIMEOUT, transport, Map.of(testMethod, failingHandler), Map.of());

// Simulate incoming request that will trigger the error
McpSchema.JSONRPCRequest request = new McpSchema.JSONRPCRequest(McpSchema.JSONRPC_VERSION, testMethod,
"test-id", null);
transport.simulateIncomingMessage(request);

// Verify: The response should preserve the RESOURCE_NOT_FOUND error code and
// data
McpSchema.JSONRPCMessage sentMessage = transport.getLastSentMessage();
assertThat(sentMessage).isInstanceOf(McpSchema.JSONRPCResponse.class);
McpSchema.JSONRPCResponse response = (McpSchema.JSONRPCResponse) sentMessage;
assertThat(response.error()).isNotNull();
assertThat(response.error().code()).isEqualTo(McpSchema.ErrorCodes.RESOURCE_NOT_FOUND);
assertThat(response.error().message()).isEqualTo("Resource not found");
assertThat(response.error().data()).isEqualTo(Map.of("uri", resourceUri));
}

@Test
void testGracefulShutdown() {
StepVerifier.create(session.closeGracefully()).verifyComplete();
Expand Down