-
Notifications
You must be signed in to change notification settings - Fork 707
fix: Improve content type handling in streamable_http.go #210
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe update refines how the HTTP response Content-Type header is processed in the Changes
✨ 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/transport/streamable_http.go
(4 hunks)
🔇 Additional comments (2)
client/transport/streamable_http.go (2)
10-10
: Add themime
import for media type parsing
This import enables use ofmime.ParseMediaType
to robustly extract the media type from theContent-Type
header.
252-259
: Approve whitespace cleanup in SSE handler
The removal of extraneous blank lines around JSON unmarshal calls improves readability without altering behavior.
mediaType, _, _ := mime.ParseMediaType(resp.Header.Get("Content-Type")) | ||
switch mediaType { | ||
case "application/json": | ||
// Single response | ||
var response JSONRPCResponse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Ensure robust error handling when parsing Content-Type
Currently, parsing errors from mime.ParseMediaType
are ignored, which may lead to an empty mediaType
and fall into the default case for malformed headers. It’s safer to handle errors explicitly and fallback to the raw header (or its trimmed subtype) to preserve existing behavior.
Proposed diff:
- mediaType, _, _ := mime.ParseMediaType(resp.Header.Get("Content-Type"))
+ rawCT := resp.Header.Get("Content-Type")
+ mediaType, _, err := mime.ParseMediaType(rawCT)
+ if err != nil {
+ // Fallback to the raw content type (trimmed, lowercase) without parameters
+ mediaType = strings.ToLower(strings.TrimSpace(strings.SplitN(rawCT, ";", 2)[0]))
+ }
I discovered this problem when using the Streamable HTTP API provided by |
Optimized content type header parsing
mime.ParseMediaType()
Test case
Other
I didn't handle the error here. If processing fails, it will default to the original code's default case.
Summary by CodeRabbit