-
Notifications
You must be signed in to change notification settings - Fork 902
feat: add DB MCP Server execute, list tables, describe table tools #2506
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add DB MCP Server execute, list tables, describe table tools #2506
Conversation
Signed-off-by: hongzhouzi <[email protected]>
添加数据库MCP服务器的执行、列表和表结构查询工具变更概述新功能
重构
性能优化
变更文件
时序图sequenceDiagram
participant MCPServer
participant DBClient
participant ToolHandler
MCPServer->>ToolHandler: 调用execute工具
ToolHandler->>DBClient: 执行SQL语句
DBClient->>DBClient: reconnectIfDbEmpty()
DBClient->>DBClient: Execute(sql)
DBClient-->>ToolHandler: 返回影响行数
ToolHandler->>MCPServer: 返回JSON结果
MCPServer->>ToolHandler: 调用describe table
ToolHandler->>DBClient: DescribeTable(table_name)
DBClient->>DBClient: 生成方言SQL
DBClient-->>ToolHandler: 返回列结构数据
ToolHandler-->>MCPServer: 结果序列化
💡 小贴士与 lingma-agents 交流的方式📜 直接回复评论
📜 在代码行处标记
📜 在讨论中提问
Change OverviewNew Features
Refactor
Performance Optimization
Change file
Sequence chartsequenceDiagram
participant MCPServer
participant DBClient
participant ToolHandler
MCPServer->>ToolHandler: Call execute tool
ToolHandler->>DBClient: Execute SQL statements
DBClient->>DBClient: reconnectIfDbEmpty()
DBClient->>DBClient: Execute(sql)
DBClient-->>ToolHandler: Returns the number of rows affected
ToolHandler->>MCPServer: Return JSON results
MCPServer->>ToolHandler: Call describe table
ToolHandler->>DBClient: DescribeTable(table_name)
DBClient->>DBClient: Generate dialect SQL
DBClient-->>ToolHandler: Return column structure data
ToolHandler-->>MCPServer: Result Serialization
💡 TipsHow to communicate with lingma-agents📜 Reply to comments directly
**📜 Mark ** at line of code
📜 Ask a question during discussion
|
lexburner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
@hongzhouzi Please sign the CLA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔎 代码评审报告
🎯 评审意见概览
| 严重度 | 数量 | 说明 |
|---|---|---|
| 🔴 Blocker | 0 | 阻断性问题,需立即修复。例如:系统崩溃、关键功能不可用或严重安全漏洞。 |
| 🟠 Critical | 1 | 严重问题,高优先级修复。例如:核心功能异常或性能瓶颈影响用户体验。 |
| 🟡 Major | 3 | 主要问题,建议修复。例如:非核心功能缺陷或代码维护性较差。 |
| 🟢 Minor | 1 | 次要问题,酬情优化。例如:代码格式不规范或注释缺失。 |
总计: 5 个问题
📋 评审意见详情
💡 代码实现建议
以下是文件级别的代码建议,聚焦于代码的可读性、可维护性和潜在问题。
🔹 plugins/golang-filter/mcp-server/servers/gorm/db.go (3 💬)
- 规范化数据库类型常量 (L143-L144)
- 避免SQL注入风险,使用参数化查询代替字符串拼接 (L145-L155)
- 提取公共错误处理逻辑 (L260-L265)
🔹 plugins/golang-filter/mcp-server/servers/gorm/tools.go (2 💬)
- 统一错误信息并明确操作类型 (L39)
- 统一工具Schema生成方式 (L111-L123)
🚀 架构设计建议
以下是对代码架构和设计的综合分析,聚焦于跨文件交互、系统一致性和潜在优化空间。
🔍1. 跨数据库SQL语句硬编码导致扩展性问题
新增的DescribeTable、ListTables等方法通过switch case硬编码不同数据库的SQL语句,违反了开闭原则。当需要支持新数据库类型时需要修改多个文件的内部实现,不符合架构的扩展性设计。建议将不同数据库的查询逻辑抽象为接口实现,通过工厂模式创建对应数据库的适配器。
📌 关键代码
switch c.dbType {
case "mysql":
sql = fmt.Sprintf(`...`)
case "postgres":
sql = fmt.Sprintf(`...`)
}新增数据库类型时需要修改多个文件,维护成本高且容易引入不一致性。
🔍2. 工具错误信息不明确影响调试
所有数据库操作工具的错误信息统一显示"failed to execute SQL query",无法区分具体失败原因(如连接失败、语法错误等)。建议在错误信息中包含具体操作类型和数据库类型,例如"Execute SQL failed on MySQL: %w"。
📌 关键代码
return nil, fmt.Errorf("failed to execute SQL query: %w", err)运维人员难以快速定位问题根源,增加故障排查时间。
🔍3. 重连机制可能引发竞态条件
reconnectIfDbEmpty方法在多个操作中触发通道发送,但未对通道容量进行限制。当连续多次触发重连时可能导致资源竞争,建议添加重试次数限制或使用更可靠的连接管理策略。
📌 关键代码
select {
case c.reconnect <- struct{}{}:
default:
}高并发场景下可能引发连接频繁重建或资源泄漏。
🔍4. 工具返回结果格式未统一
不同工具的返回结果类型不一致(如ListTables返回[]string而DescribeTable返回[]map),但Schema生成方式未做统一处理。建议建立统一的响应结构规范,确保所有工具返回结果符合预定义的JSON格式要求。
📌 关键代码
func buildCallToolResult(results any) ...客户端可能因格式差异需要额外处理逻辑,降低工具的易用性。
🔍5. 数据库类型常量维护分散
数据库类型常量在多个地方(如db.go的switch和tools.go的schema描述)重复定义,存在维护不一致的风险。建议将数据库类型常量集中定义并统一管理。
📌 关键代码
-
plugins/golang-filter/mcp-server/servers/gorm/db.go(143-208) -
plugins/golang-filter/mcp-server/servers/gorm/server.go(49-53)
新增数据库类型时需要修改多处代码,容易遗漏导致不兼容问题。
审查详情
📒 文件清单 (3 个文件)
📝 变更: 3 个文件
📝 变更文件:
plugins/golang-filter/mcp-server/servers/gorm/db.goplugins/golang-filter/mcp-server/servers/gorm/server.goplugins/golang-filter/mcp-server/servers/gorm/tools.go
💡 小贴士
与 lingma-agents 交流的方式
📜 直接回复评论
直接回复本条评论,lingma-agents 将自动处理您的请求。例如:
-
在当前代码中添加详细的注释说明。
-
请详细介绍一下你说的 LRU 改造方案,并使用伪代码加以说明。
📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如:
-
@lingma-agents 分析这个方法的性能瓶颈并提供优化建议。
-
@lingma-agents 对这个方法生成优化代码。
📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如:
-
@lingma-agents 请总结上述讨论并提出解决方案。
-
@lingma-agents 请根据讨论内容生成优化代码。
| return nil, fmt.Errorf("invalid message argument") | ||
| } | ||
|
|
||
| results, err := dbClient.Execute(message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
统一错误信息并明确操作类型
🟢 Minor | 🧹 Code Smells
📋 问题详情
多个工具处理函数中使用统一的错误信息'failed to execute SQL query',无法区分具体操作类型(如查询/执行/列表),导致调试困难。
💡 解决方案
根据操作类型修改错误信息:
- return nil, fmt.Errorf("failed to execute SQL query: %w", err)
+ return nil, fmt.Errorf("failed to execute SQL %s: %w", toolName, err)需在各工具中传递操作类型参数:
您的反馈对我们很重要!(建议右键在新标签页中打开以下链接)
| func GetExecuteToolSchema() json.RawMessage { | ||
| return json.RawMessage(` | ||
| { | ||
| "type": "object", | ||
| "properties": { | ||
| "sql": { | ||
| "type": "string", | ||
| "description": "The sql to execute" | ||
| } | ||
| } | ||
| } | ||
| `) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
统一工具Schema生成方式
🟡 Major | 🧹 Code Smells
📋 问题详情
多个Schema定义重复相似的JSON结构,应提取公共生成函数减少冗余。
💡 解决方案
创建Schema生成工具:
+ func generateSchema(propertyName, description string) json.RawMessage {
+ return json.RawMessage(fmt.Sprintf(`{
+ "type": "object",
+ "properties": {
+ "%s": {
+ "type": "string",
+ "description": "%s"
+ }
+ }
+ }`, propertyName, description))
+ }调用方式:
- 原代码
+ return generateSchema("sql", "The sql to execute")您的反馈对我们很重要!(建议右键在新标签页中打开以下链接)
Signed-off-by: hongzhouzi <[email protected]>
| var sql string | ||
| var args []string | ||
| switch c.dbType { | ||
| case MYSQL: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
感觉这些是不是应该做一些抽象和封装,每个地方都单独 switch..case 不太好吧?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
可以单独搞个 pr,目前沿用的上个版本的设计,这个 pr 仅仅在这个基础上添加了一些 tool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
感觉这些是不是应该做一些抽象和封装,每个地方都单独 switch..case 不太好吧?
可以的,这儿我单独再提个 PR 用工厂模式和策略模式抽象优化一下。
johnlanni
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2506 +/- ##
===========================================
+ Coverage 35.91% 46.02% +10.11%
===========================================
Files 69 81 +12
Lines 11576 13020 +1444
===========================================
+ Hits 4157 5993 +1836
+ Misses 7104 6681 -423
- Partials 315 346 +31 🚀 New features to boost your workflow:
|
…libaba#2506) Signed-off-by: hongzhouzi <[email protected]>
…libaba#2506) Signed-off-by: hongzhouzi <[email protected]>
Ⅰ. Describe what this PR did
Support more DB MCP server atomic capability tools, add execute, list tables, describe table tools.
Ⅱ. Does this pull request fix one issue?
fixes #2247
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews