Skip to content

Commit eaf75e8

Browse files
author
Sainath Reddy Bobbala
committed
fix: Address PR review feedback for SEP-1034
- Build new HashMap from content instead of mutating in-place, preventing UnsupportedOperationException with unmodifiable maps (Map.of()) - Add IllegalArgumentException guard when applyDefaults=true but form=false - Add Javadoc documenting applyDefaults as SDK-level behavior flag consistent with TypeScript SDK design - Add integration test for unmodifiable map content - Fix assertion to use assertThatJson node check instead of broad contains - Fix copyright year to 2024-2025
1 parent 7e99b5b commit eaf75e8

5 files changed

Lines changed: 93 additions & 25 deletions

File tree

‎mcp-core/src/main/java/io/modelcontextprotocol/client/McpAsyncClient.java‎

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,9 @@
1515
import java.util.concurrent.ConcurrentHashMap;
1616
import java.util.function.Function;
1717

18+
import org.slf4j.Logger;
19+
import org.slf4j.LoggerFactory;
20+
1821
import io.modelcontextprotocol.client.LifecycleInitializer.Initialization;
1922
import io.modelcontextprotocol.json.TypeRef;
2023
import io.modelcontextprotocol.json.schema.JsonSchemaValidator;
@@ -30,16 +33,14 @@
3033
import io.modelcontextprotocol.spec.McpSchema.ElicitResult;
3134
import io.modelcontextprotocol.spec.McpSchema.GetPromptRequest;
3235
import io.modelcontextprotocol.spec.McpSchema.GetPromptResult;
33-
import io.modelcontextprotocol.util.ToolNameValidator;
3436
import io.modelcontextprotocol.spec.McpSchema.ListPromptsResult;
3537
import io.modelcontextprotocol.spec.McpSchema.LoggingLevel;
3638
import io.modelcontextprotocol.spec.McpSchema.LoggingMessageNotification;
3739
import io.modelcontextprotocol.spec.McpSchema.PaginatedRequest;
3840
import io.modelcontextprotocol.spec.McpSchema.Root;
3941
import io.modelcontextprotocol.util.Assert;
42+
import io.modelcontextprotocol.util.ToolNameValidator;
4043
import io.modelcontextprotocol.util.Utils;
41-
import org.slf4j.Logger;
42-
import org.slf4j.LoggerFactory;
4344
import reactor.core.publisher.Flux;
4445
import reactor.core.publisher.Mono;
4546

@@ -565,13 +566,9 @@ private RequestHandler<ElicitResult> elicitationCreateHandler() {
565566
// Apply defaults from schema when applyDefaults is enabled
566567
if (result.action() == ElicitResult.Action.ACCEPT && result.content() != null
567568
&& shouldApplyElicitationDefaults()) {
568-
try {
569-
applyElicitationDefaults(request.requestedSchema(), result.content());
570-
}
571-
catch (Exception e) {
572-
// Gracefully ignore errors in default application
573-
logger.debug("Error applying elicitation defaults: {}", e.getMessage());
574-
}
569+
Map<String, Object> merged = new HashMap<>(result.content());
570+
applyElicitationDefaults(request.requestedSchema(), merged);
571+
return new ElicitResult(result.action(), merged, result.meta());
575572
}
576573
return result;
577574
});

‎mcp-core/src/main/java/io/modelcontextprotocol/spec/McpSchema.java‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,17 +11,19 @@
1111
import java.util.List;
1212
import java.util.Map;
1313

14+
import org.slf4j.Logger;
15+
import org.slf4j.LoggerFactory;
16+
1417
import com.fasterxml.jackson.annotation.JsonCreator;
1518
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
1619
import com.fasterxml.jackson.annotation.JsonInclude;
1720
import com.fasterxml.jackson.annotation.JsonProperty;
1821
import com.fasterxml.jackson.annotation.JsonSubTypes;
1922
import com.fasterxml.jackson.annotation.JsonTypeInfo;
23+
2024
import io.modelcontextprotocol.json.McpJsonMapper;
2125
import io.modelcontextprotocol.json.TypeRef;
2226
import io.modelcontextprotocol.util.Assert;
23-
import org.slf4j.Logger;
24-
import org.slf4j.LoggerFactory;
2527

2628
/**
2729
* Based on the <a href="http://www.jsonrpc.org/specification">JSON-RPC 2.0
@@ -524,13 +526,24 @@ public Builder elicitation(boolean form, boolean url) {
524526

525527
/**
526528
* Enables elicitation capability with form mode and applyDefaults setting.
529+
* <p>
530+
* Note: {@code applyDefaults} is an SDK-level behavior flag that controls
531+
* whether the client automatically fills in missing fields from schema
532+
* defaults. It is serialized as part of the capabilities sent to the server
533+
* during initialization, consistent with the TypeScript SDK behavior. Servers
534+
* should tolerate unknown capability fields per the MCP specification.
527535
* @param form whether to support form-based elicitation
528536
* @param url whether to support URL-based elicitation
529537
* @param applyDefaults whether the client should apply schema defaults to
530-
* elicitation results
538+
* elicitation results. Requires {@code form} to be {@code true}.
531539
* @return this builder
540+
* @throws IllegalArgumentException if {@code applyDefaults} is {@code true}
541+
* but {@code form} is {@code false}
532542
*/
533543
public Builder elicitation(boolean form, boolean url, boolean applyDefaults) {
544+
if (!form && applyDefaults) {
545+
throw new IllegalArgumentException("applyDefaults requires form to be true");
546+
}
534547
this.elicitation = new Elicitation(form ? new Elicitation.Form(applyDefaults) : null,
535548
url ? new Elicitation.Url() : null);
536549
return this;

‎mcp-core/src/test/java/io/modelcontextprotocol/client/McpAsyncClientElicitationDefaultsTests.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2024-2024 the original author or authors.
2+
* Copyright 2024-2025 the original author or authors.
33
*/
44

55
package io.modelcontextprotocol.client;
@@ -8,9 +8,8 @@
88
import java.util.List;
99
import java.util.Map;
1010

11-
import org.junit.jupiter.api.Test;
12-
1311
import static org.assertj.core.api.Assertions.assertThat;
12+
import org.junit.jupiter.api.Test;
1413

1514
/**
1615
* Tests for {@link McpAsyncClient#applyElicitationDefaults(Map, Map)}.

‎mcp-test/src/main/java/io/modelcontextprotocol/AbstractMcpClientServerIntegrationTests.java‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -516,6 +516,67 @@ void testCreateElicitationWithApplyDefaults(String clientType) {
516516
}
517517
}
518518

519+
@ParameterizedTest(name = "{0} : {displayName} ")
520+
@MethodSource("clientsForTesting")
521+
void testCreateElicitationWithApplyDefaultsAndUnmodifiableMap(String clientType) {
522+
523+
var clientBuilder = clientBuilders.get(clientType);
524+
525+
// Client handler returns an unmodifiable map (Map.of()) — SDK should handle this
526+
// gracefully by copying into a new HashMap before applying defaults
527+
Function<McpSchema.ElicitRequest, McpSchema.ElicitResult> elicitationHandler = request -> {
528+
return new McpSchema.ElicitResult(McpSchema.ElicitResult.Action.ACCEPT, Map.of());
529+
};
530+
531+
CallToolResult callResponse = McpSchema.CallToolResult.builder()
532+
.addContent(new McpSchema.TextContent("CALL RESPONSE"))
533+
.build();
534+
535+
AtomicReference<McpSchema.ElicitResult> elicitResultRef = new AtomicReference<>();
536+
537+
McpServerFeatures.AsyncToolSpecification tool = McpServerFeatures.AsyncToolSpecification.builder()
538+
.tool(Tool.builder().name("tool1").description("tool1 description").inputSchema(EMPTY_JSON_SCHEMA).build())
539+
.callHandler((exchange, request) -> {
540+
541+
var elicitationRequest = McpSchema.ElicitRequest.builder()
542+
.message("Provide your preferences")
543+
.requestedSchema(Map.of("type", "object", "properties",
544+
Map.of("nickname", Map.of("type", "string", "default", "Guest"), "age",
545+
Map.of("type", "integer", "default", 18)),
546+
"required", java.util.List.of("nickname", "age")))
547+
.build();
548+
549+
return exchange.createElicitation(elicitationRequest)
550+
.doOnNext(elicitResultRef::set)
551+
.thenReturn(callResponse);
552+
})
553+
.build();
554+
555+
var mcpServer = prepareAsyncServerBuilder().serverInfo("test-server", "1.0.0").tools(tool).build();
556+
557+
try (var mcpClient = clientBuilder.clientInfo(new McpSchema.Implementation("Sample client", "0.0.0"))
558+
.capabilities(ClientCapabilities.builder().elicitation(true, false, true).build())
559+
.elicitation(elicitationHandler)
560+
.build()) {
561+
562+
InitializeResult initResult = mcpClient.initialize();
563+
assertThat(initResult).isNotNull();
564+
565+
CallToolResult response = mcpClient.callTool(new McpSchema.CallToolRequest("tool1", Map.of()));
566+
567+
assertThat(response).isNotNull();
568+
assertWith(elicitResultRef.get(), result -> {
569+
assertThat(result).isNotNull();
570+
assertThat(result.action()).isEqualTo(McpSchema.ElicitResult.Action.ACCEPT);
571+
assertThat(result.content()).containsEntry("nickname", "Guest");
572+
assertThat(result.content()).containsEntry("age", 18);
573+
});
574+
}
575+
finally {
576+
mcpServer.closeGracefully().block();
577+
}
578+
}
579+
519580
@ParameterizedTest(name = "{0} : {displayName} ")
520581
@MethodSource("clientsForTesting")
521582
void testCreateElicitationWithRequestTimeoutSuccess(String clientType) {

‎mcp-test/src/test/java/io/modelcontextprotocol/spec/McpSchemaTests.java‎

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,23 +4,22 @@
44

55
package io.modelcontextprotocol.spec;
66

7-
import static io.modelcontextprotocol.util.McpJsonMapperUtils.JSON_MAPPER;
8-
import static net.javacrumbs.jsonunit.assertj.JsonAssertions.assertThatJson;
9-
import static net.javacrumbs.jsonunit.assertj.JsonAssertions.json;
10-
import static org.assertj.core.api.Assertions.assertThat;
11-
import static org.assertj.core.api.Assertions.assertThatThrownBy;
12-
137
import java.io.IOException;
148
import java.util.Arrays;
159
import java.util.Collections;
1610
import java.util.HashMap;
1711
import java.util.List;
1812
import java.util.Map;
1913

20-
import io.modelcontextprotocol.spec.McpSchema.TextResourceContents;
14+
import static org.assertj.core.api.Assertions.assertThat;
15+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
2116
import org.assertj.core.api.InstanceOfAssertFactories;
2217
import org.junit.jupiter.api.Test;
2318

19+
import io.modelcontextprotocol.spec.McpSchema.TextResourceContents;
20+
import static io.modelcontextprotocol.util.McpJsonMapperUtils.JSON_MAPPER;
21+
import static net.javacrumbs.jsonunit.assertj.JsonAssertions.assertThatJson;
22+
import static net.javacrumbs.jsonunit.assertj.JsonAssertions.json;
2423
import net.javacrumbs.jsonunit.core.Option;
2524

2625
/**
@@ -1728,8 +1727,7 @@ void testElicitationCapabilityWithApplyDefaults() throws Exception {
17281727
assertThat(capabilities.elicitation().form().applyDefaults()).isTrue();
17291728

17301729
String json = JSON_MAPPER.writeValueAsString(capabilities);
1731-
assertThat(json).contains("\"applyDefaults\"");
1732-
assertThat(json).contains("true");
1730+
assertThatJson(json).node("elicitation.form.applyDefaults").isEqualTo(true);
17331731
}
17341732

17351733
@Test

0 commit comments

Comments
 (0)