fix: type mismatch for request/response ID #291
Conversation
|
""" WalkthroughThe changes update the types used for JSON-RPC request and response IDs from raw Changes
Assessment against linked issues
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ 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 (
|
pottekkat
force-pushed
the
fix/type-mismatch/154
branch
from
0a54ebb to
5e0b31e
Compare
pottekkat
force-pushed
the
fix/type-mismatch/154
branch
from
5e0b31e to
25a3e3f
Compare
| @@ -62,7 +62,7 @@ func NewSSE(baseURL string, options ...ClientOption) (*SSE, error) { | |||
| smc := &SSE{ | |||
| baseURL: parsedURL, | |||
| httpClient: &http.Client{}, | |||
| responses: make(map[int64]chan *JSONRPCResponse), | |||
| responses: make(map[string]chan *JSONRPCResponse), | |||
There was a problem hiding this comment.
Always use string version of the id for lookups to ensure consistency.
| idValue, ok := result.ID.Value().(int64) | ||
| if !ok { | ||
| t.Errorf("Expected ID to be int64, got %T", result.ID.Value()) |
There was a problem hiding this comment.
Also check the type.
| t.Run("SendRequestWithStringID", func(t *testing.T) { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer cancel() | ||
|
|
||
| params := map[string]any{ | ||
| "string": "string id test", | ||
| "array": []any{4, 5, 6}, | ||
| } | ||
|
|
||
| // Use a string ID instead of an integer | ||
| request := JSONRPCRequest{ | ||
| JSONRPC: "2.0", | ||
| ID: mcp.NewRequestId("request-123"), | ||
| Method: "debug/echo", | ||
| Params: params, | ||
| } |
There was a problem hiding this comment.
Make sure string Ids work. Only this test uses a string ID now but I guess that's sufficient.
mcp/types.go
Outdated
| func (r *RequestId) UnmarshalJSON(data []byte) error { | ||
| // Try unmarshaling as string first | ||
| var s string | ||
| if err := json.Unmarshal(data, &s); err == nil { | ||
| r.value = s | ||
| return nil | ||
| } | ||
|
|
||
| // JSON numbers are unmarshaled as float64 in Go | ||
| var f float64 | ||
| if err := json.Unmarshal(data, &f); err == nil { | ||
| if f == float64(int64(f)) { | ||
| r.value = int64(f) | ||
| } else { | ||
| r.value = f | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| return fmt.Errorf("invalid request id: %s", string(data)) | ||
| } |
There was a problem hiding this comment.
Custom unmarshal function to make sure numbers converted to float are converted to int. This ensures both string and int ids work.
![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 (2)
client/transport/sse_test.go (1)
190-195: Deduplicate repeated ID-assertion logic via a helperThe exact same 6-line block for asserting an
int64request-ID appears four times in this test. Re-using a small helper reduces noise and makes future type-changes easier:+func assertInt64ID(t *testing.T, got mcp.RequestId, want int64, context string) { + t.Helper() + v, ok := got.Value().(int64) + if !ok { + t.Fatalf("%s: expected ID to be int64, got %T", context, got.Value()) + } + if v != want { + t.Fatalf("%s: expected ID %d, got %d", context, want, v) + } +}Then replace each call site, e.g.
- idValue, ok := result.ID.Value().(int64) - ... - } else if idValue != 1 { - t.Errorf("Expected ID 1, got %d", idValue) + assertInt64ID(t, result.ID, 1, "SendRequest/result")This keeps the test focused on behaviour rather than bookkeeping.
Also applies to: 323-334, 352-357, 398-401
client/transport/stdio_test.go (1)
110-115: Extract common ID checks to a helper (same as SSE tests)The integer-ID validation block is repeated five times here and once more in
SendRequestWithStringID(string variant). Consider introducing a pair of helpers likeassertInt64IDandassertStringIDin this file (or a sharedtestutilpackage) to keep each sub-test concise and DRY.Also applies to: 248-254, 272-276, 315-319, 365-368
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (14)
client/client.go(1 hunks)client/transport/interface.go(1 hunks)client/transport/sse.go(6 hunks)client/transport/sse_test.go(11 hunks)client/transport/stdio.go(6 hunks)client/transport/stdio_test.go(12 hunks)client/transport/streamable_http.go(2 hunks)client/transport/streamable_http_test.go(13 hunks)mcp/types.go(2 hunks)server/server.go(3 hunks)server/server_test.go(5 hunks)server/sse.go(1 hunks)server/sse_test.go(1 hunks)testdata/mockstdio_server.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- server/server_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- client/transport/stdio.go
- client/transport/sse.go
🧰 Additional context used
🧬 Code Graph Analysis (7)
client/client.go (1)
mcp/types.go (1)
NewRequestId(202-204)
client/transport/interface.go (1)
mcp/types.go (1)
RequestId(197-199)
server/sse.go (1)
mcp/types.go (1)
NewRequestId(202-204)
testdata/mockstdio_server.go (1)
mcp/types.go (1)
RequestId(197-199)
client/transport/streamable_http_test.go (1)
mcp/types.go (2)
NewRequestId(202-204)RequestId(197-199)
server/server.go (1)
mcp/types.go (1)
NewRequestId(202-204)
client/transport/sse_test.go (1)
mcp/types.go (2)
NewRequestId(202-204)RequestId(197-199)
🔇 Additional comments (28)
server/sse.go (1)
341-341: Correctly using the type-safemcp.RequestIdwrapperThis change wraps the request ID with
mcp.NewRequestId()to ensure type safety when dealing with request IDs. This addresses the issue mentioned in the PR where integer IDs were causing type mismatches due to JSON unmarshaling converting numbers to floats.client/client.go (1)
107-107: Correctly using typed RequestId instead of raw integerThis change aligns with the PR objective to fix type mismatches for request/response IDs by wrapping the raw integer ID with
mcp.NewRequestId(). This ensures consistent type handling throughout the JSON-RPC communication.testdata/mockstdio_server.go (3)
9-10: Adding necessary mcp package importAdded the import needed for
mcp.RequestIdtype.
15-15: Updated request ID type to match new type systemChanged from raw integer to
*mcp.RequestIdfor type safety and consistency with the PR objective of fixing request/response ID type handling.
21-23: Updated response ID and result types to match new type systemThe
JSONRPCResponsestruct has been updated to use*mcp.RequestIdfor the ID field andanyfor the Result field, ensuring type safety and consistency.client/transport/interface.go (2)
29-34: Updated request structure to use typed RequestIdThe
JSONRPCRequeststruct now uses the strongly typedmcp.RequestIdfor the ID field instead of a raw integer. This is part of the type mismatch fix described in the PR objective and ensures consistent handling of request IDs in JSON-RPC communication.
38-38: Updated response ID to use typed RequestIdThe
JSONRPCResponsestruct now uses the strongly typedmcp.RequestIdfor the ID field, maintaining consistency with the request structure and ensuring proper type handling during JSON marshaling/unmarshaling.client/transport/streamable_http.go (2)
219-221: Changed nil check for response ID to use IsNil() method.This change updates the nil check to use the
IsNil()method of themcp.RequestIdtype instead of a direct nil comparison. This addresses the core issue in the PR of properly handling request/response IDs of different types.
260-262: Updated notification detection to use ID.IsNil() method.This modification properly handles the nil check for notification messages (which lack an ID) using the
IsNil()method rather than a direct nil pointer comparison, consistent with the newmcp.RequestIdtype.server/sse_test.go (1)
813-822: Improved type checking for ping message IDs.The updated code adds proper type assertion logic for handling the ping message ID, checking for both
int64andfloat64types instead of assuming it's always afloat64. This handles the case where request IDs might be unmarshaled from JSON as different numeric types.The change correctly addresses the issue described in the PR where numbers could be interpreted as different types when unmarshaled from JSON.
server/server.go (3)
104-105: Updated ID field to use RequestId type for error responses.This change wraps the raw ID value with
mcp.NewRequestId()when constructing a JSON-RPC error response, standardizing ID handling throughout the transport interface.
931-932: Updated response ID creation to use RequestId wrapper.The change uses
mcp.NewRequestId()to wrap the raw ID value when constructing a JSON-RPC response, ensuring consistent ID type handling across the codebase.
943-944: Updated error response ID creation to use RequestId wrapper.Similar to the other changes, this modification wraps the raw ID value with
mcp.NewRequestId()when creating an error response, maintaining type consistency for JSON-RPC IDs.client/transport/streamable_http_test.go (14)
150-151: Updated test to use typed RequestId for initialization request.Modified the test code to use
mcp.NewRequestId(int64(0))instead of a raw integer value, aligning with the new type system for request IDs.
171-172: Updated test request to use typed RequestId.Modified the test code to use
mcp.NewRequestId(int64(1))instead of a raw integer value, ensuring proper testing of the new ID type system.
185-186: Updated result struct to use RequestId type.Changed the ID field type in the test result struct to
mcp.RequestIdto match the updated type system for JSON-RPC messages.
198-203: Added type-safe ID value extraction and validation.This change adds proper type assertion when extracting the ID value, with error reporting if the assertion fails. This enhances type safety and allows for better test diagnostics.
225-226: Updated timeout test to use RequestId type.Modified the test request to use
mcp.NewRequestId(int64(3))for consistency with the type changes throughout the codebase.
253-254: Updated notification test to use RequestId type.Modified the test request to use
mcp.NewRequestId(int64(1))to align with the new type system for request IDs.
272-273: Updated notification check to handle ID type conversion.The code now properly converts the float64 value from JSON to int64 for comparison with the request ID value, addressing the core issue of the PR regarding type handling.
308-309: Updated concurrent request test to use RequestId type.Changed the code to use
mcp.NewRequestId(int64(100 + idx))for proper ID typing in the concurrent request test.
333-345: Added robust response ID validation in multi-request test.This change adds proper null checks, type assertions, and value comparisons for response IDs, improving test robustness and error reporting. This ensures that the response IDs are correctly handled in all concurrent test cases.
351-352: Updated result struct ID type in multi-request test.Changed the ID field type in the multi-request test result struct to
mcp.RequestIdfor consistency with the type system changes.
362-364: Updated ID value comparison in multi-request test.Modified the test to directly extract and compare the ID value from the RequestId object rather than comparing raw integer values.
384-385: Updated error test to use RequestId type.Changed the error test to use
mcp.NewRequestId(int64(100))for the request ID, maintaining consistency with the type system changes.
406-411: Added robust ID verification in error response test.This change adds proper type assertion and value checks when verifying the ID in error responses, improving test robustness and error reporting.
440-441: Updated error test URL to use RequestId type.Modified the non-existent URL test to use
mcp.NewRequestId(int64(1))for consistency with the type system changes throughout the test file.client/transport/stdio_test.go (1)
326-382: 👍 Good addition covering string IDsThe new
SendRequestWithStringIDsub-test proves that non-numeric IDs round-trip correctly through the transport layer. This complements the earlier int64 coverage and guards against regressions.
pottekkat
changed the title
pottekkat
added
type: enhancement
Description
Note: Usinganycauses issues with using anintrequest ID because of how the unmarshal function works (it unmarshals numbers into floats) which will break this for integer IDs. This needs to be fixed before we merge it. I'm working on a fix but I made the PR public so that others can also look into how we can handle this properly.Use the correct type for the request ID.
Request ID can be an
intor astringbut right now we handle it asanyinmcp.RequestIdwhich is fine. We can come back to it later if needed for more specific type handling.The
intis converted tostringfor lookups.You can test the SDK by adding this to your
go.modfile:Ref: https://github.com/modelcontextprotocol/modelcontextprotocol/blob/c87a0da6d8c2436d56a6398023c80b0562224454/schema/2025-03-26/schema.json#L1533
Fixes #154
Type of Change
Checklist
Summary by CodeRabbit