Repository navigation
fix: advertise Data API clause JSON in MCP schemas - #2619
Conversation
RED: schema contract tests compile and execute on Java 21, with seven schema failures and one malformed OpenAPI example error.
GREEN: 124 MCP schema, parser, builder, and ObjectMapper tests pass on Java 21. Schema examples validate and deserialize, and rerank override fields remain scoped to the request.
Align the HTTP and MCP hybrid descriptions. All 124 targeted tests, formatting, and the packaged build pass on Java 21.
📈 Unit Test Coverage Delta vs Main Branch
|
Unit Test Coverage Report
|
📈 Integration Test Coverage Delta vs Main Branch (dse69-it)
|
Integration Test Coverage Report (dse69-it)
|
📈 Integration Test Coverage Delta vs Main Branch (hcd-it)
|
Integration Test Coverage Report (hcd-it)
|
RED: McpProtocolTest executes against MCP 1.13.0 and fails because the first tools/list POST returns HTTP 400 instead of 200.
GREEN: the same raw HTTP McpProtocolTest passes with MCP BOM 2.0.3 on Quarkus 3.38.1 and JDK 21. Both independent tools/list requests return complete results without Mcp-Session-Id.
Both DSE and HCD CI exposed four obsolete empty-metadata assertions after the framework upgrade. Require only serverInfo and validate its name and version. The two affected DSE integration classes pass locally: 17 tests, zero failures.
amorton
left a comment
amorton
left a comment
There was a problem hiding this comment.
I see where this is going, it's a bit dangerour having multi line strings that must be valid JSON, and we only find out if it is not at request time.
Also, can we collapse into the McpSchemaDescriptionCustomizer ?
The handwritten schema JSON has been replaced with shared @Schema definitions, and the clause and description customizers are combined into McpSchemaCustomizer. |
amorton
left a comment
amorton
left a comment
There was a problem hiding this comment.
the way it is working looks better, but the code quality is still needs work.
I dont want to being in code if we dont know how it works.
| private record AdaptedInputSchema(JsonObject value) implements InputSchema { | ||
| @Override | ||
| public String asJson() { | ||
| return value.encode(); |
There was a problem hiding this comment.
WHY using .encode and not toString() like other code in this PR
There was a problem hiding this comment.
Fixed in 6e2cf87. It now uses toString(), matching the library's own InputSchema implementation. The output is the same either way.
| * preserves validation constraints without expanding or merging schemas. | ||
| */ | ||
| @Decorator | ||
| @Priority(1) |
There was a problem hiding this comment.
Fixed in 6e2cf87 with a comment. @Decorator wraps the MCP library's generator instead of replacing it, and CDI enables a decorator for the whole app only when it has @Priority. The value only orders decorators on the same bean, and this is the only one.
| * shapes that differ from the Java fields, so those types need overrides. Ordinary records still | ||
| * use field-based generation. | ||
| * | ||
| * <p>Named {@code @Schema} definitions refer to the schemas Quarkus already generates for Swagger. |
There was a problem hiding this comment.
nit: polite to say this includes the code that was in McpSchemaDescriptionCustomizer
There was a problem hiding this comment.
Added a line to the class doc saying it now includes the description resolver from McpSchemaDescriptionCustomizer.
| @Singleton | ||
| public class McpSchemaCustomizer implements SchemaGeneratorConfigCustomizer { | ||
| @Override | ||
| public void customize(SchemaGeneratorConfigBuilder builder) { |
There was a problem hiding this comment.
pls explain what why any of this happens, it generally looks like default behaviour
There was a problem hiding this comment.
Added this to the class doc, plus a short comment on each block. The MCP server builds its generator with the victools PLAIN_JSON preset and no modules, so by default it reads no annotations, and each block adds one back.
| return null; | ||
| } | ||
| var reference = annotation.ref(); | ||
| if (reference.isEmpty()) { |
There was a problem hiding this comment.
use isBlank() to test strings
There was a problem hiding this comment.
Switched the ref, name, and description checks to isBlank(). The @JsonProperty check stays isEmpty() for the Jackson reason on the earlier thread.
Use the reviewer's filter and sort descriptions, drop trailing sentences from schema descriptions, align example text blocks, restore the JsonDefinition field comment, rename SCHEMA_MAPS to SCHEMA_OBJECTS, use isBlank for annotation strings, and note that McpSchemaCustomizer absorbed McpSchemaDescriptionCustomizer.
Replace @Schema(hidden) with @JsonIgnore on clause wrapper state, restore Map.class on FindAndRerankSort, explain why HybridLimits sets implementation = Void.class, document the schema-only classes, nest the hybrid limit values schema inside HybridLimits, and explain each McpSchemaCustomizer block. The generated OpenAPI document is unchanged.
Address review feedback on McpOpenApiSchemaAdapter. - Inject the delegate through the constructor and read the generated OpenAPI document there. A missing document now stops startup instead of failing tools/list requests, and the synchronized lazy loader is gone. - Replace adapt() with static bundleOpenApiSchemas(), which changes the given schema in place. This drops the input deepCopy, and copyReferencedSchemas() returns its copies instead of filling a parameter. - Convert the bundled tree straight to the Vert.x object the MCP library sends, removing the second JSON text round trip. - Add OpenApiConstants.Components for the components/schemas pointer and ref prefix, plus componentRef() and named JSON Pointer escape helpers shared with McpSchemaCustomizer. - Document the class, @priority, the $defs prefix, the schema keyword sets, and the argument pointer fix.
Bring the findAndRerank tool arguments to one standard, following review feedback. - filter now takes its description from FilterDefinition, as sort already does from FindAndRerankSort. A @ToolArg description replaces the class description, so it was showing only "filter". - projection and options get real descriptions in place of "projection" and "Reranking options". - collection says "Name of the collection", since findAndRerank has no table support. Add contract tests that the filter description comes from its class and that no argument is described only by its own name.
Apply the findAndRerank argument standard to all MCP tools, following review feedback. - filter and sort arguments take their descriptions from FilterDefinition and SortDefinition, which Swagger also uses. A @ToolArg description replaced them, so clients saw only "filter" or "sort". - Collection arguments name what the command supports: the collection, the table, or a collection or table, based on CommandName targets. - Projection, update, replacement, document, definition, and options arguments get real descriptions in place of their bare names or generic text. - Descriptions drop "Optional" and "Required" prefixes, since the schema already lists required arguments, and use consistent capitalization. Generalize the contract tests to every tool: filter and sort use their class descriptions, and no argument is described only by its name or starts with "Optional" or "Required".
This reverts commit 94d14c7.
| name = "HybridSort", | ||
| type = SchemaType.OBJECT, | ||
| implementation = Object.class, | ||
| additionalProperties = Schema.False.class, |
There was a problem hiding this comment.
how does it look when we call with some {"foo": 1} with and without this additionalProperties?
amorton
left a comment
amorton
left a comment
There was a problem hiding this comment.
+1 discussed with Eric, some tidy up, and some comments to help with later refactor if needed
amorton
left a comment
amorton
left a comment
There was a problem hiding this comment.
+1 discussed with Eric, some tidy up, and some comments to help with later refactor if needed
…chemas # Conflicts: # src/test/java/io/stargate/sgv2/jsonapi/api/v1/mcp/McpHttpTestSupport.java # src/test/java/io/stargate/sgv2/jsonapi/api/v1/mcp/McpIntegrationTestBase.java # src/test/java/io/stargate/sgv2/jsonapi/api/v1/mcp/McpProtocolTest.java
What this PR does:
Uses OpenAPI annotations on the owning models to describe clause JSON accepted by Data API parsers. The existing generator remains the fallback. Nested authentication and parameters maps in rerank overrides accept empty and null values, including authentication maps for shared-secret providers. Contract tests compare schema acceptance with runtime behavior.
This builds on #2644. Merge #2618 first so the parser and schema reject the same conflicting vector sort keys.
Which issue(s) this PR fixes:
Addresses the schema portion of #2604.
Verification
All 89 focused tests passed with no failures, errors, or skips on this branch with the cleanup.
JAVA_HOME=/Library/Java/JavaVirtualMachines/graalvm-21.jdk/Contents/Home ./mvnw -B -ntp test -Dtest=McpProtocolTest,McpClauseSchemaContractTest,McpOpenApiSchemaAdapterTest,McpSchemaCustomizerTest -DskipITs -Dxml-format.skip=truePackaged integration tests and manual client checks were not rerun in this sweep.
Checklist