From 0ff09e55c7dea13b7a6171a865ca4231185b1e75 Mon Sep 17 00:00:00 2001 From: dignifiedquire Date: Sun, 7 Jul 2019 21:05:47 +0200 Subject: [PATCH] more improvements in sql code --- src/dc_chatlist.rs | 112 ++++++++++++++------------ src/dc_imex.rs | 158 ++++++++++++++++++------------------ src/dc_sqlite3.rs | 194 +++++++++++++++++++-------------------------- src/error.rs | 23 ++++++ src/lib.rs | 1 + 5 files changed, 240 insertions(+), 248 deletions(-) create mode 100644 src/error.rs diff --git a/src/dc_chatlist.rs b/src/dc_chatlist.rs index a029c5328..d791cd134 100644 --- a/src/dc_chatlist.rs +++ b/src/dc_chatlist.rs @@ -148,8 +148,9 @@ unsafe fn dc_chatlist_load_from_db( Ok(()) }; - let success = if query_contact_id != 0 { - (*chatlist).context.sql.query_map( + let success = + if query_contact_id != 0 { + (*chatlist).context.sql.query_map( "SELECT c.id, m.id FROM chats c LEFT JOIN msgs m \ ON c.id=m.chat_id \ AND m.timestamp=( SELECT MAX(timestamp) \ @@ -159,64 +160,73 @@ unsafe fn dc_chatlist_load_from_db( GROUP BY c.id ORDER BY IFNULL(m.timestamp,0) DESC, m.id DESC;", params![query_contact_id as i32], process_fn, - |res| res.collect::>>(), + |res| res.collect::>>().map_err(Into::into), ) - } else if 0 != listflags & 0x1 { - (*chatlist).context.sql.query_map( - "SELECT c.id, m.id FROM chats c LEFT JOIN msgs m \ - ON c.id=m.chat_id \ - AND m.timestamp=( SELECT MAX(timestamp) \ - FROM msgs WHERE chat_id=c.id \ - AND (hidden=0 OR (hidden=1 AND state=19))) WHERE c.id>9 \ - AND c.blocked=0 AND c.archived=1 GROUP BY c.id \ - ORDER BY IFNULL(m.timestamp,0) DESC, m.id DESC;", - params![], - process_fn, - |res| res.collect::>>(), - ) - } else if query__.is_null() { - if 0 == listflags & 0x2 { - let last_deaddrop_fresh_msg_id = get_last_deaddrop_fresh_msg((*chatlist).context); - if last_deaddrop_fresh_msg_id > 0 { - dc_array_add_id((*chatlist).chatNlastmsg_ids, 1); - dc_array_add_id((*chatlist).chatNlastmsg_ids, last_deaddrop_fresh_msg_id); - } - add_archived_link_item = 1; - } - (*chatlist).context.sql.query_map( - "SELECT c.id, m.id FROM chats c \ - LEFT JOIN msgs m \ - ON c.id=m.chat_id \ - AND m.timestamp=( SELECT MAX(timestamp) \ - FROM msgs WHERE chat_id=c.id \ - AND (hidden=0 OR (hidden=1 AND state=19))) WHERE c.id>9 \ - AND c.blocked=0 AND c.archived=0 \ - GROUP BY c.id \ - ORDER BY IFNULL(m.timestamp,0) DESC, m.id DESC;", - params![], - process_fn, - |res| res.collect::>>(), - ) - } else { - let query = to_string(query__).trim().to_string(); - if query.is_empty() { - return 1; - } else { - let strLikeCmd = format!("%{}%", query); + } else if 0 != listflags & 0x1 { (*chatlist).context.sql.query_map( "SELECT c.id, m.id FROM chats c LEFT JOIN msgs m \ ON c.id=m.chat_id \ AND m.timestamp=( SELECT MAX(timestamp) \ FROM msgs WHERE chat_id=c.id \ AND (hidden=0 OR (hidden=1 AND state=19))) WHERE c.id>9 \ - AND c.blocked=0 AND c.name LIKE ? \ - GROUP BY c.id ORDER BY IFNULL(m.timestamp,0) DESC, m.id DESC;", - params![strLikeCmd], + AND c.blocked=0 AND c.archived=1 GROUP BY c.id \ + ORDER BY IFNULL(m.timestamp,0) DESC, m.id DESC;", + params![], process_fn, - |res| res.collect::>>(), + |res| { + res.collect::>>() + .map_err(Into::into) + }, ) - } - }; + } else if query__.is_null() { + if 0 == listflags & 0x2 { + let last_deaddrop_fresh_msg_id = get_last_deaddrop_fresh_msg((*chatlist).context); + if last_deaddrop_fresh_msg_id > 0 { + dc_array_add_id((*chatlist).chatNlastmsg_ids, 1); + dc_array_add_id((*chatlist).chatNlastmsg_ids, last_deaddrop_fresh_msg_id); + } + add_archived_link_item = 1; + } + (*chatlist).context.sql.query_map( + "SELECT c.id, m.id FROM chats c \ + LEFT JOIN msgs m \ + ON c.id=m.chat_id \ + AND m.timestamp=( SELECT MAX(timestamp) \ + FROM msgs WHERE chat_id=c.id \ + AND (hidden=0 OR (hidden=1 AND state=19))) WHERE c.id>9 \ + AND c.blocked=0 AND c.archived=0 \ + GROUP BY c.id \ + ORDER BY IFNULL(m.timestamp,0) DESC, m.id DESC;", + params![], + process_fn, + |res| { + res.collect::>>() + .map_err(Into::into) + }, + ) + } else { + let query = to_string(query__).trim().to_string(); + if query.is_empty() { + return 1; + } else { + let strLikeCmd = format!("%{}%", query); + (*chatlist).context.sql.query_map( + "SELECT c.id, m.id FROM chats c LEFT JOIN msgs m \ + ON c.id=m.chat_id \ + AND m.timestamp=( SELECT MAX(timestamp) \ + FROM msgs WHERE chat_id=c.id \ + AND (hidden=0 OR (hidden=1 AND state=19))) WHERE c.id>9 \ + AND c.blocked=0 AND c.name LIKE ? \ + GROUP BY c.id ORDER BY IFNULL(m.timestamp,0) DESC, m.id DESC;", + params![strLikeCmd], + process_fn, + |res| { + res.collect::>>() + .map_err(Into::into) + }, + ) + } + }; if 0 != add_archived_link_item && dc_get_archived_cnt((*chatlist).context) > 0 { if dc_array_get_cnt((*chatlist).chatNlastmsg_ids) == 0 && 0 != listflags & 0x4 { diff --git a/src/dc_imex.rs b/src/dc_imex.rs index a593c9032..2aaa29172 100644 --- a/src/dc_imex.rs +++ b/src/dc_imex.rs @@ -1,5 +1,6 @@ use std::ffi::CString; +use failure::format_err; use mmime::mailmime_content::*; use mmime::mmapstring::*; use mmime::other::*; @@ -798,11 +799,6 @@ pub unsafe fn dc_job_do_DC_JOB_IMEX_IMAP(context: &Context, job: *mut dc_job_t) // TODO should return bool /rtn unsafe fn import_backup(context: &Context, backup_to_import: *const libc::c_char) -> libc::c_int { - let mut success = 0; - let mut processed_files_cnt = 0; - - let total_files_cnt: usize; - info!( context, 0, @@ -813,49 +809,54 @@ unsafe fn import_backup(context: &Context, backup_to_import: *const libc::c_char if 0 != dc_is_configured(context) { error!(context, 0, "Cannot import backups to accounts in use."); - } else { - &context.sql.close(&context); - dc_delete_file(context, context.get_dbfile()); - if 0 != dc_file_exist(context, context.get_dbfile()) { - error!( - context, - 0, "Cannot import backups: Cannot delete the old file.", - ); - } else if !(0 == dc_copy_file(context, backup_to_import, context.get_dbfile())) { - /* error already logged */ - /* re-open copied database file */ - if context.sql.open(&context, as_path(context.get_dbfile()), 0) { - total_files_cnt = dc_sqlite3_query_row::<_, isize>( - context, - &context.sql, - "SELECT COUNT(*) FROM backup_blobs;", - params![], - 0, - ) - .unwrap_or_default() as usize; - info!( - context, - 0, "***IMPORT-in-progress: total_files_cnt={:?}", total_files_cnt, - ); + return 0; + } + &context.sql.close(&context); + dc_delete_file(context, context.get_dbfile()); + if 0 != dc_file_exist(context, context.get_dbfile()) { + error!( + context, + 0, "Cannot import backups: Cannot delete the old file.", + ); + return 0; + } - let files = if let Some(mut stmt) = dc_sqlite3_prepare( - context, - &context.sql, - "SELECT file_name, file_content FROM backup_blobs ORDER BY id;", - ) { - stmt.query_map(params![], |row| { - let name: String = row.get(0)?; - let blob: Vec = row.get(1)?; + if 0 == dc_copy_file(context, backup_to_import, context.get_dbfile()) { + return 0; + } + /* error already logged */ + /* re-open copied database file */ + if !context.sql.open(&context, as_path(context.get_dbfile()), 0) { + return 0; + } - Ok((name, blob)) - }) - .map(|res| res.collect::>()) - .unwrap() - } else { - panic!("invalid sql"); - }; + let total_files_cnt = dc_sqlite3_query_row::<_, isize>( + context, + &context.sql, + "SELECT COUNT(*) FROM backup_blobs;", + params![], + 0, + ) + .unwrap_or_default() as usize; + info!( + context, + 0, "***IMPORT-in-progress: total_files_cnt={:?}", total_files_cnt, + ); + context + .sql + .query_map( + "SELECT file_name, file_content FROM backup_blobs ORDER BY id;", + params![], + |row| { + let name: String = row.get(0)?; + let blob: Vec = row.get(1)?; + + Ok((name, blob)) + }, + |files| { let mut loop_success = true; + let mut processed_files_cnt = 0; for file in files { if file.is_err() { @@ -904,21 +905,15 @@ unsafe fn import_backup(context: &Context, backup_to_import: *const libc::c_char break; } - if loop_success { - dc_sqlite3_execute( - context, - &context.sql, - "DROP TABLE backup_blobs;", - params![], - ); - dc_sqlite3_try_execute(context, &context.sql, "VACUUM;"); - success = 1; + if !loop_success { + return Err(format_err!("fail").into()); } - } - } - } - - success + dc_sqlite3_execute(context, &context.sql, "DROP TABLE backup_blobs;", params![]); + dc_sqlite3_try_execute(context, &context.sql, "VACUUM;"); + Ok(()) + }, + ) + .is_ok() as libc::c_int } /******************************************************************************* @@ -1266,25 +1261,24 @@ unsafe fn import_self_keys(context: &Context, dir_name: *const libc::c_char) -> unsafe fn export_self_keys(context: &Context, dir: *const libc::c_char) -> libc::c_int { let mut export_errors = 0; - if let Some(mut stmt) = dc_sqlite3_prepare( - context, - &context.sql, - "SELECT id, public_key, private_key, is_default FROM keypairs;", - ) { - let rows = stmt.query_map(params![], |row| { - let id = row.get(0)?; - let public_key_blob: Vec = row.get(1)?; - let public_key = Key::from_slice(&public_key_blob, KeyType::Public); - let private_key_blob: Vec = row.get(2)?; - let private_key = Key::from_slice(&private_key_blob, KeyType::Private); - let is_default = row.get(3)?; + context + .sql + .query_map( + "SELECT id, public_key, private_key, is_default FROM keypairs;", + params![], + |row| { + let id = row.get(0)?; + let public_key_blob: Vec = row.get(1)?; + let public_key = Key::from_slice(&public_key_blob, KeyType::Public); + let private_key_blob: Vec = row.get(2)?; + let private_key = Key::from_slice(&private_key_blob, KeyType::Private); + let is_default = row.get(3)?; - Ok((id, public_key, private_key, is_default)) - }); - - if let Ok(keys) = rows { - for key_pair in keys { - if let Ok((id, public_key, private_key, is_default)) = key_pair { + Ok((id, public_key, private_key, is_default)) + }, + |keys| { + for key_pair in keys { + let (id, public_key, private_key, is_default) = key_pair?; if let Some(key) = public_key { if 0 == export_key_to_asc_file(context, dir, id, &key, is_default) { export_errors += 1; @@ -1300,13 +1294,11 @@ unsafe fn export_self_keys(context: &Context, dir: *const libc::c_char) -> libc: export_errors += 1; } } - } - } else { - return 1; - } - } else { - return 1; - } + + Ok(()) + }, + ) + .unwrap(); if export_errors == 0 { 1 diff --git a/src/dc_sqlite3.rs b/src/dc_sqlite3.rs index 92dadd3eb..a709a15d1 100644 --- a/src/dc_sqlite3.rs +++ b/src/dc_sqlite3.rs @@ -6,6 +6,7 @@ use crate::constants::*; use crate::context::Context; use crate::dc_param::*; use crate::dc_tools::*; +use crate::error::Result; use crate::peerstate::*; use crate::x::*; @@ -44,13 +45,13 @@ impl SQLite { } } - pub fn execute

(&self, sql: &str, params: P) -> rusqlite::Result + pub fn execute

(&self, sql: &str, params: P) -> Result where P: IntoIterator, P::Item: rusqlite::ToSql, { match &*self.connection.read().unwrap() { - Some(conn) => conn.execute(sql, params), + Some(conn) => conn.execute(sql, params).map_err(Into::into), None => panic!("Querying closed SQLite database"), } } @@ -62,18 +63,12 @@ impl SQLite { /// Prepares and executes the statement and maps a function over the resulting rows. /// Then executes the second function over the returned iterator and returns the /// result of that function. - pub fn query_map( - &self, - sql: &str, - params: P, - f: F, - mut g: G, - ) -> rusqlite::Result + pub fn query_map(&self, sql: &str, params: P, f: F, mut g: G) -> Result where P: IntoIterator, P::Item: rusqlite::ToSql, F: FnMut(&rusqlite::Row) -> rusqlite::Result, - G: FnMut(rusqlite::MappedRows) -> rusqlite::Result, + G: FnMut(rusqlite::MappedRows) -> Result, { let conn_lock = self.connection.read().unwrap(); let conn = conn_lock.as_ref().expect("database closed"); @@ -85,7 +80,7 @@ impl SQLite { /// Return `true` if a query in the SQL statement it executes returns one or more /// rows and false if the SQL returns an empty set. - pub fn exists

(&self, sql: &str, params: P) -> rusqlite::Result + pub fn exists

(&self, sql: &str, params: P) -> Result where P: IntoIterator, P::Item: rusqlite::ToSql, @@ -98,14 +93,14 @@ impl SQLite { Ok(res) } - pub fn query_row(&self, sql: &str, params: P, f: F) -> rusqlite::Result + pub fn query_row(&self, sql: &str, params: P, f: F) -> Result where P: IntoIterator, P::Item: rusqlite::ToSql, F: FnOnce(&rusqlite::Row) -> rusqlite::Result, { match &*self.connection.read().unwrap() { - Some(conn) => conn.query_row(sql, params, f), + Some(conn) => conn.query_row(sql, params, f).map_err(Into::into), None => panic!("Querying closed SQLite database"), } } @@ -127,7 +122,7 @@ impl SQLite { // Return 1 -> success // Return 0 -> failure -pub fn dc_sqlite3_open( +fn dc_sqlite3_open( context: &Context, sql: &SQLite, dbfile: impl AsRef, @@ -773,26 +768,23 @@ pub fn dc_sqlite3_open( } if 0 != recalc_fingerprints { - let rows = if let Some(mut stmt) = - dc_sqlite3_prepare(context, sql, "SELECT addr FROM acpeerstates;") - { - stmt.query_map(params![], |row| row.get::<_, String>(0)) - .and_then(|res| res.collect::>>()) - .ok() - } else { - None - }; - - if let Some(addrs) = rows { - for addr in addrs { - if let Some(ref mut peerstate) = - Peerstate::from_addr(context, sql, &addr) - { - peerstate.recalc_fingerprint(); - peerstate.save_to_db(sql, false); + sql.query_map( + "SELECT addr FROM acpeerstates;", + params![], + |row| row.get::<_, String>(0), + |addrs| { + for addr in addrs { + if let Some(ref mut peerstate) = + Peerstate::from_addr(context, sql, &addr?) + { + peerstate.recalc_fingerprint(); + peerstate.save_to_db(sql, false); + } } - } - } + Ok(()) + }, + ) + .unwrap(); } if 0 != update_file_paths { let repl_from = dc_sqlite3_get_config( @@ -854,28 +846,23 @@ pub fn dc_sqlite3_set_config( let good; if let Some(ref value) = value { - if let Some(exists) = - dc_sqlite3_prepare(context, sql, "SELECT value FROM config WHERE keyname=?;") - .and_then(|mut stmt| stmt.exists(params![]).ok()) - { - if exists { - good = dc_sqlite3_execute( - context, - sql, - "UPDATE config SET value=? WHERE keyname=?;", - params![value, key], - ); - } else { - good = dc_sqlite3_execute( - context, - sql, - "INSERT INTO config (keyname, value) VALUES (?, ?);", - params![key, value], - ); - } + let exists = sql + .exists("SELECT value FROM config WHERE keyname=?;", params![]) + .unwrap_or_default(); + if exists { + good = dc_sqlite3_execute( + context, + sql, + "UPDATE config SET value=? WHERE keyname=?;", + params![value, key], + ); } else { - error!(context, 0, "dc_sqlite3_set_config(): Cannot read value.",); - return 0; + good = dc_sqlite3_execute( + context, + sql, + "INSERT INTO config (keyname, value) VALUES (?, ?);", + params![key, value], + ); } } else { good = dc_sqlite3_execute( @@ -896,19 +883,14 @@ pub fn dc_sqlite3_set_config( // TODO: Remove the option from the return type pub fn dc_sqlite3_prepare<'a>( - context: &Context, - sql: &'a SQLite, - querystr: &'a str, + _context: &Context, + _sql: &'a SQLite, + _querystr: &'a str, ) -> Option> { // TODO: remove once it is not used anymore unimplemented!() } -pub fn dc_sqlite3_is_open(sql: &SQLite) -> libc::c_int { - sql.is_open() as libc::c_int -} - -/* the returned string must be free()'d, returns NULL on errors */ pub fn dc_sqlite3_get_config( context: &Context, sql: &SQLite, @@ -929,7 +911,7 @@ pub fn dc_sqlite3_get_config( } pub fn dc_sqlite3_execute

( - context: &Context, + _context: &Context, sql: &SQLite, querystr: impl AsRef, params: P, @@ -938,23 +920,7 @@ where P: IntoIterator, P::Item: rusqlite::ToSql, { - if let Some(mut stmt) = dc_sqlite3_prepare(context, sql, querystr.as_ref()) { - match stmt.execute(params) { - Ok(_) => true, - Err(err) => { - error!( - context, - 0, - "Cannot execute \"{}\". ({})", - querystr.as_ref(), - err - ); - false - } - } - } else { - false - } + sql.execute(querystr.as_ref(), params).is_ok() } // TODO Remove the Option<> from the return type. @@ -1036,22 +1002,18 @@ pub fn dc_sqlite3_try_execute( querystr: impl AsRef, ) -> libc::c_int { // same as dc_sqlite3_execute() but does not pass error to ui - if let Some(mut stmt) = dc_sqlite3_prepare(context, sql, querystr.as_ref()) { - match stmt.execute(params![]) { - Ok(_) => 1, - Err(err) => { - warn!( - context, - 0, - "Try-execute for \"{}\" failed: {}", - querystr.as_ref(), - err, - ); - 0 - } + match sql.execute(querystr.as_ref(), params![]) { + Ok(_) => 1, + Err(err) => { + warn!( + context, + 0, + "Try-execute for \"{}\" failed: {}", + querystr.as_ref(), + err, + ); + 0 } - } else { - 0 } } @@ -1146,18 +1108,23 @@ pub fn dc_housekeeping(context: &Context) { 'i' as i32, ); - if let Some(mut stmt) = dc_sqlite3_prepare(context, &context.sql, "SELECT value FROM config;") { - match stmt.query_map(params![], |row| row.get::<_, String>(0)) { - Ok(rows) => { + context + .sql + .query_map( + "SELECT value FROM config;", + params![], + |row| row.get::<_, String>(0), + |rows| { for row in rows { - maybe_add_file(&mut files_in_use, row.expect("invalid sql")); + maybe_add_file(&mut files_in_use, row?); } - } - Err(err) => { - warn!(context, 0, "sql: failed query: {}", err); - } - } - } + Ok(()) + }, + ) + .unwrap_or_else(|err| { + warn!(context, 0, "sql: failed query: {}", err); + }); + info!(context, 0, "{} files in use.", files_in_use.len(),); /* go through directory and delete unused files */ let p = std::path::Path::new(as_str(context.get_blobdir())); @@ -1285,8 +1252,9 @@ fn maybe_add_from_param( ) { let param = unsafe { dc_param_new() }; - if let Some(ref mut stmt) = dc_sqlite3_prepare(context, &context.sql, query) { - match stmt.query_row(NO_PARAMS, |row| { + context + .sql + .query_row(query, NO_PARAMS, |row| { let v = to_cstring(row.get::<_, String>(0)?); unsafe { dc_param_set_packed(param, v.as_ptr() as *const libc::c_char); @@ -1297,13 +1265,11 @@ fn maybe_add_from_param( } } Ok(()) - }) { - Ok(_) => {} - Err(err) => { - warn!(context, 0, "sql: failed to add_from_param: {}", err); - } - } - } + }) + .unwrap_or_else(|err| { + warn!(context, 0, "sql: failed to add_from_param: {}", err); + }); + unsafe { dc_param_unref(param) }; } diff --git a/src/error.rs b/src/error.rs new file mode 100644 index 000000000..907899f33 --- /dev/null +++ b/src/error.rs @@ -0,0 +1,23 @@ +use failure::Fail; + +#[derive(Debug, Fail)] +pub enum Error { + #[fail(display = "Sqlite Error: {:?}", _0)] + Sql(rusqlite::Error), + #[fail(display = "{:?}", _0)] + Failure(failure::Error), +} + +pub type Result = std::result::Result; + +impl From for Error { + fn from(err: rusqlite::Error) -> Error { + Error::Sql(err) + } +} + +impl From for Error { + fn from(err: failure::Error) -> Error { + Error::Failure(err) + } +} diff --git a/src/lib.rs b/src/lib.rs index f95276fb2..a27956106 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -23,6 +23,7 @@ pub mod dc_log; pub mod aheader; pub mod constants; pub mod context; +pub mod error; pub mod imap; pub mod key; pub mod keyhistory;