Skip to content

Commit 7ad2d1e

Browse files
ttmunyungumattmunyunguma
committed
Rename illigal args to missing args, rename source set for consistency, review comments
review comment to make names consistent Co-authored-by: Andrew Rouse <[email protected]>
1 parent 7576891 commit 7ad2d1e

File tree

7 files changed

+37
-51
lines changed

7 files changed

+37
-51
lines changed

‎dev/io.openliberty.mcp.internal/src/io/openliberty/mcp/internal/McpCdiExtension.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,8 +63,10 @@ void afterDeploymentValidation(@Observes AfterDeploymentValidation afterDeployme
6363
private void reportOnToolArgEdgeCases(AfterDeploymentValidation afterDeploymentValidation) {
6464
StringBuilder sbBlankArgs = new StringBuilder("Blank arguments found in MCP Tool:");
6565
StringBuilder sbDuplicateArgs = new StringBuilder("Duplicate arguments found in MCP Tool:");
66+
StringBuilder sbMissingArgs = new StringBuilder("Missing arguments found in MCP Tool:");
6667
boolean blankArgumentsFound = false;
6768
boolean duplicateArgumentsFound = false;
69+
boolean missingArgumentName = false;
6870

6971
for (ToolMetadata tool : tools.getAllTools()) {
7072
Map<String, ArgumentMetadata> arguments = tool.arguments();
@@ -76,6 +78,10 @@ private void reportOnToolArgEdgeCases(AfterDeploymentValidation afterDeploymentV
7678
} else if (arguments.get(argName).isDuplicate()) {
7779
sbDuplicateArgs.append("\n").append("Tool: " + tool.getToolQualifiedName() + " - Argument: " + argName);
7880
duplicateArgumentsFound = true;
81+
} else if (argName.equals(ToolMetadata.MISSING_TOOL_ARG_NAME)) {
82+
sbMissingArgs.append("\n").append("Tool: " + tool.getToolQualifiedName());
83+
sbMissingArgs.append("\n Tool argument name was not provided for the parameter. Either add a name to the @ToolArg annotation, or to add the -parameters compiler option to use the parameter name");
84+
missingArgumentName = true;
7985
}
8086
}
8187
}
@@ -85,6 +91,9 @@ private void reportOnToolArgEdgeCases(AfterDeploymentValidation afterDeploymentV
8591
if (duplicateArgumentsFound) {
8692
afterDeploymentValidation.addDeploymentProblem(new Exception(sbDuplicateArgs.toString()));
8793
}
94+
if (missingArgumentName) {
95+
afterDeploymentValidation.addDeploymentProblem(new Exception(sbMissingArgs.toString()));
96+
}
8897
}
8998

9099
private void reportOnDuplicateTools(AfterDeploymentValidation afterDeploymentValidation) {

‎dev/io.openliberty.mcp.internal/src/io/openliberty/mcp/internal/ToolMetadata.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ public record ToolMetadata(Tool annotation, Bean<?> bean, AnnotatedMethod<?> met
2929
String name, String title, String description,
3030
List<Class<? extends Throwable>> businessExceptions) {
3131

32+
public static final String MISSING_TOOL_ARG_NAME = "<<<MISSING TOOL_ARG NAME>>>";
33+
3234
public record ArgumentMetadata(Type type, int index, String description, boolean required, boolean isDuplicate) {}
3335

3436
public record SpecialArgumentMetadata(SpecialArgumentType.Resolution typeResolution, int index) {}
@@ -84,7 +86,7 @@ private static String resolveArgumentName(AnnotatedParameter<?> param, ToolArg a
8486
return param.getJavaParameter().getName();
8587
}
8688

87-
throw new IllegalStateException("Parameter name not set. Ensure that javac -parameter flag is enabled");
89+
return MISSING_TOOL_ARG_NAME;
8890
}
8991

9092
private static List<SpecialArgumentMetadata> getSpecialArgumentList(AnnotatedMethod<?> method) {

‎dev/io.openliberty.mcp.internal_fat/.classpath‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
<attribute name="test" value="true"/>
66
</attributes>
77
</classpathentry>
8-
<classpathentry kind="src" path="noParamTest">
8+
<classpathentry kind="src" path="noParamNames">
99
<attributes>
1010
<attribute name="test" value="true"/>
1111
</attributes>

‎dev/io.openliberty.mcp.internal_fat/build.gradle‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -18,19 +18,19 @@ configurations.requiredLibs {
1818

1919
sourceSets {
2020
// Source set where we don't include parameter names
21-
noParamNamesTest {
21+
noParamNames {
2222
java {
23-
srcDirs = ['noParamTest']
23+
srcDirs = ['noParamNames']
2424
}
2525
}
2626
}
2727

2828
dependencies {
2929
requiredLibs 'org.json:json:20080701',
3030
'org.skyscreamer:jsonassert:2.0-rc1'
31-
noParamNamesTestCompileOnly project(':io.openliberty.mcp'),
31+
noParamNamesCompileOnly project(':io.openliberty.mcp'),
3232
project(path: ':io.openliberty.jakarta', configuration: 'cdi40')
33-
implementation sourceSets.noParamNamesTest.output
33+
implementation sourceSets.noParamNames.output
3434
}
3535

3636
/* Added to allow method parmaters to be available for MCP Tool Server*/
@@ -39,14 +39,14 @@ compileJava {
3939
options.compilerArgs << '-parameters'
4040
}
4141

42-
// Build the noParamNamesTest code into a jar
43-
tasks.register('noParamNamesTestJar', Jar) {
44-
dependsOn noParamNamesTestClasses
45-
from sourceSets.noParamNamesTest.output
46-
archiveFileName = 'noParamNamesTest.jar'
42+
// Build the noParamNames code into a jar
43+
tasks.register('noParamNamesJar', Jar) {
44+
dependsOn noParamNamesClasses
45+
from sourceSets.noParamNames.output
46+
archiveFileName = 'noParamNames.jar'
4747
}
4848

49-
// Build the noParamNamesTest jar before the autoFVT zip so that it gets included in the zip
49+
// Build the noParamNames jar before the autoFVT zip so that it gets included in the zip
5050
tasks.named('autoFVT') {
51-
dependsOn noParamNamesTestJar
51+
dependsOn noParamNamesJar
5252
}

‎dev/io.openliberty.mcp.internal_fat/fat/src/io/openliberty/mcp/internal/fat/tool/NoParamNameTest.java‎

Lines changed: 7 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -9,25 +9,18 @@
99
*******************************************************************************/
1010
package io.openliberty.mcp.internal.fat.tool;
1111

12-
import static com.ibm.websphere.simplicity.ShrinkHelper.DeployOptions.SERVER_ONLY;
12+
import java.util.List;
1313

14-
import org.jboss.shrinkwrap.api.ShrinkWrap;
15-
import org.jboss.shrinkwrap.api.spec.WebArchive;
16-
import org.json.JSONObject;
1714
import org.junit.AfterClass;
1815
import org.junit.BeforeClass;
1916
import org.junit.Test;
2017
import org.junit.runner.RunWith;
21-
import org.skyscreamer.jsonassert.JSONAssert;
22-
23-
import com.ibm.websphere.simplicity.ShrinkHelper;
2418

2519
import componenttest.annotation.Server;
2620
import componenttest.custom.junit.runner.FATRunner;
2721
import componenttest.topology.impl.LibertyServer;
2822
import componenttest.topology.utils.FATServletClient;
2923
import io.openliberty.mcp.internal.fat.noparamtool.NoParamTools;
30-
import io.openliberty.mcp.internal.fat.utils.HttpTestUtils;
3124

3225
@RunWith(FATRunner.class)
3326
public class NoParamNameTest extends FATServletClient {
@@ -36,40 +29,18 @@ public class NoParamNameTest extends FATServletClient {
3629

3730
@BeforeClass
3831
public static void setup() throws Exception {
39-
WebArchive war = ShrinkWrap.create(WebArchive.class, "toolTest.war").addPackage(NoParamTools.class.getPackage());
40-
41-
ShrinkHelper.exportDropinAppToServer(server, war, SERVER_ONLY);
42-
43-
server.startServer();
32+
ExpectedAppFailureValidator.deployAppToAssertFailure(server, "ExpectedNoParamNameFailureTest", NoParamTools.class.getPackage());
4433
}
4534

4635
@AfterClass
4736
public static void teardown() throws Exception {
48-
server.stopServer();
37+
server.stopServer(ExpectedAppFailureValidator.APP_START_FAILED_CODE);
4938
}
5039

5140
@Test
52-
public void testIllegalToolArgNameTool() throws Exception {
53-
String request = """
54-
{
55-
"jsonrpc": "2.0",
56-
"id": "2",
57-
"method": "tools/call",
58-
"params": {
59-
"name": "illegalToolArgNameTool",
60-
"arguments": {
61-
"input": "Hello"
62-
}
63-
}
64-
}
65-
""";
66-
67-
String response = HttpTestUtils.callMCP(server, "/toolTest", request);
68-
JSONObject jsonResponse = new JSONObject(response);
69-
70-
String expectedResponseString = """
71-
Exception thrown
72-
""";
73-
JSONAssert.assertEquals(expectedResponseString, response, true);
41+
public void testNoParamNameToolArg() throws Exception {
42+
String expectedErrorHeader = "Missing arguments found in MCP Tool:";
43+
List<String> expectedErrorList = List.of("io.openliberty.mcp.internal.fat.noparamtool.NoParamTools.missingToolArgNameTool");
44+
ExpectedAppFailureValidator.findAndAssertExpectedErrorsInLogs("Missing arguments found in MCP Tool: ", expectedErrorHeader, expectedErrorList, server);
7445
}
7546
}

‎dev/io.openliberty.mcp.internal_fat/noParamTest/io/openliberty/mcp/internal/fat/noparamtool/NoParamTools.java‎ renamed to ‎dev/io.openliberty.mcp.internal_fat/noParamNames/io/openliberty/mcp/internal/fat/noparamtool/NoParamTools.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,8 @@
1616
@ApplicationScoped
1717
public class NoParamTools {
1818

19-
@Tool(name = "illegalToolArgNameTool", title = "illegal ToolArgName Tool", description = "thows illegalArgumentExeption because of the ToolArgName")
20-
public String illegalToolArgNameTool(@ToolArg(name = Tool.ELEMENT_NAME, description = "input to echo") String input) {
19+
@Tool(name = "missingToolArgNameTool", title = "missing ToolArgName Tool", description = "ToolArgName is missing so app wont start")
20+
public String missingToolArgNameTool(@ToolArg(description = "input to echo") String input) {
2121
return input;
2222
}
2323
}

‎dev/io.openliberty.mcp.internal_fat/noParamNames/io/openliberty/mcp/internal/fat/noparamtool/README.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
# No Param Names
2+
3+
This source set contains a basic tool called *missingToolArgNameTool*. The reason the tool has been separated out of the the rest of the application is because we need this to be compiled without the -parameters flag. This allows us to test that if parameter names are not included, then we report an error.
4+

0 commit comments

Comments
 (0)