refactor(server): extract common HTTP transport configuration options #253
Conversation
|
Warning Rate limit exceeded@robert-jackson-glean has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 32 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughA new interface-based system for configuring HTTP transport options for servers is introduced, primarily affecting SSEServer and a stub StreamableHTTPServer. The code refactors option functions into typed interfaces and internal setters, consolidating and replacing previous exported option functions with a flexible, extensible approach for server configuration. Changes
Possibly related PRs
Suggested reviewers
✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 2
🧹 Nitpick comments (4)
server/sse_test.go (2)
718-725: Use canonicalinterface{}syntax to keepgo fmtcleanThe slice type is written with a space –
[]interface {}– whereas Go’s canonical formatting is[]interface{}.
Runninggo fmtwill rewrite this anyway, but it’s better to commit formatted code to avoid needless churn in future diffs.- rands := []interface {}{ + rands := []interface{}{
728-733: Minor formatting nit for consistencySame trailing space issue as above:
- var options []interface {} + var options []interface{}server/sse.go (1)
200-208: Unknown option types are silently discarded – surface misconfiguration early
NewSSEServerignores any value that is neitherSSEOptionnorHTTPTransportOption.
A typo in option wiring will therefore compile but have no effect, making the bug hard to spot.Consider returning an error (or at least
log.Printf) when an unknown option is encountered:default: - // Optionally: log or panic for unknown option type + log.Printf("sse: unrecognised option of type %T ignored", opt)server/http_transport_options.go (1)
106-110: Overwriting an existing customhttp.Serverwithout warningIf the user supplies two
WithHTTPServeroptions, the latter silently overwrites the first.
Either prevent double assignment or warn the caller to avoid hard-to-trace behaviour.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/http_transport_options.go(1 hunks)server/sse.go(4 hunks)server/sse_test.go(2 hunks)
🔇 Additional comments (1)
server/sse.go (1)
109-112:setBasePath("")now normalises to “/” – verify this doesn’t break existing callers
normalizeURLPath("")yields/, so callingWithStaticBasePath("")(or leaving the base path unset but later invoking this setter internally) changes the previous default of an empty string to/.
Down-stream helpers such asCompleteSsePathwill therefore produce//sse⇒/sse.
If any code relied onbasePath==""to detect “not configured”, this behaviour change could be breaking.Double-check call-sites (including external consumers) or guard against empty input:
-func (s *SSEServer) setBasePath(basePath string) { - s.basePath = normalizeURLPath(basePath) +func (s *SSEServer) setBasePath(basePath string) { + if basePath == "" { + s.basePath = "" + return + } + s.basePath = normalizeURLPath(basePath) }
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
server/http_transport_options.go (1)
143-168:⚠️ Potential issueFix the silent error handling in WithBaseURL
The
WithBaseURLfunction silently ignores invalid URLs by returning early when validation fails. This can lead to subtle bugs where misconfigured URLs go unnoticed until runtime.Consider either returning an error from the option factory or logging the failure:
func WithBaseURL(baseURL string) CommonHTTPServerOption { return commonOption{ apply: func(c httpTransportConfigurable) { if s, ok := c.(*SSEServer); ok { if baseURL != "" { u, err := url.Parse(baseURL) if err != nil { - return + log.Printf("http transport: invalid baseURL %q: %v", baseURL, err) + return } if u.Scheme != "http" && u.Scheme != "https" { - return + log.Printf("http transport: invalid scheme in baseURL %q: must be http or https", baseURL) + return } if u.Host == "" || strings.HasPrefix(u.Host, ":") { - return + log.Printf("http transport: invalid host in baseURL %q", baseURL) + return } if len(u.Query()) > 0 { - return + log.Printf("http transport: baseURL %q contains query parameters, which is not supported", baseURL) + return } } s.baseURL = strings.TrimSuffix(baseURL, "/") } }, } }Alternatively, consider refactoring
HTTPTransportOptionto return an error:// HTTPTransportOption is a function that configures an httpTransportConfigurable. type HTTPTransportOption func(httpTransportConfigurable) errorserver/sse.go (1)
223-225:⚠️ Potential issueRemove invalid
//go:deprecatedpragmaThe
//go:deprecatedline is an invalid Go compiler directive that will cause a build error.Go only accepts specific
//go:directives, anddeprecatedis not one of them. Use the conventional comment format instead:// WithSSEContextFunc sets a function that will be called to customise the context // to the server using the incoming request. // // Deprecated: Use WithContextFunc instead. This will be removed in a future version. -//go:deprecated
🧹 Nitpick comments (1)
server/sse.go (1)
697-705: Consider moving StreamableHTTPServer to http_transport_options.goThe StreamableHTTPServer implementation appears in both files. To maintain the single responsibility principle, consider defining it only in
http_transport_options.goand importing it inserver/sse.go.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/http_transport_options.go(1 hunks)server/sse.go(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
server/http_transport_options.go (1)
server/sse.go (6)
HTTPServerOption(23-25)SSEServer(131-148)StreamableHTTPServer(698-698)SSEOption(28-31)StreamableHTTPOption(34-37)CommonHTTPServerOption(40-43)
server/sse.go (2)
server/http_transport_options.go (7)
HTTPServerOption(30-32)StreamableHTTPServer(74-74)SSEOption(35-38)StreamableHTTPOption(41-44)HTTPContextFunc(14-14)DynamicBasePathFunc(99-99)WithHTTPContextFunc(135-141)mcp/types.go (1)
Request(103-116)
🪛 GitHub Check: test
server/http_transport_options.go
[failure] 30-30:
other declaration of HTTPServerOption
[failure] 35-35:
other declaration of SSEOption
[failure] 41-41:
other declaration of StreamableHTTPOption
[failure] 47-47:
other declaration of CommonHTTPServerOption
[failure] 53-53:
other declaration of sseOption
server/sse.go
[failure] 23-23:
HTTPServerOption redeclared in this block
[failure] 28-28:
SSEOption redeclared in this block
[failure] 34-34:
StreamableHTTPOption redeclared in this block
[failure] 40-40:
CommonHTTPServerOption redeclared in this block
[failure] 46-46:
sseOption redeclared in this block
🪛 GitHub Actions: go
server/sse.go
[error] 23-23: HTTPServerOption redeclared in this block
🔇 Additional comments (3)
server/sse.go (3)
227-229: Good use of composition in WithSSEContextFuncThe implementation elegantly delegates to
WithHTTPContextFuncwhich ensures that both the old and new options behave consistently, providing good backward compatibility.
150-182: Well-structured implementation of httpTransportConfigurableThe SSEServer correctly implements the httpTransportConfigurable interface with clear, focused setter methods. This approach makes the code more modular and easy to maintain.
244-246: Clean option application in NewSSEServerThe refactored approach using
opt.applyToSSE(s)is cleaner and more type-safe than the previous implementation. It ensures that only compatible options can be applied to the SSEServer.
rwjblue-glean
force-pushed
the
rwjblue/push-qprrotsqwysw
branch
from
389ad2c to
37497a4
Compare
server/http_transport_options.go
Outdated
| // TODO: Implement StreamableHTTPServer in the future | ||
| type StreamableHTTPServer struct{} | ||
|
|
||
| // Add stub methods to satisfy httpTransportConfigurable | ||
|
|
||
| func (s *StreamableHTTPServer) setBasePath(string) {} | ||
| func (s *StreamableHTTPServer) setDynamicBasePath(DynamicBasePathFunc) {} | ||
| func (s *StreamableHTTPServer) setKeepAliveInterval(time.Duration) {} | ||
| func (s *StreamableHTTPServer) setKeepAlive(bool) {} | ||
| func (s *StreamableHTTPServer) setContextFunc(HTTPContextFunc) {} |
There was a problem hiding this comment.
These are just stubs, to make it clearer what to do in #228
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
server/http_transport_options.go (1)
143-168:WithBaseURLsilently ignores invalid URLs – return an error or panicAt the moment, any validation failure leads to an early
return, leavingbaseURLunchanged but without signaling the problem. A mis-typed scheme or host would therefore go unnoticed until runtime when URLs are composed.Expose the failure to the caller, for example by returning a second
errorvalue from the option factory or by logging:func WithBaseURL(baseURL string) CommonHTTPServerOption { return commonOption{ apply: func(c httpTransportConfigurable) { if s, ok := c.(*SSEServer); ok { if baseURL != "" { u, err := url.Parse(baseURL) - if err != nil { - return - } + if err != nil { + log.Printf("server: invalid baseURL %q: %v", baseURL, err) + return + } if u.Scheme != "http" && u.Scheme != "https" { + log.Printf("server: invalid scheme in baseURL %q: %v", baseURL, u.Scheme) return } // Other validation checks...
🧹 Nitpick comments (1)
server/http_transport_options.go (1)
170-180: Consider documenting implications of custom HTTP serverThe
WithHTTPServeroption allows setting a custom HTTP server, but the documentation doesn't explain how this interacts with other configuration options. Some options might be ineffective when using a custom server.Enhance the documentation:
// WithHTTPServer sets the HTTP server instance for the transport. // This is useful for advanced scenarios where you want to provide your own http.Server. +// Note: When using a custom server, some configuration options might not be applied +// automatically, such as TLS settings or timeouts. It's the caller's responsibility +// to ensure the custom server is properly configured. func WithHTTPServer(srv *http.Server) CommonHTTPServerOption {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/http_transport_options.go(1 hunks)server/sse.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/sse.go
🧰 Additional context used
🪛 golangci-lint (1.64.8)
server/http_transport_options.go
59-59: type streamableHTTPOption is unused
(unused)
61-61: func streamableHTTPOption.isHTTPServerOption is unused
(unused)
62-62: func streamableHTTPOption.applyToStreamableHTTP is unused
(unused)
🔇 Additional comments (4)
server/http_transport_options.go (4)
16-23: Well-designed internal interface for common HTTP transport settingsThe
httpTransportConfigurableinterface provides a clean abstraction for shared HTTP transport configuration across different server types. This approach follows good design principles by defining a common contract for configuration methods.
29-50: Good interface hierarchy design for server optionsThe option interface hierarchy is well-structured, with appropriate specialization through interface embedding. This design enables type-safe options that can be applied to specific server types or shared across multiple server types.
52-72: Clean implementation of the functional options patternThe option wrappers (
sseOption,streamableHTTPOption, andcommonOption) provide a clean implementation of the functional options pattern. The use of method implementations to satisfy the interfaces creates a flexible and extensible system.🧰 Tools
🪛 golangci-lint (1.64.8)
59-59: type
streamableHTTPOptionis unused(unused)
61-61: func
streamableHTTPOption.isHTTPServerOptionis unused(unused)
62-62: func
streamableHTTPOption.applyToStreamableHTTPis unused(unused)
112-141: Well-implemented configuration optionsThe keep-alive and HTTP context function options are well-implemented, following a consistent pattern and providing appropriate setter functions for each configurable aspect.
rwjblue-glean
force-pushed
the
rwjblue/push-qprrotsqwysw
branch
3 times, most recently
from
31bcb79 to
d16a773
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server/http_transport_options.go (1)
148-173: WithBaseURL silently ignores invalid URLs – consider better error handlingThe
WithBaseURLfunction silently returns when validation fails, which could lead to unexpected behavior. This was noted in a previous review comment, and you mentioned you'd address it in a future PR.As a reminder for your future PR, consider either:
- Returning an error from the option function
- Logging a warning about the invalid URL
- Panicking with a clear message (if this is a programmer error)
This would make it easier to diagnose configuration issues.
🧹 Nitpick comments (1)
server/http_transport_options.go (1)
53-72: Consider adding compiler checks for streamableHTTPOptionThe
streamableHTTPOptiontype and its methods are flagged as unused by the static analyzer. Since they're part of your interface design but not currently used, consider adding a compile-time check similar to what you did for StreamableHTTPServer.+ // Ensure the option types implement the correct interfaces + var _ SSEOption = sseOption(nil) + var _ StreamableHTTPOption = streamableHTTPOption(nil) + var _ CommonHTTPServerOption = commonOption{}🧰 Tools
🪛 golangci-lint (1.64.8)
60-60: type
streamableHTTPOptionis unused(unused)
62-62: func
streamableHTTPOption.isHTTPServerOptionis unused(unused)
63-63: func
streamableHTTPOption.applyToStreamableHTTPis unused(unused)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/http_transport_options.go(1 hunks)server/sse.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/sse.go
🧰 Additional context used
🪛 golangci-lint (1.64.8)
server/http_transport_options.go
60-60: type streamableHTTPOption is unused
(unused)
62-62: func streamableHTTPOption.isHTTPServerOption is unused
(unused)
63-63: func streamableHTTPOption.applyToStreamableHTTP is unused
(unused)
🔇 Additional comments (5)
server/http_transport_options.go (5)
11-15: Good documentation and type definition for HTTPContextFuncThe HTTPContextFunc type is well-defined with clear documentation explaining its purpose and usage. This establishes a solid foundation for context manipulation across different HTTP server implementations.
16-24: Well-designed interface for HTTP transport configurationThe
httpTransportConfigurableinterface provides a clean abstraction for shared HTTP transport configuration, enabling code reuse across different server types. The set of methods covers essential configuration aspects like base paths, keep-alive settings, and HTTP server customization.
29-51: Good interface hierarchy for server configuration optionsThe option interfaces establish a logical hierarchy that enables both server-specific options and common options. This design supports the PR's goal of sharing configuration between server types while preserving type safety.
74-87: Good approach for the StreamableHTTPServer stub implementationThe StreamableHTTPServer stub with empty method implementations and compile-time interface check (
var _ httpTransportConfigurable = (*StreamableHTTPServer)(nil)) is a clean way to ensure interface compliance while indicating future implementation. This satisfies the PR's goal of demonstrating the implementation pattern.
89-97: Well-designed option functions with good documentationThe option functions follow a consistent pattern and have clear documentation explaining their purpose and usage. They correctly use the
commonOptionwrapper to provide common functionality for different server types.Also applies to: 106-115, 117-125, 127-135, 137-146, 175-183
rwjblue-glean
force-pushed
the
rwjblue/push-qprrotsqwysw
branch
from
d16a773 to
f7ef338
Compare
Create a common interface and pattern for HTTP transport configuration to enable code sharing between SSEServer and the upcoming StreamableHTTPServer. - Add new httpTransportConfigurable interface for shared configuration - Refactor SSEServer to implement the shared interface - Convert With* option functions to work with both server types - Add stub for StreamableHTTPServer to demonstrate implementation pattern - Deprecate WithSSEContextFunc in favor of WithHTTPContextFunc This change preserves backward compatibility while allowing the reuse of configuration code across different HTTP server implementations.
rwjblue-glean
force-pushed
the
rwjblue/push-qprrotsqwysw
branch
from
f7ef338 to
29f87db
Compare
…mark3labs#253) Create a common interface and pattern for HTTP transport configuration to enable code sharing between SSEServer and the upcoming StreamableHTTPServer. - Add new httpTransportConfigurable interface for shared configuration - Refactor SSEServer to implement the shared interface - Convert With* option functions to work with both server types - Add stub for StreamableHTTPServer to demonstrate implementation pattern - Deprecate WithSSEContextFunc in favor of WithHTTPContextFunc This change preserves backward compatibility while allowing the reuse of configuration code across different HTTP server implementations.
…mark3labs#253) Create a common interface and pattern for HTTP transport configuration to enable code sharing between SSEServer and the upcoming StreamableHTTPServer. - Add new httpTransportConfigurable interface for shared configuration - Refactor SSEServer to implement the shared interface - Convert With* option functions to work with both server types - Add stub for StreamableHTTPServer to demonstrate implementation pattern - Deprecate WithSSEContextFunc in favor of WithHTTPContextFunc This change preserves backward compatibility while allowing the reuse of configuration code across different HTTP server implementations.
Create a common interface and pattern for HTTP transport configuration to enable code sharing between SSEServer and the upcoming StreamableHTTPServer.
This change preserves backward compatibility while allowing the reuse of
configuration code across different HTTP server implementations (e.g. #228).
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Documentation