Improve error handling & return meaningful error messages #246
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request improves error handling across SQL query translation by replacing panic calls with proper error types and returning meaningful error messages. The changes make the codebase more robust by providing helpful context when errors occur.
Key Changes
- Replaced panic/unwrap calls with Result-based error handling using custom error types
- Added descriptive error messages that include query context and available options
- Introduced new TsGeneratorError variants for specific failure scenarios
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/ts_generator/errors.rs | Added 6 new error variants with detailed error messages for common failure cases |
| src/ts_generator/sql_parser/translate_query.rs | Replaced panic with proper error propagation for table join processing |
| src/ts_generator/sql_parser/translate_insert.rs | Converted unwrap/panic calls to Result-based errors with helpful column mismatch details |
| src/ts_generator/sql_parser/expressions/translate_expr.rs | Replaced panics with error logging and None returns; converted unwrap chains to proper error handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| error!( | ||
| "Table '{}' not found in database schema. Check that the table exists and is accessible.", | ||
| table_name | ||
| ); | ||
| return None; |
There was a problem hiding this comment.
This error logs a message but returns None instead of propagating an error. The caller won't be able to distinguish this failure from a legitimate None case. Consider returning TsGeneratorError::TableNotFoundInSchema which is already defined in errors.rs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/ts_generator/sql_parser/translate_insert.rs:1
- The error variant is named
InsertStatementProcessingFailedbut the query format shows an UPDATE statement. This error type should either be renamed to handle both INSERT and UPDATE statements, or a separate error variant should be used for UPDATE operations.
use crate::common::lazy::DB_SCHEMA;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let column_name = translate_column_name_assignment(assignment).ok_or_else(|| { | ||
| TsGeneratorError::InsertStatementProcessingFailed { | ||
| reason: "Failed to extract column name from assignment".to_string(), | ||
| query: format!("UPDATE {} SET {} = ...", table_name, assignment), | ||
| } | ||
| })?; |
There was a problem hiding this comment.
The error uses InsertStatementProcessingFailed for an UPDATE statement scenario. This is misleading as the error variant name suggests it's for INSERT operations, but it's being used in translate_assignment which handles UPDATE statements. Consider creating a new error variant like UpdateStatementProcessingFailed or renaming the existing variant to something more generic like StatementProcessingFailed.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/ts_generator/errors.rs
Outdated
| InsertStatementProcessingFailed { reason: String, query: String }, | ||
| #[error("[E018] Failed to process UPDATE statement: {reason}. Query: `{query}`")] | ||
| UpdateStatementProcessingFailed { reason: String, query: String }, | ||
| #[error("[E018] Table '{table}' not found in database schema. Check that the table exists and is accessible.")] |
There was a problem hiding this comment.
Two different error variants are using the same error code [E018]. The TableNotFoundInSchema error on line 47 and UpdateStatementProcessingFailed error on line 45 both use [E018]. One should be changed to [E020] or another unique code to avoid confusion.
| #[error("[E018] Table '{table}' not found in database schema. Check that the table exists and is accessible.")] | |
| #[error("[E020] Table '{table}' not found in database schema. Check that the table exists and is accessible.")] |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.