-
Notifications
You must be signed in to change notification settings - Fork 3k
Proper handle extra data in MCP objects #1937
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/mcp/types/_types.py
Outdated
| class GetTaskPayloadResult(Result): | ||
| """The response to a tasks/result request. | ||
| The structure matches the result type of the original request. | ||
| For example, a tools/call task would return the CallToolResult structure. | ||
| """ | ||
|
|
||
| model_config = ConfigDict(extra="allow") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This matches the additionalProperties in the specification.
|
|
||
| # TODO(Marcelo): The extra="allow" should be only on specific types e.g. `Meta`, not on the base class. | ||
| model_config = ConfigDict(extra="allow", alias_generator=to_camel, populate_by_name=True) | ||
| model_config = ConfigDict(alias_generator=to_camel, populate_by_name=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now properly done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(quick comment before I review) just to link related things, there's ongoing discussions around this here: modelcontextprotocol/modelcontextprotocol#2084
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we should have it just ignore extra fields. Wouldn't this cause errors if a newer server sends back extra stuff in an object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ignoring extra fields now. That's what this PR is doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 3 options: extra being allow, ignore or forbid. By default it's ignore - which is now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohh I thought default was forbid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool!
| "httpx-sse>=0.4", | ||
| "pydantic>=2.12.0; python_version >= '3.14'", | ||
| "pydantic>=2.11.0; python_version < '3.14'", | ||
| "pydantic>=2.12.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to bump it for this to work because the extra="allow" behavior doesn't work properly in TypedDicts on previous versions. Given that we are forced to do it on 3.14, I think it makes sense to change this to have a clean state here.
Code Review - Issue 1: model_config overwrites parent configurationFile: Issue: The In Pydantic v2, when a child class defines model_config = ConfigDict(alias_generator=to_camel, populate_by_name=True)See: python-sdk/src/mcp/types/_types.py Lines 33 to 36 in 701779a
By setting
This breaks serialization consistency for this class compared to all other MCP types. Fix: Merge the configs instead of replacing: |
Code Review - Issue 2: Missing tests for behavior changeReference: CLAUDE.md Testing Requirements This PR removes
According to CLAUDE.md:
No tests were added in this PR to verify:
Please add tests to cover this validation behavior change. |
Code Review - Issue 3: Breaking changes not documented in migration guideReference: CLAUDE.md Breaking Changes This PR introduces two breaking changes that require migration documentation: 1. Removal of
|
…hon-sdk into fix-loose-extra
|
Ah, now I understand what the comment about: |
Answering my own question: I checked the C# SDK, and they just ignore the fields in older SDK versions, which makes completely sense since you can't handle those fields - but still doesn't error, which is what we want here! |
this follows the typescript approach as well, so sounds good to me too |
Now properly matching the specification.