-
Notifications
You must be signed in to change notification settings - Fork 12
feat: log proof log errors to db #64
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,7 @@ use std::fs; | |
| use std::path::Path; | ||
|
|
||
| pub const DEFAULT_DB_VERSION: u16 = 1; | ||
| pub const MIN_SUPPORTED_DB_VERSION: u16 = 1; | ||
| pub const CURRENT_DB_VERSION: u16 = 2; | ||
| pub const DEFAULT_NETWORK: &str = "dash"; | ||
|
|
||
| impl Database { | ||
|
|
@@ -16,17 +16,44 @@ impl Database { | |
| self.set_default_version()?; | ||
| } else { | ||
| // Perform version check and back up and recreate the database if outdated. | ||
| if self.is_outdated()? { | ||
| self.backup_and_recreate_db(db_file_path)?; | ||
| self.create_tables()?; | ||
| self.set_default_version()?; | ||
| println!("Database reinitialized with default settings."); | ||
| if let Some(current_version) = self.is_outdated()? { | ||
| self.backup_db(db_file_path)?; | ||
| if let Err(e) = self.try_perform_migration(current_version, CURRENT_DB_VERSION) { | ||
| // The migration failed | ||
| println!("Migration failed: {:?}", e); | ||
| self.recreate_db(db_file_path)?; | ||
| self.create_tables()?; | ||
| self.set_default_version()?; | ||
| println!("Database reinitialized with default settings."); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| fn apply_version_changes(&self, version: u16) -> rusqlite::Result<()> { | ||
| match version { | ||
| 2 => { | ||
| self.initialize_proof_log_table()?; | ||
| } | ||
| _ => {} | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
| fn try_perform_migration( | ||
| &self, | ||
| original_version: u16, | ||
| to_version: u16, | ||
| ) -> rusqlite::Result<()> { | ||
| for version in (original_version + 1)..=to_version { | ||
| self.apply_version_changes(version)?; | ||
| self.update_database_version(version)?; | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Checks if the `settings` table is empty or missing, indicating a first-time setup. | ||
| fn is_first_time_setup(&self) -> rusqlite::Result<bool> { | ||
| let conn = self.conn.lock().unwrap(); | ||
|
|
@@ -50,7 +77,7 @@ impl Database { | |
| } | ||
|
|
||
| /// Checks if the current database version is below the minimum supported version. | ||
| fn is_outdated(&self) -> rusqlite::Result<bool> { | ||
| fn is_outdated(&self) -> rusqlite::Result<Option<u16>> { | ||
| let conn = self.conn.lock().unwrap(); | ||
| let version: u16 = conn | ||
| .query_row( | ||
|
|
@@ -59,11 +86,15 @@ impl Database { | |
| |row| row.get(0), | ||
| ) | ||
| .unwrap_or(0); // Default to version 0 if there's no version set | ||
| Ok(version < MIN_SUPPORTED_DB_VERSION) | ||
| if version < CURRENT_DB_VERSION { | ||
| Ok(Some(version)) | ||
| } else { | ||
| Ok(None) | ||
| } | ||
| } | ||
|
|
||
| /// Backs up the existing database with a unique timestamped filename, recreates `data.db`, and refreshes the connection. | ||
| fn backup_and_recreate_db(&self, db_file_path: &Path) -> rusqlite::Result<()> { | ||
| fn backup_db(&self, db_file_path: &Path) -> rusqlite::Result<()> { | ||
| if db_file_path.exists() { | ||
| // Create a "backups" folder in the same directory as `data.db` if not exists | ||
| let backups_dir = db_file_path | ||
|
|
@@ -82,11 +113,25 @@ impl Database { | |
| let backup_path = backups_dir.join(backup_filename); | ||
|
|
||
| // Rename `data.db` to the unique backup file | ||
| fs::rename(db_file_path, &backup_path) | ||
| fs::copy(db_file_path, &backup_path) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Use appropriate error types for filesystem operations In Consider propagating the Apply this diff to adjust error handling: // In backup_db
fs::copy(db_file_path, &backup_path)
- .map_err(|e| rusqlite::Error::ToSqlConversionFailure(e.into()))?;
+ .map_err(|e| rusqlite::Error::Other(Box::new(e)))?;
// In recreate_db
fs::remove_file(db_file_path).map_err(|e| {
- rusqlite::Error::FromSqlConversionFailure(
- 0,
- rusqlite::types::Type::Text,
- Box::new(e),
- )
+ rusqlite::Error::Other(Box::new(e))
})?;Alternatively, if you decide to define a custom error type: #[derive(Debug)]
enum DatabaseError {
IoError(std::io::Error),
SqliteError(rusqlite::Error),
// Add other variants as needed
}
impl From<std::io::Error> for DatabaseError {
fn from(error: std::io::Error) -> Self {
DatabaseError::IoError(error)
}
}
impl From<rusqlite::Error> for DatabaseError {
fn from(error: rusqlite::Error) -> Self {
DatabaseError::SqliteError(error)
}
}Then update your method signatures to return Also applies to: 129-135 |
||
| .map_err(|e| rusqlite::Error::ToSqlConversionFailure(e.into()))?; | ||
| println!("Old database backed up to {:?}", backup_path); | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Backs up the existing database with a unique timestamped filename, recreates `data.db`, and refreshes the connection. | ||
| fn recreate_db(&self, db_file_path: &Path) -> rusqlite::Result<()> { | ||
| // Remove the existing database file if it exists | ||
| if db_file_path.exists() { | ||
| fs::remove_file(db_file_path).map_err(|e| { | ||
| rusqlite::Error::FromSqlConversionFailure( | ||
| 0, | ||
| rusqlite::types::Type::Text, | ||
| Box::new(e), | ||
| ) | ||
| })?; | ||
| } | ||
| // Create a new empty `data.db` file and set up the initial schema | ||
| let new_conn = Connection::open(db_file_path)?; | ||
|
|
||
|
|
@@ -291,6 +336,8 @@ impl Database { | |
| [], | ||
| )?; | ||
|
|
||
| self.initialize_proof_log_table()?; | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,125 @@ | ||||||||||||||||||||||||||||||
| use crate::database::Database; | ||||||||||||||||||||||||||||||
| use crate::model::proof_log_item::{ProofLogItem, RequestType}; | ||||||||||||||||||||||||||||||
| use rusqlite::params; | ||||||||||||||||||||||||||||||
| use std::ops::Range; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| impl Database { | ||||||||||||||||||||||||||||||
| pub fn initialize_proof_log_table(&self) -> rusqlite::Result<()> { | ||||||||||||||||||||||||||||||
| // Create the proof log tree | ||||||||||||||||||||||||||||||
| self.execute( | ||||||||||||||||||||||||||||||
| "CREATE TABLE IF NOT EXISTS proof_log ( | ||||||||||||||||||||||||||||||
| proof_id INTEGER PRIMARY KEY AUTOINCREMENT, | ||||||||||||||||||||||||||||||
| request_type INTEGER NOT NULL, | ||||||||||||||||||||||||||||||
| request_bytes BLOB NOT NULL, | ||||||||||||||||||||||||||||||
| height INTEGER NOT NULL, | ||||||||||||||||||||||||||||||
| time_ms INTEGER NOT NULL, | ||||||||||||||||||||||||||||||
| proof_bytes BLOB NOT NULL, | ||||||||||||||||||||||||||||||
| error TEXT | ||||||||||||||||||||||||||||||
| )", | ||||||||||||||||||||||||||||||
| [], | ||||||||||||||||||||||||||||||
| )?; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Create an index on request_type and time for combined queries | ||||||||||||||||||||||||||||||
| self.execute( | ||||||||||||||||||||||||||||||
| "CREATE INDEX IF NOT EXISTS idx_proof_log_request_type_time ON proof_log (request_type, time_ms)", | ||||||||||||||||||||||||||||||
| [], | ||||||||||||||||||||||||||||||
| )?; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Create an index on time for queries ordered by time | ||||||||||||||||||||||||||||||
| self.execute( | ||||||||||||||||||||||||||||||
| "CREATE INDEX IF NOT EXISTS idx_proof_log_time ON proof_log (time_ms)", | ||||||||||||||||||||||||||||||
| [], | ||||||||||||||||||||||||||||||
| )?; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Index for error, request_type, and time | ||||||||||||||||||||||||||||||
| self.execute( | ||||||||||||||||||||||||||||||
| "CREATE INDEX IF NOT EXISTS idx_proof_log_error_request_type_time ON proof_log (error, request_type, time_ms)", | ||||||||||||||||||||||||||||||
| [], | ||||||||||||||||||||||||||||||
| )?; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Index for error and time | ||||||||||||||||||||||||||||||
| self.execute( | ||||||||||||||||||||||||||||||
| "CREATE INDEX IF NOT EXISTS idx_proof_log_error_time ON proof_log (error, time_ms)", | ||||||||||||||||||||||||||||||
| [], | ||||||||||||||||||||||||||||||
| )?; | ||||||||||||||||||||||||||||||
| Ok(()) | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /// Inserts a new ProofLogItem into the proof_log table | ||||||||||||||||||||||||||||||
| pub fn insert_proof_log_item(&self, item: ProofLogItem) -> rusqlite::Result<()> { | ||||||||||||||||||||||||||||||
| let conn = self.conn.lock().unwrap(); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Convert RequestType to u8 | ||||||||||||||||||||||||||||||
| let request_type_int: u8 = item.request_type.into(); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| conn.execute( | ||||||||||||||||||||||||||||||
| "INSERT INTO proof_log (request_type, request_bytes, height, time_ms, proof_bytes, error) | ||||||||||||||||||||||||||||||
| VALUES (?, ?, ?, ?, ?, ?)", | ||||||||||||||||||||||||||||||
| params![ | ||||||||||||||||||||||||||||||
| request_type_int, | ||||||||||||||||||||||||||||||
| item.request_bytes, | ||||||||||||||||||||||||||||||
| item.height, | ||||||||||||||||||||||||||||||
| item.time_ms, | ||||||||||||||||||||||||||||||
| item.proof_bytes, | ||||||||||||||||||||||||||||||
| item.error, | ||||||||||||||||||||||||||||||
| ], | ||||||||||||||||||||||||||||||
| )?; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| Ok(()) | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /// Retrieves ProofLogItems with options for filtering and pagination | ||||||||||||||||||||||||||||||
| pub fn get_proof_log_items( | ||||||||||||||||||||||||||||||
| &self, | ||||||||||||||||||||||||||||||
| only_get_errored: bool, | ||||||||||||||||||||||||||||||
| range: Range<u64>, | ||||||||||||||||||||||||||||||
| ) -> rusqlite::Result<Vec<ProofLogItem>> { | ||||||||||||||||||||||||||||||
| let conn = self.conn.lock().unwrap(); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Build the query based on the only_get_errored flag | ||||||||||||||||||||||||||||||
| let mut query = String::from( | ||||||||||||||||||||||||||||||
| "SELECT request_type, request_bytes, height, time_ms, proof_bytes, error FROM proof_log", | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| if only_get_errored { | ||||||||||||||||||||||||||||||
| query.push_str(" WHERE error IS NOT NULL"); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| query.push_str(" ORDER BY time_ms DESC LIMIT ? OFFSET ?"); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| let mut stmt = conn.prepare(&query)?; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| let proof_log_iter = | ||||||||||||||||||||||||||||||
| stmt.query_map(params![range.end - range.start, range.start], |row| { | ||||||||||||||||||||||||||||||
| let request_type_int: u8 = row.get(0)?; | ||||||||||||||||||||||||||||||
| let request_bytes: Vec<u8> = row.get(1)?; | ||||||||||||||||||||||||||||||
| let height: u64 = row.get(2)?; | ||||||||||||||||||||||||||||||
| let time_ms: u64 = row.get(3)?; | ||||||||||||||||||||||||||||||
| let proof_bytes: Vec<u8> = row.get(4)?; | ||||||||||||||||||||||||||||||
| let error: Option<String> = row.get(5)?; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Convert u8 to RequestType | ||||||||||||||||||||||||||||||
| let request_type = RequestType::try_from(request_type_int).map_err(|_| { | ||||||||||||||||||||||||||||||
| rusqlite::Error::FromSqlConversionFailure( | ||||||||||||||||||||||||||||||
| request_type_int as usize, | ||||||||||||||||||||||||||||||
| rusqlite::types::Type::Integer, | ||||||||||||||||||||||||||||||
| Box::new(std::fmt::Error), | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
| })?; | ||||||||||||||||||||||||||||||
|
Comment on lines
+102
to
+108
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Use a more appropriate error type in The error handling when converting Consider applying the following change: let request_type = RequestType::try_from(request_type_int).map_err(|_| {
rusqlite::Error::FromSqlConversionFailure(
request_type_int as usize,
rusqlite::types::Type::Integer,
- Box::new(std::fmt::Error),
+ Box::new(std::io::Error::new(std::io::ErrorKind::InvalidData, format!("Invalid RequestType value: {}", request_type_int))),
)
})?;📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| Ok(ProofLogItem { | ||||||||||||||||||||||||||||||
| request_type, | ||||||||||||||||||||||||||||||
| request_bytes, | ||||||||||||||||||||||||||||||
| height, | ||||||||||||||||||||||||||||||
| time_ms, | ||||||||||||||||||||||||||||||
| proof_bytes, | ||||||||||||||||||||||||||||||
| error, | ||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||
| })?; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Collect the results into a vector | ||||||||||||||||||||||||||||||
| let items: rusqlite::Result<Vec<ProofLogItem>> = proof_log_iter.collect(); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| items | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| pub mod contested_name; | ||
| pub mod password_info; | ||
| pub mod proof_log_item; | ||
| pub mod qualified_contract; | ||
| pub mod qualified_identity; | ||
| pub mod wallet; |
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.
💡 Codebase verification
Using test/testWithoutSpan2 branch is risky for production dependency
Based on the verification results:
Recommendations:
a78c7b06ef6522478c22c3116632ceef483eee85)🔗 Analysis chain
Verify the stability and production readiness of the test branch.
Switching from a fixed commit hash (
994f7627080e4ab490f1dbf0f09c0fb37d0fbbb0) to a test branch (test/testWithoutSpan2) raises several concerns:Consider either:
Let's verify the branch status and recent changes:
Let me gather more information about the relationship between this test branch and the main development branches.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 1104
Script:
Length of output: 992