From b189a2bcb2747de8ee83101d27e7ffb916a59563 Mon Sep 17 00:00:00 2001 From: Simon Binder Date: Sat, 2 Nov 2019 11:48:11 +0100 Subject: [PATCH] Better errors on QueryExecutor misuse, fix beforeOpen --- .../tests/lib/database/database.dart | 7 +++- moor/lib/src/runtime/database.dart | 13 +++--- .../src/runtime/executor/helpers/engines.dart | 42 ++++++++++++++++++- .../src/runtime/executor/transactions.dart | 13 ++++++ 4 files changed, 63 insertions(+), 12 deletions(-) diff --git a/extras/integration_tests/tests/lib/database/database.dart b/extras/integration_tests/tests/lib/database/database.dart index 5e10265c..a033fd37 100644 --- a/extras/integration_tests/tests/lib/database/database.dart +++ b/extras/integration_tests/tests/lib/database/database.dart @@ -101,8 +101,11 @@ class Database extends _$Database { }, beforeOpen: (details) async { if (details.wasCreated) { - await into(users) - .insertAll([People.dash, People.duke, People.gopher]); + // make sure that transactions can be used in the beforeOpen callback. + await transaction(() { + return into(users) + .insertAll([People.dash, People.duke, People.gopher]); + }); } }, ); diff --git a/moor/lib/src/runtime/database.dart b/moor/lib/src/runtime/database.dart index ff890ee3..9084bfa6 100644 --- a/moor/lib/src/runtime/database.dart +++ b/moor/lib/src/runtime/database.dart @@ -135,7 +135,7 @@ mixin QueryEngine on DatabaseConnectionUser { /// Whether this connection user is "top level", e.g. there is no parent /// connection user. We consider a [GeneratedDatabase] and a /// [DatabaseAccessor] to be top-level, while a [Transaction] or a - /// [BeforeOpenEngine] aren't. + /// [BeforeOpenRunner] aren't. /// /// If any query method is called on a [topLevel] database user, we check if /// it could instead be delegated to a child executor. For instance, consider @@ -478,7 +478,10 @@ abstract class GeneratedDatabase extends DatabaseConnectionUser final migration = _resolvedMigration; if (migration.beforeOpen != null) { - return migration.beforeOpen(details); + return _runEngineZoned( + BeforeOpenRunner(this, executor), + () => migration.beforeOpen(details), + ); } return Future.value(); } @@ -487,10 +490,4 @@ abstract class GeneratedDatabase extends DatabaseConnectionUser Future close() async { await executor.close(); } - - /// Creates another instance of this [GeneratedDatabase] that uses the - /// [connection] instead of the current connection. - GeneratedDatabase cloneWith(DatabaseConnection connection) { - throw UnimplementedError(); - } } diff --git a/moor/lib/src/runtime/executor/helpers/engines.dart b/moor/lib/src/runtime/executor/helpers/engines.dart index a55144cd..341a580c 100644 --- a/moor/lib/src/runtime/executor/helpers/engines.dart +++ b/moor/lib/src/runtime/executor/helpers/engines.dart @@ -13,6 +13,10 @@ mixin _ExecutorWithQueryDelegate on QueryExecutor { bool get logStatements => false; final Lock _lock = Lock(); + /// Used to provide better error messages when calling operations without + /// calling [ensureOpen] before. + bool _ensureOpenCalled = false; + Future _synchronized(FutureOr Function() action) async { if (isSequential) { return await _lock.synchronized(action); @@ -31,6 +35,7 @@ mixin _ExecutorWithQueryDelegate on QueryExecutor { @override Future>> runSelect( String statement, List args) async { + assert(_ensureOpenCalled); final result = await _synchronized(() { _log(statement, args); return impl.runSelect(statement, args); @@ -40,6 +45,7 @@ mixin _ExecutorWithQueryDelegate on QueryExecutor { @override Future runUpdate(String statement, List args) { + assert(_ensureOpenCalled); return _synchronized(() { _log(statement, args); return impl.runUpdate(statement, args); @@ -48,6 +54,7 @@ mixin _ExecutorWithQueryDelegate on QueryExecutor { @override Future runDelete(String statement, List args) { + assert(_ensureOpenCalled); return _synchronized(() { _log(statement, args); return impl.runUpdate(statement, args); @@ -56,6 +63,7 @@ mixin _ExecutorWithQueryDelegate on QueryExecutor { @override Future runInsert(String statement, List args) { + assert(_ensureOpenCalled); return _synchronized(() { _log(statement, args); return impl.runInsert(statement, args); @@ -64,6 +72,7 @@ mixin _ExecutorWithQueryDelegate on QueryExecutor { @override Future runCustom(String statement, [List args]) { + assert(_ensureOpenCalled); return _synchronized(() { final resolvedArgs = args ?? const []; _log(statement, resolvedArgs); @@ -73,6 +82,7 @@ mixin _ExecutorWithQueryDelegate on QueryExecutor { @override Future runBatched(List statements) { + assert(_ensureOpenCalled); return _synchronized(() { if (logStatements) { print('Moor: Executing $statements in a batch'); @@ -113,6 +123,7 @@ class _TransactionExecutor extends TransactionExecutor @override Future ensureOpen() async { + _ensureOpenCalled = true; if (_openingCompleter != null) { return await _openingCompleter.future; } @@ -189,7 +200,7 @@ class _TransactionExecutor extends TransactionExecutor } } -/// A database engine (implements [QueryExecutor]) that delegated the relevant +/// A database engine (implements [QueryExecutor]) that delegates the relevant /// work to a [DatabaseDelegate]. class DelegatedDatabase extends QueryExecutor with _ExecutorWithQueryDelegate { /// The [DatabaseDelegate] to send queries to. @@ -217,12 +228,15 @@ class DelegatedDatabase extends QueryExecutor with _ExecutorWithQueryDelegate { @override Future ensureOpen() { + _ensureOpenCalled = true; return _openingLock.synchronized(() async { final alreadyOpen = await delegate.isOpen; if (alreadyOpen) { return true; } + assert(databaseInfo != null, + 'A databaseInfo needs to be set to use a QueryExeuctor'); await delegate.open(databaseInfo); await _runMigrations(); return true; @@ -274,6 +288,30 @@ class DelegatedDatabase extends QueryExecutor with _ExecutorWithQueryDelegate { } Future _runBeforeOpen(OpeningDetails d) { - return databaseInfo.beforeOpenCallback(this, d); + return databaseInfo.beforeOpenCallback(_BeforeOpeningExecutor(this), d); } } + +/// Inside a `beforeOpen` callback, all moor apis must be available. At the same +/// time, the `beforeOpen` callback must complete before any query sent outside +/// of a `beforeOpen` callback can run. We do this by introducing a special +/// executor that delegates all work to the original executor, but without +/// blocking on `ensureOpen` +class _BeforeOpeningExecutor extends QueryExecutor + with _ExecutorWithQueryDelegate { + final DelegatedDatabase _base; + + _BeforeOpeningExecutor(this._base); + + @override + TransactionExecutor beginTransaction() => _base.beginTransaction(); + + @override + Future ensureOpen() { + _ensureOpenCalled = true; + return Future.value(true); + } + + @override + QueryDelegate get impl => _base.impl; +} diff --git a/moor/lib/src/runtime/executor/transactions.dart b/moor/lib/src/runtime/executor/transactions.dart index c00fe442..1627fd21 100644 --- a/moor/lib/src/runtime/executor/transactions.dart +++ b/moor/lib/src/runtime/executor/transactions.dart @@ -2,6 +2,8 @@ import 'package:moor/moor.dart'; import 'package:moor/src/runtime/executor/stream_queries.dart'; /// Runs multiple statements transactionally. +/// +/// Moor users should use [QueryEngine.transaction] to use this api. class Transaction extends DatabaseConnectionUser with QueryEngine { /// Constructs a transaction executor from the [other] user and the underlying /// [executor]. @@ -46,3 +48,14 @@ class _TransactionStreamStore extends StreamQueryStore { return parent.handleTableUpdates(affectedTables); } } + +/// Special query engine to run the [MigrationStrategy.beforeOpen] callback. +/// +/// To use this api, moor users should use the [MigrationStrategy.beforeOpen] +/// parameter inside the [GeneratedDatabase.migration] getter. +class BeforeOpenRunner extends DatabaseConnectionUser with QueryEngine { + /// Creates a [BeforeOpenRunner] from the [database] and the special + /// [executor] running the queries. + BeforeOpenRunner(DatabaseConnectionUser database, QueryExecutor executor) + : super.delegate(database, executor: executor); +}