diff --git a/CHANGELOG-Unreleased.md b/CHANGELOG-Unreleased.md index 097c7585a..12d8eadaf 100644 --- a/CHANGELOG-Unreleased.md +++ b/CHANGELOG-Unreleased.md @@ -26,6 +26,13 @@ - [Android] Generate `BuildConfig` explicitly (AGP 9 no longer does for libraries) and stop applying the unused Kotlin plugin, which collides with AGP 9's built-in Kotlin support - [SQLite] [iOS] Database setup failures on the legacy (non-JSI) bridge (database cannot be opened, schema setup or migration fails) now reject the promise and reach `onSetUpError` instead of raising an `NSException` that crashes the app - [SQLite] [iOS] [Android] After a failed schema setup or migration on the legacy (non-JSI) bridge, operations queued during setup are rejected with the setup error and the driver stays connected, so the app can recover with `database.unsafeResetDatabase()` instead of every call hanging (iOS) or being rejected with "No driver with tag N available" (Android). The Node.js SQLite bridge (`sqlite-node`) behaves the same way +- [SQLite] [JSI] Database setup failures on the JSI path (database file cannot be opened, schema setup or migration fails) now reject the setup promise and reach `onSetUpError` instead of crashing the app +- [SQLite] [JSI] A failed `unsafeResetDatabase()` now rejects instead of crashing the app, and leaves the (empty, consistent) database open so the reset can be retried. Other calls on a database whose setup or reset failed are rejected instead of failing an assertion +- `database.unsafeResetDatabase()` that fails now restores the adapter (and clears record caches, since data may already be erased), so the reset can be retried instead of every later call failing with "Cannot call database.adapter.X while the database is being reset" +- Adapters now clear their cached record IDs at the start of `unsafeResetDatabase()`, so a reset that fails early no longer leaves later fetches failing with "Record ID ... was sent over the bridge, but it's not cached" +- [SQLite] [JSI] Calling a native method with the wrong number of arguments now throws a JS error on Android too, instead of crashing the app +- [SQLite] [JSI] Empty or comment-only raw SQL queries, and record queries (`Q.unsafeSqlQuery`) whose first column is not `id`, now reject instead of failing a native assertion +- [SQLite] [JSI] The prepared statement cache is now bounded (256 statements, least recently used are finalized first), so one-off queries no longer keep a prepared statement alive for the lifetime of the database ### Performance diff --git a/native/android/src/main/java/com/nozbe/watermelondb/WMDatabaseDriver.java b/native/android/src/main/java/com/nozbe/watermelondb/WMDatabaseDriver.java index 154c493d5..a5eb9eab9 100644 --- a/native/android/src/main/java/com/nozbe/watermelondb/WMDatabaseDriver.java +++ b/native/android/src/main/java/com/nozbe/watermelondb/WMDatabaseDriver.java @@ -222,8 +222,10 @@ public void unsafeResetDatabase(Schema schema) { if (log != null) { log.info("Unsafe reset database"); } - database.unsafeDestroyEverything(); + // NOTE: Clear cached record IDs before anything that can fail: JS clears its record cache even if the + // reset fails, and IDs left cached here would make later fetches return records JS no longer has cachedRecords.clear(); + database.unsafeDestroyEverything(); database.transaction(() -> { database.unsafeExecuteStatements(schema.sql); database.setUserVersion(schema.version); diff --git a/native/ios/WatermelonDB/objc/WMDatabaseDriver.m b/native/ios/WatermelonDB/objc/WMDatabaseDriver.m index 487b14aaf..be0eda1cf 100644 --- a/native/ios/WatermelonDB/objc/WMDatabaseDriver.m +++ b/native/ios/WatermelonDB/objc/WMDatabaseDriver.m @@ -234,10 +234,12 @@ - (NSString *) getLocal:(NSString *)key error:(NSError **)errorPtr - (BOOL) unsafeResetDatabaseWithSchema:(NSString *)sql schemaVersion:(long)version error:(NSError **)errorPtr { + // NOTE: Clear cached record IDs before anything that can fail: JS clears its record cache even if the + // reset fails, and IDs left cached here would make later fetches return records JS no longer has + _cachedRecords = [NSMutableDictionary dictionary]; if (![_db unsafeDestroyEverything:errorPtr]) { return NO; } - _cachedRecords = [NSMutableDictionary dictionary]; __block WMDatabase *db = _db; BOOL txnResult = [db inTransaction:^BOOL(NSError **innerErrorPtr) { diff --git a/native/shared/Database-batch.cpp b/native/shared/Database-batch.cpp index e304ac9fd..ed4bec66f 100644 --- a/native/shared/Database-batch.cpp +++ b/native/shared/Database-batch.cpp @@ -87,8 +87,8 @@ void Database::batchJSON(jsi::String &&jsiJson) { sql = (std::string_view) field; } else if (fieldIdx == 3) { ondemand::array argsBatches = field; - auto stmt = prepareQuery(sql); - SqliteStatement statement(stmt); + auto statement = prepareQuery(sql); + sqlite3_stmt *stmt = statement.stmt; for (ondemand::array args : argsBatches) { // NOTE: We must capture the ID once first parsed diff --git a/native/shared/Database-query.cpp b/native/shared/Database-query.cpp index 2ea723f60..d5f255b9d 100644 --- a/native/shared/Database-query.cpp +++ b/native/shared/Database-query.cpp @@ -6,6 +6,15 @@ namespace watermelondb { using platform::consoleError; using platform::consoleLog; +// Record queries may come from the app (Q.unsafeSqlQuery), so their shape is checked, not asserted: +// the first column must be the record ID +void Database::requireIdColumn(sqlite3_stmt *statement) { + const char *column = sqlite3_column_count(statement) > 0 ? sqlite3_column_name(statement, 0) : nullptr; + if (!column || std::string(column) != "id") { + throw jsi::JSError(getRt(), "Invalid query - the first selected column must be the record id"); + } +} + jsi::Value Database::find(jsi::String &tableName, jsi::String &id) { auto &rt = getRt(); const std::lock_guard lock(mutex_); @@ -40,7 +49,7 @@ jsi::Value Database::query(jsi::String &tableName, jsi::String &sql, jsi::Array break; } - assert(std::string(sqlite3_column_name(statement.stmt, 0)) == "id"); + requireIdColumn(statement.stmt); const char *id = (const char *)sqlite3_column_text(statement.stmt, 0); if (!id) { @@ -72,7 +81,7 @@ jsi::Value Database::queryAsArray(jsi::String &tableName, jsi::String &sql, jsi: break; } - assert(std::string(sqlite3_column_name(statement.stmt, 0)) == "id"); + requireIdColumn(statement.stmt); const char *id = (const char *)sqlite3_column_text(statement.stmt, 0); if (!id) { @@ -109,7 +118,7 @@ jsi::Array Database::queryIds(jsi::String &sql, jsi::Array &arguments) { break; } - assert(std::string(sqlite3_column_name(statement.stmt, 0)) == "id"); + requireIdColumn(statement.stmt); const char *idText = (const char *)sqlite3_column_text(statement.stmt, 0); if (!idText) { @@ -149,7 +158,10 @@ jsi::Value Database::count(jsi::String &sql, jsi::Array &arguments) { auto statement = executeQuery(sql.utf8(rt), arguments); getRow(statement.stmt); - assert(sqlite3_data_count(statement.stmt) == 1); + // NOTE: The query may come from the app (Q.unsafeSqlQuery), so this is checked, not asserted + if (sqlite3_data_count(statement.stmt) < 1) { + throw jsi::JSError(rt, "Count query did not return a count"); + } int count = sqlite3_column_int(statement.stmt, 0); return jsi::Value(count); } diff --git a/native/shared/Database-sqlite.cpp b/native/shared/Database-sqlite.cpp index d7e54d713..e424205bf 100644 --- a/native/shared/Database-sqlite.cpp +++ b/native/shared/Database-sqlite.cpp @@ -8,26 +8,54 @@ namespace watermelondb { using platform::consoleError; using platform::consoleLog; -sqlite3_stmt* Database::prepareQuery(std::string sql) { - sqlite3_stmt *statement = cachedStatements_[sql]; +SqliteStatement Database::prepareQuery(std::string sql) { + auto cached = cachedStatements_.find(sql); + + if (cached != cachedStatements_.end()) { + // most recently used entries go to the front, so the least recently used ones are evicted first + statementCache_.splice(statementCache_.begin(), statementCache_, cached->second); + CachedStatement &entry = *cached->second; + assert(entry.stmt != nullptr); + // in theory, this shouldn't be necessary, since statements ought to be reset *after* use, not before use + // but still this might prevent some crashes if this is not done right + // TODO: Remove this later - should not be necessary, and it wastes time + sqlite3_reset(entry.stmt); + return SqliteStatement(entry.stmt, &entry.pinCount); + } + + sqlite3_stmt *statement = nullptr; + int resultPrepare = sqlite3_prepare_v2(db_->sqlite, sql.c_str(), -1, &statement, nullptr); + if (resultPrepare != SQLITE_OK) { + sqlite3_finalize(statement); + throw dbError("Failed to prepare query statement"); + } if (statement == nullptr) { - int resultPrepare = sqlite3_prepare_v2(db_->sqlite, sql.c_str(), -1, &statement, nullptr); + // NOTE: sqlite3_prepare_v2 succeeds without a statement if the SQL is empty or only a comment + throw jsi::JSError(getRt(), "Failed to prepare query statement - the query is empty"); + } - if (resultPrepare != SQLITE_OK) { - sqlite3_finalize(statement); - throw dbError("Failed to prepare query statement"); - } + evictCachedStatements(); + statementCache_.push_front({ sql, statement, 0 }); + cachedStatements_[sql] = statementCache_.begin(); - cachedStatements_[sql] = statement; - } else { - // in theory, this shouldn't be necessary, since statements ought to be reset *after* use, not before use - // but still this might prevent some crashes if this is not done right - // TODO: Remove this later - should not be necessary, and it wastes time - sqlite3_reset(statement); + CachedStatement &entry = statementCache_.front(); + return SqliteStatement(entry.stmt, &entry.pinCount); +} + +// Finalizes least recently used statements until there is room for one more in the cache. +// Statements pinned by a live SqliteStatement (i.e. still in use by a caller) are skipped. +void Database::evictCachedStatements() { + auto it = statementCache_.end(); + while (statementCache_.size() >= maxCachedStatements_ && it != statementCache_.begin()) { + --it; + if (it->pinCount > 0) { + continue; + } + sqlite3_finalize(it->stmt); + cachedStatements_.erase(it->sql); + it = statementCache_.erase(it); } - assert(statement != nullptr); - return statement; } void Database::bindArgs(sqlite3_stmt *statement, jsi::Array &arguments) { @@ -111,8 +139,8 @@ std::string Database::bindArgsAndReturnId(sqlite3_stmt *statement, simdjson::ond SqliteStatement Database::executeQuery(std::string sql, jsi::Array &arguments) { auto statement = prepareQuery(sql); - bindArgs(statement, arguments); - return SqliteStatement(statement); + bindArgs(statement.stmt, arguments); + return statement; } void Database::executeUpdate(sqlite3_stmt *statement) { @@ -124,16 +152,14 @@ void Database::executeUpdate(sqlite3_stmt *statement) { } void Database::executeUpdate(std::string sql, jsi::Array &args) { - auto stmt = prepareQuery(sql); - bindArgs(stmt, args); - SqliteStatement statement(stmt); - executeUpdate(stmt); + auto statement = prepareQuery(sql); + bindArgs(statement.stmt, args); + executeUpdate(statement.stmt); } void Database::executeUpdate(std::string sql) { - auto stmt = prepareQuery(sql); - SqliteStatement statement(stmt); - executeUpdate(stmt); + auto statement = prepareQuery(sql); + executeUpdate(statement.stmt); } void Database::getRow(sqlite3_stmt *stmt) { diff --git a/native/shared/Database-turboSync.cpp b/native/shared/Database-turboSync.cpp index 4557c835a..dde071ef0 100644 --- a/native/shared/Database-turboSync.cpp +++ b/native/shared/Database-turboSync.cpp @@ -116,8 +116,8 @@ jsi::Value Database::unsafeLoadFromSync(int jsonId, jsi::Object &schema, std::st auto tableSchemaArray = tableSchemas.first; auto tableSchema = tableSchemas.second; - sqlite3_stmt *stmt = prepareQuery(insertSqlFor(rt, tableName, tableSchemaArray)); - SqliteStatement statement(stmt); + auto statement = prepareQuery(insertSqlFor(rt, tableName, tableSchemaArray)); + sqlite3_stmt *stmt = statement.stmt; for (ondemand::object record : records) { // TODO: It would be much more natural to iterate over schema, and then get json's field diff --git a/native/shared/Database.cpp b/native/shared/Database.cpp index 8a4e9b421..77c56157d 100644 --- a/native/shared/Database.cpp +++ b/native/shared/Database.cpp @@ -5,7 +5,7 @@ namespace watermelondb { using platform::consoleError; using platform::consoleLog; -Database::Database(jsi::Runtime *runtime, std::string path, bool usesExclusiveLocking) : runtime_(runtime), mutex_() { +Database::Database(jsi::Runtime *runtime, std::string path, bool usesExclusiveLocking) : initialized_(false), isDestroyed_(false), mutex_(), runtime_(runtime) { db_ = std::make_unique(path); std::string initSql = ""; @@ -46,11 +46,11 @@ void Database::destroy() { return; } isDestroyed_ = true; - for (auto const &cachedStatement : cachedStatements_) { - sqlite3_stmt *statement = cachedStatement.second; - sqlite3_finalize(statement); + for (auto const &cachedStatement : statementCache_) { + sqlite3_finalize(cachedStatement.stmt); } cachedStatements_ = {}; + statementCache_ = {}; db_->destroy(); } @@ -58,6 +58,14 @@ Database::~Database() { destroy(); } +// Throws (i.e. rejects the call in JS) if the database was not successfully set up, or was closed - +// e.g. if the app keeps using it after a failed setup or reset instead of resetting it first +void Database::requireInitialized() { + if (!initialized_) { + throw jsi::JSError(getRt(), "Database is not set up - its setup or reset failed, or it was closed"); + } +} + bool Database::isCached(std::string cacheKey) { return cachedRecords_.find(cacheKey) != cachedRecords_.end(); } @@ -77,11 +85,21 @@ void Database::unsafeResetDatabase(jsi::String &schema, int schemaVersion) { // They seem to be enabling "defensive" config. So we use another obscure method to clear the database // https://www.sqlite.org/c3ref/c_dbconfig_defensive.html#sqlitedbconfigresetdatabase + // NOTE: Clear cached record IDs before anything that can fail: JS clears its record cache even if the + // reset fails, and IDs left cached here would make later fetches return records JS no longer has + cachedRecords_ = {}; + if (sqlite3_db_config(db_->sqlite, SQLITE_DBCONFIG_RESET_DATABASE, 1, 0) != SQLITE_OK) { throw jsi::JSError(rt, "Failed to enable reset database mode"); } // NOTE: We can't VACUUM in a transaction - executeMultiple("vacuum"); + try { + executeMultiple("vacuum"); + } catch (...) { + // NOTE: Reset mode must not stay enabled on a connection that keeps being used + sqlite3_db_config(db_->sqlite, SQLITE_DBCONFIG_RESET_DATABASE, 0, 0); + throw; + } if (sqlite3_db_config(db_->sqlite, SQLITE_DBCONFIG_RESET_DATABASE, 0, 0) != SQLITE_OK) { throw jsi::JSError(rt, "Failed to disable reset database mode"); @@ -89,8 +107,6 @@ void Database::unsafeResetDatabase(jsi::String &schema, int schemaVersion) { beginTransaction(); try { - cachedRecords_ = {}; - // Reinitialize schema executeMultiple(schema.utf8(rt)); setUserVersion(schemaVersion); diff --git a/native/shared/Database.h b/native/shared/Database.h index f0073c59b..43a6e329c 100644 --- a/native/shared/Database.h +++ b/native/shared/Database.h @@ -1,6 +1,7 @@ #pragma once #include +#include #include #include #include @@ -49,13 +50,29 @@ class Database : public jsi::HostObject { std::mutex mutex_; jsi::Runtime *runtime_; // TODO: std::shared_ptr would be better, but I don't know how to make it from void* in RCTCxxBridge std::unique_ptr db_; - std::unordered_map cachedStatements_; // NOTE: may contain null pointers! + + // Prepared statements are cached by their SQL string. The cache is bounded: once it holds + // maxCachedStatements_ entries, the least recently used statement is finalized to make room, + // so one-off queries (e.g. with ids inlined into the SQL) don't accumulate for the lifetime of + // the database, while the statements batch/sync/find reuse all the time stay cached. + // Entries pinned by a live SqliteStatement are never evicted. + struct CachedStatement { + std::string sql; + sqlite3_stmt *stmt; + unsigned pinCount; + }; + static constexpr size_t maxCachedStatements_ = 256; + std::list statementCache_; // most recently used first + std::unordered_map::iterator> cachedStatements_; std::unordered_set cachedRecords_; jsi::Runtime &getRt(); jsi::JSError dbError(std::string description); + void requireInitialized(); + void requireIdColumn(sqlite3_stmt *statement); - sqlite3_stmt* prepareQuery(std::string sql); + SqliteStatement prepareQuery(std::string sql); + void evictCachedStatements(); void bindArgs(sqlite3_stmt *statement, jsi::Array &arguments); std::string bindArgsAndReturnId(sqlite3_stmt *statement, simdjson::ondemand::array &args); SqliteStatement executeQuery(std::string sql, jsi::Array &arguments); diff --git a/native/shared/DatabaseBridge.cpp b/native/shared/DatabaseBridge.cpp index ef0f23bcc..c7f0d775a 100644 --- a/native/shared/DatabaseBridge.cpp +++ b/native/shared/DatabaseBridge.cpp @@ -109,7 +109,9 @@ void Database::install(jsi::Runtime *runtime) { database->unsafeResetDatabase(schema, schemaVersion); } catch (const std::exception &ex) { consoleError("Failed to set up the database correctly - " + std::string(ex.what())); - std::abort(); + // NOTE: Thrown errors reach JS as a rejected setup promise (and onSetUpError). The schema was + // applied in a transaction, so the database is empty but consistent and can be set up again + throw jsi::JSError(rt, ex.what()); } database->initialized_ = true; @@ -125,68 +127,68 @@ void Database::install(jsi::Runtime *runtime) { database->migrate(migrationSchema, fromVersion, toVersion); } catch (const std::exception &ex) { consoleError("Failed to migrate the database correctly - " + std::string(ex.what())); - return makeError(rt, ex.what()); + throw jsi::JSError(rt, ex.what()); } database->initialized_ = true; return jsi::Value::undefined(); }); createMethod(rt, adapter, "find", 2, [database](jsi::Runtime &rt, const jsi::Value *args) { - assert(database->initialized_); + database->requireInitialized(); jsi::String tableName = args[0].getString(rt); jsi::String id = args[1].getString(rt); return database->find(tableName, id); }); createMethod(rt, adapter, "query", 3, [database](jsi::Runtime &rt, const jsi::Value *args) { - assert(database->initialized_); + database->requireInitialized(); jsi::String tableName = args[0].getString(rt); jsi::String sql = args[1].getString(rt); jsi::Array arguments = args[2].getObject(rt).getArray(rt); return database->query(tableName, sql, arguments); }); createMethod(rt, adapter, "queryAsArray", 3, [database](jsi::Runtime &rt, const jsi::Value *args) { - assert(database->initialized_); + database->requireInitialized(); jsi::String tableName = args[0].getString(rt); jsi::String sql = args[1].getString(rt); jsi::Array arguments = args[2].getObject(rt).getArray(rt); return database->queryAsArray(tableName, sql, arguments); }); createMethod(rt, adapter, "queryIds", 2, [database](jsi::Runtime &rt, const jsi::Value *args) { - assert(database->initialized_); + database->requireInitialized(); jsi::String sql = args[0].getString(rt); jsi::Array arguments = args[1].getObject(rt).getArray(rt); return database->queryIds(sql, arguments); }); createMethod(rt, adapter, "unsafeQueryRaw", 2, [database](jsi::Runtime &rt, const jsi::Value *args) { - assert(database->initialized_); + database->requireInitialized(); jsi::String sql = args[0].getString(rt); jsi::Array arguments = args[1].getObject(rt).getArray(rt); return database->unsafeQueryRaw(sql, arguments); }); createMethod(rt, adapter, "count", 2, [database](jsi::Runtime &rt, const jsi::Value *args) { - assert(database->initialized_); + database->requireInitialized(); jsi::String sql = args[0].getString(rt); jsi::Array arguments = args[1].getObject(rt).getArray(rt); return database->count(sql, arguments); }); createMethod(rt, adapter, "batch", 1, [database](jsi::Runtime &rt, const jsi::Value *args) { - assert(database->initialized_); + database->requireInitialized(); jsi::Array operations = args[0].getObject(rt).getArray(rt); database->batch(operations); return jsi::Value::undefined(); }); createMethod(rt, adapter, "batchJSON", 1, [database](jsi::Runtime &rt, const jsi::Value *args) { - assert(database->initialized_); + database->requireInitialized(); database->batchJSON(args[0].getString(rt)); return jsi::Value::undefined(); }); createMethod(rt, adapter, "getLocal", 1, [database](jsi::Runtime &rt, const jsi::Value *args) { - assert(database->initialized_); + database->requireInitialized(); jsi::String key = args[0].getString(rt); return database->getLocal(key); }); createMethod(rt, adapter, "unsafeLoadFromSync", 4, [database](jsi::Runtime &rt, const jsi::Value *args) { - assert(database->initialized_); + database->requireInitialized(); auto jsonId = (int) args[0].getNumber(); auto schema = args[1].getObject(rt); auto preamble = args[2].getString(rt).utf8(rt); @@ -194,27 +196,36 @@ void Database::install(jsi::Runtime *runtime) { return database->unsafeLoadFromSync(jsonId, schema, preamble, postamble); }); createMethod(rt, adapter, "unsafeExecuteMultiple", 1, [database](jsi::Runtime &rt, const jsi::Value *args) { - assert(database->initialized_); + database->requireInitialized(); auto sqlString = args[0].getString(rt).utf8(rt); database->executeMultiple(sqlString); return jsi::Value::undefined(); }); createMethod(rt, adapter, "unsafeResetDatabase", 2, [database](jsi::Runtime &rt, const jsi::Value *args) { - assert(database->initialized_); + // NOTE: Resetting is allowed on an open database that was not (successfully) set up: that is how + // the app recovers from a failed schema setup or migration (onSetUpError -> unsafeResetDatabase()) + if (database->isDestroyed_) { + throw jsi::JSError(rt, "Cannot reset database - it has been closed"); + } jsi::String schema = args[0].getString(rt); int schemaVersion = (int)args[1].getNumber(); try { database->unsafeResetDatabase(schema, schemaVersion); + database->initialized_ = true; return jsi::Value::undefined(); } catch (const std::exception &ex) { consoleError("Failed to reset database correctly - " + std::string(ex.what())); - // Partially reset database is likely corrupted, so it's probably less bad to crash - std::abort(); + database->initialized_ = false; + // NOTE: The reset is applied in a transaction that is rolled back on failure, so the database is + // left empty but consistent and the connection stays open: JS gets a rejected promise and can + // retry unsafeResetDatabase() (same as the non-JSI bridges) instead of the app crashing. Until + // then, other calls are rejected (see requireInitialized) + throw jsi::JSError(rt, ex.what()); } }); createMethod(rt, adapter, "unsafeClose", 0, [database](jsi::Runtime &rt, const jsi::Value *args) { - assert(database->initialized_); + // NOTE: Closing is allowed on a database that was not (successfully) set up database->destroy(); database->initialized_ = false; return jsi::Value::undefined(); diff --git a/native/shared/JSIHelpers.h b/native/shared/JSIHelpers.h index 18fcb1a56..5e8bdbf7e 100644 --- a/native/shared/JSIHelpers.h +++ b/native/shared/JSIHelpers.h @@ -45,16 +45,13 @@ void createMethod(jsi::Runtime &runtime, jsi::Object &object, const char *method jsi::PropNameID name = jsi::PropNameID::forAscii(runtime, methodName); jsi::Function function = jsi::Function::createFromHostFunction(runtime, name, argCount, [methodName, argCount, func] (jsi::Runtime &rt, const jsi::Value &, const jsi::Value *args, size_t count) { - if (count != argCount) { - std::string error = std::string(methodName) + " takes " + std::to_string(argCount) + " arguments"; - #ifdef ANDROID - consoleError(error); - std::abort(); - #else - throw std::invalid_argument(error); - #endif - } + // NOTE: The argument check runs inside runBlock so that on Android the error is returned + // (see runBlock) instead of crossing the JSI boundary as a C++ exception return runBlock(rt, [&]() { + if (count != argCount) { + std::string error = std::string(methodName) + " takes " + std::to_string(argCount) + " arguments"; + throw jsi::JSError(rt, error); + } return func(rt, args); }); }); diff --git a/native/shared/Sqlite.cpp b/native/shared/Sqlite.cpp index e08153b64..a3a12df15 100644 --- a/native/shared/Sqlite.cpp +++ b/native/shared/Sqlite.cpp @@ -17,7 +17,7 @@ std::string resolveDatabasePath(std::string path) { } } -SqliteDb::SqliteDb(std::string path) { +SqliteDb::SqliteDb(std::string path) : sqlite(nullptr), isDestroyed_(false) { consoleLog("Will open database..."); platform::initializeSqlite(); #ifndef ANDROID @@ -30,10 +30,14 @@ SqliteDb::SqliteDb(std::string path) { if (openResult != SQLITE_OK) { if (sqlite) { auto error = std::string(sqlite3_errmsg(sqlite)); - throw new std::runtime_error("Error while trying to open database - " + error); + // NOTE: sqlite3_open returns a handle even on failure, and the destructor won't run when the + // constructor throws, so close it here to not leak it + sqlite3_close(sqlite); + sqlite = nullptr; + throw std::runtime_error("Error while trying to open database - " + error); } else { // whoa, sqlite couldn't allocate memory - throw new std::runtime_error("Error while trying to open database, sqlite is null - " + std::to_string(openResult)); + throw std::runtime_error("Error while trying to open database, sqlite is null - " + std::to_string(openResult)); } } assert(sqlite != nullptr); @@ -76,11 +80,23 @@ SqliteDb::~SqliteDb() { destroy(); } -SqliteStatement::SqliteStatement(sqlite3_stmt *statement) : stmt(statement) { +SqliteStatement::SqliteStatement(sqlite3_stmt *statement, unsigned *pinCount) : stmt(statement), pinCount_(pinCount) { + if (pinCount_) { + (*pinCount_)++; + } +} + +SqliteStatement::SqliteStatement(SqliteStatement &&other) noexcept : stmt(other.stmt), pinCount_(other.pinCount_) { + other.stmt = nullptr; + other.pinCount_ = nullptr; } SqliteStatement::~SqliteStatement() { reset(); + if (pinCount_) { + assert(*pinCount_ > 0); + (*pinCount_)--; + } } void SqliteStatement::reset() { diff --git a/native/shared/Sqlite.h b/native/shared/Sqlite.h index 4fe52f18d..933f0fb36 100644 --- a/native/shared/Sqlite.h +++ b/native/shared/Sqlite.h @@ -21,14 +21,24 @@ class SqliteDb { bool isDestroyed_; }; +// RAII guard for a prepared statement handed out by Database::prepareQuery: resets the statement +// when it goes out of scope and, while alive, pins the statement in the statement cache so that +// it cannot be evicted (and finalized) while a caller is still using it class SqliteStatement { public: - SqliteStatement(sqlite3_stmt *statement); + SqliteStatement(sqlite3_stmt *statement, unsigned *pinCount = nullptr); + SqliteStatement(SqliteStatement &&other) noexcept; ~SqliteStatement(); + SqliteStatement &operator=(const SqliteStatement &) = delete; + SqliteStatement(const SqliteStatement &) = delete; + sqlite3_stmt *stmt; void reset(); + +private: + unsigned *pinCount_; }; } // namespace watermelondb diff --git a/src/Database/index.js b/src/Database/index.js index 6175192e5..5c31b6b51 100644 --- a/src/Database/index.js +++ b/src/Database/index.js @@ -364,17 +364,21 @@ export default class Database { } // Clear the database - await adapter.unsafeResetDatabase() - - // Only now clear caches, since there may have been queued fetches from DB still bringing in items to cache - Object.values(this.collections.map).forEach((collection) => { - // $FlowFixMe - collection._cache.unsafeClear() - }) + try { + await adapter.unsafeResetDatabase() + } finally { + // Only now clear caches, since there may have been queued fetches from DB still bringing in items to cache + // NOTE: This is also done if the reset failed, since it may have already erased some or all data + Object.values(this.collections.map).forEach((collection) => { + // $FlowFixMe + collection._cache.unsafeClear() + }) - // Restore working Database - this._resetCount += 1 - this.adapter = adapter + // Restore working Database + // NOTE: Also done if the reset failed, so that the app can retry it (e.g. from onSetUpError) + this._resetCount += 1 + this.adapter = adapter + } } finally { this._isBeingReset = false } diff --git a/src/Database/test.js b/src/Database/test.js index c570b910d..fe4de0793 100644 --- a/src/Database/test.js +++ b/src/Database/test.js @@ -49,6 +49,57 @@ describe('Database', () => { expect(await tasks.find(m1.id)).toBe(m1) }) + it('can retry a failed reset', async () => { + const { database, tasks } = mockDatabase() + const { adapter } = database + + const m1 = await database.write(() => tasks.create()) + expect(tasks._cache.get(m1.id)).toBe(m1) + + jest + .spyOn(adapter, 'unsafeResetDatabase') + .mockImplementationOnce(() => Promise.reject(new Error('reset failed'))) + await expectToRejectWithMessage( + database.write(() => database.unsafeResetDatabase()), + 'reset failed', + ) + + // the adapter is restored (not left as the placeholder that rejects every call), and caches + // are cleared, since a failed reset may have already erased data + expect(database.adapter).toBe(adapter) + expect(database._isBeingReset).toBe(false) + expect(tasks._cache.get(m1.id)).toBe(undefined) + expect(database._resetCount).toBe(1) + + // so the reset can be retried + await database.write(() => database.unsafeResetDatabase()) + expect(adapter.unsafeResetDatabase).toHaveBeenCalledTimes(2) + expect(database._resetCount).toBe(2) + await expectToRejectWithMessage(tasks.find(m1.id), 'not found') + expect(await database.adapter.getLocal('test')).toBe(null) + }) + it('keeps adapter and JS record caches consistent when a reset fails', async () => { + const { database, tasks } = mockDatabase() + + const m1 = await database.write(() => tasks.create()) + expect(await tasks.find(m1.id)).toBe(m1) + const driver = database.adapter.underlyingAdapter._driver + + // the reset fails before the adapter erased anything + jest.spyOn(driver, '_assertNotBroken').mockImplementationOnce(() => { + throw new Error('broken driver') + }) + await expectToRejectWithMessage( + database.write(() => database.unsafeResetDatabase()), + 'broken driver', + ) + + // the record is still in the database, and must be fetched in full (not as a cached ID, + // since the JS record cache was cleared) + const m1Again = await tasks.find(m1.id) + expect(m1Again.id).toBe(m1.id) + expect(m1Again).not.toBe(m1) + }) it('increments reset count after every reset', async () => { const { database } = mockDatabase() expect(database._resetCount).toBe(0) diff --git a/src/adapters/__tests__/commonTests.js b/src/adapters/__tests__/commonTests.js index 683bcdfe9..dd5e9622e 100644 --- a/src/adapters/__tests__/commonTests.js +++ b/src/adapters/__tests__/commonTests.js @@ -1161,12 +1161,9 @@ export default () => { await expect(adapterPromise).rejects.toBeInstanceOf(Error) } - if ( - AdapterClass.name === 'SQLiteAdapter' && - adapter.underlyingAdapter._dispatcherType === 'asynchronous' - ) { - // With the asynchronous (non-JSI) bridge, the app must be able to recover from a failed - // migration (e.g. from onSetUpError) by resetting the database with the same adapter + if (AdapterClass.name === 'SQLiteAdapter') { + // The app must be able to recover from a failed migration (e.g. from onSetUpError) by + // resetting the database with the same adapter const failedAdapter = new DatabaseAdapterCompat( new AdapterClass({ schema: { ...testSchema, version: 2 }, @@ -1174,14 +1171,16 @@ export default () => { ...extraAdapterOptions, }), ) - // operations issued while the migration is still running are queued by the bridge + // operations issued while the migration is still running are queued by the asynchronous + // bridge. With JSI, setup is synchronous, so this runs after setup has already failed const queuedCount = failedAdapter.count(taskQuery()) await expect(failedAdapter.underlyingAdapter.initializingPromise).rejects.toBeInstanceOf(Error) if (platform === 'node') { // NOTE: The node bridge settles every call synchronously, so nothing was actually queued await queuedCount } else { - // queued operations are rejected with the setup error instead of hanging forever + // queued operations are rejected with the setup error instead of hanging forever, and + // (JSI) operations on a database that failed to set up are rejected instead of crashing await expect(queuedCount).rejects.toBeInstanceOf(Error) } @@ -1192,6 +1191,62 @@ export default () => { expect(await failedAdapter.count(taskQuery())).toBe(1) } }) + it('can retry a failed reset', async (adapter, AdapterClass) => { + if (AdapterClass.name !== 'SQLiteAdapter') { + // NOTE: Only SQLite resets by running the schema, which this test breaks to make the reset fail + return + } + + await adapter.batch([['create', 'tasks', mockTaskRaw({})]]) + expect(await adapter.count(taskQuery())).toBe(1) + + // make the reset fail after the database was cleared, while setting up the schema + const sqliteAdapter = adapter.underlyingAdapter + sqliteAdapter._encodedSchema = () => 'create table broken (' + try { + await expect(adapter.unsafeResetDatabase()).rejects.toBeInstanceOf(Error) + } finally { + delete sqliteAdapter._encodedSchema + } + + if (sqliteAdapter._dispatcherType === 'jsi') { + // the database is not set up until it is reset successfully + await expect(adapter.count(taskQuery())).rejects.toBeInstanceOf(Error) + } + + // the same adapter can retry the reset, and is usable afterwards + await adapter.unsafeResetDatabase() + expect(await adapter.count(taskQuery())).toBe(0) + await adapter.batch([['create', 'tasks', mockTaskRaw({})]]) + expect(await adapter.count(taskQuery())).toBe(1) + }) + it('rejects raw SQL queries it cannot run (JSI)', async (adapter, AdapterClass) => { + if ( + AdapterClass.name !== 'SQLiteAdapter' || + adapter.underlyingAdapter._dispatcherType !== 'jsi' + ) { + return + } + await adapter.batch([['create', 'tasks', mockTaskRaw({ id: 't1', text1: 'foo' })]]) + + // empty or comment-only SQL compiles to no statement at all + await expect( + adapter.unsafeQueryRaw(taskQuery(Q.unsafeSqlQuery('-- nothing to run'))), + ).rejects.toBeInstanceOf(Error) + // record queries must select the id first + await expect( + adapter.query(taskQuery(Q.unsafeSqlQuery('select text1 from tasks'))), + ).rejects.toBeInstanceOf(Error) + await expect( + adapter.queryIds(taskQuery(Q.unsafeSqlQuery('select text1 from tasks'))), + ).rejects.toBeInstanceOf(Error) + + // the database is still usable + expect(await adapter.queryIds(taskQuery(Q.unsafeSqlQuery('select id from tasks')))).toEqual([ + 't1', + ]) + expect(await adapter.count(taskQuery())).toBe(1) + }) it('can actually save and read from file system', async (_adapter, AdapterClass, extraAdapterOptions, platform) => { if (AdapterClass.name === 'LokiJSAdapter') { // Loki is tested differently diff --git a/src/adapters/lokijs/worker/DatabaseDriver.js b/src/adapters/lokijs/worker/DatabaseDriver.js index 0cb38929e..9c035ce00 100644 --- a/src/adapters/lokijs/worker/DatabaseDriver.js +++ b/src/adapters/lokijs/worker/DatabaseDriver.js @@ -237,12 +237,15 @@ export default class DatabaseDriver { } async unsafeResetDatabase(): Promise { + // NOTE: Clear cached record IDs before anything that can fail: JS clears its record cache even if the + // reset fails, and IDs left cached here would make later fetches return records JS no longer has + this.cachedRecords.clear() + // A broken driver must not delete and re-create the database (e.g. if it was deleted in // another tab, that would bring it back with this driver's autosave) this._assertNotBroken() await deleteDatabase(this.loki) - this.cachedRecords.clear() logger.log('[Loki] Database is now reset') await this._openDatabase() diff --git a/src/adapters/sqlite/makeDispatcher/index.native.js b/src/adapters/sqlite/makeDispatcher/index.native.js index 1551b46ff..944a0342e 100644 --- a/src/adapters/sqlite/makeDispatcher/index.native.js +++ b/src/adapters/sqlite/makeDispatcher/index.native.js @@ -59,14 +59,33 @@ class SqliteNativeModulesDispatcher implements SqliteDispatcher { class SqliteJsiDispatcher implements SqliteDispatcher { _db: any + _openError: ?Error // set if the database could not be opened - every call is rejected with it _unsafeErrorListener: (Error) => void // debug hook for NT use constructor(dbName: string, { usesExclusiveLocking }: SqliteDispatcherOptions): void { - this._db = global.nativeWatermelonCreateAdapter(dbName, usesExclusiveLocking) + this._openError = null + try { + this._db = global.nativeWatermelonCreateAdapter(dbName, usesExclusiveLocking) + // On Android, errors are returned, not thrown - see DatabaseBridge.cpp + if (this._db instanceof Error) { + this._openError = this._db + } + } catch (error) { + // e.g. the database file cannot be opened. Keep the error so that `initialize` is rejected + // with it (and onSetUpError is called) instead of the adapter constructor throwing + this._openError = error + } this._unsafeErrorListener = () => {} } call(name: SqliteDispatcherMethod, _args: any[], callback: ResultCallback): void { + const openError = this._openError + if (openError) { + this._unsafeErrorListener(openError) + callback({ error: openError }) + return + } + let methodName: string = name let args = _args diff --git a/src/adapters/sqlite/makeDispatcher/test.js b/src/adapters/sqlite/makeDispatcher/test.js new file mode 100644 index 000000000..e04ca56c8 --- /dev/null +++ b/src/adapters/sqlite/makeDispatcher/test.js @@ -0,0 +1,136 @@ +import { testSchema } from '../../__tests__/helpers' +import DatabaseAdapterCompat from '../../compat' +import { schemaMigrations, addColumns } from '../../../Schema/migrations' + +// NOTE: The JSI dispatcher is the native (React Native) variant of makeDispatcher. These tests drive +// it with a fake `global.nativeWatermelonCreateAdapter` to check that errors coming out of the C++ +// side (thrown on iOS, returned on Android - see DatabaseBridge.cpp / JSIHelpers.h) end up as +// rejected promises rather than uncaught exceptions +jest.mock( + 'react-native', + () => ({ NativeModules: { WMDatabaseBridge: {} }, Platform: { OS: 'android' } }), + { virtual: true }, +) +jest.mock('./index', () => require('./index.native')) + +// eslint-disable-next-line import/first +import SQLiteAdapter from '../index' + +const openError = new Error('Error while trying to open database - unable to open database file') +const setUpError = new Error('Failed to set up the database correctly - table "tasks" already exists') +const resetError = new Error('Failed to reset database correctly - disk I/O error') + +const makeAdapter = (options = {}) => { + const onSetUpError = jest.fn() + const adapter = new SQLiteAdapter({ + dbName: 'test', + schema: testSchema, + jsi: true, + onSetUpError, + ...options, + }) + expect(adapter._dispatcherType).toBe('jsi') + return { adapter, compat: new DatabaseAdapterCompat(adapter), onSetUpError } +} + +describe('SqliteJsiDispatcher', () => { + afterEach(() => { + delete global.nativeWatermelonCreateAdapter + }) + + describe.each([ + ['throws (iOS)', (error) => () => { throw error }], + ['returns an Error (Android)', (error) => () => error], + ])('when native %s', (_name, failingWith) => { + it('rejects setup and calls onSetUpError when the database cannot be opened', async () => { + global.nativeWatermelonCreateAdapter = jest.fn(failingWith(openError)) + + const { adapter, compat, onSetUpError } = makeAdapter() + await expect(adapter.initializingPromise).rejects.toBe(openError) + expect(onSetUpError).toHaveBeenCalledWith(openError) + + // there is no database to work with, so later calls are rejected too instead of throwing + await expect(compat.unsafeResetDatabase()).rejects.toBe(openError) + await expect(compat.getLocal('foo')).rejects.toBe(openError) + }) + it('rejects setup and calls onSetUpError when setting up the schema fails', async () => { + const db = { + initialize: jest.fn(() => ({ code: 'schema_needed' })), + setUpWithSchema: jest.fn(failingWith(setUpError)), + } + global.nativeWatermelonCreateAdapter = jest.fn(() => db) + + const { adapter, onSetUpError } = makeAdapter() + await expect(adapter.initializingPromise).rejects.toBe(setUpError) + expect(onSetUpError).toHaveBeenCalledWith(setUpError) + expect(db.setUpWithSchema).toHaveBeenCalledTimes(1) + }) + it('rejects setup and calls onSetUpError when a migration fails', async () => { + const db = { + initialize: jest.fn(() => ({ code: 'migrations_needed', databaseVersion: 1 })), + setUpWithMigrations: jest.fn(failingWith(setUpError)), + } + global.nativeWatermelonCreateAdapter = jest.fn(() => db) + + const onError = jest.fn() + const { adapter, onSetUpError } = makeAdapter({ + schema: { ...testSchema, version: 2 }, + migrations: schemaMigrations({ + migrations: [ + { + toVersion: 2, + steps: [addColumns({ table: 'tasks', columns: [{ name: 'foo', type: 'string' }] })], + }, + ], + }), + migrationEvents: { onStart: jest.fn(), onSuccess: jest.fn(), onError }, + }) + await expect(adapter.initializingPromise).rejects.toBe(setUpError) + expect(onSetUpError).toHaveBeenCalledWith(setUpError) + expect(onError).toHaveBeenCalledWith(setUpError) + }) + it('rejects unsafeResetDatabase and lets it be retried', async () => { + const db = { + initialize: jest.fn(() => ({ code: 'ok' })), + unsafeResetDatabase: jest.fn(failingWith(resetError)), + getLocal: jest.fn(() => null), + } + global.nativeWatermelonCreateAdapter = jest.fn(() => db) + + const { adapter, compat, onSetUpError } = makeAdapter() + await adapter.initializingPromise + expect(onSetUpError).not.toHaveBeenCalled() + + await expect(compat.unsafeResetDatabase()).rejects.toBe(resetError) + + // the database stays usable - e.g. to retry the reset + db.unsafeResetDatabase.mockImplementation(() => undefined) + await expect(compat.unsafeResetDatabase()).resolves.toBeUndefined() + expect(db.unsafeResetDatabase).toHaveBeenCalledTimes(2) + expect(await compat.getLocal('foo')).toBe(null) + }) + }) + + it('rejects calls made with the wrong number of arguments', async () => { + // JSIHelpers.h returns an Error (Android) / throws (iOS) when a host function is called with the + // wrong number of arguments - either way the call must be rejected, not crash + const db = { + initialize: jest.fn(() => ({ code: 'ok' })), + getLocal: jest.fn((...args) => + args.length === 1 ? 'value' : new Error(`getLocal takes 1 arguments`), + ), + } + global.nativeWatermelonCreateAdapter = jest.fn(() => db) + + const { adapter, compat } = makeAdapter() + await adapter.initializingPromise + expect(await compat.getLocal('foo')).toBe('value') + await expect( + new Promise((resolve, reject) => + adapter._dispatcher.call('getLocal', ['foo', 'extra'], (result) => + result.error ? reject(result.error) : resolve(result.value), + ), + ), + ).rejects.toThrow('getLocal takes 1 arguments') + }) +}) diff --git a/src/adapters/sqlite/sqlite-node/DatabaseDriver.js b/src/adapters/sqlite/sqlite-node/DatabaseDriver.js index fc1351aaa..fe0ae5f34 100644 --- a/src/adapters/sqlite/sqlite-node/DatabaseDriver.js +++ b/src/adapters/sqlite/sqlite-node/DatabaseDriver.js @@ -213,8 +213,10 @@ class DatabaseDriver { } unsafeResetDatabase(schema: { sql: string, version: number }): void { - this.database.unsafeDestroyEverything() + // NOTE: Clear cached record IDs before anything that can fail: JS clears its record cache even if the + // reset fails, and IDs left cached here would make later fetches return records JS no longer has this.cachedRecords = {} + this.database.unsafeDestroyEverything() this.database.inTransaction(() => { this.database.executeStatements(schema.sql)