Skip to content

feat(mcp): Refactor MCP server API to use Specification pattern#2508

Closed
tzolov wants to merge 7 commits intospring-projects:mainfrom
tzolov:mcp-0.8.0-migration
Closed

feat(mcp): Refactor MCP server API to use Specification pattern#2508
tzolov wants to merge 7 commits intospring-projects:mainfrom
tzolov:mcp-0.8.0-migration

Conversation

@tzolov
Copy link
Copy Markdown
Contributor

@tzolov tzolov commented Mar 18, 2025

  • 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.

@tzolov tzolov added the MCP label Mar 18, 2025
@tzolov tzolov added this to the 1.0.0-M7 milestone Mar 18, 2025
@tzolov
Copy link
Copy Markdown
Contributor Author

tzolov commented Mar 18, 2025

Depends on modelcontextprotocol/java-sdk#31

Copy link
Copy Markdown
Member

@chemicL chemicL left a 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!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

typo: specificaitons -> specifications

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

specificaitons -> specifications

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

will do

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

registration -> specification

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

toolCallbacks2 -> toolCallbacks ?

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

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.

@tzolov
Copy link
Copy Markdown
Contributor Author

tzolov commented Mar 19, 2025

Thanks for the review @chemicL . Last commit should address the review comments

Copy link
Copy Markdown
Member

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))

Copy link
Copy Markdown
Contributor Author

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.

@tzolov tzolov force-pushed the mcp-0.8.0-migration branch 2 times, most recently from 46acf04 to fd1ae33 Compare March 22, 2025 08:43
@tzolov tzolov marked this pull request as ready for review March 22, 2025 08:45
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
public List<McpServerFeatures.SyncToolSpecification> syncToolsRegistrationToSpecificaiton(
public List<McpServerFeatures.SyncToolSpecification> syncToolsRegistrationToSpecification(

Several method names contain a typo: "Specificaiton" instead of "Specification".

@tzolov tzolov force-pushed the mcp-0.8.0-migration branch from 5a001b8 to 5320640 Compare March 24, 2025 15:29
@markpollack markpollack self-assigned this Mar 24, 2025
tzolov added 6 commits March 25, 2025 11:00
- 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 <christian.tzolov@broadcom.com>
Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
…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 <christian.tzolov@broadcom.com>
Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
@tzolov tzolov force-pushed the mcp-0.8.0-migration branch from 5320640 to 55a11f6 Compare March 25, 2025 10:04
- 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 <christian.tzolov@broadcom.com>
@markpollack
Copy link
Copy Markdown
Member

squashed and merged in fc955c7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants