[ISSUE#2259] support db2mcp for redis#2399
[ISSUE#2259] support db2mcp for redis#2399undertaker86001 wants to merge 3 commits intohigress-group:mainfrom
Conversation
undertaker86001
requested review from
2456868764,
CH3CHO,
Xunzhuo,
johnlanni and
rinfx
as code owners
|
undertaker86001 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
新增Redis MCP服务器支持及端到端测试变更概述新功能
测试更新
配置调整
变更统计
变更文件
💡 小贴士与 lingma-agents 交流的方式📜 直接回复评论
📜 在代码行处标记
📜 在讨论中提问
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![lingma-agents[bot]](https://avatars.githubusercontent.com/in/1198481?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔎 代码评审报告
🎯 评审意见概览
| 严重度 | 数量 | 说明 |
|---|---|---|
| 🔴 Blocker | 1 | 阻断性问题，需立即修复。例如：系统崩溃、关键功能不可用或严重安全漏洞。 |
| 🟠 Critical | 2 | 严重问题，高优先级修复。例如：核心功能异常或性能瓶颈影响用户体验。 |
| 🟡 Major | 3 | 主要问题，建议修复。例如：非核心功能缺陷或代码维护性较差。 |
| 🟢 Minor | 0 | 次要问题，酬情优化。例如：代码格式不规范或注释缺失。 |
总计: 6 个问题
📋 评审意见详情
💡 代码实现建议
以下是文件级别的代码建议，聚焦于代码的可读性、可维护性和潜在问题。
🔹 plugins/golang-filter/mcp-server/servers/redis/server.go (3 💬)
- 避免在日志中记录敏感配置信息 (L63)
- 添加Set命令的过期时间合法性校验 (L309-L312)
- 修复HandleTTLTool函数中未处理键不存在的情况 (L427-L462)
🔹 plugins/golang-filter/mcp-session/common/redis.go (1 💬)
- 缺失的Redis客户端库导入导致编译错误 (L280-L323)
🔹 test/e2e/conformance/tests/go-wasm-ai-security-guard.go (1 💬)
- 无效的JSON响应体格式 (L106)
🔹 test/e2e/conformance/tests/go-wasm-redis-mcpserver.go (1 💬)
- TTL响应验证不够严格 (L87-L90)
🚀 架构设计建议
以下是对代码架构和设计的综合分析，聚焦于跨文件交互、系统一致性和潜在优化空间。
🔍1. 代码重复问题：多个工具处理函数存在相似结构
在新增的redis/server.go文件中，多个工具处理函数（如HandleGetTool、HandleSetTool等）的结构高度相似，均包含参数解析、错误处理和客户端调用的流程。这种重复代码增加了维护成本，当需要修改通用逻辑时（如错误处理方式），需逐个修改所有函数。建议将公共逻辑抽象为工具辅助函数或基类，提升代码复用性和可维护性。
📌 关键代码
func HandleGetTool(client *common.RedisClient) common.ToolHandlerFunc {
return func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) {
params := struct {
Key string `json:"key"`
}{}
if err := request.ParseParams(¶ms); err != nil {
return nil, fmt.Errorf("invalid parameters: %w", err)
}
// 具体逻辑
}
}func HandleSetTool(client *common.RedisClient) common.ToolHandlerFunc {
return func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) {
params := struct {
Key string `json:"key"`
Value string `json:"value"`
Expiration int `json:"expiration,omitempty"`
}()
if err := request.ParseParams(¶ms); err != nil {
return nil, fmt.Errorf("invalid parameters: %w", err)
}
// 具体逻辑
}
}重复代码导致维护成本上升，修改逻辑时容易遗漏部分函数，引发不一致问题。
🔍2. 测试覆盖不足：未包含关键错误场景验证
新增的测试文件test/e2e/conformance/tests/go-wasm-redis-mcpserver.go中，测试用例主要验证了正常操作（如Set/Get/Del），但缺少对以下错误场景的覆盖：
- 配置缺失或无效（如未指定Redis地址）
- 连接失败（如Redis服务不可达）
- 密钥不存在时的异常处理（如TTL对不存在的key）
- 参数校验失败（如Set命令未提供value）
这些场景未被覆盖可能导致实际使用中出现未捕获的异常。
📌 关键代码
// 测试用例仅包含正常操作和部分TTL验证
testcases := []http.Assertion{
// Set、Get、Del等正常操作
// TTL测试仅检查响应长度
}未覆盖的错误场景可能导致系统在异常情况下崩溃或返回错误结果。
🔍3. 安全风险：Keys命令可能引发数据暴露
Redis的Keys工具允许通过通配符匹配所有键（如Keys "*"），这在生产环境中可能导致敏感数据暴露或性能问题。虽然Keys命令本身是Redis原生功能，但作为MCP Server的接口暴露给外部调用时，需增加安全限制：
- 限制匹配模式的复杂度（如禁止"*"）
- 添加调用频率限制
- 在文档中明确警告该命令的潜在风险
📌 关键代码
// KeysSchema允许任意pattern输入
func KeysSchema() string {
return `{ "type": "object", "properties": {"pattern": {"type": "string"}}, "required": ["pattern"] }`
}恶意调用Keys命令可能暴露系统中所有Redis键，导致敏感数据泄露或服务性能下降。
🔍4. Redis客户端创建方式可能导致资源浪费
在RedisConfig.NewServer函数中，每次调用都会创建新的Redis客户端实例：
redisClient, err := common.NewRedisClient(redisConfig)若该客户端未被复用（如单例模式），可能导致重复连接和资源浪费。需确认common.RedisClient是否实现了连接池或单例管理。
📌 关键代码
redisClient, err := common.NewRedisClient(redisConfig)
if err != nil {
return nil, fmt.Errorf("failed to create Redis client: %w", err)
}频繁创建客户端可能增加连接开销，影响系统性能和稳定性。
🔍5. 依赖管理问题：Redis客户端库缺失导入
在修改的common/redis.go文件中，删除了Redis客户端库的导入（github.com/go-redis/redis/v8），可能导致编译失败。虽然用户已标注该文件存在"缺失的Redis客户端库导入"问题，需确认是否已通过其他方式解决依赖问题。
📌 关键代码
// 缺少redis/v8导入
import (
"time"
"github.com/envoyproxy/envoy/contrib/golang/common/go/api"
)缺失依赖将导致代码无法编译，影响部署。
审查详情
📒 文件清单 （6 个文件）
✅ 新增: 4 个文件
📝 变更: 2 个文件
✅ 新增文件:
plugins/golang-filter/mcp-server/servers/redis/server.gotest/e2e/conformance/tests/go-wasm-ai-security-guard.gotest/e2e/conformance/tests/go-wasm-redis-mcpserver.gotest/e2e/conformance/tests/go-wasm-redis-mcpserver.yaml
📝 变更文件:
plugins/golang-filter/mcp-server/config.goplugins/golang-filter/mcp-session/common/redis.go
💡 小贴士
与 lingma-agents 交流的方式
📜 直接回复评论
直接回复本条评论，lingma-agents 将自动处理您的请求。例如：
-
在当前代码中添加详细的注释说明。
-
请详细介绍一下你说的 LRU 改造方案，并使用伪代码加以说明。
📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如：
-
@lingma-agents 分析这个方法的性能瓶颈并提供优化建议。
-
@lingma-agents 对这个方法生成优化代码。
📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如：
-
@lingma-agents 请总结上述讨论并提出解决方案。
-
@lingma-agents 请根据讨论内容生成优化代码。
|
|
||
| } | ||
|
|
||
| api.LogDebugf("RedisConfig ParseConfig: %+v", config) |
There was a problem hiding this comment.
避免在日志中记录敏感配置信息
🟠 Critical | 🔒 Security Hotspots
📋 问题详情
ParseConfig函数的日志记录了整个配置对象（包括可能包含密码的字段），存在敏感信息泄露风险。
💡 解决方案
添加过滤敏感字段的日志处理函数：
+func filterConfig(config map[string]any) map[string]any {
+ filtered := make(map[string]any)
+ for k, v := range config {
+ if k == "password" || k == "secret" {
+ filtered[k] = "******"
+ } else {
+ filtered[k] = v
+ }
+ }
+ return filtered
+}
-
-api.LogDebugf("RedisConfig ParseConfig: %+v", config)
+api.LogDebugf("RedisConfig ParseConfig: %+v", filterConfig(config))您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
| exp := time.Duration(0) | ||
| if params.Expiration > 0 { | ||
| exp = time.Duration(params.Expiration) * time.Second | ||
|
|
There was a problem hiding this comment.
添加Set命令的过期时间合法性校验
🟡 Major | 🐞 Bugs
📋 问题详情
HandleSetTool函数未验证expiration参数是否为非负值，可能导致Redis设置无效的过期时间。
💡 解决方案
添加过期时间非负校验：
+if params.Expiration < 0 {
+ return nil, fmt.Errorf("invalid parameters: expiration must be non-negative")
+}
-if params.Expiration > 0 {
- exp = time.Duration(params.Expiration) * time.Second
-}
+exp := time.Duration(params.Expiration) * time.Second您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
| func HandleTTLTool(client *common.RedisClient) common.ToolHandlerFunc { | ||
| return func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { | ||
| params := struct { | ||
| Key string `json:"key"` | ||
| }{} | ||
|
|
||
| if err := request.ParseParams(¶ms); err != nil { | ||
| return nil, fmt.Errorf("invalid parameters: %w", err) | ||
| } | ||
| ttl, err := client.TTL(params.Key) | ||
| if err != nil { | ||
| return &mcp.CallToolResult{ | ||
| Output: fmt.Sprintf("Error getting TTL: %v", err), | ||
| }, nil | ||
| } | ||
|
|
||
| if ttl < 0 { | ||
| if ttl == -1 { | ||
| return &mcp.CallToolResult{ | ||
| Output: "Key exists but has no expiration", | ||
| }, nil | ||
| } else { | ||
| return &mcp.CallToolResult{ | ||
| Output: "Key does not exist", | ||
| }, nil | ||
| } | ||
|
|
||
| } | ||
|
|
||
| return &mcp.CallToolResult{ | ||
| Output: fmt.Sprintf("%d", int(ttl.Seconds())), | ||
| }, nil | ||
| } | ||
| } | ||
|
|
||
| // Publish工具处理函数 |
There was a problem hiding this comment.
修复HandleTTLTool函数中未处理键不存在的情况
🟡 Major | 🐞 Bugs
📋 问题详情
在HandleTTLTool函数中，当Redis键不存在时（TTL返回-2），当前逻辑仅处理了-1（无过期时间）和非负值（有剩余时间）情况，导致键不存在时返回错误信息不准确。
💡 解决方案
修改HandleTTLTool函数的TTL判断逻辑，明确处理-2（键不存在）的情况：
- if ttl < 0 {
- if ttl == -1 {
- return &mcp.CallToolResult{Output: "Key exists but has no expiration"}, nil
- } else {
- return &mcp.CallToolResult{Output: "Key does not exist"}, nil
- }
- }
+ switch {
+ case ttl == -1:
+ return &mcp.CallToolResult{Output: "Key exists but has no expiration"}, nil
+ case ttl == -2:
+ return &mcp.CallToolResult{Output: "Key does not exist"}, nil
+ default:
+ // 正常处理剩余时间
+ }您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
| // Del 删除一个或多个key | ||
| func (r *RedisClient) Del(keys ...string) (int64, error) { | ||
| result, err := r.client.Del(r.ctx, keys...).Result() | ||
| if err != nil { | ||
| return 0, fmt.Errorf("failed to delete keys: %w", err) | ||
| } | ||
| return result, nil | ||
| } | ||
|
|
||
| // Keys 返回匹配指定模式的所有key | ||
| func (r *RedisClient) Keys(pattern string) ([]string, error) { | ||
| result, err := r.client.Keys(r.ctx, pattern).Result() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get keys: %w", err) | ||
| } | ||
| return result, nil | ||
| } | ||
|
|
||
| // Exists 检查key是否存在 | ||
| func (r *RedisClient) Exists(keys ...string) (int64, error) { | ||
| result, err := r.client.Exists(r.ctx, keys...).Result() | ||
| if err != nil { | ||
| return 0, fmt.Errorf("failed to check keys: %w", err) | ||
| } | ||
| return result, nil | ||
| } | ||
|
|
||
| // Expire 设置key的过期时间 | ||
| func (r *RedisClient) Expire(key string, expiration time.Duration) (bool, error) { | ||
| result, err := r.client.Expire(r.ctx, key, expiration).Result() | ||
| if err != nil { | ||
| return false, fmt.Errorf("failed to set expiration: %w", err) | ||
| } | ||
| return result, nil | ||
| } | ||
|
|
||
| // TTL 返回key的剩余生存时间 | ||
| func (r *RedisClient) TTL(key string) (time.Duration, error) { | ||
| result, err := r.client.TTL(r.ctx, key).Result() | ||
| if err != nil { | ||
| return 0, fmt.Errorf("failed to get TTL: %w", err) | ||
| } | ||
| return result, nil | ||
| } |
There was a problem hiding this comment.
缺失的Redis客户端库导入导致编译错误
🔴 Blocker | 🐞 Bugs
📋 问题详情
删除了github.com/go-redis/redis/v8的导入语句，但新增的Redis方法如Del/Keys/Expire依赖该包。这会导致编译失败，因为r.client的类型无法识别，且缺少Redis客户端方法的定义。
💡 解决方案
需要恢复github.com/go-redis/redis/v8的导入以保证客户端方法可用：
@@ -6,7 +6,7 @@
-import (
+import (
+ "github.com/go-redis/redis/v8"
"time"
"github.com/envoyproxy/envoy/contrib/golang/common/go/api"
)您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
| }}, Response: http.AssertionResponse{ExpectedResponse: http.Response{ | ||
| StatusCode: 200, | ||
| ContentType: http.ContentTypeApplicationJson, | ||
| Body: []byte(`"很抱歉，我无法回答您的问题"`), |
There was a problem hiding this comment.
| JsonBodyMatcher: func(t *testing.T, body []byte) bool { | ||
| // TTL值会不断变化，所以我们只检查是否能获取到TTL，不检查具体值 | ||
| return len(body) > 0 | ||
| }}}}, {Meta: http.AssertionMeta{ |
There was a problem hiding this comment.
TTL响应验证不够严格
🟡 Major | 🧹 Code Smells
📋 问题详情
TTL测试用例仅检查响应长度>0，未验证JSON结构。可能导致非预期格式的响应被误认为成功，建议添加结构校验。
💡 解决方案
添加结构化验证：
-return len(body) > 0
+var resp struct{ Result struct{ Output int64 } }
+return json.Unmarshal(body, &resp) == nil && resp.Result.Output >= 0您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
|
@undertaker86001 Please add the license head. And the commit user information is also problematic, the CLA check has not passed. |
Sorry, I just saw this, I will change it soon |
5 participants
Ⅰ. Describe what this PR did
Related to ISSUE#2259
Ⅱ. 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