[core] support moving Database object during a Transaction

5 files changed, 72 insertions, 21 deletions

diff --git a/platform/default/sqlite3.cpp b/platform/default/sqlite3.cpp
index fb293b2b5b..2e76f6eec4 100644
--- a/platform/default/sqlite3.cpp
+++ b/platform/default/sqlite3.cpp
@@ -27,6 +27,9 @@ public:
         }
     }
 
+    void setBusyTimeout(std::chrono::milliseconds timeout);
+    void exec(const std::string& sql);
+
     sqlite3* db;
 };
 
@@ -116,23 +119,31 @@ Database::~Database() = default;
 
 void Database::setBusyTimeout(std::chrono::milliseconds timeout) {
     assert(impl);
-    const int err = sqlite3_busy_timeout(impl->db,
+    impl->setBusyTimeout(timeout);
+}
+
+void DatabaseImpl::setBusyTimeout(std::chrono::milliseconds timeout) {
+    const int err = sqlite3_busy_timeout(db,
         int(std::min<std::chrono::milliseconds::rep>(timeout.count(), std::numeric_limits<int>::max())));
     if (err != SQLITE_OK) {
-        throw Exception { err, sqlite3_errmsg(impl->db) };
+        throw Exception { err, sqlite3_errmsg(db) };
     }
 }
 
 void Database::exec(const std::string &sql) {
     assert(impl);
+    impl->exec(sql);
+}
+
+void DatabaseImpl::exec(const std::string& sql) {
     char *msg = nullptr;
-    const int err = sqlite3_exec(impl->db, sql.c_str(), nullptr, nullptr, &msg);
+    const int err = sqlite3_exec(db, sql.c_str(), nullptr, nullptr, &msg);
     if (msg) {
         const std::string message = msg;
         sqlite3_free(msg);
         throw Exception { err, message };
     } else if (err != SQLITE_OK) {
-        throw Exception { err, sqlite3_errmsg(impl->db) };
+        throw Exception { err, sqlite3_errmsg(db) };
     }
 }
 
@@ -405,16 +416,16 @@ uint64_t Query::changes() const {
 }
 
 Transaction::Transaction(Database& db_, Mode mode)
-    : db(db_) {
+    : dbImpl(*db_.impl) {
     switch (mode) {
     case Deferred:
-        db.exec("BEGIN DEFERRED TRANSACTION");
+        dbImpl.exec("BEGIN DEFERRED TRANSACTION");
         break;
     case Immediate:
-        db.exec("BEGIN IMMEDIATE TRANSACTION");
+        dbImpl.exec("BEGIN IMMEDIATE TRANSACTION");
         break;
     case Exclusive:
-        db.exec("BEGIN EXCLUSIVE TRANSACTION");
+        dbImpl.exec("BEGIN EXCLUSIVE TRANSACTION");
         break;
     }
 }
@@ -431,12 +442,12 @@ Transaction::~Transaction() {
 
 void Transaction::commit() {
     needRollback = false;
-    db.exec("COMMIT TRANSACTION");
+    dbImpl.exec("COMMIT TRANSACTION");
 }
 
 void Transaction::rollback() {
     needRollback = false;
-    db.exec("ROLLBACK TRANSACTION");
+    dbImpl.exec("ROLLBACK TRANSACTION");
 }
 
 } // namespace sqlite
diff --git a/platform/default/sqlite3.hpp b/platform/default/sqlite3.hpp
index cdc94298fe..52cc3f8fc5 100644
--- a/platform/default/sqlite3.hpp
+++ b/platform/default/sqlite3.hpp
@@ -69,6 +69,7 @@ class DatabaseImpl;
 class Statement;
 class StatementImpl;
 class Query;
+class Transaction;
 
 class Database {
 private:
@@ -91,6 +92,7 @@ private:
     std::unique_ptr<DatabaseImpl> impl;
 
     friend class Statement;
+    friend class Transaction;
 };
 
 // A Statement object represents a prepared statement that can be run repeatedly run with a Query object.
@@ -173,7 +175,7 @@ public:
     void rollback();
 
 private:
-    Database& db;
+    DatabaseImpl& dbImpl;
     bool needRollback = true;
 };
 
diff --git a/platform/qt/src/sqlite3.cpp b/platform/qt/src/sqlite3.cpp
index 351991f881..9617bac6cd 100644
--- a/platform/qt/src/sqlite3.cpp
+++ b/platform/qt/src/sqlite3.cpp
@@ -72,6 +72,9 @@ public:
         checkDatabaseError(db);
     }
 
+    void setBusyTimeout(std::chrono::milliseconds timeout);
+    void exec(const std::string& sql);
+
     QString connectionName;
 };
 
@@ -156,12 +159,15 @@ Database::~Database() {
 
 void Database::setBusyTimeout(std::chrono::milliseconds timeout) {
     assert(impl);
+    impl->setBusyTimeout(timeout);
+}
 
+void DatabaseImpl::setBusyTimeout(std::chrono::milliseconds timeout) {
     // std::chrono::milliseconds.count() is a long and Qt will cast
     // internally to int, so we need to make sure the limits apply.
     std::string timeoutStr = mbgl::util::toString(timeout.count() & INT_MAX);
 
-    auto db = QSqlDatabase::database(impl->connectionName);
+    auto db = QSqlDatabase::database(connectionName);
     QString connectOptions = db.connectOptions();
     if (connectOptions.isEmpty()) {
         if (!connectOptions.isEmpty()) connectOptions.append(';');
@@ -180,13 +186,17 @@ void Database::setBusyTimeout(std::chrono::milliseconds timeout) {
 
 void Database::exec(const std::string &sql) {
     assert(impl);
+    impl->exec(sql);
+}
+
+void DatabaseImpl::exec(const std::string& sql) {
     QStringList statements = QString::fromStdString(sql).split(';', QString::SkipEmptyParts);
     statements.removeAll("\n");
     for (QString statement : statements) {
         if (!statement.endsWith(';')) {
             statement.append(';');
         }
-        QSqlQuery query(QSqlDatabase::database(impl->connectionName));
+        QSqlQuery query(QSqlDatabase::database(connectionName));
         query.prepare(statement);
 
         if (!query.exec()) {
@@ -433,16 +443,16 @@ uint64_t Query::changes() const {
 }
 
 Transaction::Transaction(Database& db_, Mode mode)
-        : db(db_) {
+    : dbImpl(*db_.impl) {
     switch (mode) {
     case Deferred:
-        db.exec("BEGIN DEFERRED TRANSACTION");
+        dbImpl.exec("BEGIN DEFERRED TRANSACTION");
         break;
     case Immediate:
-        db.exec("BEGIN IMMEDIATE TRANSACTION");
+        dbImpl.exec("BEGIN IMMEDIATE TRANSACTION");
         break;
     case Exclusive:
-        db.exec("BEGIN EXCLUSIVE TRANSACTION");
+        dbImpl.exec("BEGIN EXCLUSIVE TRANSACTION");
         break;
     }
 }
@@ -459,12 +469,12 @@ Transaction::~Transaction() {
 
 void Transaction::commit() {
     needRollback = false;
-    db.exec("COMMIT TRANSACTION");
+    dbImpl.exec("COMMIT TRANSACTION");
 }
 
 void Transaction::rollback() {
     needRollback = false;
-    db.exec("ROLLBACK TRANSACTION");
+    dbImpl.exec("ROLLBACK TRANSACTION");
 }
 
 } // namespace sqlite
diff --git a/test/storage/offline_database.test.cpp b/test/storage/offline_database.test.cpp
index cb75551e40..e54cd6813b 100644
--- a/test/storage/offline_database.test.cpp
+++ b/test/storage/offline_database.test.cpp
@@ -7,7 +7,6 @@
 #include <mbgl/util/io.hpp>
 #include <mbgl/util/string.hpp>
 
-#include <gtest/gtest.h>
 #include <sqlite3.hpp>
 #include <thread>
 #include <random>
diff --git a/test/storage/sqlite.test.cpp b/test/storage/sqlite.test.cpp
index 553736a0b1..a7c9d95e9b 100644
--- a/test/storage/sqlite.test.cpp
+++ b/test/storage/sqlite.test.cpp
@@ -1,8 +1,10 @@
 #include <mbgl/test/util.hpp>
+#include <mbgl/test/fixture_log_observer.hpp>
 
-#include <gtest/gtest.h>
 #include <sqlite3.hpp>
 
+using namespace mbgl;
+
 TEST(SQLite, Statement) {
     using namespace mbgl;
 
@@ -29,9 +31,36 @@ TEST(SQLite, Statement) {
 }
 
 TEST(SQLite, TEST_REQUIRES_WRITE(TryOpen)) {
+    FixtureLog log;
+
     // Should return a CANTOPEN exception when the database doesn't exist,
     // make sure all the backends behave the same way.
     auto result = mapbox::sqlite::Database::tryOpen("test/fixtures/offline_database/foobar123.db", mapbox::sqlite::ReadOnly);
     ASSERT_TRUE(result.is<mapbox::sqlite::Exception>());
     ASSERT_EQ(result.get<mapbox::sqlite::Exception>().code, mapbox::sqlite::ResultCode::CantOpen);
+
+#ifndef __QT__
+    // Only non-Qt platforms are setting a logger on the SQLite object.
+    EXPECT_EQ(1u, log.count({ EventSeverity::Info, Event::Database, static_cast<int64_t>(mapbox::sqlite::ResultCode::CantOpen), "cannot open file" }, true));
+    EXPECT_EQ(1u, log.count({ EventSeverity::Info, Event::Database, static_cast<int64_t>(mapbox::sqlite::ResultCode::CantOpen), "No such file or directory" }, true));
+#endif
+    EXPECT_EQ(0u, log.uncheckedCount());
+}
+
+TEST(SQLite, CloseDatabaseWithPendingTransaction) {
+    auto db = std::make_unique<mapbox::sqlite::Database>(mapbox::sqlite::Database::open(
+        ":memory:", mapbox::sqlite::ReadWrite | mapbox::sqlite::Create));
+    mapbox::sqlite::Transaction transaction(*db);
+    transaction.commit();
+}
+
+TEST(SQLite, CloseMovedDatabaseWithPendingTransaction) {
+    // Verifies that we can correctly commit a transaction even if we move the Database object to
+    // another address.
+    auto db1 = mapbox::sqlite::Database::open(":memory:",
+                                              mapbox::sqlite::ReadWrite | mapbox::sqlite::Create);
+    std::unique_ptr<mapbox::sqlite::Database> db2;
+    mapbox::sqlite::Transaction transaction(db1);
+    db2 = std::make_unique<mapbox::sqlite::Database>(std::move(db1));
+    transaction.commit();
 }