Avoid exceptions for flow control during database creationupstream/fix-10796

Unfortuntely, it's difficult to avoid all exceptions, because sqlite3_open_v2 does not reliably return SQLITE_NOTADB if the file is not a database. However, this should avoid cases where developers misinterpret the SQLITE_CANTOPEN exception as a crash, which is the common case.

7 files changed, 137 insertions, 102 deletions

diff --git a/platform/default/mbgl/storage/offline_database.cpp b/platform/default/mbgl/storage/offline_database.cpp
index 4611e69f43..2b872ab87b 100644
--- a/platform/default/mbgl/storage/offline_database.cpp
+++ b/platform/default/mbgl/storage/offline_database.cpp
@@ -27,44 +27,58 @@ OfflineDatabase::~OfflineDatabase() {
     }
 }
 
-void OfflineDatabase::connect(int flags) {
-    db = std::make_unique<mapbox::sqlite::Database>(path.c_str(), flags);
-    db->setBusyTimeout(Milliseconds::max());
-    db->exec("PRAGMA foreign_keys = ON");
-}
-
 void OfflineDatabase::ensureSchema() {
     if (path != ":memory:") {
-        try {
-            connect(mapbox::sqlite::ReadWrite);
-
-            switch (userVersion()) {
-            case 0: break; // cache-only database; ok to delete
-            case 1: break; // cache-only database; ok to delete
-            case 2: migrateToVersion3(); // fall through
-            case 3: // no-op and fall through
-            case 4: migrateToVersion5(); // fall through
-            case 5: migrateToVersion6(); // fall through
-            case 6: return;
-            default: break; // downgrade, delete the database
-            }
-
-            removeExisting();
-            connect(mapbox::sqlite::ReadWrite | mapbox::sqlite::Create);
-        } catch (mapbox::sqlite::Exception& ex) {
-            if (ex.code != mapbox::sqlite::ResultCode::CantOpen && ex.code != mapbox::sqlite::ResultCode::NotADB) {
+        auto result = mapbox::sqlite::Database::tryOpen(path, mapbox::sqlite::ReadWrite);
+        if (result.is<mapbox::sqlite::Exception>()) {
+            const auto& ex = result.get<mapbox::sqlite::Exception>();
+            if (ex.code == mapbox::sqlite::ResultCode::NotADB) {
+                // Corrupted; blow it away.
+                removeExisting();
+            } else if (ex.code == mapbox::sqlite::ResultCode::CantOpen) {
+                // Doesn't exist yet.
+            } else {
                 Log::Error(Event::Database, "Unexpected error connecting to database: %s", ex.what());
-                throw;
+                throw ex;
             }
-
+        } else {
             try {
+                db = std::make_unique<mapbox::sqlite::Database>(std::move(result.get<mapbox::sqlite::Database>()));
+                db->setBusyTimeout(Milliseconds::max());
+                db->exec("PRAGMA foreign_keys = ON");
+
+                switch (userVersion()) {
+                case 0:
+                case 1:
+                    // cache-only database; ok to delete
+                    removeExisting();
+                    break;
+                case 2:
+                    migrateToVersion3();
+                    // fall through
+                case 3:
+                case 4:
+                    migrateToVersion5();
+                    // fall through
+                case 5:
+                    migrateToVersion6();
+                    // fall through
+                case 6:
+                    // happy path; we're done
+                    return;
+                default:
+                    // downgrade, delete the database
+                    removeExisting();
+                    break;
+                }
+            } catch (const mapbox::sqlite::Exception& ex) {
+                // Unfortunately, SQLITE_NOTADB is not always reported upon opening the database.
+                // Apparently sometimes it is delayed until the first read operation.
                 if (ex.code == mapbox::sqlite::ResultCode::NotADB) {
                     removeExisting();
+                } else {
+                    throw;
                 }
-                connect(mapbox::sqlite::ReadWrite | mapbox::sqlite::Create);
-            } catch (...) {
-                Log::Error(Event::Database, "Unexpected error creating database: %s", util::toString(std::current_exception()).c_str());
-                throw;
             }
         }
     }
@@ -72,9 +86,9 @@ void OfflineDatabase::ensureSchema() {
     try {
         #include "offline_schema.cpp.include"
 
-        connect(mapbox::sqlite::ReadWrite | mapbox::sqlite::Create);
-
-        // If you change the schema you must write a migration from the previous version.
+        db = std::make_unique<mapbox::sqlite::Database>(mapbox::sqlite::Database::open(path, mapbox::sqlite::ReadWrite | mapbox::sqlite::Create));
+        db->setBusyTimeout(Milliseconds::max());
+        db->exec("PRAGMA foreign_keys = ON");
         db->exec("PRAGMA auto_vacuum = INCREMENTAL");
         db->exec("PRAGMA journal_mode = DELETE");
         db->exec("PRAGMA synchronous = FULL");
@@ -93,6 +107,7 @@ int OfflineDatabase::userVersion() {
 void OfflineDatabase::removeExisting() {
     Log::Warning(Event::Database, "Removing existing incompatible offline database");
 
+    statements.clear();
     db.reset();
 
     try {
diff --git a/platform/default/mbgl/storage/offline_database.hpp b/platform/default/mbgl/storage/offline_database.hpp
index 9673ad8212..e0d90a9a15 100644
--- a/platform/default/mbgl/storage/offline_database.hpp
+++ b/platform/default/mbgl/storage/offline_database.hpp
@@ -59,7 +59,6 @@ public:
     uint64_t getOfflineMapboxTileCount();
 
 private:
-    void connect(int flags);
     int userVersion();
     void ensureSchema();
     void removeExisting();
diff --git a/platform/default/sqlite3.cpp b/platform/default/sqlite3.cpp
index 8f9c34191f..776adc8f3a 100644
--- a/platform/default/sqlite3.cpp
+++ b/platform/default/sqlite3.cpp
@@ -14,27 +14,20 @@ namespace sqlite {
 
 class DatabaseImpl {
 public:
-    DatabaseImpl(const char* filename, int flags)
+    DatabaseImpl(sqlite3* db_)
+        : db(db_)
     {
-        const int error = sqlite3_open_v2(filename, &db, flags, nullptr);
-        if (error != SQLITE_OK) {
-            const auto message = sqlite3_errmsg(db);
-            db = nullptr;
-            throw Exception { error, message };
-        }
     }
 
     ~DatabaseImpl()
     {
-        if (!db) return;
-
         const int error = sqlite3_close(db);
         if (error != SQLITE_OK) {
             mbgl::Log::Error(mbgl::Event::Database, "%s (Code %i)", sqlite3_errmsg(db), error);
         }
     }
 
-    sqlite3* db = nullptr;
+    sqlite3* db;
 };
 
 class StatementImpl {
@@ -164,11 +157,29 @@ const static bool sqliteVersionCheck __attribute__((unused)) = []() {
     return true;
 }();
 
-Database::Database(const std::string &filename, int flags)
-    : impl(std::make_unique<DatabaseImpl>(filename.c_str(), flags))
-{
+mapbox::util::variant<Database, Exception> Database::tryOpen(const std::string &filename, int flags) {
+    sqlite3* db = nullptr;
+    const int error = sqlite3_open_v2(filename.c_str(), &db, flags, nullptr);
+    if (error != SQLITE_OK) {
+        const auto message = sqlite3_errmsg(db);
+        return Exception { error, message };
+    }
+    return Database(std::make_unique<DatabaseImpl>(db));
 }
 
+Database Database::open(const std::string &filename, int flags) {
+    auto result = tryOpen(filename, flags);
+    if (result.is<Exception>()) {
+        throw result.get<Exception>();
+    } else {
+        return std::move(result.get<Database>());
+    }
+}
+
+Database::Database(std::unique_ptr<DatabaseImpl> impl_)
+    : impl(std::move(impl_))
+{}
+
 Database::Database(Database &&other)
     : impl(std::move(other.impl)) {}
 
diff --git a/platform/default/sqlite3.hpp b/platform/default/sqlite3.hpp
index 20d09b550c..cdc94298fe 100644
--- a/platform/default/sqlite3.hpp
+++ b/platform/default/sqlite3.hpp
@@ -5,6 +5,7 @@
 #include <stdexcept>
 #include <chrono>
 #include <memory>
+#include <mapbox/variant.hpp>
 
 namespace mapbox {
 namespace sqlite {
@@ -71,11 +72,14 @@ class Query;
 
 class Database {
 private:
+    Database(std::unique_ptr<DatabaseImpl>);
     Database(const Database &) = delete;
     Database &operator=(const Database &) = delete;
 
 public:
-    Database(const std::string &filename, int flags = 0);
+    static mapbox::util::variant<Database, Exception> tryOpen(const std::string &filename, int flags = 0);
+    static Database open(const std::string &filename, int flags = 0);
+
     Database(Database &&);
     ~Database();
     Database &operator=(Database &&);
diff --git a/platform/qt/src/sqlite3.cpp b/platform/qt/src/sqlite3.cpp
index 4bcaea0e31..351991f881 100644
--- a/platform/qt/src/sqlite3.cpp
+++ b/platform/qt/src/sqlite3.cpp
@@ -52,15 +52,6 @@ void checkDatabaseError(const QSqlDatabase &db) {
     }
 }
 
-void checkDatabaseOpenError(const QSqlDatabase &db) {
-    // Assume every error when opening the data as CANTOPEN. Qt
-    // always returns -1 for `nativeErrorCode()` on database errors.
-    QSqlError lastError = db.lastError();
-    if (lastError.type() != QSqlError::NoError) {
-        throw Exception { ResultCode::CantOpen, "Error opening the database." };
-    }
-}
-
 namespace {
     QString incrementCounter() {
         static QAtomicInt count = 0;
@@ -70,32 +61,9 @@ namespace {
 
 class DatabaseImpl {
 public:
-    DatabaseImpl(const char* filename, int flags)
-        : connectionName(QString::number(uint64_t(QThread::currentThread())) + incrementCounter())
+    DatabaseImpl(QString connectionName_)
+        : connectionName(std::move(connectionName_))
     {
-        if (!QSqlDatabase::drivers().contains("QSQLITE")) {
-            throw Exception { ResultCode::CantOpen, "SQLite driver not found." };
-        }
-
-        assert(!QSqlDatabase::contains(connectionName));
-        auto db = QSqlDatabase::addDatabase("QSQLITE", connectionName);
-
-        QString connectOptions = db.connectOptions();
-        if (flags & OpenFlag::ReadOnly) {
-            if (!connectOptions.isEmpty()) connectOptions.append(';');
-            connectOptions.append("QSQLITE_OPEN_READONLY");
-        }
-        if (flags & OpenFlag::SharedCache) {
-            if (!connectOptions.isEmpty()) connectOptions.append(';');
-            connectOptions.append("QSQLITE_ENABLE_SHARED_CACHE");
-        }
-
-        db.setConnectOptions(connectOptions);
-        db.setDatabaseName(QString(filename));
-
-        if (!db.open()) {
-            checkDatabaseOpenError(db);
-        }
     }
 
     ~DatabaseImpl() {
@@ -127,12 +95,51 @@ public:
 template <typename T>
 using optional = std::experimental::optional<T>;
 
+mapbox::util::variant<Database, Exception> Database::tryOpen(const std::string &filename, int flags) {
+    if (!QSqlDatabase::drivers().contains("QSQLITE")) {
+        return Exception { ResultCode::CantOpen, "SQLite driver not found." };
+    }
 
-Database::Database(const std::string& file, int flags)
-        : impl(std::make_unique<DatabaseImpl>(file.c_str(), flags)) {
-    assert(impl);
+    QString connectionName = QString::number(uint64_t(QThread::currentThread())) + incrementCounter();
+
+    assert(!QSqlDatabase::contains(connectionName));
+    auto db = QSqlDatabase::addDatabase("QSQLITE", connectionName);
+
+    QString connectOptions = db.connectOptions();
+    if (flags & OpenFlag::ReadOnly) {
+        if (!connectOptions.isEmpty()) connectOptions.append(';');
+        connectOptions.append("QSQLITE_OPEN_READONLY");
+    }
+    if (flags & OpenFlag::SharedCache) {
+        if (!connectOptions.isEmpty()) connectOptions.append(';');
+        connectOptions.append("QSQLITE_ENABLE_SHARED_CACHE");
+    }
+
+    db.setConnectOptions(connectOptions);
+    db.setDatabaseName(QString(filename.c_str()));
+
+    if (!db.open()) {
+        // Assume every error when opening the data as CANTOPEN. Qt
+        // always returns -1 for `nativeErrorCode()` on database errors.
+        return Exception { ResultCode::CantOpen, "Error opening the database." };
+    }
+
+    return Database(std::make_unique<DatabaseImpl>(connectionName));
+}
+
+Database Database::open(const std::string &filename, int flags) {
+    auto result = tryOpen(filename, flags);
+    if (result.is<Exception>()) {
+        throw result.get<Exception>();
+    } else {
+        return std::move(result.get<Database>());
+    }
 }
 
+Database::Database(std::unique_ptr<DatabaseImpl> impl_)
+    : impl(std::move(impl_))
+{}
+
 Database::Database(Database &&other)
         : impl(std::move(other.impl)) {
     assert(impl);
@@ -165,7 +172,9 @@ void Database::setBusyTimeout(std::chrono::milliseconds timeout) {
     }
     db.setConnectOptions(connectOptions);
     if (!db.open()) {
-        checkDatabaseOpenError(db);
+        // Assume every error when opening the data as CANTOPEN. Qt
+        // always returns -1 for `nativeErrorCode()` on database errors.
+        throw Exception { ResultCode::CantOpen, "Error opening the database." };
     }
 }
 
diff --git a/test/storage/offline_database.test.cpp b/test/storage/offline_database.test.cpp
index 620e6eaa6d..656231eebe 100644
--- a/test/storage/offline_database.test.cpp
+++ b/test/storage/offline_database.test.cpp
@@ -66,7 +66,7 @@ TEST(OfflineDatabase, TEST_REQUIRES_WRITE(SchemaVersion)) {
     std::string path("test/fixtures/offline_database/offline.db");
 
     {
-        mapbox::sqlite::Database db{ path, mapbox::sqlite::Create | mapbox::sqlite::ReadWrite };
+        mapbox::sqlite::Database db = mapbox::sqlite::Database::open(path, mapbox::sqlite::Create | mapbox::sqlite::ReadWrite);
         db.exec("PRAGMA user_version = 1");
     }
 
@@ -599,7 +599,7 @@ TEST(OfflineDatabase, OfflineMapboxTileCount) {
 }
 
 static int databasePageCount(const std::string& path) {
-    mapbox::sqlite::Database db{ path, mapbox::sqlite::ReadOnly };
+    mapbox::sqlite::Database db = mapbox::sqlite::Database::open(path, mapbox::sqlite::ReadOnly);
     mapbox::sqlite::Statement stmt{ db, "pragma page_count" };
     mapbox::sqlite::Query query{ stmt };
     query.run();
@@ -607,7 +607,7 @@ static int databasePageCount(const std::string& path) {
 }
 
 static int databaseUserVersion(const std::string& path) {
-    mapbox::sqlite::Database db{ path, mapbox::sqlite::ReadOnly };
+    mapbox::sqlite::Database db = mapbox::sqlite::Database::open(path, mapbox::sqlite::ReadOnly);
     mapbox::sqlite::Statement stmt{ db, "pragma user_version" };
     mapbox::sqlite::Query query{ stmt };
     query.run();
@@ -615,7 +615,7 @@ static int databaseUserVersion(const std::string& path) {
 }
 
 static std::string databaseJournalMode(const std::string& path) {
-    mapbox::sqlite::Database db{ path, mapbox::sqlite::ReadOnly };
+    mapbox::sqlite::Database db = mapbox::sqlite::Database::open(path, mapbox::sqlite::ReadOnly);
     mapbox::sqlite::Statement stmt{ db, "pragma journal_mode" };
     mapbox::sqlite::Query query{ stmt };
     query.run();
@@ -623,7 +623,7 @@ static std::string databaseJournalMode(const std::string& path) {
 }
 
 static int databaseSyncMode(const std::string& path) {
-    mapbox::sqlite::Database db{ path, mapbox::sqlite::ReadOnly };
+    mapbox::sqlite::Database db = mapbox::sqlite::Database::open(path, mapbox::sqlite::ReadOnly);
     mapbox::sqlite::Statement stmt{ db, "pragma synchronous" };
     mapbox::sqlite::Query query{ stmt };
     query.run();
@@ -631,7 +631,7 @@ static int databaseSyncMode(const std::string& path) {
 }
 
 static std::vector<std::string> databaseTableColumns(const std::string& path, const std::string& name) {
-    mapbox::sqlite::Database db{ path, mapbox::sqlite::ReadOnly };
+    mapbox::sqlite::Database db = mapbox::sqlite::Database::open(path, mapbox::sqlite::ReadOnly);
     const auto sql = std::string("pragma table_info(") + name + ")";
     mapbox::sqlite::Statement stmt{ db, sql.c_str() };
     mapbox::sqlite::Query query{ stmt };
diff --git a/test/storage/sqlite.test.cpp b/test/storage/sqlite.test.cpp
index 918200181f..553736a0b1 100644
--- a/test/storage/sqlite.test.cpp
+++ b/test/storage/sqlite.test.cpp
@@ -6,7 +6,7 @@
 TEST(SQLite, Statement) {
     using namespace mbgl;
 
-    mapbox::sqlite::Database db(":memory:", mapbox::sqlite::Create | mapbox::sqlite::ReadWrite);
+    mapbox::sqlite::Database db = mapbox::sqlite::Database::open(":memory:", mapbox::sqlite::Create | mapbox::sqlite::ReadWrite);
     db.exec("CREATE TABLE test (id INTEGER);");
 
     mapbox::sqlite::Statement stmt1{ db, "INSERT INTO test (id) VALUES (?1);" };
@@ -28,13 +28,10 @@ TEST(SQLite, Statement) {
     ASSERT_EQ(query2.changes(), 1u);
 }
 
-TEST(SQLite, TEST_REQUIRES_WRITE(CantOpenException)) {
-    try {
-        // Should throw a CANTOPEN when the database doesn't exist,
-        // make sure all the backends behave the same way.
-        mapbox::sqlite::Database("test/fixtures/offline_database/foobar123.db", mapbox::sqlite::ReadOnly);
-        FAIL();
-    } catch (mapbox::sqlite::Exception& ex) {
-        ASSERT_EQ(ex.code, mapbox::sqlite::ResultCode::CantOpen);
-    }
+TEST(SQLite, TEST_REQUIRES_WRITE(TryOpen)) {
+    // 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);
 }