Fix memory leaks, performance problems, and restore paged result browsing

CMakeLists.txt

@@ -43,6 +43,10 @@ set(COMMON_SRCS
   src/database/dbanalyzer.cpp
   src/database/dbanalyzer.h
   src/database/dbtree.cpp
+  src/database/pagedresultmodel.cpp
+  src/database/pagedresultmodel.h
+  src/database/sqldatabaseadapter.cpp
+  src/database/sqldatabaseadapter.h
   src/database/dbtree.h
   src/gui/highlighter.cpp
   src/gui/highlighter.h

CONTEXT.md

@@ -36,6 +36,10 @@ architecture, the modules, and the tests all refer to the same things.
 - **Tree** — renders a DatabaseInfo into the left-hand QTreeWidget.
 - **QueryExecutor** — runs a list of Queries against a Database, returns
   results. Pure logic, no widgets.
+- **PagedResult** — a lazily fetched view over a Query's result set. Rows are
+  read in pages as they are scrolled into view, so a result set of any size can
+  be browsed without holding it in memory. Ordering is done by the Database,
+  not over the rows already fetched.
 - **QueryResultPresenter** — renders QueryExecutor output into the result
   area of the main window. Owns the scroll area and table views.
 - **SchemaExporter** — produces a `CREATE TABLE` script from a DatabaseInfo.

src/database/dbexport.h

@@ -10,8 +10,8 @@ class DbExport {
     }
 
 protected:
-    [[nodiscard]] DatabaseInfo getDatabaseInfo() const { return info; }
-    [[nodiscard]] QStringList getTextTypes() const { return textTypes; }
+    [[nodiscard]] const DatabaseInfo &getDatabaseInfo() const { return info; }
+    [[nodiscard]] const QStringList &getTextTypes() const { return textTypes; }
     static bool isInternalTable(const Table &table);
 
 private:

src/database/dbexportdata.cpp

@@ -3,26 +3,37 @@
 
 QStringList DbDataExport::getColumnDefs(const Table &table) {
     QStringList columnDefinitions;
-    for (const auto &column: table.columns) {
+    for (const auto &column : table.columns) {
         columnDefinitions.append(column.name);
     }
     return columnDefinitions;
 }
 
-QStringList DbDataExport::getColumnValueDefs(const Table &table,
-                                             const QList<QVariant> &values) const {
-    QStringList valueDefinitions;
-    for (int i = 0; i < table.columns.size(); ++i) {
-        const auto &column = table.columns.at(i);
-        auto value = values.at(i).toString();
+// Whether a column holds text depends only on the schema, so this is resolved
+// once per table rather than once per exported cell.
+QList<bool> DbDataExport::getTextColumnFlags(const Table &table) const {
+    QList<bool> isTextColumn;
+    isTextColumn.reserve(table.columns.size());
+    for (const auto &column : table.columns) {
         bool isText = false;
-        for (const auto &type: getTextTypes()) {
+        for (const auto &type : getTextTypes()) {
             if (column.dataType.contains(type, Qt::CaseInsensitive)) {
                 isText = true;
                 break;
             }
         }
-        if (isText) {
+        isTextColumn.append(isText);
+    }
+    return isTextColumn;
+}
+
+QStringList DbDataExport::getColumnValueDefs(const QList<bool> &isTextColumn,
+                                             const QList<QVariant> &values) {
+    QStringList valueDefinitions;
+    valueDefinitions.reserve(isTextColumn.size());
+    for (int i = 0; i < isTextColumn.size(); ++i) {
+        auto value = values.at(i).toString();
+        if (isTextColumn.at(i)) {
             value = value.replace("\"", "\"\"");
             valueDefinitions.append(QString("\"%1\"").arg(value));
         } else {
@@ -44,19 +55,20 @@ void DbDataExport::exportDataToSqlFile(IDatabase *database,
 
     progress->reset();
     QTextStream out(file.get());
-    for (const auto &table: this->getDatabaseInfo().tables) {
+    for (const auto &table : this->getDatabaseInfo().tables) {
         if (isInternalTable(table)) {
             continue;
         }
         out << "-- " << table.name << "\n";
 
         const auto columns = getColumnDefs(table).join(", ");
+        const auto isTextColumn = getTextColumnFlags(table);
         const QueryResult streamResult = database->streamRows(
             QString("SELECT * FROM \"%1\"").arg(table.name),
             [&](const QList<QVariant> &values) {
                 if (cancellationToken->isCancellationRequested())
                     return false;
-                const auto valueList = getColumnValueDefs(table, values).join(", ");
+                const auto valueList = getColumnValueDefs(isTextColumn, values).join(", ");
                 out << "INSERT INTO \"" << table.name << "\"(" << columns << ") ";
                 out << "VALUES (" << valueList << ");\n";
                 progress->increment();
@@ -78,7 +90,7 @@ void DbDataExport::exportDataToCsvFile(IDatabase *database,
                                        const CancellationToken *cancellationToken,
                                        ExportDataProgress *progress) const {
     progress->reset();
-    for (const auto &table: this->getDatabaseInfo().tables) {
+    for (const auto &table : this->getDatabaseInfo().tables) {
         if (isInternalTable(table) || cancellationToken->isCancellationRequested()) {
             continue;
         }
@@ -90,6 +102,7 @@ void DbDataExport::exportDataToCsvFile(IDatabase *database,
         }
 
         const auto columns = getColumnDefs(table).join(delimiter);
+        const auto isTextColumn = getTextColumnFlags(table);
         QTextStream out(file.get());
         out << columns << "\n";
 
@@ -98,7 +111,7 @@ void DbDataExport::exportDataToCsvFile(IDatabase *database,
             [&](const QList<QVariant> &values) {
                 if (cancellationToken->isCancellationRequested())
                     return false;
-                const auto valueList = getColumnValueDefs(table, values).join(delimiter);
+                const auto valueList = getColumnValueDefs(isTextColumn, values).join(delimiter);
                 out << valueList << "\n";
                 progress->increment();
                 return true;

src/database/dbexportdata.h

@@ -29,8 +29,10 @@ class DbDataExport : public DbExport {
 private:
     static QStringList getColumnDefs(const Table &table);
 
-    [[nodiscard]] QStringList getColumnValueDefs(const Table &table,
-                                                 const QList<QVariant> &values) const;
+    [[nodiscard]] QList<bool> getTextColumnFlags(const Table &table) const;
+
+    static QStringList getColumnValueDefs(const QList<bool> &isTextColumn,
+                                          const QList<QVariant> &values);
 };
 
 #endif // DBDATAEXPORT_H

src/database/dbtree.cpp

@@ -49,13 +49,13 @@ void DbTree::populateTree(const DatabaseInfo &info) {
     const auto tablesRootNodePtr = tablesRootNode.get();
     this->tree->addTopLevelItem(tablesRootNode.release());
 
-    foreach(Table table, info.tables) {
+    for (const auto &table : info.tables) {
         auto tableNode = std::make_unique<QTreeWidgetItem>(QTreeWidgetItem::UserType + 1);
         tableNode->setText(0, table.name);
         const auto tableNodePtr = tableNode.get();
         tablesRootNodePtr->addChild(tableNode.release());
 
-        foreach(Column col, table.columns) {
+        for (const auto &col : table.columns) {
             auto colName = std::make_unique<QTreeWidgetItem>();
             colName->setText(0, col.name);
             const auto colNamePtr = colName.get();

src/database/idatabase.h

@@ -9,6 +9,8 @@
 
 #include "queryresult.h"
 
+class QAbstractItemModel;
+
 class IDatabase {
 public:
     virtual ~IDatabase() = default;
@@ -25,6 +27,16 @@ class IDatabase {
 
     virtual QueryResult runStatement(const QString &sql) = 0;
 
+    // A lazily fetched, pageable model over the statement's result set. Rows
+    // are read in pages as they are scrolled into view, so a result set of any
+    // size can be browsed. The caller owns the returned model.
+    // Returns nullptr when the statement produces no result set (an INSERT,
+    // say) or when it failed -- `error` then holds the reason, and is cleared
+    // otherwise. A SELECT matching no rows still returns a model, so that its
+    // columns can be displayed.
+    virtual QAbstractItemModel *createResultModel(const QString &sql,
+                                                  QString *error = nullptr) = 0;
+
     virtual QueryResult streamRows(const QString &sql,
                                    const std::function<bool(const QList<QVariant> &)> &onRow) = 0;
 };

src/database/inmemorydatabase.cpp

@@ -1,8 +1,7 @@
 #include "inmemorydatabase.h"
+
 #include <QSqlDatabase>
 #include <QSqlQuery>
-#include <QSqlRecord>
-#include <QSqlError>
 
 InMemoryDatabase::InMemoryDatabase() {
     database = QSqlDatabase::addDatabase("QSQLITE", "in_memory_connection");
@@ -21,85 +20,10 @@ bool InMemoryDatabase::open() {
     return database.open();
 }
 
-void InMemoryDatabase::close() {
-    if (database.isOpen())
-        database.close();
-}
-
 void InMemoryDatabase::shrink() {
     if (!database.isOpen())
         return;
 
     QSqlQuery query(database);
     query.exec("VACUUM");
 }
-
-QueryResult InMemoryDatabase::runStatement(const QString &sql) {
-    QueryResult result;
-    if (!database.isOpen()) {
-        result.ok = false;
-        result.error = "Database is not open";
-        return result;
-    }
-
-    QSqlQuery query(database);
-    if (!query.exec(sql)) {
-        result.ok = false;
-        result.error = query.lastError().text();
-        return result;
-    }
-
-    result.isSelect = query.isSelect();
-    if (result.isSelect) {
-        const QSqlRecord record = query.record();
-        for (int i = 0; i < record.count(); ++i) {
-            result.columns.append(record.fieldName(i));
-        }
-        while (query.next()) {
-            QueryRow row;
-            row.values.reserve(record.count());
-            for (int i = 0; i < record.count(); ++i) {
-                row.values.append(query.value(i));
-            }
-            result.rows.append(row);
-        }
-    } else {
-        result.rowsAffected = query.numRowsAffected();
-    }
-    return result;
-}
-
-QueryResult InMemoryDatabase::streamRows(const QString &sql,
-                                         const std::function<bool(const QList<QVariant> &)> &onRow) {
-    QueryResult result;
-    if (!database.isOpen()) {
-        result.ok = false;
-        result.error = "Database is not open";
-        return result;
-    }
-
-    QSqlQuery query(database);
-    query.setForwardOnly(true);
-    if (!query.exec(sql)) {
-        result.ok = false;
-        result.error = query.lastError().text();
-        return result;
-    }
-
-    result.isSelect = query.isSelect();
-    const QSqlRecord record = query.record();
-    for (int i = 0; i < record.count(); ++i) {
-        result.columns.append(record.fieldName(i));
-    }
-
-    while (query.next()) {
-        QList<QVariant> values;
-        values.reserve(record.count());
-        for (int i = 0; i < record.count(); ++i) {
-            values.append(query.value(i));
-        }
-        if (!onRow(values))
-            break;
-    }
-    return result;
-}

src/database/inmemorydatabase.h

@@ -1,35 +1,20 @@
 #ifndef INMEMORYDATABASE_H
 #define INMEMORYDATABASE_H
 
-#include <QSqlDatabase>
 #include <QString>
 
-#include "idatabase.h"
+#include "sqldatabaseadapter.h"
 
-class InMemoryDatabase : public IDatabase {
+// Test adapter: a SQLite database held in memory.
+class InMemoryDatabase final : public SqlDatabaseAdapter {
 public:
     InMemoryDatabase();
 
     void setSource(const QString &filename) override;
 
     bool open() override;
 
-    void close() override;
-
     void shrink() override;
-
-    [[nodiscard]] QString getFilename() const override { return source; }
-
-    QueryResult runStatement(const QString &sql) override;
-
-    QueryResult streamRows(const QString &sql,
-                           const std::function<bool(const QList<QVariant> &)> &onRow) override;
-
-    [[nodiscard]] QSqlDatabase getConnection() const { return database; }
-
-private:
-    QSqlDatabase database;
-    QString source;
 };
 
 #endif // INMEMORYDATABASE_H

src/database/pagedresultmodel.cpp

@@ -0,0 +1,54 @@
+#include "pagedresultmodel.h"
+
+#include <QSqlError>
+#include <QSqlQuery>
+#include <QSqlRecord>
+
+#include <utility>
+
+PagedResultModel::PagedResultModel(const QSqlDatabase &database, QString sql, QObject *parent)
+    : QSqlQueryModel(parent),
+      database(database),
+      statement(std::move(sql)) {
+    run(statement);
+}
+
+bool PagedResultModel::run(const QString &sql) {
+    QSqlQuery query(this->database);
+    if (!query.exec(sql)) {
+        this->error = query.lastError().text();
+        return false;
+    }
+
+    // QSqlQueryModel reads the first page here and the rest on demand.
+    setQuery(std::move(query));
+    if (lastError().isValid()) {
+        this->error = lastError().text();
+        return false;
+    }
+
+    this->error.clear();
+    return true;
+}
+
+void PagedResultModel::sort(const int column, const Qt::SortOrder order) {
+    if (column < 0 || column >= columnCount())
+        return;
+
+    QString columnName = record().fieldName(column);
+    if (columnName.isEmpty())
+        return;
+
+    // A quoted identifier escapes a double quote by doubling it.
+    columnName.replace('"', "\"\"");
+
+    const QString sorted = QString("SELECT * FROM (%1) ORDER BY \"%2\" %3")
+            .arg(statement,
+                 columnName,
+                 order == Qt::AscendingOrder ? "ASC" : "DESC");
+
+    // Not every statement can be wrapped in a subquery. run() leaves the model
+    // untouched when it fails, so there is nothing to restore -- and re-running
+    // the original would execute a statement with side effects a second time.
+    run(sorted);
+}