Use execptions for sqlite errors that should never happen

Signed-off-by: Tim Serong <[email protected]>

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));

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<const char*>(sqlite3_column_text(stm, 0))};

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<const char*>(sqlite3_column_text(stm, 0))};

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<OrphanedObjectsFix>(
-                        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<OrphanedObjectsFix>(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(

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;
+  }
 }

src/sqlite.cc

@@ -14,29 +14,28 @@
 #include <filesystem>
 #include <iostream>
 
-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);
   }
 }
 
 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<std::string> Database::select_from_table(
   std::vector<std::string> 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);

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;