Skip to content

refactor: deprecate tool spec's arguments-only handler in favor of CallToolRequest #375

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+1,680 −698
Open

refactor: deprecate tool spec's arguments-only handler in favor of CallToolRequest #375

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
371 changes: 206 additions & 165 deletions .../mcp-spring-webflux/src/test/java/io/modelcontextprotocol/WebFluxSseIntegrationTests.java
View file
Open in desktop

Large diffs are not rendered by default.

374 changes: 198 additions & 176 deletions ...spring-webmvc/src/test/java/io/modelcontextprotocol/server/WebMvcSseIntegrationTests.java
View file
Open in desktop

Large diffs are not rendered by default.

100 changes: 97 additions & 3 deletions mcp-test/src/main/java/io/modelcontextprotocol/server/AbstractMcpAsyncServerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
*
* @author Christian Tzolov
*/
// KEEP IN SYNC with the class in mcp-test module
public abstract class AbstractMcpAsyncServerTests {

private static final String TEST_TOOL_NAME = "test-tool";
Expand Down Expand Up @@ -102,6 +101,7 @@ void testImmediateClose() {
""";

@Test
@Deprecated
void testAddTool() {
Tool newTool = new McpSchema.Tool("new-tool", "New test tool", emptyJsonSchema);
var mcpAsyncServer = McpServer.async(createMcpTransportProvider())
Expand All @@ -117,6 +117,23 @@ void testAddTool() {
}

@Test
void testAddToolCall() {
Tool newTool = new McpSchema.Tool("new-tool", "New test tool", emptyJsonSchema);
var mcpAsyncServer = McpServer.async(createMcpTransportProvider())
.serverInfo("test-server", "1.0.0")
.capabilities(ServerCapabilities.builder().tools(true).build())
.build();

StepVerifier.create(mcpAsyncServer.addTool(McpServerFeatures.AsyncToolSpecification.builder()
.tool(newTool)
.callTool((exchange, request) -> Mono.just(new CallToolResult(List.of(), false)))
.build())).verifyComplete();

assertThatCode(() -> mcpAsyncServer.closeGracefully().block(Duration.ofSeconds(10))).doesNotThrowAnyException();
}

@Test
@Deprecated
void testAddDuplicateTool() {
Tool duplicateTool = new McpSchema.Tool(TEST_TOOL_NAME, "Duplicate tool", emptyJsonSchema);

Expand All @@ -137,14 +154,91 @@ void testAddDuplicateTool() {
assertThatCode(() -> mcpAsyncServer.closeGracefully().block(Duration.ofSeconds(10))).doesNotThrowAnyException();
}

@Test
void testAddDuplicateToolCall() {
Tool duplicateTool = new McpSchema.Tool(TEST_TOOL_NAME, "Duplicate tool", emptyJsonSchema);

var mcpAsyncServer = McpServer.async(createMcpTransportProvider())
.serverInfo("test-server", "1.0.0")
.capabilities(ServerCapabilities.builder().tools(true).build())
.toolCall(duplicateTool, (exchange, request) -> Mono.just(new CallToolResult(List.of(), false)))
.build();

StepVerifier.create(mcpAsyncServer.addTool(McpServerFeatures.AsyncToolSpecification.builder()
.tool(duplicateTool)
.callTool((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");
});

assertThatCode(() -> mcpAsyncServer.closeGracefully().block(Duration.ofSeconds(10))).doesNotThrowAnyException();
}

@Test
void testDuplicateToolCallDuringBuilding() {
Tool duplicateTool = new Tool("duplicate-build-toolcall", "Duplicate toolcall during building",
emptyJsonSchema);

assertThatThrownBy(() -> McpServer.async(createMcpTransportProvider())
.serverInfo("test-server", "1.0.0")
.capabilities(ServerCapabilities.builder().tools(true).build())
.toolCall(duplicateTool, (exchange, request) -> Mono.just(new CallToolResult(List.of(), false)))
.toolCall(duplicateTool, (exchange, request) -> Mono.just(new CallToolResult(List.of(), false))) // Duplicate!
.build()).isInstanceOf(IllegalArgumentException.class)
.hasMessage("Tool with name 'duplicate-build-toolcall' is already registered.");
}

@Test
void testDuplicateToolsInBatchListRegistration() {
Tool duplicateTool = new Tool("batch-list-tool", "Duplicate tool in batch list", emptyJsonSchema);
List<McpServerFeatures.AsyncToolSpecification> specs = List.of(
McpServerFeatures.AsyncToolSpecification.builder()
.tool(duplicateTool)
.callTool((exchange, request) -> Mono.just(new CallToolResult(List.of(), false)))
.build(),
McpServerFeatures.AsyncToolSpecification.builder()
.tool(duplicateTool)
.callTool((exchange, request) -> Mono.just(new CallToolResult(List.of(), false)))
.build() // Duplicate!
);

assertThatThrownBy(() -> McpServer.async(createMcpTransportProvider())
.serverInfo("test-server", "1.0.0")
.capabilities(ServerCapabilities.builder().tools(true).build())
.tools(specs)
.build()).isInstanceOf(IllegalArgumentException.class)
.hasMessage("Tool with name 'batch-list-tool' is already registered.");
}

@Test
void testDuplicateToolsInBatchVarargsRegistration() {
Tool duplicateTool = new Tool("batch-varargs-tool", "Duplicate tool in batch varargs", emptyJsonSchema);

assertThatThrownBy(() -> McpServer.async(createMcpTransportProvider())
.serverInfo("test-server", "1.0.0")
.capabilities(ServerCapabilities.builder().tools(true).build())
.tools(McpServerFeatures.AsyncToolSpecification.builder()
.tool(duplicateTool)
.callTool((exchange, request) -> Mono.just(new CallToolResult(List.of(), false)))
.build(),
McpServerFeatures.AsyncToolSpecification.builder()
.tool(duplicateTool)
.callTool((exchange, request) -> Mono.just(new CallToolResult(List.of(), false)))
.build() // Duplicate!
)
.build()).isInstanceOf(IllegalArgumentException.class)
.hasMessage("Tool with name 'batch-varargs-tool' is already registered.");
}

@Test
void testRemoveTool() {
Tool too = new McpSchema.Tool(TEST_TOOL_NAME, "Duplicate tool", emptyJsonSchema);

var mcpAsyncServer = McpServer.async(createMcpTransportProvider())
.serverInfo("test-server", "1.0.0")
.capabilities(ServerCapabilities.builder().tools(true).build())
.tool(too, (exchange, args) -> Mono.just(new CallToolResult(List.of(), false)))
.toolCall(too, (exchange, args) -> Mono.just(new CallToolResult(List.of(), false)))
.build();

StepVerifier.create(mcpAsyncServer.removeTool(TEST_TOOL_NAME)).verifyComplete();
Expand Down Expand Up @@ -173,7 +267,7 @@ void testNotifyToolsListChanged() {
var mcpAsyncServer = McpServer.async(createMcpTransportProvider())
.serverInfo("test-server", "1.0.0")
.capabilities(ServerCapabilities.builder().tools(true).build())
.tool(too, (exchange, args) -> Mono.just(new CallToolResult(List.of(), false)))
.toolCall(too, (exchange, args) -> Mono.just(new CallToolResult(List.of(), false)))
.build();

StepVerifier.create(mcpAsyncServer.notifyToolsListChanged()).verifyComplete();
Expand Down
113 changes: 103 additions & 10 deletions mcp-test/src/main/java/io/modelcontextprotocol/server/AbstractMcpSyncServerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,16 @@

package io.modelcontextprotocol.server;

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

import java.util.List;

import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import io.modelcontextprotocol.spec.McpError;
import io.modelcontextprotocol.spec.McpSchema;
import io.modelcontextprotocol.spec.McpSchema.CallToolResult;
Expand All @@ -17,21 +25,13 @@
import io.modelcontextprotocol.spec.McpSchema.ServerCapabilities;
import io.modelcontextprotocol.spec.McpSchema.Tool;
import io.modelcontextprotocol.spec.McpServerTransportProvider;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

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

/**
* Test suite for the {@link McpSyncServer} that can be used with different
* {@link io.modelcontextprotocol.spec.McpServerTransportProvider} implementations.
* {@link McpTransportProvider} implementations.
*
* @author Christian Tzolov
*/
// KEEP IN SYNC with the class in mcp-test module
public abstract class AbstractMcpSyncServerTests {

private static final String TEST_TOOL_NAME = "test-tool";
Expand Down Expand Up @@ -109,6 +109,7 @@ void testGetAsyncServer() {
""";

@Test
@Deprecated
void testAddTool() {
var mcpSyncServer = McpServer.sync(createMcpTransportProvider())
.serverInfo("test-server", "1.0.0")
Expand All @@ -124,6 +125,23 @@ void testAddTool() {
}

@Test
void testAddToolCall() {
var mcpSyncServer = McpServer.sync(createMcpTransportProvider())
.serverInfo("test-server", "1.0.0")
.capabilities(ServerCapabilities.builder().tools(true).build())
.build();

Tool newTool = new McpSchema.Tool("new-tool", "New test tool", emptyJsonSchema);
assertThatCode(() -> mcpSyncServer.addTool(McpServerFeatures.SyncToolSpecification.builder()
.tool(newTool)
.callTool((exchange, request) -> new CallToolResult(List.of(), false))
.build())).doesNotThrowAnyException();

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

@Test
@Deprecated
void testAddDuplicateTool() {
Tool duplicateTool = new McpSchema.Tool(TEST_TOOL_NAME, "Duplicate tool", emptyJsonSchema);

Expand All @@ -141,14 +159,89 @@ void testAddDuplicateTool() {
assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
}

@Test
void testAddDuplicateToolCall() {
Tool duplicateTool = new McpSchema.Tool(TEST_TOOL_NAME, "Duplicate tool", emptyJsonSchema);

var mcpSyncServer = McpServer.sync(createMcpTransportProvider())
.serverInfo("test-server", "1.0.0")
.capabilities(ServerCapabilities.builder().tools(true).build())
.toolCall(duplicateTool, (exchange, request) -> new CallToolResult(List.of(), false))
.build();

assertThatThrownBy(() -> mcpSyncServer.addTool(McpServerFeatures.SyncToolSpecification.builder()
.tool(duplicateTool)
.callTool((exchange, request) -> new CallToolResult(List.of(), false))
.build())).isInstanceOf(McpError.class)
.hasMessage("Tool with name '" + TEST_TOOL_NAME + "' already exists");

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

@Test
void testDuplicateToolCallDuringBuilding() {
Tool duplicateTool = new Tool("duplicate-build-toolcall", "Duplicate toolcall during building",
emptyJsonSchema);

assertThatThrownBy(() -> McpServer.sync(createMcpTransportProvider())
.serverInfo("test-server", "1.0.0")
.capabilities(ServerCapabilities.builder().tools(true).build())
.toolCall(duplicateTool, (exchange, request) -> new CallToolResult(List.of(), false))
.toolCall(duplicateTool, (exchange, request) -> new CallToolResult(List.of(), false)) // Duplicate!
.build()).isInstanceOf(IllegalArgumentException.class)
.hasMessage("Tool with name 'duplicate-build-toolcall' is already registered.");
}

@Test
void testDuplicateToolsInBatchListRegistration() {
Tool duplicateTool = new Tool("batch-list-tool", "Duplicate tool in batch list", emptyJsonSchema);
List<McpServerFeatures.SyncToolSpecification> specs = List.of(
McpServerFeatures.SyncToolSpecification.builder()
.tool(duplicateTool)
.callTool((exchange, request) -> new CallToolResult(List.of(), false))
.build(),
McpServerFeatures.SyncToolSpecification.builder()
.tool(duplicateTool)
.callTool((exchange, request) -> new CallToolResult(List.of(), false))
.build() // Duplicate!
);

assertThatThrownBy(() -> McpServer.sync(createMcpTransportProvider())
.serverInfo("test-server", "1.0.0")
.capabilities(ServerCapabilities.builder().tools(true).build())
.tools(specs)
.build()).isInstanceOf(IllegalArgumentException.class)
.hasMessage("Tool with name 'batch-list-tool' is already registered.");
}

@Test
void testDuplicateToolsInBatchVarargsRegistration() {
Tool duplicateTool = new Tool("batch-varargs-tool", "Duplicate tool in batch varargs", emptyJsonSchema);

assertThatThrownBy(() -> McpServer.sync(createMcpTransportProvider())
.serverInfo("test-server", "1.0.0")
.capabilities(ServerCapabilities.builder().tools(true).build())
.tools(McpServerFeatures.SyncToolSpecification.builder()
.tool(duplicateTool)
.callTool((exchange, request) -> new CallToolResult(List.of(), false))
.build(),
McpServerFeatures.SyncToolSpecification.builder()
.tool(duplicateTool)
.callTool((exchange, request) -> new CallToolResult(List.of(), false))
.build() // Duplicate!
)
.build()).isInstanceOf(IllegalArgumentException.class)
.hasMessage("Tool with name 'batch-varargs-tool' is already registered.");
}

@Test
void testRemoveTool() {
Tool tool = new McpSchema.Tool(TEST_TOOL_NAME, "Test tool", emptyJsonSchema);

var mcpSyncServer = McpServer.sync(createMcpTransportProvider())
.serverInfo("test-server", "1.0.0")
.capabilities(ServerCapabilities.builder().tools(true).build())
.tool(tool, (exchange, args) -> new CallToolResult(List.of(), false))
.toolCall(tool, (exchange, args) -> new CallToolResult(List.of(), false))
.build();

assertThatCode(() -> mcpSyncServer.removeTool(TEST_TOOL_NAME)).doesNotThrowAnyException();
Expand Down
22 changes: 11 additions & 11 deletions mcp/src/main/java/io/modelcontextprotocol/server/McpAsyncServer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,18 +268,18 @@ private McpServerSession.NotificationHandler asyncRootsListChangedNotificationHa
// ---------------------------------------

/**
* Add a new tool specification at runtime.
* @param toolSpecification The tool specification to add
* Add a new tool call specification at runtime.
* @param toolCallSpecification The tool specification to add
* @return Mono that completes when clients have been notified of the change
*/
public Mono<Void> addTool(McpServerFeatures.AsyncToolSpecification toolSpecification) {
if (toolSpecification == null) {
public Mono<Void> addTool(McpServerFeatures.AsyncToolSpecification toolCallSpecification) {
if (toolCallSpecification == null) {
return Mono.error(new McpError("Tool specification must not be null"));
}
if (toolSpecification.tool() == null) {
if (toolCallSpecification.tool() == null) {
return Mono.error(new McpError("Tool must not be null"));
}
if (toolSpecification.call() == null) {
if (toolCallSpecification.call() == null && toolCallSpecification.callTool() == null) {
return Mono.error(new McpError("Tool call handler must not be null"));
}
if (this.serverCapabilities.tools() == null) {
Expand All @@ -288,13 +288,13 @@ public Mono<Void> addTool(McpServerFeatures.AsyncToolSpecification toolSpecifica

return Mono.defer(() -> {
// Check for duplicate tool names
if (this.tools.stream().anyMatch(th -> th.tool().name().equals(toolSpecification.tool().name()))) {
if (this.tools.stream().anyMatch(th -> th.tool().name().equals(toolCallSpecification.tool().name()))) {
return Mono
.error(new McpError("Tool with name '" + toolSpecification.tool().name() + "' already exists"));
.error(new McpError("Tool with name '" + toolCallSpecification.tool().name() + "' already exists"));
}

this.tools.add(toolSpecification);
logger.debug("Added tool handler: {}", toolSpecification.tool().name());
this.tools.add(toolCallSpecification);
logger.debug("Added tool handler: {}", toolCallSpecification.tool().name());

if (this.serverCapabilities.tools().listChanged()) {
return notifyToolsListChanged();
Expand Down Expand Up @@ -360,7 +360,7 @@ private McpServerSession.RequestHandler<CallToolResult> toolsCallRequestHandler(
return Mono.error(new McpError("Tool not found: " + callToolRequest.name()));
}

return toolSpecification.map(tool -> tool.call().apply(exchange, callToolRequest.arguments()))
return toolSpecification.map(tool -> tool.callTool().apply(exchange, callToolRequest))
.orElse(Mono.error(new McpError("Tool not found: " + callToolRequest.name())));
};
}
Expand Down
Loading