Avoid exceptions for flow control during database creation

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.

4 files changed, 77 insertions, 48 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 &&);