Repository navigation
Conversation
There was a problem hiding this comment.
Thanks for the fix. This approach makes sense: match protocol-defined fields exactly while preserving the existing DefaultOptions behavior for user-defined data.
I checked TypeScript, Python, Go, Rust, and Ruby. None binds params.Name as the canonical tool name, and canonical name wins when both spellings are present, regardless of order. TypeScript and Python runtime probes confirmed this in both protocol eras. This supports the change here.
The important distinction is exact matching, not blanket rejection of noncanonical JSON. Peers differ in required-field validation, defaults, and raw/custom-result handling.
Looks good overall. A few non-blocking suggestions:
- Add a client-side result test covering alternate-cased optional fields such as
ContentandIsError, and a nested user-DTO-through-tool test confirming that its existing binding behavior is preserved. - Track the remaining internal casing differences separately, particularly task-based
CallToolResultdeserialization and transport-levelJsonRpcErrorDetailparsing. These do not bypass the tool-name fix. - Include the behavior change in the planned minor-release notes. The shared context also changes OAuth model parsing, so that compatibility impact should be mentioned.
| /// </para> | ||
| /// <para> | ||
| /// Property names are matched case-insensitively, as with <see cref="JsonSerializerDefaults.Web"/>. The SDK itself | ||
| /// deserializes incoming protocol messages with case-sensitive contracts, so a protocol property such as |
There was a problem hiding this comment.
"The SDK itself deserializes incoming protocol messages with case-sensitive contracts" is broader than the implementation. Some internal readers still use the case-insensitive public options, including task-based tool-result readers and transport-level error-detail parsing.
Could we qualify this wording and explicitly document that protocol types deserialized through DefaultOptions retain case-insensitive matching? That would describe the boundary accurately without requiring those follow-ups to block this fix.
There was a problem hiding this comment.
Reworded in 39ec6e5. Since the remaining SDK readers now use the new ProtocolOptions, the remarks say the SDK deserializes protocol messages with ProtocolOptions, and that protocol types deserialized through DefaultOptions keep case-insensitive matching.
|
I'd rather finish the protocol/user-data split here than leave the remaining readers as follow-ups. Tasks tool results still bind We could add cached, case-sensitive @tarekgh I know you suggested tracking these separately, but fixing the remaining protocol-field casing inconsistencies together seems manageable in one PR. Does that sound good to you? |
|
@halter73 Yes, that sounds good to me. My suggestion to track these separately was to keep the original fix focused, but I agree that finishing the protocol/user-data split in this PR is cleaner. A cached, read-only Let's use it for the remaining protocol readers, including task-based tool results and stream/ASP.NET error details, while retaining That would let us document the protocol boundary consistently rather than carrying the known internal exceptions. @FenjuFu Would you be able to include these changes in this PR, or would you like help with the implementation or tests? |
Protocol DTOs were deserialized with the source-generated context, which uses the web defaults and binds property names case-insensitively, while the JSON-RPC envelope converter matches its properties exactly. A request such as tools/call with params.Name was therefore dispatched even though middleware inspecting params.name would not recognize it. The source-generated context now matches property names case-sensitively. DefaultOptions keeps case-insensitive matching because it is also used for user-defined types such as tool parameters. Fixes modelcontextprotocol#1897 Signed-off-by: FenjuFu <fufenjupku@gmail.com>
Add McpJsonUtilities.ProtocolOptions, a cached read-only copy of DefaultOptions that matches property names case-sensitively, and use it wherever the SDK deserializes protocol messages through options rather than the source-generated context: the stdio client and server transports, the ASP.NET Core Streamable HTTP and SSE request reader, and the Tasks extension (task-based CallToolResult, InputRequest, InputResponse and error details). DefaultOptions and caller-supplied options are still used for user-defined data. Its docs now point to ProtocolOptions and state that protocol types deserialized through DefaultOptions keep case-insensitive matching. Signed-off-by: FenjuFu <fufenjupku@gmail.com>
e09ea77 to
39ec6e5
Compare
|
@halter73 @tarekgh Sure, done in 39ec6e5 (also rebased onto main):
Two things I left for you to decide: net10.0 locally: core 2414 passed / 6 skipped, AspNetCore 636 passed / 31 skipped, 0 failed. |
Fixes #1897
The JSON-RPC envelope converter matches
jsonrpc/id/method/params/result/errorexactly, but the typed contents ofparamsandresultwere deserialized with the internal source-generatedJsonContext, which is declared withJsonSerializerDefaults.Weband therefore binds property names case-insensitively. Atools/callrequest withparams.Namewas dispatched as if it wereparams.name, even though middleware or proxies inspectingparams.namewould not recognize it. Some transports and the Tasks extension also deserialized protocol types through the case-insensitiveDefaultOptions, so fields such aserror.Codeor a task-basedCallToolResult.IsErrorwere bound regardless of casing.Changes
McpJsonUtilities.JsonContextnow setsPropertyNameCaseInsensitive = false. Protocol DTOs (requests, results and notifications, on both client and server) only bind the property names defined by the specification, consistent with the envelope.McpJsonUtilities.ProtocolOptions: a cached, read-only copy ofDefaultOptionswith case-sensitive property matching. It is built from the same source-generated context, so trimming and Native AOT behave as before. Consumers deserializing protocol messages themselves can use it.ProtocolOptions: the stdio client and server transports (StreamClientSessionTransport,StreamServerTransport), the ASP.NET Core Streamable HTTP and SSE request reader (StreamableHttpHandler), and every protocol type inModelContextProtocol.Extensions.Tasks(task-basedCallToolResult,InputRequest,IDictionary<string, InputResponse>,JsonRpcErrorDetail). The HTTP client transports already used the case-sensitive context.McpJsonUtilities.DefaultOptionsexplicitly keepsPropertyNameCaseInsensitive = true. It is also used for user-defined types (tool, prompt and resource parameters,McpClientToolarguments,SendRequestAsync<TParameters, TResult>), so binding of user data is unchanged. Its XML docs now point toProtocolOptionsand state that protocol types deserialized throughDefaultOptionskeep case-insensitive matching.As discussed in the issue, this is exact protocol-field matching, not new validation of non-canonical JSON:
tools/callparams{"Name":"echo"}echonamemissing, JSON-RPC error, handler not invoked{"name":"echo","Name":"other"}other)echo{"Name":"other","name":"echo"}echoecho{"name":"echo","Arguments":{...}}null){"name":"echo","arguments":{"text":"a","Text":"B"}}Fields with defaults keep their defaults when only an alternately cased key is present; for example a
CallToolResultwith onlyContentstill ends up with an emptycontentlist rather than failing.Not changed
McpApps.SerializerOptions(Apps extension) is public and derived fromDefaultOptions. I left it as is; it can switch toProtocolOptionsif the_meta.uitypes should follow the protocol rules too.Tests
McpServerTests.Call_Tool_Request_Params_Are_Matched_Case_Sensitivelysends rawtools/callrequests through the server:Nameonly (rejected, handler not invoked), alternately casedArguments(not bound), canonical, both key orders withnameandName, and alternately cased keys insidearguments(preserved as distinct entries).McpJsonUtilitiesTests:ProtocolOptionsis a read-only singleton with case-sensitive matching; for a JSON-RPC error with bothcode/messageandCode/Message, and for aCallToolResultwithContent/IsError,ProtocolOptionsbinds only the canonical names whileDefaultOptionskeeps the previous behavior.DefaultOptions_MatchesPropertyNamesCaseInsensitivelystill covers user types.StdioServerTransportTests.ReadMessagesAsync_Should_Match_Error_Properties_Case_Sensitivelyreads such an error line through the stdio transport.McpServerToolTests.BindsNestedArgumentPropertiesCaseInsensitivelyinvokes a tool whose parameter is a nested user DTO, bound fromLOCATION/CITY, to confirm user-data binding is unchanged.The server and stdio transport tests fail without the change. Locally on net10.0 (Windows):
ModelContextProtocol.Tests2414 passed / 6 skipped,ModelContextProtocol.AspNetCore.Tests636 passed / 31 skipped, 0 failed.As noted in the issue, this is a subtle behavior change for peers that relied on alternate casing, so it probably belongs in a minor release.
cc @tarekgh for review