refactor(torii-server): mcp and sql cleanup with instructions static file #2790
Conversation
WalkthroughOhayo! This pull request introduces significant refactoring in the Changes
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
crates/torii/server/static/mcp-instructions.txt (2)
7-7: Typographical Correction: Duplicate WordOhayo sensei! In line 7, there is a duplicated word "data":
- dynamic model tables: Stores model data data associated with entitiesPlease remove the extra word:
- dynamic model tables: Stores model data associated with entities🧰 Tools
🪛 LanguageTool
[duplication] ~7-~7: Possible typo: you repeated a word
Context: ...te - dynamic model tables: Stores model data data associated with entities - models: Cont...(ENGLISH_WORD_REPEAT_RULE)
14-14: Grammar Suggestion: Insert Comma for ClarityOhayo sensei! In line 14, consider inserting a comma after "schema" to improve readability:
Original:
You should always retrieve the schema if unsure about how to query the database.Revised:
You should always retrieve the schema, if unsure about how to query the database.🧰 Tools
🪛 LanguageTool
[formatting] ~14-~14: Consider inserting a comma here, unless the first half is essential to the meaning of the sentence.
Context: ...out how to query the database. With the schema you can then associate entities with mo...(WITH_THAT_COMMA)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
crates/torii/server/src/handlers/mcp.rs(5 hunks)crates/torii/server/src/handlers/sql.rs(2 hunks)crates/torii/server/static/mcp-instructions.txt(1 hunks)
🧰 Additional context used
🪛 LanguageTool
crates/torii/server/static/mcp-instructions.txt
[duplication] ~7-~7: Possible typo: you repeated a word
Context: ...te - dynamic model tables: Stores model data data associated with entities - models: Cont...
(ENGLISH_WORD_REPEAT_RULE)
[formatting] ~14-~14: Consider inserting a comma here, unless the first half is essential to the meaning of the sentence.
Context: ...out how to query the database. With the schema you can then associate entities with mo...
(WITH_THAT_COMMA)
🔇 Additional comments (1)
crates/torii/server/src/handlers/sql.rs (1)
119-158: Solid Implementation of map_row_to_json Function
Ohayo sensei! The new map_row_to_json function provides a comprehensive and efficient way to convert SQL rows to JSON values. It thoughtfully handles various data types, including TEXT, INTEGER, REAL, and BLOB, enhancing code modularity and reusability.
| if let Some(query) = params.get("arguments").and_then(Value::as_str) { | ||
| match sqlx::query(query).fetch_all(&*self.pool).await { | ||
| Ok(rows) => { | ||
| // Convert rows to JSON using the same logic as SqlHandler | ||
| let result = rows | ||
| .iter() | ||
| .map(|row| { | ||
| let mut obj = serde_json::Map::new(); | ||
| for (i, column) in row.columns().iter().enumerate() { | ||
| let value: serde_json::Value = match column.type_info().name() { | ||
| "TEXT" => row.get::<Option<String>, _>(i).map_or( | ||
| serde_json::Value::Null, | ||
| serde_json::Value::String, | ||
| ), | ||
| "INTEGER" => row | ||
| .get::<Option<i64>, _>(i) | ||
| .map_or(serde_json::Value::Null, |n| { | ||
| serde_json::Value::Number(n.into()) | ||
| }), | ||
| "REAL" => row.get::<Option<f64>, _>(i).map_or( | ||
| serde_json::Value::Null, | ||
| |f| { | ||
| serde_json::Number::from_f64(f).map_or( | ||
| serde_json::Value::Null, | ||
| serde_json::Value::Number, | ||
| ) | ||
| }, | ||
| ), | ||
| "BLOB" => row.get::<Option<Vec<u8>>, _>(i).map_or( | ||
| serde_json::Value::Null, | ||
| |bytes| { | ||
| serde_json::Value::String(STANDARD.encode(bytes)) | ||
| }, | ||
| ), | ||
| _ => { | ||
| // Try different types in order | ||
| if let Ok(val) = row.try_get::<i64, _>(i) { | ||
| serde_json::Value::Number(val.into()) | ||
| } else if let Ok(val) = row.try_get::<f64, _>(i) { | ||
| // Handle floating point numbers | ||
| serde_json::json!(val) | ||
| } else if let Ok(val) = row.try_get::<bool, _>(i) { | ||
| serde_json::Value::Bool(val) | ||
| } else if let Ok(val) = row.try_get::<String, _>(i) { | ||
| serde_json::Value::String(val) | ||
| } else { | ||
| // Handle or fallback to BLOB as base64 | ||
| let val = row.get::<Option<Vec<u8>>, _>(i); | ||
| val.map_or(serde_json::Value::Null, |bytes| { | ||
| serde_json::Value::String( | ||
| STANDARD.encode(bytes), | ||
| ) | ||
| }) | ||
| } | ||
| } | ||
| }; | ||
| obj.insert(column.name().to_string(), value); | ||
| } | ||
| Value::Object(obj) | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
| // Convert rows to JSON using shared mapping function | ||
| let result = rows.iter().map(map_row_to_json).collect::<Vec<_>>(); |
There was a problem hiding this comment.
Potential Inconsistency in Parameter Handling in handle_query_tool
Ohayo sensei! In the handle_query_tool method, the code is attempting to extract the query parameter as a string directly from params.get("arguments"):
if let Some(query) = params.get("arguments").and_then(Value::as_str) {
// Proceed with query execution
}However, according to the inputSchema defined in the handle_tools_list method (lines 128-142), the arguments should be an object containing the query field. To ensure consistency and proper parameter extraction, consider modifying the code to:
if let Some(query) = params.get("arguments").and_then(|args| args.get("query")).and_then(Value::as_str) {
// Proceed with query execution
}This adjustment aligns with the defined schema and ensures the query parameter is correctly retrieved.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2790 +/- ##
==========================================
+ Coverage 55.58% 55.72% +0.13%
==========================================
Files 436 436
Lines 55521 55384 -137
==========================================
+ Hits 30862 30863 +1
+ Misses 24659 24521 -138 ☔ View full report in Codecov by Sentry. |
glihm
changed the title
Summary by CodeRabbit
New Features
Bug Fixes