Skip to content

feat: refactor the tool specifications #351

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

Closed
tzolov wants to merge 3 commits into from
Closed

feat: refactor the tool specifications #351

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
37c342c
feat: refactor the tool specifications
tzolov Jun 29, 2025
3a1a1b4
feat: add duplicate tool name validation for MCP server registration
tzolov Jul 1, 2025
29ca07e
address review comments
tzolov Jul 2, 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
70 changes: 70 additions & 0 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 @@ -153,6 +153,76 @@ 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(new McpServerFeatures.AsyncToolCallSpecification(duplicateTool,
(exchange, request) -> Mono.just(new CallToolResult(List.of(), false)))))
.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.AsyncToolCallSpecification> specs = List.of(
new McpServerFeatures.AsyncToolCallSpecification(duplicateTool,
(exchange, request) -> Mono.just(new CallToolResult(List.of(), false))),
new McpServerFeatures.AsyncToolCallSpecification(duplicateTool,
(exchange, request) -> Mono.just(new CallToolResult(List.of(), false))) // Duplicate!
);

assertThatThrownBy(() -> McpServer.async(createMcpTransportProvider())
.serverInfo("test-server", "1.0.0")
.capabilities(ServerCapabilities.builder().tools(true).build())
.toolCalls(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())
.toolCalls(
new McpServerFeatures.AsyncToolCallSpecification(duplicateTool,
(exchange, request) -> Mono.just(new CallToolResult(List.of(), false))),
new McpServerFeatures.AsyncToolCallSpecification(duplicateTool,
(exchange, request) -> Mono.just(new CallToolResult(List.of(), false))) // 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);
Expand Down
68 changes: 68 additions & 0 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 @@ -140,6 +140,7 @@ void testAddToolCall() {
}

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

Expand All @@ -157,6 +158,73 @@ 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(new McpServerFeatures.SyncToolCallSpecification(duplicateTool,
(exchange, request) -> new CallToolResult(List.of(), false))))
.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.SyncToolCallSpecification> specs = List.of(
new McpServerFeatures.SyncToolCallSpecification(duplicateTool,
(exchange, request) -> new CallToolResult(List.of(), false)),
new McpServerFeatures.SyncToolCallSpecification(duplicateTool,
(exchange, request) -> new CallToolResult(List.of(), false)) // Duplicate!
);

assertThatThrownBy(() -> McpServer.sync(createMcpTransportProvider())
.serverInfo("test-server", "1.0.0")
.capabilities(ServerCapabilities.builder().tools(true).build())
.toolCalls(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())
.toolCalls(
new McpServerFeatures.SyncToolCallSpecification(duplicateTool,
(exchange, request) -> new CallToolResult(List.of(), false)),
new McpServerFeatures.SyncToolCallSpecification(duplicateTool,
(exchange, request) -> new CallToolResult(List.of(), false)) // 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);
Expand Down
52 changes: 49 additions & 3 deletions mcp/src/main/java/io/modelcontextprotocol/server/McpServer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,12 @@ private AsyncSpecification(McpServerTransportProvider transportProvider) {
this.transportProvider = transportProvider;
}

private void AssertNotDupplicateTool(String toolName) {
Copy link
Copy Markdown
Contributor

@LucaButBoring LucaButBoring Jul 1, 2025

Choose a reason for hiding this comment

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

nit: method name -> assertNotDuplicateTool

if (this.tools.stream().anyMatch(toolSpec -> toolSpec.tool().name().equals(toolName))) {
throw new IllegalArgumentException("Tool with name '" + toolName + "' is already registered.");
}
}

/**
* Sets the URI template manager factory to use for creating URI templates. This
* allows for custom URI template parsing and variable extraction.
Expand Down Expand Up @@ -329,6 +335,7 @@ public AsyncSpecification tool(McpSchema.Tool tool,
BiFunction<McpAsyncServerExchange, Map<String, Object>, Mono<CallToolResult>> handler) {
Assert.notNull(tool, "Tool must not be null");
Assert.notNull(handler, "Handler must not be null");
AssertNotDupplicateTool(tool.name());

this.tools.add(new McpServerFeatures.AsyncToolSpecification(tool, handler).toToolCall());

Expand All @@ -353,6 +360,7 @@ public AsyncSpecification toolCall(McpSchema.Tool tool,

Assert.notNull(tool, "Tool must not be null");
Assert.notNull(handler, "Handler must not be null");
AssertNotDupplicateTool(tool.name());

this.tools.add(new McpServerFeatures.AsyncToolCallSpecification(tool, handler));

Expand All @@ -374,6 +382,12 @@ public AsyncSpecification toolCall(McpSchema.Tool tool,
@Deprecated
public AsyncSpecification tools(List<McpServerFeatures.AsyncToolSpecification> toolSpecifications) {
Assert.notNull(toolSpecifications, "Tool handlers list must not be null");

for (var tool : toolSpecifications) {
AssertNotDupplicateTool(tool.tool().name());
this.tools.add(tool.toToolCall());
}

this.tools.addAll(toolSpecifications.stream().map(s -> s.toToolCall()).toList());
return this;
}
Expand All @@ -390,7 +404,11 @@ public AsyncSpecification tools(List<McpServerFeatures.AsyncToolSpecification> t
*/
public AsyncSpecification toolCalls(List<McpServerFeatures.AsyncToolCallSpecification> toolCallSpecifications) {
Assert.notNull(toolCallSpecifications, "Tool handlers list must not be null");
this.tools.addAll(toolCallSpecifications);

for (var tool : toolCallSpecifications) {
AssertNotDupplicateTool(tool.tool().name());
this.tools.add(tool);
}
return this;
}
Comment on lines +391 to 407
Copy link
Copy Markdown
Contributor

@LucaButBoring LucaButBoring Jun 30, 2025

Choose a reason for hiding this comment

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

Hadn't considered until reviewing this just now, but this should probably do some minimal validation for things like duplicate tool names. Unsure if that's already planned as a more comprehensive backlog thing or if we should do this now.

Copy link
Copy Markdown
Contributor Author

@tzolov tzolov Jul 1, 2025

Choose a reason for hiding this comment

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

Thanks @LucaButBoring , while we had some duplication validation for the runtime tool addition we lacked such for the initial tool registration.
I've updated the PR to add duplication validation.


Expand All @@ -415,7 +433,9 @@ public AsyncSpecification toolCalls(List<McpServerFeatures.AsyncToolCallSpecific
@Deprecated
public AsyncSpecification tools(McpServerFeatures.AsyncToolSpecification... toolSpecifications) {
Assert.notNull(toolSpecifications, "Tool handlers list must not be null");

for (McpServerFeatures.AsyncToolSpecification tool : toolSpecifications) {
AssertNotDupplicateTool(tool.tool().name());
this.tools.add(tool.toToolCall());
}
return this;
Expand All @@ -430,7 +450,9 @@ public AsyncSpecification tools(McpServerFeatures.AsyncToolSpecification... tool
*/
public AsyncSpecification toolCalls(McpServerFeatures.AsyncToolCallSpecification... toolCallSpecifications) {
Assert.notNull(toolCallSpecifications, "Tool handlers list must not be null");

for (McpServerFeatures.AsyncToolCallSpecification tool : toolCallSpecifications) {
AssertNotDupplicateTool(tool.tool().name());
this.tools.add(tool);
}
return this;
Expand Down Expand Up @@ -763,6 +785,12 @@ private SyncSpecification(McpServerTransportProvider transportProvider) {
this.transportProvider = transportProvider;
}

private void AssertNotDupplicateTool(String toolName) {
if (this.tools.stream().anyMatch(toolSpec -> toolSpec.tool().name().equals(toolName))) {
throw new IllegalArgumentException("Tool with name '" + toolName + "' is already registered.");
}
}

/**
* Sets the URI template manager factory to use for creating URI templates. This
* allows for custom URI template parsing and variable extraction.
Expand Down Expand Up @@ -883,6 +911,7 @@ public SyncSpecification tool(McpSchema.Tool tool,
BiFunction<McpSyncServerExchange, Map<String, Object>, McpSchema.CallToolResult> handler) {
Assert.notNull(tool, "Tool must not be null");
Assert.notNull(handler, "Handler must not be null");
AssertNotDupplicateTool(tool.name());

this.tools.add(new McpServerFeatures.SyncToolSpecification(tool, handler).toToolCall());

Expand All @@ -906,6 +935,7 @@ public SyncSpecification toolCall(McpSchema.Tool tool,
BiFunction<McpSyncServerExchange, McpSchema.CallToolRequest, McpSchema.CallToolResult> handler) {
Assert.notNull(tool, "Tool must not be null");
Assert.notNull(handler, "Handler must not be null");
AssertNotDupplicateTool(tool.name());

this.tools.add(new McpServerFeatures.SyncToolCallSpecification(tool, handler));

Expand All @@ -927,7 +957,14 @@ public SyncSpecification toolCall(McpSchema.Tool tool,
@Deprecated
public SyncSpecification tools(List<McpServerFeatures.SyncToolSpecification> toolSpecifications) {
Assert.notNull(toolSpecifications, "Tool handlers list must not be null");
this.tools.addAll(toolSpecifications.stream().map(s -> s.toToolCall()).toList());

for (var tool : toolSpecifications) {
String toolName = tool.tool().name();
AssertNotDupplicateTool(toolName); // Check against existing tools
this.tools.add(tool.toToolCall()); // Add the tool call specification
// directly
}

return this;
}

Expand All @@ -942,7 +979,12 @@ public SyncSpecification tools(List<McpServerFeatures.SyncToolSpecification> too
*/
public SyncSpecification toolCalls(List<McpServerFeatures.SyncToolCallSpecification> toolCallSpecifications) {
Assert.notNull(toolCallSpecifications, "Tool handlers list must not be null");
this.tools.addAll(toolCallSpecifications);

for (var tool : toolCallSpecifications) {
AssertNotDupplicateTool(tool.tool().name());
this.tools.add(tool);
}

return this;
}

Expand All @@ -969,7 +1011,9 @@ public SyncSpecification toolCalls(List<McpServerFeatures.SyncToolCallSpecificat
@Deprecated
public SyncSpecification tools(McpServerFeatures.SyncToolSpecification... toolSpecifications) {
Assert.notNull(toolSpecifications, "Tool handlers list must not be null");

for (McpServerFeatures.SyncToolSpecification tool : toolSpecifications) {
AssertNotDupplicateTool(tool.tool().name());
this.tools.add(tool.toToolCall());
}
return this;
Expand All @@ -985,7 +1029,9 @@ public SyncSpecification tools(McpServerFeatures.SyncToolSpecification... toolSp
*/
public SyncSpecification toolCalls(McpServerFeatures.SyncToolCallSpecification... toolCallSpecifications) {
Assert.notNull(toolCallSpecifications, "Tool handlers list must not be null");

for (McpServerFeatures.SyncToolCallSpecification tool : toolCallSpecifications) {
AssertNotDupplicateTool(tool.tool().name());
this.tools.add(tool);
}
return this;
Expand Down
49 changes: 49 additions & 0 deletions mcp/src/test/java/io/modelcontextprotocol/server/AbstractMcpAsyncServerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,55 @@ void testAddDuplicateToolCall() {
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.AsyncToolCallSpecification> specs = List.of(
new McpServerFeatures.AsyncToolCallSpecification(duplicateTool,
(exchange, request) -> Mono.just(new CallToolResult(List.of(), false))),
new McpServerFeatures.AsyncToolCallSpecification(duplicateTool,
(exchange, request) -> Mono.just(new CallToolResult(List.of(), false))) // Duplicate!
);

assertThatThrownBy(() -> McpServer.async(createMcpTransportProvider())
.serverInfo("test-server", "1.0.0")
.capabilities(ServerCapabilities.builder().tools(true).build())
.toolCalls(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())
.toolCalls(
new McpServerFeatures.AsyncToolCallSpecification(duplicateTool,
(exchange, request) -> Mono.just(new CallToolResult(List.of(), false))),
new McpServerFeatures.AsyncToolCallSpecification(duplicateTool,
(exchange, request) -> Mono.just(new CallToolResult(List.of(), false))) // 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);
Expand Down
Loading
Loading