From fce9b911f4a9f994f71477bf3b63cd79c65a787e Mon Sep 17 00:00:00 2001 From: Tim Serong Date: Thu, 16 Nov 2023 18:19:52 +1100 Subject: [PATCH] Use execptions for sqlite errors that should never happen Signed-off-by: Tim Serong --- src/checks/metadata_schema_version.cc | 17 +++-------- src/checks/object_integrity.cc | 11 ++----- src/checks/orphaned_metadata.cc | 10 ++---- src/checks/orphaned_objects.cc | 44 ++++++++++++--------------- src/main.cc | 8 ++++- src/sqlite.cc | 31 ++++++++----------- src/sqlite.h | 2 +- 7 files changed, 48 insertions(+), 75 deletions(-) diff --git a/src/checks/metadata_schema_version.cc b/src/checks/metadata_schema_version.cc index 814cb95..34577f3 100644 --- a/src/checks/metadata_schema_version.cc +++ b/src/checks/metadata_schema_version.cc @@ -27,21 +27,14 @@ std::string MetadataSchemaVersionFix::to_string() const { } bool MetadataSchemaVersionCheck::do_check() { - std::string query = "PRAGMA user_version;"; int version = 0; - int rc = 0; - sqlite3_stmt* stm; - rc = metadata->prepare(query, &stm); - if (rc != SQLITE_OK) { - return 1; - } - rc = sqlite3_step(stm); - if (rc == SQLITE_ROW && sqlite3_column_count(stm) > 0) { + sqlite3_stmt* stm = metadata->prepare("PRAGMA user_version;"); + if (sqlite3_step(stm) == SQLITE_ROW && sqlite3_column_count(stm) > 0) { version = sqlite3_column_int(stm, 0); } else { - // FIXME: this should probably raise an exception (at the - // very least we shouldn't be printing anything directly here) - std::cout << "Error" << sqlite3_errmsg(metadata->handle) << std::endl; + const char* err = sqlite3_errmsg(metadata->handle); + sqlite3_finalize(stm); + throw std::runtime_error(err); } sqlite3_finalize(stm); log_verbose("Got schema version " + std::to_string(version)); diff --git a/src/checks/object_integrity.cc b/src/checks/object_integrity.cc index a2257cd..24177cd 100644 --- a/src/checks/object_integrity.cc +++ b/src/checks/object_integrity.cc @@ -36,16 +36,9 @@ bool ObjectIntegrityCheck::do_check() { std::string query = "SELECT object_id, id, checksum, size FROM versioned_objects " "WHERE object_id IS NOT NULL;"; - int rc = 0; - sqlite3_stmt* stm; - rc = metadata->prepare(query, &stm); - if (rc != SQLITE_OK) { - // TODO: this needs an error messages - return false; - } - - rc = sqlite3_step(stm); + sqlite3_stmt* stm = metadata->prepare(query); + int rc = sqlite3_step(stm); while (rc == SQLITE_ROW && sqlite3_column_count(stm) > 0) { std::string uuid{ reinterpret_cast(sqlite3_column_text(stm, 0))}; diff --git a/src/checks/orphaned_metadata.cc b/src/checks/orphaned_metadata.cc index 0eb0e58..a48f26c 100644 --- a/src/checks/orphaned_metadata.cc +++ b/src/checks/orphaned_metadata.cc @@ -37,15 +37,9 @@ bool OrphanedMetadataCheck::do_check() { std::string query = "SELECT object_id, id FROM versioned_objects WHERE object_id IS NOT " "NULL;"; - int rc = 0; - sqlite3_stmt* stm; + sqlite3_stmt* stm = metadata->prepare(query); - rc = metadata->prepare(query, &stm); - if (rc != SQLITE_OK) { - return rc; - } - - rc = sqlite3_step(stm); + int rc = sqlite3_step(stm); while (rc == SQLITE_ROW && sqlite3_column_count(stm) > 0) { std::string uuid{ reinterpret_cast(sqlite3_column_text(stm, 0))}; diff --git a/src/checks/orphaned_objects.cc b/src/checks/orphaned_objects.cc index bf48f01..e45f849 100644 --- a/src/checks/orphaned_objects.cc +++ b/src/checks/orphaned_objects.cc @@ -126,34 +126,28 @@ bool OrphanedObjectsCheck::do_check() { " AND " " path_uuid = '" + uuid + "'"; - sqlite3_stmt* stm; - if (metadata->prepare(query, &stm) == SQLITE_OK) { - if (sqlite3_step(stm) == SQLITE_ROW && - sqlite3_column_count(stm) > 0) { - int count = sqlite3_column_int(stm, 0); - if (count == 0) { - fixes.emplace_back( - // TODO: Consider making an OrphanedMultipartFix class. - // OrphanedOjectsFix works fine, but the messaging might - // be slightly misleading ("orphaned object: uuid-n ..." - // vs. what would be "orhpaned multipart part: ...") - std::make_shared( - root_path, rel.string() - ) - ); - orphan_count++; - } - } else { - std::cout << "This can't happen" << std::endl; - // TODO: You sure about that bro? - // FIXME: Throw an exception here rather than printing to stdout + sqlite3_stmt* stm = metadata->prepare(query); + if (sqlite3_step(stm) == SQLITE_ROW && + sqlite3_column_count(stm) > 0) { + int count = sqlite3_column_int(stm, 0); + if (count == 0) { + fixes.emplace_back( + // TODO: Consider making an OrphanedMultipartFix class. + // OrphanedOjectsFix works fine, but the messaging might + // be slightly misleading ("orphaned object: uuid-n ..." + // vs. what would be "orhpaned multipart part: ...") + std::make_shared(root_path, rel.string()) + ); + orphan_count++; } - sqlite3_finalize(stm); } else { - std::cout << "This shouldn't happen" << std::endl; - // TODO: What? Seriously? Do better with the error handling. - // FIXME: Throw an exception here rather than printing to stdout + // This can't happen ("SELECT COUNT(...)" is _always_ going + // to give us one row with one column...) + const char* err = sqlite3_errmsg(metadata->handle); + sqlite3_finalize(stm); + throw std::runtime_error(err); } + sqlite3_finalize(stm); } else { // This is something else fixes.emplace_back( diff --git a/src/main.cc b/src/main.cc index bf20037..3d4da59 100644 --- a/src/main.cc +++ b/src/main.cc @@ -97,5 +97,11 @@ int main(int argc, char* argv[]) { log_level = Check::LogLevel::VERBOSE; } - return run_checks(path_root, log_level, options_map.count("fix") > 0) ? 0 : 1; + try { + return run_checks(path_root, log_level, options_map.count("fix") > 0) ? 0 + : 1; + } catch (std::runtime_error& ex) { + std::cerr << "Runtime error: " << ex.what() << std::endl; + return 1; + } } diff --git a/src/sqlite.cc b/src/sqlite.cc index 36b9ea4..b3f36f5 100644 --- a/src/sqlite.cc +++ b/src/sqlite.cc @@ -14,12 +14,13 @@ #include #include -Database::Database(const std::filesystem::path& _db) : db(_db) { +Database::Database(const std::filesystem::path& _db) + : db(_db), handle(nullptr) { int rc = sqlite3_open(db.string().c_str(), &handle); if (rc != SQLITE_OK) { - std::cout << "failed to open db" << std::endl; + const char* err = sqlite3_errmsg(handle); sqlite3_close(handle); - // FIXME: throw exception + throw std::runtime_error(err); } } @@ -27,16 +28,14 @@ Database::~Database() { sqlite3_close(handle); } -int Database::prepare(const std::string& query, sqlite3_stmt** stm) const { - int rc = 0; +sqlite3_stmt* Database::prepare(const std::string& query) const { + sqlite3_stmt* stm; const char* unused = 0; - rc = sqlite3_prepare(handle, query.c_str(), query.length(), stm, &unused); - if (rc != SQLITE_OK) { - // FIXME: either throw an exception or log to stderr - std::cout << "error while prepare: " << rc << std::endl; - std::cout << query << std::endl; + if (sqlite3_prepare(handle, query.c_str(), query.length(), &stm, &unused) != + SQLITE_OK) { + throw std::runtime_error(sqlite3_errmsg(handle)); } - return rc; + return stm; } /* Count in Table - Count number of rows in named table where the condition @@ -55,10 +54,7 @@ int Database::count_in_table( const char* unused = 0; rc = sqlite3_prepare(handle, query.c_str(), query.length(), &stm, &unused); if (rc != SQLITE_OK) { - // FIXME: either throw an exception or log to stderr - std::cout << "error while prepare: " << rc << std::endl; - std::cout << query << std::endl; - return 0; + throw std::runtime_error(sqlite3_errmsg(handle)); } rc = sqlite3_step(stm); if (rc == SQLITE_ROW && sqlite3_column_count(stm) > 0) { @@ -84,10 +80,7 @@ std::vector Database::select_from_table( std::vector result; rc = sqlite3_prepare(handle, query.c_str(), query.length(), &stm, &unused); if (rc != SQLITE_OK) { - // FIXME: either throw an exception or log to stderr - std::cout << "error while prepare: " << rc << std::endl; - std::cout << query << std::endl; - return result; + throw std::runtime_error(sqlite3_errmsg(handle)); } do { rc = sqlite3_step(stm); diff --git a/src/sqlite.h b/src/sqlite.h index 2243d31..6785e22 100644 --- a/src/sqlite.h +++ b/src/sqlite.h @@ -29,7 +29,7 @@ class Database { Database(const std::filesystem::path& _db); ~Database(); - int prepare(const std::string& query, sqlite3_stmt** stm) const; + sqlite3_stmt* prepare(const std::string& query) const; int count_in_table(const std::string& table, const std::string& condition) const;