From d9006ecb78a40b1c551bad08cacaa483eb83fcca Mon Sep 17 00:00:00 2001 From: Christian Tzolov <1351573+tzolov@users.noreply.github.com> Date: Fri, 27 Jun 2025 21:03:46 +0200 Subject: [PATCH] refactor: return immutable lists from list methods (#345) - Wrap the list responses in immutable lists. - Add test to verify the immutable responses. - Better timeout handling Signed-off-by: Christian Tzolov --- .../WebFluxSseIntegrationTests.java | 14 ++--- .../client/AbstractMcpAsyncClientTests.java | 58 +++++++++++++++++ .../client/McpAsyncClient.java | 62 +++++++++---------- .../client/AbstractMcpAsyncClientTests.java | 58 +++++++++++++++++ 4 files changed, 150 insertions(+), 42 deletions(-) diff --git a/mcp-spring/mcp-spring-webflux/src/test/java/io/modelcontextprotocol/WebFluxSseIntegrationTests.java b/mcp-spring/mcp-spring-webflux/src/test/java/io/modelcontextprotocol/WebFluxSseIntegrationTests.java index 2f85654..cac0ffa 100644 --- a/mcp-spring/mcp-spring-webflux/src/test/java/io/modelcontextprotocol/WebFluxSseIntegrationTests.java +++ b/mcp-spring/mcp-spring-webflux/src/test/java/io/modelcontextprotocol/WebFluxSseIntegrationTests.java @@ -432,12 +432,6 @@ class WebFluxSseIntegrationTests { Function elicitationHandler = request -> { assertThat(request.message()).isNotEmpty(); assertThat(request.requestedSchema()).isNotNull(); - try { - TimeUnit.SECONDS.sleep(2); - } - catch (InterruptedException e) { - throw new RuntimeException(e); - } return new ElicitResult(ElicitResult.Action.ACCEPT, Map.of("message", request.message())); }; @@ -491,14 +485,18 @@ class WebFluxSseIntegrationTests { @ValueSource(strings = { "httpclient", "webflux" }) void testCreateElicitationWithRequestTimeoutFail(String clientType) { + var latch = new CountDownLatch(1); // Client var clientBuilder = clientBuilders.get(clientType); Function elicitationHandler = request -> { assertThat(request.message()).isNotEmpty(); assertThat(request.requestedSchema()).isNotNull(); + try { - TimeUnit.SECONDS.sleep(2); + if (!latch.await(2, TimeUnit.SECONDS)) { + throw new RuntimeException("Timeout waiting for elicitation processing"); + } } catch (InterruptedException e) { throw new RuntimeException(e); @@ -536,7 +534,7 @@ class WebFluxSseIntegrationTests { var mcpServer = McpServer.async(mcpServerTransportProvider) .serverInfo("test-server", "1.0.0") - .requestTimeout(Duration.ofSeconds(1)) + .requestTimeout(Duration.ofSeconds(1)) // 1 second. .tools(tool) .build(); diff --git a/mcp-test/src/main/java/io/modelcontextprotocol/client/AbstractMcpAsyncClientTests.java b/mcp-test/src/main/java/io/modelcontextprotocol/client/AbstractMcpAsyncClientTests.java index 4f7d567..9cccd39 100644 --- a/mcp-test/src/main/java/io/modelcontextprotocol/client/AbstractMcpAsyncClientTests.java +++ b/mcp-test/src/main/java/io/modelcontextprotocol/client/AbstractMcpAsyncClientTests.java @@ -182,6 +182,20 @@ public abstract class AbstractMcpAsyncClientTests { }); } + @Test + void testListAllToolsReturnsImmutableList() { + withClient(createMcpTransport(), mcpAsyncClient -> { + StepVerifier.create(mcpAsyncClient.initialize().then(mcpAsyncClient.listTools())) + .consumeNextWith(result -> { + assertThat(result.tools()).isNotNull(); + // Verify that the returned list is immutable + assertThatThrownBy(() -> result.tools().add(new Tool("test", "test", "{\"type\":\"object\"}"))) + .isInstanceOf(UnsupportedOperationException.class); + }) + .verifyComplete(); + }); + } + @Test void testPingWithoutInitialization() { verifyCallSucceedsWithImplicitInitialization(client -> client.ping(), "pinging the server"); @@ -333,6 +347,21 @@ public abstract class AbstractMcpAsyncClientTests { }); } + @Test + void testListAllResourcesReturnsImmutableList() { + withClient(createMcpTransport(), mcpAsyncClient -> { + StepVerifier.create(mcpAsyncClient.initialize().then(mcpAsyncClient.listResources())) + .consumeNextWith(result -> { + assertThat(result.resources()).isNotNull(); + // Verify that the returned list is immutable + assertThatThrownBy( + () -> result.resources().add(Resource.builder().uri("test://uri").name("test").build())) + .isInstanceOf(UnsupportedOperationException.class); + }) + .verifyComplete(); + }); + } + @Test void testMcpAsyncClientState() { withClient(createMcpTransport(), mcpAsyncClient -> { @@ -384,6 +413,20 @@ public abstract class AbstractMcpAsyncClientTests { }); } + @Test + void testListAllPromptsReturnsImmutableList() { + withClient(createMcpTransport(), mcpAsyncClient -> { + StepVerifier.create(mcpAsyncClient.initialize().then(mcpAsyncClient.listPrompts())) + .consumeNextWith(result -> { + assertThat(result.prompts()).isNotNull(); + // Verify that the returned list is immutable + assertThatThrownBy(() -> result.prompts().add(new Prompt("test", "test", null))) + .isInstanceOf(UnsupportedOperationException.class); + }) + .verifyComplete(); + }); + } + @Test void testGetPromptWithoutInitialization() { GetPromptRequest request = new GetPromptRequest("simple_prompt", Map.of()); @@ -553,6 +596,21 @@ public abstract class AbstractMcpAsyncClientTests { }); } + @Test + void testListAllResourceTemplatesReturnsImmutableList() { + withClient(createMcpTransport(), mcpAsyncClient -> { + StepVerifier.create(mcpAsyncClient.initialize().then(mcpAsyncClient.listResourceTemplates())) + .consumeNextWith(result -> { + assertThat(result.resourceTemplates()).isNotNull(); + // Verify that the returned list is immutable + assertThatThrownBy(() -> result.resourceTemplates() + .add(new McpSchema.ResourceTemplate("test://template", "test", null, null, null))) + .isInstanceOf(UnsupportedOperationException.class); + }) + .verifyComplete(); + }); + } + // @Test void testResourceSubscription() { withClient(createMcpTransport(), mcpAsyncClient -> { diff --git a/mcp/src/main/java/io/modelcontextprotocol/client/McpAsyncClient.java b/mcp/src/main/java/io/modelcontextprotocol/client/McpAsyncClient.java index d0fd5c0..fa76f39 100644 --- a/mcp/src/main/java/io/modelcontextprotocol/client/McpAsyncClient.java +++ b/mcp/src/main/java/io/modelcontextprotocol/client/McpAsyncClient.java @@ -5,6 +5,7 @@ package io.modelcontextprotocol.client; import java.time.Duration; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -677,15 +678,13 @@ public class McpAsyncClient { * @return A Mono that emits the list of all tools result */ public Mono listTools() { - return this.listTools(McpSchema.FIRST_PAGE).expand(result -> { - if (result.nextCursor() != null) { - return this.listTools(result.nextCursor()); - } - return Mono.empty(); - }).reduce(new McpSchema.ListToolsResult(new ArrayList<>(), null), (allToolsResult, result) -> { - allToolsResult.tools().addAll(result.tools()); - return allToolsResult; - }); + return this.listTools(McpSchema.FIRST_PAGE) + .expand(result -> (result.nextCursor() != null) ? this.listTools(result.nextCursor()) : Mono.empty()) + .reduce(new McpSchema.ListToolsResult(new ArrayList<>(), null), (allToolsResult, result) -> { + allToolsResult.tools().addAll(result.tools()); + return allToolsResult; + }) + .map(result -> new McpSchema.ListToolsResult(Collections.unmodifiableList(result.tools()), null)); } /** @@ -739,15 +738,13 @@ public class McpAsyncClient { * @see #readResource(McpSchema.Resource) */ public Mono listResources() { - return this.listResources(McpSchema.FIRST_PAGE).expand(result -> { - if (result.nextCursor() != null) { - return this.listResources(result.nextCursor()); - } - return Mono.empty(); - }).reduce(new McpSchema.ListResourcesResult(new ArrayList<>(), null), (allResourcesResult, result) -> { - allResourcesResult.resources().addAll(result.resources()); - return allResourcesResult; - }); + return this.listResources(McpSchema.FIRST_PAGE) + .expand(result -> (result.nextCursor() != null) ? this.listResources(result.nextCursor()) : Mono.empty()) + .reduce(new McpSchema.ListResourcesResult(new ArrayList<>(), null), (allResourcesResult, result) -> { + allResourcesResult.resources().addAll(result.resources()); + return allResourcesResult; + }) + .map(result -> new McpSchema.ListResourcesResult(Collections.unmodifiableList(result.resources()), null)); } /** @@ -809,17 +806,16 @@ public class McpAsyncClient { * @see McpSchema.ListResourceTemplatesResult */ public Mono listResourceTemplates() { - return this.listResourceTemplates(McpSchema.FIRST_PAGE).expand(result -> { - if (result.nextCursor() != null) { - return this.listResourceTemplates(result.nextCursor()); - } - return Mono.empty(); - }) + return this.listResourceTemplates(McpSchema.FIRST_PAGE) + .expand(result -> (result.nextCursor() != null) ? this.listResourceTemplates(result.nextCursor()) + : Mono.empty()) .reduce(new McpSchema.ListResourceTemplatesResult(new ArrayList<>(), null), (allResourceTemplatesResult, result) -> { allResourceTemplatesResult.resourceTemplates().addAll(result.resourceTemplates()); return allResourceTemplatesResult; - }); + }) + .map(result -> new McpSchema.ListResourceTemplatesResult( + Collections.unmodifiableList(result.resourceTemplates()), null)); } /** @@ -914,15 +910,13 @@ public class McpAsyncClient { * @see #getPrompt(GetPromptRequest) */ public Mono listPrompts() { - return this.listPrompts(McpSchema.FIRST_PAGE).expand(result -> { - if (result.nextCursor() != null) { - return this.listPrompts(result.nextCursor()); - } - return Mono.empty(); - }).reduce(new ListPromptsResult(new ArrayList<>(), null), (allPromptsResult, result) -> { - allPromptsResult.prompts().addAll(result.prompts()); - return allPromptsResult; - }); + return this.listPrompts(McpSchema.FIRST_PAGE) + .expand(result -> (result.nextCursor() != null) ? this.listPrompts(result.nextCursor()) : Mono.empty()) + .reduce(new ListPromptsResult(new ArrayList<>(), null), (allPromptsResult, result) -> { + allPromptsResult.prompts().addAll(result.prompts()); + return allPromptsResult; + }) + .map(result -> new McpSchema.ListPromptsResult(Collections.unmodifiableList(result.prompts()), null)); } /** diff --git a/mcp/src/test/java/io/modelcontextprotocol/client/AbstractMcpAsyncClientTests.java b/mcp/src/test/java/io/modelcontextprotocol/client/AbstractMcpAsyncClientTests.java index 9be6e55..a3edda2 100644 --- a/mcp/src/test/java/io/modelcontextprotocol/client/AbstractMcpAsyncClientTests.java +++ b/mcp/src/test/java/io/modelcontextprotocol/client/AbstractMcpAsyncClientTests.java @@ -183,6 +183,20 @@ public abstract class AbstractMcpAsyncClientTests { }); } + @Test + void testListAllToolsReturnsImmutableList() { + withClient(createMcpTransport(), mcpAsyncClient -> { + StepVerifier.create(mcpAsyncClient.initialize().then(mcpAsyncClient.listTools())) + .consumeNextWith(result -> { + assertThat(result.tools()).isNotNull(); + // Verify that the returned list is immutable + assertThatThrownBy(() -> result.tools().add(new Tool("test", "test", "{\"type\":\"object\"}"))) + .isInstanceOf(UnsupportedOperationException.class); + }) + .verifyComplete(); + }); + } + @Test void testPingWithoutInitialization() { verifyCallSucceedsWithImplicitInitialization(client -> client.ping(), "pinging the server"); @@ -334,6 +348,21 @@ public abstract class AbstractMcpAsyncClientTests { }); } + @Test + void testListAllResourcesReturnsImmutableList() { + withClient(createMcpTransport(), mcpAsyncClient -> { + StepVerifier.create(mcpAsyncClient.initialize().then(mcpAsyncClient.listResources())) + .consumeNextWith(result -> { + assertThat(result.resources()).isNotNull(); + // Verify that the returned list is immutable + assertThatThrownBy( + () -> result.resources().add(Resource.builder().uri("test://uri").name("test").build())) + .isInstanceOf(UnsupportedOperationException.class); + }) + .verifyComplete(); + }); + } + @Test void testMcpAsyncClientState() { withClient(createMcpTransport(), mcpAsyncClient -> { @@ -385,6 +414,20 @@ public abstract class AbstractMcpAsyncClientTests { }); } + @Test + void testListAllPromptsReturnsImmutableList() { + withClient(createMcpTransport(), mcpAsyncClient -> { + StepVerifier.create(mcpAsyncClient.initialize().then(mcpAsyncClient.listPrompts())) + .consumeNextWith(result -> { + assertThat(result.prompts()).isNotNull(); + // Verify that the returned list is immutable + assertThatThrownBy(() -> result.prompts().add(new Prompt("test", "test", null))) + .isInstanceOf(UnsupportedOperationException.class); + }) + .verifyComplete(); + }); + } + @Test void testGetPromptWithoutInitialization() { GetPromptRequest request = new GetPromptRequest("simple_prompt", Map.of()); @@ -554,6 +597,21 @@ public abstract class AbstractMcpAsyncClientTests { }); } + @Test + void testListAllResourceTemplatesReturnsImmutableList() { + withClient(createMcpTransport(), mcpAsyncClient -> { + StepVerifier.create(mcpAsyncClient.initialize().then(mcpAsyncClient.listResourceTemplates())) + .consumeNextWith(result -> { + assertThat(result.resourceTemplates()).isNotNull(); + // Verify that the returned list is immutable + assertThatThrownBy(() -> result.resourceTemplates() + .add(new McpSchema.ResourceTemplate("test://template", "test", null, null, null))) + .isInstanceOf(UnsupportedOperationException.class); + }) + .verifyComplete(); + }); + } + // @Test void testResourceSubscription() { withClient(createMcpTransport(), mcpAsyncClient -> {