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 sqlitebrowser#955 and sqlitebrowser#957.
yunzixun · Jan 31, 2017 · 665837f · 665837f
1 parent 036e434
commit 665837f
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 9 deletions.
diff --git 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);
+                break;
+            default:
+                // In case of *any* error abort the execution and roll back the transaction
                 sqlite3_finalize(vm);
-                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;
-            } 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
@@ -102,6 +102,8 @@ class DBBrowserDB : public QObject
 
     QVector<QPair<QString, QString>> queryColumnInformation(const QString& object_name);
 
+    QString generateSavepointName(const QString& identifier = QString()) const;
+
     sqlite3 * _db;
 
     objectMap objMap;