fix: Remove JsonSchema and use a Map for inputSchema to support json schemas dialect#749
Open
bilaloumehdi wants to merge 7 commits intomodelcontextprotocol:mainfrom
Open
Conversation
Author
|
@tzolov conflicts have been resolved |
Kehrlann
requested changes
Apr 3, 2026
Contributor
Kehrlann
left a comment
There was a problem hiding this comment.
Thank you for your contribution!
I'd like to make the transition path smoother, by keeping the JsonSchema type around.
|
|
||
| public static final McpSchema.JsonSchema EMPTY_JSON_SCHEMA = new McpSchema.JsonSchema("object", | ||
| Collections.emptyMap(), null, null, null, null); | ||
|
|
Contributor
There was a problem hiding this comment.
Please keep the empty schema as a Map.of("type", "object", "properties", Collections.emptyMap()) ; same goes for mcp-test
This is closer to an empty schema.
Comment on lines
-1301
to
-1321
| /** | ||
| * A JSON Schema object that describes the expected structure of arguments or output. | ||
| * | ||
| * @param type The type of the schema (e.g., "object") | ||
| * @param properties The properties of the schema object | ||
| * @param required List of required property names | ||
| * @param additionalProperties Whether additional properties are allowed | ||
| * @param defs Schema definitions using the newer $defs keyword | ||
| * @param definitions Schema definitions using the legacy definitions keyword | ||
| */ | ||
| @JsonInclude(JsonInclude.Include.NON_ABSENT) | ||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| public record JsonSchema( // @formatter:off | ||
| @JsonProperty("type") String type, | ||
| @JsonProperty("properties") Map<String, Object> properties, | ||
| @JsonProperty("required") List<String> required, | ||
| @JsonProperty("additionalProperties") Boolean additionalProperties, | ||
| @JsonProperty("$defs") Map<String, Object> defs, | ||
| @JsonProperty("definitions") Map<String, Object> definitions) { // @formatter:on | ||
| } | ||
|
|
| return this; | ||
| } | ||
|
|
||
| public Builder inputSchema(JsonSchema inputSchema) { |
Contributor
There was a problem hiding this comment.
Keep this method with @Deprecated, and introduce your new method as well.
/**
* @deprecated use {@link #inputSchema(Map)} instead.
*/
@Deprecated
public Builder inputSchema(JsonSchema inputSchema) {
Map<String, Object> schema = new HashMap<>();
if (inputSchema.type() != null)
schema.put("type", inputSchema.type());
if (inputSchema.properties() != null)
schema.put("properties", inputSchema.properties());
if (inputSchema.required() != null)
schema.put("required", inputSchema.required());
if (inputSchema.additionalProperties() != null)
schema.put("additionalProperties", inputSchema.additionalProperties());
if (inputSchema.defs() != null)
schema.put("$defs", inputSchema.defs());
if (inputSchema.definitions() != null)
schema.put("definitions", inputSchema.definitions());
return inputSchema(schema);
}| private ToolsUtils() { | ||
| } | ||
|
|
||
| public static final McpSchema.JsonSchema EMPTY_JSON_SCHEMA = new McpSchema.JsonSchema("object", |
Contributor
There was a problem hiding this comment.
Keep as Map.of("type", "object", "properties", emptyMap());
Comment on lines
-848
to
+857
| assertThat(deserializedTool.inputSchema().defs()).isNotNull(); | ||
| assertThat(deserializedTool.inputSchema().defs()).containsKey("Address"); | ||
| Map<String, Object> defs = ((Map<String, Object>) deserializedTool.inputSchema().get("$defs")); | ||
| Map<String, Object> address = ((Map<String, Object>) defs.get("Address")); | ||
|
|
||
| assertThat(deserializedTool.inputSchema().containsKey("$defs")).isTrue(); | ||
| assertThat(address.get("type")).isEqualTo("object"); |
Contributor
There was a problem hiding this comment.
Instead:
// Just verify the basic structure was preserved
assertThat(deserializedTool.inputSchema()).containsKey("$defs")
.extractingByKey("$defs")
.isNotNull()
.asInstanceOf(InstanceOfAssertFactories.MAP)
.containsKey("Address");
Comment on lines
+860
to
+861
| @Test | ||
| void testToolWithSchemaThatDoesNotHavePropertiesAtTopLevel() throws Exception { |
Contributor
There was a problem hiding this comment.
What does this test vs the "complex schema" test above?
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Following the Model Context Protocol specification for JSON schemas usage, the schema should, by default follow the 2020-12 dialect: json-schema-2020-12 if no
$schemaspecified.Motivation and Context
The need for this change arose from the requirement for a tool that can handle one of two properties objects. This can be achieved by defining an
inputSchemawithoneOfat the top level, for example:The current state
The schema is currently deserialized to a
JsonSchemarecord and ignoresoneOfat the top level. A specific fix for this issue would be to add a new field to theJsonSchemarecord foroneOf; however, this approach does not scale well if additional top-level entries (such asallOf) are needed. This PR shifts to using aMap<String, Object>to deserialize theinputSchemaNext Steps and improvements
I suggest adding validation for schemas based on the schema specifications from the SDK, not only checking for serialization errors. This would help fail fast and provide feedback about any issues in the defined schemas.
How Has This Been Tested?
Existing Tests have been adjusted for these changes, and a new test has been introduced with json schemas that have
oneOfat the top level to verify that deserialization do not ignore it.Breaking Changes
Users would need to change the tool's inputSchema data type
Types of changes
Checklist
Additional context