Remove database error thows (#191)

Co-authored-by: Chris Rizzitello <crizzitello@ics.com>

Readme_Developer.md

@@ -36,6 +36,7 @@ Updating the database schema is slightly complicated, and the following rules mu
    * This will create file in the `migrations` folder with the indicated name and a timestamp
    * This should also add this migration to the qrc file. However, if this is not done, you can do this manually by editing the `rs_migrations.qrc` file.
 2. Inside the new migration file, add the necessary sql to apply the db change under `-- +migrate Up` 
+   * This section must come first.
 3. Inside the new migration file, add the necessary sql to _undo_ the db change under `-- +migrate Down`
 4. Only one statement is allowed under each heading. If multiple statements need to be applied, they should done as multiple migration files
    * This is a sqlite3/Qt limitation.

src/components/code_editor/codeblockview.cpp

@@ -6,7 +6,6 @@
 #include <QLineEdit>
 
 #include "codeeditor.h"
-#include "exceptions/fileerror.h"
 #include "helpers/ui_helpers.h"
 
 CodeBlockView::CodeBlockView(QWidget* parent)

src/components/evidence_editor/evidenceeditor.cpp

@@ -179,32 +179,28 @@ SaveEvidenceResponse EvidenceEditor::saveEvidence() {
   return resp;
 }
 
-QList<DeleteEvidenceResponse> EvidenceEditor::deleteEvidence(QList<qint64> evidenceIDs) {
-  QList<DeleteEvidenceResponse> responses;
+QList<DeleteEvidenceResponse> EvidenceEditor::deleteEvidence(QList<qint64> evidenceIDs)
+{
+    QList<DeleteEvidenceResponse> responses;
+    for (qint64 id : evidenceIDs) {
+        model::Evidence evi = db->getEvidenceDetails(id);
+        DeleteEvidenceResponse resp(evi);
 
-  for (qint64 id : evidenceIDs) {
-    model::Evidence evi = db->getEvidenceDetails(id);
-    DeleteEvidenceResponse resp(evi);
-    try {
-      db->deleteEvidence(evi.id);
-      resp.dbDeleteSuccess = true;
-    }
-    catch(QSqlError &e) {
-      resp.dbDeleteSuccess = false;
-      resp.errorText = e.text();
-    }
-    auto localFile = new QFile(evi.path);
-    if (!localFile->remove()) {
-      resp.fileDeleteSuccess = false;
-      resp.errorText += "\n" + localFile->errorString();
-    }
-    else {
-      resp.fileDeleteSuccess = true;
-    }
-    localFile->deleteLater(); // deletes the pointer, not the file
-    resp.errorText = resp.errorText.trimmed();
-    responses.append(resp);
-  }
+        try{
+            resp.dbDeleteSuccess = db->deleteEvidence(evi.id);
+        } catch(QSqlError &e) {
+            resp.errorText = e.text();
+        }
 
-  return responses;
+        auto localFile = QFile(evi.path);
+        if (!localFile.remove()) {
+            resp.fileDeleteSuccess = false;
+            resp.errorText.append(QStringLiteral("\n%1").arg(localFile.errorString()));
+        } else {
+            resp.fileDeleteSuccess = true;
+        }
+        resp.errorText = resp.errorText.trimmed();
+        responses.append(resp);
+    }
+    return responses;
 }

src/db/databaseconnection.cpp

@@ -6,28 +6,16 @@
 #include <QDir>
 #include <QVariant>
 
-#include "exceptions/databaseerr.h"
 #include "exceptions/fileerror.h"
 #include "helpers/file_helpers.h"
 
-// DatabaseConnection constructs a connection to the database, unsurpringly. Note that the
-// constructor can throw a error (see below). Additionally, many methods can throw a QSqlError,
-// though are not marked as such in their comments. Other errors are listed in throw comments, or
-// are marked as noexcept if no error is possible.
-//
-// Throws: DBDriverUnavailable if the required database driver does not exist
-DatabaseConnection::DatabaseConnection(const QString& dbPath, QString databaseName) {
-  const static QString dbDriver = "QSQLITE";
-  if (QSqlDatabase::isDriverAvailable(dbDriver)) {
-    dbName = std::move(databaseName);
-    auto db = QSqlDatabase::addDatabase(dbDriver, dbName);
+DatabaseConnection::DatabaseConnection(const QString& dbPath, const QString& databaseName)
+ : _dbName(databaseName)
+ , _dbPath(dbPath)
+{
+    auto db = QSqlDatabase::addDatabase(QStringLiteral("QSQLITE"), databaseName);
     QDir().mkpath(FileHelpers::getDirname(dbPath));
     db.setDatabaseName(dbPath);
-    this->_dbPath = dbPath;
-  }
-  else {
-    throw DBDriverUnavailableError("SQLite");
-  }
 }
 
 void DatabaseConnection::withConnection(const QString& dbPath, const QString &dbName,
@@ -45,12 +33,14 @@ void DatabaseConnection::withConnection(const QString& dbPath, const QString &db
   QSqlDatabase::removeDatabase(dbName);
 }
 
-void DatabaseConnection::connect() {
-  auto db = getDB();
-  if (!db.open()) {
-    throw db.lastError();
-  }
-  migrateDB();
+bool DatabaseConnection::connect()
+{
+    auto db = getDB();
+    if (!db.open()) {
+        return false;
+        throw db.lastError();
+    }
+    return migrateDB();
 }
 
 void DatabaseConnection::close() noexcept { getDB().close(); }
@@ -137,8 +127,13 @@ void DatabaseConnection::updateEvidenceDescription(const QString &newDescription
   executeQuery(getDB(), "UPDATE evidence SET description=? WHERE id=?", {newDescription, evidenceID});
 }
 
-void DatabaseConnection::deleteEvidence(qint64 evidenceID) {
-  executeQuery(getDB(), "DELETE FROM evidence WHERE id=?", {evidenceID});
+bool DatabaseConnection::deleteEvidence(qint64 evidenceID)
+{
+  auto q = executeQuery(getDB(), "DELETE FROM evidence WHERE id=?", {evidenceID});
+  if (q.lastError().type() == QSqlError::NoError)
+      return true;
+  QTextStream(stderr) << "Unable to Delete " << evidenceID << " " << q.lastError().text();
+  return false;
 }
 
 void DatabaseConnection::updateEvidenceError(const QString &errorText, qint64 evidenceID) {
@@ -339,7 +334,7 @@ QList<model::Evidence> DatabaseConnection::createEvidenceExportView(
 // lacking update.
 //
 // Throws exceptions/FileError if a migration file cannot be found.
-void DatabaseConnection::migrateDB() {
+bool DatabaseConnection::migrateDB() {
   auto db = getDB();
   QTextStream(stdout) << "Checking database state" << Qt::endl;
   auto migrationsToApply = DatabaseConnection::getUnappliedMigrations(db);
@@ -347,10 +342,8 @@ void DatabaseConnection::migrateDB() {
   for (const QString &newMigration : migrationsToApply) {
     QFile migrationFile(QStringLiteral(":/migrations/%1").arg(newMigration));
     auto ok = migrationFile.open(QFile::ReadOnly);
-    if (!ok) {
-      throw FileError::mkError("Error reading migration file",
-                               migrationFile.fileName().toStdString(), migrationFile.error());
-    }
+    if (!ok)
+        return false;
     auto content = QString(migrationFile.readAll());
     migrationFile.close();
 
@@ -362,6 +355,7 @@ void DatabaseConnection::migrateDB() {
                  {newMigration});
   }
   QTextStream(stdout) << "All migrations applied" << Qt::endl;
+  return true;
 }
 
 // getUnappliedMigrations retrieves a list of all of the migrations that have not been applied
@@ -372,10 +366,10 @@ void DatabaseConnection::migrateDB() {
 // Throws:
 //   * BadDatabaseStateError if some migrations have been applied that are not known
 //   * QSqlError if database queries fail
-QStringList DatabaseConnection::getUnappliedMigrations(const QSqlDatabase &db) {
+QStringList DatabaseConnection::getUnappliedMigrations(const QSqlDatabase &db)
+{
   QDir migrationsDir(QStringLiteral(":/migrations"));
-
-  auto allMigrations = migrationsDir.entryList(QDir::Files, QDir::Name);
+  const auto allMigrations = migrationsDir.entryList(QDir::Files, QDir::Name);
   QStringList appliedMigrations;
   QStringList migrationsToApply;
 
@@ -386,19 +380,17 @@ QStringList DatabaseConnection::getUnappliedMigrations(const QSqlDatabase &db) {
   }
   // compare the two list to find gaps
   for (const QString &possibleMigration : allMigrations) {
-    if (!possibleMigration.endsWith(QStringLiteral(".sql"))) {
+    if (!possibleMigration.endsWith(QStringLiteral(".sql")))
       continue;  // assume non-sql files aren't actual migrations.
-    }
+
     auto foundIndex = appliedMigrations.indexOf(possibleMigration);
-    if (foundIndex == -1) {
+    if (foundIndex == -1)
       migrationsToApply << possibleMigration;
-    }
-    else {
+    else
       appliedMigrations.removeAt(foundIndex);
-    }
   }
   if (!appliedMigrations.empty()) {
-    throw BadDatabaseStateError();
+    QTextStream(stderr) << "Database is in an inconsistent state";
   }
   return migrationsToApply;
 }
@@ -406,21 +398,15 @@ QStringList DatabaseConnection::getUnappliedMigrations(const QSqlDatabase &db) {
 // extractMigrateUpContent parses the given migration content and retrieves only
 // the portion that applies to the "up" / apply logic. The "down" section is ignored.
 QString DatabaseConnection::extractMigrateUpContent(const QString &allContent) noexcept {
-  auto copying = false;
   QString upContent;
-  for (const QString &line : allContent.split("\n")) {
-    if (line.trimmed().toLower() == "-- +migrate up") {
-      copying = true;
-    }
-    else if (line.trimmed().toLower() == "-- +migrate down") {
-      if (copying) {
-        break;
-      }
-      copying = false;
-    }
-    else if (copying) {
-      upContent.append(line + "\n");
-    }
+  const QStringList lines = allContent.split(_newLine);
+  for (const QString &line : lines) {
+    auto lowerLine = line.trimmed().toLower();
+    if (lowerLine == _migrateUp)
+      continue;
+    else if (lowerLine == _migrateDown)
+      break;
+    upContent.append(_lineTemplate.arg(line));
   }
   return upContent;
 }
@@ -465,15 +451,14 @@ qint64 DatabaseConnection::doInsert(const QSqlDatabase& db, const QString &stmt,
   return query.lastInsertId().toLongLong();
 }
 
-QSqlDatabase DatabaseConnection::getDB() {
-  if (dbName.isEmpty()) {
-    return QSqlDatabase::database();
-  }
-  return QSqlDatabase::database(dbName);
+QSqlDatabase DatabaseConnection::getDB()
+{
+    return QSqlDatabase::database(_dbName);
 }
 
-QString DatabaseConnection::getDatabasePath() {
-  return _dbPath;
+QString DatabaseConnection::getDatabasePath()
+{
+    return _dbPath;
 }
 
 void DatabaseConnection::batchInsert(const QString& baseQuery, unsigned int varsPerRow, unsigned int numRows,

src/db/databaseconnection.h

@@ -24,17 +24,20 @@ class DBQuery {
 
  public:
   DBQuery(QString query) : DBQuery(query, {}) {}
-  DBQuery(QString query, QVariantList values) {
-    this->_query = query;
-    this->_values = values;
-  }
+  DBQuery(QString query, QVariantList values) :_query(query), _values(values) { }
   inline QString query() { return _query; }
   inline QVariantList values() { return _values; }
 };
 
 class DatabaseConnection {
  public:
-  DatabaseConnection(const QString& dbPath, QString databaseName);
+   /**
+   * @brief DatabaseConnection construct a connect to the database,
+   * Uses QSQLite Driver if the driver is missing the app to exit with code 255.
+   * @param dbPath - Path to the database
+   * @param databaseName - Name of the databaseFile or defaultName if none provided
+   */
+  DatabaseConnection(const QString& dbPath, const QString& databaseName = Constants::defaultDbName);
 
   /**
    * @brief withConnection acts as a context manager for a single database connection. The goal for
@@ -49,7 +52,7 @@ class DatabaseConnection {
   static void withConnection(const QString& dbPath, const QString &dbName,
                              const std::function<void(DatabaseConnection)> &actions);
 
-  void connect();
+  bool connect();
   void close() noexcept;
 
   static DBQuery buildGetEvidenceWithFiltersQuery(const EvidenceFilters &filters);
@@ -71,7 +74,12 @@ class DatabaseConnection {
   void batchCopyTags(const QList<model::Tag> &allTags);
   QList<model::Tag> getFullTagsForEvidenceIDs(const QList<qint64>& evidenceIDs);
 
-  void deleteEvidence(qint64 evidenceID);
+  /**
+   * @brief deleteEvidence Delete Evidence from the database
+   * @param evidenceID - ID To Delete
+   * @return true if successful
+   */
+  bool deleteEvidence(qint64 evidenceID);
 
   /// createEvidenceExportView duplicates the normal database with only a subset of evidence
   /// present, as well as related data (e.g. tags)
@@ -90,10 +98,14 @@ class DatabaseConnection {
   const unsigned long SQLITE_MAX_VARS = 999;
 
  private:
-  QString dbName;
+  QString _dbName;
   QString _dbPath;
+  inline static const auto _migrateUp = QStringLiteral("-- +migrate up");
+  inline static const auto _migrateDown = QStringLiteral("-- +migrate down");
+  inline static const auto _newLine = QStringLiteral("\n");
+  inline static const auto _lineTemplate =QStringLiteral("%1").append(_newLine);
 
-  void migrateDB();
+  bool migrateDB();
   QSqlDatabase getDB();
 
   static QStringList getUnappliedMigrations(const QSqlDatabase &db);

src/exceptions/CMakeLists.txt

@@ -1,8 +1,5 @@
 
-add_library (EXCEPTIONS STATIC
-    databaseerr.h
-    fileerror.h
-)
+add_library (EXCEPTIONS STATIC fileerror.h)
 
 add_library(ASHIRT::EXCEPTIONS ALIAS EXCEPTIONS)
 
@@ -12,8 +9,4 @@ target_include_directories (EXCEPTIONS
         $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>
 )
 
-target_link_libraries ( EXCEPTIONS PUBLIC
-    Qt::Core
-)
-
-
+target_link_libraries ( EXCEPTIONS PUBLIC Qt::Core)

src/exceptions/databaseerr.h

@@ -1,17 +0,0 @@
-// Copyright 2020, Verizon Media
-// Licensed under the terms of MIT. See LICENSE file in project root for terms.
-
-#pragma once
-
-#include <stdexcept>
-
-class BadDatabaseStateError : public std::runtime_error {
- public:
-  BadDatabaseStateError() : std::runtime_error("Database is in an inconsistent state") {}
-};
-
-class DBDriverUnavailableError : public std::runtime_error {
- public:
-  DBDriverUnavailableError(std::string friendlyDriverName)
-      : std::runtime_error(friendlyDriverName + " driver is unavailable") {}
-};

src/forms/getinfo/getinfo.cpp

@@ -118,13 +118,8 @@ void GetInfo::deleteButtonClicked() {
                              .arg(evi.path));
       shouldClose = false;
     }
-    try {
-      db->deleteEvidence(evidenceID);
-    }
-    catch (QSqlError& e) {
-      QTextStream(stderr) << "Could not delete evidence from internal database. Error: "
-                << e.text() << Qt::endl;
-    }
+
+    db->deleteEvidence(evidenceID);
 
     Q_EMIT setActionButtonsEnabled(true);
     if (shouldClose) {

src/main.cpp

@@ -16,7 +16,6 @@ void handleCLI(std::vector<std::string> args);
 #include "appconfig.h"
 #include "appsettings.h"
 #include "db/databaseconnection.h"
-#include "exceptions/databaseerr.h"
 #include "exceptions/fileerror.h"
 #include "traymanager.h"
 
@@ -32,25 +31,10 @@ int main(int argc, char* argv[]) {
   QCoreApplication::setOrganizationName("ashirt");
 #endif
 
-  DatabaseConnection* conn;
-  try {
-    conn = new DatabaseConnection(Constants::dbLocation, Constants::defaultDbName);
-    conn->connect();
-  }
-  catch (FileError& err) {
-    QTextStream(stderr) << err.what() << Qt::endl;
-    return -1;
-  }
-  catch (DBDriverUnavailableError& err) {
-    QTextStream(stderr) << err.what() << Qt::endl;
-    return -1;
-  }
-  catch (QSqlError& e) {
-    QTextStream(stderr) << e.text() << Qt::endl;
-    return -1;
-  }
-  catch (std::exception& e) {
-    QTextStream(stderr) << "Unexpected error: " << e.what() << Qt::endl;
+
+  DatabaseConnection* conn = new DatabaseConnection(Constants::dbLocation, Constants::defaultDbName);
+  if(!conn->connect()) {
+    QTextStream(stderr) << "Unable to connect to Database" << Qt::endl;
     return -1;
   }
 
@@ -66,11 +50,17 @@ int main(int argc, char* argv[]) {
     qRegisterMetaType<model::Tag>();
 
     app.setWindowIcon(getWindowIcon());
+
     if (!QSystemTrayIcon::isSystemTrayAvailable()) {
       handleCLI(std::vector<std::string>(argv, argv + argc));
     }
     QApplication::setQuitOnLastWindowClosed(false);
 
+    QObject::connect(&app, &QApplication::aboutToQuit, [conn] {
+        conn->close();
+        delete conn;
+    });
+
     auto window = new TrayManager(nullptr, conn);
     rtn = app.exec();
     AppSettings::getInstance().sync();
@@ -82,9 +72,6 @@ int main(int argc, char* argv[]) {
   catch (...) {
     QTextStream(stderr) << "Unhandled exception while running" << Qt::endl;
   }
-  conn->close();
-  delete conn;
-
   return rtn;
 }
 

src/models/codeblock.h

@@ -20,7 +20,6 @@ class Codeblock {
   /**
    * @brief readCodeblock parses a local codeblock file and returns back the data as a codeblock
    * @param filepath The path to the codeblock file
-   * @throws a FileError if any issues occur while reading the file
    * @return a parsed Codeblock object, ready for use.
    */
   static Codeblock readCodeblock(const QString& filepath);
@@ -53,9 +52,7 @@ class Codeblock {
   QByteArray encode();
  public:
   /**
-   * @brief saveCodeblock encodes the provided codeblock, then writes that codeblock to it's
-   * filePath
-   * @throws a FileError if any issues occur while writing the file
+   * @brief saveCodeblock encodes the provided codeblock, then writes that codeblock to it's filePath
    * @param codeblock The codeblock to save
    */
   static bool saveCodeblock(Codeblock codeblock);