From 4477dd41198ac827781a6bbf50aeb0b1d3a66a45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Thu, 7 Jun 2018 15:05:32 +0200 Subject: [core] support moving Database object during a Transaction --- platform/default/sqlite3.cpp | 31 +++++++++++++++++++++---------- platform/default/sqlite3.hpp | 4 +++- platform/qt/src/sqlite3.cpp | 26 ++++++++++++++++++-------- test/storage/offline_database.test.cpp | 1 - test/storage/sqlite.test.cpp | 31 ++++++++++++++++++++++++++++++- 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(timeout.count(), std::numeric_limits::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 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 #include -#include #include #include #include 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 +#include -#include #include +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()); ASSERT_EQ(result.get().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(mapbox::sqlite::ResultCode::CantOpen), "cannot open file" }, true)); + EXPECT_EQ(1u, log.count({ EventSeverity::Info, Event::Database, static_cast(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::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 db2; + mapbox::sqlite::Transaction transaction(db1); + db2 = std::make_unique(std::move(db1)); + transaction.commit(); } -- cgit v1.2.1