Enforce String or Integer non-null ID for MCP JSON-RPC Messages with a new McpSchema subclass#401
Closed
ZachGerman wants to merge 2 commits intomodelcontextprotocol:mainfrom
Closed
Enforce String or Integer non-null ID for MCP JSON-RPC Messages with a new McpSchema subclass#401ZachGerman wants to merge 2 commits intomodelcontextprotocol:mainfrom
ZachGerman wants to merge 2 commits intomodelcontextprotocol:mainfrom
Conversation
Contributor
|
@ZachGerman if you just want to validate the request ID this looks unnecessarily complicated? Wouldn't something like this achieve the same: @JsonInclude(JsonInclude.Include.NON_ABSENT)
@JsonIgnoreProperties(ignoreUnknown = true)
public record JSONRPCRequest( // @formatter:off
@JsonProperty("jsonrpc") String jsonrpc,
@JsonProperty("method") String method,
@JsonProperty("id") Object id,
@JsonProperty("params") Object params) implements JSONRPCMessage { // @formatter:on
/**
* Constructor that validates MCP-specific ID requirements. Unlike base JSON-RPC,
* MCP requires that: (1) Requests MUST include a string or integer ID; (2) The ID
* MUST NOT be null
*/
public JSONRPCRequest {
Assert.notNull(id, "MCP requests MUST include an ID - null IDs are not allowed");
Assert.isTrue(id instanceof String || id instanceof Integer || id instanceof Long,
"MCP requests MUST have an ID that is either a string or integer");
}
} |
9 tasks
Contributor
|
@ZachGerman here is how I would address this requirement. Let me know if i'm missing something: |
Contributor
Author
|
@tzolov nice catch on the Long case!
I think it's great. I just wanted to be sure the folks writing tools are informed of the rules conflicting with base JSON-RPC somewhere along the lines when confronting the types they use to write their tool. In hindsight, this was a bit like taking a flamethrower to an anthill 😅. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I considered
JSONRPCMessageID, but didn't want theJSONRPCprefix as it might imply nullability or possibly a float, as is standard for a JSON-RPCid.I also considered
idbut figured it was too ambiguous.I can see an argument for
McpMessageId, but I figured it being in theMcpSchemanamespace was good enough for someone to identify theMessageIdtype as specific to MCP.Motivation and Context
The specification conditions for JSON-RPC Message
idvalues include the requirements of the value being a (non-null) String or Integer.How Has This Been Tested?
Multiple existing tests use this field, have been updated accordingly, and confirmed to work correctly after the changes.
Breaking Changes
N/A
Types of changes
Checklist
Additional context
Edit: After sleeping on it, I realized this IS a breaking change, because people are possibly building requests in tool handlers without the requirements this would enforce.