[ISSUE #13497]feat: return mcpId when create mcpserver #13561
Conversation
|
Thanks for your this PR. 🙏 感谢您提交的PR。 🙏 |
feat: 返回创建MCP服务器的mcpId并增强自定义ID验证变更概述新功能
重构
测试更新
变更文件
💡 小贴士与 lingma-agents 交流的方式📜 直接回复评论
📜 在代码行处标记
📜 在讨论中提问
|
![lingma-agents[bot]](https://avatars.githubusercontent.com/in/1198481?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔎 代码评审报告
🎯 评审意见概览
| 严重度 | 数量 | 说明 |
|---|---|---|
| 🔴 Blocker | 0 | 阻断性问题，需立即修复。例如：系统崩溃、关键功能不可用或严重安全漏洞。 |
| 🟠 Critical | 1 | 严重问题，高优先级修复。例如：核心功能异常或性能瓶颈影响用户体验。 |
| 🟡 Major | 1 | 主要问题，建议修复。例如：非核心功能缺陷或代码维护性较差。 |
| 🟢 Minor | 2 | 次要问题，酬情优化。例如：代码格式不规范或注释缺失。 |
总计: 4 个问题
📋 评审意见详情
💡 代码实现建议
以下是文件级别的代码建议，聚焦于代码的可读性、可维护性和潜在问题。
☕ ai/src/main/java/com/alibaba/nacos/ai/controller/McpAdminController.java (1 💬)
- 返回结果未处理异常情况导致潜在空指针风险 (L113-L115)
☕ client/src/test/java/com/alibaba/nacos/client/utils/StringUtilsTest.java (1 💬)
- UUID测试用例未覆盖大写格式 (L88-L93)
☕ console/src/main/java/com/alibaba/nacos/console/handler/ai/McpHandler.java (1 💬)
- 接口返回类型变更未更新文档注释 (L68-L71)
☕ maintainer-client/src/main/java/com/alibaba/nacos/maintainer/client/ai/NacosAiMaintainerServiceImpl.java (1 💬)
- HTTP响应处理未校验状态码 (L135)
🚀 架构设计建议
以下是对代码架构和设计的综合分析，聚焦于跨文件交互、系统一致性和潜在优化空间。
🔍1. 接口返回类型变更未全面覆盖所有实现
McpHandler接口的createMcpServer方法返回类型由void变更为String后，需确保所有实现类（如McpInnerHandler、McpRemoteHandler）均正确返回mcpId。当前McpInnerHandler和McpRemoteHandler的实现虽然调整了方法签名，但仅简单调用服务方法返回结果，未验证返回值是否为空或异常情况。此外，下游调用链（如ConsoleMcpController）需确保接收并正确处理返回的mcpId。
📌 关键代码
String createMcpServer(String namespaceId, McpServerBasicInfo serverSpecification, McpToolSpecification toolSpecification, McpEndpointSpec endpointSpecification) throws NacosException;public String createMcpServer(...) {
return mcpServerOperationService.createMcpServer(...);
}未处理返回值空指针或异常情况可能导致系统级错误，下游接口未正确解析mcpId将导致功能失效。
🔍2. HTTP响应处理未校验状态码
NacosAiMaintainerServiceImpl的createMcpServer方法直接返回Result.getData()，但未校验HTTP响应状态码是否成功。若服务端返回错误状态码（如500），可能导致客户端错误地解析响应内容并返回无效mcpId，引发后续操作失败。
📌 关键代码
return result.getData();未校验HTTP状态码可能导致错误响应被误认为成功，引发系统数据不一致或后续操作异常。
🔍3. UUID生成与验证逻辑存在潜在不一致性
McpServerOperationService中新增的UUID验证逻辑（检查格式和冲突）与StringUtils的isUUIDString方法绑定，但未确保其他模块（如McpMaintainerService）在生成或验证mcpId时统一使用该方法。例如，NacosAiMaintainerServiceImplTest中直接使用UUID.randomUUID().toString()生成测试ID，未通过服务层统一封装逻辑，可能引入实现差异。
📌 关键代码
if (!StringUtils.isUUIDString(customMcpId)) { ... }String mcpId = UUID.randomUUID().toString();UUID生成/验证逻辑分散可能导致不同模块对ID格式或冲突的处理标准不一致，引发系统级ID冲突或验证失败。
🔍4. 异常处理缺失导致潜在空指针风险
McpAdminController的createMcpServer方法直接返回mcpId，但未校验mcpServerOperationService.createMcpServer的返回结果是否为null。若服务层返回null（如异常处理未覆盖场景），将引发空指针异常，破坏接口稳定性。
📌 关键代码
return Result.success(mcpId);未处理null返回值可能导致接口抛出空指针异常，中断服务调用链。
🔍5. 接口文档注释未同步更新
McpHandler接口的createMcpServer方法返回类型变更后，相关实现类（如McpProxy）和调用方（如ConsoleMcpController）的文档注释未同步更新，导致开发人员无法准确理解返回值含义。
📌 关键代码
* @return mcp server id of the new mcp server不一致的文档注释可能导致下游开发人员误解接口行为，引发集成错误。
审查详情
📒 文件清单 （12 个文件）
📝 变更: 12 个文件
📝 变更文件:
ai/src/main/java/com/alibaba/nacos/ai/controller/McpAdminController.javaai/src/main/java/com/alibaba/nacos/ai/service/McpServerOperationService.javaclient/src/test/java/com/alibaba/nacos/client/utils/StringUtilsTest.javacommon/src/main/java/com/alibaba/nacos/common/utils/StringUtils.javaconsole/src/main/java/com/alibaba/nacos/console/controller/v3/ai/ConsoleMcpController.javaconsole/src/main/java/com/alibaba/nacos/console/handler/ai/McpHandler.javaconsole/src/main/java/com/alibaba/nacos/console/handler/impl/inner/ai/McpInnerHandler.javaconsole/src/main/java/com/alibaba/nacos/console/handler/impl/remote/ai/McpRemoteHandler.javaconsole/src/main/java/com/alibaba/nacos/console/proxy/ai/McpProxy.javamaintainer-client/src/main/java/com/alibaba/nacos/maintainer/client/ai/McpMaintainerService.javamaintainer-client/src/main/java/com/alibaba/nacos/maintainer/client/ai/NacosAiMaintainerServiceImpl.javamaintainer-client/src/test/java/com/alibaba/nacos/maintainer/client/ai/NacosAiMaintainerServiceImplTest.java
💡 小贴士
与 lingma-agents 交流的方式
📜 直接回复评论
直接回复本条评论，lingma-agents 将自动处理您的请求。例如：
-
在当前代码中添加详细的注释说明。
-
请详细介绍一下你说的 LRU 改造方案，并使用伪代码加以说明。
📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如：
-
@lingma-agents 分析这个方法的性能瓶颈并提供优化建议。
-
@lingma-agents 对这个方法生成优化代码。
📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如：
-
@lingma-agents 请总结上述讨论并提出解决方案。
-
@lingma-agents 请根据讨论内容生成优化代码。
| String mcpId = mcpServerOperationService.createMcpServer(mcpForm.getNamespaceId(), basicInfo, mcpTools, | ||
| endpointSpec); | ||
| return Result.success("ok"); | ||
| return Result.success(mcpId); |
There was a problem hiding this comment.
返回结果未处理异常情况导致潜在空指针风险
🟠 Critical | 🐞 Bugs
📋 问题详情
在createMcpServer方法中，直接返回mcpServerOperationService创建方法的结果，但未处理该方法可能抛出的异常。当服务层返回null时可能导致空指针异常，且未对异常场景进行错误码返回处理
💡 解决方案
需要增加对mcpId的空值检查，并处理异常场景返回错误信息：
- String mcpId = mcpServerOperationService.createMcpServer(mcpForm.getNamespaceId(), basicInfo, mcpTools,
- endpointSpec);
- return Result.success(mcpId);
+ String mcpId;
+ try {
+ mcpId = mcpServerOperationService.createMcpServer(mcpForm.getNamespaceId(), basicInfo, mcpTools, endpointSpec);
+ if (StringUtils.isEmpty(mcpId)) {
+ throw new NacosException(NacosException.INTERNALServerError, "MCP server ID generation failed");
+ }
+ return Result.success(mcpId);
+ } catch (NacosException e) {
+ return Result.fail(e.getErrorCode(), e.getMessage());
+ }您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
There was a problem hiding this comment.
this is not true, because if it return mcpId, it must have value
There was a problem hiding this comment.
根据代码逻辑分析，createMcpServer方法的返回值mcpId确实需要区分两种风险场景：
-
异常场景：即使方法返回值不为空，但若方法内部出现异常（如数据库操作失败），需要捕获
NacosException并返回错误码。当前方法签名声明了throws NacosException，但未在调用处处理，可能导致未捕获异常抛出。 -
空值场景：如果
createMcpServer方法在某些情况下返回空字符串（例如服务端逻辑未正确生成ID但未抛异常），直接返回会导致前端接收空值。虽然您认为"must have value"，但需要确认该方法是否严格保证非空返回。
建议补充异常捕获逻辑，保持接口的健壮性：
try {
String mcpId = mcpServerOperationService.createMcpServer(...);
return Result.success(mcpId);
} catch (NacosException e) {
return Result.fail(e.getErrorCode(), e.getMessage());
}这样既保证了异常场景的可控性，又保留了您的设计逻辑。是否需要空值检查取决于createMcpServer的实现规范，建议与服务层开发者确认方法的返回值约束。
There was a problem hiding this comment.
evenif there occurs error inside method, the createMcpServer method have throw Exception, eventually, the client will get errorcode and errormsg
There was a problem hiding this comment.
@jujiale 您的分析需要结合方法抛出异常的传播路径来判断：
-
异常传播逻辑
当前createMcpServer方法声明抛出NacosException，但调用该方法的客户端（如Controller）若未捕获异常，异常会向上抛出。若未在Spring Boot的全局异常处理器（如@ControllerAdvice）中捕获NacosException，则会触发HTTP 500错误，无法返回结构化的错误响应（如包含errorCode和message的JSON）。您的方案需要确保全局异常处理已覆盖此场景。 -
显式错误封装的优势
建议的try-catch方案能直接返回Result.fail()结构化响应，即使存在全局异常处理，显式捕获可：
- 精确控制错误码（如区分数据库异常与ID生成失败）
- 避免堆栈追踪暴露敏感信息
- 统一前后端错误协议
- 空值风险的补充说明
若createMcpServer保证非空返回（如通过Javadoc标注@Nonnull），则无需空值检查。但若服务层可能出现空值（如网络超时未抛异常但返回空），则需要补充校验。
综上，是否需要修改取决于：
- 全局异常处理是否已覆盖
NacosException createMcpServer的返回值约束是否严格定义
建议与服务层开发者确认方法规范，并检查是否存在未声明的异常场景（如NullPointerException）。
There was a problem hiding this comment.
@jujiale 您的分析需要结合以下关键点进一步确认：
- 全局异常处理有效性
若当前NacosException已被全局异常处理器（如@ControllerAdvice）捕获并转换为结构化响应（如Result.fail()格式），则无需本地try-catch。但需确认：
- 全局处理器是否精确处理
NacosException - 是否包含
errorCode和message的正确封装
-
未声明异常的潜在风险
当前方法签名声明抛出NacosException，但若服务层可能抛出未声明的异常（如NullPointerException或IOException），则这些异常会直接导致HTTP 500，无法返回预期的错误码。 -
空值返回的边界情况
若createMcpServer方法在某些场景未抛异常但返回空字符串（例如数据库主键生成失败但未正确抛异常），直接返回会导致前端接收空值。需确认该方法的Javadoc是否明确标注@Nonnull。
建议行动项：
- 检查是否存在覆盖
NacosException的全局异常处理逻辑 - 与服务层开发者确认
createMcpServer的异常边界（是否包含未声明异常） - 若需保留当前设计，请补充方法返回值的非空断言（
Objects.requireNonNull(mcpId)）以明确契约
| // match 8-4-4-4-12 uuid pattern | ||
| assertTrue(StringUtils.isUUIDString("123e4567-e89b-12d3-a456-426655440000")); | ||
| // not match 8-4-4-4-12 uuid pattern | ||
| assertFalse(StringUtils.isUUIDString("123e54567-e89b5-12d35-a4565-426655440000")); | ||
| // not match hexadecimal and '-' char | ||
| assertFalse(StringUtils.isUUIDString("@23e4567+e89b-12d3-a456-426655440000")); |
| * @return mcp server id of the new mcp server | ||
| * @throws NacosException any exception during handling | ||
| */ | ||
| void createMcpServer(String namespaceId, McpServerBasicInfo serverSpecification, | ||
| String createMcpServer(String namespaceId, McpServerBasicInfo serverSpecification, |
| @@ -132,7 +132,7 @@ public boolean createMcpServer(String namespaceId, String mcpName, McpServerBasi | |||
| HttpRestResult<String> restResult = clientHttpProxy.executeSyncHttpRequest(httpRequest); | |||
| Result<String> result = JacksonUtils.toObj(restResult.getData(), new TypeReference<Result<String>>() { | |||
| }); | |||
| return ErrorCode.SUCCESS.getCode().equals(result.getCode()); | |||
| return result.getData(); | |||
There was a problem hiding this comment.
HTTP响应处理未校验状态码
🟡 Major | 🐞 Bugs
📋 问题详情
在createMcpServer方法中，直接使用restResult.getData()而未校验HTTP状态码是否成功，可能导致未预期的异常情况未被处理
💡 解决方案
需增加状态码校验：
+ if (restResult.getStatusCode() != 200) {
+ throw new NacosException(NacosException.HTTP_ERROR, "HTTP request failed with code: " + restResult.getStatusCode());
+ }
Result<String> result = JacksonUtils.toObj(restResult.getData(), new TypeReference<Result<String>>() {
});
+ if (!result.isSuccess()) {
+ throw new NacosException(result.getErrorCode(), result.getMessage());
+ }
return result.getData();您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
There was a problem hiding this comment.
because if the restResult returns, it must have mcpId in data
There was a problem hiding this comment.
当前逻辑存在潜在风险：即使HTTP响应返回，但若状态码为4xx/5xx错误（如参数错误或服务不可用），直接调用restResult.getData()可能返回空或无效数据。例如：
- 当后端返回400错误时，
restResult.getData()可能包含错误描述而非Result<String>结构 - 服务端异常导致响应体格式错误时，Jackson反序列化会抛出
JsonParseException - 业务层
Result对象的isSuccess()也需要校验，例如后端可能返回200但errorCode不为成功
开发者观点restResult返回必含mcpId的假设仅在以下条件成立时安全：
clientHttpProxy保证所有HTTP调用必定成功（但实际网络请求存在中断可能）- 服务端永远不会返回非2xx状态码（需服务端协议严格保证）
- 响应体永远符合
Result<String>结构（需前后端契约严格对齐）
建议至少添加状态码校验可避免因偶发网络问题或服务异常导致的未处理异常。
|
Thanks for contribution, after the ci pass, I will merge this pr. |
|
@jujiale CI check not pass Error: /home/runner/work/nacos/nacos/common/src/main/java/com/alibaba/nacos/common/utils/StringUtils.java:816:27: Abbreviation in name 'isUUIDString' must contain no more than '2' consecutive capital letters. [AbbreviationAsWordInName] maybe the method should change to |
|
BTW, do you has |
have send you |
|
modifed the test method name |
|
可以现在本地执行一下build命令 mvn -Prelease-nacos -DskipTests clean install -U -e -Dorg.slf4j.simpleLogger.log.org.apache.maven.cli.transfer.Slf4jMavenTransferListener=warn |
本地已ci通过，麻烦再操作下，谢谢 |
|
UT好像没有过 |
请先忽略，有个UT需要调整，我调整完后会整体跑一遍再提交 |
|
|
你好，UT已调整，麻烦有空的话再触发下，多谢 |
|
fixed |
|
|
Please do not create a Pull Request without creating an issue first.
What is the purpose of the change
#13553
Brief changelog
when add mcpserver, return mcpId in response body
Verifying this change
as the above issue mentioned, this pr inclued as the following
Follow this checklist to help us incorporate your contribution quickly and easily:
[ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.mvn -B clean package apache-rat:check findbugs:findbugs -Dmaven.test.skip=trueto make sure basic checks pass. Runmvn clean install -DskipITsto make sure unit-test pass. Runmvn clean test-compile failsafe:integration-testto make sure integration-test pass.