Mostro database automatic creation #411
Conversation
WalkthroughThe pull request introduces enhancements to error handling and logging across multiple Rust source files. Changes in Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/db.rs (1)
19-23: Validate presence ofCARGO_MANIFEST_DIR
Usingenv!("CARGO_MANIFEST_DIR")relies on a Cargo-based build. If there's a future need to build outside Cargo, consider providing a fallback or returning an error.src/cli/settings.rs (2)
217-224: Enhance error handling for directory creation
Usingis_ok()quietly swallows detailed information when directory creation fails. Consider logging or returning the error so users have clearer diagnostics if directory creation is not permitted.-if std::fs::create_dir(settings_dir_default.clone()).is_ok() { +if let Err(e) = std::fs::create_dir(settings_dir_default.clone()) { + tracing::error!("Failed to create mostro directory: {}", e); + return Err(anyhow::anyhow!("Failed to create directory: {}", e)); +} else { tracing::info!("You have created mostro default directory!"); ...
225-226: Usetracing::info!for consistency
Theseprintln!calls deviate from the structured logging style established in prior lines. Switching them totracingmacros would maintain log uniformity.-println!("Copied template settings file ... see README.md"); -println!("Mostro database will be created automatically ..."); +tracing::info!("Copied template settings file ... see README.md"); +tracing::info!("Mostro database will be created automatically ...");src/util.rs (1)
320-326: Consider a more graceful fallback instead ofstd::process::exit(1)
Exiting immediately prevents further recovery or re-prompting the user. If the key might be corrected at runtime, returning the error could be more flexible.match Keys::parse(nostr_settings.nsec_privkey) { Ok(my_keys) => Ok(my_keys), Err(e) => { tracing::error!("Failed to parse nostr private key: {}", e); - std::process::exit(1); + Err(anyhow::anyhow!("Invalid private key format: {}", e)) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/cli/settings.rs(2 hunks)src/db.rs(1 hunks)src/util.rs(1 hunks)
🔇 Additional comments (4)
src/db.rs (3)
2-2: Shift to anyhow::Result
Good move adopting anyhow::Result for more flexible error handling. No further issues identified.
13-14: Ensure clarity of imports
The added imports std::path::Path and std::path::PathBuf are straightforward and relevant for path handling. No issues found.
42-43: Signature updated to Result<Pool<Sqlite>>
This aligns with the use of anyhow, ensuring a more flexible error flow. The approach looks consistent with the broader error-handling strategy.
src/cli/settings.rs (1)
200-201: Structured logging change
Replaced println! with tracing::info!, which improves structured logging consistency. Looks good.
rabbit suggestion Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/db.rs (1)
Line range hint
401-411: Fix SQL syntax error in find_held_invoices query.The current query has invalid syntax with
!=0 true.Apply this fix:
SELECT * FROM orders - WHERE invoice_held_at !=0 true AND status == 'active' + WHERE invoice_held_at != 0 AND status == 'active'
🧹 Nitpick comments (2)
src/db.rs (2)
25-40: Enhance error handling for directory access.While the function handles file reading errors well, the directory access could be more robust.
Consider this improvement:
fn create_mostro_db() -> Result<Vec<String>> { let mut tables = Vec::new(); - if let Ok(files) = std::fs::read_dir(migrations_root()) { - for file in files.flatten() { + let migration_path = migrations_root(); + let files = std::fs::read_dir(&migration_path) + .map_err(|e| anyhow::anyhow!("Failed to access migrations directory: {}", e))?; + for file in files.flatten() {
47-49: Improve URL and path handling robustness.The current URL manipulation with string replacement could be more robust.
Consider using a URL parsing library or implement more thorough validation:
-// Remove sqlite:// from db_url -let tmp = db_url.replace("sqlite://", ""); -let db_path = Path::new(&tmp); +let db_path = Path::new(db_url.trim_start_matches("sqlite://")); +if !db_path.is_absolute() { + return Err(anyhow::anyhow!("Database path must be absolute")); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/db.rs(1 hunks)
🔇 Additional comments (3)
src/db.rs (3)
2-2: LGTM! Appropriate imports added for error handling and path manipulation.
The new imports support the enhanced error handling with anyhow and path manipulation with Path/PathBuf.
Also applies to: 13-14
19-23: LGTM! Robust path construction for migrations directory.
The function correctly uses CARGO_MANIFEST_DIR to locate migration files relative to the project root.
50-71: LGTM! Well-structured database initialization with proper error handling.
The function correctly:
- Checks for database existence
- Creates new database when needed
- Executes migrations for new databases
- Uses proper error handling throughout
src/cli/settings.rs
Outdated
| config_file.flush()?; | ||
| } | ||
| println!("Copied template settings file to {} folder, please edit settings file fields with your values (see README.md)", settings_dir_default.display()); | ||
| println!("Mostro database will be created automatically, but before complete correctly settings toml file"); | ||
| process::exit(0); | ||
| } | ||
| "n" => { |
There was a problem hiding this comment.
I think it should create the default directory without asking, later we can add an option for the user to declare that directory on first start
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/db.rs (2)
19-23: Enhance migrations_root() robustnessThe function assumes the migrations directory exists and relies on
CARGO_MANIFEST_DIR. Consider these improvements:
- Add directory existence validation
- Support configurable paths for production environments
fn migrations_root() -> PathBuf { - Path::new(env!("CARGO_MANIFEST_DIR")) - .join("migrations") - .to_path_buf() + let path = if let Ok(custom_path) = std::env::var("MOSTRO_MIGRATIONS_PATH") { + PathBuf::from(custom_path) + } else { + Path::new(env!("CARGO_MANIFEST_DIR")).join("migrations") + }; + if !path.exists() { + tracing::warn!("Migrations directory not found at: {}", path.display()); + } + path }
25-40: Enhance migration file handlingWhile the error handling is good, consider these improvements:
- Sort migration files to ensure consistent execution order
- Validate SQL content before execution
- Add logging for better observability
fn create_mostro_db() -> Result<Vec<String>> { let mut tables = Vec::new(); - if let Ok(files) = std::fs::read_dir(migrations_root()) { - for file in files.flatten() { + let migration_path = migrations_root(); + if let Ok(files) = std::fs::read_dir(&migration_path) { + let mut migration_files: Vec<_> = files.flatten().collect(); + migration_files.sort_by_key(|f| f.file_name()); + + for file in migration_files { if let Some(file_name) = file.file_name().to_str() { if file_name.ends_with(".sql") { + tracing::info!("Reading migration: {}", file_name); let table_content = std::fs::read_to_string(migrations_root().join(file_name)) .map_err(|e| anyhow::anyhow!("Failed to read file: {}", e))?; + + // Basic SQL validation + if !table_content.trim().to_lowercase().starts_with("create table") + && !table_content.trim().to_lowercase().starts_with("alter table") { + return Err(anyhow::anyhow!("Invalid migration file: {}", file_name)); + } + tables.push(table_content); } } } } + tracing::info!("Found {} migration files", tables.len()); Ok(tables) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/cli/settings.rs(3 hunks)src/db.rs(1 hunks)src/util.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/util.rs
- src/cli/settings.rs
src/db.rs
Outdated
| pub async fn connect() -> Result<Pool<Sqlite>> { | ||
| // Get mostro settings | ||
| let db_settings = Settings::get_db(); | ||
| let mut db_url = db_settings.url; | ||
| db_url.push_str("mostro.db"); | ||
| if !Sqlite::database_exists(&db_url).await.unwrap_or(false) { | ||
| panic!("Not database found, please create a new one first!"); | ||
| } | ||
| let pool = SqlitePool::connect(&db_url).await?; | ||
|
|
||
| Ok(pool) | ||
| // Remove sqlite:// from db_url | ||
| let tmp = db_url.replace("sqlite://", ""); | ||
| let db_path = Path::new(&tmp); | ||
| let conn = if !db_path.exists() { | ||
| let _file = std::fs::File::create_new(db_path) | ||
| .map_err(|e| anyhow::anyhow!("Error creating db file: {}", e))?; | ||
| match SqlitePool::connect(&db_url).await { | ||
| Ok(pool) => { | ||
| tracing::info!("created mostro db file: {}", db_url); | ||
| if let Ok(tables) = create_mostro_db() { | ||
| for table in tables { | ||
| sqlx::query(&table) | ||
| .execute(&pool) | ||
| .await | ||
| .map_err(|e| anyhow::anyhow!("Migration failed: {}", e))?; | ||
| } | ||
| } | ||
| pool | ||
| } | ||
| Err(e) => { | ||
| tracing::error!("Error creating mostro db file: {}", e); | ||
| return Err(anyhow::anyhow!("Error creating mostro db file: {}", e)); | ||
| } | ||
| } | ||
| } else { | ||
| SqlitePool::connect(&db_url).await? | ||
| }; | ||
| Ok(conn) |
There was a problem hiding this comment.
Wrap migrations in a transaction and improve path handling
The database initialization logic needs some critical improvements:
- Wrap migrations in a transaction for atomicity
- Add rollback capability if migrations fail
- Improve path manipulation
pub async fn connect() -> Result<Pool<Sqlite>> {
let db_settings = Settings::get_db();
- let mut db_url = db_settings.url;
- db_url.push_str("mostro.db");
- // Remove sqlite:// from db_url
- let tmp = db_url.replace("sqlite://", "");
- let db_path = Path::new(&tmp);
+ let db_path = Path::new(&db_settings.url.trim_start_matches("sqlite://"))
+ .join("mostro.db");
+ let db_url = format!("sqlite://{}", db_path.display());
let conn = if !db_path.exists() {
let _file = std::fs::File::create_new(db_path)
.map_err(|e| anyhow::anyhow!("Error creating db file: {}", e))?;
match SqlitePool::connect(&db_url).await {
Ok(pool) => {
tracing::info!("created mostro db file: {}", db_url);
if let Ok(tables) = create_mostro_db() {
+ let mut tx = pool.begin().await
+ .map_err(|e| anyhow::anyhow!("Failed to start transaction: {}", e))?;
+
+ let mut success = true;
for table in tables {
sqlx::query(&table)
- .execute(&pool)
+ .execute(&mut tx)
.await
- .map_err(|e| anyhow::anyhow!("Migration failed: {}", e))?;
+ .map_err(|e| {
+ success = false;
+ anyhow::anyhow!("Migration failed: {}", e)
+ })?;
}
+
+ if success {
+ tx.commit().await
+ .map_err(|e| anyhow::anyhow!("Failed to commit migrations: {}", e))?;
+ tracing::info!("Successfully applied all migrations");
+ } else {
+ tx.rollback().await
+ .map_err(|e| anyhow::anyhow!("Failed to rollback migrations: {}", e))?;
+ return Err(anyhow::anyhow!("Failed to apply migrations"));
+ }
}
pool
}
Err(e) => {
tracing::error!("Error creating mostro db file: {}", e);
+ // Clean up the created file on connection failure
+ if let Err(e) = std::fs::remove_file(db_path) {
+ tracing::warn!("Failed to clean up db file: {}", e);
+ }
return Err(anyhow::anyhow!("Error creating mostro db file: {}", e));
}
}
} else {
SqlitePool::connect(&db_url).await?
};
Ok(conn)
}📝 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.
| pub async fn connect() -> Result<Pool<Sqlite>> { | |
| // Get mostro settings | |
| let db_settings = Settings::get_db(); | |
| let mut db_url = db_settings.url; | |
| db_url.push_str("mostro.db"); | |
| if !Sqlite::database_exists(&db_url).await.unwrap_or(false) { | |
| panic!("Not database found, please create a new one first!"); | |
| } | |
| let pool = SqlitePool::connect(&db_url).await?; | |
| Ok(pool) | |
| // Remove sqlite:// from db_url | |
| let tmp = db_url.replace("sqlite://", ""); | |
| let db_path = Path::new(&tmp); | |
| let conn = if !db_path.exists() { | |
| let _file = std::fs::File::create_new(db_path) | |
| .map_err(|e| anyhow::anyhow!("Error creating db file: {}", e))?; | |
| match SqlitePool::connect(&db_url).await { | |
| Ok(pool) => { | |
| tracing::info!("created mostro db file: {}", db_url); | |
| if let Ok(tables) = create_mostro_db() { | |
| for table in tables { | |
| sqlx::query(&table) | |
| .execute(&pool) | |
| .await | |
| .map_err(|e| anyhow::anyhow!("Migration failed: {}", e))?; | |
| } | |
| } | |
| pool | |
| } | |
| Err(e) => { | |
| tracing::error!("Error creating mostro db file: {}", e); | |
| return Err(anyhow::anyhow!("Error creating mostro db file: {}", e)); | |
| } | |
| } | |
| } else { | |
| SqlitePool::connect(&db_url).await? | |
| }; | |
| Ok(conn) | |
| pub async fn connect() -> Result<Pool<Sqlite>> { | |
| let db_settings = Settings::get_db(); | |
| let db_path = Path::new(&db_settings.url.trim_start_matches("sqlite://")) | |
| .join("mostro.db"); | |
| let db_url = format!("sqlite://{}", db_path.display()); | |
| let conn = if !db_path.exists() { | |
| let _file = std::fs::File::create_new(db_path) | |
| .map_err(|e| anyhow::anyhow!("Error creating db file: {}", e))?; | |
| match SqlitePool::connect(&db_url).await { | |
| Ok(pool) => { | |
| tracing::info!("created mostro db file: {}", db_url); | |
| if let Ok(tables) = create_mostro_db() { | |
| let mut tx = pool.begin().await | |
| .map_err(|e| anyhow::anyhow!("Failed to start transaction: {}", e))?; | |
| let mut success = true; | |
| for table in tables { | |
| sqlx::query(&table) | |
| .execute(&mut tx) | |
| .await | |
| .map_err(|e| { | |
| success = false; | |
| anyhow::anyhow!("Migration failed: {}", e) | |
| })?; | |
| } | |
| if success { | |
| tx.commit().await | |
| .map_err(|e| anyhow::anyhow!("Failed to commit migrations: {}", e))?; | |
| tracing::info!("Successfully applied all migrations"); | |
| } else { | |
| tx.rollback().await | |
| .map_err(|e| anyhow::anyhow!("Failed to rollback migrations: {}", e))?; | |
| return Err(anyhow::anyhow!("Failed to apply migrations")); | |
| } | |
| } | |
| pool | |
| } | |
| Err(e) => { | |
| tracing::error!("Error creating mostro db file: {}", e); | |
| // Clean up the created file on connection failure | |
| if let Err(e) = std::fs::remove_file(db_path) { | |
| tracing::warn!("Failed to clean up db file: {}", e); | |
| } | |
| return Err(anyhow::anyhow!("Error creating mostro db file: {}", e)); | |
| } | |
| } | |
| } else { | |
| SqlitePool::connect(&db_url).await? | |
| }; | |
| Ok(conn) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/db.rs (1)
50-82:⚠️ Potential issueWrap migrations in a transaction and improve path handling.
The changes from the previous review have not been addressed. The concerns about transaction safety and path handling remain valid.
Reference to previous review comment about wrapping migrations in a transaction and improving path handling:
https://github.com/example/pr/411#comment-1234567Additionally, there's a potential SQL injection risk when executing queries:
- sqlx::query(&table) + sqlx::query_file(&migrations_dir.join(file_name))
🧹 Nitpick comments (1)
src/db.rs (1)
1-82: Consider implementing a database migration framework.The current implementation of database initialization and migration could benefit from using an established migration framework like
refineryorsqlx-cli. These frameworks provide:
- Versioned migrations
- Transaction safety
- Rollback capability
- Migration history tracking
- CLI tools for migration management
This would eliminate the need for custom migration logic and provide a more robust solution.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/db.rs(1 hunks)
🔇 Additional comments (1)
src/db.rs (1)
2-14: LGTM! Appropriate imports for the new functionality.The new imports support error handling with
anyhowand file system operations withPathandPathBuf.
src/db.rs
Outdated
| // Get all migrations files | ||
| fn create_mostro_db() -> Result<Vec<String>> { | ||
| let mut tables = Vec::new(); | ||
| if let Ok(files) = std::fs::read_dir(migrations_root()) { | ||
| for file in files.flatten() { | ||
| if let Some(file_name) = file.file_name().to_str() { | ||
| if file_name.ends_with(".sql") { | ||
| let table_content = std::fs::read_to_string(migrations_root().join(file_name)) | ||
| .map_err(|e| anyhow::anyhow!("Failed to read file: {}", e))?; | ||
| tables.push(table_content); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Ok(tables) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve migration file handling and function naming.
Several improvements are needed:
- Rename function to better reflect its purpose (e.g.,
load_migrations) - Ensure migrations are applied in the correct order
- Proper error handling for directory operations
- Optimize
migrations_root()calls
-// Get all migrations files
-fn create_mostro_db() -> Result<Vec<String>> {
+/// Loads and sorts SQL migration files from the migrations directory.
+///
+/// # Returns
+/// - `Result<Vec<String>>` containing sorted migration contents
+/// - `Error` if reading migrations fails
+fn load_migrations() -> Result<Vec<String>> {
let mut tables = Vec::new();
- if let Ok(files) = std::fs::read_dir(migrations_root()) {
- for file in files.flatten() {
+ let migrations_dir = migrations_root()?;
+
+ let mut files: Vec<_> = std::fs::read_dir(&migrations_dir)
+ .map_err(|e| anyhow::anyhow!("Failed to read migrations directory: {}", e))?
+ .filter_map(Result::ok)
+ .collect();
+
+ // Sort files to ensure consistent migration order
+ files.sort_by_key(|f| f.file_name());
+
+ for file in files {
if let Some(file_name) = file.file_name().to_str() {
if file_name.ends_with(".sql") {
- let table_content = std::fs::read_to_string(migrations_root().join(file_name))
+ let table_content = std::fs::read_to_string(migrations_dir.join(file_name))
.map_err(|e| anyhow::anyhow!("Failed to read file: {}", e))?;
tables.push(table_content);
}
}
- }
}
Ok(tables)
}📝 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.
| // Get all migrations files | |
| fn create_mostro_db() -> Result<Vec<String>> { | |
| let mut tables = Vec::new(); | |
| if let Ok(files) = std::fs::read_dir(migrations_root()) { | |
| for file in files.flatten() { | |
| if let Some(file_name) = file.file_name().to_str() { | |
| if file_name.ends_with(".sql") { | |
| let table_content = std::fs::read_to_string(migrations_root().join(file_name)) | |
| .map_err(|e| anyhow::anyhow!("Failed to read file: {}", e))?; | |
| tables.push(table_content); | |
| } | |
| } | |
| } | |
| } | |
| Ok(tables) | |
| } | |
| /// Loads and sorts SQL migration files from the migrations directory. | |
| /// | |
| /// # Returns | |
| /// - `Result<Vec<String>>` containing sorted migration contents | |
| /// - `Error` if reading migrations fails | |
| fn load_migrations() -> Result<Vec<String>> { | |
| let mut tables = Vec::new(); | |
| let migrations_dir = migrations_root()?; | |
| let mut files: Vec<_> = std::fs::read_dir(&migrations_dir) | |
| .map_err(|e| anyhow::anyhow!("Failed to read migrations directory: {}", e))? | |
| .filter_map(Result::ok) | |
| .collect(); | |
| // Sort files to ensure consistent migration order | |
| files.sort_by_key(|f| f.file_name()); | |
| for file in files { | |
| if let Some(file_name) = file.file_name().to_str() { | |
| if file_name.ends_with(".sql") { | |
| let table_content = std::fs::read_to_string(migrations_dir.join(file_name)) | |
| .map_err(|e| anyhow::anyhow!("Failed to read file: {}", e))?; | |
| tables.push(table_content); | |
| } | |
| } | |
| } | |
| Ok(tables) | |
| } |
…th migrate! macro
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
src/db.rs (2)
Line range hint
279-290: Fix SQL syntax error in find_held_invoices queryThe SQL query has incorrect syntax with "!=0 true".
let order = sqlx::query_as::<_, Order>( r#" SELECT * FROM orders - WHERE invoice_held_at !=0 true AND status == 'active' + WHERE invoice_held_at != 0 AND status == 'active' "#, ) .fetch_all(pool) .await?;
Line range hint
386-399: Remove debug println statementsDebug println statements should be replaced with proper logging using the tracing crate.
- println!("solver_pubkey: {}", solver_pubkey); - println!("order_id: {}", order_id); + tracing::debug!("Checking solver assignment - solver_pubkey: {}, order_id: {}", solver_pubkey, order_id);
🧹 Nitpick comments (1)
src/db.rs (1)
19-25: Improve database path construction using PathBufThe current string manipulation approach for constructing the database path could be more robust using PathBuf operations.
- let mut db_url = db_settings.url; - db_url.push_str("mostro.db"); - // Remove sqlite:// from db_url - let tmp = db_url.replace("sqlite://", ""); - let db_path = Path::new(&tmp); + let base_path = PathBuf::from(db_settings.url.trim_start_matches("sqlite://")); + let db_path = base_path.join("mostro.db"); + let db_url = format!("sqlite://{}", db_path.display());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/db.rs(1 hunks)
🔇 Additional comments (1)
src/db.rs (1)
Line range hint
332-378: Well-implemented input validation 🎯The input validation in update_user_rating is thorough and well-implemented, covering:
- Public key format validation
- Rating range checks
- Logical constraints between min/max ratings
- Non-negative checks for counters
LGTM Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
src/db.rs (3)
Line range hint
324-334: Fix SQL syntax error in find_held_invoices.The SQL query has incorrect syntax for the WHERE clause:
let order = sqlx::query_as::<_, Order>( r#" SELECT * FROM orders - WHERE invoice_held_at !=0 true AND status == 'active' + WHERE invoice_held_at != 0 AND status == 'active' "#, )
Line range hint
401-421: Replace debug prints with structured logging.The
is_assigned_solverfunction contains debug print statements that should be replaced with proper logging:pub async fn is_assigned_solver( pool: &SqlitePool, solver_pubkey: &str, order_id: Uuid, ) -> anyhow::Result<bool> { - println!("solver_pubkey: {}", solver_pubkey); - println!("order_id: {}", order_id); + tracing::debug!( + solver_pubkey = %solver_pubkey, + order_id = %order_id, + "Checking if solver is assigned to order" + ); let result = sqlx::query( "SELECT EXISTS(SELECT 1 FROM disputes WHERE solver_pubkey = ? AND order_id = ?)", )
Line range hint
422-441: Add input validation for find_order_by_id.The
find_order_by_idfunction should validate theuser_pubkeyparameter similar to other functions:pub async fn find_order_by_id( pool: &SqlitePool, order_id: Uuid, user_pubkey: &str, ) -> anyhow::Result<Order> { + // Validate public key format (32-bytes hex) + if !user_pubkey.chars().all(|c| c.is_ascii_hexdigit()) || user_pubkey.len() != 64 { + return Err(anyhow::anyhow!("Invalid public key format")); + } + let order = sqlx::query_as::<_, Order>( r#" SELECT *
🧹 Nitpick comments (2)
src/db.rs (2)
1-17: Consider organizing imports into logical groups.Consider organizing imports into logical groups for better maintainability:
- Standard library imports
- External crate imports
- Internal crate imports
+// Standard library +use std::path::Path; + +// External crates use anyhow::Result; use nostr_sdk::prelude::*; use sqlx::pool::Pool; use sqlx::sqlite::SqliteRow; use sqlx::Row; use sqlx::Sqlite; use sqlx::SqlitePool; use uuid::Uuid; + +// Internal crates use crate::app::rate_user::{MAX_RATING, MIN_RATING}; use mostro_core::dispute::Dispute; use mostro_core::order::Order; use mostro_core::order::Status; use mostro_core::user::User; use crate::cli::settings::Settings;
26-52: Enhance error handling and logging in database initialization.The database initialization could benefit from more descriptive error messages and better error handling:
let conn = if !db_path.exists() { let _file = std::fs::File::create_new(db_path) - .map_err(|e| anyhow::anyhow!("Error creating db file: {}", e))?; + .map_err(|e| anyhow::anyhow!("Failed to create database file at {}: {}", db_path.display(), e))?; match SqlitePool::connect(&db_url).await { Ok(pool) => { - tracing::info!("created mostro db file: {}", db_url); + tracing::info!( + path = %db_path.display(), + "Successfully created Mostro database file" + ); match sqlx::migrate!().run(&pool).await { Ok(_) => (), Err(e) => { // Clean up the created file on migration failure if let Err(cleanup_err) = std::fs::remove_file(db_path) { - tracing::warn!("Failed to clean up db file: {}", cleanup_err); + tracing::warn!( + error = %cleanup_err, + path = %db_path.display(), + "Failed to clean up database file after migration failure" + ); } - return Err(anyhow::anyhow!("Failed to run migrations: {}", e)); + return Err(anyhow::anyhow!("Database migration failed: {}", e)); } } pool } Err(e) => { - tracing::error!("Error creating mostro db file: {}", e); - return Err(anyhow::anyhow!("Error creating mostro db file: {}", e)); + tracing::error!( + error = %e, + path = %db_path.display(), + "Failed to create database connection" + ); + return Err(anyhow::anyhow!("Failed to create database connection at {}: {}", db_path.display(), e)); } } } else { - SqlitePool::connect(&db_url).await? + SqlitePool::connect(&db_url) + .await + .map_err(|e| anyhow::anyhow!("Failed to connect to existing database at {}: {}", db_path.display(), e))? };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/db.rs(1 hunks)
🔇 Additional comments (1)
src/db.rs (1)
19-25: 🛠️ Refactor suggestionImprove database URL and path handling.
The current path handling could be improved for better reliability:
- Use proper path manipulation instead of string concatenation
- Validate the database directory exists before creating the file
- let mut db_url = db_settings.url; - db_url.push_str("mostro.db"); - // Remove sqlite:// from db_url - let tmp = db_url.replace("sqlite://", ""); - let db_path = Path::new(&tmp); + let base_path = Path::new(db_settings.url.trim_start_matches("sqlite://")); + if !base_path.exists() { + return Err(anyhow::anyhow!("Database directory does not exist: {}", base_path.display())); + } + let db_path = base_path.join("mostro.db"); + let db_url = format!("sqlite://{}", db_path.display());Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/db.rs (2)
19-33: Improve database path and URL handling.The current path and URL manipulation could be more robust. Consider using
PathBuffor path operations and a more structured approach for URL handling.let db_settings = Settings::get_db(); - let mut db_url = db_settings.url; - db_url.push_str("mostro.db"); - // Remove sqlite:// from db_url - let tmp = db_url.replace("sqlite://", ""); - let db_path = Path::new(&tmp); + let base_path = PathBuf::from(db_settings.url.trim_start_matches("sqlite://")); + let db_path = base_path.join("mostro.db"); + let db_url = format!("sqlite://{}", db_path.display());
34-71: Standardize error message format.While the error handling is robust, the error messages could be more consistent. Consider using a standard format for all database-related errors.
- return Err(anyhow::anyhow!( - "Failed to create database connection at {}: {}", - db_path.display(), - e - )); + return Err(anyhow::anyhow!( + "Database error: failed to create connection at {} - {}", + db_path.display(), + e + ));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/db.rs(2 hunks)
🔇 Additional comments (1)
src/db.rs (1)
332-332: LGTM! SQL query improvement.The SQL query has been simplified by removing the unnecessary condition, making it more readable and maintainable.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/cli/settings.rs(2 hunks)src/db.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cli/settings.rs
🔇 Additional comments (3)
src/db.rs (3)
2-2: LGTM! Import changes are appropriate.The addition of
anyhow::Resultandstd::path::Pathaligns well with the improved error handling and path manipulation in the codebase.Also applies to: 13-13
332-332: LGTM! SQL query condition is correct.The condition
invoice_held_at !=0 AND status == 'active'is properly formatted and logically sound.
Line range hint
1-600: Verify input validation for all user-provided data.While SQLx provides query sanitization, consider adding explicit input validation for all user-provided data before using it in database operations. This is particularly important for functions like
update_user_ratingandupdate_user_trade_index.Run this script to identify potential SQL injection vulnerabilities:
✅ Verification successful
All SQL queries use parameterized statements, no SQL injection vulnerabilities found
After analyzing the codebase, all SQL queries are properly using parameterized statements with SQLx's query builder and bind parameters (
?1,?2, etc.). No instances of string interpolation or direct concatenation in SQL queries were found. The code also includes proper input validation for critical functions:
update_user_rating: Validates rating values, public key format, and numerical constraintsupdate_user_trade_index: Validates public key format and trade index values- Other database operations use SQLx's type-safe query builder
The original review comment's concern about input validation has been addressed, as the code already implements proper validation and uses secure database access patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for direct string interpolation in SQL queries # Look for potential SQL injection vulnerabilities where user input might be directly interpolated # Search for string interpolation in SQL queries rg -n '(?i)execute\(\s*&*format!\s*\(.*SELECT|INSERT|UPDATE|DELETE.*\)' # Search for raw string literals in SQL queries that might contain user input rg -n '(?i)execute\(\s*&*format!\s*\(.*r#".*\$\{.*\}.*"#.*\)'Length of output: 16124
Hi @grunch ,
with this PR now
mostro.dbis automatically generated, the flow is this:So
.mostrofolder is created inHOMEor custom folder is created. User is warned that settings file is copied from template automatically in the folder and he has to configure it.mostrodagain we have:The error comes from not configuring the
settings.tomlcorrectly, so it's coherent.Tested on PopOs both with default Home folder and also with custom.
cc: @Catrya @bilthon
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements
Chores