-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(mcp): Refactor MCP server API to use Specification pattern #2508
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.
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
Conversation
Depends on modelcontextprotocol/java-sdk#31 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the hard work in adapting the changes :) I left a few cosmetic remarks, but overall it will be awesome!
@@ -98,11 +98,11 @@ All properties are prefixed with `spring.ai.mcp.server`: | |||
* **Synchronous Server** - The default server type implemented using `McpSyncServer`. | |||
It is designed for straightforward request-response patterns in your applications. | |||
To enable this server type, set `spring.ai.mcp.server.type=SYNC` in your configuration. | |||
When activated, it automatically handles the configuration of synchronous tool registrations. | |||
When activated, it automatically handles the configuration of synchronous tool specificaitons. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: specificaitons -> specifications
|
||
* **Asynchronous Server** - The asynchronous server implementation uses `McpAsyncServer` and is optimized for non-blocking operations. | ||
To enable this server type, configure your application with `spring.ai.mcp.server.type=ASYNC`. | ||
This server type automatically sets up asynchronous tool registrations with built-in Project Reactor support. | ||
This server type automatically sets up asynchronous tool specificaitons with built-in Project Reactor support. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
specificaitons -> specifications
@@ -115,14 +115,14 @@ The MCP Server supports three transport mechanisms, each with its dedicated star | |||
== Features and Capabilities | |||
|
|||
The MCP Server Boot Starter allows servers to expose tools, resources, and prompts to clients. | |||
It automatically converts custom capability handlers registered as Spring beans to sync/async registrations based on server type: | |||
It automatically converts custom capability handlers registered as Spring beans to sync/async specificaitons based on server type: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another typo.. and more in this file
public static List<McpServerFeatures.SyncToolRegistration> toSyncToolRegistration( | ||
List<ToolCallback> toolCallbacks) { | ||
return toolCallbacks.stream().map(McpToolUtils::toSyncToolRegistration).toList(); | ||
} | ||
|
||
/** | ||
* Converts a list of Spring AI tool callbacks to MCP synchronous tool specificaiton. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another typo for specification - perhaps worth running a spell checker on all the changes in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do
* Converts a list of Spring AI tool callbacks to MCP synchronous tool specificaiton. | ||
* <p> | ||
* This method processes multiple tool callbacks in bulk, converting each one to its | ||
* corresponding MCP tool registration while maintaining synchronous execution |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
registration -> specification
return List.of(); | ||
} | ||
|
||
return rootsChangeConsumers.stream().map(c -> new BiConsumer<McpSyncServerExchange, List<McpSchema.Root>>() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason for not using a lambda expression here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do it but it requires explicit casting that I don't like. Anyway this is a deprecated throw away
@@ -94,17 +97,18 @@ | |||
* </ul> | |||
* <p> | |||
* WebMvc transport support is provided separately by | |||
* {@link McpWebMvcServerAutoConfiguration}. | |||
* {@link McpWebFluxServerAutoConfiguration}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was correct before if MVC is meant as the line above indicates
McpServerProperties serverProperties) { | ||
List<ToolCallback> tools = toolCalls.stream().flatMap(List::stream).toList(); | ||
public List<McpServerFeatures.SyncToolSpecification> syncTools(ObjectProvider<List<ToolCallback>> toolCalls, | ||
List<ToolCallback> toolCallbacks2, McpServerProperties serverProperties) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toolCallbacks2 -> toolCallbacks ?
McpServerProperties serverProperties) { | ||
var tools = toolCalls.stream().flatMap(List::stream).toList(); | ||
public List<McpServerFeatures.AsyncToolSpecification> asyncTools(ObjectProvider<List<ToolCallback>> toolCalls, | ||
List<ToolCallback> toolCallbacks2, McpServerProperties serverProperties) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does the toolCallbacks name need to change to one suffixed by 2?
|
||
org.springframework.ai.autoconfigure.mcp.server.McpServerAutoConfiguration2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requires cleanup I suppose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, this is a blunder. renamed the classes but forgot to fix the configuration.
Thanks for the review @chemicL . Last commit should address the review comments |
} | ||
}).collect(Collectors.toList()); | ||
return rootsChangeConsumers.stream() | ||
.map(c -> (BiConsumer<McpSyncServerExchange, List<McpSchema.Root>>) ((exchange, roots) -> c.accept(roots))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this work?
.map(c -> (McpSyncServerExchange exchange, List<McpSchema.Root> roots) -> c.accept(roots))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid not. Tried it before but it couldn't infer the types (or something like this).
This code will be removed after spring-ai M7 milestone release, so it is not that important.
46acf04
to
fd1ae33
Compare
} | ||
|
||
@Bean | ||
public List<McpServerFeatures.SyncToolSpecification> syncToolsRegistrationToSpecificaiton( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public List<McpServerFeatures.SyncToolSpecification> syncToolsRegistrationToSpecificaiton( | |
public List<McpServerFeatures.SyncToolSpecification> syncToolsRegistrationToSpecification( |
Several method names contain a typo: "Specificaiton" instead of "Specification".
5a001b8
to
5320640
Compare
- Rename Registration classes to Specification (SyncToolRegistration → SyncToolSpecification) - Update transport classes to use Provider suffix (WebFluxSseServerTransport → WebFluxSseServerTransportProvider) - Add exchange parameter to handler methods for better context passing - Introduce McpBackwardCompatibility class to maintain backward compatibility - Update MCP Server documentation to reflect new API patterns - Add tests for backward compatibility The changes align with the MCP specification evolution while maintaining backward compatibility through deprecated APIs. Signed-off-by: Christian Tzolov <[email protected]>
Signed-off-by: Christian Tzolov <[email protected]>
Signed-off-by: Christian Tzolov <[email protected]>
Signed-off-by: Christian Tzolov <[email protected]>
…configuration Extracts the MCP tool callback functionality from McpClientAutoConfiguration into a new dedicated McpToolCallbackAutoConfiguration that is disabled by default. - Created new McpToolCallbackAutoConfiguration class that handles tool callback registration - Made tool callbacks opt-in by requiring explicit configuration with spring.ai.mcp.client.toolcallback.enabled=true - Removed deprecated tool callback methods from McpClientAutoConfiguration - Updated ClientMcpTransport references to McpClientTransport to align with MCP library changes - Added tests for the new auto-configuration and its conditions Signed-off-by: Christian Tzolov <[email protected]>
Signed-off-by: Christian Tzolov <[email protected]>
5320640
to
55a11f6
Compare
- Change separator in McpToolUtils.prefixedToolName from hyphen to underscore - Add conversion of any remaining hyphens to underscores in formatted tool names - Update affected tests to reflect the new naming convention - Add comprehensive tests for McpToolUtils.prefixedToolName method - Add integration test for payment transaction tools with Vertex AI Gemini Signed-off-by: Christian Tzolov <[email protected]>
squashed and merged in fc955c7 |
The changes align with the MCP specification evolution while maintaining backward compatibility through deprecated APIs.