feat!: #697 added tool input arguments validation#873
feat!: #697 added tool input arguments validation#873ashakirin wants to merge 5 commits intomodelcontextprotocol:mainfrom
Conversation
… causes tool execution error. Breaking change, because validation is activated by default
Kehrlann
added
area/server
P1
Kehrlann
left a comment
Kehrlann
left a comment
There was a problem hiding this comment.
Now that I'm revisiting this, I'm wondering whether we really want this system property to toggle this on or off.
We're going to ship this in 2.0, let it be a breaking change - after all, servers MUST validate tool inputs.
(And we should probably revisit tool name validation too, then)
mcp-core/src/main/java/io/modelcontextprotocol/util/ToolInputValidator.java
Outdated
Show resolved
Hide resolved
mcp-test/src/test/java/io/modelcontextprotocol/server/ToolInputValidationIntegrationTests.java
Outdated
Show resolved
Hide resolved
mcp-test/src/test/java/io/modelcontextprotocol/server/ToolInputValidationIntegrationTests.java
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,270 @@ | |||
| /* | |||
| * Copyright 2024-2024 the original author or authors. | |||
There was a problem hiding this comment.
| * Copyright 2024-2024 the original author or authors. | |
| * Copyright 2026-2026 the original author or authors. |
| } | ||
| Map<String, Object> inputSchema = jsonMapper.convertValue(tool.inputSchema(), | ||
| new TypeRef<Map<String, Object>>() { | ||
| }); |
There was a problem hiding this comment.
I wonder if we should leave this marshalling on the hot path. Look at what we've done in DefaultJsonSchemaValidator, there's a schema cache to avoid specifically doing to much of these operations.
Maybe we should have something similar here? Or maybe we should update the JsonSchemaValidator to also validate JsonSchema objects?
There was a problem hiding this comment.
thanks for pointing it, I will wait until #749 is merged
Kehrlann
added
waiting for user
…alidator.java Co-authored-by: Daniel Garnier-Moiroux <daniel.garnier-moiroux@broadcom.com>
…tValidationIntegrationTests.java Co-authored-by: Daniel Garnier-Moiroux <daniel.garnier-moiroux@broadcom.com>
|
@Kehrlann regarding toggling:
|
|
For toggling, I'm in favor of leaving an option to turn it off. But let's get rid of the system property - we don't use that pattern anywhere else in the codebase. |
2 participants
Added tool input arguments validation causes Tool Execution Error accordingly SEP-1303.
It is breaking change, because tool input validation is activated by default