feat: refactor the tool specifications #351
Conversation
- Deprecate AsyncToolSpecification in favor of AsyncToolCallSpecification - Deprecate SyncToolSpecification in favor of SyncToolCallSpecification - Tool handlers now receive CallToolRequest instead of raw arguments to enable access to additional metadata parameters like progressToken - Add toolCall() and toolCalls() builder methods alongside deprecated variants - Update addTool() methods to support new tool call specifications - Maintain backward compatibility through deprecation annotations - Add tests for new tool call functionality This change allows tool handlers to process additional metadata parameters from the full CallToolRequest object, enabling features like progress tracking. Signed-off-by: Christian Tzolov <[email protected]>
| public AsyncSpecification toolCalls(List<McpServerFeatures.AsyncToolCallSpecification> toolCallSpecifications) { | ||
| Assert.notNull(toolCallSpecifications, "Tool handlers list must not be null"); | ||
| this.tools.addAll(toolCallSpecifications); | ||
| return this; | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
- Add AssertNotDupplicateTool validation method to prevent duplicate tool registration - Throw IllegalArgumentException during server building when duplicate tool names detected - Throw McpError when attempting to add duplicate tools to existing server - Add test coverage for duplicate detection scenarios: - Individual tool registration during building - Batch tool registration (list and varargs) - Runtime duplicate tool addition - Apply validation to both sync and async server implementations This prevents tool name conflicts that could cause unexpected behavior or runtime errors in MCP server implementations. Signed-off-by: Christian Tzolov <[email protected]>
| @@ -208,6 +208,12 @@ private AsyncSpecification(McpServerTransportProvider transportProvider) { | |||
| this.transportProvider = transportProvider; | |||
| } | |||
|
|
|||
| private void AssertNotDupplicateTool(String toolName) { | |||
There was a problem hiding this comment.
nit: method name -> assertNotDuplicateTool
Signed-off-by: Christian Tzolov <[email protected]>
|
Replaced by #375 @134130 , @LucaButBoring , @chemicL I've re-designed the the approach. |
Motivation and Context
This change allows tool handlers to process additional metadata parameters from the full CallToolRequest object, enabling features like progress tracking.
Related to #300
How Has This Been Tested?
Additional tests to
AbstractMcpAsyncClientTestsandAbstractMcpSyncClientTestsBreaking Changes
AsyncToolCallSpecification/SyncToolCallSpecificationreplace the deprecatedAsyncToolSpecification/SyncToolSpecification.The old specifications are maintained for backward compatibility during transition. Internally, the deprecated specifications are converted to the new format, but only arguments remain visible to
CallToolRequest.Types of changes
Checklist
Additional context