feat: Add Tool.outputSchema and CallToolResult.structuredContent #302
Conversation
- Implement support for outputSchema in Tool and structuredContent in CallToolResult. - Add a client-side cache that maintains track of all server tools and their output schemas (if any). - Implement a mechanism to refresh the client side cache when encountering a tool call request for an uncached tool. - Implement json validation between tool's output schema and result's structured content, when an output schema is specified for the tool.
mcp-test/src/main/java/io/modelcontextprotocol/server/.LCKAbstractMcpSyncServerTests.java~
Outdated
Show resolved
Hide resolved
| <!-- Json validator dependency --> | ||
| <dependency> | ||
| <groupId>com.networknt</groupId> | ||
| <artifactId>json-schema-validator</artifactId> | ||
| <version>1.5.7</version> | ||
| </dependency> |
There was a problem hiding this comment.
Worth getting extra clarification from maintainers on if we want schema validation as part of this or a separate effort, along the lines of the discussion in #271.
There was a problem hiding this comment.
Discussed some of the issues offline with @LucaButBoring. I wanted to get clarity around the scope of the work on the issue that I had created. There are a few questions that we might want to answer regarding the output validation:
- Do we want validation to be part of this PR or do we want to separate it out and think more about how we can cache/validate the results?
- Should cache be in the client class or should we construct a separate cache class?
- Should there be refresh intervals for cache invalidation? How else can we deal with it and should it be configurable by the client?
- Is a Map the best way to represent cache or should we opt for an in-memory cache implementation like Guava Cache?
Would like inputs from the maintainers to decide on the best way to proceed.
mcp/src/main/java/io/modelcontextprotocol/client/McpAsyncClient.java
Outdated
Show resolved
Hide resolved
mcp/src/main/java/io/modelcontextprotocol/client/McpSyncClient.java
Outdated
Show resolved
Hide resolved
| catch (JsonProcessingException e) { | ||
| // Log error if output schema can't be parsed to prevent erroring out for | ||
| // successful call tool request | ||
| logger.error("Encountered exception when parsing outputSchema: {}", e); | ||
| } |
There was a problem hiding this comment.
Worth getting other opinions on this, I think this should be rethrown (potentially wrapped in McpError to keep it unchecked).
There was a problem hiding this comment.
I agree. For now, changing this to log a warning. This leaves the option open for the client side implementation to proceed as it deems fit. I would like feedback from others about what would be the ideal behavior in this scenario.
mcp/src/main/java/io/modelcontextprotocol/client/McpSyncClient.java
Outdated
Show resolved
Hide resolved
|
Just gave this a preliminary review by request, review done for now. |
|
Addressed all comments from @LucaButBoring barring a few that where I would like a discussion with maintainers/others from the community. |
Motivation and Context
This PR adds support for Tool.outputSchema and CallToolResult.structuredContent that was introduced in the specification in modelcontextprotocol/modelcontextprotocol@03dc24b.
Resolves #285
Changes in this PR:
How Has This Been Tested?
Breaking Changes
A new field for structuredContent was added to CallToolResult as part of the changes. CallToolResult fields were modified from (List<Content> content, Boolean isError) -> (List<Content> content, Boolean isError, Map<String, Object> structuredContent).
In order to maintain backward compatibility with earlier initializations of CallToolResult that relied on just specifying (List<Content> , Boolean), a new constructor was created that would accept (List<Content> , Boolean). This ensured that no further changes were required to the unit tests across java-sdk. There is already a preexisting constructor for CallToolResult that accepts (String, Boolean).
The addition of the new constructor can lead to constructor ambiguity in case a CallToolResult was created with content set to null (since null can be either String or List). Example, CallToolResult(null, true). Such initializations would now require explicit typecasting to ensure that they are compiled successfully. This can be treated as a breaking change. I am open to suggestions on a better way to tackle this.
Types of changes
Checklist