Skip to content

Commit be5ec12

Browse files
KehrlannKehrlann
committed
Polish gh-764
Signed-off-by: Daniel Garnier-Moiroux <git@garnier.wf>
1 parent 09a853f commit be5ec12

File tree

5 files changed

+117
-49
lines changed

5 files changed

+117
-49
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1466,6 +1466,7 @@ public Builder meta(Map<String, Object> meta) {
14661466
}
14671467

14681468
public Tool build() {
1469+
Assert.hasText(name, "name must not be empty");
14691470
return new Tool(name, title, description, inputSchema, outputSchema, annotations, meta);
14701471
}
14711472

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

Lines changed: 27 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -4,30 +4,33 @@
44

55
package io.modelcontextprotocol.server;
66

7-
import static io.modelcontextprotocol.util.ToolsUtils.EMPTY_JSON_SCHEMA;
8-
import static org.assertj.core.api.Assertions.assertThat;
9-
import static org.assertj.core.api.Assertions.assertThatCode;
10-
import static org.assertj.core.api.Assertions.assertThatThrownBy;
11-
import static org.mockito.Mockito.mock;
12-
137
import java.util.List;
148
import java.util.Map;
159

10+
import ch.qos.logback.classic.Logger;
11+
import ch.qos.logback.classic.spi.ILoggingEvent;
12+
import ch.qos.logback.core.read.ListAppender;
1613
import io.modelcontextprotocol.spec.McpSchema;
14+
import io.modelcontextprotocol.spec.McpSchema.CallToolRequest;
15+
import io.modelcontextprotocol.spec.McpSchema.CallToolResult;
16+
import io.modelcontextprotocol.spec.McpSchema.TextContent;
17+
import io.modelcontextprotocol.spec.McpSchema.Tool;
1718
import io.modelcontextprotocol.spec.McpServerTransportProvider;
1819
import io.modelcontextprotocol.util.ToolNameValidator;
1920
import org.junit.jupiter.api.AfterEach;
2021
import org.junit.jupiter.api.BeforeEach;
2122
import org.junit.jupiter.api.Nested;
2223
import org.junit.jupiter.api.Test;
23-
24-
import io.modelcontextprotocol.spec.McpSchema.CallToolRequest;
25-
import io.modelcontextprotocol.spec.McpSchema.CallToolResult;
26-
import io.modelcontextprotocol.spec.McpSchema.TextContent;
27-
import io.modelcontextprotocol.spec.McpSchema.Tool;
24+
import org.slf4j.LoggerFactory;
2825
import reactor.core.publisher.Mono;
2926
import reactor.test.StepVerifier;
3027

28+
import static io.modelcontextprotocol.util.ToolsUtils.EMPTY_JSON_SCHEMA;
29+
import static org.assertj.core.api.Assertions.assertThat;
30+
import static org.assertj.core.api.Assertions.assertThatCode;
31+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
32+
import static org.mockito.Mockito.mock;
33+
3134
/**
3235
* Tests for {@link McpServerFeatures.AsyncToolSpecification.Builder}.
3336
*
@@ -276,15 +279,23 @@ class ToolNameValidation {
276279

277280
private McpServerTransportProvider transportProvider;
278281

282+
private final Logger logger = (Logger) LoggerFactory.getLogger(ToolNameValidator.class);
283+
284+
private final ListAppender<ILoggingEvent> logAppender = new ListAppender<>();
285+
279286
@BeforeEach
280287
void setUp() {
281288
transportProvider = mock(McpServerTransportProvider.class);
282289
System.clearProperty(ToolNameValidator.STRICT_VALIDATION_PROPERTY);
290+
logAppender.start();
291+
logger.addAppender(logAppender);
283292
}
284293

285294
@AfterEach
286295
void tearDown() {
287296
System.clearProperty(ToolNameValidator.STRICT_VALIDATION_PROPERTY);
297+
logger.detachAppender(logAppender);
298+
logAppender.stop();
288299
}
289300

290301
@Test
@@ -297,25 +308,27 @@ void defaultShouldThrowOnInvalidName() {
297308
}
298309

299310
@Test
300-
void systemPropertyFalseShouldWarnOnly() {
311+
void lenientDefaultShouldLogOnInvalidName() {
301312
System.setProperty(ToolNameValidator.STRICT_VALIDATION_PROPERTY, "false");
302313
Tool invalidTool = Tool.builder().name("invalid tool name").build();
303314

304315
assertThatCode(() -> McpServer.async(transportProvider).tool(invalidTool, (exchange, args) -> null))
305316
.doesNotThrowAnyException();
317+
assertThat(logAppender.list).hasSize(1);
306318
}
307319

308320
@Test
309-
void perServerFalseShouldWarnOnly() {
321+
void lenientConfigurationShouldLogOnInvalidName() {
310322
Tool invalidTool = Tool.builder().name("invalid tool name").build();
311323

312324
assertThatCode(() -> McpServer.async(transportProvider)
313325
.strictToolNameValidation(false)
314326
.tool(invalidTool, (exchange, args) -> null)).doesNotThrowAnyException();
327+
assertThat(logAppender.list).hasSize(1);
315328
}
316329

317330
@Test
318-
void perServerTrueShouldOverrideSystemProperty() {
331+
void serverConfigurationShouldOverrideDefault() {
319332
System.setProperty(ToolNameValidator.STRICT_VALIDATION_PROPERTY, "false");
320333
Tool invalidTool = Tool.builder().name("invalid tool name").build();
321334

‎mcp-core/src/test/java/io/modelcontextprotocol/server/SyncToolSpecificationBuilderTest.java‎

Lines changed: 26 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -4,26 +4,29 @@
44

55
package io.modelcontextprotocol.server;
66

7-
import static io.modelcontextprotocol.util.ToolsUtils.EMPTY_JSON_SCHEMA;
8-
import static org.assertj.core.api.Assertions.assertThat;
9-
import static org.assertj.core.api.Assertions.assertThatCode;
10-
import static org.assertj.core.api.Assertions.assertThatThrownBy;
11-
import static org.mockito.Mockito.mock;
12-
137
import java.util.List;
148
import java.util.Map;
159

10+
import ch.qos.logback.classic.Logger;
11+
import ch.qos.logback.classic.spi.ILoggingEvent;
12+
import ch.qos.logback.core.read.ListAppender;
13+
import io.modelcontextprotocol.spec.McpSchema.CallToolRequest;
14+
import io.modelcontextprotocol.spec.McpSchema.CallToolResult;
15+
import io.modelcontextprotocol.spec.McpSchema.TextContent;
16+
import io.modelcontextprotocol.spec.McpSchema.Tool;
1617
import io.modelcontextprotocol.spec.McpServerTransportProvider;
1718
import io.modelcontextprotocol.util.ToolNameValidator;
1819
import org.junit.jupiter.api.AfterEach;
1920
import org.junit.jupiter.api.BeforeEach;
2021
import org.junit.jupiter.api.Nested;
2122
import org.junit.jupiter.api.Test;
23+
import org.slf4j.LoggerFactory;
2224

23-
import io.modelcontextprotocol.spec.McpSchema.CallToolRequest;
24-
import io.modelcontextprotocol.spec.McpSchema.CallToolResult;
25-
import io.modelcontextprotocol.spec.McpSchema.TextContent;
26-
import io.modelcontextprotocol.spec.McpSchema.Tool;
25+
import static io.modelcontextprotocol.util.ToolsUtils.EMPTY_JSON_SCHEMA;
26+
import static org.assertj.core.api.Assertions.assertThat;
27+
import static org.assertj.core.api.Assertions.assertThatCode;
28+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
29+
import static org.mockito.Mockito.mock;
2730

2831
/**
2932
* Tests for {@link McpServerFeatures.SyncToolSpecification.Builder}.
@@ -114,15 +117,23 @@ class ToolNameValidation {
114117

115118
private McpServerTransportProvider transportProvider;
116119

120+
private final Logger logger = (Logger) LoggerFactory.getLogger(ToolNameValidator.class);
121+
122+
private final ListAppender<ILoggingEvent> logAppender = new ListAppender<>();
123+
117124
@BeforeEach
118125
void setUp() {
119126
transportProvider = mock(McpServerTransportProvider.class);
120127
System.clearProperty(ToolNameValidator.STRICT_VALIDATION_PROPERTY);
128+
logAppender.start();
129+
logger.addAppender(logAppender);
121130
}
122131

123132
@AfterEach
124133
void tearDown() {
125134
System.clearProperty(ToolNameValidator.STRICT_VALIDATION_PROPERTY);
135+
logger.detachAppender(logAppender);
136+
logAppender.stop();
126137
}
127138

128139
@Test
@@ -135,25 +146,27 @@ void defaultShouldThrowOnInvalidName() {
135146
}
136147

137148
@Test
138-
void systemPropertyFalseShouldWarnOnly() {
149+
void lenientDefaultShouldLogOnInvalidName() {
139150
System.setProperty(ToolNameValidator.STRICT_VALIDATION_PROPERTY, "false");
140151
Tool invalidTool = Tool.builder().name("invalid tool name").build();
141152

142153
assertThatCode(() -> McpServer.sync(transportProvider).tool(invalidTool, (exchange, args) -> null))
143154
.doesNotThrowAnyException();
155+
assertThat(logAppender.list).hasSize(1);
144156
}
145157

146158
@Test
147-
void perServerFalseShouldWarnOnly() {
159+
void lenientConfigurationShouldLogOnInvalidName() {
148160
Tool invalidTool = Tool.builder().name("invalid tool name").build();
149161

150162
assertThatCode(() -> McpServer.sync(transportProvider)
151163
.strictToolNameValidation(false)
152164
.tool(invalidTool, (exchange, args) -> null)).doesNotThrowAnyException();
165+
assertThat(logAppender.list).hasSize(1);
153166
}
154167

155168
@Test
156-
void perServerTrueShouldOverrideSystemProperty() {
169+
void serverConfigurationShouldOverrideDefault() {
157170
System.setProperty(ToolNameValidator.STRICT_VALIDATION_PROPERTY, "false");
158171
Tool invalidTool = Tool.builder().name("invalid tool name").build();
159172

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

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1765,14 +1765,4 @@ void testProgressNotificationWithoutMessage() throws Exception {
17651765
{"progressToken":"progress-token-789","progress":0.25}"""));
17661766
}
17671767

1768-
// Tool Name Validation Tests
1769-
1770-
@Test
1771-
void testToolBuilderWithValidName() {
1772-
McpSchema.Tool tool = McpSchema.Tool.builder().name("valid_tool-name.v1").description("A test tool").build();
1773-
1774-
assertThat(tool.name()).isEqualTo("valid_tool-name.v1");
1775-
assertThat(tool.description()).isEqualTo("A test tool");
1776-
}
1777-
17781768
}

‎mcp-core/src/test/java/io/modelcontextprotocol/util/ToolNameValidatorTests.java‎

Lines changed: 63 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,21 @@
44

55
package io.modelcontextprotocol.util;
66

7+
import java.util.List;
8+
import java.util.function.Consumer;
9+
10+
import ch.qos.logback.classic.Level;
11+
import ch.qos.logback.classic.Logger;
12+
import ch.qos.logback.classic.spi.ILoggingEvent;
13+
import ch.qos.logback.core.read.ListAppender;
14+
import org.junit.jupiter.api.AfterEach;
15+
import org.junit.jupiter.api.BeforeEach;
716
import org.junit.jupiter.api.Test;
817
import org.junit.jupiter.params.ParameterizedTest;
918
import org.junit.jupiter.params.provider.ValueSource;
19+
import org.slf4j.LoggerFactory;
1020

21+
import static org.assertj.core.api.Assertions.assertThat;
1122
import static org.assertj.core.api.Assertions.assertThatCode;
1223
import static org.assertj.core.api.Assertions.assertThatThrownBy;
1324

@@ -16,33 +27,49 @@
1627
*/
1728
class ToolNameValidatorTests {
1829

30+
private final Logger logger = (Logger) LoggerFactory.getLogger(ToolNameValidator.class);
31+
32+
private final ListAppender<ILoggingEvent> logAppender = new ListAppender<>();
33+
34+
@BeforeEach
35+
void setUp() {
36+
logAppender.start();
37+
logger.addAppender(logAppender);
38+
}
39+
40+
@AfterEach
41+
void tearDown() {
42+
logger.detachAppender(logAppender);
43+
logAppender.stop();
44+
}
45+
1946
@ParameterizedTest
2047
@ValueSource(strings = { "getUser", "DATA_EXPORT_v2", "admin.tools.list", "my-tool", "Tool123", "a", "A",
2148
"_private", "tool_name", "tool-name", "tool.name", "UPPERCASE", "lowercase", "MixedCase123" })
22-
void validToolNames_strictMode(String name) {
49+
void validToolNames(String name) {
2350
assertThatCode(() -> ToolNameValidator.validate(name, true)).doesNotThrowAnyException();
51+
ToolNameValidator.validate(name, false);
52+
assertThat(logAppender.list).isEmpty();
2453
}
2554

2655
@Test
27-
void validToolName_maxLength() {
56+
void validToolNameMaxLength() {
2857
String name = "a".repeat(128);
2958
assertThatCode(() -> ToolNameValidator.validate(name, true)).doesNotThrowAnyException();
59+
ToolNameValidator.validate(name, false);
60+
assertThat(logAppender.list).isEmpty();
3061
}
3162

3263
@Test
33-
void invalidToolName_null_strictMode() {
64+
void nullOrEmpty() {
3465
assertThatThrownBy(() -> ToolNameValidator.validate(null, true)).isInstanceOf(IllegalArgumentException.class)
3566
.hasMessageContaining("null or empty");
36-
}
37-
38-
@Test
39-
void invalidToolName_empty_strictMode() {
4067
assertThatThrownBy(() -> ToolNameValidator.validate("", true)).isInstanceOf(IllegalArgumentException.class)
4168
.hasMessageContaining("null or empty");
4269
}
4370

4471
@Test
45-
void invalidToolName_tooLong_strictMode() {
72+
void strictLength() {
4673
String name = "a".repeat(129);
4774
assertThatThrownBy(() -> ToolNameValidator.validate(name, true)).isInstanceOf(IllegalArgumentException.class)
4875
.hasMessageContaining("128 characters");
@@ -79,18 +106,42 @@ void invalidToolName_tooLong_strictMode() {
79106
"tööl", // non-ASCII
80107
"工具" // unicode
81108
})
82-
void invalidToolNames_specialCharacters_strictMode(String name) {
109+
void strictInvalidCharacters(String name) {
83110
assertThatThrownBy(() -> ToolNameValidator.validate(name, true)).isInstanceOf(IllegalArgumentException.class)
84111
.hasMessageContaining("invalid characters");
85112
}
86113

87114
@Test
88-
void invalidToolName_nonStrictMode_doesNotThrow() {
89-
// strict=false means warn only, should not throw
90-
assertThatCode(() -> ToolNameValidator.validate("invalid name", false)).doesNotThrowAnyException();
115+
void lenientNull() {
91116
assertThatCode(() -> ToolNameValidator.validate(null, false)).doesNotThrowAnyException();
117+
assertThat(logAppender.list).satisfies(hasWarning("null or empty"));
118+
}
119+
120+
@Test
121+
void lenientEmpty() {
92122
assertThatCode(() -> ToolNameValidator.validate("", false)).doesNotThrowAnyException();
123+
assertThat(logAppender.list).satisfies(hasWarning("null or empty"));
124+
}
125+
126+
@Test
127+
void lenientLength() {
93128
assertThatCode(() -> ToolNameValidator.validate("a".repeat(129), false)).doesNotThrowAnyException();
129+
assertThat(logAppender.list).satisfies(hasWarning("128 characters"));
130+
}
131+
132+
@Test
133+
void lenientInvalidCharacters() {
134+
assertThatCode(() -> ToolNameValidator.validate("invalid name", false)).doesNotThrowAnyException();
135+
assertThat(logAppender.list).satisfies(hasWarning("invalid characters"));
136+
}
137+
138+
private Consumer<List<? extends ILoggingEvent>> hasWarning(String errorMessage) {
139+
return logs -> {
140+
assertThat(logs).hasSize(1).first().satisfies(log -> {
141+
assertThat(log.getLevel()).isEqualTo(Level.WARN);
142+
assertThat(log.getFormattedMessage()).contains(errorMessage);
143+
});
144+
};
94145
}
95146

96147
}

0 commit comments

Comments
 (0)