feat: #SEP-986 refactored validation control property to avoid negation in name, used nested tests and minor refactorings done

Author: ashakirin

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

@@ -659,7 +659,7 @@ private Mono<McpSchema.ListToolsResult> listToolsInternal(Initialization init, S
 			.doOnNext(result -> {
 				// Validate tool names (warn only)
 				if (result.tools() != null) {
-					result.tools().forEach(tool -> ToolNameValidator.validate(tool.name(), true));
+					result.tools().forEach(tool -> ToolNameValidator.validate(tool.name(), false));
 				}
 				if (this.enableCallToolSchemaCaching && result.tools() != null) {
 					// Cache tools output schema

mcp-core/src/main/java/io/modelcontextprotocol/server/McpServer.java

@@ -286,7 +286,7 @@ abstract class AsyncSpecification<S extends AsyncSpecification<S>> {
 
 		String instructions;
 
-		Boolean skipStrictToolNameValidation;
+		boolean strictToolNameValidation = ToolNameValidator.isStrictByDefault();
 
 		/**
 		 * The Model Context Protocol (MCP) allows servers to expose tools that can be
@@ -405,14 +405,14 @@ public AsyncSpecification<S> instructions(String instructions) {
 		}
 
 		/**
-		 * Sets whether to skip strict tool name validation for this server. When set,
-		 * this takes priority over the system property
-		 * {@code io.modelcontextprotocol.skipStrictToolNameValidation}.
-		 * @param skip true to warn only, false to throw exception on invalid names
+		 * Sets whether to use strict tool name validation for this server. When set, this
+		 * takes priority over the system property
+		 * {@code io.modelcontextprotocol.strictToolNameValidation}.
+		 * @param strict true to throw exception on invalid names and false to warn only
 		 * @return This builder instance for method chaining
 		 */
-		public AsyncSpecification<S> skipStrictToolNameValidation(boolean strict) {
-			this.skipStrictToolNameValidation = strict;
+		public AsyncSpecification<S> strictToolNameValidation(boolean strict) {
+			this.strictToolNameValidation = strict;
 			return this;
 		}
 
@@ -553,7 +553,7 @@ public AsyncSpecification<S> tools(McpServerFeatures.AsyncToolSpecification... t
 		}
 
 		private void validateToolName(String toolName) {
-			ToolNameValidator.validate(toolName, this.skipStrictToolNameValidation);
+			ToolNameValidator.validate(toolName, this.strictToolNameValidation);
 		}
 
 		private void assertNoDuplicateTool(String toolName) {
@@ -905,7 +905,7 @@ abstract class SyncSpecification<S extends SyncSpecification<S>> {
 
 		String instructions;
 
-		Boolean skipStrictToolNameValidation;
+		boolean strictToolNameValidation = ToolNameValidator.isStrictByDefault();
 
 		/**
 		 * The Model Context Protocol (MCP) allows servers to expose tools that can be
@@ -1028,14 +1028,14 @@ public SyncSpecification<S> instructions(String instructions) {
 		}
 
 		/**
-		 * Sets whether to skip strict tool name validation for this server. When set,
-		 * this takes priority over the system property
-		 * {@code io.modelcontextprotocol.skipStrictToolNameValidation}.
-		 * @param skip true to warn only, false to throw exception on invalid names
+		 * Sets whether to use strict tool name validation for this server. When set, this
+		 * takes priority over the system property
+		 * {@code io.modelcontextprotocol.strictToolNameValidation}.
+		 * @param strict true to throw exception on invalid names, false to warn only
 		 * @return This builder instance for method chaining
 		 */
-		public SyncSpecification<S> skipStrictToolNameValidation(boolean strict) {
-			this.skipStrictToolNameValidation = strict;
+		public SyncSpecification<S> strictToolNameValidation(boolean strict) {
+			this.strictToolNameValidation = strict;
 			return this;
 		}
 
@@ -1175,7 +1175,7 @@ public SyncSpecification<S> tools(McpServerFeatures.SyncToolSpecification... too
 		}
 
 		private void validateToolName(String toolName) {
-			ToolNameValidator.validate(toolName, this.skipStrictToolNameValidation);
+			ToolNameValidator.validate(toolName, this.strictToolNameValidation);
 		}
 
 		private void assertNoDuplicateTool(String toolName) {
@@ -1473,7 +1473,7 @@ class StatelessAsyncSpecification {
 
 		String instructions;
 
-		Boolean skipStrictToolNameValidation;
+		boolean strictToolNameValidation = ToolNameValidator.isStrictByDefault();
 
 		/**
 		 * The Model Context Protocol (MCP) allows servers to expose tools that can be
@@ -1593,14 +1593,14 @@ public StatelessAsyncSpecification instructions(String instructions) {
 		}
 
 		/**
-		 * Sets whether to skip strict tool name validation for this server. When set,
-		 * this takes priority over the system property
-		 * {@code io.modelcontextprotocol.skipStrictToolNameValidation}.
-		 * @param skip true to warn only, false to throw exception on invalid names
+		 * Sets whether to use strict tool name validation for this server. When set, this
+		 * takes priority over the system property
+		 * {@code io.modelcontextprotocol.strictToolNameValidation}.
+		 * @param strict true to throw exception on invalid names, false to warn only
 		 * @return This builder instance for method chaining
 		 */
-		public StatelessAsyncSpecification skipStrictToolNameValidation(boolean strict) {
-			this.skipStrictToolNameValidation = strict;
+		public StatelessAsyncSpecification strictToolNameValidation(boolean strict) {
+			this.strictToolNameValidation = strict;
 			return this;
 		}
 
@@ -1702,7 +1702,7 @@ public StatelessAsyncSpecification tools(
 		}
 
 		private void validateToolName(String toolName) {
-			ToolNameValidator.validate(toolName, this.skipStrictToolNameValidation);
+			ToolNameValidator.validate(toolName, this.strictToolNameValidation);
 		}
 
 		private void assertNoDuplicateTool(String toolName) {
@@ -1956,7 +1956,7 @@ class StatelessSyncSpecification {
 
 		String instructions;
 
-		Boolean skipStrictToolNameValidation;
+		boolean strictToolNameValidation = ToolNameValidator.isStrictByDefault();
 
 		/**
 		 * The Model Context Protocol (MCP) allows servers to expose tools that can be
@@ -2076,14 +2076,14 @@ public StatelessSyncSpecification instructions(String instructions) {
 		}
 
 		/**
-		 * Sets whether to skip strict tool name validation for this server. When set,
-		 * this takes priority over the system property
-		 * {@code io.modelcontextprotocol.skipStrictToolNameValidation}.
-		 * @param skip true to warn only, false to throw exception on invalid names
+		 * Sets whether to use strict tool name validation for this server. When set, this
+		 * takes priority over the system property
+		 * {@code io.modelcontextprotocol.strictToolNameValidation}.
+		 * @param strict true to throw exception on invalid names, false to warn only
 		 * @return This builder instance for method chaining
 		 */
-		public StatelessSyncSpecification skipStrictToolNameValidation(boolean strict) {
-			this.skipStrictToolNameValidation = strict;
+		public StatelessSyncSpecification strictToolNameValidation(boolean strict) {
+			this.strictToolNameValidation = strict;
 			return this;
 		}
 
@@ -2185,7 +2185,7 @@ public StatelessSyncSpecification tools(
 		}
 
 		private void validateToolName(String toolName) {
-			ToolNameValidator.validate(toolName, this.skipStrictToolNameValidation);
+			ToolNameValidator.validate(toolName, this.strictToolNameValidation);
 		}
 
 		private void assertNoDuplicateTool(String toolName) {

mcp-core/src/main/java/io/modelcontextprotocol/util/ToolNameValidator.java

@@ -23,6 +23,7 @@
  * @see <a href=
  * "https://modelcontextprotocol.io/specification/draft/server/tools#tool-names">MCP
  * Specification - Tool Names</a>
+ * @author Andrei Shakirin
  */
 public final class ToolNameValidator {
 
@@ -33,55 +34,49 @@ public final class ToolNameValidator {
 	private static final Pattern VALID_NAME_PATTERN = Pattern.compile("^[A-Za-z0-9_\\-.]+$");
 
 	/**
-	 * System property to skip strict tool name validation. Set to "true" to warn only
-	 * instead of throwing exceptions. Default is false (strict).
+	 * System property for strict tool name validation. Set to "false" to warn only
+	 * instead of throwing exceptions. Default is true (strict).
 	 */
-	public static final String SKIP_STRICT_VALIDATION_PROPERTY = "io.modelcontextprotocol.skipStrictToolNameValidation";
+	public static final String STRICT_VALIDATION_PROPERTY = "io.modelcontextprotocol.strictToolNameValidation";
 
 	private ToolNameValidator() {
 	}
 
 	/**
-	 * Validates a tool name according to MCP specification. Uses strict validation
-	 * (throws exception) unless system property {@link #SKIP_STRICT_VALIDATION_PROPERTY}
-	 * is set to true.
-	 * @param name the tool name to validate
-	 * @throws IllegalArgumentException if validation fails and skip is not enabled
+	 * Returns the default strict validation setting from system property.
+	 * @return true if strict validation is enabled (default), false if disabled via
+	 * system property
 	 */
-	public static void validate(String name) {
-		validate(name, null);
+	public static boolean isStrictByDefault() {
+		return !"false".equalsIgnoreCase(System.getProperty(STRICT_VALIDATION_PROPERTY));
 	}
 
 	/**
 	 * Validates a tool name according to MCP specification.
 	 * @param name the tool name to validate
-	 * @param skip if true, logs warning only; if false, throws exception; if null, uses
-	 * system property {@link #SKIP_STRICT_VALIDATION_PROPERTY} (default: false)
-	 * @throws IllegalArgumentException if validation fails and skip is false
+	 * @param strict if true, throws exception on invalid name; if false, logs warning
+	 * only
+	 * @throws IllegalArgumentException if validation fails and strict is true
 	 */
-	public static void validate(String name, Boolean skip) {
-		boolean warnOnly = skip != null ? skip : Boolean.getBoolean(SKIP_STRICT_VALIDATION_PROPERTY);
-
+	public static void validate(String name, boolean strict) {
 		if (name == null || name.isEmpty()) {
-			handleError("Tool name must not be null or empty", name, warnOnly);
-			return;
+			handleError("Tool name must not be null or empty", name, strict);
 		}
-		if (name.length() > MAX_LENGTH) {
-			handleError("Tool name must not exceed 128 characters", name, warnOnly);
-			return;
+		else if (name.length() > MAX_LENGTH) {
+			handleError("Tool name must not exceed 128 characters", name, strict);
 		}
-		if (!VALID_NAME_PATTERN.matcher(name).matches()) {
-			handleError("Tool name contains invalid characters (allowed: A-Z, a-z, 0-9, _, -, .)", name, warnOnly);
+		else if (!VALID_NAME_PATTERN.matcher(name).matches()) {
+			handleError("Tool name contains invalid characters (allowed: A-Z, a-z, 0-9, _, -, .)", name, strict);
 		}
 	}
 
-	private static void handleError(String message, String name, boolean warnOnly) {
+	private static void handleError(String message, String name, boolean strict) {
 		String fullMessage = message + ": '" + name + "'";
-		if (warnOnly) {
-			logger.warn("{}. Processing continues, but tool name should be fixed.", fullMessage);
+		if (strict) {
+			throw new IllegalArgumentException(fullMessage);
 		}
 		else {
-			throw new IllegalArgumentException(fullMessage);
+			logger.warn("{}. Processing continues, but tool name should be fixed.", fullMessage);
 		}
 	}
 

mcp-core/src/test/java/io/modelcontextprotocol/server/AsyncToolSpecificationBuilderTest.java

@@ -18,6 +18,7 @@
 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;
@@ -34,19 +35,6 @@
  */
 class AsyncToolSpecificationBuilderTest {
 
-	private McpServerTransportProvider transportProvider;
-
-	@BeforeEach
-	void setUp() {
-		transportProvider = mock(McpServerTransportProvider.class);
-		System.clearProperty(ToolNameValidator.SKIP_STRICT_VALIDATION_PROPERTY);
-	}
-
-	@AfterEach
-	void tearDown() {
-		System.clearProperty(ToolNameValidator.SKIP_STRICT_VALIDATION_PROPERTY);
-	}
-
 	@Test
 	void builderShouldCreateValidAsyncToolSpecification() {
 
@@ -283,42 +271,60 @@ void fromSyncShouldReturnNullWhenSyncSpecIsNull() {
 		assertThat(McpServerFeatures.AsyncToolSpecification.fromSync(null)).isNull();
 	}
 
-	@Test
-	void toolNameValidation_defaultShouldThrowOnInvalidName() {
-		Tool invalidTool = Tool.builder().name("invalid tool name").build();
+	@Nested
+	class ToolNameValidation {
 
-		assertThatThrownBy(() -> McpServer.async(transportProvider).tool(invalidTool, (exchange, args) -> null))
-			.isInstanceOf(IllegalArgumentException.class)
-			.hasMessageContaining("invalid characters");
-	}
+		private McpServerTransportProvider transportProvider;
 
-	@Test
-	void toolNameValidation_systemPropertySkipShouldWarnOnly() {
-		System.setProperty(ToolNameValidator.SKIP_STRICT_VALIDATION_PROPERTY, "true");
-		Tool invalidTool = Tool.builder().name("invalid tool name").build();
+		@BeforeEach
+		void setUp() {
+			transportProvider = mock(McpServerTransportProvider.class);
+			System.clearProperty(ToolNameValidator.STRICT_VALIDATION_PROPERTY);
+		}
 
-		assertThatCode(() -> McpServer.async(transportProvider).tool(invalidTool, (exchange, args) -> null))
-			.doesNotThrowAnyException();
-	}
+		@AfterEach
+		void tearDown() {
+			System.clearProperty(ToolNameValidator.STRICT_VALIDATION_PROPERTY);
+		}
 
-	@Test
-	void toolNameValidation_perServerSkipShouldWarnOnly() {
-		Tool invalidTool = Tool.builder().name("invalid tool name").build();
+		@Test
+		void defaultShouldThrowOnInvalidName() {
+			Tool invalidTool = Tool.builder().name("invalid tool name").build();
 
-		assertThatCode(() -> McpServer.async(transportProvider)
-			.skipStrictToolNameValidation(true)
-			.tool(invalidTool, (exchange, args) -> null)).doesNotThrowAnyException();
-	}
+			assertThatThrownBy(() -> McpServer.async(transportProvider).tool(invalidTool, (exchange, args) -> null))
+				.isInstanceOf(IllegalArgumentException.class)
+				.hasMessageContaining("invalid characters");
+		}
+
+		@Test
+		void systemPropertyFalseShouldWarnOnly() {
+			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();
+		}
+
+		@Test
+		void perServerFalseShouldWarnOnly() {
+			Tool invalidTool = Tool.builder().name("invalid tool name").build();
+
+			assertThatCode(() -> McpServer.async(transportProvider)
+				.strictToolNameValidation(false)
+				.tool(invalidTool, (exchange, args) -> null)).doesNotThrowAnyException();
+		}
+
+		@Test
+		void perServerTrueShouldOverrideSystemProperty() {
+			System.setProperty(ToolNameValidator.STRICT_VALIDATION_PROPERTY, "false");
+			Tool invalidTool = Tool.builder().name("invalid tool name").build();
+
+			assertThatThrownBy(() -> McpServer.async(transportProvider)
+				.strictToolNameValidation(true)
+				.tool(invalidTool, (exchange, args) -> null)).isInstanceOf(IllegalArgumentException.class)
+				.hasMessageContaining("invalid characters");
+		}
 
-	@Test
-	void toolNameValidation_perServerSkipShouldOverrideSystemProperty() {
-		System.setProperty(ToolNameValidator.SKIP_STRICT_VALIDATION_PROPERTY, "true");
-		Tool invalidTool = Tool.builder().name("invalid tool name").build();
-
-		assertThatThrownBy(() -> McpServer.async(transportProvider)
-			.skipStrictToolNameValidation(false)
-			.tool(invalidTool, (exchange, args) -> null)).isInstanceOf(IllegalArgumentException.class)
-			.hasMessageContaining("invalid characters");
 	}
 
 }