From be5ec12e1b5dfb766621fda2f9e141d32b9cf7d0 Mon Sep 17 00:00:00 2001 From: Daniel Garnier-Moiroux Date: Tue, 10 Feb 2026 11:29:08 +0100 Subject: [PATCH] Polish gh-764 Signed-off-by: Daniel Garnier-Moiroux --- .../modelcontextprotocol/spec/McpSchema.java | 1 + .../AsyncToolSpecificationBuilderTest.java | 41 ++++++---- .../SyncToolSpecificationBuilderTest.java | 39 ++++++---- .../spec/McpSchemaTests.java | 10 --- .../util/ToolNameValidatorTests.java | 75 ++++++++++++++++--- 5 files changed, 117 insertions(+), 49 deletions(-) diff --git a/mcp-core/src/main/java/io/modelcontextprotocol/spec/McpSchema.java b/mcp-core/src/main/java/io/modelcontextprotocol/spec/McpSchema.java index 50e4310..97bde0b 100644 --- a/mcp-core/src/main/java/io/modelcontextprotocol/spec/McpSchema.java +++ b/mcp-core/src/main/java/io/modelcontextprotocol/spec/McpSchema.java @@ -1466,6 +1466,7 @@ public final class McpSchema { } public Tool build() { + Assert.hasText(name, "name must not be empty"); return new Tool(name, title, description, inputSchema, outputSchema, annotations, meta); } diff --git a/mcp-core/src/test/java/io/modelcontextprotocol/server/AsyncToolSpecificationBuilderTest.java b/mcp-core/src/test/java/io/modelcontextprotocol/server/AsyncToolSpecificationBuilderTest.java index b0a5b2b..c16b06a 100644 --- a/mcp-core/src/test/java/io/modelcontextprotocol/server/AsyncToolSpecificationBuilderTest.java +++ b/mcp-core/src/test/java/io/modelcontextprotocol/server/AsyncToolSpecificationBuilderTest.java @@ -4,30 +4,33 @@ package io.modelcontextprotocol.server; -import static io.modelcontextprotocol.util.ToolsUtils.EMPTY_JSON_SCHEMA; -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.mockito.Mockito.mock; - import java.util.List; import java.util.Map; +import ch.qos.logback.classic.Logger; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.read.ListAppender; import io.modelcontextprotocol.spec.McpSchema; +import io.modelcontextprotocol.spec.McpSchema.CallToolRequest; +import io.modelcontextprotocol.spec.McpSchema.CallToolResult; +import io.modelcontextprotocol.spec.McpSchema.TextContent; +import io.modelcontextprotocol.spec.McpSchema.Tool; import io.modelcontextprotocol.spec.McpServerTransportProvider; import io.modelcontextprotocol.util.ToolNameValidator; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; - -import io.modelcontextprotocol.spec.McpSchema.CallToolRequest; -import io.modelcontextprotocol.spec.McpSchema.CallToolResult; -import io.modelcontextprotocol.spec.McpSchema.TextContent; -import io.modelcontextprotocol.spec.McpSchema.Tool; +import org.slf4j.LoggerFactory; import reactor.core.publisher.Mono; import reactor.test.StepVerifier; +import static io.modelcontextprotocol.util.ToolsUtils.EMPTY_JSON_SCHEMA; +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.mockito.Mockito.mock; + /** * Tests for {@link McpServerFeatures.AsyncToolSpecification.Builder}. * @@ -276,15 +279,23 @@ class AsyncToolSpecificationBuilderTest { private McpServerTransportProvider transportProvider; + private final Logger logger = (Logger) LoggerFactory.getLogger(ToolNameValidator.class); + + private final ListAppender logAppender = new ListAppender<>(); + @BeforeEach void setUp() { transportProvider = mock(McpServerTransportProvider.class); System.clearProperty(ToolNameValidator.STRICT_VALIDATION_PROPERTY); + logAppender.start(); + logger.addAppender(logAppender); } @AfterEach void tearDown() { System.clearProperty(ToolNameValidator.STRICT_VALIDATION_PROPERTY); + logger.detachAppender(logAppender); + logAppender.stop(); } @Test @@ -297,25 +308,27 @@ class AsyncToolSpecificationBuilderTest { } @Test - void systemPropertyFalseShouldWarnOnly() { + void lenientDefaultShouldLogOnInvalidName() { System.setProperty(ToolNameValidator.STRICT_VALIDATION_PROPERTY, "false"); Tool invalidTool = Tool.builder().name("invalid tool name").build(); assertThatCode(() -> McpServer.async(transportProvider).tool(invalidTool, (exchange, args) -> null)) .doesNotThrowAnyException(); + assertThat(logAppender.list).hasSize(1); } @Test - void perServerFalseShouldWarnOnly() { + void lenientConfigurationShouldLogOnInvalidName() { Tool invalidTool = Tool.builder().name("invalid tool name").build(); assertThatCode(() -> McpServer.async(transportProvider) .strictToolNameValidation(false) .tool(invalidTool, (exchange, args) -> null)).doesNotThrowAnyException(); + assertThat(logAppender.list).hasSize(1); } @Test - void perServerTrueShouldOverrideSystemProperty() { + void serverConfigurationShouldOverrideDefault() { System.setProperty(ToolNameValidator.STRICT_VALIDATION_PROPERTY, "false"); Tool invalidTool = Tool.builder().name("invalid tool name").build(); diff --git a/mcp-core/src/test/java/io/modelcontextprotocol/server/SyncToolSpecificationBuilderTest.java b/mcp-core/src/test/java/io/modelcontextprotocol/server/SyncToolSpecificationBuilderTest.java index e8075eb..a2030e4 100644 --- a/mcp-core/src/test/java/io/modelcontextprotocol/server/SyncToolSpecificationBuilderTest.java +++ b/mcp-core/src/test/java/io/modelcontextprotocol/server/SyncToolSpecificationBuilderTest.java @@ -4,26 +4,29 @@ package io.modelcontextprotocol.server; -import static io.modelcontextprotocol.util.ToolsUtils.EMPTY_JSON_SCHEMA; -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.mockito.Mockito.mock; - import java.util.List; import java.util.Map; +import ch.qos.logback.classic.Logger; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.read.ListAppender; +import io.modelcontextprotocol.spec.McpSchema.CallToolRequest; +import io.modelcontextprotocol.spec.McpSchema.CallToolResult; +import io.modelcontextprotocol.spec.McpSchema.TextContent; +import io.modelcontextprotocol.spec.McpSchema.Tool; import io.modelcontextprotocol.spec.McpServerTransportProvider; import io.modelcontextprotocol.util.ToolNameValidator; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import org.slf4j.LoggerFactory; -import io.modelcontextprotocol.spec.McpSchema.CallToolRequest; -import io.modelcontextprotocol.spec.McpSchema.CallToolResult; -import io.modelcontextprotocol.spec.McpSchema.TextContent; -import io.modelcontextprotocol.spec.McpSchema.Tool; +import static io.modelcontextprotocol.util.ToolsUtils.EMPTY_JSON_SCHEMA; +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.mockito.Mockito.mock; /** * Tests for {@link McpServerFeatures.SyncToolSpecification.Builder}. @@ -114,15 +117,23 @@ class SyncToolSpecificationBuilderTest { private McpServerTransportProvider transportProvider; + private final Logger logger = (Logger) LoggerFactory.getLogger(ToolNameValidator.class); + + private final ListAppender logAppender = new ListAppender<>(); + @BeforeEach void setUp() { transportProvider = mock(McpServerTransportProvider.class); System.clearProperty(ToolNameValidator.STRICT_VALIDATION_PROPERTY); + logAppender.start(); + logger.addAppender(logAppender); } @AfterEach void tearDown() { System.clearProperty(ToolNameValidator.STRICT_VALIDATION_PROPERTY); + logger.detachAppender(logAppender); + logAppender.stop(); } @Test @@ -135,25 +146,27 @@ class SyncToolSpecificationBuilderTest { } @Test - void systemPropertyFalseShouldWarnOnly() { + void lenientDefaultShouldLogOnInvalidName() { System.setProperty(ToolNameValidator.STRICT_VALIDATION_PROPERTY, "false"); Tool invalidTool = Tool.builder().name("invalid tool name").build(); assertThatCode(() -> McpServer.sync(transportProvider).tool(invalidTool, (exchange, args) -> null)) .doesNotThrowAnyException(); + assertThat(logAppender.list).hasSize(1); } @Test - void perServerFalseShouldWarnOnly() { + void lenientConfigurationShouldLogOnInvalidName() { Tool invalidTool = Tool.builder().name("invalid tool name").build(); assertThatCode(() -> McpServer.sync(transportProvider) .strictToolNameValidation(false) .tool(invalidTool, (exchange, args) -> null)).doesNotThrowAnyException(); + assertThat(logAppender.list).hasSize(1); } @Test - void perServerTrueShouldOverrideSystemProperty() { + void serverConfigurationShouldOverrideDefault() { System.setProperty(ToolNameValidator.STRICT_VALIDATION_PROPERTY, "false"); Tool invalidTool = Tool.builder().name("invalid tool name").build(); diff --git a/mcp-core/src/test/java/io/modelcontextprotocol/spec/McpSchemaTests.java b/mcp-core/src/test/java/io/modelcontextprotocol/spec/McpSchemaTests.java index 45c1c9a..82ffe9e 100644 --- a/mcp-core/src/test/java/io/modelcontextprotocol/spec/McpSchemaTests.java +++ b/mcp-core/src/test/java/io/modelcontextprotocol/spec/McpSchemaTests.java @@ -1765,14 +1765,4 @@ public class McpSchemaTests { {"progressToken":"progress-token-789","progress":0.25}""")); } - // Tool Name Validation Tests - - @Test - void testToolBuilderWithValidName() { - McpSchema.Tool tool = McpSchema.Tool.builder().name("valid_tool-name.v1").description("A test tool").build(); - - assertThat(tool.name()).isEqualTo("valid_tool-name.v1"); - assertThat(tool.description()).isEqualTo("A test tool"); - } - } diff --git a/mcp-core/src/test/java/io/modelcontextprotocol/util/ToolNameValidatorTests.java b/mcp-core/src/test/java/io/modelcontextprotocol/util/ToolNameValidatorTests.java index f6eaf54..f8e301f 100644 --- a/mcp-core/src/test/java/io/modelcontextprotocol/util/ToolNameValidatorTests.java +++ b/mcp-core/src/test/java/io/modelcontextprotocol/util/ToolNameValidatorTests.java @@ -4,10 +4,21 @@ package io.modelcontextprotocol.util; +import java.util.List; +import java.util.function.Consumer; + +import ch.qos.logback.classic.Level; +import ch.qos.logback.classic.Logger; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.read.ListAppender; +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 org.slf4j.LoggerFactory; +import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -16,33 +27,49 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; */ class ToolNameValidatorTests { + private final Logger logger = (Logger) LoggerFactory.getLogger(ToolNameValidator.class); + + private final ListAppender logAppender = new ListAppender<>(); + + @BeforeEach + void setUp() { + logAppender.start(); + logger.addAppender(logAppender); + } + + @AfterEach + void tearDown() { + logger.detachAppender(logAppender); + logAppender.stop(); + } + @ParameterizedTest @ValueSource(strings = { "getUser", "DATA_EXPORT_v2", "admin.tools.list", "my-tool", "Tool123", "a", "A", "_private", "tool_name", "tool-name", "tool.name", "UPPERCASE", "lowercase", "MixedCase123" }) - void validToolNames_strictMode(String name) { + void validToolNames(String name) { assertThatCode(() -> ToolNameValidator.validate(name, true)).doesNotThrowAnyException(); + ToolNameValidator.validate(name, false); + assertThat(logAppender.list).isEmpty(); } @Test - void validToolName_maxLength() { + void validToolNameMaxLength() { String name = "a".repeat(128); assertThatCode(() -> ToolNameValidator.validate(name, true)).doesNotThrowAnyException(); + ToolNameValidator.validate(name, false); + assertThat(logAppender.list).isEmpty(); } @Test - void invalidToolName_null_strictMode() { + void nullOrEmpty() { assertThatThrownBy(() -> ToolNameValidator.validate(null, true)).isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("null or empty"); - } - - @Test - void invalidToolName_empty_strictMode() { assertThatThrownBy(() -> ToolNameValidator.validate("", true)).isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("null or empty"); } @Test - void invalidToolName_tooLong_strictMode() { + void strictLength() { String name = "a".repeat(129); assertThatThrownBy(() -> ToolNameValidator.validate(name, true)).isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("128 characters"); @@ -79,18 +106,42 @@ class ToolNameValidatorTests { "tööl", // non-ASCII "工具" // unicode }) - void invalidToolNames_specialCharacters_strictMode(String name) { + void strictInvalidCharacters(String name) { assertThatThrownBy(() -> ToolNameValidator.validate(name, true)).isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("invalid characters"); } @Test - void invalidToolName_nonStrictMode_doesNotThrow() { - // strict=false means warn only, should not throw - assertThatCode(() -> ToolNameValidator.validate("invalid name", false)).doesNotThrowAnyException(); + void lenientNull() { assertThatCode(() -> ToolNameValidator.validate(null, false)).doesNotThrowAnyException(); + assertThat(logAppender.list).satisfies(hasWarning("null or empty")); + } + + @Test + void lenientEmpty() { assertThatCode(() -> ToolNameValidator.validate("", false)).doesNotThrowAnyException(); + assertThat(logAppender.list).satisfies(hasWarning("null or empty")); + } + + @Test + void lenientLength() { assertThatCode(() -> ToolNameValidator.validate("a".repeat(129), false)).doesNotThrowAnyException(); + assertThat(logAppender.list).satisfies(hasWarning("128 characters")); + } + + @Test + void lenientInvalidCharacters() { + assertThatCode(() -> ToolNameValidator.validate("invalid name", false)).doesNotThrowAnyException(); + assertThat(logAppender.list).satisfies(hasWarning("invalid characters")); + } + + private Consumer> hasWarning(String errorMessage) { + return logs -> { + assertThat(logs).hasSize(1).first().satisfies(log -> { + assertThat(log.getLevel()).isEqualTo(Level.WARN); + assertThat(log.getFormattedMessage()).contains(errorMessage); + }); + }; } }