feat(sse): Add support for dynamic route parameters in SSE server #121
Conversation
|
""" WalkthroughThis pull request introduces a new example and updates the SSE server. A new Go file implements an SSE server using the MCP framework with custom connection context handling and a message handler. Enhancements in the server allow extraction and utilization of route parameters via a new pattern matching mechanism. A corresponding test case validates the dynamic routing functionality by checking proper parameter extraction and message processing. Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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_test.go (1)
749-755: Consider returning structured error information for missing channel.
The current approach returns a Goerrorifchannelis missing, which is fine for demonstration purposes. For production, consider returning a structured, JSON-RPC-compliant error code or documented error details for better client-side error handling.- return nil, fmt.Errorf("channel parameter not found") + return nil, mcp.NewJSONRPCError(mcp.INVALID_PARAMS, "Channel parameter not found")examples/custom_sse_pattern/main.go (1)
21-33: Graceful handling of missing route parameters.
Returning a fallback text result whenchannel == ""ensures the server avoids panics. Optionally, you might consider returning an error with an explicit code for better client-side error handling.server/sse.go (1)
504-527: Robust path matching method.
Splitting patterns on “/:” and storing them asRouteParamsis a simple approach. For advanced use cases, consider supporting wildcards or optional segments, but this is excellent for typical dynamic routes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/custom_sse_pattern/main.go(1 hunks)server/sse.go(12 hunks)server/sse_test.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
server/sse_test.go (5)
server/server.go (2)
NewMCPServer(328-354)WithResourceCapabilities(274-282)mcp/tools.go (3)
NewTool(127-142)CallToolRequest(44-59)CallToolResult(34-41)server/sse.go (4)
GetRouteParam(42-47)NewTestServer(190-199)WithSSEPattern(151-155)WithSSEContextFunc(166-170)mcp/utils.go (1)
NewToolResultText(208-217)mcp/types.go (1)
Request(89-102)
examples/custom_sse_pattern/main.go (3)
server/sse.go (5)
GetRouteParams(50-55)GetRouteParam(42-47)NewSSEServer(173-187)WithSSEPattern(151-155)WithSSEContextFunc(166-170)mcp/tools.go (3)
CallToolRequest(44-59)CallToolResult(34-41)NewTool(127-142)mcp/utils.go (1)
NewToolResultText(208-217)
🔇 Additional comments (18)
server/sse_test.go (5)
742-746: Test block naming and resource capabilities usage look good.
The newly added test block "Can handle custom route parameters" is clearly named, and initializing the MCP server withWithResourceCapabilities(true, true)aligns well with the new SSE functionalities.
757-763: Custom route pattern test is comprehensive.
Creating the SSE server withWithSSEPattern("/:channel/sse")and a context function ensures that any route parameter is parsed. This is a solid approach for dynamic routing.
766-789: SSE response validation is sufficient.
Connecting to/test-channel/sseand verifying theevent: endpointfield is a good sanity check. Everything looks correct.
790-819: Initialization request is well-formed.
The request structure follows typical JSON-RPC conventions, and the subsequent checks ensure the server processed it correctly.
820-874: Tool invocation test confirms valid route parameter usage.
Invoking the “test_route” tool and verifying the “Channel: test-channel” response effectively validates the SSE route parameters in a real call scenario. Great job ensuring the test covers both the SSE handshake and the tool usage.examples/custom_sse_pattern/main.go (2)
13-19: Effective logging of SSE connection details.
UsingGetRouteParamsand logging them provides clear visibility into dynamic route usage. This is helpful for debugging and auditing.
35-57: Main function is clear and demonstrates custom SSE usage.
Creating the SSE server withWithSSEPattern("/:channel/sse")and a custom context function is a solid example of how to handle dynamic routes. The console logs are beneficial for quick local tests.server/sse.go (11)
27-27: Storing route parameters in each session.
PersistingRouteParamsat the session level is a straightforward approach, allowing consistent retrieval in both SSE and message handling.
35-39: Context key and map-based parameter storage are reasonable.
DefiningRouteParamsKeyas an empty struct and storing parameters as a map ensures a lightweight, flexible approach for dynamic routing data.
41-47: Helper function cleanly retrieves a single parameter.
The direct cast and lookup is simple and maintains minimal overhead. Consider logging or hooking into a fallback if a parameter key is not found, though this is optional.
49-55: Helper function for retrieving all parameters.
This complementsGetRouteParamand eliminates repetitive code for multi-parameter scenarios. No issues found.
150-155: Customizable SSE route pattern.
IntroducingWithSSEPatternis a powerful way to define route structures. The parameter approach is consistent with typical Go router patterns.
266-268: Context function usage is properly integrated.
Applying a custom context after the standard SSE flow ensures the session is fully initialized before further context decoration.
352-353: Route parameters attached to message context.
Re-injecting route parameters here ensures message handlers see the same dynamic route data that SSE sessions used initially. Good consistency.
368-368: Routing context usage in message handling.
Callings.server.HandleMessage(ctx, rawMessage)with the updated context ensures that route data is always available for tool and method processing.
471-472: Clear distinction between message and SSE paths.
Early returning on the message path is logical before handling SSE. This helps avoid route collisions and confusion.
478-492: Pattern matching logic ensures dynamic SSE endpoints.
The fallback to a 404 if the pattern doesn’t match is a helpful way to surface misrouted requests. The approach is straightforward and maintainable.
494-500: Graceful fallback when no pattern is specified.
If no custom pattern is set, the server falls back to the default/sseendpoint. This maintains backward compatibility with existing code.
|
@ezynda3 Hi! My PR is ready for merge and all checks have passed. Could you please review and merge it into the main branch if everything looks good? Thank you! 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/sse.go (1)
541-564: Consider enhancing the path matching to handle edge cases.The
matchPathfunction works for standard route patterns but could be improved to handle more edge cases.func matchPath(pattern, path string) (bool, RouteParams) { patternParts := strings.Split(strings.Trim(pattern, "/"), "/") pathParts := strings.Split(strings.Trim(path, "/"), "/") if len(patternParts) != len(pathParts) { - return false, nil + return false, RouteParams{} } params := make(RouteParams) for i, part := range patternParts { - if strings.HasPrefix(part, ":") { + if part == "" { + // Skip empty parts + continue + } else if strings.HasPrefix(part, ":") { // This is a parameter paramName := strings.TrimPrefix(part, ":") + // Ensure parameter name is valid + if paramName == "" { + return false, RouteParams{} + } params[paramName] = pathParts[i] } else if part != pathParts[i] { // Static part doesn't match - return false, nil + return false, RouteParams{} } } return true, params }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/sse.go(13 hunks)
🔇 Additional comments (9)
server/sse.go (9)
28-28: Good addition of route parameters to the session.The addition of a
routeParamsfield to thesseSessionstruct allows for storing route parameters throughout the lifecycle of an SSE connection.
36-40: Good pattern for context key and parameter storage.The empty struct
RouteParamsKeyfollows Go best practices for context key types, and definingRouteParamsas a map makes the parameter storage intention clear.
42-56: Safe implementation of parameter retrieval functions.These helper functions are well implemented, properly handling cases where route parameters might not exist in the context by returning empty values rather than panicking.
85-85: LGTM: New field for pattern support.The
ssePatternfield is a good addition to support custom route patterns.
153-158: Good option function pattern.The
WithSSEPatternfunction follows the established option pattern in the codebase, making configuration intuitive and consistent.
273-273: Good use of context value extraction.Storing route parameters from the request context in the session ensures that they're available throughout the SSE connection's lifecycle.
279-287: Proper context handling with session and custom context function.This section correctly creates a context with the session first, then applies the custom context function if provided, ensuring the proper context chain.
387-390: Good context enrichment in message handling.Adding route parameters back to the context during message handling ensures parameters are available for message processing logic.
515-536: Well-structured pattern matching implementation.The implementation properly handles both pattern-based and traditional endpoint matching, with appropriate context creation and error handling.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
server/sse.go (3)
42-56: Well-designed utility functions for route parameters.The
GetRouteParamandGetRouteParamsfunctions provide a clean API for retrieving route parameters from the context. Consider adding more descriptive comments about the return values when keys are not found (empty string or empty map).// GetRouteParam retrieves a route parameter from context +// Returns the value for the given key, or an empty string if the key doesn't exist or no parameters are found func GetRouteParam(ctx context.Context, key string) string { // GetRouteParams retrieves all route parameters from context +// Returns all route parameters, or an empty map if no parameters are found in the context func GetRouteParams(ctx context.Context) RouteParams {
515-537: Pattern matching logic works but lacks trailing slash handling.The implementation for handling SSE endpoints with route parameters is logical, but it might behave inconsistently with trailing slashes.
Consider normalizing paths by trimming trailing slashes consistently before pattern matching:
// Handle SSE endpoint with route parameters if s.ssePattern != "" { // Try pattern matching if pattern is set fullPattern := s.basePath + s.ssePattern + // Normalize path by trimming trailing slash + normalizedPath := strings.TrimSuffix(path, "/") + normalizedPattern := strings.TrimSuffix(fullPattern, "/") - matches, params := matchPath(fullPattern, path) + matches, params := matchPath(normalizedPattern, normalizedPath) if matches {
541-564: Check for duplicate parameter names.The current implementation doesn't check for duplicate parameter names in patterns (e.g.,
/user/:id/profile/:id), which could lead to one value overwriting another.Consider adding a validation function for route patterns:
// validatePattern checks if a pattern has duplicate parameter names func validatePattern(pattern string) error { patternParts := strings.Split(strings.Trim(pattern, "/"), "/") seenParams := make(map[string]bool) for _, part := range patternParts { if strings.HasPrefix(part, ":") { paramName := strings.TrimPrefix(part, ":") if seenParams[paramName] { return fmt.Errorf("duplicate parameter name in pattern: %s", paramName) } seenParams[paramName] = true } } return nil }Then call this function in
WithSSEPattern:// WithSSEPattern sets the SSE endpoint pattern with route parameters func WithSSEPattern(pattern string) SSEOption { return func(s *SSEServer) { + if err := validatePattern(pattern); err != nil { + // Log the error or handle it as appropriate + // Consider panicking or returning early + return + } s.ssePattern = pattern } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/sse.go(13 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
server/sse.go (1)
mcp/types.go (1)
Request(89-102)
🔇 Additional comments (6)
server/sse.go (6)
36-40: Good implementation of context key type.You've correctly used a struct type for the context key as recommended by Go best practices, which helps avoid key collisions. The
RouteParamstype definition is also clear and appropriate.
153-158: LGTM - Consistent with existing option pattern.The
WithSSEPatternoption function follows the established option pattern in the codebase and provides a clean way to configure custom route patterns.
273-273: Good context handling for route parameters.Storing route parameters from the context in the session is a clean approach for making them available throughout the session lifecycle.
279-290: Improved context handling sequence.The updated context handling sequence properly ensures that:
- A base context with the session is created first
- Custom context modifications are applied if configured
- The session is registered with the proper context
This is a good refactoring that enhances context flow.
387-390: Good addition of route parameters to the message context.Adding the stored route parameters to the context during message handling ensures they're available to message handlers, fulfilling the core requirement of this PR.
541-564:❓ Verification inconclusive
matchPathfunction lacks URL decoding.The
matchPathfunction implements basic route pattern matching but doesn't handle URL-encoded parameters, which could cause issues when parameter values contain special characters.Add URL decoding for route parameters:
params := make(RouteParams) for i, part := range patternParts { if strings.HasPrefix(part, ":") { // This is a parameter paramName := strings.TrimPrefix(part, ":") - params[paramName] = pathParts[i] + // URL-decode the parameter value + decodedValue, err := url.QueryUnescape(pathParts[i]) + if err != nil { + // If decoding fails, use the raw value + params[paramName] = pathParts[i] + } else { + params[paramName] = decodedValue + } } else if part != pathParts[i] {Run this script to verify if there are any URL-encoded parameters in your test cases:
🏁 Script executed:
#!/bin/bash # Search for tests that use URL-encoded parameters rg -A 5 "url.QueryEscape|url.PathEscape" --type goLength of output: 52
Action: Incorporate URL Decoding in
matchPathFunctionThe
matchPathimplementation currently doesn’t perform URL decoding for parameters. Even though our automated test search (usingrgfor URL encoding/decoding functions) returned no results, it’s possible that URL-encoded values might eventually be passed to this function. This could lead to incorrect parameter matching when special characters are encoded.Please update the function as outlined below and manually verify that URL-encoded parameters are correctly handled in all relevant routes:
File:
server/sse.go(lines 541–564)Suggested Diff:
params := make(RouteParams) for i, part := range patternParts { if strings.HasPrefix(part, ":") { // This is a parameter paramName := strings.TrimPrefix(part, ":")params[paramName] = pathParts[i]
// URL-decode the parameter valuedecodedValue, err := url.QueryUnescape(pathParts[i])if err != nil {// If decoding fails, use the raw valueparams[paramName] = pathParts[i]} else {params[paramName] = decodedValue} } else if part != pathParts[i] {Since our search for related tests (using
rg -A 5 "url.QueryEscape|url.PathEscape") returned no output, please manually verify that any routes using this function behave correctly when their parameters are URL-encoded.
|
I really need this feature. |
+1 |
|
+1 This will be awesome and works as expected. Lets resolve the conflicts, whenever you get time. @anhao |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/sse.go (1)
555-578: Well-implemented path matching algorithm.The matchPath function properly handles parameter extraction and static path matching. Consider adding validation for parameter names and optimizing with pattern caching for high-traffic applications in the future.
In a future enhancement, you might want to add validation for parameter names and consider caching parsed patterns:
func matchPath(pattern, path string) (bool, RouteParams) { + // Cache could be implemented here for frequently accessed patterns patternParts := strings.Split(strings.Trim(pattern, "/"), "/") pathParts := strings.Split(strings.Trim(path, "/"), "/") if len(patternParts) != len(pathParts) { return false, nil } params := make(RouteParams) for i, part := range patternParts { if strings.HasPrefix(part, ":") { // This is a parameter paramName := strings.TrimPrefix(part, ":") + // Validate parameter name is not empty + if paramName == "" { + return false, nil + } params[paramName] = pathParts[i] } else if part != pathParts[i] { // Static part doesn't match return false, nil } } return true, params }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/custom_sse_pattern/main.go(1 hunks)server/sse.go(10 hunks)server/sse_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/custom_sse_pattern/main.go
- server/sse_test.go
🔇 Additional comments (8)
server/sse.go (8)
29-29: Excellent addition of route parameters to the session!This enhancement enables tracking URL path parameters throughout the SSE connection's lifetime.
37-57: Well-designed route parameter handling system!The implementation follows Go best practices with:
- Custom context key type for type safety
- Clear helper functions for parameter retrieval
- Proper error handling when parameters aren't found
This API design will make it intuitive for developers to access route parameters.
86-86: Good addition of the ssePattern field to support dynamic routes.This field logically extends the server's capabilities to support parameterized URL patterns.
156-161: Well-implemented WithSSEPattern option function.The function maintains consistency with other option functions in the codebase. Good job!
279-292: Great improvement to context handling in handleSSE.The changes correctly:
- Store route parameters from the request context
- Apply the base context with session
- Apply custom context function if provided
This ensures route parameters are preserved throughout the session lifecycle.
401-404: Good context enrichment with route parameters.You've correctly added the stored route parameters back to the context, ensuring they're available during message processing.
529-550: Excellent pattern matching implementation in ServeHTTP.The routing logic correctly handles both custom patterns and the default SSE endpoint path, maintaining backward compatibility while adding new functionality.
315-315: Good improvement to context cancellation handling.You've updated the event handling loops to use the enhanced context that contains route parameters, ensuring proper cancellation propagation.
Also applies to: 359-359
Thank you, I have resolved the conflict. Could you please check it? |
|
I took a crack at an alternative implementation fro allowing the server to be mounted at dynamic paths over in #214, the key differences are:
@anhao - What do you think? |
@robert-jackson-glean Sounds good to me! The approach is more flexible . Happy to see it implemented. 👍 |
Description
This PR adds support for dynamic route parameters in the SSE server, similar to the routing parameters in the Gin framework. This enhancement allows users to define custom route patterns with parameters (e.g.,
/:channel/sse) and access these parameters in their message handlers.Changes
WithSSEPatternoption to configure custom route patternsExample Usage
Summary by CodeRabbit
New Features
Tests