Skip to content

Conversation

@findleyr
Copy link
Contributor

@findleyr findleyr commented Sep 11, 2025

Our validation logic was avoiding double-unmarshalling as much as
possible, by parsing before validation and validating the Go type.

This only works if the Go type has the same structure as its JSON
representation, which may not be the case in the presence of types with
custom MarshalJSON or UnmarshalJSON methods (such as time.Time).

But even if the Go type doesn't use any custom marshalling, validation
is broken, because we can't differentiate zero values from missing
values.

Bite the bullet and use double-unmarshalling for both input and output
schemas. Coincidentally, this fixes three bugs:

  • We were accepting case-insensitive JSON keys, since we parsed first,
    even though they should have been rejected. A number of tests were
    wrong.
  • Defaults were overriding present-yet-zero values, as noted in an
    incorrect test case.
  • When "arguments" was missing, validation wasn't performed, no defaults
    were applied, and unmarshalling failed even if all properties were
    optional.

First unmarshalling to map[string]any allows us to fix all these bugs.
Unfortunately, it means a 3x increase in the number of reflection
operations (we need to unmarshal, apply defaults and validate,
re-marshal with the defaults, and then unmarshal into the Go type).
However, this is not likely to be a significant overhead, and we can
always optimize in the future.

Update github.com/google/jsonschema-go to pick up necessary improvements
supporting this change.

Additionally, fix the error codes for invalid tool parameters, to be
consistent with other SDKs (Invalid Params: -32602).

Fixes #447
Fixes #449
Updates #450

Our validation logic was avoiding double-unmarshalling as much as
possible, by parsing before validation and validating the Go type.

This only works if the Go type has the same structure as its JSON
representation, which may not be the case in the presence of types with
custom MarshalJSON or UnmarshalJSON methods (such as time.Time).

But even if the Go type doesn't use any custom marshalling, validation
is broken, because we can't differentiate zero values from missing
values.

Bite the bullet and use double-unmarshalling for both input and output
schemas. Coincidentally, this fixes three bugs:
- We were accepting case-insensitive JSON keys, since we parsed first,
  even though they should have been rejected. A number of tests were
  wrong.
- Defaults were overriding present-yet-zero values, as noted in an
  incorrect test case.
- When "arguments" was missing, validation wasn't performed, no defaults
  were applied, and unmarshalling failed even if all properties were
  optional.

First unmarshalling to map[string]any allows us to fix all these bugs.
Unfortunately, it means a 3x increase in the number of reflection
operations (we need to unmarshal, apply defaults and validate,
re-marshal with the defaults, and then unmarshal into the Go type).
However, this is not likely to be a significant overhead, and we can
always optimize in the future.

Update github.com/google/jsonschema-go to pick up necessary improvements
supporting this change.

Additionally, fix the error codes for invalid tool parameters, to be
consistent with other SDKs (Invalid Params: -32602).

Fixes modelcontextprotocol#447
Fixes modelcontextprotocol#449
Updates modelcontextprotocol#450
@findleyr findleyr requested review from jba and samthanawalla and removed request for jba September 11, 2025 22:19
@findleyr findleyr merged commit 9f1b864 into modelcontextprotocol:main Sep 12, 2025
5 checks passed
@findleyr findleyr deleted the fixvalidation branch September 25, 2025 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Input validation doesn't catch missing fields Validate tool output fails for time.Time and other types that get serialized to strings in JSON

2 participants