Upstream develop refactor regitry #13492
Conversation
… registry adapter.
… for registry adaptor.
优化MCP服务器索引与操作逻辑，增强分页与跨命名空间支持变更文件
时序图Sequence diagram for MCP server search logic
Sequence diagram for PR code logic with no detached elements
sequenceDiagram
participant McpRegistryController as 控制器
participant NacosMcpRegistryService as 服务层
participant McpServerOperationService as 操作服务
McpRegistryController->>NacosMcpRegistryService: listMcpServers(分页参数)
NacosMcpRegistryService->>NamespaceOperationService: 获取有序命名空间列表
NacosMcpRegistryService->>McpServerOperationService: 分页查询每个命名空间数据
loop 多命名空间数据合并
NacosMcpRegistryService->>McpServerOperationService: 获取当前命名空间分页结果
NacosMcpRegistryService-->>NacosMcpRegistryService: 合并分页结果并计算总页数
end
NacosMcpRegistryService-->>McpRegistryController: 返回聚合分页结果
💡 小贴士与 lingma-agents 交流的方式📜 直接回复评论
📜 在代码行处标记
📜 在讨论中提问
|
|
Thanks for your this PR. 🙏 感谢您提交的PR。 🙏 |
There was a problem hiding this comment.
🔍 代码评审报告
🎯 评审意见概览
| 严重度 | 数量 | 说明 |
|---|---|---|
| 🔴 Blocker | 0 | 阻断性问题，需立即修复。例如：系统崩溃、关键功能不可用或严重安全漏洞。 |
| 🟠 Critical | 1 | 严重问题，高优先级修复。例如：核心功能异常或性能瓶颈影响用户体验。 |
| 🟡 Major | 3 | 主要问题，建议修复。例如：非核心功能缺陷或代码维护性较差。 |
| 🟢 Minor | 3 | 次要问题，酌情优化。例如：代码格式不规范或注释缺失。 |
总计： 7 个问题
📋 评审意见详情
💡 单文件建议
以下是文件级别的代码建议，聚焦于代码的可读性、可维护性和潜在问题。
☕ ai/src/main/java/com/alibaba/nacos/ai/index/PlainMcpServerIndex.java (1 💬)
- 修复分页逻辑错误，确保跨命名空间搜索时正确计算总数和分页结果 (L81-L86)
☕ ai/src/test/java/com/alibaba/nacos/ai/service/McpServerOperationServiceTest.java (1 💬)
- 测试依赖mock未正确设置可能导致测试不通过 (L238)
☕ console/src/test/java/com/alibaba/nacos/console/handler/impl/inner/ai/McpInnerHandlerTest.java (1 💬)
- 命名空间参数使用硬编码字符串应替换为常量 (L56-L57)
☕ mcp-registry-adaptor/src/main/java/com/alibaba/nacos/mcpregistry/controller/McpRegistryController.java (1 💬)
- 表单验证未处理异常可能导致未处理的异常 (L83)
☕ mcp-registry-adaptor/src/main/java/com/alibaba/nacos/mcpregistry/service/NacosMcpRegistryService.java (1 💬)
- 统一异常抛出类型 (L158)
☕ mcp-registry-adaptor/src/test/java/com/alibaba/nacos/mcpregistry/controller/McpRegistryControllerTest.java (1 💬)
- 规范测试用例断言方式 (L149-L150)
☕ mcp-registry-adaptor/src/test/java/com/alibaba/nacos/mcpregistry/service/NacosMcpRegistryServiceTest.java (1 💬)
- Mock数据生成方法未保证测试可预测性 (L364)
🚀 跨文件建议
以下是对代码架构和设计的综合分析，聚焦于跨文件交互、系统一致性和潜在优化空间。
🔍 1. 跨命名空间分页逻辑可能未正确聚合总数导致数据不一致
在PlainMcpServerIndex.java和NacosMcpRegistryService.java中，分页逻辑被重构后可能未正确处理跨命名空间搜索时的总数计算。新实现使用searchMcpServerByName0方法时，虽然简化了代码，但未明确说明如何聚合多个命名空间的总数，可能导致分页的totalCount字段计算错误。例如，在listMcpServers方法中，当同时查询多个命名空间时，各命名空间的总数可能未被正确累加。
📌 关键代码：
public Page<McpServerIndexData> searchMcpServerByName(...) {
return searchMcpServerByName0(...);
}
private Page<McpServerIndexData> searchMcpServerByName0(...) {
Page<ConfigInfo> serverInfos = searchMcpServers(namespaceId, name, search, limit);
...
}private Page<McpServerBasicInfo> listMcpServerByNamespaceList(...) {
for (String each : namespaceIdList) {
namespaceResult = listMcpServerByNamespace(...);
totalCount += namespaceResult.getTotalCount();
}
...
}🔍 2. McpServerIndex数据验证缺失导致潜在空指针风险
在NacosMcpRegistryService.java的updateMcpServer方法中，移除了对服务器ID存在的显式检查(checkMcpServerIndex方法被删除)。虽然依赖McpServerIndex的getMcpServerById方法返回null时抛出异常，但在并发场景或缓存未命中情况下可能引发空指针异常。
📌 关键代码：
public void updateMcpServer(...) {
McpServerIndexData indexData = mcpServerIndex.getMcpServerById(serverId);
if (Objects.isNull(indexData)) {
throw new NacosApiException(...);
}
}🔍 3. 命名空间依赖注入未覆盖所有相关服务
NacosMcpRegistryService.java新增了NamespaceOperationService依赖，但在listMcpServerByNamespaceList方法中直接调用该服务获取命名空间列表，但未验证命名空间访问权限。若存在跨命名空间访问权限控制需求，可能导致安全漏洞。
📌 关键代码：
private List<String> fetchOrderedNamespaceList() {
return namespaceOperationService.getNamespaceList().stream()...
}🔍 4. 分页参数边界条件未充分测试
新增的NacosMcpRegistryServiceTest.java测试用例覆盖了部分分页场景，但未包含offset超过总数或limit为0等极端情况。现有测试用例中listMcpServersWithZeroOffset测试虽然设置了limit为0，但未验证分页结果是否符合预期。
📌 关键代码：
void listMcpServersWithZeroOffset() {
listServerForm.setLimit(0);
...
}🔍 5. 文件移动导致的包路径依赖问题
GetServerForm和ListServerForm类从ai模块移动到mcp-registry-adaptor模块后，可能存在未更新的引用。例如，McpRegistryController.java中虽然更新了导入路径，但需要确认其他模块（如测试用例）是否已同步更新依赖。
📌 关键代码：
import com.alibaba.nacos.mcpregistry.form.GetServerForm;
import com.alibaba.nacos.mcpregistry.form.ListServerForm;🔍 6. 分页数据流未保证原子性
在PlainMcpServerIndex.java中，searchMcpServers方法返回的Page对象直接赋值给totalCount，但未考虑分页查询时的并发修改问题。当多个线程同时修改数据时，可能导致总数统计与实际数据不一致。
📌 关键代码：
result.setTotalCount(serverInfos.getTotalCount());
result.setPagesAvailable((int) Math.ceil((double) serverInfos.getTotalCount() / (double) limit));🔍 7. 异常处理覆盖不足
McpRegistryController.java的GET接口新增了getServerForm.validate()，但未处理验证失败时的异常类型转换。例如，验证异常未转换为标准HTTP状态码，可能导致500错误而非预期的400错误。
📌 关键代码：
public Object getServer(...) {
getServerForm.validate();
}🔍 8. 命名空间ID处理逻辑不一致
McpServerOperationService.java的createMcpServer方法中，未对传入的namespaceId进行空值检查。当传入空命名空间ID时，可能创建到默认命名空间，但业务逻辑是否允许这种情况未明确说明。
📌 关键代码：
public String createMcpServer(String namespaceId, ...) {
McpServerIndexData existId = resolveMcpServerId(namespaceId, ...);
...
}🔍 9. 工具类依赖关系未解耦
NacosMcpRegistryService.java直接依赖McpServerIndex的具体实现（如getMcpServerById），而非通过接口定义，这会增加组件替换难度。
📌 关键代码：
private final McpServerIndex mcpServerIndex;
public NacosMcpRegistryService(...) {
this.mcpServerIndex = mcpServerIndex;
}🔍 10. 分页偏移量计算存在逻辑漏洞
在NacosMcpRegistryService的listMcpServerByNamespaceList方法中，remindOffset的计算可能不准确。当剩余limit不足时，可能错误地重置偏移量，导致数据跳跃或重复。
📌 关键代码：
if (result.getPageItems().size() >= limit) {
namespaceResult = listMcpServerByNamespace(...);
} else {
remindOffset -= namespaceResult.getTotalCount();
}💡 小贴士
与 lingma-agents 交流的方式
📜 直接回复评论
直接回复本条评论，lingma-agents 将自动处理您的请求。例如：
-
在当前代码中添加详细的注释说明。
-
请详细介绍一下你说的 LRU 改造方案，并使用伪代码加以说明。
📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如：
-
@Lingma-Agent 分析这个方法的性能瓶颈并提供优化建议。
-
@Lingma-Agent 对这个方法生成优化代码。
📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如：
-
@Lingma-Agent 请总结上述讨论并提出解决方案。
-
@Lingma-Agent 请根据讨论内容生成优化代码。
| Page<ConfigInfo> serverInfos = searchMcpServers(namespaceId, name, search, limit); | ||
| List<McpServerIndexData> indexDataList = serverInfos.getPageItems().stream().skip(offset) | ||
| .map(this::mapMcpServerVersionConfigToIndexData).toList(); | ||
| Page<McpServerIndexData> result = new Page<>(); | ||
| result.setPageItems(indexDataList); | ||
| result.setTotalCount(serverInfos.getTotalCount()); |
There was a problem hiding this comment.
修复分页逻辑错误，确保跨命名空间搜索时正确计算总数和分页结果
🟠 Critical | 🐞 Bugs
📋 问题详情
在searchMcpServerByName0方法中，当namespaceId为空时，新代码仅通过单个命名空间搜索获取总数，而未聚合所有命名空间的结果。这会导致跨命名空间搜索时总数统计错误，分页结果不完整。原逻辑通过遍历所有命名空间并累加统计，新代码未保留该逻辑，存在分页功能失效风险。
💡 解决方案
需要恢复跨命名空间的总数统计和分页逻辑：
- Page<ConfigInfo> serverInfos = searchMcpServers(namespaceId, name, search, limit);
+ Page<ConfigInfo> aggregatedServerInfos = new Page<>();
+ if (StringUtils.isEmpty(namespaceId)) {
+ List<String> namespaces = fetchOrderedNamespaceList();
+ for (String ns : namespaces) {
+ Page<ConfigInfo> nsPage = searchMcpServers(ns, name, search, limit);
+ aggregatedServerInfos.getPageItems().addAll(nsPage.getPageItems());
+ aggregatedServerInfos.setTotalCount(aggregatedServerInfos.getTotalCount() + nsPage.getTotalCount());
+ }
+ } else {
+ aggregatedServerInfos = searchMcpServers(namespaceId, name, search, limit);
+ }
+ List<McpServerIndexData> indexDataList = aggregatedServerInfos.getPageItems().stream()
+ .skip(offset).limit(limit)
+ .map(this::mapMcpServerVersionConfigToIndexData).toList();这样能正确聚合所有命名空间的结果并计算总数。
您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
| @@ -244,7 +234,8 @@ void getMcpServerDetailByIdFoundHttpTypeWithoutToolsWithoutNamespace() throws Na | |||
| instance.setMetadata(Collections.singletonMap(Constants.META_PATH, "/nacos")); | |||
| when(endpointOperationService.getMcpServerEndpointInstances(any(McpServiceRef.class))).thenReturn( | |||
| Collections.singletonList(instance)); | |||
| McpServerDetailInfo actual = serverOperationService.getMcpServerDetail("", id, null, null); | |||
| McpServerDetailInfo actual = serverOperationService.getMcpServerDetail(AiConstants.Mcp.MCP_DEFAULT_NAMESPACE, | |||
| id, null, null); | |||
There was a problem hiding this comment.
测试依赖mock未正确设置可能导致测试不通过
🟡 Major | 🧹 Code Smells
📋 问题详情
多个测试方法（如updateMcpServerByIdWithOldSpec）删除了对mcpServerIndex.getMcpServerById的mock，可能导致测试无法获取预期的mock数据，需恢复必要的mock设置以确保测试稳定性。
💡 解决方案
在相关测试中恢复mcpServerIndex的mock设置：
+ when(mcpServerIndex.getMcpServerById(id)).thenReturn(mockIndexData(id).getPageItems().get(0));您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
| when(mcpServerOperationService.listMcpServerWithPage(AiConstants.Mcp.MCP_DEFAULT_NAMESPACE, "test", | ||
| Constants.MCP_LIST_SEARCH_ACCURATE, 1, 100)).thenReturn(mockPage); |
There was a problem hiding this comment.
命名空间参数使用硬编码字符串应替换为常量
🟢 Minor | 🧹 Code Smells
📋 问题详情
测试代码中直接使用空字符串作为命名空间参数，应统一使用AiConstants.Mcp.MCP_DEFAULT_NAMESPACE常量以提高代码标准化和可维护性。
💡 解决方案
确保所有命名空间参数使用常量替代硬编码：
- when(mcpServerOperationService.listMcpServerWithPage("", "test", ...))
+ when(mcpServerOperationService.listMcpServerWithPage(AiConstants.Mcp.MCP_DEFAULT_NAMESPACE, "test", ...))您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
| @@ -80,6 +80,7 @@ public McpRegistryServerList listMcpServers(ListServerForm listServerForm) throw | |||
| @GetMapping(value = "/v0/servers/{id}") | |||
| public Object getServer(@PathVariable String id, GetServerForm getServerForm, HttpServletResponse response) | |||
| throws NacosException { | |||
| getServerForm.validate(); | |||
There was a problem hiding this comment.
表单验证未处理异常可能导致未处理的异常
🟡 Major | 🐞 Bugs
📋 问题详情
新增getServerForm.validate()调用但未捕获可能的NacosApiException，需在控制器中处理异常并返回错误响应。
💡 解决方案
添加异常处理并返回错误响应：
+ try {
+ getServerForm.validate();
+ } catch (NacosApiException e) {
+ response.setStatus(400);
+ return new McpErrorResponse(e.getMessage());
+ }您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
| if (Objects.isNull(mcpServerDetail)) { | ||
| public McpRegistryServerDetail getServer(String id, GetServerForm getServerForm) throws NacosException { | ||
| McpServerIndexData indexData = mcpServerIndex.getMcpServerById(id); | ||
| if (Objects.isNull(indexData)) { |
There was a problem hiding this comment.
统一异常抛出类型
🟡 Major | 🐞 Bugs
📋 问题详情
在updateMcpServer方法中，当indexData为null时抛出NacosApiException，但getServer方法直接返回null。应统一异常处理策略，避免部分路径返回null导致后续调用出现空指针。
💡 解决方案
修改getServer方法的null返回为抛出异常：
@@ -157,6 +157,7 @@
if (Objects.isNull(indexData)) {
- return null;
+ throw new NacosApiException(NacosApiException.NOT_FOUND, ErrorCode.RESOURCE_NOT_FOUND, String.format("mcp server `%s` not found", id));
}您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
| void getMcpServerToolsInfo() throws Exception { | ||
| String id = UUID.randomUUID().toString(); |
There was a problem hiding this comment.
规范测试用例断言方式
🟢 Minor | 🧹 Code Smells
📋 问题详情
部分测试用例直接断言JSON字符串内容（如assertEquals(mapper.writeValueAsString(...)...），当对象字段顺序变化时可能导致断言失败。应使用对象比较替代字符串比较。
💡 解决方案
修改断言方式为对象比较：
@@ -149,3 +149,5 @@
+ McpToolSpecification expected = new McpToolSpecification();
+ McpToolSpecification actual = mapper.readValue(response.getContentAsString(), McpToolSpecification.class);
+ assertEquals(expected, actual);您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
| } | ||
|
|
||
| private McpServerBasicInfo mockMcpServerBasicInfo(int number, String namespaceId) { | ||
| String id = UUID.randomUUID().toString(); |
There was a problem hiding this comment.
|
|
Please do not create a Pull Request without creating an issue first.
What is the purpose of the change
XXXXX
Brief changelog
XX
Verifying this change
XXXX
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.