-
Notifications
You must be signed in to change notification settings - Fork 864
feat: Add SEP-1034 elicitation schema default values #908
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -431,19 +431,34 @@ public record Sampling() { | |
| * @param url support for out-of-band URL-based elicitation | ||
| */ | ||
| @JsonInclude(JsonInclude.Include.NON_ABSENT) | ||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| public record Elicitation(@JsonProperty("form") Form form, @JsonProperty("url") Url url) { | ||
|
|
||
| /** | ||
| * Marker record indicating support for form-based elicitation mode. | ||
| * Record indicating support for form-based elicitation mode. | ||
| * | ||
| * @param applyDefaults Whether the client should apply default values from | ||
| * the schema to the elicitation result content when fields are missing. When | ||
| * true, the SDK will automatically fill in missing fields with their | ||
| * schema-defined defaults before returning the result to the server. | ||
| */ | ||
| @JsonInclude(JsonInclude.Include.NON_ABSENT) | ||
| public record Form() { | ||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| public record Form(@JsonProperty("applyDefaults") Boolean applyDefaults) { | ||
|
|
||
| /** | ||
| * Creates a Form with default settings (no applyDefaults). | ||
| */ | ||
| public Form() { | ||
| this(null); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Marker record indicating support for URL-based elicitation mode. | ||
| */ | ||
| @JsonInclude(JsonInclude.Include.NON_ABSENT) | ||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| public record Url() { | ||
| } | ||
|
|
||
|
|
@@ -507,6 +522,20 @@ public Builder elicitation(boolean form, boolean url) { | |
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Enables elicitation capability with form mode and applyDefaults setting. | ||
| * @param form whether to support form-based elicitation | ||
| * @param url whether to support URL-based elicitation | ||
| * @param applyDefaults whether the client should apply schema defaults to | ||
| * elicitation results | ||
| * @return this builder | ||
| */ | ||
| public Builder elicitation(boolean form, boolean url, boolean applyDefaults) { | ||
| this.elicitation = new Elicitation(form ? new Elicitation.Form(applyDefaults) : null, | ||
| url ? new Elicitation.Url() : null); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Consider keeping
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch on the interoperability concern. This follows the TypeScript SDK's design — The Java SDK already uses That said, if the maintainers prefer keeping it outside the protocol record (separate builder flag), happy to refactor. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new builder overload At minimum, add a guard: if (!form && applyDefaults) {
throw new IllegalArgumentException("applyDefaults requires form to be true");
}Or document the behavior clearly in the Javadoc.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. Added an |
||
| return this; | ||
| } | ||
|
|
||
| public ClientCapabilities build() { | ||
| return new ClientCapabilities(experimental, roots, sampling, elicitation); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,151 @@ | ||
| /* | ||
| * Copyright 2024-2024 the original author or authors. | ||
|
||
| */ | ||
|
|
||
| package io.modelcontextprotocol.client; | ||
|
|
||
| import java.util.HashMap; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| /** | ||
| * Tests for {@link McpAsyncClient#applyElicitationDefaults(Map, Map)}. | ||
| * | ||
| * Verifies that the client-side default application logic correctly fills in missing | ||
| * fields from schema defaults, matching the behavior specified in SEP-1034. | ||
| */ | ||
| class McpAsyncClientElicitationDefaultsTests { | ||
|
|
||
| @Test | ||
| void appliesStringDefault() { | ||
| Map<String, Object> schema = Map.of("properties", Map.of("name", Map.of("type", "string", "default", "Guest"))); | ||
|
|
||
| Map<String, Object> content = new HashMap<>(); | ||
| McpAsyncClient.applyElicitationDefaults(schema, content); | ||
|
|
||
| assertThat(content).containsEntry("name", "Guest"); | ||
| } | ||
|
|
||
| @Test | ||
| void appliesNumberDefault() { | ||
| Map<String, Object> schema = Map.of("properties", Map.of("age", Map.of("type", "integer", "default", 18))); | ||
|
|
||
| Map<String, Object> content = new HashMap<>(); | ||
| McpAsyncClient.applyElicitationDefaults(schema, content); | ||
|
|
||
| assertThat(content).containsEntry("age", 18); | ||
| } | ||
|
|
||
| @Test | ||
| void appliesBooleanDefault() { | ||
| Map<String, Object> schema = Map.of("properties", | ||
| Map.of("subscribe", Map.of("type", "boolean", "default", true))); | ||
|
|
||
| Map<String, Object> content = new HashMap<>(); | ||
| McpAsyncClient.applyElicitationDefaults(schema, content); | ||
|
|
||
| assertThat(content).containsEntry("subscribe", true); | ||
| } | ||
|
|
||
| @Test | ||
| void appliesEnumDefault() { | ||
| Map<String, Object> schema = Map.of("properties", | ||
| Map.of("color", Map.of("type", "string", "enum", List.of("red", "green"), "default", "green"))); | ||
|
|
||
| Map<String, Object> content = new HashMap<>(); | ||
| McpAsyncClient.applyElicitationDefaults(schema, content); | ||
|
|
||
| assertThat(content).containsEntry("color", "green"); | ||
| } | ||
|
|
||
| @Test | ||
| void doesNotOverrideExistingValues() { | ||
| Map<String, Object> schema = Map.of("properties", Map.of("name", Map.of("type", "string", "default", "Guest"))); | ||
|
|
||
| Map<String, Object> content = new HashMap<>(); | ||
| content.put("name", "Alice"); | ||
| McpAsyncClient.applyElicitationDefaults(schema, content); | ||
|
|
||
| assertThat(content).containsEntry("name", "Alice"); | ||
| } | ||
|
|
||
| @Test | ||
| void skipsPropertiesWithoutDefault() { | ||
| Map<String, Object> schema = Map.of("properties", Map.of("email", Map.of("type", "string"))); | ||
|
|
||
| Map<String, Object> content = new HashMap<>(); | ||
| McpAsyncClient.applyElicitationDefaults(schema, content); | ||
|
|
||
| assertThat(content).doesNotContainKey("email"); | ||
| } | ||
|
|
||
| @Test | ||
| void appliesMultipleDefaults() { | ||
| Map<String, Object> schema = Map.of("properties", | ||
| Map.of("name", Map.of("type", "string", "default", "Guest"), "age", | ||
| Map.of("type", "integer", "default", 18), "subscribe", | ||
| Map.of("type", "boolean", "default", true), "color", | ||
| Map.of("type", "string", "enum", List.of("red", "green"), "default", "green"))); | ||
|
|
||
| Map<String, Object> content = new HashMap<>(); | ||
| McpAsyncClient.applyElicitationDefaults(schema, content); | ||
|
|
||
| assertThat(content).containsEntry("name", "Guest") | ||
| .containsEntry("age", 18) | ||
| .containsEntry("subscribe", true) | ||
| .containsEntry("color", "green"); | ||
| } | ||
|
|
||
| @Test | ||
| void handlesNullSchema() { | ||
| Map<String, Object> content = new HashMap<>(); | ||
| McpAsyncClient.applyElicitationDefaults(null, content); | ||
|
|
||
| assertThat(content).isEmpty(); | ||
| } | ||
|
|
||
| @Test | ||
| void handlesNullContent() { | ||
| Map<String, Object> schema = Map.of("properties", Map.of("name", Map.of("type", "string", "default", "Guest"))); | ||
|
|
||
| // Should not throw | ||
| McpAsyncClient.applyElicitationDefaults(schema, null); | ||
| } | ||
|
|
||
| @Test | ||
| void handlesSchemaWithoutProperties() { | ||
| Map<String, Object> schema = Map.of("type", "object"); | ||
|
|
||
| Map<String, Object> content = new HashMap<>(); | ||
| McpAsyncClient.applyElicitationDefaults(schema, content); | ||
|
|
||
| assertThat(content).isEmpty(); | ||
| } | ||
|
|
||
| @Test | ||
| void appliesDefaultsOnlyToMissingFields() { | ||
| Map<String, Object> schema = Map.of("properties", Map.of("name", Map.of("type", "string", "default", "Guest"), | ||
| "age", Map.of("type", "integer", "default", 18))); | ||
|
|
||
| Map<String, Object> content = new HashMap<>(); | ||
| content.put("name", "John"); | ||
| McpAsyncClient.applyElicitationDefaults(schema, content); | ||
|
|
||
| assertThat(content).containsEntry("name", "John").containsEntry("age", 18); | ||
| } | ||
|
|
||
| @Test | ||
| void appliesFloatingPointDefault() { | ||
| Map<String, Object> schema = Map.of("properties", Map.of("score", Map.of("type", "number", "default", 95.5))); | ||
|
|
||
| Map<String, Object> content = new HashMap<>(); | ||
| McpAsyncClient.applyElicitationDefaults(schema, content); | ||
|
|
||
| assertThat(content).containsEntry("score", 95.5); | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -446,6 +446,76 @@ void testCreateElicitationSuccess(String clientType) { | |
| } | ||
| } | ||
|
|
||
| @ParameterizedTest(name = "{0} : {displayName} ") | ||
| @MethodSource("clientsForTesting") | ||
| void testCreateElicitationWithApplyDefaults(String clientType) { | ||
|
|
||
| var clientBuilder = clientBuilders.get(clientType); | ||
|
|
||
| // Client handler returns empty content — SDK should apply defaults | ||
| Function<McpSchema.ElicitRequest, McpSchema.ElicitResult> elicitationHandler = request -> { | ||
| assertThat(request.message()).isNotEmpty(); | ||
| assertThat(request.requestedSchema()).isNotNull(); | ||
| // Return accept with empty content, simulating a user who didn't fill | ||
| // anything | ||
| return new McpSchema.ElicitResult(McpSchema.ElicitResult.Action.ACCEPT, new java.util.HashMap<>()); | ||
| }; | ||
|
|
||
| CallToolResult callResponse = McpSchema.CallToolResult.builder() | ||
| .addContent(new McpSchema.TextContent("CALL RESPONSE")) | ||
| .build(); | ||
|
|
||
| AtomicReference<McpSchema.ElicitResult> elicitResultRef = new AtomicReference<>(); | ||
|
|
||
| McpServerFeatures.AsyncToolSpecification tool = McpServerFeatures.AsyncToolSpecification.builder() | ||
| .tool(Tool.builder().name("tool1").description("tool1 description").inputSchema(EMPTY_JSON_SCHEMA).build()) | ||
| .callHandler((exchange, request) -> { | ||
|
|
||
| var elicitationRequest = McpSchema.ElicitRequest.builder() | ||
| .message("Provide your preferences") | ||
| .requestedSchema(Map.of("type", "object", "properties", | ||
| Map.of("nickname", Map.of("type", "string", "default", "Guest"), "age", | ||
| Map.of("type", "integer", "default", 18), "subscribe", | ||
| Map.of("type", "boolean", "default", true), "color", | ||
| Map.of("type", "string", "enum", java.util.List.of("red", "green"), "default", | ||
| "green")), | ||
| "required", java.util.List.of("nickname", "age", "subscribe", "color"))) | ||
| .build(); | ||
|
|
||
| return exchange.createElicitation(elicitationRequest) | ||
| .doOnNext(elicitResultRef::set) | ||
| .thenReturn(callResponse); | ||
| }) | ||
| .build(); | ||
|
|
||
| var mcpServer = prepareAsyncServerBuilder().serverInfo("test-server", "1.0.0").tools(tool).build(); | ||
|
|
||
| // Enable applyDefaults via the capability | ||
| try (var mcpClient = clientBuilder.clientInfo(new McpSchema.Implementation("Sample client", "0.0.0")) | ||
| .capabilities(ClientCapabilities.builder().elicitation(true, false, true).build()) | ||
| .elicitation(elicitationHandler) | ||
| .build()) { | ||
|
|
||
| InitializeResult initResult = mcpClient.initialize(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The integration test only exercises the return new McpSchema.ElicitResult(McpSchema.ElicitResult.Action.ACCEPT, Map.of());This would surface the |
||
| assertThat(initResult).isNotNull(); | ||
|
|
||
| CallToolResult response = mcpClient.callTool(new McpSchema.CallToolRequest("tool1", Map.of())); | ||
|
|
||
| assertThat(response).isNotNull(); | ||
| assertWith(elicitResultRef.get(), result -> { | ||
| assertThat(result).isNotNull(); | ||
| assertThat(result.action()).isEqualTo(McpSchema.ElicitResult.Action.ACCEPT); | ||
| assertThat(result.content()).containsEntry("nickname", "Guest"); | ||
| assertThat(result.content()).containsEntry("age", 18); | ||
| assertThat(result.content()).containsEntry("subscribe", true); | ||
| assertThat(result.content()).containsEntry("color", "green"); | ||
| }); | ||
| } | ||
| finally { | ||
| mcpServer.closeGracefully().block(); | ||
| } | ||
| } | ||
|
|
||
| @ParameterizedTest(name = "{0} : {displayName} ") | ||
| @MethodSource("clientsForTesting") | ||
| void testCreateElicitationWithRequestTimeoutSuccess(String clientType) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
catch (Exception e)silently swallows anUnsupportedOperationExceptionif the user returns an unmodifiable map (e.g.,Map.of(...)orCollections.unmodifiableMap(...)). Defaults are silently not applied, with only a DEBUG log. This is a real correctness hazard because nothing in the API contract prevents users from returning an unmodifiable content map.The safer approach is to build a new merged map and return a new
ElicitResultrather than mutating in-place:This also removes the need for the try/catch entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good Catch!