mcp: propertly validate against JSON, independent of Go values
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
This commit is contained in:
parent
eddef064df
commit
9f1b8646bd
1 changed files with 287 additions and 85 deletions
127
mcp/testdata/conformance/server/tools.txtar
vendored
127
mcp/testdata/conformance/server/tools.txtar
vendored
|
|
@ -5,10 +5,17 @@ Fixed bugs:
|
|||
- "_meta" should not be nil
|
||||
- empty resource or prompts should not be returned as 'null'
|
||||
- the server should not crash when params are passed to tools/call
|
||||
- missing required input fields should be rejected (#449)
|
||||
- output and input should be validated against their actual json, not Go
|
||||
representation
|
||||
- When arguments are missing, the request should succeed if all properties are
|
||||
optional, and observe any default values.
|
||||
|
||||
-- tools --
|
||||
greet
|
||||
structured
|
||||
tomorrow
|
||||
inc
|
||||
|
||||
-- client --
|
||||
{
|
||||
|
|
@ -26,9 +33,14 @@ structured
|
|||
{ "jsonrpc": "2.0", "id": 3, "method": "resources/list" }
|
||||
{ "jsonrpc": "2.0", "id": 4, "method": "prompts/list" }
|
||||
{ "jsonrpc": "2.0", "id": 5, "method": "tools/call" }
|
||||
{ "jsonrpc": "2.0", "id": 6, "method": "tools/call", "params": {"name": "greet", "arguments": {"name": "you"} } }
|
||||
{ "jsonrpc": "2.0", "id": 6, "method": "tools/call", "params": {"name": "greet", "arguments": {"Name": "you"} } }
|
||||
{ "jsonrpc": "2.0", "id": 1, "result": {} }
|
||||
{ "jsonrpc": "2.0", "id": 7, "method": "tools/call", "params": {"name": "structured", "arguments": {"In": "input"} } }
|
||||
{ "jsonrpc": "2.0", "id": 8, "method": "tools/call", "params": {"name": "structured", "arguments": {} } }
|
||||
{ "jsonrpc": "2.0", "id": 9, "method": "tools/call", "params": {"name": "tomorrow", "arguments": { "Now": "2025-06-18T15:04:05Z" } } }
|
||||
{ "jsonrpc": "2.0", "id": 10, "method": "tools/call", "params": {"name": "greet" } }
|
||||
{ "jsonrpc": "2.0", "id": 11, "method": "tools/call", "params": {"name": "inc", "arguments": { "x": 3 } } }
|
||||
{ "jsonrpc": "2.0", "id": 11, "method": "tools/call", "params": {"name": "inc" } }
|
||||
|
||||
-- server --
|
||||
{
|
||||
|
|
@ -69,6 +81,31 @@ structured
|
|||
},
|
||||
"name": "greet"
|
||||
},
|
||||
{
|
||||
"inputSchema": {
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"x": {
|
||||
"type": "integer",
|
||||
"default": 6
|
||||
}
|
||||
},
|
||||
"additionalProperties": false
|
||||
},
|
||||
"name": "inc",
|
||||
"outputSchema": {
|
||||
"type": "object",
|
||||
"required": [
|
||||
"y"
|
||||
],
|
||||
"properties": {
|
||||
"y": {
|
||||
"type": "integer"
|
||||
}
|
||||
},
|
||||
"additionalProperties": false
|
||||
}
|
||||
},
|
||||
{
|
||||
"inputSchema": {
|
||||
"type": "object",
|
||||
|
|
@ -97,6 +134,33 @@ structured
|
|||
},
|
||||
"additionalProperties": false
|
||||
}
|
||||
},
|
||||
{
|
||||
"inputSchema": {
|
||||
"type": "object",
|
||||
"required": [
|
||||
"Now"
|
||||
],
|
||||
"properties": {
|
||||
"Now": {
|
||||
"type": "string"
|
||||
}
|
||||
},
|
||||
"additionalProperties": false
|
||||
},
|
||||
"name": "tomorrow",
|
||||
"outputSchema": {
|
||||
"type": "object",
|
||||
"required": [
|
||||
"Tomorrow"
|
||||
],
|
||||
"properties": {
|
||||
"Tomorrow": {
|
||||
"type": "string"
|
||||
}
|
||||
},
|
||||
"additionalProperties": false
|
||||
}
|
||||
}
|
||||
]
|
||||
}
|
||||
|
|
@ -155,3 +219,64 @@ structured
|
|||
}
|
||||
}
|
||||
}
|
||||
{
|
||||
"jsonrpc": "2.0",
|
||||
"id": 8,
|
||||
"error": {
|
||||
"code": -32602,
|
||||
"message": "invalid params: validating \"arguments\": validating root: required: missing properties: [\"In\"]"
|
||||
}
|
||||
}
|
||||
{
|
||||
"jsonrpc": "2.0",
|
||||
"id": 9,
|
||||
"result": {
|
||||
"content": [
|
||||
{
|
||||
"type": "text",
|
||||
"text": "{\"Tomorrow\":\"2025-06-19T15:04:05Z\"}"
|
||||
}
|
||||
],
|
||||
"structuredContent": {
|
||||
"Tomorrow": "2025-06-19T15:04:05Z"
|
||||
}
|
||||
}
|
||||
}
|
||||
{
|
||||
"jsonrpc": "2.0",
|
||||
"id": 10,
|
||||
"error": {
|
||||
"code": -32602,
|
||||
"message": "invalid params: validating \"arguments\": validating root: required: missing properties: [\"Name\"]"
|
||||
}
|
||||
}
|
||||
{
|
||||
"jsonrpc": "2.0",
|
||||
"id": 11,
|
||||
"result": {
|
||||
"content": [
|
||||
{
|
||||
"type": "text",
|
||||
"text": "{\"y\":4}"
|
||||
}
|
||||
],
|
||||
"structuredContent": {
|
||||
"y": 4
|
||||
}
|
||||
}
|
||||
}
|
||||
{
|
||||
"jsonrpc": "2.0",
|
||||
"id": 11,
|
||||
"result": {
|
||||
"content": [
|
||||
{
|
||||
"type": "text",
|
||||
"text": "{\"y\":7}"
|
||||
}
|
||||
],
|
||||
"structuredContent": {
|
||||
"y": 7
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue