From fb40047c39b7a36dd770000c9b66dbc50bdff8f4 Mon Sep 17 00:00:00 2001 From: Luca Chang Date: Fri, 16 May 2025 15:49:43 -0700 Subject: [PATCH] refactor: consolidate content annotations into Annotations (#242) Updates all Content types to represent annotations as nested objects instead of directly on the content itself. - Refactor TextContent, ImageContent, and EmbeddedResource to use Annotations object instead of separate audience/priority fields - Add backward-compatible deprecated methods for audience() and priority() accessors - Add parameterized tests for message annotations across different message types (success, error, debug) - Test both text and image content annotations with proper priority and audience validation This brings the implementation in line with the content types as modeled by the specification: https://github.com/modelcontextprotocol/modelcontextprotocol/blob/c87a0da6d8c2436d56a6398023c80b0562224454/schema/2025-03-26/schema.json#L1970-L1973 --- mcp-test/pom.xml | 5 ++ .../client/AbstractMcpAsyncClientTests.java | 61 +++++++++++++ .../client/AbstractMcpSyncClientTests.java | 57 ++++++++++++ .../modelcontextprotocol/spec/McpSchema.java | 90 ++++++++++++++++--- .../client/AbstractMcpAsyncClientTests.java | 61 +++++++++++++ .../client/AbstractMcpSyncClientTests.java | 57 ++++++++++++ 6 files changed, 321 insertions(+), 10 deletions(-) diff --git a/mcp-test/pom.xml b/mcp-test/pom.xml index 9998569..f24d9fa 100644 --- a/mcp-test/pom.xml +++ b/mcp-test/pom.xml @@ -54,6 +54,11 @@ junit-jupiter-api ${junit.version} + + org.junit.jupiter + junit-jupiter-params + ${junit.version} + org.mockito mockito-core 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 02e713a..d08bf70 100644 --- a/mcp-test/src/main/java/io/modelcontextprotocol/client/AbstractMcpAsyncClientTests.java +++ b/mcp-test/src/main/java/io/modelcontextprotocol/client/AbstractMcpAsyncClientTests.java @@ -34,6 +34,8 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import reactor.test.StepVerifier; @@ -41,6 +43,7 @@ import reactor.test.StepVerifier; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.assertj.core.api.Assertions.fail; import static org.junit.jupiter.api.Assertions.assertInstanceOf; /** @@ -208,6 +211,64 @@ public abstract class AbstractMcpAsyncClientTests { }); } + @ParameterizedTest + @ValueSource(strings = { "success", "error", "debug" }) + void testCallToolWithMessageAnnotations(String messageType) { + McpClientTransport transport = createMcpTransport(); + + withClient(transport, mcpAsyncClient -> { + StepVerifier.create(mcpAsyncClient.initialize() + .then(mcpAsyncClient.callTool(new McpSchema.CallToolRequest("annotatedMessage", + Map.of("messageType", messageType, "includeImage", true))))) + .consumeNextWith(result -> { + assertThat(result).isNotNull(); + assertThat(result.isError()).isNotEqualTo(true); + assertThat(result.content()).isNotEmpty(); + assertThat(result.content()).allSatisfy(content -> { + switch (content.type()) { + case "text": + McpSchema.TextContent textContent = assertInstanceOf(McpSchema.TextContent.class, + content); + assertThat(textContent.text()).isNotEmpty(); + assertThat(textContent.annotations()).isNotNull(); + + switch (messageType) { + case "error": + assertThat(textContent.annotations().priority()).isEqualTo(1.0); + assertThat(textContent.annotations().audience()) + .containsOnly(McpSchema.Role.USER, McpSchema.Role.ASSISTANT); + break; + case "success": + assertThat(textContent.annotations().priority()).isEqualTo(0.7); + assertThat(textContent.annotations().audience()) + .containsExactly(McpSchema.Role.USER); + break; + case "debug": + assertThat(textContent.annotations().priority()).isEqualTo(0.3); + assertThat(textContent.annotations().audience()) + .containsExactly(McpSchema.Role.ASSISTANT); + break; + default: + throw new IllegalStateException("Unexpected value: " + content.type()); + } + break; + case "image": + McpSchema.ImageContent imageContent = assertInstanceOf(McpSchema.ImageContent.class, + content); + assertThat(imageContent.data()).isNotEmpty(); + assertThat(imageContent.annotations()).isNotNull(); + assertThat(imageContent.annotations().priority()).isEqualTo(0.5); + assertThat(imageContent.annotations().audience()).containsExactly(McpSchema.Role.USER); + break; + default: + fail("Unexpected content type: " + content.type()); + } + }); + }) + .verifyComplete(); + }); + } + @Test void testListResourcesWithoutInitialization() { verifyCallSucceedsWithImplicitInitialization(client -> client.listResources(null), "listing resources"); diff --git a/mcp-test/src/main/java/io/modelcontextprotocol/client/AbstractMcpSyncClientTests.java b/mcp-test/src/main/java/io/modelcontextprotocol/client/AbstractMcpSyncClientTests.java index 5c7256e..d949296 100644 --- a/mcp-test/src/main/java/io/modelcontextprotocol/client/AbstractMcpSyncClientTests.java +++ b/mcp-test/src/main/java/io/modelcontextprotocol/client/AbstractMcpSyncClientTests.java @@ -31,12 +31,15 @@ import io.modelcontextprotocol.spec.McpSchema.UnsubscribeRequest; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import reactor.core.publisher.Mono; import reactor.test.StepVerifier; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.assertj.core.api.Assertions.fail; import static org.junit.jupiter.api.Assertions.assertInstanceOf; /** @@ -183,6 +186,60 @@ public abstract class AbstractMcpSyncClientTests { }); } + @ParameterizedTest + @ValueSource(strings = { "success", "error", "debug" }) + void testCallToolWithMessageAnnotations(String messageType) { + McpClientTransport transport = createMcpTransport(); + + withClient(transport, client -> { + client.initialize(); + + McpSchema.CallToolResult result = client.callTool(new McpSchema.CallToolRequest("annotatedMessage", + Map.of("messageType", messageType, "includeImage", true))); + + assertThat(result).isNotNull(); + assertThat(result.isError()).isNotEqualTo(true); + assertThat(result.content()).isNotEmpty(); + assertThat(result.content()).allSatisfy(content -> { + switch (content.type()) { + case "text": + McpSchema.TextContent textContent = assertInstanceOf(McpSchema.TextContent.class, content); + assertThat(textContent.text()).isNotEmpty(); + assertThat(textContent.annotations()).isNotNull(); + + switch (messageType) { + case "error": + assertThat(textContent.annotations().priority()).isEqualTo(1.0); + assertThat(textContent.annotations().audience()).containsOnly(McpSchema.Role.USER, + McpSchema.Role.ASSISTANT); + break; + case "success": + assertThat(textContent.annotations().priority()).isEqualTo(0.7); + assertThat(textContent.annotations().audience()).containsExactly(McpSchema.Role.USER); + break; + case "debug": + assertThat(textContent.annotations().priority()).isEqualTo(0.3); + assertThat(textContent.annotations().audience()) + .containsExactly(McpSchema.Role.ASSISTANT); + break; + default: + throw new IllegalStateException("Unexpected value: " + content.type()); + } + break; + case "image": + McpSchema.ImageContent imageContent = assertInstanceOf(McpSchema.ImageContent.class, content); + assertThat(imageContent.data()).isNotEmpty(); + assertThat(imageContent.annotations()).isNotNull(); + assertThat(imageContent.annotations().priority()).isEqualTo(0.5); + assertThat(imageContent.annotations().audience()).containsExactly(McpSchema.Role.USER); + break; + default: + fail("Unexpected content type: " + content.type()); + } + }); + }); + } + @Test void testPingWithoutInitialization() { verifyCallSucceedsWithImplicitInitialization(client -> client.ping(), "pinging the server"); diff --git a/mcp/src/main/java/io/modelcontextprotocol/spec/McpSchema.java b/mcp/src/main/java/io/modelcontextprotocol/spec/McpSchema.java index 5792aad..a78600a 100644 --- a/mcp/src/main/java/io/modelcontextprotocol/spec/McpSchema.java +++ b/mcp/src/main/java/io/modelcontextprotocol/spec/McpSchema.java @@ -1384,22 +1384,68 @@ public final class McpSchema { @JsonInclude(JsonInclude.Include.NON_ABSENT) @JsonIgnoreProperties(ignoreUnknown = true) public record TextContent( // @formatter:off - @JsonProperty("audience") List audience, - @JsonProperty("priority") Double priority, - @JsonProperty("text") String text) implements Content { // @formatter:on + @JsonProperty("annotations") Annotations annotations, + @JsonProperty("text") String text) implements Annotated, Content { // @formatter:on public TextContent(String content) { - this(null, null, content); + this(null, content); + } + + /** + * @deprecated Only exists for backwards-compatibility purposes. Use + * {@link TextContent#TextContent(Annotations, String)} instead. + */ + public TextContent(List audience, Double priority, String content) { + this(audience != null || priority != null ? new Annotations(audience, priority) : null, content); + } + + /** + * @deprecated Only exists for backwards-compatibility purposes. Use + * {@link TextContent#annotations()} instead. + */ + public List audience() { + return annotations == null ? null : annotations.audience(); + } + + /** + * @deprecated Only exists for backwards-compatibility purposes. Use + * {@link TextContent#annotations()} instead. + */ + public Double priority() { + return annotations == null ? null : annotations.priority(); } } @JsonInclude(JsonInclude.Include.NON_ABSENT) @JsonIgnoreProperties(ignoreUnknown = true) public record ImageContent( // @formatter:off - @JsonProperty("audience") List audience, - @JsonProperty("priority") Double priority, + @JsonProperty("annotations") Annotations annotations, @JsonProperty("data") String data, - @JsonProperty("mimeType") String mimeType) implements Content { // @formatter:on + @JsonProperty("mimeType") String mimeType) implements Annotated, Content { // @formatter:on + + /** + * @deprecated Only exists for backwards-compatibility purposes. Use + * {@link ImageContent#ImageContent(Annotations, String, String)} instead. + */ + public ImageContent(List audience, Double priority, String data, String mimeType) { + this(audience != null || priority != null ? new Annotations(audience, priority) : null, data, mimeType); + } + + /** + * @deprecated Only exists for backwards-compatibility purposes. Use + * {@link ImageContent#annotations()} instead. + */ + public List audience() { + return annotations == null ? null : annotations.audience(); + } + + /** + * @deprecated Only exists for backwards-compatibility purposes. Use + * {@link ImageContent#annotations()} instead. + */ + public Double priority() { + return annotations == null ? null : annotations.priority(); + } } @JsonInclude(JsonInclude.Include.NON_ABSENT) @@ -1413,9 +1459,33 @@ public final class McpSchema { @JsonInclude(JsonInclude.Include.NON_ABSENT) @JsonIgnoreProperties(ignoreUnknown = true) public record EmbeddedResource( // @formatter:off - @JsonProperty("audience") List audience, - @JsonProperty("priority") Double priority, - @JsonProperty("resource") ResourceContents resource) implements Content { // @formatter:on + @JsonProperty("annotations") Annotations annotations, + @JsonProperty("resource") ResourceContents resource) implements Annotated, Content { // @formatter:on + + /** + * @deprecated Only exists for backwards-compatibility purposes. Use + * {@link EmbeddedResource#EmbeddedResource(Annotations, ResourceContents)} + * instead. + */ + public EmbeddedResource(List audience, Double priority, ResourceContents resource) { + this(audience != null || priority != null ? new Annotations(audience, priority) : null, resource); + } + + /** + * @deprecated Only exists for backwards-compatibility purposes. Use + * {@link EmbeddedResource#annotations()} instead. + */ + public List audience() { + return annotations == null ? null : annotations.audience(); + } + + /** + * @deprecated Only exists for backwards-compatibility purposes. Use + * {@link EmbeddedResource#annotations()} instead. + */ + public Double priority() { + return annotations == null ? null : annotations.priority(); + } } // --------------------------- diff --git a/mcp/src/test/java/io/modelcontextprotocol/client/AbstractMcpAsyncClientTests.java b/mcp/src/test/java/io/modelcontextprotocol/client/AbstractMcpAsyncClientTests.java index e146656..af130be 100644 --- a/mcp/src/test/java/io/modelcontextprotocol/client/AbstractMcpAsyncClientTests.java +++ b/mcp/src/test/java/io/modelcontextprotocol/client/AbstractMcpAsyncClientTests.java @@ -34,6 +34,8 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import reactor.test.StepVerifier; @@ -41,6 +43,7 @@ import reactor.test.StepVerifier; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.assertj.core.api.Assertions.fail; import static org.junit.jupiter.api.Assertions.assertInstanceOf; /** @@ -209,6 +212,64 @@ public abstract class AbstractMcpAsyncClientTests { }); } + @ParameterizedTest + @ValueSource(strings = { "success", "error", "debug" }) + void testCallToolWithMessageAnnotations(String messageType) { + McpClientTransport transport = createMcpTransport(); + + withClient(transport, mcpAsyncClient -> { + StepVerifier.create(mcpAsyncClient.initialize() + .then(mcpAsyncClient.callTool(new McpSchema.CallToolRequest("annotatedMessage", + Map.of("messageType", messageType, "includeImage", true))))) + .consumeNextWith(result -> { + assertThat(result).isNotNull(); + assertThat(result.isError()).isNotEqualTo(true); + assertThat(result.content()).isNotEmpty(); + assertThat(result.content()).allSatisfy(content -> { + switch (content.type()) { + case "text": + McpSchema.TextContent textContent = assertInstanceOf(McpSchema.TextContent.class, + content); + assertThat(textContent.text()).isNotEmpty(); + assertThat(textContent.annotations()).isNotNull(); + + switch (messageType) { + case "error": + assertThat(textContent.annotations().priority()).isEqualTo(1.0); + assertThat(textContent.annotations().audience()) + .containsOnly(McpSchema.Role.USER, McpSchema.Role.ASSISTANT); + break; + case "success": + assertThat(textContent.annotations().priority()).isEqualTo(0.7); + assertThat(textContent.annotations().audience()) + .containsExactly(McpSchema.Role.USER); + break; + case "debug": + assertThat(textContent.annotations().priority()).isEqualTo(0.3); + assertThat(textContent.annotations().audience()) + .containsExactly(McpSchema.Role.ASSISTANT); + break; + default: + throw new IllegalStateException("Unexpected value: " + content.type()); + } + break; + case "image": + McpSchema.ImageContent imageContent = assertInstanceOf(McpSchema.ImageContent.class, + content); + assertThat(imageContent.data()).isNotEmpty(); + assertThat(imageContent.annotations()).isNotNull(); + assertThat(imageContent.annotations().priority()).isEqualTo(0.5); + assertThat(imageContent.annotations().audience()).containsExactly(McpSchema.Role.USER); + break; + default: + fail("Unexpected content type: " + content.type()); + } + }); + }) + .verifyComplete(); + }); + } + @Test void testListResourcesWithoutInitialization() { verifyCallSucceedsWithImplicitInitialization(client -> client.listResources(null), "listing resources"); diff --git a/mcp/src/test/java/io/modelcontextprotocol/client/AbstractMcpSyncClientTests.java b/mcp/src/test/java/io/modelcontextprotocol/client/AbstractMcpSyncClientTests.java index c626628..10ecade 100644 --- a/mcp/src/test/java/io/modelcontextprotocol/client/AbstractMcpSyncClientTests.java +++ b/mcp/src/test/java/io/modelcontextprotocol/client/AbstractMcpSyncClientTests.java @@ -31,6 +31,8 @@ import io.modelcontextprotocol.spec.McpSchema.UnsubscribeRequest; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import reactor.core.publisher.Mono; import reactor.core.scheduler.Scheduler; import reactor.core.scheduler.Schedulers; @@ -39,6 +41,7 @@ import reactor.test.StepVerifier; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.assertj.core.api.Assertions.fail; import static org.junit.jupiter.api.Assertions.assertInstanceOf; /** @@ -229,6 +232,60 @@ public abstract class AbstractMcpSyncClientTests { }); } + @ParameterizedTest + @ValueSource(strings = { "success", "error", "debug" }) + void testCallToolWithMessageAnnotations(String messageType) { + McpClientTransport transport = createMcpTransport(); + + withClient(transport, client -> { + client.initialize(); + + McpSchema.CallToolResult result = client.callTool(new McpSchema.CallToolRequest("annotatedMessage", + Map.of("messageType", messageType, "includeImage", true))); + + assertThat(result).isNotNull(); + assertThat(result.isError()).isNotEqualTo(true); + assertThat(result.content()).isNotEmpty(); + assertThat(result.content()).allSatisfy(content -> { + switch (content.type()) { + case "text": + McpSchema.TextContent textContent = assertInstanceOf(McpSchema.TextContent.class, content); + assertThat(textContent.text()).isNotEmpty(); + assertThat(textContent.annotations()).isNotNull(); + + switch (messageType) { + case "error": + assertThat(textContent.annotations().priority()).isEqualTo(1.0); + assertThat(textContent.annotations().audience()).containsOnly(McpSchema.Role.USER, + McpSchema.Role.ASSISTANT); + break; + case "success": + assertThat(textContent.annotations().priority()).isEqualTo(0.7); + assertThat(textContent.annotations().audience()).containsExactly(McpSchema.Role.USER); + break; + case "debug": + assertThat(textContent.annotations().priority()).isEqualTo(0.3); + assertThat(textContent.annotations().audience()) + .containsExactly(McpSchema.Role.ASSISTANT); + break; + default: + throw new IllegalStateException("Unexpected value: " + content.type()); + } + break; + case "image": + McpSchema.ImageContent imageContent = assertInstanceOf(McpSchema.ImageContent.class, content); + assertThat(imageContent.data()).isNotEmpty(); + assertThat(imageContent.annotations()).isNotNull(); + assertThat(imageContent.annotations().priority()).isEqualTo(0.5); + assertThat(imageContent.annotations().audience()).containsExactly(McpSchema.Role.USER); + break; + default: + fail("Unexpected content type: " + content.type()); + } + }); + }); + } + @Test void testRootsListChangedWithoutInitialization() { verifyNotificationSucceedsWithImplicitInitialization(client -> client.rootsListChangedNotification(),