Skip to content

Commit 5a6be18

Browse files
tzolovtzolov
committed
refactor: replace generic McpError with specific exception types
BREAKING CHANGE: McpError is no longer used for validation and internal errors. Client code catching McpError should now catch specific exception types like IllegalArgumentException, IllegalStateException, McpTransportException, or McpClientInternalException. - Add McpTransportException for transport layer errors - Add McpClientInternalException for internal client failures - Use standard Java exceptions for validation errors - Implement structured error builder with JSON-RPC error codes - Change tool not found errors to be returned in CallToolResult - Update logging levels for unregistered notification handlers Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
1 parent 4937fc1 commit 5a6be18

31 files changed

+349
-209
lines changed

‎mcp-spring/mcp-spring-webflux/src/main/java/io/modelcontextprotocol/client/transport/WebClientStreamableHttpTransport.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import io.modelcontextprotocol.spec.McpClientTransport;
2727
import io.modelcontextprotocol.spec.McpError;
2828
import io.modelcontextprotocol.spec.McpSchema;
29+
import io.modelcontextprotocol.spec.McpTransportException;
2930
import io.modelcontextprotocol.spec.McpTransportSession;
3031
import io.modelcontextprotocol.spec.McpTransportSessionNotFoundException;
3132
import io.modelcontextprotocol.spec.McpTransportStream;
@@ -428,11 +429,11 @@ private Tuple2<Optional<String>, Iterable<McpSchema.JSONRPCMessage>> parse(Serve
428429
return Tuples.of(Optional.ofNullable(event.id()), List.of(message));
429430
}
430431
catch (IOException ioException) {
431-
throw new McpError("Error parsing JSON-RPC message: " + event.data());
432+
throw new McpTransportException("Error parsing JSON-RPC message: " + event.data(), ioException);
432433
}
433434
}
434435
else {
435-
throw new McpError("Received unrecognized SSE event type: " + event.event());
436+
throw new McpTransportException("Received unrecognized SSE event type: " + event.event());
436437
}
437438
}
438439

‎mcp-spring/mcp-spring-webflux/src/main/java/io/modelcontextprotocol/client/transport/WebFluxSseClientTransport.java‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,12 @@
99

1010
import com.fasterxml.jackson.core.type.TypeReference;
1111
import com.fasterxml.jackson.databind.ObjectMapper;
12+
13+
import io.modelcontextprotocol.spec.McpClientInternalException;
1214
import io.modelcontextprotocol.spec.McpClientTransport;
1315
import io.modelcontextprotocol.spec.McpError;
1416
import io.modelcontextprotocol.spec.McpSchema;
17+
import io.modelcontextprotocol.spec.McpTransportException;
1518
import io.modelcontextprotocol.spec.McpSchema.JSONRPCMessage;
1619
import io.modelcontextprotocol.util.Assert;
1720
import org.slf4j.Logger;
@@ -197,13 +200,14 @@ public Mono<Void> connect(Function<Mono<JSONRPCMessage>, Mono<JSONRPCMessage>> h
197200
this.inboundSubscription = events.concatMap(event -> Mono.just(event).<JSONRPCMessage>handle((e, s) -> {
198201
if (ENDPOINT_EVENT_TYPE.equals(event.event())) {
199202
String messageEndpointUri = event.data();
200-
if (messageEndpointSink.tryEmitValue(messageEndpointUri).isSuccess()) {
203+
var emitResult = messageEndpointSink.tryEmitValue(messageEndpointUri);
204+
if (emitResult.isSuccess()) {
201205
s.complete();
202206
}
203207
else {
204208
// TODO: clarify with the spec if multiple events can be
205209
// received
206-
s.error(new McpError("Failed to handle SSE endpoint event"));
210+
s.error(new McpClientInternalException("Failed to handle SSE endpoint event"));
207211
}
208212
}
209213
else if (MESSAGE_EVENT_TYPE.equals(event.event())) {
@@ -216,7 +220,7 @@ else if (MESSAGE_EVENT_TYPE.equals(event.event())) {
216220
}
217221
}
218222
else {
219-
s.error(new McpError("Received unrecognized SSE event type: " + event.event()));
223+
s.error(new McpTransportException("Received unrecognized SSE event type: " + event.event()));
220224
}
221225
}).transform(handler)).subscribe();
222226

‎mcp-spring/mcp-spring-webflux/src/main/java/io/modelcontextprotocol/server/transport/WebFluxSseServerTransportProvider.java‎

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,12 @@
77
import com.fasterxml.jackson.databind.ObjectMapper;
88
import io.modelcontextprotocol.spec.McpError;
99
import io.modelcontextprotocol.spec.McpSchema;
10+
import io.modelcontextprotocol.spec.McpSchema.ErrorCodes;
11+
import io.modelcontextprotocol.spec.McpSchema.JSONRPCResponse.JSONRPCError;
1012
import io.modelcontextprotocol.spec.McpServerSession;
1113
import io.modelcontextprotocol.spec.McpServerTransport;
1214
import io.modelcontextprotocol.spec.McpServerTransportProvider;
15+
import io.modelcontextprotocol.spec.McpTransportException;
1316
import io.modelcontextprotocol.util.Assert;
1417
import org.slf4j.Logger;
1518
import org.slf4j.LoggerFactory;
@@ -300,31 +303,29 @@ private Mono<ServerResponse> handleMessage(ServerRequest request) {
300303
}
301304

302305
if (request.queryParam("sessionId").isEmpty()) {
303-
return ServerResponse.badRequest().bodyValue(new McpError("Session ID missing in message endpoint"));
306+
return ServerResponse.badRequest()
307+
.bodyValue(McpError.builder(ErrorCodes.INVALID_REQUEST).message("Missing session ID param").build());
304308
}
305309

306-
McpServerSession session = sessions.get(request.queryParam("sessionId").get());
310+
String sessionId = request.queryParam("sessionId").get();
311+
McpServerSession session = sessions.get(sessionId);
307312

308313
if (session == null) {
309314
return ServerResponse.status(HttpStatus.NOT_FOUND)
310-
.bodyValue(new McpError("Session not found: " + request.queryParam("sessionId").get()));
315+
.bodyValue(McpError.builder(ErrorCodes.INVALID_REQUEST)
316+
.message("SessionId not found")
317+
.data("Empty sessionId: " + sessionId)
318+
.build());
311319
}
312320

313321
return request.bodyToMono(String.class).flatMap(body -> {
314322
try {
315323
McpSchema.JSONRPCMessage message = McpSchema.deserializeJsonRpcMessage(objectMapper, body);
316-
return session.handle(message).flatMap(response -> ServerResponse.ok().build()).onErrorResume(error -> {
317-
logger.error("Error processing message: {}", error.getMessage());
318-
// TODO: instead of signalling the error, just respond with 200 OK
319-
// - the error is signalled on the SSE connection
320-
// return ServerResponse.ok().build();
321-
return ServerResponse.status(HttpStatus.INTERNAL_SERVER_ERROR)
322-
.bodyValue(new McpError(error.getMessage()));
323-
});
324+
return session.handle(message).flatMap(response -> ServerResponse.ok().build());
324325
}
325326
catch (IllegalArgumentException | IOException e) {
326327
logger.error("Failed to deserialize message: {}", e.getMessage());
327-
return ServerResponse.badRequest().bodyValue(new McpError("Invalid message format"));
328+
return ServerResponse.badRequest().bodyValue(new McpTransportException("Invalid message format", e));
328329
}
329330
});
330331
}

‎mcp-spring/mcp-spring-webmvc/src/main/java/io/modelcontextprotocol/server/transport/WebMvcSseServerTransportProvider.java‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,11 @@
1313
import com.fasterxml.jackson.databind.ObjectMapper;
1414
import io.modelcontextprotocol.spec.McpError;
1515
import io.modelcontextprotocol.spec.McpSchema;
16+
import io.modelcontextprotocol.spec.McpSchema.ErrorCodes;
17+
import io.modelcontextprotocol.spec.McpSchema.JSONRPCResponse.JSONRPCError;
1618
import io.modelcontextprotocol.spec.McpServerTransport;
1719
import io.modelcontextprotocol.spec.McpServerTransportProvider;
20+
import io.modelcontextprotocol.spec.McpTransportException;
1821
import io.modelcontextprotocol.spec.McpServerSession;
1922
import io.modelcontextprotocol.util.Assert;
2023
import org.slf4j.Logger;
@@ -300,14 +303,19 @@ private ServerResponse handleMessage(ServerRequest request) {
300303
}
301304

302305
if (request.param("sessionId").isEmpty()) {
303-
return ServerResponse.badRequest().body(new McpError("Session ID missing in message endpoint"));
306+
return ServerResponse.badRequest()
307+
.body(McpError.builder(ErrorCodes.INVALID_REQUEST).message("Missing session ID param").build());
304308
}
305309

306310
String sessionId = request.param("sessionId").get();
307311
McpServerSession session = sessions.get(sessionId);
308312

309313
if (session == null) {
310-
return ServerResponse.status(HttpStatus.NOT_FOUND).body(new McpError("Session not found: " + sessionId));
314+
return ServerResponse.status(HttpStatus.NOT_FOUND)
315+
.body(McpError.builder(ErrorCodes.INVALID_REQUEST)
316+
.message("SessionId not found")
317+
.data("Empty sessionId: " + sessionId)
318+
.build());
311319
}
312320

313321
try {
@@ -321,11 +329,11 @@ private ServerResponse handleMessage(ServerRequest request) {
321329
}
322330
catch (IllegalArgumentException | IOException e) {
323331
logger.error("Failed to deserialize message: {}", e.getMessage());
324-
return ServerResponse.badRequest().body(new McpError("Invalid message format"));
332+
return ServerResponse.badRequest().body(new McpTransportException("Invalid message format", e));
325333
}
326334
catch (Exception e) {
327335
logger.error("Error handling message: {}", e.getMessage());
328-
return ServerResponse.status(HttpStatus.INTERNAL_SERVER_ERROR).body(new McpError(e.getMessage()));
336+
return ServerResponse.status(HttpStatus.INTERNAL_SERVER_ERROR).body(new McpTransportException(e));
329337
}
330338
}
331339

‎mcp-test/src/main/java/io/modelcontextprotocol/client/AbstractMcpAsyncClientTests.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -486,7 +486,8 @@ void testAddRoot() {
486486
void testAddRootWithNullValue() {
487487
withClient(createMcpTransport(), mcpAsyncClient -> {
488488
StepVerifier.create(mcpAsyncClient.addRoot(null))
489-
.consumeErrorWith(e -> assertThat(e).isInstanceOf(McpError.class).hasMessage("Root must not be null"))
489+
.consumeErrorWith(e -> assertThat(e).isInstanceOf(IllegalArgumentException.class)
490+
.hasMessage("Root must not be null"))
490491
.verify();
491492
});
492493
}
@@ -505,7 +506,7 @@ void testRemoveRoot() {
505506
void testRemoveNonExistentRoot() {
506507
withClient(createMcpTransport(), mcpAsyncClient -> {
507508
StepVerifier.create(mcpAsyncClient.removeRoot("nonexistent-uri"))
508-
.consumeErrorWith(e -> assertThat(e).isInstanceOf(McpError.class)
509+
.consumeErrorWith(e -> assertThat(e).isInstanceOf(IllegalStateException.class)
509510
.hasMessage("Root with uri 'nonexistent-uri' not found"))
510511
.verify();
511512
});

‎mcp-test/src/main/java/io/modelcontextprotocol/server/AbstractMcpAsyncServerTests.java‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ void testAddDuplicateTool() {
147147
.create(mcpAsyncServer.addTool(new McpServerFeatures.AsyncToolSpecification(duplicateTool,
148148
(exchange, args) -> Mono.just(new CallToolResult(List.of(), false)))))
149149
.verifyErrorSatisfies(error -> {
150-
assertThat(error).isInstanceOf(McpError.class)
150+
assertThat(error).isInstanceOf(IllegalArgumentException.class)
151151
.hasMessage("Tool with name '" + TEST_TOOL_NAME + "' already exists");
152152
});
153153

@@ -168,7 +168,7 @@ void testAddDuplicateToolCall() {
168168
.tool(duplicateTool)
169169
.callHandler((exchange, request) -> Mono.just(new CallToolResult(List.of(), false)))
170170
.build())).verifyErrorSatisfies(error -> {
171-
assertThat(error).isInstanceOf(McpError.class)
171+
assertThat(error).isInstanceOf(IllegalArgumentException.class)
172172
.hasMessage("Tool with name '" + TEST_TOOL_NAME + "' already exists");
173173
});
174174

@@ -254,7 +254,8 @@ void testRemoveNonexistentTool() {
254254
.build();
255255

256256
StepVerifier.create(mcpAsyncServer.removeTool("nonexistent-tool")).verifyErrorSatisfies(error -> {
257-
assertThat(error).isInstanceOf(McpError.class).hasMessage("Tool with name 'nonexistent-tool' not found");
257+
assertThat(error).isInstanceOf(IllegalArgumentException.class)
258+
.hasMessage("Tool with name 'nonexistent-tool' not found");
258259
});
259260

260261
assertThatCode(() -> mcpAsyncServer.closeGracefully().block(Duration.ofSeconds(10))).doesNotThrowAnyException();
@@ -326,7 +327,7 @@ void testAddResourceWithNullSpecification() {
326327

327328
StepVerifier.create(mcpAsyncServer.addResource((McpServerFeatures.AsyncResourceSpecification) null))
328329
.verifyErrorSatisfies(error -> {
329-
assertThat(error).isInstanceOf(McpError.class).hasMessage("Resource must not be null");
330+
assertThat(error).isInstanceOf(IllegalArgumentException.class).hasMessage("Resource must not be null");
330331
});
331332

332333
assertThatCode(() -> mcpAsyncServer.closeGracefully().block(Duration.ofSeconds(10))).doesNotThrowAnyException();
@@ -345,7 +346,7 @@ void testAddResourceWithoutCapability() {
345346
resource, (exchange, req) -> Mono.just(new ReadResourceResult(List.of())));
346347

347348
StepVerifier.create(serverWithoutResources.addResource(specification)).verifyErrorSatisfies(error -> {
348-
assertThat(error).isInstanceOf(McpError.class)
349+
assertThat(error).isInstanceOf(IllegalStateException.class)
349350
.hasMessage("Server must be configured with resource capabilities");
350351
});
351352
}
@@ -358,7 +359,7 @@ void testRemoveResourceWithoutCapability() {
358359
.build();
359360

360361
StepVerifier.create(serverWithoutResources.removeResource(TEST_RESOURCE_URI)).verifyErrorSatisfies(error -> {
361-
assertThat(error).isInstanceOf(McpError.class)
362+
assertThat(error).isInstanceOf(IllegalStateException.class)
362363
.hasMessage("Server must be configured with resource capabilities");
363364
});
364365
}
@@ -385,7 +386,8 @@ void testAddPromptWithNullSpecification() {
385386

386387
StepVerifier.create(mcpAsyncServer.addPrompt((McpServerFeatures.AsyncPromptSpecification) null))
387388
.verifyErrorSatisfies(error -> {
388-
assertThat(error).isInstanceOf(McpError.class).hasMessage("Prompt specification must not be null");
389+
assertThat(error).isInstanceOf(IllegalArgumentException.class)
390+
.hasMessage("Prompt specification must not be null");
389391
});
390392
}
391393

@@ -402,7 +404,7 @@ void testAddPromptWithoutCapability() {
402404
.of(new PromptMessage(McpSchema.Role.ASSISTANT, new McpSchema.TextContent("Test content"))))));
403405

404406
StepVerifier.create(serverWithoutPrompts.addPrompt(specification)).verifyErrorSatisfies(error -> {
405-
assertThat(error).isInstanceOf(McpError.class)
407+
assertThat(error).isInstanceOf(IllegalStateException.class)
406408
.hasMessage("Server must be configured with prompt capabilities");
407409
});
408410
}
@@ -415,7 +417,7 @@ void testRemovePromptWithoutCapability() {
415417
.build();
416418

417419
StepVerifier.create(serverWithoutPrompts.removePrompt(TEST_PROMPT_NAME)).verifyErrorSatisfies(error -> {
418-
assertThat(error).isInstanceOf(McpError.class)
420+
assertThat(error).isInstanceOf(IllegalStateException.class)
419421
.hasMessage("Server must be configured with prompt capabilities");
420422
});
421423
}
@@ -448,7 +450,7 @@ void testRemoveNonexistentPrompt() {
448450
.build();
449451

450452
StepVerifier.create(mcpAsyncServer2.removePrompt("nonexistent-prompt")).verifyErrorSatisfies(error -> {
451-
assertThat(error).isInstanceOf(McpError.class)
453+
assertThat(error).isInstanceOf(IllegalArgumentException.class)
452454
.hasMessage("Prompt with name 'nonexistent-prompt' not found");
453455
});
454456

‎mcp-test/src/main/java/io/modelcontextprotocol/server/AbstractMcpSyncServerTests.java‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ void testAddDuplicateTool() {
153153

154154
assertThatThrownBy(() -> mcpSyncServer.addTool(new McpServerFeatures.SyncToolSpecification(duplicateTool,
155155
(exchange, args) -> new CallToolResult(List.of(), false))))
156-
.isInstanceOf(McpError.class)
156+
.isInstanceOf(IllegalArgumentException.class)
157157
.hasMessage("Tool with name '" + TEST_TOOL_NAME + "' already exists");
158158

159159
assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
@@ -172,7 +172,7 @@ void testAddDuplicateToolCall() {
172172
assertThatThrownBy(() -> mcpSyncServer.addTool(McpServerFeatures.SyncToolSpecification.builder()
173173
.tool(duplicateTool)
174174
.callHandler((exchange, request) -> new CallToolResult(List.of(), false))
175-
.build())).isInstanceOf(McpError.class)
175+
.build())).isInstanceOf(IllegalArgumentException.class)
176176
.hasMessage("Tool with name '" + TEST_TOOL_NAME + "' already exists");
177177

178178
assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
@@ -256,7 +256,8 @@ void testRemoveNonexistentTool() {
256256
.capabilities(ServerCapabilities.builder().tools(true).build())
257257
.build();
258258

259-
assertThatThrownBy(() -> mcpSyncServer.removeTool("nonexistent-tool")).isInstanceOf(McpError.class)
259+
assertThatThrownBy(() -> mcpSyncServer.removeTool("nonexistent-tool"))
260+
.isInstanceOf(IllegalArgumentException.class)
260261
.hasMessage("Tool with name 'nonexistent-tool' not found");
261262

262263
assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
@@ -320,7 +321,7 @@ void testAddResourceWithNullSpecification() {
320321
.build();
321322

322323
assertThatThrownBy(() -> mcpSyncServer.addResource((McpServerFeatures.SyncResourceSpecification) null))
323-
.isInstanceOf(McpError.class)
324+
.isInstanceOf(IllegalArgumentException.class)
324325
.hasMessage("Resource must not be null");
325326

326327
assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
@@ -337,7 +338,8 @@ void testAddResourceWithoutCapability() {
337338
McpServerFeatures.SyncResourceSpecification specification = new McpServerFeatures.SyncResourceSpecification(
338339
resource, (exchange, req) -> new ReadResourceResult(List.of()));
339340

340-
assertThatThrownBy(() -> serverWithoutResources.addResource(specification)).isInstanceOf(McpError.class)
341+
assertThatThrownBy(() -> serverWithoutResources.addResource(specification))
342+
.isInstanceOf(IllegalStateException.class)
341343
.hasMessage("Server must be configured with resource capabilities");
342344
}
343345

@@ -347,7 +349,8 @@ void testRemoveResourceWithoutCapability() {
347349
.serverInfo("test-server", "1.0.0")
348350
.build();
349351

350-
assertThatThrownBy(() -> serverWithoutResources.removeResource(TEST_RESOURCE_URI)).isInstanceOf(McpError.class)
352+
assertThatThrownBy(() -> serverWithoutResources.removeResource(TEST_RESOURCE_URI))
353+
.isInstanceOf(IllegalStateException.class)
351354
.hasMessage("Server must be configured with resource capabilities");
352355
}
353356

@@ -372,7 +375,7 @@ void testAddPromptWithNullSpecification() {
372375
.build();
373376

374377
assertThatThrownBy(() -> mcpSyncServer.addPrompt((McpServerFeatures.SyncPromptSpecification) null))
375-
.isInstanceOf(McpError.class)
378+
.isInstanceOf(IllegalArgumentException.class)
376379
.hasMessage("Prompt specification must not be null");
377380
}
378381

@@ -387,7 +390,8 @@ void testAddPromptWithoutCapability() {
387390
(exchange, req) -> new GetPromptResult("Test prompt description", List
388391
.of(new PromptMessage(McpSchema.Role.ASSISTANT, new McpSchema.TextContent("Test content")))));
389392

390-
assertThatThrownBy(() -> serverWithoutPrompts.addPrompt(specification)).isInstanceOf(McpError.class)
393+
assertThatThrownBy(() -> serverWithoutPrompts.addPrompt(specification))
394+
.isInstanceOf(IllegalStateException.class)
391395
.hasMessage("Server must be configured with prompt capabilities");
392396
}
393397

@@ -397,7 +401,8 @@ void testRemovePromptWithoutCapability() {
397401
.serverInfo("test-server", "1.0.0")
398402
.build();
399403

400-
assertThatThrownBy(() -> serverWithoutPrompts.removePrompt(TEST_PROMPT_NAME)).isInstanceOf(McpError.class)
404+
assertThatThrownBy(() -> serverWithoutPrompts.removePrompt(TEST_PROMPT_NAME))
405+
.isInstanceOf(IllegalStateException.class)
401406
.hasMessage("Server must be configured with prompt capabilities");
402407
}
403408

@@ -426,7 +431,8 @@ void testRemoveNonexistentPrompt() {
426431
.capabilities(ServerCapabilities.builder().prompts(true).build())
427432
.build();
428433

429-
assertThatThrownBy(() -> mcpSyncServer.removePrompt("nonexistent-prompt")).isInstanceOf(McpError.class)
434+
assertThatThrownBy(() -> mcpSyncServer.removePrompt("nonexistent-prompt"))
435+
.isInstanceOf(IllegalArgumentException.class)
430436
.hasMessage("Prompt with name 'nonexistent-prompt' not found");
431437

432438
assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();

0 commit comments

Comments
 (0)