From b5d565e16427e5e9c30475c4329a5c0ad34fbce2 Mon Sep 17 00:00:00 2001 From: Simon Binder Date: Tue, 23 May 2023 14:19:56 +0200 Subject: [PATCH 01/17] Add test for #2436 --- drift/test/generated/todos.dart | 2 +- .../migrations_integration_test.dart | 41 +++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/drift/test/generated/todos.dart b/drift/test/generated/todos.dart index 63ef3ad0..301c586d 100644 --- a/drift/test/generated/todos.dart +++ b/drift/test/generated/todos.dart @@ -216,7 +216,7 @@ class TodoDb extends _$TodoDb { DriftDatabaseOptions options = const DriftDatabaseOptions(); @override - int get schemaVersion => 1; + int schemaVersion = 1; } @DriftAccessor( diff --git a/drift/test/integration_tests/migrations_integration_test.dart b/drift/test/integration_tests/migrations_integration_test.dart index 6d74f84e..0610f72e 100644 --- a/drift/test/integration_tests/migrations_integration_test.dart +++ b/drift/test/integration_tests/migrations_integration_test.dart @@ -420,6 +420,47 @@ void main() { expect(db.validateDatabaseSchema(), completes); }); }); + + test('custom schema upgrades', () async { + // I promised this would work in https://github.com/simolus3/drift/discussions/2436, + // so we better make sure this keeps working. + final underlying = sqlite3.openInMemory() + ..execute('pragma user_version = 1;'); + addTearDown(underlying.dispose); + + const maxSchema = 10; + final expectedException = Exception('schema upgrade!'); + + for (var currentSchema = 1; currentSchema < maxSchema; currentSchema++) { + final db = TodoDb(NativeDatabase.opened(underlying)); + db.schemaVersion = maxSchema; + db.migration = MigrationStrategy( + onUpgrade: expectAsync3((m, from, to) async { + // This upgrade callback does one step and then throws. Opening the + // database multiple times should run the individual migrations. + expect(from, currentSchema); + expect(to, maxSchema); + + await db.customStatement('CREATE TABLE t$from (id INTEGER);'); + await db.customStatement('pragma user_version = ${from + 1}'); + + if (from != to - 1) { + // Simulate a failed upgrade + throw expectedException; + } + }), + ); + + if (currentSchema != maxSchema - 1) { + // Opening the database should throw this exception + await expectLater( + db.customSelect('SELECT 1').get(), throwsA(expectedException)); + } else { + // The last migration should work + await expectLater(db.customSelect('SELECT 1').get(), completes); + } + } + }); } class _TestDatabase extends GeneratedDatabase { From 2354cb43cf5539a6bf5f81af196ab0def85c18cb Mon Sep 17 00:00:00 2001 From: Simon Binder Date: Tue, 23 May 2023 14:33:39 +0200 Subject: [PATCH 02/17] Clarify `readsFrom` parameter for custom queries --- drift/lib/src/runtime/api/connection_user.dart | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/drift/lib/src/runtime/api/connection_user.dart b/drift/lib/src/runtime/api/connection_user.dart index d616a348..6acc6332 100644 --- a/drift/lib/src/runtime/api/connection_user.dart +++ b/drift/lib/src/runtime/api/connection_user.dart @@ -343,12 +343,17 @@ abstract class DatabaseConnectionUser { return result; } - /// Creates a custom select statement from the given sql [query]. To run the - /// query once, use [Selectable.get]. For an auto-updating streams, set the - /// set of tables the ready [readsFrom] and use [Selectable.watch]. If you - /// know the query will never emit more than one row, you can also use - /// `getSingle` and `SelectableUtils.watchSingle` which return the item - /// directly without wrapping it into a list. + /// Creates a custom select statement from the given sql [query]. + /// + /// The query can be run once by calling [Selectable.get]. + /// + /// For an auto-updating query stream, the [readsFrom] parameter needs to be + /// set to the tables the SQL statement reads from - drift can't infer it + /// automatically like for other queries constructed with its Dart API. + /// When, [Selectable.watch] can be used to construct an updating stream. + /// + /// For queries that are known to only return a single row, + /// [Selectable.getSingle] and [Selectable.watchSingle] can be used as well. /// /// If you use variables in your query (for instance with "?"), they will be /// bound to the [variables] you specify on this query. From 451bc9c930c8dfa283892ceba38c742c4482ff23 Mon Sep 17 00:00:00 2001 From: Simon Binder Date: Tue, 23 May 2023 15:58:15 +0200 Subject: [PATCH 03/17] Load imports for Dart files too (#2433) --- drift/test/generated/todos.g.dart | 2 +- drift_dev/CHANGELOG.md | 5 +++ drift_dev/lib/src/analysis/driver/cache.dart | 8 +++- drift_dev/lib/src/analysis/driver/driver.dart | 4 ++ drift_dev/lib/src/analysis/driver/state.dart | 9 +++- .../lib/src/analysis/resolver/dart/table.dart | 8 +++- .../lib/src/analysis/resolver/discover.dart | 21 +++++++++- .../analysis/resolver/dart/table_test.dart | 42 +++++++++++++++++++ 8 files changed, 91 insertions(+), 8 deletions(-) diff --git a/drift/test/generated/todos.g.dart b/drift/test/generated/todos.g.dart index 3872c361..096ca4b9 100644 --- a/drift/test/generated/todos.g.dart +++ b/drift/test/generated/todos.g.dart @@ -1846,9 +1846,9 @@ class AllTodosWithCategoryResult extends CustomResultSet { mixin _$SomeDaoMixin on DatabaseAccessor { $UsersTable get users => attachedDatabase.users; - $SharedTodosTable get sharedTodos => attachedDatabase.sharedTodos; $CategoriesTable get categories => attachedDatabase.categories; $TodosTableTable get todosTable => attachedDatabase.todosTable; + $SharedTodosTable get sharedTodos => attachedDatabase.sharedTodos; $TodoWithCategoryViewView get todoWithCategoryView => attachedDatabase.todoWithCategoryView; Selectable todosForUser({required int user}) { diff --git a/drift_dev/CHANGELOG.md b/drift_dev/CHANGELOG.md index a947e4b5..c9116efb 100644 --- a/drift_dev/CHANGELOG.md +++ b/drift_dev/CHANGELOG.md @@ -1,3 +1,8 @@ +## 2.8.3-dev + +- Allow Dart-defined tables to reference imported tables through SQL + [#2433](https://github.com/simolus3/drift/issues/2433). + ## 2.8.2 - Fix generated to write qualified column references for Dart components in diff --git a/drift_dev/lib/src/analysis/driver/cache.dart b/drift_dev/lib/src/analysis/driver/cache.dart index 70781ec5..9d160c26 100644 --- a/drift_dev/lib/src/analysis/driver/cache.dart +++ b/drift_dev/lib/src/analysis/driver/cache.dart @@ -63,8 +63,12 @@ class DriftAnalysisCache { yield found; for (final imported in found.imports ?? const []) { - if (seenUris.add(imported)) { - pending.add(knownFiles[imported]!); + // We might not have a known file for all imports of a Dart file, since + // not all imports are drift-related there. + final knownImport = knownFiles[imported]; + + if (seenUris.add(imported) && knownImport != null) { + pending.add(knownImport); } } } diff --git a/drift_dev/lib/src/analysis/driver/driver.dart b/drift_dev/lib/src/analysis/driver/driver.dart index 5b8058f4..e0ea0c7c 100644 --- a/drift_dev/lib/src/analysis/driver/driver.dart +++ b/drift_dev/lib/src/analysis/driver/driver.dart @@ -184,6 +184,10 @@ class DriftAnalysisDriver { ); } } + } else if (state is DiscoveredDartLibrary) { + for (final import in state.importDependencies) { + await prepareFileForAnalysis(import); + } } } diff --git a/drift_dev/lib/src/analysis/driver/state.dart b/drift_dev/lib/src/analysis/driver/state.dart index 861d8d68..b0cb5ae5 100644 --- a/drift_dev/lib/src/analysis/driver/state.dart +++ b/drift_dev/lib/src/analysis/driver/state.dart @@ -129,10 +129,17 @@ class DriftFileImport { class DiscoveredDartLibrary extends DiscoveredFileState { final LibraryElement library; + @override + final List importDependencies; + @override bool get isValidImport => true; - DiscoveredDartLibrary(this.library, super.locallyDefinedElements); + DiscoveredDartLibrary( + this.library, + super.locallyDefinedElements, + this.importDependencies, + ); } class NotADartLibrary extends DiscoveredFileState { diff --git a/drift_dev/lib/src/analysis/resolver/dart/table.dart b/drift_dev/lib/src/analysis/resolver/dart/table.dart index f01c22f1..4d08b5ac 100644 --- a/drift_dev/lib/src/analysis/resolver/dart/table.dart +++ b/drift_dev/lib/src/analysis/resolver/dart/table.dart @@ -46,6 +46,9 @@ class DartTableResolver extends LocalElementResolver { } } + final tableConstraints = + await _readCustomConstraints(references, columns, element); + final table = DriftTable( discovered.ownId, DriftDeclaration.dartElement(element), @@ -60,7 +63,7 @@ class DartTableResolver extends LocalElementResolver { for (final uniqueKey in uniqueKeys ?? const >[]) UniqueColumns(uniqueKey), ], - overrideTableConstraints: await _readCustomConstraints(columns, element), + overrideTableConstraints: tableConstraints, withoutRowId: await _overrideWithoutRowId(element) ?? false, ); @@ -272,7 +275,7 @@ class DartTableResolver extends LocalElementResolver { return ColumnParser(this).parse(declaration, element); } - Future> _readCustomConstraints( + Future> _readCustomConstraints(Set references, List localColumns, ClassElement element) async { final customConstraints = element.lookUpGetter('customConstraints', element.library); @@ -343,6 +346,7 @@ class DartTableResolver extends LocalElementResolver { (msg) => DriftAnalysisError.inDartAst(element, source, msg)); if (table != null) { + references.add(table); final missingColumns = clause.columnNames .map((e) => e.columnName) .where((e) => !table.columnBySqlName.containsKey(e)); diff --git a/drift_dev/lib/src/analysis/resolver/discover.dart b/drift_dev/lib/src/analysis/resolver/discover.dart index 3819dc6d..c7537ea0 100644 --- a/drift_dev/lib/src/analysis/resolver/discover.dart +++ b/drift_dev/lib/src/analysis/resolver/discover.dart @@ -77,8 +77,11 @@ class DiscoverStep { await finder.find(); _file.errorsDuringDiscovery.addAll(finder.errors); - _file.discovery = - DiscoveredDartLibrary(library, _checkForDuplicates(finder.found)); + _file.discovery = DiscoveredDartLibrary( + library, + _checkForDuplicates(finder.found), + finder.imports, + ); break; case '.drift': case '.moor': @@ -153,6 +156,8 @@ class _FindDartElements extends RecursiveElementVisitor { final DiscoverStep _discoverStep; final LibraryElement _library; + final List imports = []; + final TypeChecker _isTable, _isView, _isTableInfo, _isDatabase, _isDao; final List> _pendingWork = []; @@ -231,6 +236,18 @@ class _FindDartElements extends RecursiveElementVisitor { super.visitClassElement(element); } + @override + void visitLibraryImportElement(LibraryImportElement element) { + final imported = element.importedLibrary; + + if (imported != null && !imported.isInSdk) { + _pendingWork.add(Future(() async { + final uri = await _discoverStep._driver.backend.uriOfDart(imported); + imports.add(uri); + })); + } + } + String _defaultNameForTableOrView(ClassElement definingElement) { return _discoverStep._driver.options.caseFromDartToSql .apply(definingElement.name); diff --git a/drift_dev/test/analysis/resolver/dart/table_test.dart b/drift_dev/test/analysis/resolver/dart/table_test.dart index b81dd61a..974a06d7 100644 --- a/drift_dev/test/analysis/resolver/dart/table_test.dart +++ b/drift_dev/test/analysis/resolver/dart/table_test.dart @@ -364,6 +364,48 @@ class WithConstraints extends Table { ]); }); + test('can resolve references from import', () async { + final backend = TestBackend.inTest({ + 'a|lib/topic.dart': ''' +import 'package:drift/drift.dart'; + +import 'language.dart'; + +class Topics extends Table { + IntColumn get id => integer()(); + TextColumn get langCode => text()(); +} + +''', + 'a|lib/video.dart': ''' +import 'package:drift/drift.dart'; + +import 'topic.dart'; + +class Videos extends Table { + IntColumn get id => integer()(); + + IntColumn get topicId => integer()(); + TextColumn get topicLang => text()(); + + @override + List get customConstraints => [ + 'FOREIGN KEY (topic_id, topic_lang) REFERENCES topics (id, lang_code)', + ]; +} +''', + }); + + final state = await backend.analyze('package:a/video.dart'); + backend.expectNoErrors(); + + final table = state.analyzedElements.single as DriftTable; + expect( + table.references, + contains(isA() + .having((e) => e.schemaName, 'schemaName', 'topics'))); + }); + test('supports autoIncrement on int64 columns', () async { final backend = TestBackend.inTest({ 'a|lib/a.dart': ''' From 054b91b5077bc5143ae11ccd176f0e741855a589 Mon Sep 17 00:00:00 2001 From: Simon Binder Date: Tue, 23 May 2023 16:18:01 +0200 Subject: [PATCH 04/17] Don't warn about unrecognized Dart imports --- drift_dev/lib/src/analysis/driver/driver.dart | 15 +++++++++++---- drift_dev/lib/src/analysis/resolver/discover.dart | 4 ++-- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/drift_dev/lib/src/analysis/driver/driver.dart b/drift_dev/lib/src/analysis/driver/driver.dart index e0ea0c7c..82d5762c 100644 --- a/drift_dev/lib/src/analysis/driver/driver.dart +++ b/drift_dev/lib/src/analysis/driver/driver.dart @@ -155,12 +155,16 @@ class DriftAnalysisDriver { } /// Runs the first step (element discovery) on a file with the given [uri]. - Future prepareFileForAnalysis(Uri uri, - {bool needsDiscovery = true}) async { + Future prepareFileForAnalysis( + Uri uri, { + bool needsDiscovery = true, + bool warnIfFileDoesntExist = true, + }) async { var known = cache.knownFiles[uri] ?? cache.notifyFileChanged(uri); if (known.discovery == null && needsDiscovery) { - await DiscoverStep(this, known).discover(); + await DiscoverStep(this, known) + .discover(warnIfFileDoesntExist: warnIfFileDoesntExist); cache.postFileDiscoveryResults(known); // todo: Mark elements that need to be analyzed again @@ -186,7 +190,10 @@ class DriftAnalysisDriver { } } else if (state is DiscoveredDartLibrary) { for (final import in state.importDependencies) { - await prepareFileForAnalysis(import); + // We might import a generated file that doesn't exist yet, that + // should not be a user-visible error. Users will notice because the + // import is reported as an error by the analyzer either way. + await prepareFileForAnalysis(import, warnIfFileDoesntExist: false); } } } diff --git a/drift_dev/lib/src/analysis/resolver/discover.dart b/drift_dev/lib/src/analysis/resolver/discover.dart index c7537ea0..f03a3c67 100644 --- a/drift_dev/lib/src/analysis/resolver/discover.dart +++ b/drift_dev/lib/src/analysis/resolver/discover.dart @@ -51,7 +51,7 @@ class DiscoverStep { return result; } - Future discover() async { + Future discover({required bool warnIfFileDoesntExist}) async { final extension = _file.extension; _file.discovery = UnknownFile(); @@ -61,7 +61,7 @@ class DiscoverStep { try { library = await _driver.backend.readDart(_file.ownUri); } catch (e) { - if (e is! NotALibraryException) { + if (e is! NotALibraryException && warnIfFileDoesntExist) { // Backends are supposed to throw NotALibraryExceptions if the // library is a part file. For other exceptions, we better report // the error. From 45353a7693c928f06d81b357428585514729c417 Mon Sep 17 00:00:00 2001 From: David Martos Date: Wed, 24 May 2023 23:23:40 +0200 Subject: [PATCH 05/17] Prepared statements cache (#2434) Create a cache of prepared statements to optimize performance. --- drift/lib/native.dart | 68 +++++++--- drift/lib/src/sqlite3/database.dart | 119 +++++++++++++++--- drift/lib/wasm.dart | 23 +++- .../ffi/prepared_statements_cache_test.dart | 29 +++++ extras/benchmarks/bin/benchmarks.dart | 4 + extras/benchmarks/lib/benchmarks.dart | 4 + .../src/moor/cache_prepared_statements.dart | 65 ++++++++++ extras/benchmarks/lib/src/moor/database.dart | 14 ++- 8 files changed, 290 insertions(+), 36 deletions(-) create mode 100644 drift/test/platforms/ffi/prepared_statements_cache_test.dart create mode 100644 extras/benchmarks/lib/src/moor/cache_prepared_statements.dart diff --git a/drift/lib/native.dart b/drift/lib/native.dart index 8908d4d0..861d33fe 100644 --- a/drift/lib/native.dart +++ b/drift/lib/native.dart @@ -48,9 +48,19 @@ class NativeDatabase extends DelegatedDatabase { /// add custom user-defined sql functions or to provide encryption keys in /// SQLCipher implementations. /// {@endtemplate} - factory NativeDatabase(File file, - {bool logStatements = false, DatabaseSetup? setup}) { - return NativeDatabase._(_NativeDelegate(file, setup), logStatements); + factory NativeDatabase( + File file, { + bool logStatements = false, + DatabaseSetup? setup, + bool cachePreparedStatements = true, + }) { + return NativeDatabase._( + _NativeDelegate( + file, + setup, + cachePreparedStatements, + ), + logStatements); } /// Creates a database storing its result in [file]. @@ -102,9 +112,15 @@ class NativeDatabase extends DelegatedDatabase { /// Creates an in-memory database won't persist its changes on disk. /// /// {@macro drift_vm_database_factory} - factory NativeDatabase.memory( - {bool logStatements = false, DatabaseSetup? setup}) { - return NativeDatabase._(_NativeDelegate(null, setup), logStatements); + factory NativeDatabase.memory({ + bool logStatements = false, + DatabaseSetup? setup, + bool cachePreparedStatements = true, + }) { + return NativeDatabase._( + _NativeDelegate(null, setup, cachePreparedStatements), + logStatements, + ); } /// Creates a drift executor for an opened [database] from the `sqlite3` @@ -119,12 +135,20 @@ class NativeDatabase extends DelegatedDatabase { /// internally when running [integration tests for migrations](https://drift.simonbinder.eu/docs/advanced-features/migrations/#verifying-migrations). /// /// {@macro drift_vm_database_factory} - factory NativeDatabase.opened(Database database, - {bool logStatements = false, - DatabaseSetup? setup, - bool closeUnderlyingOnClose = true}) { + factory NativeDatabase.opened( + Database database, { + bool logStatements = false, + DatabaseSetup? setup, + bool closeUnderlyingOnClose = true, + bool cachePreparedStatements = true, + }) { return NativeDatabase._( - _NativeDelegate.opened(database, setup, closeUnderlyingOnClose), + _NativeDelegate.opened( + database, + setup, + closeUnderlyingOnClose, + cachePreparedStatements, + ), logStatements); } @@ -181,12 +205,24 @@ class NativeDatabase extends DelegatedDatabase { class _NativeDelegate extends Sqlite3Delegate { final File? file; - _NativeDelegate(this.file, DatabaseSetup? setup) : super(setup); + _NativeDelegate(this.file, DatabaseSetup? setup, bool cachePreparedStatements) + : super( + setup, + cachePreparedStatements: cachePreparedStatements, + ); _NativeDelegate.opened( - Database db, DatabaseSetup? setup, bool closeUnderlyingWhenClosed) - : file = null, - super.opened(db, setup, closeUnderlyingWhenClosed); + Database db, + DatabaseSetup? setup, + bool closeUnderlyingWhenClosed, + bool cachePreparedStatements, + ) : file = null, + super.opened( + db, + setup, + closeUnderlyingWhenClosed, + cachePreparedStatements: cachePreparedStatements, + ); @override Database openDatabase() { @@ -242,6 +278,8 @@ class _NativeDelegate extends Sqlite3Delegate { @override Future close() async { + await super.close(); + if (closeUnderlyingWhenClosed) { try { tracker.markClosed(database); diff --git a/drift/lib/src/sqlite3/database.dart b/drift/lib/src/sqlite3/database.dart index 863633e7..ca6f0a46 100644 --- a/drift/lib/src/sqlite3/database.dart +++ b/drift/lib/src/sqlite3/database.dart @@ -1,5 +1,6 @@ @internal import 'dart:async'; +import 'dart:collection'; import 'package:meta/meta.dart'; import 'package:sqlite3/common.dart'; @@ -24,6 +25,9 @@ abstract class Sqlite3Delegate final void Function(DB)? _setup; + /// Whether prepared statements should be cached. + final bool cachePreparedStatements; + /// Whether the [database] should be closed when [close] is called on this /// instance. /// @@ -31,13 +35,22 @@ abstract class Sqlite3Delegate /// connections to the same database. final bool closeUnderlyingWhenClosed; + final PreparedStatementsCache _preparedStmtsCache = PreparedStatementsCache(); + /// A delegate that will call [openDatabase] to open the database. - Sqlite3Delegate(this._setup) : closeUnderlyingWhenClosed = true; + Sqlite3Delegate( + this._setup, { + required this.cachePreparedStatements, + }) : closeUnderlyingWhenClosed = true; /// A delegate using an underlying sqlite3 database object that has already /// been opened. Sqlite3Delegate.opened( - this._database, this._setup, this.closeUnderlyingWhenClosed) { + this._database, + this._setup, + this.closeUnderlyingWhenClosed, { + required this.cachePreparedStatements, + }) { _initializeDatabase(); } @@ -68,6 +81,10 @@ abstract class Sqlite3Delegate _database?.dispose(); _database = null; + // We can call clear instead of disposeAll because disposing the + // database will also dispose all prepared statements on it. + _preparedStmtsCache.clear(); + rethrow; } } @@ -85,6 +102,12 @@ abstract class Sqlite3Delegate _hasInitializedDatabase = true; } + @override + @mustCallSuper + Future close() { + return Future(_preparedStmtsCache.disposeAll); + } + /// Synchronously prepares and runs [statements] collected from a batch. @protected void runBatchSync(BatchedStatements statements) { @@ -114,23 +137,32 @@ abstract class Sqlite3Delegate if (args.isEmpty) { database.execute(statement); } else { - final stmt = database.prepare(statement, checkNoTail: true); - try { - stmt.execute(args); - } finally { - stmt.dispose(); - } + final stmt = _getPreparedStatement(statement); + stmt.execute(args); } } @override Future runSelect(String statement, List args) async { - final stmt = database.prepare(statement, checkNoTail: true); - try { - final result = stmt.select(args); - return QueryResult.fromRows(result.toList()); - } finally { - stmt.dispose(); + final stmt = _getPreparedStatement(statement); + final result = stmt.select(args); + return QueryResult.fromRows(result.toList()); + } + + CommonPreparedStatement _getPreparedStatement(String sql) { + if (cachePreparedStatements) { + final cachedStmt = _preparedStmtsCache.use(sql); + if (cachedStmt != null) { + return cachedStmt; + } + + final stmt = database.prepare(sql, checkNoTail: true); + _preparedStmtsCache.addNew(sql, stmt); + + return stmt; + } else { + final stmt = database.prepare(sql, checkNoTail: true); + return stmt; } } } @@ -149,3 +181,62 @@ class _SqliteVersionDelegate extends DynamicVersionDelegate { return Future.value(); } } + +/// A cache of prepared statements to avoid having to parse SQL statements +/// multiple time when they're used frequently. +@internal +class PreparedStatementsCache { + /// The maximum amount of cached statements. + final int maxSize; + + // The linked map returns entries in the order in which they have been + // inserted (with the first insertion being reported first). + // So, we treat it as a LRU cache with `entries.last` being the MRU and + // `entries.last` being the LRU element. + final LinkedHashMap _cachedStatements = + LinkedHashMap(); + + /// Create a cache of prepared statements with a capacity of [maxSize]. + PreparedStatementsCache({this.maxSize = 64}); + + /// Attempts to look up the cached [sql] statement, if it exists. + /// + /// If the statement exists, it is marked as most recently used as well. + CommonPreparedStatement? use(String sql) { + // Remove and add the statement if it was found to move it to the end, + // which marks it as the MRU element. + final foundStatement = _cachedStatements.remove(sql); + + if (foundStatement != null) { + _cachedStatements[sql] = foundStatement; + } + + return foundStatement; + } + + /// Caches a statement that has not been cached yet for subsequent uses. + void addNew(String sql, CommonPreparedStatement statement) { + assert(!_cachedStatements.containsKey(sql)); + + if (_cachedStatements.length == maxSize) { + final lru = _cachedStatements.remove(_cachedStatements.keys.first)!; + lru.dispose(); + } + + _cachedStatements[sql] = statement; + } + + /// Removes all cached statements. + void disposeAll() { + for (final statement in _cachedStatements.values) { + statement.dispose(); + } + + _cachedStatements.clear(); + } + + /// Forgets cached statements without explicitly disposing them. + void clear() { + _cachedStatements.clear(); + } +} diff --git a/drift/lib/wasm.dart b/drift/lib/wasm.dart index 92ee1ff8..870f2a9a 100644 --- a/drift/lib/wasm.dart +++ b/drift/lib/wasm.dart @@ -50,9 +50,12 @@ class WasmDatabase extends DelegatedDatabase { WasmDatabaseSetup? setup, IndexedDbFileSystem? fileSystem, bool logStatements = false, + bool cachePreparedStatements = true, }) { return WasmDatabase._( - _WasmDelegate(sqlite3, path, setup, fileSystem), logStatements); + _WasmDelegate(sqlite3, path, setup, fileSystem, cachePreparedStatements), + logStatements, + ); } /// Creates an in-memory database in the loaded [sqlite3] database. @@ -60,9 +63,12 @@ class WasmDatabase extends DelegatedDatabase { CommmonSqlite3 sqlite3, { WasmDatabaseSetup? setup, bool logStatements = false, + bool cachePreparedStatements = true, }) { return WasmDatabase._( - _WasmDelegate(sqlite3, null, setup, null), logStatements); + _WasmDelegate(sqlite3, null, setup, null, cachePreparedStatements), + logStatements, + ); } } @@ -72,8 +78,15 @@ class _WasmDelegate extends Sqlite3Delegate { final IndexedDbFileSystem? _fileSystem; _WasmDelegate( - this._sqlite3, this._path, WasmDatabaseSetup? setup, this._fileSystem) - : super(setup); + this._sqlite3, + this._path, + WasmDatabaseSetup? setup, + this._fileSystem, + bool cachePreparedStatements, + ) : super( + setup, + cachePreparedStatements: cachePreparedStatements, + ); @override CommonDatabase openDatabase() { @@ -125,6 +138,8 @@ class _WasmDelegate extends Sqlite3Delegate { @override Future close() async { + await super.close(); + if (closeUnderlyingWhenClosed) { database.dispose(); await _flush(); diff --git a/drift/test/platforms/ffi/prepared_statements_cache_test.dart b/drift/test/platforms/ffi/prepared_statements_cache_test.dart new file mode 100644 index 00000000..3be57fd0 --- /dev/null +++ b/drift/test/platforms/ffi/prepared_statements_cache_test.dart @@ -0,0 +1,29 @@ +@TestOn('vm') +import 'package:drift/src/sqlite3/database.dart'; +import 'package:sqlite3/sqlite3.dart'; +import 'package:test/test.dart'; + +import '../../test_utils/database_vm.dart'; + +void main() { + preferLocalSqlite3(); + + test("lru/mru order and remove callback", () { + final cache = PreparedStatementsCache(maxSize: 3); + final database = sqlite3.openInMemory(); + addTearDown(database.dispose); + + expect(cache.use('SELECT 1'), isNull); + cache.addNew('SELECT 1', database.prepare('SELECT 1')); + cache.addNew('SELECT 2', database.prepare('SELECT 2')); + cache.addNew('SELECT 3', database.prepare('SELECT 3')); + + expect(cache.use('SELECT 3'), isNotNull); + expect(cache.use('SELECT 1'), isNotNull); + + // Inserting another statement should remove #2, which is now the LRU + cache.addNew('SELECT 4', database.prepare('SELECT 4')); + expect(cache.use('SELECT 2'), isNull); + expect(cache.use('SELECT 1'), isNotNull); + }); +} diff --git a/extras/benchmarks/bin/benchmarks.dart b/extras/benchmarks/bin/benchmarks.dart index fbc5dc14..b8a25bcb 100644 --- a/extras/benchmarks/bin/benchmarks.dart +++ b/extras/benchmarks/bin/benchmarks.dart @@ -24,4 +24,8 @@ Future main() async { } output.writeAsStringSync(json.encode(tracker.timings)); + + // Make sure the process exits. Otherwise, unclosed resources from the + // benchmarks will keep the process alive. + exit(0); } diff --git a/extras/benchmarks/lib/benchmarks.dart b/extras/benchmarks/lib/benchmarks.dart index 2f5f3e11..f5ef2b93 100644 --- a/extras/benchmarks/lib/benchmarks.dart +++ b/extras/benchmarks/lib/benchmarks.dart @@ -3,6 +3,7 @@ import 'dart:async'; import 'package:benchmark_harness/benchmark_harness.dart' show ScoreEmitter; import 'package:intl/intl.dart'; +import 'src/moor/cache_prepared_statements.dart'; import 'src/moor/key_value_insert.dart'; import 'src/sqlite/bind_string.dart'; import 'src/sqlparser/parse_drift_file.dart'; @@ -22,6 +23,9 @@ List allBenchmarks(ScoreEmitter emitter) { // sql parser ParseDriftFile(emitter), TokenizerBenchmark(emitter), + // prepared statements cache + CachedPreparedStatements(emitter), + NonCachedPreparedStatements(emitter), ]; } diff --git a/extras/benchmarks/lib/src/moor/cache_prepared_statements.dart b/extras/benchmarks/lib/src/moor/cache_prepared_statements.dart new file mode 100644 index 00000000..24edca0d --- /dev/null +++ b/extras/benchmarks/lib/src/moor/cache_prepared_statements.dart @@ -0,0 +1,65 @@ +import 'package:benchmarks/benchmarks.dart'; +import 'package:drift/drift.dart'; +import 'package:uuid/uuid.dart'; + +import 'database.dart'; + +// ignore_for_file: invalid_use_of_visible_for_testing_member +// ignore_for_file: invalid_use_of_protected_member + +const int _numQueries = 100000; + +const Uuid uuid = Uuid(); + +Future _runQueries(Database _db) async { + // a query with a subquery so that the query planner takes some time + const queryToBench = ''' +SELECT * FROM key_values WHERE value IN (SELECT value FROM key_values WHERE value = ?); +'''; + + final fs = []; + + for (var i = 0; i < _numQueries; i++) { + fs.add( + _db.customSelect(queryToBench, variables: [Variable(uuid.v4())]).get(), + ); + } + + await Future.wait(fs); +} + +class CachedPreparedStatements extends AsyncBenchmarkBase { + final _db = Database(cachePreparedStatements: true); + + CachedPreparedStatements(ScoreEmitter emitter) + : super('Running $_numQueries queries (cached prepared statements)', + emitter); + + @override + Future setup() async { + // Empty db so that we mostly bench the prepared statement time + } + + @override + Future run() async { + await _runQueries(_db); + } +} + +class NonCachedPreparedStatements extends AsyncBenchmarkBase { + final _db = Database(cachePreparedStatements: false); + + NonCachedPreparedStatements(ScoreEmitter emitter) + : super('Running $_numQueries queries (non-cached prepared statements)', + emitter); + + @override + Future setup() async { + // Empty db so that we mostly bench the prepared statement time + } + + @override + Future run() async { + await _runQueries(_db); + } +} diff --git a/extras/benchmarks/lib/src/moor/database.dart b/extras/benchmarks/lib/src/moor/database.dart index 33272a7c..e63ae3b4 100644 --- a/extras/benchmarks/lib/src/moor/database.dart +++ b/extras/benchmarks/lib/src/moor/database.dart @@ -17,7 +17,10 @@ class KeyValues extends Table { @DriftDatabase(tables: [KeyValues]) class Database extends _$Database { - Database() : super(_obtainExecutor()); + Database({bool cachePreparedStatements = true}) + : super(_obtainExecutor( + cachePreparedStatements: cachePreparedStatements, + )); @override int get schemaVersion => 1; @@ -25,10 +28,15 @@ class Database extends _$Database { const _uuid = Uuid(); -QueryExecutor _obtainExecutor() { +QueryExecutor _obtainExecutor({ + required bool cachePreparedStatements, +}) { final file = File(p.join(Directory.systemTemp.path, 'drift_benchmarks', _uuid.v4())); file.parent.createSync(); - return NativeDatabase(file); + return NativeDatabase( + file, + cachePreparedStatements: cachePreparedStatements, + ); } From 7810ad9a48f0c17b186959ffb9c2164cce63201f Mon Sep 17 00:00:00 2001 From: Simon Binder Date: Wed, 24 May 2023 23:31:46 +0200 Subject: [PATCH 06/17] Write nulls as constants on postgres --- .../lib/src/runtime/query_builder/expressions/variables.dart | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drift/lib/src/runtime/query_builder/expressions/variables.dart b/drift/lib/src/runtime/query_builder/expressions/variables.dart index 92e03dbd..21a2d9b2 100644 --- a/drift/lib/src/runtime/query_builder/expressions/variables.dart +++ b/drift/lib/src/runtime/query_builder/expressions/variables.dart @@ -65,7 +65,10 @@ class Variable extends Expression { @override void writeInto(GenerationContext context) { - if (!context.supportsVariables) { + if (!context.supportsVariables || + // Workaround for https://github.com/simolus3/drift/issues/2441 + // Binding nulls on postgres is currently untyped which causes issues. + (value == null && context.dialect == SqlDialect.postgres)) { // Write as constant instead. Constant(value).writeInto(context); return; From 1cc5745b4123fd9a2c22a731d8d6c1497d12ee24 Mon Sep 17 00:00:00 2001 From: Simon Binder Date: Thu, 25 May 2023 15:13:39 +0200 Subject: [PATCH 07/17] Only release remote transaction if commit succeeds --- drift/CHANGELOG.md | 4 ++ drift/lib/src/remote/server_impl.dart | 29 ++++++----- .../integration_tests/regress_1589_test.dart | 50 ++++++++++++------- 3 files changed, 53 insertions(+), 30 deletions(-) diff --git a/drift/CHANGELOG.md b/drift/CHANGELOG.md index 0c621336..8f86d6ee 100644 --- a/drift/CHANGELOG.md +++ b/drift/CHANGELOG.md @@ -1,3 +1,7 @@ +## 2.9.0-dev + +- Fix a deadlock after rolling back a transaction in a remote isolate. + ## 2.8.0 - Don't keep databases in an unusable state if the `setup` callback throws an diff --git a/drift/lib/src/remote/server_impl.dart b/drift/lib/src/remote/server_impl.dart index f04a5b96..10af3fd6 100644 --- a/drift/lib/src/remote/server_impl.dart +++ b/drift/lib/src/remote/server_impl.dart @@ -209,19 +209,24 @@ class ServerImplementation implements DriftServer { ); } - try { - switch (action) { - case TransactionControl.commit: - await executor.send(); - break; - case TransactionControl.rollback: + switch (action) { + case TransactionControl.commit: + await executor.send(); + // The transaction should only be released if the commit doesn't throw. + _releaseExecutor(executorId!); + break; + case TransactionControl.rollback: + // Rollbacks shouldn't fail. Other parts of drift assume the transaction + // to be over after a rollback either way, so we always release the + // executor in this case. + try { await executor.rollback(); - break; - default: - assert(false, 'Unknown TransactionControl'); - } - } finally { - _releaseExecutor(executorId!); + } finally { + _releaseExecutor(executorId!); + } + break; + default: + assert(false, 'Unknown TransactionControl'); } } diff --git a/drift/test/integration_tests/regress_1589_test.dart b/drift/test/integration_tests/regress_1589_test.dart index 42dab9c2..6baa30ea 100644 --- a/drift/test/integration_tests/regress_1589_test.dart +++ b/drift/test/integration_tests/regress_1589_test.dart @@ -1,5 +1,7 @@ @TestOn('vm') + import 'package:drift/drift.dart'; +import 'package:drift/isolate.dart'; import 'package:drift/native.dart'; import 'package:test/test.dart'; @@ -8,11 +10,12 @@ import '../test_utils/database_vm.dart'; void main() { preferLocalSqlite3(); - test('a failing commit does not block the whole database', () async { - final db = _Database(NativeDatabase.memory()); - addTearDown(db.close); + group('a failing commit does not block the whole database', () { + Future testWith(QueryExecutor executor) async { + final db = _Database(executor); + addTearDown(db.close); - await db.customStatement(''' + await db.customStatement(''' CREATE TABLE IF NOT EXISTS todo_items ( id INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT, title TEXT NOT NULL, content TEXT NULL, @@ -22,27 +25,38 @@ CREATE TABLE IF NOT EXISTS todo_items ( GENERATED ALWAYS AS (title || ' (' || content || ')') VIRTUAL ); '''); - await db.customStatement(''' + await db.customStatement(''' CREATE TABLE IF NOT EXISTS todo_categories ( id INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT, name TEXT NOT NULL ); '''); - await db.customStatement('PRAGMA foreign_keys = ON;'); + await db.customStatement('PRAGMA foreign_keys = ON;'); - await expectLater( - db.transaction(() async { - // Thanks to the deferrable clause, this statement will only cause a - // failing COMMIT. - await db.customStatement( - 'INSERT INTO todo_items (title, category_id) VALUES (?, ?);', - ['a', 100]); - }), - throwsA(isA()), - ); + await expectLater( + db.transaction(() async { + // Thanks to the deferrable clause, this statement will only cause a + // failing COMMIT. + await db.customStatement( + 'INSERT INTO todo_items (title, category_id) VALUES (?, ?);', + ['a', 100]); + }), + throwsA(anyOf(isA(), isA())), + ); - expect( - db.customSelect('SELECT * FROM todo_items').get(), completion(isEmpty)); + expect(db.customSelect('SELECT * FROM todo_items').get(), + completion(isEmpty)); + } + + test('sync client', () async { + await testWith(NativeDatabase.memory()); + }); + + test('through isolate', () async { + final isolate = await DriftIsolate.spawn(NativeDatabase.memory); + + await testWith(await isolate.connect(singleClientMode: true)); + }); }); } From 3b68386992c0dbdb1256391ac3668c9981831328 Mon Sep 17 00:00:00 2001 From: Simon Binder Date: Thu, 25 May 2023 16:29:01 +0200 Subject: [PATCH 08/17] Internal workaround for #2443 --- .../src/runtime/executor/stream_queries.dart | 2 - .../query_builder/statements/query.dart | 45 ++++++++++++++++--- .../integration_tests/regress_2443_test.dart | 35 +++++++++++++++ 3 files changed, 75 insertions(+), 7 deletions(-) create mode 100644 drift/test/integration_tests/regress_2443_test.dart diff --git a/drift/lib/src/runtime/executor/stream_queries.dart b/drift/lib/src/runtime/executor/stream_queries.dart index 941639e3..b160609c 100644 --- a/drift/lib/src/runtime/executor/stream_queries.dart +++ b/drift/lib/src/runtime/executor/stream_queries.dart @@ -80,8 +80,6 @@ class StreamQueryStore { final StreamController> _tableUpdates = StreamController.broadcast(sync: true); - StreamQueryStore(); - /// Creates a new stream from the select statement. Stream>> registerStream( QueryStreamFetcher fetcher) { diff --git a/drift/lib/src/runtime/query_builder/statements/query.dart b/drift/lib/src/runtime/query_builder/statements/query.dart index ab17b85c..54b2d298 100644 --- a/drift/lib/src/runtime/query_builder/statements/query.dart +++ b/drift/lib/src/runtime/query_builder/statements/query.dart @@ -267,7 +267,7 @@ abstract mixin class Selectable /// Maps this selectable by the [mapper] function. /// /// Like [map] just async. - Selectable asyncMap(Future Function(T) mapper) { + Selectable asyncMap(FutureOr Function(T) mapper) { return _AsyncMappedSelectable(this, mapper); } } @@ -293,7 +293,7 @@ class _MappedSelectable extends Selectable { class _AsyncMappedSelectable extends Selectable { final Selectable _source; - final Future Function(S) _mapper; + final FutureOr Function(S) _mapper; _AsyncMappedSelectable(this._source, this._mapper); @@ -304,11 +304,46 @@ class _AsyncMappedSelectable extends Selectable { @override Stream> watch() { - return _source.watch().asyncMap(_mapResults); + final source = _source.watch(); + + // The easiest thing to do here would be to just + // `source.watch().asyncMap(_mapResults)`. However, since _source is + // typically a broadcast stream, asyncMap also uses a broadcast stream + // controller internally which will not generally call `onListen` multiple + // times for multiple stream subscriptions. + // Drift streams are broadcast streams (since they can be listened too + // multiple times), but also special since each subscription receives the + // current snapshot when it gets added. The `asyncMap` implementation in the + // SDK breaks this because listen events don't get forwarded. + // + // So, this small implementation of asyncMap does the same thing while making + // sure the stream returned by this function behaves like one would expect + // drift streams to behave. + return Stream.multi( + (listener) { + late StreamSubscription> subscription; + + void onData(List original) { + subscription.pause(); + _mapResults(original) + .then(listener.addSync, onError: listener.addErrorSync) + .whenComplete(subscription.resume); + } + + subscription = source.listen( + onData, + onError: listener.addErrorSync, + onDone: listener.closeSync, + cancelOnError: false, // Determined by downstream subscription + ); + }, + isBroadcast: source.isBroadcast, + ); } - Future> _mapResults(List results) async => - [for (final result in results) await _mapper(result)]; + Future> _mapResults(List results) async { + return [for (final result in results) await _mapper(result)]; + } } /// Mixin for a [Query] that operates on a single primary table only. diff --git a/drift/test/integration_tests/regress_2443_test.dart b/drift/test/integration_tests/regress_2443_test.dart new file mode 100644 index 00000000..1e1b11a0 --- /dev/null +++ b/drift/test/integration_tests/regress_2443_test.dart @@ -0,0 +1,35 @@ +import 'package:drift/native.dart'; +import 'package:drift/drift.dart'; +import 'package:test/test.dart'; + +void main() { + driftRuntimeOptions.dontWarnAboutMultipleDatabases = true; + for (final suspendBetweenListeners in [true, false]) { + for (final asyncMap in [true, false]) { + test( + 'suspendBetweenListeners=$suspendBetweenListeners, asyncMap=$asyncMap', + () async { + final db = TestDb(); + final select = db.customSelect('select 1'); + final stream = asyncMap + ? select.asyncMap(Future.value).watch() + : select.map((row) => row).watch(); + + final log = []; + stream.listen(log.add); + if (suspendBetweenListeners) await pumpEventQueue(); + stream.listen(log.add); + await pumpEventQueue(); + expect(log, hasLength(2)); + }); + } + } +} + +class TestDb extends GeneratedDatabase { + TestDb() : super(NativeDatabase.memory()); + @override + final List allTables = const []; + @override + final int schemaVersion = 1; +} From 01771758789070c9deb8401fe72b925a7087dba6 Mon Sep 17 00:00:00 2001 From: Simon Binder Date: Fri, 26 May 2023 20:40:03 +0200 Subject: [PATCH 09/17] Avoid internal uses of `asyncMap` --- .../runtime/query_builder/query_builder.dart | 1 + .../query_builder/statements/query.dart | 37 +-------------- .../statements/select/select.dart | 2 +- .../statements/select/select_with_join.dart | 2 +- drift/lib/src/utils/async_map.dart | 45 +++++++++++++++++++ drift/test/database/streams_test.dart | 9 ++-- 6 files changed, 55 insertions(+), 41 deletions(-) create mode 100644 drift/lib/src/utils/async_map.dart diff --git a/drift/lib/src/runtime/query_builder/query_builder.dart b/drift/lib/src/runtime/query_builder/query_builder.dart index 152644a8..2150aa1a 100644 --- a/drift/lib/src/runtime/query_builder/query_builder.dart +++ b/drift/lib/src/runtime/query_builder/query_builder.dart @@ -15,6 +15,7 @@ import 'package:drift/src/runtime/exceptions.dart'; import 'package:drift/src/runtime/executor/stream_queries.dart'; import 'package:drift/src/runtime/types/converters.dart'; import 'package:drift/src/runtime/types/mapping.dart'; +import 'package:drift/src/utils/async_map.dart'; import 'package:drift/src/utils/single_transformer.dart'; import 'package:meta/meta.dart'; diff --git a/drift/lib/src/runtime/query_builder/statements/query.dart b/drift/lib/src/runtime/query_builder/statements/query.dart index 54b2d298..542c7bc9 100644 --- a/drift/lib/src/runtime/query_builder/statements/query.dart +++ b/drift/lib/src/runtime/query_builder/statements/query.dart @@ -304,41 +304,8 @@ class _AsyncMappedSelectable extends Selectable { @override Stream> watch() { - final source = _source.watch(); - - // The easiest thing to do here would be to just - // `source.watch().asyncMap(_mapResults)`. However, since _source is - // typically a broadcast stream, asyncMap also uses a broadcast stream - // controller internally which will not generally call `onListen` multiple - // times for multiple stream subscriptions. - // Drift streams are broadcast streams (since they can be listened too - // multiple times), but also special since each subscription receives the - // current snapshot when it gets added. The `asyncMap` implementation in the - // SDK breaks this because listen events don't get forwarded. - // - // So, this small implementation of asyncMap does the same thing while making - // sure the stream returned by this function behaves like one would expect - // drift streams to behave. - return Stream.multi( - (listener) { - late StreamSubscription> subscription; - - void onData(List original) { - subscription.pause(); - _mapResults(original) - .then(listener.addSync, onError: listener.addErrorSync) - .whenComplete(subscription.resume); - } - - subscription = source.listen( - onData, - onError: listener.addErrorSync, - onDone: listener.closeSync, - cancelOnError: false, // Determined by downstream subscription - ); - }, - isBroadcast: source.isBroadcast, - ); + return AsyncMapPerSubscription(_source.watch()) + .asyncMapPerSubscription(_mapResults); } Future> _mapResults(List results) async { diff --git a/drift/lib/src/runtime/query_builder/statements/select/select.dart b/drift/lib/src/runtime/query_builder/statements/select/select.dart index ccdbac1e..7c179024 100644 --- a/drift/lib/src/runtime/query_builder/statements/select/select.dart +++ b/drift/lib/src/runtime/query_builder/statements/select/select.dart @@ -73,7 +73,7 @@ class SimpleSelectStatement extends Query key: StreamKey(query.sql, query.boundVariables), ); - return database.createStream(fetcher).asyncMap(_mapResponse); + return database.createStream(fetcher).asyncMapPerSubscription(_mapResponse); } Future>> _getRaw(GenerationContext ctx) { diff --git a/drift/lib/src/runtime/query_builder/statements/select/select_with_join.dart b/drift/lib/src/runtime/query_builder/statements/select/select_with_join.dart index 034239bf..036ec079 100644 --- a/drift/lib/src/runtime/query_builder/statements/select/select_with_join.dart +++ b/drift/lib/src/runtime/query_builder/statements/select/select_with_join.dart @@ -233,7 +233,7 @@ class JoinedSelectStatement return database .createStream(fetcher) - .asyncMap((rows) => _mapResponse(ctx, rows)); + .asyncMapPerSubscription((rows) => _mapResponse(ctx, rows)); } @override diff --git a/drift/lib/src/utils/async_map.dart b/drift/lib/src/utils/async_map.dart new file mode 100644 index 00000000..ad8adcb9 --- /dev/null +++ b/drift/lib/src/utils/async_map.dart @@ -0,0 +1,45 @@ +import 'dart:async'; + +/// Extension to make the drift-specific version of [asyncMap] available. +extension AsyncMapPerSubscription on Stream { + /// A variant of [Stream.asyncMap] that forwards each subscription of the + /// returned stream to the source (`this`). + /// + /// The `asyncMap` implementation from the SDK uses a broadcast controller + /// when given an input stream that [Stream.isBroadcast]. As broadcast + /// controllers only call `onListen` once, these subscriptions aren't + /// forwarded to the original stream. + /// + /// Drift query streams send the current snapshot to each attaching listener, + /// a behavior that is lost when wrapping these streams in a broadcast stream + /// controller. Since we need the behavior of `asyncMap` internally though, we + /// re-implement it in a simple variant that transforms each subscription + /// individually. + Stream asyncMapPerSubscription(Future Function(S) mapper) { + return Stream.multi( + (listener) { + late StreamSubscription subscription; + + void onData(S original) { + subscription.pause(); + mapper(original) + .then(listener.addSync, onError: listener.addErrorSync) + .whenComplete(subscription.resume); + } + + subscription = listen( + onData, + onError: listener.addErrorSync, + onDone: listener.closeSync, + cancelOnError: false, // Determined by downstream subscription + ); + + listener + ..onPause = subscription.pause + ..onResume = subscription.resume + ..onCancel = subscription.cancel; + }, + isBroadcast: isBroadcast, + ); + } +} diff --git a/drift/test/database/streams_test.dart b/drift/test/database/streams_test.dart index 91efdd7a..f82216ec 100644 --- a/drift/test/database/streams_test.dart +++ b/drift/test/database/streams_test.dart @@ -1,5 +1,6 @@ import 'dart:async'; +import 'package:async/async.dart'; import 'package:drift/drift.dart'; import 'package:drift/src/runtime/api/runtime_api.dart'; import 'package:drift/src/runtime/executor/stream_queries.dart'; @@ -177,13 +178,13 @@ void main() { await first.first; // will listen to stream, then cancel await pumpEventQueue(times: 1); // give cancel event time to propagate - final checkEmits = - expectLater(second, emitsInOrder([[], []])); + final listener = StreamQueue(second); + await expectLater(listener, emits(isEmpty)); db.markTablesUpdated({db.users}); - await pumpEventQueue(times: 1); + await expectLater(listener, emits(isEmpty)); - await checkEmits; + await listener.cancel(); }); test('same stream instance can be listened to multiple times', () async { From 9154f60dfd6d9aa977573b9b5b9efa579370c837 Mon Sep 17 00:00:00 2001 From: Simon Binder Date: Tue, 30 May 2023 13:51:47 +0200 Subject: [PATCH 10/17] Improve resolving recursive CTEs --- sqlparser/lib/src/analysis/error.dart | 1 + .../src/analysis/steps/column_resolver.dart | 230 ++++++++++++------ sqlparser/lib/src/ast/clauses/with.dart | 30 +-- sqlparser/lib/src/engine/sql_engine.dart | 22 +- .../test/analysis/column_resolver_test.dart | 22 ++ 5 files changed, 186 insertions(+), 119 deletions(-) diff --git a/sqlparser/lib/src/analysis/error.dart b/sqlparser/lib/src/analysis/error.dart index 9d9ff22c..9f742903 100644 --- a/sqlparser/lib/src/analysis/error.dart +++ b/sqlparser/lib/src/analysis/error.dart @@ -69,6 +69,7 @@ enum AnalysisErrorType { starColumnWithoutTable, compoundColumnCountMismatch, cteColumnCountMismatch, + circularReference, valuesSelectCountMismatch, viewColumnNamesMismatch, rowValueMisuse, diff --git a/sqlparser/lib/src/analysis/steps/column_resolver.dart b/sqlparser/lib/src/analysis/steps/column_resolver.dart index 6ac8df11..62260958 100644 --- a/sqlparser/lib/src/analysis/steps/column_resolver.dart +++ b/sqlparser/lib/src/analysis/steps/column_resolver.dart @@ -11,17 +11,55 @@ class ColumnResolver extends RecursiveVisitor { @override void visitSelectStatement(SelectStatement e, ColumnResolverContext arg) { - // visit children first so that common table expressions are resolved - visitChildren(e, arg); + e.withClause?.accept(this, arg); _resolveSelect(e, arg); + + // We've handled the from clause in _resolveSelect, but we still need to + // visit other children to handle things like subquery expressions. + for (final child in e.childNodes) { + if (child != e.withClause && child != e.from) { + visit(child, arg); + } + } + } + + @override + void visitCreateIndexStatement( + CreateIndexStatement e, ColumnResolverContext arg) { + _resolveTableReference(e.on, arg); + visitExcept(e, e.on, arg); + } + + @override + void visitCreateTriggerStatement( + CreateTriggerStatement e, ColumnResolverContext arg) { + final table = _resolveTableReference(e.onTable, arg); + if (table == null) { + // further analysis is not really possible without knowing the table + super.visitCreateTriggerStatement(e, arg); + return; + } + + final scope = e.statementScope; + + // Add columns of the target table for when and update of clauses + scope.expansionOfStarColumn = table.resolvedColumns; + + if (e.target.introducesNew) { + scope.addAlias(e, table, 'new'); + } + if (e.target.introducesOld) { + scope.addAlias(e, table, 'old'); + } + + visitChildren(e, arg); } @override void visitCompoundSelectStatement( CompoundSelectStatement e, ColumnResolverContext arg) { - // first, visit all children so that the compound parts have their columns - // resolved - visitChildren(e, arg); + e.base.accept(this, arg); + visitList(e.additional, arg); _resolveCompoundSelect(e); } @@ -29,29 +67,75 @@ class ColumnResolver extends RecursiveVisitor { @override void visitValuesSelectStatement( ValuesSelectStatement e, ColumnResolverContext arg) { - // visit children to resolve CTEs - visitChildren(e, arg); - + e.withClause?.accept(this, arg); _resolveValuesSelect(e); + + // Still visit expressions because they could have subqueries that we need + // to handle. + visitList(e.values, arg); } @override void visitCommonTableExpression( CommonTableExpression e, ColumnResolverContext arg) { - visitChildren( - e, - const ColumnResolverContext(referencesUseNameOfReferencedColumn: false), + // If we have a compound select statement as a CTE, resolve the initial + // query first because the whole CTE will have those columns in the end. + // This allows subsequent parts of the compound select to refer to the CTE. + final query = e.as; + final contextForFirstChild = ColumnResolverContext( + referencesUseNameOfReferencedColumn: false, + inDefinitionOfCte: [ + ...arg.inDefinitionOfCte, + e.cteTableName.toLowerCase(), + ], ); - final resolved = e.as.resolvedColumns; - final names = e.columnNames; - if (names != null && resolved != null && names.length != resolved.length) { - context.reportError(AnalysisError( - type: AnalysisErrorType.cteColumnCountMismatch, - message: 'This CTE declares ${names.length} columns, but its select ' - 'statement actually returns ${resolved.length}.', - relevantNode: e, - )); + void applyColumns(BaseSelectStatement source) { + final resolved = source.resolvedColumns!; + final names = e.columnNames; + + if (names == null) { + e.resolvedColumns = resolved; + } else { + if (names.length != resolved.length) { + context.reportError(AnalysisError( + type: AnalysisErrorType.cteColumnCountMismatch, + message: + 'This CTE declares ${names.length} columns, but its select ' + 'statement actually returns ${resolved.length}.', + relevantNode: e.tableNameToken ?? e, + )); + } + + final cteColumns = names + .map((name) => CommonTableExpressionColumn(name)..containingSet = e) + .toList(); + for (var i = 0; i < cteColumns.length; i++) { + if (i < resolved.length) { + final selectColumn = resolved[i]; + cteColumns[i].innerColumn = selectColumn; + } + } + e.resolvedColumns = cteColumns; + } + } + + if (query is CompoundSelectStatement) { + // The first nested select statement determines the columns of this CTE. + query.base.accept(this, contextForFirstChild); + applyColumns(query.base); + + // Subsequent queries can refer to the CTE though. + final contextForOtherChildren = ColumnResolverContext( + referencesUseNameOfReferencedColumn: false, + inDefinitionOfCte: arg.inDefinitionOfCte, + ); + + visitList(query.additional, contextForOtherChildren); + _resolveCompoundSelect(query); + } else { + visitChildren(e, contextForFirstChild); + applyColumns(query); } } @@ -70,10 +154,9 @@ class ColumnResolver extends RecursiveVisitor { } @override - void visitTableReference(TableReference e, void arg) { - if (e.resolved == null) { - _resolveTableReference(e); - } + void visitForeignKeyClause(ForeignKeyClause e, ColumnResolverContext arg) { + _resolveTableReference(e.foreignTable, arg); + visitExcept(e, e.foreignTable, arg); } @override @@ -100,8 +183,9 @@ class ColumnResolver extends RecursiveVisitor { _resolveReturningClause(e, e.table.resultSet, arg); } - ResultSet? _addIfResolved(AstNode node, TableReference ref) { - final table = _resolveTableReference(ref); + ResultSet? _addIfResolved( + AstNode node, TableReference ref, ColumnResolverContext arg) { + final table = _resolveTableReference(ref, arg); if (table != null) { node.statementScope.expansionOfStarColumn = table.resolvedColumns; } @@ -114,7 +198,7 @@ class ColumnResolver extends RecursiveVisitor { // Resolve CTEs first e.withClause?.accept(this, arg); - final into = _addIfResolved(e, e.table); + final into = _addIfResolved(e, e.table, arg); for (final child in e.childNodes) { if (child != e.withClause) visit(child, arg); } @@ -126,7 +210,7 @@ class ColumnResolver extends RecursiveVisitor { // Resolve CTEs first e.withClause?.accept(this, arg); - final from = _addIfResolved(e, e.from); + final from = _addIfResolved(e, e.from, arg); for (final child in e.childNodes) { if (child != e.withClause) visit(child, arg); } @@ -168,31 +252,10 @@ class ColumnResolver extends RecursiveVisitor { stmt.returnedResultSet = CustomResultSet(columns); } - @override - void visitCreateTriggerStatement( - CreateTriggerStatement e, ColumnResolverContext arg) { - final table = _resolveTableReference(e.onTable); - if (table == null) { - // further analysis is not really possible without knowing the table - super.visitCreateTriggerStatement(e, arg); - return; - } - - final scope = e.statementScope; - - // Add columns of the target table for when and update of clauses - scope.expansionOfStarColumn = table.resolvedColumns; - - if (e.target.introducesNew) { - scope.addAlias(e, table, 'new'); - } - if (e.target.introducesOld) { - scope.addAlias(e, table, 'old'); - } - - visitChildren(e, arg); - } - + /// Visits a [queryable] appearing in a `FROM` clause under the state [state]. + /// + /// This also adds columns contributed to the resolved source to + /// [availableColumns], which is later used to expand `*` parameters. void _handle(Queryable queryable, List availableColumns, ColumnResolverContext state) { void addColumns(Iterable columns) { @@ -211,39 +274,32 @@ class ColumnResolver extends RecursiveVisitor { queryable.when( isTable: (table) { - final resolved = _resolveTableReference(table); + final resolved = _resolveTableReference(table, state); if (resolved != null) { - // an error will be logged when resolved is null, so the != null check - // is fine and avoids crashes addColumns(table.resultSet!.resolvedColumns!); } }, isSelect: (select) { // Inside subqueries, references don't take the name of the referenced // column. - final childState = - ColumnResolverContext(referencesUseNameOfReferencedColumn: false); - - // the inner select statement doesn't have access to columns defined in - // the outer statements, which is why we use _resolveSelect instead of - // passing availableColumns down to a recursive call of _handle + final childState = ColumnResolverContext( + referencesUseNameOfReferencedColumn: false, + inDefinitionOfCte: state.inDefinitionOfCte, + ); final stmt = select.statement; - if (stmt is CompoundSelectStatement) { - _resolveCompoundSelect(stmt); - } else if (stmt is SelectStatement) { - _resolveSelect(stmt, childState); - } else if (stmt is ValuesSelectStatement) { - _resolveValuesSelect(stmt); - } else { - throw AssertionError('Unknown type of select statement: $stmt'); - } + visit(stmt, childState); addColumns(stmt.resolvedColumns!); }, - isJoin: (join) { - _handle(join.primary, availableColumns, state); - for (final query in join.joins.map((j) => j.query)) { - _handle(query, availableColumns, state); + isJoin: (joinClause) { + _handle(joinClause.primary, availableColumns, state); + for (final join in joinClause.joins) { + _handle(join.query, availableColumns, state); + + final constraint = join.constraint; + if (constraint is OnConstraint) { + visit(constraint.expression, state); + } } }, isTableFunction: (function) { @@ -458,7 +514,18 @@ class ColumnResolver extends RecursiveVisitor { return span; } - ResultSet? _resolveTableReference(TableReference r) { + ResultSet? _resolveTableReference( + TableReference r, ColumnResolverContext state) { + // Check for circular references + if (state.inDefinitionOfCte.contains(r.tableName.toLowerCase())) { + context.reportError(AnalysisError( + type: AnalysisErrorType.circularReference, + relevantNode: r, + message: 'Circular reference to its own CTE', + )); + return null; + } + final scope = r.scope; // Try resolving to a top-level table in the schema and to a result set that @@ -528,6 +595,13 @@ class ColumnResolverContext { /// column in subqueries or CTEs. final bool referencesUseNameOfReferencedColumn; - const ColumnResolverContext( - {this.referencesUseNameOfReferencedColumn = true}); + /// The common table expressions that are currently being defined. + /// + /// This is used to detect forbidden circular references. + final List inDefinitionOfCte; + + const ColumnResolverContext({ + this.referencesUseNameOfReferencedColumn = true, + this.inDefinitionOfCte = const [], + }); } diff --git a/sqlparser/lib/src/ast/clauses/with.dart b/sqlparser/lib/src/ast/clauses/with.dart index 0dc98dcd..a4a2c334 100644 --- a/sqlparser/lib/src/ast/clauses/with.dart +++ b/sqlparser/lib/src/ast/clauses/with.dart @@ -49,7 +49,8 @@ class CommonTableExpression extends AstNode with ResultSet { Token? asToken; IdentifierToken? tableNameToken; - List? _cachedColumns; + @override + List? resolvedColumns; CommonTableExpression({ required this.cteTableName, @@ -71,33 +72,6 @@ class CommonTableExpression extends AstNode with ResultSet { @override Iterable get childNodes => [as]; - @override - List? get resolvedColumns { - final columnsOfSelect = as.resolvedColumns; - - // we don't override column names, so just return the columns declared by - // the select statement - if (columnNames == null) return columnsOfSelect; - - final cached = _cachedColumns ??= columnNames! - .map((name) => CommonTableExpressionColumn(name)..containingSet = this) - .toList(); - - if (columnsOfSelect != null) { - // bind the CommonTableExpressionColumn to the real underlying column - // returned by the select statement - - for (var i = 0; i < cached.length; i++) { - if (i < columnsOfSelect.length) { - final selectColumn = columnsOfSelect[i]; - cached[i].innerColumn = selectColumn; - } - } - } - - return _cachedColumns; - } - @override bool get visibleToChildren => true; } diff --git a/sqlparser/lib/src/engine/sql_engine.dart b/sqlparser/lib/src/engine/sql_engine.dart index d1bf4c43..62ae4ada 100644 --- a/sqlparser/lib/src/engine/sql_engine.dart +++ b/sqlparser/lib/src/engine/sql_engine.dart @@ -270,22 +270,18 @@ class SqlEngine { final node = context.root; node.scope = context.rootScope; - try { - AstPreparingVisitor(context: context).start(node); + AstPreparingVisitor(context: context).start(node); - node - ..accept(ColumnResolver(context), const ColumnResolverContext()) - ..accept(ReferenceResolver(context), const ReferenceResolvingContext()); + node + ..accept(ColumnResolver(context), const ColumnResolverContext()) + ..accept(ReferenceResolver(context), const ReferenceResolvingContext()); - final session = TypeInferenceSession(context, options); - final resolver = TypeResolver(session); - resolver.run(node); - context.types2 = session.results!; + final session = TypeInferenceSession(context, options); + final resolver = TypeResolver(session); + resolver.run(node); + context.types2 = session.results!; - node.acceptWithoutArg(LintingVisitor(options, context)); - } catch (_) { - rethrow; - } + node.acceptWithoutArg(LintingVisitor(options, context)); } } diff --git a/sqlparser/test/analysis/column_resolver_test.dart b/sqlparser/test/analysis/column_resolver_test.dart index f19f28d6..b357ce58 100644 --- a/sqlparser/test/analysis/column_resolver_test.dart +++ b/sqlparser/test/analysis/column_resolver_test.dart @@ -100,6 +100,21 @@ END; }); }); + test('resolves index', () { + final context = engine.analyze('CREATE INDEX foo ON demo (content)'); + context.expectNoError(); + + final tableReference = + context.root.allDescendants.whereType().first; + final columnReference = context.root.allDescendants + .whereType() + .first + .expression as Reference; + + expect(tableReference.resolved, demoTable); + expect(columnReference.resolvedColumn, isA()); + }); + test("DO UPDATE action in upsert can refer to 'exluded'", () { final context = engine.analyze(''' INSERT INTO demo VALUES (?, ?) @@ -270,4 +285,11 @@ INSERT INTO demo VALUES (?, ?) .root as SelectStatement; expect(cte.resolvedColumns?.map((e) => e.name), ['RoWiD']); }); + + test('reports error for circular reference', () { + final query = engine.analyze('WITH x AS (SELECT * FROM x) SELECT 1;'); + expect(query.errors, [ + analysisErrorWith(lexeme: 'x', type: AnalysisErrorType.circularReference), + ]); + }); } From e2fe4228ad8dee4c5f3ef7c6f01d0c553af13a34 Mon Sep 17 00:00:00 2001 From: Simon Binder Date: Tue, 30 May 2023 13:52:30 +0200 Subject: [PATCH 11/17] Make driver error fatal in tests --- drift/test/test_utils/test_utils.mocks.dart | 4 +++- drift_dev/lib/src/analysis/driver/driver.dart | 6 +++++- drift_dev/lib/src/analysis/results/dart.dart | 5 +++-- drift_dev/test/analysis/test_utils.dart | 2 +- 4 files changed, 12 insertions(+), 5 deletions(-) diff --git a/drift/test/test_utils/test_utils.mocks.dart b/drift/test/test_utils/test_utils.mocks.dart index 0d8db574..e398128b 100644 --- a/drift/test/test_utils/test_utils.mocks.dart +++ b/drift/test/test_utils/test_utils.mocks.dart @@ -1,7 +1,9 @@ -// Mocks generated by Mockito 5.4.0 from annotations +// Mocks generated by Mockito 5.4.1 from annotations // in drift/test/test_utils/test_utils.dart. // Do not manually edit this file. +// @dart=2.19 + // ignore_for_file: no_leading_underscores_for_library_prefixes import 'dart:async' as _i4; diff --git a/drift_dev/lib/src/analysis/driver/driver.dart b/drift_dev/lib/src/analysis/driver/driver.dart index 82d5762c..37062bcd 100644 --- a/drift_dev/lib/src/analysis/driver/driver.dart +++ b/drift_dev/lib/src/analysis/driver/driver.dart @@ -56,6 +56,7 @@ class DriftAnalysisDriver { final DriftBackend backend; final DriftAnalysisCache cache = DriftAnalysisCache(); final DriftOptions options; + final bool _isTesting; late final TypeMapping typeMapping = TypeMapping(this); late final ElementDeserializer deserializer = ElementDeserializer(this); @@ -64,7 +65,8 @@ class DriftAnalysisDriver { KnownDriftTypes? _knownTypes; - DriftAnalysisDriver(this.backend, this.options); + DriftAnalysisDriver(this.backend, this.options, {bool isTesting = false}) + : _isTesting = isTesting; SqlEngine newSqlEngine() { return SqlEngine( @@ -218,6 +220,8 @@ class DriftAnalysisDriver { } catch (e, s) { if (e is! CouldNotResolveElementException) { backend.log.warning('Could not analyze ${discovered.ownId}', e, s); + + if (_isTesting) rethrow; } } } diff --git a/drift_dev/lib/src/analysis/results/dart.dart b/drift_dev/lib/src/analysis/results/dart.dart index 52676a2b..95745177 100644 --- a/drift_dev/lib/src/analysis/results/dart.dart +++ b/drift_dev/lib/src/analysis/results/dart.dart @@ -222,7 +222,8 @@ class DartTopLevelSymbol { factory DartTopLevelSymbol.topLevelElement(Element element, [String? elementName]) { - assert(element.library?.topLevelElements.contains(element) == true); + assert(element.library?.topLevelElements.contains(element) == true, + '${element.name} is not a top-level element'); // We're using this to recover the right import URI when using // `package:build`: @@ -437,7 +438,7 @@ class _AddFromAst extends GeneralizingAstVisitor { _AddFromAst(this._builder, this._excluding); void _addTopLevelReference(Element? element, Token name2) { - if (element == null) { + if (element == null || (element.isSynthetic && element.library == null)) { _builder.addText(name2.lexeme); } else { _builder.addTopLevel( diff --git a/drift_dev/test/analysis/test_utils.dart b/drift_dev/test/analysis/test_utils.dart index fba9607a..34501a2e 100644 --- a/drift_dev/test/analysis/test_utils.dart +++ b/drift_dev/test/analysis/test_utils.dart @@ -45,7 +45,7 @@ class TestBackend extends DriftBackend { for (final entry in sourceContents.entries) AssetId.parse(entry.key).uri.toString(): entry.value, } { - driver = DriftAnalysisDriver(this, options); + driver = DriftAnalysisDriver(this, options, isTesting: true); } factory TestBackend.inTest( From 07fe555753ccc7ea3323696662bde900256f0595 Mon Sep 17 00:00:00 2001 From: Simon Binder Date: Thu, 1 Jun 2023 00:08:41 +0200 Subject: [PATCH 12/17] Report syntax error for WITH in trigger --- sqlparser/CHANGELOG.md | 4 ++++ .../lib/src/analysis/steps/linting_visitor.dart | 13 +++++++++++++ .../test/analysis/errors/syntax_error_test.dart | 11 +++++++++++ 3 files changed, 28 insertions(+) diff --git a/sqlparser/CHANGELOG.md b/sqlparser/CHANGELOG.md index a8823d04..7d46fd59 100644 --- a/sqlparser/CHANGELOG.md +++ b/sqlparser/CHANGELOG.md @@ -1,3 +1,7 @@ +## 0.30.1 + +- Report syntax error for `WITH` clauses in triggers. + ## 0.30.0 - Add `previous` and `next` fields for tokens diff --git a/sqlparser/lib/src/analysis/steps/linting_visitor.dart b/sqlparser/lib/src/analysis/steps/linting_visitor.dart index 1e64bcab..b99ac651 100644 --- a/sqlparser/lib/src/analysis/steps/linting_visitor.dart +++ b/sqlparser/lib/src/analysis/steps/linting_visitor.dart @@ -544,4 +544,17 @@ class LintingVisitor extends RecursiveVisitor { visitChildren(e, arg); } + + @override + void visitWithClause(WithClause e, void arg) { + if (_isInTopLevelTriggerStatement) { + context.reportError(AnalysisError( + type: AnalysisErrorType.synctactic, + relevantNode: e.withToken ?? e, + message: 'WITH clauses cannot appear in triggers.', + )); + } + + visitChildren(e, arg); + } } diff --git a/sqlparser/test/analysis/errors/syntax_error_test.dart b/sqlparser/test/analysis/errors/syntax_error_test.dart index 9ed536c9..e9ba8041 100644 --- a/sqlparser/test/analysis/errors/syntax_error_test.dart +++ b/sqlparser/test/analysis/errors/syntax_error_test.dart @@ -80,6 +80,17 @@ void main() { .expectError('DEFAULT VALUES', type: AnalysisErrorType.synctactic); }); + test('WITH clauses', () { + // https://sqlite.org/lang_with.html#limitations_and_caveats + engine.analyze('WITH x AS (SELECT 1) SELECT 2').expectNoError(); + + engine.analyze(''' +CREATE TRIGGER tgr AFTER INSERT ON demo BEGIN + WITH x AS (SELECT 1) SELECT 2; +END; +''').expectError('WITH', type: AnalysisErrorType.synctactic); + }); + group('aliased source tables', () { test('insert', () { engine.analyze('INSERT INTO demo AS d VALUES (?, ?)').expectNoError(); From ba9360f0636b3b1eaa0e22e774e6789598cea5a9 Mon Sep 17 00:00:00 2001 From: Simon Binder Date: Thu, 1 Jun 2023 11:23:57 +0200 Subject: [PATCH 13/17] Fix dependency conflicts in some examples --- drift_sqflite/pubspec.yaml | 4 ---- examples/encryption/pubspec.yaml | 7 ++----- extras/encryption/pubspec.yaml | 4 ---- .../macos/Flutter/GeneratedPluginRegistrant.swift | 2 -- 4 files changed, 2 insertions(+), 15 deletions(-) diff --git a/drift_sqflite/pubspec.yaml b/drift_sqflite/pubspec.yaml index 76884d1f..96303d4a 100644 --- a/drift_sqflite/pubspec.yaml +++ b/drift_sqflite/pubspec.yaml @@ -27,7 +27,3 @@ dev_dependencies: flutter: assets: - test_asset.db - -dependency_overrides: - # Flutter's test packages don't support the latest analyzer yet. - test_api: ^0.4.16 diff --git a/examples/encryption/pubspec.yaml b/examples/encryption/pubspec.yaml index a51ef9bc..637b63f9 100644 --- a/examples/encryption/pubspec.yaml +++ b/examples/encryption/pubspec.yaml @@ -5,7 +5,8 @@ publish_to: 'none' version: 1.0.0+1 environment: - sdk: ">=2.17.6 <3.0.0" + sdk: ">=2.17.6 <4.0.0" + dependencies: drift: ^2.0.2+1 sqlcipher_flutter_libs: ^0.5.1 @@ -24,7 +25,3 @@ dev_dependencies: flutter: uses-material-design: true - -dependency_overrides: - # Flutter's test packages don't support the latest analyzer yet. - test_api: ^0.4.16 diff --git a/extras/encryption/pubspec.yaml b/extras/encryption/pubspec.yaml index a82a08ea..d9678fc7 100644 --- a/extras/encryption/pubspec.yaml +++ b/extras/encryption/pubspec.yaml @@ -21,7 +21,3 @@ dev_dependencies: sdk: flutter integration_test: sdk: flutter - -dependency_overrides: - # Flutter's test packages don't support the latest analyzer yet. - test_api: ^0.4.16 diff --git a/extras/integration_tests/ffi_on_flutter/macos/Flutter/GeneratedPluginRegistrant.swift b/extras/integration_tests/ffi_on_flutter/macos/Flutter/GeneratedPluginRegistrant.swift index 4856abc9..a76a051a 100644 --- a/extras/integration_tests/ffi_on_flutter/macos/Flutter/GeneratedPluginRegistrant.swift +++ b/extras/integration_tests/ffi_on_flutter/macos/Flutter/GeneratedPluginRegistrant.swift @@ -5,10 +5,8 @@ import FlutterMacOS import Foundation -import sqflite import sqlite3_flutter_libs func RegisterGeneratedPlugins(registry: FlutterPluginRegistry) { - SqflitePlugin.register(with: registry.registrar(forPlugin: "SqflitePlugin")) Sqlite3FlutterLibsPlugin.register(with: registry.registrar(forPlugin: "Sqlite3FlutterLibsPlugin")) } From 232140c083f5b190e9f78115a10f98f65091581a Mon Sep 17 00:00:00 2001 From: Simon Binder Date: Thu, 1 Jun 2023 11:26:57 +0200 Subject: [PATCH 14/17] Prepare minor release --- drift/CHANGELOG.md | 3 ++- drift/pubspec.yaml | 2 +- drift_dev/CHANGELOG.md | 2 +- drift_dev/pubspec.yaml | 2 +- sqlparser/pubspec.yaml | 2 +- 5 files changed, 6 insertions(+), 5 deletions(-) diff --git a/drift/CHANGELOG.md b/drift/CHANGELOG.md index 8f86d6ee..486cdb1d 100644 --- a/drift/CHANGELOG.md +++ b/drift/CHANGELOG.md @@ -1,6 +1,7 @@ -## 2.9.0-dev +## 2.8.1 - Fix a deadlock after rolling back a transaction in a remote isolate. +- Remove unintended log messages when using `connectToDriftWorker`. ## 2.8.0 diff --git a/drift/pubspec.yaml b/drift/pubspec.yaml index a894d487..8a9f63e9 100644 --- a/drift/pubspec.yaml +++ b/drift/pubspec.yaml @@ -1,6 +1,6 @@ name: drift description: Drift is a reactive library to store relational data in Dart and Flutter applications. -version: 2.8.0 +version: 2.8.1 repository: https://github.com/simolus3/drift homepage: https://drift.simonbinder.eu/ issue_tracker: https://github.com/simolus3/drift/issues diff --git a/drift_dev/CHANGELOG.md b/drift_dev/CHANGELOG.md index c9116efb..40a95bbf 100644 --- a/drift_dev/CHANGELOG.md +++ b/drift_dev/CHANGELOG.md @@ -1,4 +1,4 @@ -## 2.8.3-dev +## 2.8.3 - Allow Dart-defined tables to reference imported tables through SQL [#2433](https://github.com/simolus3/drift/issues/2433). diff --git a/drift_dev/pubspec.yaml b/drift_dev/pubspec.yaml index 4a178559..f5c2d59d 100644 --- a/drift_dev/pubspec.yaml +++ b/drift_dev/pubspec.yaml @@ -1,6 +1,6 @@ name: drift_dev description: Dev-dependency for users of drift. Contains the generator and development tools. -version: 2.8.2 +version: 2.8.3 repository: https://github.com/simolus3/drift homepage: https://drift.simonbinder.eu/ issue_tracker: https://github.com/simolus3/drift/issues diff --git a/sqlparser/pubspec.yaml b/sqlparser/pubspec.yaml index 67519852..57aa501f 100644 --- a/sqlparser/pubspec.yaml +++ b/sqlparser/pubspec.yaml @@ -1,6 +1,6 @@ name: sqlparser description: Parses sqlite statements and performs static analysis on them -version: 0.30.0 +version: 0.30.1 homepage: https://github.com/simolus3/drift/tree/develop/sqlparser repository: https://github.com/simolus3/drift #homepage: https://drift.simonbinder.eu/ From ce504f44c21d2b076304bfbaf9fdaf8e1068ab21 Mon Sep 17 00:00:00 2001 From: Simon Binder Date: Fri, 2 Jun 2023 16:49:58 +0200 Subject: [PATCH 15/17] Add changelog entry and test for caching stmts --- drift/CHANGELOG.md | 1 + drift/pubspec.yaml | 2 +- .../ffi/prepared_statements_cache_test.dart | 18 ++++++++++++++++++ 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/drift/CHANGELOG.md b/drift/CHANGELOG.md index 486cdb1d..bf623ffe 100644 --- a/drift/CHANGELOG.md +++ b/drift/CHANGELOG.md @@ -1,5 +1,6 @@ ## 2.8.1 +- Performance improvement: Cache and re-use prepared statements - thanks to [@davidmartos96](https://github.com/davidmartos96/) - Fix a deadlock after rolling back a transaction in a remote isolate. - Remove unintended log messages when using `connectToDriftWorker`. diff --git a/drift/pubspec.yaml b/drift/pubspec.yaml index 8a9f63e9..99544737 100644 --- a/drift/pubspec.yaml +++ b/drift/pubspec.yaml @@ -15,7 +15,7 @@ dependencies: js: ^0.6.3 meta: ^1.3.0 stream_channel: ^2.1.0 - sqlite3: ^1.11.0 + sqlite3: ^1.11.2 dev_dependencies: archive: ^3.3.1 diff --git a/drift/test/platforms/ffi/prepared_statements_cache_test.dart b/drift/test/platforms/ffi/prepared_statements_cache_test.dart index 3be57fd0..a2350791 100644 --- a/drift/test/platforms/ffi/prepared_statements_cache_test.dart +++ b/drift/test/platforms/ffi/prepared_statements_cache_test.dart @@ -1,8 +1,10 @@ @TestOn('vm') +import 'package:drift/native.dart'; import 'package:drift/src/sqlite3/database.dart'; import 'package:sqlite3/sqlite3.dart'; import 'package:test/test.dart'; +import '../../generated/todos.dart'; import '../../test_utils/database_vm.dart'; void main() { @@ -26,4 +28,20 @@ void main() { expect(cache.use('SELECT 2'), isNull); expect(cache.use('SELECT 1'), isNotNull); }); + + test('returns new columns after recompilation', () async { + // https://github.com/simolus3/drift/issues/2454 + final db = TodoDb(NativeDatabase.memory(cachePreparedStatements: true)); + + await db.customStatement('create table t (c1)'); + await db.customInsert('insert into t values (1)'); + + final before = await db.customSelect('select * from t').getSingle(); + expect(before.data, {'c1': 1}); + + await db.customStatement('alter table t add column c2'); + + final after = await db.customSelect('select * from t').getSingle(); + expect(after.data, {'c1': 1, 'c2': null}); + }); } From e481f29138a77d737aa78f0769bd286bf24609ab Mon Sep 17 00:00:00 2001 From: Simon Binder Date: Sat, 3 Jun 2023 23:10:54 +0200 Subject: [PATCH 16/17] Fix resolving table aliases (#2453) --- sqlparser/CHANGELOG.md | 5 ++ .../lib/src/analysis/schema/references.dart | 33 +++++++---- .../src/analysis/steps/column_resolver.dart | 55 ++++++++++++------- .../lib/src/analysis/steps/prepare_ast.dart | 40 +++----------- .../analysis/steps/reference_resolver.dart | 2 +- .../lib/src/analysis/types/join_analysis.dart | 2 +- .../test/analysis/column_resolver_test.dart | 47 ++++++++++++++++ .../analysis/types2/join_analysis_test.dart | 6 +- 8 files changed, 121 insertions(+), 69 deletions(-) diff --git a/sqlparser/CHANGELOG.md b/sqlparser/CHANGELOG.md index 7d46fd59..6fae24c7 100644 --- a/sqlparser/CHANGELOG.md +++ b/sqlparser/CHANGELOG.md @@ -1,3 +1,8 @@ +## 0.30.2 + +- Fix false-positive "unknown table" errors when the same table is used in a + join with and then without an alias. + ## 0.30.1 - Report syntax error for `WITH` clauses in triggers. diff --git a/sqlparser/lib/src/analysis/schema/references.dart b/sqlparser/lib/src/analysis/schema/references.dart index d2515267..7d6e2330 100644 --- a/sqlparser/lib/src/analysis/schema/references.dart +++ b/sqlparser/lib/src/analysis/schema/references.dart @@ -45,7 +45,8 @@ abstract class ReferenceScope { /// This is useful to resolve qualified references (e.g. to resolve `foo.bar` /// the resolver would call [resolveResultSet]("foo") and then look up the /// `bar` column in that result set). - ResultSetAvailableInStatement? resolveResultSet(String name) => null; + ResultSetAvailableInStatement? resolveResultSetForReference(String name) => + null; /// Adds an added result set to this scope. /// @@ -126,8 +127,8 @@ mixin _HasParentScope on ReferenceScope { _parentScopeForLookups.resultSetAvailableToChildScopes; @override - ResultSetAvailableInStatement? resolveResultSet(String name) => - _parentScopeForLookups.resolveResultSet(name); + ResultSetAvailableInStatement? resolveResultSetForReference(String name) => + _parentScopeForLookups.resolveResultSetForReference(name); @override ResultSet? resolveResultSetToAdd(String name) => @@ -219,8 +220,8 @@ class StatementScope extends ReferenceScope with _HasParentScope { } @override - ResultSetAvailableInStatement? resolveResultSet(String name) { - return resultSets[name] ?? parent.resolveResultSet(name); + ResultSetAvailableInStatement? resolveResultSetForReference(String name) { + return resultSets[name] ?? parent.resolveResultSetForReference(name); } @override @@ -279,12 +280,19 @@ class StatementScope extends ReferenceScope with _HasParentScope { } } -/// A special intermediate scope used for subqueries appearing in a `FROM` -/// clause so that the subquery can't see outer columns and tables being added. -class SubqueryInFromScope extends ReferenceScope with _HasParentScope { +/// A special intermediate scope used for nodes that don't see columns and +/// tables added to the statement they're in. +/// +/// An example for this are subqueries appearing in a `FROM` clause, which can't +/// see outer columns and tables of the select statement. +/// +/// Another example is the [InsertStatement.source] of an [InsertStatement], +/// which cannot refer to columns of the table being inserted to of course. +/// Things like `INSERT INTO tbl (col) VALUES (tbl.col)` are not allowed. +class SourceScope extends ReferenceScope with _HasParentScope { final StatementScope enclosingStatement; - SubqueryInFromScope(this.enclosingStatement); + SourceScope(this.enclosingStatement); @override RootScope get rootScope => enclosingStatement.rootScope; @@ -321,8 +329,9 @@ class MiscStatementSubScope extends ReferenceScope with _HasParentScope { RootScope get rootScope => parent.rootScope; @override - ResultSetAvailableInStatement? resolveResultSet(String name) { - return additionalResultSets[name] ?? parent.resolveResultSet(name); + ResultSetAvailableInStatement? resolveResultSetForReference(String name) { + return additionalResultSets[name] ?? + parent.resolveResultSetForReference(name); } @override @@ -348,7 +357,7 @@ class SingleTableReferenceScope extends ReferenceScope { RootScope get rootScope => parent.rootScope; @override - ResultSetAvailableInStatement? resolveResultSet(String name) { + ResultSetAvailableInStatement? resolveResultSetForReference(String name) { if (name == addedTableName) { return addedTable; } else { diff --git a/sqlparser/lib/src/analysis/steps/column_resolver.dart b/sqlparser/lib/src/analysis/steps/column_resolver.dart index 62260958..8eed56c0 100644 --- a/sqlparser/lib/src/analysis/steps/column_resolver.dart +++ b/sqlparser/lib/src/analysis/steps/column_resolver.dart @@ -26,7 +26,7 @@ class ColumnResolver extends RecursiveVisitor { @override void visitCreateIndexStatement( CreateIndexStatement e, ColumnResolverContext arg) { - _resolveTableReference(e.on, arg); + _handle(e.on, [], arg); visitExcept(e, e.on, arg); } @@ -185,12 +185,13 @@ class ColumnResolver extends RecursiveVisitor { ResultSet? _addIfResolved( AstNode node, TableReference ref, ColumnResolverContext arg) { - final table = _resolveTableReference(ref, arg); - if (table != null) { - node.statementScope.expansionOfStarColumn = table.resolvedColumns; - } + final availableColumns = []; + _handle(ref, availableColumns, arg); - return table; + final scope = node.statementScope; + scope.expansionOfStarColumn = availableColumns; + + return ref.resultSet; } @override @@ -198,11 +199,11 @@ class ColumnResolver extends RecursiveVisitor { // Resolve CTEs first e.withClause?.accept(this, arg); - final into = _addIfResolved(e, e.table, arg); + _handle(e.table, [], arg); for (final child in e.childNodes) { if (child != e.withClause) visit(child, arg); } - _resolveReturningClause(e, into, arg); + _resolveReturningClause(e, e.table.resultSet, arg); } @override @@ -272,14 +273,32 @@ class ColumnResolver extends RecursiveVisitor { } } + final scope = queryable.scope; + + void markAvailableResultSet( + Queryable source, ResolvesToResultSet resultSet, String? name) { + final added = ResultSetAvailableInStatement(source, resultSet); + + if (source is TableOrSubquery) { + source.availableResultSet = added; + } + + scope.addResolvedResultSet(name, added); + } + queryable.when( isTable: (table) { final resolved = _resolveTableReference(table, state); + markAvailableResultSet( + table, resolved ?? table, table.as ?? table.tableName); + if (resolved != null) { addColumns(table.resultSet!.resolvedColumns!); } }, isSelect: (select) { + markAvailableResultSet(select, select.statement, select.as); + // Inside subqueries, references don't take the name of the referenced // column. final childState = ColumnResolverContext( @@ -307,6 +326,9 @@ class ColumnResolver extends RecursiveVisitor { .engineOptions.addedTableFunctions[function.name.toLowerCase()]; final resolved = handler?.resolveTableValued(context, function); + markAvailableResultSet( + function, resolved ?? function, function.as ?? function.name); + if (resolved == null) { context.reportError(AnalysisError( type: AnalysisErrorType.unknownFunction, @@ -346,7 +368,8 @@ class ColumnResolver extends RecursiveVisitor { Iterable? visibleColumnsForStar; if (resultColumn.tableName != null) { - final tableResolver = scope.resolveResultSet(resultColumn.tableName!); + final tableResolver = + scope.resolveResultSetForReference(resultColumn.tableName!); if (tableResolver == null) { context.reportError(AnalysisError( type: AnalysisErrorType.referencedUnknownTable, @@ -417,7 +440,8 @@ class ColumnResolver extends RecursiveVisitor { } } } else if (resultColumn is NestedStarResultColumn) { - final target = scope.resolveResultSet(resultColumn.tableName); + final target = + scope.resolveResultSetForReference(resultColumn.tableName); if (target == null) { context.reportError(AnalysisError( @@ -531,18 +555,9 @@ class ColumnResolver extends RecursiveVisitor { // Try resolving to a top-level table in the schema and to a result set that // may have been added to the table final resolvedInSchema = scope.resolveResultSetToAdd(r.tableName); - final resolvedInQuery = scope.resolveResultSet(r.tableName); final createdName = r.as; - // Prefer using a table that has already been added if this isn't the - // definition of the added table reference - if (resolvedInQuery != null && resolvedInQuery.origin != r) { - final resolved = resolvedInQuery.resultSet.resultSet; - if (resolved != null) { - return r.resolved = - createdName != null ? TableAlias(resolved, createdName) : resolved; - } - } else if (resolvedInSchema != null) { + if (resolvedInSchema != null) { return r.resolved = createdName != null ? TableAlias(resolvedInSchema, createdName) : resolvedInSchema; diff --git a/sqlparser/lib/src/analysis/steps/prepare_ast.dart b/sqlparser/lib/src/analysis/steps/prepare_ast.dart index bdda1a0b..4f56235a 100644 --- a/sqlparser/lib/src/analysis/steps/prepare_ast.dart +++ b/sqlparser/lib/src/analysis/steps/prepare_ast.dart @@ -19,6 +19,12 @@ class AstPreparingVisitor extends RecursiveVisitor { resolveIndexOfVariables(_foundVariables); } + @override + void defaultInsertSource(InsertSource e, void arg) { + e.scope = SourceScope(e.parent!.statementScope); + visitChildren(e, arg); + } + @override void visitCreateTableStatement(CreateTableStatement e, void arg) { final scope = e.scope = StatementScope.forStatement(context.rootScope, e); @@ -52,6 +58,7 @@ class AstPreparingVisitor extends RecursiveVisitor { // query: "SELECT * FROM demo d1, // (SELECT * FROM demo i WHERE i.id = d1.id) d2;" // it won't work. + final isInFROM = e.parent is Queryable; StatementScope scope; @@ -59,7 +66,7 @@ class AstPreparingVisitor extends RecursiveVisitor { final surroundingSelect = e.parents .firstWhere((node) => node is HasFrom) .scope as StatementScope; - scope = StatementScope(SubqueryInFromScope(surroundingSelect)); + scope = StatementScope(SourceScope(surroundingSelect)); } else { scope = StatementScope.forStatement(context.rootScope, e); } @@ -107,37 +114,6 @@ class AstPreparingVisitor extends RecursiveVisitor { visitChildren(e, arg); } - @override - void defaultQueryable(Queryable e, void arg) { - final scope = e.scope; - - e.when( - isTable: (table) { - final added = ResultSetAvailableInStatement(table, table); - table.availableResultSet = added; - - scope.addResolvedResultSet(table.as ?? table.tableName, added); - }, - isSelect: (select) { - final added = ResultSetAvailableInStatement(select, select.statement); - select.availableResultSet = added; - scope.addResolvedResultSet(select.as, added); - }, - isJoin: (join) { - // the join can contain multiple tables. Luckily for us, all of them are - // Queryables, so we can deal with them by visiting the children and - // dont't need to do anything here. - }, - isTableFunction: (function) { - final added = ResultSetAvailableInStatement(function, function); - function.availableResultSet = added; - scope.addResolvedResultSet(function.as ?? function.name, added); - }, - ); - - visitChildren(e, arg); - } - @override void visitCommonTableExpression(CommonTableExpression e, void arg) { StatementScope.cast(e.scope).additionalKnownTables[e.cteTableName] = e; diff --git a/sqlparser/lib/src/analysis/steps/reference_resolver.dart b/sqlparser/lib/src/analysis/steps/reference_resolver.dart index e53732d0..9cdba948 100644 --- a/sqlparser/lib/src/analysis/steps/reference_resolver.dart +++ b/sqlparser/lib/src/analysis/steps/reference_resolver.dart @@ -57,7 +57,7 @@ class ReferenceResolver if (e.entityName != null) { // first find the referenced table or view, // then use the column on that table or view. - final entityResolver = scope.resolveResultSet(e.entityName!); + final entityResolver = scope.resolveResultSetForReference(e.entityName!); final resultSet = entityResolver?.resultSet.resultSet; if (resultSet == null) { diff --git a/sqlparser/lib/src/analysis/types/join_analysis.dart b/sqlparser/lib/src/analysis/types/join_analysis.dart index 47d9914a..b7909b89 100644 --- a/sqlparser/lib/src/analysis/types/join_analysis.dart +++ b/sqlparser/lib/src/analysis/types/join_analysis.dart @@ -89,7 +89,7 @@ class JoinModel { } // The boolean arg indicates whether a visited queryable is needed for the -// result to have any rows (which, in particular, mean's its non-nullable) +// result to have any rows (which, in particular, means its non-nullable) class _FindNonNullableJoins extends RecursiveVisitor { final List nonNullable = []; diff --git a/sqlparser/test/analysis/column_resolver_test.dart b/sqlparser/test/analysis/column_resolver_test.dart index b357ce58..38fb511f 100644 --- a/sqlparser/test/analysis/column_resolver_test.dart +++ b/sqlparser/test/analysis/column_resolver_test.dart @@ -125,6 +125,12 @@ INSERT INTO demo VALUES (?, ?) expect(context.errors, isEmpty); }); + test('columns in an insert cannot refer to table', () { + engine + .analyze('INSERT INTO demo (content) VALUES (demo.content)') + .expectError('demo.content'); + }); + test('columns from values statement', () { final context = engine.analyze("VALUES ('foo', 3), ('bar', 5)"); @@ -142,6 +148,33 @@ INSERT INTO demo VALUES (?, ?) expect(context.errors, isEmpty); }); + test('joining table with and without alias', () { + final context = engine.analyze(''' + SELECT * FROM demo a + JOIN demo ON demo.id = a.id + '''); + + context.expectNoError(); + }); + + test("from clause can't use its own table aliases", () { + final context = engine.analyze(''' + SELECT * FROM demo a + JOIN a b ON b.id = a.id + '''); + + expect(context.errors, [ + analysisErrorWith( + lexeme: 'a b', type: AnalysisErrorType.referencedUnknownTable), + analysisErrorWith( + lexeme: 'b.id', type: AnalysisErrorType.referencedUnknownTable), + ]); + }); + + test('can use columns from deleted table', () { + engine.analyze('DELETE FROM demo WHERE demo.id = 2').expectNoError(); + }); + test('gracefully handles tuples of different lengths in VALUES', () { final context = engine.analyze("VALUES ('foo', 3), ('bar')"); @@ -292,4 +325,18 @@ INSERT INTO demo VALUES (?, ?) analysisErrorWith(lexeme: 'x', type: AnalysisErrorType.circularReference), ]); }); + + test('regression test for #2453', () { + // https://github.com/simolus3/drift/issues/2453 + engine + ..registerTableFromSql('CREATE TABLE persons (id INTEGER);') + ..registerTableFromSql('CREATE TABLE cars (driver INTEGER);'); + + final query = engine.analyze(''' +SELECT * FROM cars + JOIN persons second_person ON second_person.id = cars.driver + JOIN persons ON persons.id = cars.driver; +'''); + query.expectNoError(); + }); } diff --git a/sqlparser/test/analysis/types2/join_analysis_test.dart b/sqlparser/test/analysis/types2/join_analysis_test.dart index 87959216..17f2e53d 100644 --- a/sqlparser/test/analysis/types2/join_analysis_test.dart +++ b/sqlparser/test/analysis/types2/join_analysis_test.dart @@ -19,17 +19,17 @@ void main() { final model = JoinModel.of(stmt)!; expect( model.isNullableTable( - stmt.scope.resolveResultSet('a1')!.resultSet.resultSet!), + stmt.scope.resolveResultSetForReference('a1')!.resultSet.resultSet!), isFalse, ); expect( model.isNullableTable( - stmt.scope.resolveResultSet('a2')!.resultSet.resultSet!), + stmt.scope.resolveResultSetForReference('a2')!.resultSet.resultSet!), isTrue, ); expect( model.isNullableTable( - stmt.scope.resolveResultSet('a3')!.resultSet.resultSet!), + stmt.scope.resolveResultSetForReference('a3')!.resultSet.resultSet!), isFalse, ); }); From bbf2a434a322324500367fb74f975147b6fa4ac4 Mon Sep 17 00:00:00 2001 From: Simon Binder Date: Sat, 3 Jun 2023 23:28:50 +0200 Subject: [PATCH 17/17] Mention `computeWithDatabase` limitations more --- drift/lib/isolate.dart | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drift/lib/isolate.dart b/drift/lib/isolate.dart index 2ce1f95d..3be568f1 100644 --- a/drift/lib/isolate.dart +++ b/drift/lib/isolate.dart @@ -313,6 +313,11 @@ extension ComputeWithDriftIsolate on DB { /// [computeWithDatabase] is beneficial when an an expensive work unit needs /// to use the database, or when creating the SQL statements itself is /// expensive. + /// In particular, note that [computeWithDatabase] does not create a second + /// database connection to sqlite3 - the current one is re-used. So if you're + /// using a synchronous database connection, using this method is unlikely to + /// take significant loads off the main isolate. For that reason, the use of + /// `NativeDatabase.createInBackground` is encouraged. @experimental Future computeWithDatabase({ required FutureOr Function(DB) computation,