feat(torii): sql playground #2714
Conversation
WalkthroughOhayo, sensei! The changes in this pull request enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SqlHandler
participant Playground
Client->>SqlHandler: GET /sql-playground
SqlHandler->>Playground: serve_playground()
Playground-->>SqlHandler: HTML Response
SqlHandler-->>Client: 200 OK with HTML
sequenceDiagram
participant Client
participant SqlHandler
Client->>SqlHandler: Request (GET or Query)
SqlHandler->>SqlHandler: handle_request(req)
alt If GET and empty query
SqlHandler->>SqlHandler: serve_playground()
else If Query
SqlHandler->>SqlHandler: extract_query()
alt If successful
SqlHandler->>SqlHandler: execute_query(query)
else
SqlHandler->>SqlHandler: serve_playground()
end
end
SqlHandler-->>Client: Response
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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: 7
🧹 Outside diff range and nitpick comments (2)
crates/torii/server/src/handlers/sql.rs (1)
Line range hint
24-24: Critical: SQL Injection Risk in Query Execution!Ohayo sensei! The
execute_querymethod directly executes user input as SQL queries without proper sanitization or validation. This is a critical security vulnerability that could lead to SQL injection attacks.Consider these security improvements:
- Implement a SQL query validator/sanitizer
- Use parameterized queries where possible
- Add a whitelist of allowed SQL operations
- Implement query timeout mechanism
Would you like me to provide an implementation with these security measures?
crates/torii/server/static/sql-playground.html (1)
95-95: Unused referencemonacoRefOhayo, sensei! The
monacoRefreference is assigned but never used, which can be removed to clean up the code.Apply this diff to remove
monacoRef:- const monacoRef = useRef(null);And remove the assignment:
- monacoRef.current = monaco;Also applies to: 277-277
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
crates/torii/server/src/handlers/sql.rs(3 hunks)crates/torii/server/static/sql-playground.html(1 hunks)
🔇 Additional comments (1)
crates/torii/server/src/handlers/sql.rs (1)
152-152: Verify SQL endpoint path handling, sensei!
The implementation looks good, but let's verify that all SQL-related paths are properly handled.
✅ Verification successful
SQL endpoint path handling is properly implemented, sensei!
The implementation in sql.rs shows that:
- The handler correctly checks for
/sqlpath prefix inshould_handle - Path handling is properly implemented with both GET and POST methods
- The handler serves SQL playground for empty GET requests
- Query parameter handling is properly implemented for both methods
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other SQL endpoint handlers that might conflict
rg -l "starts_with\(\"/sql\"\)" --type rust
# Search for SQL route definitions
rg "route.*sql" --type rust
Length of output: 113
Script:
#!/bin/bash
# Let's check the SQL handler implementation to verify path handling
ast-grep --pattern 'impl SqlHandler {
$$$
async fn handle_request($$$) {
$$$
}
$$$
}'
# Also check for any route registration or path handling in the main server setup
rg -A 5 "Router::new\(\).*sql" --type rust
Length of output: 144
Script:
#!/bin/bash
# Let's check the entire SQL handler file content
cat crates/torii/server/src/handlers/sql.rs
# Also check for router setup in main.rs or lib.rs
fd -e rs "main|lib" -X cat
Length of output: 102409
| React.createElement(Button, { | ||
| variant: 'outline', | ||
| onClick: () => copyToClipboard(query) | ||
| }, copied ? '✓ Copied!' : 'Copy') | ||
| ]), |
There was a problem hiding this comment.
Undefined function copyToClipboard
Ohayo, sensei! The function copyToClipboard is called but not defined, which will cause a runtime error when attempting to copy the query.
To fix this issue, define the copyToClipboard function.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2714 +/- ##
==========================================
+ Coverage 56.16% 56.23% +0.07%
==========================================
Files 414 415 +1
Lines 52965 53206 +241
==========================================
+ Hits 29747 29921 +174
- Misses 23218 23285 +67 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
crates/torii/server/src/handlers/sql.rs (1)
152-152: Ohayo! Consider adding error handling wrapper, sensei!The direct delegation to
handle_requestcould benefit from a top-level error handling wrapper to ensure consistent error responses:-self.handle_request(req).await +match self.handle_request(req).await { + response if response.status().is_success() => response, + response => { + tracing::error!("SQL handler error: {:?}", response.status()); + response + } +}crates/torii/server/static/sql-playground.html (3)
7-11: Consider production-ready setup for external dependenciesOhayo, sensei! While the current setup works, consider these production improvements:
- Pin versions for all dependencies (currently only Monaco Editor is pinned)
- Bundle TailwindCSS instead of using CDN for production
- Consider self-hosting these dependencies for better reliability and performance
105-233: Enhance SQL autocompletion suggestionsOhayo, sensei! Consider these improvements to the SQL suggestions:
- Add missing common SQL keywords (HAVING, DISTINCT, etc.)
- Add documentation for all SQL functions like you did for COUNT
- Consider fetching table suggestions dynamically from the backend
const suggestions = [ + { + label: 'HAVING', + kind: monaco.languages.CompletionItemKind.Keyword, + insertText: 'HAVING', + detail: 'SQL Keyword', + documentation: 'Filter grouped rows' + }, + { + label: 'DISTINCT', + kind: monaco.languages.CompletionItemKind.Keyword, + insertText: 'DISTINCT', + detail: 'SQL Keyword', + documentation: 'Return unique values' + }, // ... existing suggestions ... { label: 'SUM', kind: monaco.languages.CompletionItemKind.Function, insertText: 'SUM($0)', insertTextRules: monaco.languages.CompletionItemInsertTextRule.InsertAsSnippet, - detail: 'SQL Function' + detail: 'SQL Function', + documentation: 'Calculate the sum of values' }, { label: 'AVG', kind: monaco.languages.CompletionItemKind.Function, insertText: 'AVG($0)', insertTextRules: monaco.languages.CompletionItemInsertTextRule.InsertAsSnippet, - detail: 'SQL Function' + detail: 'SQL Function', + documentation: 'Calculate the average of values' },
377-411: Enhance results table functionalityOhayo, sensei! Consider adding these features to improve the results display:
- Pagination for large result sets
- Column sorting
- Cell content truncation with expandable view
React.createElement('table', { className: 'min-w-full divide-y divide-zinc-800' }, [ React.createElement('thead', { className: 'bg-zinc-900' }, React.createElement('tr', {}, Object.keys(response[0]).map(header => React.createElement('th', { key: header, - className: 'px-6 py-3 text-left text-xs font-medium text-zinc-400 uppercase tracking-wider' + className: 'px-6 py-3 text-left text-xs font-medium text-zinc-400 uppercase tracking-wider cursor-pointer hover:text-white', + onClick: () => handleSort(header) }, header) ) ) ), React.createElement('tbody', { className: 'divide-y divide-zinc-800' }, - response.map((row, rowIndex) => + paginatedResults.map((row, rowIndex) => React.createElement('tr', { key: rowIndex, className: rowIndex % 2 === 0 ? 'bg-zinc-900' : 'bg-zinc-900/50' }, Object.values(row).map((value, colIndex) => React.createElement('td', { key: colIndex, - className: 'px-6 py-4 whitespace-nowrap text-sm text-zinc-300 font-mono' + className: 'px-6 py-4 text-sm text-zinc-300 font-mono', + title: String(value) }, value === null ? React.createElement('span', { className: 'text-zinc-500 italic' }, 'NULL') : typeof value === 'object' ? - JSON.stringify(value) : - String(value) + truncateText(JSON.stringify(value), 100) : + truncateText(String(value), 100) ) ) ) ) ) ])
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
crates/torii/server/src/handlers/sql.rs(3 hunks)crates/torii/server/static/sql-playground.html(1 hunks)
🔇 Additional comments (4)
crates/torii/server/src/handlers/sql.rs (4)
7-7: Previous comment about include_str import is still valid, sensei!
122-131: Previous security improvement suggestions are still valid, sensei!
133-142: Previous suggestions about error handling and security measures are still valid, sensei!
134-134:
Ohayo! Let's handle the unwrap_or_default more safely, sensei!
The current implementation might panic if the URI is malformed. Consider using a more defensive approach:
-if req.method() == Method::GET && req.uri().query().unwrap_or_default().is_empty() {
+if req.method() == Method::GET && req.uri().query().map(|q| q.is_empty()).unwrap_or(true) {Likely invalid or redundant comment.
| return React.createElement('div', { className: 'min-h-screen bg-black text-white p-4' }, | ||
| React.createElement('div', { className: 'max-w-8xl mx-auto' }, [ | ||
| React.createElement('div', { className: 'mb-6', key: 'header' }, [ | ||
| React.createElement('div', { className: 'flex items-center gap-2 mb-1' }, [ | ||
| React.createElement('h1', { className: 'text-2xl font-bold text-white' }, 'Torii SQL Playground'), | ||
| React.createElement('span', { className: 'px-2 py-1 rounded-full bg-zinc-800 text-xs text-zinc-400' }, 'BETA') | ||
| ]), | ||
| React.createElement('p', { className: 'text-zinc-400' }, 'Write and execute SQL queries in real-time') | ||
| ]), | ||
| React.createElement('div', { className: 'grid grid-cols-1 lg:grid-cols-2 gap-6', key: 'content' }, [ | ||
| // Query Editor Panel | ||
| React.createElement(Card, { key: 'editor', className: 'flex flex-col h-[75vh]' }, [ | ||
| React.createElement('div', { className: 'p-4 border-b border-zinc-800 flex items-center justify-between' }, [ | ||
| React.createElement('h2', { className: 'text-lg font-semibold text-white' }, 'Query Editor'), | ||
| React.createElement(Button, { | ||
| variant: 'outline', | ||
| onClick: () => copyToClipboard(query) | ||
| }, copied ? '✓ Copied!' : 'Copy') | ||
| ]), | ||
| React.createElement('div', { className: 'flex-1 relative' }, | ||
| React.createElement('div', { | ||
| id: 'monaco-editor', | ||
| className: 'absolute inset-0' | ||
| }) | ||
| ), | ||
| React.createElement('div', { className: 'p-4 border-t border-zinc-800' }, | ||
| React.createElement(Button, { | ||
| onClick: executeQuery, | ||
| disabled: loading, | ||
| className: 'w-full' | ||
| }, loading ? '⟳ Executing...' : '▶ Run Query') | ||
| ) | ||
| ]), | ||
|
|
||
| // Results Panel updates... | ||
| React.createElement(Card, { key: 'results', className: 'flex flex-col h-[75vh]' }, [ | ||
| React.createElement('div', { className: 'p-4 border-b border-zinc-800 flex items-center justify-between' }, [ | ||
| React.createElement('h2', { className: 'text-lg font-semibold text-white' }, 'Results'), | ||
| response && React.createElement(Button, { | ||
| variant: 'outline', | ||
| onClick: downloadResults | ||
| }, '⇩ Download JSON') | ||
| ]), | ||
| React.createElement('div', { className: 'flex-1 p-4 overflow-auto' }, [ | ||
| error && React.createElement(Alert, { variant: 'destructive' }, | ||
| '⚠ ' + error | ||
| ), | ||
| response && response.length > 0 && React.createElement('div', { className: 'overflow-x-auto' }, | ||
| React.createElement('table', { className: 'min-w-full divide-y divide-zinc-800' }, [ | ||
| React.createElement('thead', { className: 'bg-zinc-900' }, | ||
| React.createElement('tr', {}, | ||
| Object.keys(response[0]).map(header => | ||
| React.createElement('th', { | ||
| key: header, | ||
| className: 'px-6 py-3 text-left text-xs font-medium text-zinc-400 uppercase tracking-wider' | ||
| }, header) | ||
| ) | ||
| ) | ||
| ), | ||
| React.createElement('tbody', { className: 'divide-y divide-zinc-800' }, | ||
| response.map((row, rowIndex) => | ||
| React.createElement('tr', { | ||
| key: rowIndex, | ||
| className: rowIndex % 2 === 0 ? 'bg-zinc-900' : 'bg-zinc-900/50' | ||
| }, | ||
| Object.values(row).map((value, colIndex) => | ||
| React.createElement('td', { | ||
| key: colIndex, | ||
| className: 'px-6 py-4 whitespace-nowrap text-sm text-zinc-300 font-mono' | ||
| }, | ||
| value === null ? | ||
| React.createElement('span', { className: 'text-zinc-500 italic' }, 'NULL') : | ||
| typeof value === 'object' ? | ||
| JSON.stringify(value) : | ||
| String(value) | ||
| ) | ||
| ) | ||
| ) | ||
| ) | ||
| ) | ||
| ]) | ||
| ), | ||
| (!response || response.length === 0) && !error && !loading && React.createElement('div', { | ||
| className: 'flex flex-col items-center justify-center h-full text-zinc-500' | ||
| }, [ | ||
| React.createElement('p', { className: 'text-lg font-medium' }, 'No Results Yet'), | ||
| React.createElement('p', { className: 'text-sm' }, 'Execute a query to see results here') | ||
| ]) | ||
| ]), | ||
| response && React.createElement('div', { className: 'p-4 border-t border-zinc-800 bg-zinc-900' }, | ||
| React.createElement('p', { className: 'text-sm text-zinc-400' }, | ||
| `Showing ${response.length} row${response.length !== 1 ? 's' : ''}` | ||
| ) | ||
| ) | ||
| ]) | ||
| ]) | ||
| ]) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility support
Ohayo, sensei! The interface needs accessibility improvements:
- Add ARIA labels and roles
- Implement keyboard navigation
- Add screen reader support
return React.createElement('div', { className: 'min-h-screen bg-black text-white p-4' },
- React.createElement('div', { className: 'max-w-8xl mx-auto' }, [
+ React.createElement('div', {
+ className: 'max-w-8xl mx-auto',
+ role: 'main',
+ 'aria-label': 'SQL Playground'
+ }, [
React.createElement('div', { className: 'mb-6', key: 'header' }, [
React.createElement('div', { className: 'flex items-center gap-2 mb-1' }, [
- React.createElement('h1', { className: 'text-2xl font-bold text-white' }, 'Torii SQL Playground'),
+ React.createElement('h1', {
+ className: 'text-2xl font-bold text-white',
+ role: 'heading',
+ 'aria-level': '1'
+ }, 'Torii SQL Playground'),Committable suggestion skipped: line range outside the PR's diff.
| const executeQuery = async () => { | ||
| setLoading(true); | ||
| setError(null); | ||
| try { | ||
| const response = await fetch('/sql?' + new URLSearchParams({ query })); | ||
| if (!response.ok) { | ||
| throw new Error(`Query failed: ${response.statusText}`); | ||
| } | ||
| const data = await response.json(); | ||
| setResponse(data); | ||
| } catch (error) { | ||
| setError(error.message); | ||
| setResponse(null); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve query execution robustness
Ohayo, sensei! Consider enhancing the query execution with:
- Timeout handling for long-running queries
- Query size validation
- Rate limiting feedback
const executeQuery = async () => {
setLoading(true);
setError(null);
try {
+ if (query.length > 10000) {
+ throw new Error('Query too large. Please limit to 10000 characters.');
+ }
+ const controller = new AbortController();
+ const timeoutId = setTimeout(() => controller.abort(), 30000);
const response = await fetch('/sql?' + new URLSearchParams({ query }), {
+ signal: controller.signal
});
+ clearTimeout(timeoutId);
if (!response.ok) {
+ const errorData = await response.json();
+ if (response.status === 429) {
+ throw new Error('Rate limit exceeded. Please wait before trying again.');
+ }
throw new Error(`Query failed: ${response.statusText}`);
}
const data = await response.json();
setResponse(data);
} catch (error) {
+ if (error.name === 'AbortError') {
+ setError('Query timed out. Please try a simpler query.');
+ } else {
setError(error.message);
+ }
setResponse(null);
} finally {
setLoading(false);
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const executeQuery = async () => { | |
| setLoading(true); | |
| setError(null); | |
| try { | |
| const response = await fetch('/sql?' + new URLSearchParams({ query })); | |
| if (!response.ok) { | |
| throw new Error(`Query failed: ${response.statusText}`); | |
| } | |
| const data = await response.json(); | |
| setResponse(data); | |
| } catch (error) { | |
| setError(error.message); | |
| setResponse(null); | |
| } finally { | |
| setLoading(false); | |
| } | |
| }; | |
| const executeQuery = async () => { | |
| setLoading(true); | |
| setError(null); | |
| try { | |
| if (query.length > 10000) { | |
| throw new Error('Query too large. Please limit to 10000 characters.'); | |
| } | |
| const controller = new AbortController(); | |
| const timeoutId = setTimeout(() => controller.abort(), 30000); | |
| const response = await fetch('/sql?' + new URLSearchParams({ query }), { | |
| signal: controller.signal | |
| }); | |
| clearTimeout(timeoutId); | |
| if (!response.ok) { | |
| const errorData = await response.json(); | |
| if (response.status === 429) { | |
| throw new Error('Rate limit exceeded. Please wait before trying again.'); | |
| } | |
| throw new Error(`Query failed: ${response.statusText}`); | |
| } | |
| const data = await response.json(); | |
| setResponse(data); | |
| } catch (error) { | |
| if (error.name === 'AbortError') { | |
| setError('Query timed out. Please try a simpler query.'); | |
| } else { | |
| setError(error.message); | |
| } | |
| setResponse(null); | |
| } finally { | |
| setLoading(false); | |
| } | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
crates/torii/server/src/handlers/sql.rs (1)
52-70: Ohayo sensei! Let's improve the type handling maintainability!The fallback type handling logic is quite complex and would benefit from being extracted into a separate method with proper documentation explaining the type conversion strategy.
Consider this refactor:
+ /// Attempts to convert a SQL value to a JSON value using various type conversions + /// in a specific order: i64 -> f64 -> bool -> String -> BLOB(base64) + fn convert_sql_value(&self, row: &sqlx::Row, i: usize) -> serde_json::Value { + 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) { + 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 { + let val = row.get::<Option<Vec<u8>>, _>(i); + val.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)) - }) - } - }, + _ => self.convert_sql_value(row, i),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/torii/server/src/handlers/sql.rs(5 hunks)
🔇 Additional comments (2)
crates/torii/server/src/handlers/sql.rs (2)
168-168: Ohayo sensei! The handler implementation looks good!
The delegation to handle_request is clean and follows good separation of concerns.
149-158:
Ohayo sensei! The error handling needs improvement!
The current implementation falls back to serving the playground on query extraction errors, which could mask important issues from clients.
Consider this enhanced implementation:
async fn handle_request(&self, req: Request<Body>) -> Response<Body> {
if req.method() == Method::GET && req.uri().query().unwrap_or_default().is_empty() {
self.serve_playground().await
} else {
match self.extract_query(req).await {
Ok(query) => self.execute_query(query).await,
- Err(_) => self.serve_playground().await,
+ Err(response) => response, // Return the actual error response
}
}
}Likely invalid or redundant comment.
| async fn serve_playground(&self) -> Response<Body> { | ||
| let html = include_str!("../../static/sql-playground.html"); | ||
|
|
||
| Response::builder() | ||
| .status(StatusCode::OK) | ||
| .header(CONTENT_TYPE, "text/html") | ||
| .header("Access-Control-Allow-Origin", "*") | ||
| .body(Body::from(html)) | ||
| .unwrap() | ||
| } |
There was a problem hiding this comment.
Ohayo sensei! Let's enhance the playground security!
The playground serving implementation needs some security improvements:
- Using
unwrap()could cause panics - The
Access-Control-Allow-Origin: *header is too permissive - Missing important security headers (CSP, X-Content-Type-Options)
Consider this safer implementation:
async fn serve_playground(&self) -> Response<Body> {
let html = include_str!("../../static/sql-playground.html");
- Response::builder()
- .status(StatusCode::OK)
- .header(CONTENT_TYPE, "text/html")
- .header("Access-Control-Allow-Origin", "*")
- .body(Body::from(html))
- .unwrap()
+ Response::builder()
+ .status(StatusCode::OK)
+ .header(CONTENT_TYPE, "text/html")
+ .header("Content-Security-Policy", "default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline';")
+ .header("X-Content-Type-Options", "nosniff")
+ .body(Body::from(html))
+ .unwrap_or_else(|e| {
+ Response::builder()
+ .status(StatusCode::INTERNAL_SERVER_ERROR)
+ .body(Body::from(format!("Failed to serve playground: {}", e)))
+ .unwrap_or_default()
+ })Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
crates/torii/server/src/handlers/sql.rs (1)
Line range hint
33-70: Ohayo sensei! Nice type handling implementation!The enhanced type conversion logic is well-structured with proper fallbacks. Consider extracting the type conversion logic into a separate method for better maintainability.
impl SqlHandler { + fn convert_column_value(&self, row: &sqlx::sqlite::SqliteRow, i: usize, column: &sqlx::sqlite::SqliteColumn) -> serde_json::Value { + match column.type_info().name() { + "TEXT" => row.get::<Option<String>, _>(i) + .map_or(serde_json::Value::Null, serde_json::Value::String), + // ... rest of the type conversion logic ... + } + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/torii/server/src/handlers/sql.rs(5 hunks)
🔇 Additional comments (1)
crates/torii/server/src/handlers/sql.rs (1)
168-168: Ohayo! Clean implementation, sensei!
Good job delegating to the new handle_request method. This improves code organization and separation of concerns.
| async fn handle_request(&self, req: Request<Body>) -> Response<Body> { | ||
| if req.method() == Method::GET && req.uri().query().unwrap_or_default().is_empty() { | ||
| self.serve_playground().await | ||
| } else { | ||
| match self.extract_query(req).await { | ||
| Ok(query) => self.execute_query(query).await, | ||
| Err(_) => self.serve_playground().await, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Ohayo! Let's enhance the request validation, sensei!
The current implementation has two areas for improvement:
- Falling back to playground on query extraction error masks potential issues
- Missing input validation before query execution
Consider this enhanced implementation:
async fn handle_request(&self, req: Request<Body>) -> Response<Body> {
if req.method() == Method::GET && req.uri().query().unwrap_or_default().is_empty() {
self.serve_playground().await
} else {
match self.extract_query(req).await {
Ok(query) => {
+ // Validate query before execution
+ if query.trim().is_empty() {
+ return Response::builder()
+ .status(StatusCode::BAD_REQUEST)
+ .body(Body::from("Empty query"))
+ .unwrap_or_default();
+ }
self.execute_query(query).await
}
- Err(_) => self.serve_playground().await,
+ Err(e) => e, // Return the actual error for better visibility
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async fn handle_request(&self, req: Request<Body>) -> Response<Body> { | |
| if req.method() == Method::GET && req.uri().query().unwrap_or_default().is_empty() { | |
| self.serve_playground().await | |
| } else { | |
| match self.extract_query(req).await { | |
| Ok(query) => self.execute_query(query).await, | |
| Err(_) => self.serve_playground().await, | |
| } | |
| } | |
| } | |
| async fn handle_request(&self, req: Request<Body>) -> Response<Body> { | |
| if req.method() == Method::GET && req.uri().query().unwrap_or_default().is_empty() { | |
| self.serve_playground().await | |
| } else { | |
| match self.extract_query(req).await { | |
| Ok(query) => { | |
| // Validate query before execution | |
| if query.trim().is_empty() { | |
| return Response::builder() | |
| .status(StatusCode::BAD_REQUEST) | |
| .body(Body::from("Empty query")) | |
| .unwrap_or_default(); | |
| } | |
| self.execute_query(query).await | |
| } | |
| Err(e) => e, // Return the actual error for better visibility | |
| } | |
| } | |
| } |
Summary by CodeRabbit
New Features
Bug Fixes