Repository navigation
Conversation
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>
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.
|
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? |
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.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.DefaultOptionsexplicitly keepsPropertyNameCaseInsensitive = true. It is copied from the context's options but is also used for user-defined types (tool, prompt and resource parameters,McpClientToolarguments), so binding of user data is unchanged. The XML docs onDefaultOptionsnow describe this split.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
DefaultOptions.GetTypeInfo(<protocol type>)remains case-insensitive, because the setting applies per options instance rather than per type. Code that deserializes protocol types manually throughDefaultOptionskeeps the old behavior. Inside the SDK, this affectsModelContextProtocol.Extensions.Tasks, which deserializesCallToolResult,InputRequestandIDictionary<string, InputResponse>throughDefaultOptionsin six places (McpTasksClientExtensions,GetTaskResult,TaskStatusNotificationParams,McpTasksBuilderExtensions). I left them as they are to keep this change scoped to the core protocol path; happy to switch them to the source-generated type infos here or in a follow-up if you prefer.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.DefaultOptions_MatchesPropertyNamesCaseInsensitivelychecks thatDefaultOptionsstill binds user types case-insensitively.The new server test fails without the
JsonContextchange. Locally on net10.0 (Windows):ModelContextProtocol.Tests2393 passed / 6 skipped,ModelContextProtocol.AspNetCore.Tests635 passed / 30 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