fix : fix credential process logic for nacos mcp util and add ut for it #2394
Conversation
Erica177
requested review from
2456868764,
NameHaibinZhang and
johnlanni
as code owners
修复Nacos MCP凭证处理逻辑并添加单元测试变更文件
时序图sequenceDiagram
participant NacosClient as NacosRegistryClient
participant Cache as MemoryCache
participant Listener as ConfigListener
NacosClient->>Cache: GetAllConfigs()
Cache->>Cache: Check pluginConfig.Rules length
Cache-->>NacosClient: Return empty map if no rules
NacosClient->>Listener: replaceTemplateAndExactConfigsItems
Listener->>NacosClient: Collect credentialsNeedDelete
NacosClient->>Listener: Trigger config update
Listener->>NacosClient: Update credentials map
NacosClient->>NacosClient: refreshServiceListenerIfNeeded
💡 小贴士与 lingma-agents 交流的方式📜 直接回复评论
📜 在代码行处标记
📜 在讨论中提问
|
![lingma-agents[bot]](https://avatars.githubusercontent.com/in/1198481?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 代码评审报告
📋 评审意见详情
💡 单文件建议
✅ 未发现需要特别关注的代码问题。
🚀 跨文件建议
以下是对代码架构和设计的综合分析，聚焦于跨文件交互、系统一致性和潜在优化空间。
🔍 1. 潜在的资源泄漏风险：未正确清理配置监听器可能导致内存泄漏
在文件registry/nacos/mcpserver/client.go的resetNacosTemplateConfigs方法中，新增的credentialsNeedDelete列表用于记录需要删除的凭证密钥，但仅删除了ctx.configsMap中的条目，却未处理对应的监听器资源。原代码在删除配置时虽然调用了n.cancelListenToConfig(wrap)，但未确保监听器的完全释放，可能导致残留的监听器继续占用资源或触发不必要的回调。此外，新配置添加时直接覆盖了ctx.configsMap，但未检查是否已存在相同密钥的监听器，可能导致重复注册监听器，增加内存消耗和潜在竞争条件。
📌 关键代码：
for key, wrap := range ctx.configsMap {
if strings.HasPrefix(key, CredentialPrefix) {
if _, ok := newCredentials[key]; !ok {
credentialsNeedDelete = append(credentialsNeedDelete, key)
err := n.cancelListenToConfig(wrap)
// 未处理err可能导致未释放资源
}
}
}
// 延迟删除操作可能导致监听器未及时释放🔍 2. 递归调用风险：配置变更触发无限循环
在registry/nacos/mcpserver/client.go的configListener函数中，当group为McpToolSpecGroup时会调用resetNacosTemplateConfigs，该方法会重新注册新的配置监听器。若新配置再次触发相同分组的监听事件，可能导致无限递归调用，最终引发堆栈溢出。例如，当修改工具模板时重新初始化监听器，可能再次触发同一类型事件。
📌 关键代码：
if group == McpToolSpecGroup {
n.resetNacosTemplateConfigs(ctx, &wrap)
} else if group == McpServerSpecGroup {
n.refreshServiceListenerIfNeeded(ctx, data)
}🔍 3. 测试覆盖不完整：Mock对象未实现关键方法可能导致测试失效
新增的测试文件registry/nacos/mcpserver/client_test.go中定义的MockedNacosConfigClient和MockedNacosNamingClient存在大量未实现的方法（如PublishConfig、RegisterInstance等），这些方法直接panic。如果实际测试场景中调用了这些未实现的方法（例如通过间接路径），会导致测试崩溃而非失败，从而掩盖真实问题。应确保所有可能被调用的方法均有合理mock实现。
📌 关键代码：
func (m MockedNacosConfigClient) PublishConfig(param vo.ConfigParam) (bool, error) {
panic("implement me")
}🔍 4. 凭证处理逻辑不一致：敏感数据日志记录风险
在registry/memory/cache.go的GetAllConfigs方法新增日志log.Infof("there is no mcp server rule exist, skip generate wasm plugin")，但未对可能包含在pluginConfig中的敏感数据（如凭证信息）进行过滤。若pluginConfig包含未过滤的日志输出，可能导致敏感信息泄露。需确保日志记录不包含敏感数据或使用安全日志级别。
📌 关键代码：
log.Infof("there is no mcp server rule exist, skip generate wasm plugin")
return map[string]*config.Config{}🔍 5. 分页处理性能隐患：大数据量下可能引发内存问题
测试用例TestNacosRegistryClient_ListMcpServer模拟了151条配置项的分页查询，但SearchConfig方法在MockedNacosConfigClient中将所有匹配配置加载到内存切片result中。当实际数据量极大时，此实现可能导致内存占用过高。应考虑分页流式处理或优化内存使用策略，避免单次查询加载全部数据。
📌 关键代码：
for key, value := range m.configs {
// 将所有匹配项加载到内存切片
result = append(result, model.ConfigItem{...})
}💡 小贴士
与 lingma-agents 交流的方式
📜 直接回复评论
直接回复本条评论，lingma-agents 将自动处理您的请求。例如：
-
在当前代码中添加详细的注释说明。
-
请详细介绍一下你说的 LRU 改造方案，并使用伪代码加以说明。
📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如：
-
@Lingma-Agent 分析这个方法的性能瓶颈并提供优化建议。
-
@Lingma-Agent 对这个方法生成优化代码。
📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如：
-
@Lingma-Agent 请总结上述讨论并提出解决方案。
-
@Lingma-Agent 请根据讨论内容生成优化代码。
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2394 +/- ##
===========================================
+ Coverage 35.91% 45.97% +10.06%
===========================================
Files 69 81 +12
Lines 11576 13018 +1442
===========================================
+ Hits 4157 5985 +1828
+ Misses 7104 6687 -417
- Partials 315 346 +31 🚀 New features to boost your workflow:
|
3 participants
Ⅰ. Describe what this PR did
Ⅱ. Does this pull request fix one issue?
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews