FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

types: Setting default value for jsonrpc by sreenaths · Pull Request #1310 · modelcontextprotocol/python-sdk · GitHub

/ python-sdk Public

types: Setting default value for jsonrpc - #1310

Closed
sreenaths wants to merge 1 commit into
modelcontextprotocol:mainfrom
sreenaths:setting-default-jsonrpc-version
ConversationCommits (1)ChecksFiles changed
Closed

types: Setting default value for jsonrpc#1310
sreenaths wants to merge 1 commit into
modelcontextprotocol:mainfrom
sreenaths:setting-default-jsonrpc-version

Conversation

sreenaths commented Aug 26, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

The JSON-RPC version is fixed at 2.0 for MCP messages. However, as the value is not set by default, we have to pass the correct value every time a model is instantiated.

Motivation and Context

This change simplifies model instantiation by automatically setting the JSON-RPC version. Additionally, if the version ever needs to change in the future, it only has to be updated in a single place.

For example

JSONRPCMessage(
    root=JSONRPCRequest(
        # jsonrpc="2.0", - This wouldn't be required
        id=1,
        method="ping"
    )
),

How Has This Been Tested?

Tests were improved and are working as expected.

Breaking Changes

No

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

None

sreenaths requested review from a team and ihrpr August 26, 2025 22:53
sreenaths force-pushed the setting-default-jsonrpc-version branch from 0302041 to d626174 Compare August 26, 2025 22:58

sreenaths commented Aug 28, 2025

Copy link
Copy Markdown
Contributor Author

Hi @ochafik
When you have a chance, please review this PR.
It is similar to PR #1292 that you had helped with before.
Thank you.

sreenaths force-pushed the setting-default-jsonrpc-version branch from d626174 to 18051bc Compare September 1, 2025 04:44
hesreallyhim reviewed Sep 17, 2025
View reviewed changes

hesreallyhim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

this seems like a move in the right direction but my feeling is - why isn't this just defined at the type that is at the top of the message hierarchy? It's mandatory in every protocol message that goes over the wire. Intuitively, I would expect that to be JSONRPCMessage, I think maybe it's not, but this is the "correct" ontology in my opinion. Whatever type is at the root of all message types (if there is one, which there should be) should have this field set always.

sreenaths commented Sep 18, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Hi @hesreallyhim
Thank you so much for the suggestion. It would be nice to have a common base class for JSONRPCRequest, JSONRPCNotification, JSONRPCResponse, and JSONRPCError.

But, as the original schema is defined in TypeScript, and types.py depends on it... I am not sure if we can change the base structure in Python SDK. Additionally, we might have to use multiple inheritance as JSONRPCRequest already inherits from Request and JSONRPCNotification from Notification.

I hope one of the maintainers can guide us.

\cc @ochafik

hesreallyhim commented Sep 18, 2025

Copy link
Copy Markdown
Contributor

But, as the original schema is defined in TypeScript, and types.py depends on it... I am not sure if we can change the base structure in Python SDK. Additionally, we might have to use multiple inheritance as JSONRPCRequest already inherits from Request and JSONRPCNotification from Notification.

yeah... well i think they've kind of fixed this in the draft spec, but the sdk may still choose to do things a little differently.

Kludex commented Sep 19, 2025

Copy link
Copy Markdown
Member

We need to migrate to this: https://github.com/Kludex/mcp/blob/main/src/mcp-types/mcp_types/__init__.py

ochafik approved these changes Sep 19, 2025
View reviewed changes

ochafik left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks @sreenaths !

ochafik commented Sep 19, 2025

Copy link
Copy Markdown
Contributor

We need to migrate to this: https://github.com/Kludex/mcp/blob/main/src/mcp-types/mcp_types/__init__.py

@Kludex not sure it would support passing the literal values as defaults?

hesreallyhim commented Sep 19, 2025

Copy link
Copy Markdown
Contributor

sorry, perhaps I should have been more direct - i would need to study the codebase more closely to understand how the sdk consumes the spec schema.ts - in the case of the typescript SDK i think they had decided to declare the internal types as not including jsonprc, and adding this in at a "protocol" layer before sending the message over the wire. In any case, the observations made above by @sreenaths, such as:

It would be nice to have a common base class for JSONRPCRequest, JSONRPCNotification, JSONRPCResponse, and JSONRPCError.

was actually just an oversight in the MCP schema.ts (or so I argued) - the schema hierarchy was not type-theoretically correct. I merged a change into the draft so that all requests inherit from JSONRPCRequest, and notifications extend JSONRPCNotification - see modelcontextprotocol/modelcontextprotocol#1026 for more info if interested. I hope this is relevant, sorry if it's not, just flagging that there is an upcoming change surrounding this part of the schema.

sreenaths force-pushed the setting-default-jsonrpc-version branch from 18051bc to becfff2 Compare September 19, 2025 15:54

sreenaths commented Sep 19, 2025

Copy link
Copy Markdown
Contributor Author

Thank you for the review, @ochafik.

@hesreallyhim It looks like your changes are one level down the class hierarchy, so I believe our changes are independent.

Thanks, @Kludex. Will mcp_types be included in the Python SDK? As @ochafik mentioned, does mcp_types set literal values as defaults? From what I can see, it only defines the types here: mcp_types

sreenaths force-pushed the setting-default-jsonrpc-version branch from becfff2 to 7e2292a Compare September 23, 2025 05:23
felixweinberger self-assigned this Sep 30, 2025
felixweinberger added needs maintainer Potentially serious issue - needs proactive fix and maintainer attention enhancement and removed needs more eyes labels Sep 30, 2025

sreenaths commented Sep 30, 2025

Copy link
Copy Markdown
Contributor Author

Thank you, @felixweinberger
Hope this can be merged soon.

felixweinberger removed their assignment Oct 10, 2025
sreenaths force-pushed the setting-default-jsonrpc-version branch from 7e2292a to 43ecf96 Compare October 19, 2025 07:03
felixweinberger added enhancement Request for a new feature that's not currently supported and removed enhancement deprecated labels Nov 12, 2025

Kludex commented Feb 11, 2026

Copy link
Copy Markdown
Member

Closing this as stale. I think this is not that important since it's for internal code.

Kludex closed this Feb 11, 2026
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ochafik ochafik approved these changes
ihrpr Awaiting requested review from ihrpr
+1 more reviewer
hesreallyhim hesreallyhim left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement Request for a new feature that's not currently supported needs maintainer Potentially serious issue - needs proactive fix and maintainer attention

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL