Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
fd6bbba
docs: update stale references in AGENTS.md and CONTEXT.md
christianhelle Jun 9, 2026
ab980ed
build: fail on missing tests, add Linux/macOS test execution, fix PATH
christianhelle Jun 9, 2026
72745ff
database: remove duplicate include, setHostName, add VACUUM error check
christianhelle Jun 9, 2026
2682bbd
database: use unique connection names per SqliteDatabase instance
christianhelle Jun 9, 2026
115ae8f
database: add isOpen() to IDatabase interface and adapters
christianhelle Jun 9, 2026
a60e9d7
database: reject setSource on in-memory DB, remove getConnection leak
christianhelle Jun 9, 2026
1486af1
database: remove redundant Qt::CaseInsensitive, change rowsAffected t…
christianhelle Jun 9, 2026
540c40e
database: add error tracking to ExportDataProgress, report streamRows…
christianhelle Jun 9, 2026
49e8142
gui: fix ExportOrchestrator UAF and race condition with QPointer guards
christianhelle Jun 9, 2026
0510189
gui: decouple MainWindow from SqliteDatabase, use IDatabase interface
christianhelle Jun 9, 2026
1adeb29
gui: fix QueryResultPresenter widget and model memory leaks
christianhelle Jun 9, 2026
ecc7232
gui: add executeScript to QueryExecutionPresenter, use UniqueConnection
christianhelle Jun 9, 2026
97ef5e4
cli: output execution errors and exit non-zero on failure
christianhelle Jun 9, 2026
cdd9fd7
tests: support Qt5/Qt6, remove progress.h from compiled sources
christianhelle Jun 9, 2026
6c2e88e
tests: document FK/UNIQUE limitation in schema export test
christianhelle Jun 9, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ implementation details. If you introduce a new domain term, add it to
run SQL, it goes through the seam. The seam owns query construction.
- **No widget pointers in non-GUI modules.** A module that takes a
`QWidget*` is a presenter, not a domain module. Presenters are thin.
- **No SQL string concatenation in non-database modules.** `dbquery.h`
- **No SQL string concatenation in non-database modules.** `QueryExecutor`
splitting-by-`;` lives in the database module, not in `mainwindow.cpp` or
the CLI.

Expand Down
7 changes: 4 additions & 3 deletions CONTEXT.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ architecture, the modules, and the tests all refer to the same things.

## User-facing actions

- **Query** — a single SQL statement the user wrote and wants executed.
- **Query** — a single SQL statement the user wrote and wants to execute.
- **Script** — a sequence of Queries loaded from a `.sql` file.
- **Export** — a one-shot transformation of Database content into a different
representation. Two formats: **CSV** (one file per table) and **SQL**
Expand Down Expand Up @@ -52,8 +52,9 @@ architecture, the modules, and the tests all refer to the same things.
- **IDatabase** — abstract Database. Production adapter is `SqliteDatabase`;
test adapter is `InMemoryDatabase`. Anything that takes a Database takes
`IDatabase*`. `QSqlDatabase` does not leak across this seam.
- **ExportStrategy** — abstract DataExporter workflow (`SqlExportStrategy`,
`CsvExportStrategy`). Two adapters = real seam.
- **ExportOrchestrator** — runs a DataExporter in the background, marshals
progress to the GUI thread, surfaces cancel and completion via exportToSql()
and exportToCsv().
Comment on lines +55 to +57

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

ExportOrchestrator is not a seam and should not be listed in the Seams section.

Per the coding guidelines, seams must be abstract interfaces with at least two adapters. ExportOrchestrator is a concrete orchestrator class, not an abstraction. This description already exists correctly in the Modules section (lines 44-45).

The Seams section should only list abstract interfaces like IDatabase. Remove ExportOrchestrator from here to avoid confusion about the architecture.

📐 Proposed fix
 ## Seams
 
 - **IDatabase** — abstract Database. Production adapter is `SqliteDatabase`;
   test adapter is `InMemoryDatabase`. Anything that takes a Database takes
   `IDatabase*`. `QSqlDatabase` does not leak across this seam.
-- **ExportOrchestrator** — runs a DataExporter in the background, marshals
-  progress to the GUI thread, surfaces cancel and completion via exportToSql()
-  and exportToCsv().
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- **ExportOrchestrator** — runs a DataExporter in the background, marshals
progress to the GUI thread, surfaces cancel and completion via exportToSql()
and exportToCsv().
## Seams
- **IDatabase** — abstract Database. Production adapter is `SqliteDatabase`;
test adapter is `InMemoryDatabase`. Anything that takes a Database takes
`IDatabase*`. `QSqlDatabase` does not leak across this seam.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@CONTEXT.md` around lines 55 - 57, Remove ExportOrchestrator from the Seams
section: edit the Seams list to only include abstract interfaces (e.g.,
IDatabase) and delete the ExportOrchestrator bullet/description
(ExportOrchestrator is a concrete orchestrator already documented under
Modules). Ensure the Seams section contains only true seams (abstract interfaces
with multiple adapters) and leave the Modules section entry for
ExportOrchestrator unchanged.

Source: Coding guidelines


## Out of scope (do not introduce)

Expand Down
64 changes: 58 additions & 6 deletions build.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -83,16 +83,19 @@ if ($IsWindows) {
if (Test-Path $testExe) {
$env:PATH = "$QtPath\bin;$QtPath\lib;$env:PATH"
Write-Host "Executing: $testExe"
& $testExe
$env:PATH = $env:PATH -replace [regex]::Escape("$QtPath\bin;$QtPath\lib;"), ""
if ($LASTEXITCODE -ne 0) {
Write-Error "Tests failed (exit code: $LASTEXITCODE)"
Push-Location build
& .\SQLiteQueryTests.exe
$testExitCode = $LASTEXITCODE
Pop-Location
if ($testExitCode -ne 0) {
Write-Error "Tests failed (exit code: $testExitCode)"
exit 1
}
Write-Host "`n=== All tests passed ===" -ForegroundColor Green
}
else {
Write-Warning "Test executable not found at: $testExe"
Write-Error "Test executable not found at: $testExe"
exit 1
}
}

Expand All @@ -106,6 +109,30 @@ if ($IsLinux) {
cmake --build build --config Release --parallel $(nproc)
cmake --install build

Write-Host "`nRunning tests..."
$testExe = "./build/SQLiteQueryTests"
if (Test-Path $testExe) {
$originalPath = $env:PATH
if ($QtPath) {
$pathArray = ($env:PATH -split ':') | Where-Object { $_ -ne "$QtPath/bin" -and $_ -ne "$QtPath/lib" }
$pathArray = @("$QtPath/bin", "$QtPath/lib") + $pathArray
$env:PATH = $pathArray -join ':'
}
Write-Host "Executing: $testExe"
& $testExe
if ($QtPath) {
$env:PATH = $originalPath
}
if ($LASTEXITCODE -ne 0) {
Write-Error "Tests failed (exit code: $LASTEXITCODE)"
exit 1
}
Write-Host "`n=== All tests passed ===" -ForegroundColor Green
}
else {
Write-Warning "Test executable not found at: $testExe"
}
Comment on lines +112 to +134

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Inconsistent handling of missing test executables across platforms.

On Windows (lines 97-98), a missing test executable triggers Write-Error and exit 1. However, on Linux (line 133), it only emits Write-Warning and continues. This violates the stated PR objective that "missing test executables are treated as errors."

🔧 Proposed fix to make Linux consistent with Windows
     else {
-        Write-Warning "Test executable not found at: $testExe"
+        Write-Error "Test executable not found at: $testExe"
+        exit 1
     }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@build.ps1` around lines 112 - 134, The script treats a missing test
executable differently on Linux vs Windows; locate the block that checks
Test-Path for $testExe (uses Test-Path, Write-Warning, Write-Host "Executing:
$testExe", and the $QtPath PATH manipulation) and change the else branch so it
mirrors the Windows behavior: replace the Write-Warning with a Write-Error
including the missing path and then call exit 1 so missing test executables are
treated as errors consistently across platforms; preserve the surrounding PATH
restore logic tied to $QtPath and ensure $LASTEXITCODE handling remains
unchanged after running the executable.


if ($Package) {
cpack -G 7Z --config ./build/CPackConfig.cmake
cpack -G ZIP --config ./build/CPackConfig.cmake
Expand All @@ -125,10 +152,35 @@ if ($IsLinux) {
}
}

if ($IsMacOS -And $Package) {
if ($IsMacOS) {

cmake -B build -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=/tmp/sqlitequery
cmake --build build --config Release --parallel 32

Write-Host "`nRunning tests..."
$testExe = "./build/SQLiteQueryTests"
if (Test-Path $testExe) {
$originalPath = $env:PATH
if ($QtPath) {
$pathArray = ($env:PATH -split ':') | Where-Object { $_ -ne "$QtPath/bin" -and $_ -ne "$QtPath/lib" }
$pathArray = @("$QtPath/bin", "$QtPath/lib") + $pathArray
$env:PATH = $pathArray -join ':'
}
Write-Host "Executing: $testExe"
& $testExe
if ($QtPath) {
$env:PATH = $originalPath
}
if ($LASTEXITCODE -ne 0) {
Write-Error "Tests failed (exit code: $LASTEXITCODE)"
exit 1
}
Write-Host "`n=== All tests passed ===" -ForegroundColor Green
}
else {
Write-Warning "Test executable not found at: $testExe"
}
Comment on lines +155 to +182

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Inconsistent handling of missing test executables across platforms.

On Windows (lines 97-98), a missing test executable triggers Write-Error and exit 1. However, on macOS (line 181), it only emits Write-Warning and continues. This violates the stated PR objective that "missing test executables are treated as errors."

🔧 Proposed fix to make macOS consistent with Windows
     else {
-        Write-Warning "Test executable not found at: $testExe"
+        Write-Error "Test executable not found at: $testExe"
+        exit 1
     }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@build.ps1` around lines 155 - 182, The macOS branch currently treats a
missing test executable with Write-Warning which is inconsistent with the
Windows behavior; update the macOS test-executable handling so that when
Test-Path $testExe is false it calls Write-Error with a descriptive message and
then exits with exit 1 (same as Windows), ensuring $testExe, Test-Path,
Write-Error and exit 1 are used to signal a failure rather than just warning.


if ($Package) {
macdeployqt build/SQLiteQueryAnalyzer.app -dmg -appstore-compliant
}
Expand Down
11 changes: 11 additions & 0 deletions src/cli/script.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,20 @@ void Script::executeSqlFile(const QString &sqlFilePath,
QStringList errors;
executor.runScript(sqlScript, &errors);

if (!errors.isEmpty()) {
QTextStream err(stderr);
for (const auto &error : errors) {
err << "ERROR: " << error << "\n";
}
}

const auto milliseconds = static_cast<double>(time.elapsed());
const auto msg = "Script execution took " + QString::number(milliseconds / 1000) + " seconds";
QTextStream out(stdout);
out << msg;
fflush(stdout);

if (!errors.isEmpty()) {
exit(1);
}
}
11 changes: 4 additions & 7 deletions src/database/dbanalyzer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,6 @@

#include <QFileInfo>

#include "dbanalyzer.h"

#include <QFileInfo>

DbAnalyzer::DbAnalyzer(IDatabase *database)
: database(database) {
}
Expand All @@ -26,13 +22,14 @@ bool DbAnalyzer::analyze(DatabaseInfo &info) const {
void DbAnalyzer::loadTables(DatabaseInfo &info) const {
const QString sql = "SELECT * FROM sqlite_master WHERE type='table'";

if (!this->database->open()) {
const bool wasOpen = this->database->isOpen();
if (!wasOpen && !this->database->open()) {
return;
}

const QueryResult result = this->database->runStatement(sql);
if (!result.ok) {
database->close();
if (!wasOpen) database->close();
return;
}

Expand All @@ -47,7 +44,7 @@ void DbAnalyzer::loadTables(DatabaseInfo &info) const {
info.tables.append(table);
}

database->close();
if (!wasOpen) database->close();
}

void DbAnalyzer::loadColumns(DatabaseInfo &info) const {
Expand Down
11 changes: 9 additions & 2 deletions src/database/dbexportdata.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,15 @@ void DbDataExport::exportDataToSqlFile(IDatabase *database,
});

if (!streamResult.ok) {
progress->addError(table.name, streamResult.error);
continue;
}
out << "\n";
}
file->close();
progress->setCompleted();
if (!progress->hasAnyErrors()) {
progress->setCompleted();
}
}

void DbDataExport::exportDataToCsvFile(IDatabase *database,
Expand Down Expand Up @@ -106,9 +109,13 @@ void DbDataExport::exportDataToCsvFile(IDatabase *database,

file->close();
if (!streamResult.ok) {
progress->addError(table.name, streamResult.error);
QFile::remove(filename);
continue;
}
}

progress->setCompleted();
if (!progress->hasAnyErrors()) {
progress->setCompleted();
}
}
2 changes: 2 additions & 0 deletions src/database/idatabase.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ class IDatabase {

virtual void close() = 0;

[[nodiscard]] virtual bool isOpen() const = 0;

virtual void shrink() = 0;

[[nodiscard]] virtual QString getFilename() const = 0;
Expand Down
5 changes: 4 additions & 1 deletion src/database/inmemorydatabase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,13 @@
InMemoryDatabase::InMemoryDatabase() {
database = QSqlDatabase::addDatabase("QSQLITE", "in_memory_connection");
database.setDatabaseName(":memory:");
database.setHostName("localhost");
}

void InMemoryDatabase::setSource(const QString &filename) {
if (database.databaseName() == ":memory:") {
qWarning("InMemoryDatabase::setSource: cannot change source of in-memory database");
return;
}
this->source = filename;
database.setDatabaseName(filename);
}
Expand Down
4 changes: 2 additions & 2 deletions src/database/inmemorydatabase.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ class InMemoryDatabase : public IDatabase {

void close() override;

[[nodiscard]] bool isOpen() const override { return database.isOpen(); }

void shrink() override;

[[nodiscard]] QString getFilename() const override { return source; }
Expand All @@ -25,8 +27,6 @@ class InMemoryDatabase : public IDatabase {
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;
Expand Down
15 changes: 15 additions & 0 deletions src/database/progress.h
Original file line number Diff line number Diff line change
@@ -1,15 +1,23 @@
#ifndef PROGRESS_H
#define PROGRESS_H

#include <QStringList>
#include <QTextStream>
#include <stdio.h>

class ExportDataProgress {
uint64_t affectedRows = 0;
bool isComplete = false;
bool printProgress = false;
bool hasErrors = false;
QStringList errors;

public:
void reset() {
affectedRows = 0;
isComplete = false;
hasErrors = false;
errors.clear();
}

void increment() {
Expand All @@ -22,8 +30,15 @@ class ExportDataProgress {
}
}

void addError(const QString &table, const QString &message) {
hasErrors = true;
errors.append(QString("[%1] %2").arg(table, message));
}

[[nodiscard]] uint64_t getAffectedRows() const { return affectedRows; }
[[nodiscard]] bool isCompleted() const { return isComplete; }
[[nodiscard]] bool hasAnyErrors() const { return hasErrors; }
[[nodiscard]] const QStringList &getErrors() const { return errors; }
void setCompleted() { isComplete = true; }
void setShowProgress(const bool value) { printProgress = value; }
[[nodiscard]] bool isShowProgress() const { return printProgress; }
Expand Down
2 changes: 1 addition & 1 deletion src/database/queryexecutor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ QList<QueryResult> QueryExecutor::runScript(const QString &script, QStringList *
QList<QueryResult> QueryExecutor::runStatements(const QStringList &statements, QStringList *errors) {
QList<QueryResult> results;
for (const auto &raw: statements) {
const QString sql = raw.trimmed().replace('\n', "", Qt::CaseInsensitive);
const QString sql = raw.trimmed().replace('\n', "");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Replace newline stripping with whitespace normalization.

At Line 15, replacing '\n' with "" can merge tokens and break valid multiline SQL coming from the editor (SELECT ...\nFROM ... becomes SELECT ...FROM ... without spacing).

Suggested fix
-        const QString sql = raw.trimmed().replace('\n', "");
+        QString sql = raw.trimmed();
+        sql.replace('\r', ' ');
+        sql.replace('\n', ' ');
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const QString sql = raw.trimmed().replace('\n', "");
QString sql = raw.trimmed();
sql.replace('\r', ' ');
sql.replace('\n', ' ');
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/database/queryexecutor.cpp` at line 15, The current assignment to QString
sql uses raw.trimmed().replace('\n', "") which removes newlines and can glue
tokens; change it to normalize whitespace instead: replace newlines and other
whitespace runs with a single space and trim the result so multiline SQL like
"SELECT ...\nFROM ..." becomes "SELECT ... FROM ...". Update the expression that
sets sql (the line creating QString sql from raw) to perform a regex-based
whitespace collapse (e.g., replace sequences of whitespace characters with a
single space) and then call trimmed() so you preserve token separation while
still removing excess whitespace.

if (sql.isEmpty())
continue;

Expand Down
2 changes: 1 addition & 1 deletion src/database/queryresult.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ struct QueryResult {
bool isSelect = false;
QStringList columns;
QList<QueryRow> rows;
int rowsAffected = 0;
qint64 rowsAffected = 0;
};

#endif // QUERYRESULT_H
12 changes: 8 additions & 4 deletions src/database/sqlitedatabase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@
#include <QSqlRecord>

SqliteDatabase::SqliteDatabase() {
database = QSqlDatabase::addDatabase("QSQLITE");
database.setHostName("localhost");
static int instanceCounter = 0;
const QString connectionName = QString("sqlite_connection_%1").arg(++instanceCounter);
database = QSqlDatabase::addDatabase("QSQLITE", connectionName);
}

void SqliteDatabase::setSource(const QString &filename) {
Expand All @@ -16,7 +17,8 @@ void SqliteDatabase::setSource(const QString &filename) {
}

bool SqliteDatabase::open() {
this->close();
if (database.isOpen())
return true;
return database.open();
}

Expand All @@ -30,7 +32,9 @@ void SqliteDatabase::shrink() {
database.open();

QSqlQuery query(database);
query.exec("VACUUM");
if (!query.exec("VACUUM")) {
qWarning("VACUUM failed: %s", qPrintable(query.lastError().text()));
}
}

QueryResult SqliteDatabase::runStatement(const QString &sql) {
Expand Down
2 changes: 2 additions & 0 deletions src/database/sqlitedatabase.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ class SqliteDatabase : public IDatabase {

void close() override;

[[nodiscard]] bool isOpen() const override { return database.isOpen(); }

void shrink() override;

[[nodiscard]] QString getFilename() const override { return source; }
Expand Down
Loading
Loading