From 665837ff251023d4b8b9289bb85bd92393d89b9a Mon Sep 17 00:00:00 2001
From: Martin Kleusberg <mkleusberg@gmail.com>
Date: Tue, 31 Jan 2017 19:05:21 +0100
Subject: [PATCH] Improve error handling in execution of SQL commands

This improves the error handling when executing multiple SQL commands at
once in a couple of ways.

We didn't detect any sort of possible error. For example syntax error
were reported and execution stopped but constraint errors were just
silently ignored. This is fixed now so that no silent errors should
occur.

Also we would execute the statements one after another until hitting an
error and then just stop, even if a savepoint was created before. With
this commit we're now reverting back to this savepoint and telling the
user about this. This should bring the database back to a consistent
state.

We have to remove any transaction statements from the SQL statements
because we're always already in a transactions and they can't be nested.
However, when removing a BEGIN TRANSACTION statement this would happen
silently and not in all cases a savepoint would be created instead. This
is fixed as well by making sure a savepoint is always created by this
function when a transaction was in the original list of commands.

See issues #955 and #957.
---
 src/sqlitedb.cpp | 49 +++++++++++++++++++++++++++++++++++++++---------
 src/sqlitedb.h   |  2 ++
 2 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/src/sqlitedb.cpp b/src/sqlitedb.cpp
index 2de17164..6e3616c4 100644
--- a/src/sqlitedb.cpp
+++ b/src/sqlitedb.cpp
@@ -12,6 +12,7 @@
 #include <QInputDialog>
 #include <QFileInfo>
 #include <QDir>
+#include <QDateTime>
 
 // collation callbacks
 int collCompare(void* /*pArg*/, int /*eTextRepA*/, const void* sA, int /*eTextRepB*/, const void* sB)
@@ -666,15 +667,26 @@ bool DBBrowserDB::executeMultiSQL(const QString& statement, bool dirty, bool log
 
     QString query = statement.trimmed();
 
-    query.remove(QRegExp("^\\s*BEGIN TRANSACTION;|COMMIT;\\s*$"));
+    // Check if this SQL containts any transaction statements
+    QRegExp transactionRegex("^\\s*BEGIN TRANSACTION;|COMMIT;\\s*$");
+    if(query.contains(transactionRegex))
+    {
+        // If so remove them anc create a savepoint instead by overriding the dirty parameter
+        query.remove(transactionRegex);
+        dirty = true;
+    }
 
     // Log the statement if needed
     if(log)
         logSQL(query, kLogMsg_App);
 
     // Set DB to dirty/create restore point if necessary
+    QString savepoint_name;
     if(dirty)
-        setSavepoint();
+    {
+        savepoint_name = sqlb::escapeIdentifier(generateSavepointName("execmultisql"));
+        setSavepoint(savepoint_name);
+    }
 
     // Show progress dialog
     int statement_size = query.size();
@@ -716,19 +728,32 @@ bool DBBrowserDB::executeMultiSQL(const QString& statement, bool dirty, bool log
         res = sqlite3_prepare_v2(_db, tail, tail_length, &vm, &tail);
         if(res == SQLITE_OK)
         {
-            if(sqlite3_step(vm) == SQLITE_ERROR)
+            switch(sqlite3_step(vm))
             {
+            case SQLITE_OK:
+            case SQLITE_ROW:
+            case SQLITE_DONE:
                 sqlite3_finalize(vm);
-                lastErrorMessage = tr("Error in statement #%1: %2.\n"
-                    "Aborting execution.").arg(line).arg(sqlite3_errmsg(_db));
+                break;
+            default:
+                // In case of *any* error abort the execution and roll back the transaction
+                sqlite3_finalize(vm);
+                if(dirty)
+                    revertToSavepoint(savepoint_name);
+                lastErrorMessage = tr("Error in statement #%1: %2.\nAborting execution%3.")
+                        .arg(line)
+                        .arg(sqlite3_errmsg(_db))
+                        .arg(dirty ? tr(" and rolling back") : "");
                 qWarning() << lastErrorMessage;
                 return false;
-            } else {
-                sqlite3_finalize(vm);
             }
         } else {
-            lastErrorMessage = tr("Error in statement #%1: %2.\n"
-                "Aborting execution.").arg(line).arg(sqlite3_errmsg(_db));
+            if(dirty)
+                revertToSavepoint(savepoint_name);
+            lastErrorMessage = tr("Error in statement #%1: %2.\nAborting execution%3.")
+                    .arg(line)
+                    .arg(sqlite3_errmsg(_db))
+                    .arg(dirty ? tr(" and rolling back") : "");
             qWarning() << lastErrorMessage;
             return false;
         }
@@ -1424,3 +1449,9 @@ QVector<QPair<QString, QString>> DBBrowserDB::queryColumnInformation(const QStri
 
     return result;
 }
+
+QString DBBrowserDB::generateSavepointName(const QString& identifier) const
+{
+    // Generate some sort of unique name for a savepoint for internal use.
+    return QString("db4s_%1_%2").arg(identifier).arg(QDateTime::currentMSecsSinceEpoch());
+}
diff --git a/src/sqlitedb.h b/src/sqlitedb.h
index e68c36f6..21799d3d 100644
--- a/src/sqlitedb.h
+++ b/src/sqlitedb.h
@@ -102,6 +102,8 @@ public:
 
     QVector<QPair<QString, QString>> queryColumnInformation(const QString& object_name);
 
+    QString generateSavepointName(const QString& identifier = QString()) const;
+
     sqlite3 * _db;
 
     objectMap objMap;