fix java mcp message endpoint #75
Conversation
WalkthroughThe pull request introduces a new boolean field, Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code Definitions (1)server/sse.go (2)
🔇 Additional comments (5)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
server/sse.go (3)
110-115: Enhance documentation for the WithIsCompleteMessageEndpoint function.The current comment is brief and doesn't fully explain the purpose of this configuration option. Consider adding more context about why this option exists and when it should be used.
-// WithIsCompleteMessageEndpoint sets the flag for whether the endpoint is for complete messages or not +// WithIsCompleteMessageEndpoint controls whether the SSE server returns a complete URL (including baseURL) +// or just the path portion for the message endpoint. Set to false when clients will concatenate +// the baseURL themselves to avoid malformed URLs like "http://localhost/mcphttp://localhost/mcp/message".
256-260: Well-implemented conditional logic for message endpoint construction.The implementation correctly uses either the complete URL or just the path based on the
isCompleteMessageEndpointflag. This effectively addresses the issue with Java MCP SDK creating malformed URLs.Consider adding a brief comment to explain the purpose of this conditional logic:
messageEndpoint := s.messageEndpoint +// Use either just the path or the complete URL based on configuration. +// This prevents issues with clients that concatenate the base URL themselves. if s.isCompleteMessageEndpoint { messageEndpoint = s.CompleteMessageEndpoint() } messageEndpoint = fmt.Sprintf("%s?sessionId=%s", messageEndpoint, sessionID)
58-60: Consider a more descriptive field name.While
isCompleteMessageEndpointworks, a more descriptive name likeuseFullURLForMessageEndpointmight better communicate its purpose, as it controls whether to use the full URL (including base URL) or just the path portion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/sse.go(4 hunks)server/sse_test.go(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
server/sse.go (2)
server/server.go (1)
MCPServer(138-152)examples/custom_context/main.go (1)
MCPServer(100-102)
🔇 Additional comments (4)
server/sse_test.go (2)
421-494: Good implementation of test case for the newisCompleteMessageEndpointbehavior.This test case effectively verifies the behavior when
isCompleteMessageEndpointis set to false. The test properly configures the server, makes the SSE connection, and then verifies that the message URL starts with the expected message endpoint. It also correctly demonstrates how to use the message URL by prepending the base URL when making the POST request.
699-723: Well-done consistency check for the new configuration option.The addition of
isCompleteMessageEndpointto the random options test ensures that this new configuration option is properly applied and maintains consistency when used alongside other options. This helps prevent regressions when multiple configuration options are used together.server/sse.go (2)
59-59: Appropriate addition ofisCompleteMessageEndpointfield to the SSEServer struct.The new field addresses the issue mentioned in the PR objectives where the Java MCP SDK was creating malformed URLs by concatenating the base URL with a message endpoint that already included a full URL.
145-145: Good default value for backward compatibility.Setting
isCompleteMessageEndpointto true by default ensures backward compatibility with existing clients, which is important for a fix like this.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/sse.go (1)
257-263: Correct implementation of the URL generation logicThe conditional logic correctly implements the feature toggle, using either the complete URL or just the path based on the configuration. This effectively solves the issue mentioned in the PR objectives where Java clients were experiencing malformed URLs.
Consider creating a helper method that encapsulates this logic, such as
GetMessageEndpointForClient(sessionID string) string, which could be reused elsewhere if needed.+// GetMessageEndpointForClient returns the appropriate message endpoint URL with session ID +// based on the useFullURLForMessageEndpoint configuration. +func (s *SSEServer) GetMessageEndpointForClient(sessionID string) string { + messageEndpoint := s.messageEndpoint + if s.useFullURLForMessageEndpoint { + messageEndpoint = s.CompleteMessageEndpoint() + } + return fmt.Sprintf("%s?sessionId=%s", messageEndpoint, sessionID) +}And then update the handleSSE method to use it:
-// Use either just the path or the complete URL based on configuration. -// This prevents issues with clients that concatenate the base URL themselves. -messageEndpoint := s.messageEndpoint -if s.useFullURLForMessageEndpoint { - messageEndpoint = s.CompleteMessageEndpoint() -} -messageEndpoint = fmt.Sprintf("%s?sessionId=%s", messageEndpoint, sessionID) +messageEndpoint := s.GetMessageEndpointForClient(sessionID)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/sse.go(4 hunks)server/sse_test.go(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
server/sse_test.go (2)
server/server.go (1)
NewMCPServer(326-352)server/sse.go (3)
NewSSEServer(142-156)SSEOption(67-67)WithUseFullURLForMessageEndpoint(113-117)
🔇 Additional comments (5)
server/sse.go (3)
59-59: Good addition of the useFullURLForMessageEndpoint fieldThis new field provides a clean solution to the Java MCP SDK issue mentioned in the PR objectives, allowing clients to control whether they receive a full URL or just the path.
110-117: Well-documented configuration optionThe comment clearly explains the purpose of this option and provides a good example of the issue it solves. The implementation follows the same pattern as other SSEOption functions, maintaining consistency.
147-147: Appropriate default value for backward compatibilitySetting
useFullURLForMessageEndpointtotrueby default ensures backward compatibility with existing clients, which is a good approach for introducing this new parameter.server/sse_test.go (2)
421-494: Good test coverage for the new featureThe test case effectively validates the behavior when
useFullURLForMessageEndpointis set to false. It properly checks that:
- The SSE connection works
- The message endpoint URL is correctly formatted without the full URL
- The client can successfully post to the endpoint by concatenating the base URL
This test ensures the feature works as expected for Java MCP SDK users.
699-700: Comprehensive test for option consistencyThe existing test for SSEOption consistency has been appropriately updated to include the new option. This ensures that the option behaves correctly when applied multiple times in different orders.
Also applies to: 705-705, 721-723
Because the implementation of java mcp sdk does not handle the problem of messageEndpoint returning CompleteMessageEndpoint URI when splicing messageEndpoint and base URL, it simply splices, resulting in the phenomenon of http://localhost/mcphttp://localhost/mcp/message.
In order to be compatible with it, I added a new parameter and kept the original logic unchanged to avoid affecting users who are already using it.
Here are the references:
https://github.com/modelcontextprotocol/java-sdk/releases/tag/v0.8.1
Summary by CodeRabbit