fix SSEOption #63
fix SSEOption #63
Conversation
WalkthroughThis update enhances the validation logic for the Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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: 5
🧹 Nitpick comments (2)
server/sse.go (2)
59-71: Improved validation for baseURL.The enhanced validation in WithBaseURL function ensures proper URL format, guards against empty hosts, and provides clear error messages. However, using panic might be too severe for configuration issues.
Consider returning an error instead of panicking to allow more graceful error handling:
-func WithBaseURL(baseURL string) SSEOption { - return func(s *SSEServer) { - if baseURL != "" { - u, err := url.Parse(baseURL) - if err != nil { - panic(fmt.Sprintf("invalid baseURL: %s", baseURL)) - } - // Check if the host is empty or only contains a port - if u.Host == "" || strings.HasPrefix(u.Host, ":") { - panic(fmt.Sprintf("invalid baseURL: %s", baseURL)) - } - } - s.baseURL = strings.TrimSuffix(baseURL, "/") - } -} +// WithBaseURL sets the base URL for the SSE server +func WithBaseURL(baseURL string) SSEOption { + return func(s *SSEServer) { + if baseURL != "" { + u, err := url.Parse(baseURL) + if err != nil { + s.baseURL = "" + return + } + // Check if the host is empty or only contains a port + if u.Host == "" || strings.HasPrefix(u.Host, ":") { + s.baseURL = "" + return + } + } + s.baseURL = strings.TrimSuffix(baseURL, "/") + } +}
8-8: Use time.Now().UnixNano() for better seed in tests.While using math/rand is appropriate here, using a better seed would enhance test reliability.
import ( "bytes" "context" "encoding/json" "fmt" - "math/rand" + "math/rand" "net/http" "net/http/httptest" // Later in the added test: + // Initialize random seed + rand.Seed(time.Now().UnixNano()) for i := 0; i < 100; i++ {
📜 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(2 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
server/sse_test.go (2)
server/server.go (1) (1)
NewMCPServer(251-277)server/sse.go (7) (7)
SSEOption(56-56)WithBasePath(76-84)WithBaseURL(59-73)WithMessageEndpoint(87-91)WithSSEEndpoint(94-98)WithHTTPServer(101-105)NewSSEServer(116-129)
server/sse.go (1)
server/server.go (16) (16)
s(100-105)s(108-116)s(119-123)s(126-150)s(153-179)s(280-470)s(473-486)s(489-502)s(505-513)s(516-518)s(521-536)s(539-544)s(547-559)s(562-569)s(571-623)s(625-631)
🪛 GitHub Actions: go
server/sse_test.go
[error] 34-34: TestSSEServer/Can_instantiate failed: Expected baseURL http://localhost:8080/mcp, got http://localhost:8080
[error] 376-376: TestSSEServer/Works_with_custom_mux failed: Expected status 200, got 404
[error] 383-383: TestSSEServer/Works_with_custom_mux failed: Failed to read SSE response: EOF
🔇 Additional comments (4)
server/sse_test.go (1)
618-661: Good addition of tests to ensure SSEOption consistency.This test verifies that SSEOption functions behave consistently when applied repeatedly in random order, which is a critical check given the PR objective of ensuring SSEOption doesn't have negative effects when used repeatedly.
The test is thorough in checking all configurable properties of the SSEServer, including basePath, baseURL, messageEndpoint, sseEndpoint, and HTTP server instance.
server/sse.go (3)
231-231: Nice simplification using the new CompleteMessageEndpoint method.Using the CompleteMessageEndpoint method instead of manually constructing the URL makes the code more maintainable and consistent.
360-362: Good addition of helper method for constructing the complete SSE endpoint.The CompleteSseEndpoint method provides a clean way to construct the full URL for the SSE endpoint, improving code consistency.
367-369: Good addition of helper method for constructing the complete message endpoint.The CompleteMessageEndpoint method provides a clean way to construct the full URL for the message endpoint, improving code consistency.
server/sse.go
Outdated
| // Use exact path matching rather than Contains | ||
| if path == ssePath { | ||
| if path == s.CompleteSsePath() { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix path comparison to handle URL parsing errors.
The path comparison will fail if CompleteSsePath encounters URL parsing errors, which is causing the pipeline failures.
🏁 Script executed:
#!/bin/bash
# Check the failing test cases to verify our hypothesis
# First, let's check what's happening in the "Can instantiate" test
echo "Checking the 'Can instantiate' test failure..."
grep -A 10 "Can instantiate" server/sse_test.go
# Now let's verify the path comparison in the ServeHTTP method
echo "Verifying the path comparison in ServeHTTP..."
cat server/sse.go | grep -A 20 "ServeHTTP"Length of output: 963
Action Required: Update URL Path Comparison to Handle Parsing Errors
The current implementation in server/sse.go compares r.URL.Path directly against s.CompleteSsePath() without considering that URL parsing might fail. If CompleteSsePath encounters a parsing error, the resulting comparison will likely be invalid and lead to the pipeline failures observed.
Suggestions:
- Error Handling: Modify the logic in
ServeHTTPto capture and inspect any errors fromCompleteSsePath(). If an error occurs, return a suitable HTTP error (e.g., a 500 Internal Server Error) rather than proceeding with a faulty comparison. - Consistency: Review similar comparisons (like the one for
s.CompleteMessagePath()) to ensure they also handle potential URL parsing issues consistently.
Example Diff Snippet:
- if path == s.CompleteSsePath() {
- s.handleSSE(w, r)
- return
- }
+ pathSse, err := s.CompleteSsePath()
+ if err != nil {
+ http.Error(w, "Internal server error", http.StatusInternalServerError)
+ return
+ }
+ if path == pathSse {
+ s.handleSSE(w, r)
+ return
+ }Please implement similar error checks where needed to ensure robust URL handling.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if path == s.CompleteSsePath() { | |
| pathSse, err := s.CompleteSsePath() | |
| if err != nil { | |
| http.Error(w, "Internal server error", http.StatusInternalServerError) | |
| return | |
| } | |
| if path == pathSse { | |
| s.handleSSE(w, r) | |
| return | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
server/sse.go (4)
358-364: 🛠️ Refactor suggestionImprove error handling in GetUrlPath method
The
GetUrlPathmethod silently fails by returning an empty string when URL parsing fails. This can lead to subtle bugs if callers don't know to check for empty strings.-func (s *SSEServer) GetUrlPath(input string) string { - parse, err := url.Parse(input) - if err != nil { - return "" - } - return parse.Path -} +func (s *SSEServer) GetUrlPath(input string) (string, error) { + parse, err := url.Parse(input) + if err != nil { + return "", fmt.Errorf("failed to parse URL %s: %w", input, err) + } + return parse.Path, nil +}
366-371:⚠️ Potential issueFix error handling in CompleteSsePath
The
CompleteSsePathfunction doesn't handle potential URL parsing errors fromGetUrlPath, which could lead to 404 errors when path matching fails.func (s *SSEServer) CompleteSseEndpoint() string { return s.baseURL + s.basePath + s.sseEndpoint } -func (s *SSEServer) CompleteSsePath() string { - return s.GetUrlPath(s.CompleteSseEndpoint()) +func (s *SSEServer) CompleteSsePath() string { + path := s.GetUrlPath(s.CompleteSseEndpoint()) + if path == "" { + return s.basePath + s.sseEndpoint + } + return path +}
373-378:⚠️ Potential issueFix error handling in CompleteMessagePath
Similarly to
CompleteSsePath, theCompleteMessagePathfunction doesn't handle potential URL parsing errors fromGetUrlPath.func (s *SSEServer) CompleteMessageEndpoint() string { return s.baseURL + s.basePath + s.messageEndpoint } -func (s *SSEServer) CompleteMessagePath() string { - return s.GetUrlPath(s.CompleteMessageEndpoint()) +func (s *SSEServer) CompleteMessagePath() string { + path := s.GetUrlPath(s.CompleteMessageEndpoint()) + if path == "" { + return s.basePath + s.messageEndpoint + } + return path +}
389-390:⚠️ Potential issueFix path comparison for message endpoint
Similar to the SSE endpoint, the message path comparison doesn't handle URL parsing errors.
- if path == s.CompleteMessagePath() { + messagePath := s.CompleteMessagePath() + if messagePath != "" && path == messagePath { s.handleMessage(w, r) return }
🧹 Nitpick comments (1)
server/sse.go (1)
61-76: Robust validation for baseURL parameterThe improved validation ensures the
baseURLis a properly formatted URL with the correct scheme, host, and no query parameters. This prevents potential issues with URL construction later in the code.However, consider returning errors instead of using
panic, as this would give callers a chance to handle invalid values more gracefully, especially in production code.-func WithBaseURL(baseURL string) SSEOption { - return func(s *SSEServer) { - if baseURL != "" { - u, err := url.Parse(baseURL) - if err != nil { - panic(fmt.Sprintf("invalid baseURL: %s", baseURL)) - } - if u.Scheme != "http" && u.Scheme != "https" { - panic(fmt.Sprintf("invalid baseURL: %s", baseURL)) - } - // Check if the host is empty or only contains a port - if u.Host == "" || strings.HasPrefix(u.Host, ":") { - panic(fmt.Sprintf("invalid baseURL: %s", baseURL)) - } - if len(u.Query()) > 0 { - panic(fmt.Sprintf("invalid baseURL: %s", baseURL)) - } - } - s.baseURL = strings.TrimSuffix(baseURL, "/") - } -} +// WithBaseURL sets the base URL for the SSE server +// Returns an error if the URL is invalid +func WithBaseURL(baseURL string) SSEOption { + return func(s *SSEServer) { + if baseURL != "" { + u, err := url.Parse(baseURL) + if err != nil { + // Log the error but continue with empty baseURL + fmt.Printf("Warning: invalid baseURL ignored: %s - %v\n", baseURL, err) + baseURL = "" + } else if u.Scheme != "http" && u.Scheme != "https" { + fmt.Printf("Warning: invalid scheme in baseURL: %s\n", baseURL) + baseURL = "" + } else if u.Host == "" || strings.HasPrefix(u.Host, ":") { + fmt.Printf("Warning: invalid host in baseURL: %s\n", baseURL) + baseURL = "" + } else if len(u.Query()) > 0 { + fmt.Printf("Warning: baseURL should not contain query parameters: %s\n", baseURL) + baseURL = "" + } + } + s.baseURL = strings.TrimSuffix(baseURL, "/") + } +}
📜 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(4 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
server/sse_test.go (1)
server/sse.go (7) (7)
SSEOption(56-56)WithBasePath(82-90)WithBaseURL(59-79)WithMessageEndpoint(93-97)WithSSEEndpoint(100-104)WithHTTPServer(107-111)NewSSEServer(122-135)
🪛 golangci-lint (1.64.8)
server/sse_test.go
33-33: SA5011: possible nil pointer dereference
(staticcheck)
🔇 Additional comments (5)
server/sse_test.go (4)
8-8: Updated import to support random option testingThe addition of the
math/randimport is necessary for the new test case that verifies the consistency ofSSEOptionwhen used randomly and repeatedly.
33-36: Updated expected baseURL value to match new implementationThe test now correctly expects
"http://localhost:8080"without the path component, which aligns with the changes to URL handling in the implementation.🧰 Tools
🪛 golangci-lint (1.64.8)
33-33: SA5011: possible nil pointer dereference
(staticcheck)
354-354: Simplified handler registration to directly use sseServerUsing the SSEServer directly with the mux matches the new implementation of URL path handling, where the server now intelligently manages paths internally.
618-661: Good test for ensuring SSEOption consistencyThis test properly validates that
SSEOptionbehaves consistently when used repeatedly with random combinations of options, which directly addresses the PR objective of fixing SSEOption behavior.The test systematically verifies:
- Random combinations of options (100 iterations with 100 random options each)
- All server properties (
basePath,baseURL,sseEndpoint,messageEndpoint,srv)- Consistency of the final configuration regardless of application order
server/sse.go (1)
237-237: Proper use of CompleteMessageEndpoint methodGood refactoring using the new
CompleteMessageEndpoint()method instead of manual string formatting, which makes the code more maintainable and consistent.
|
@a67793581 could you please address the coderabbit comments with either a fix or justifcation? |
Good, thank you very much for the reminder, I have completed the repair |
SSEOption should not have negative effects when used repeatedly but should always remain consistent.
Summary by CodeRabbit
New Features
Tests
baseURLand HTTP server handling.