From d7fbff284e19c882fbb7ee777e7998db64c70b61 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Fri, 13 Dec 2024 16:40:54 -0500 Subject: [PATCH 1/8] db [nfc]: Remove dead code Signed-off-by: Zixuan James Li --- lib/model/database.dart | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/lib/model/database.dart b/lib/model/database.dart index 6ca2aa3726..6aa3309c6d 100644 --- a/lib/model/database.dart +++ b/lib/model/database.dart @@ -1,10 +1,5 @@ -import 'dart:io'; - import 'package:drift/drift.dart'; -import 'package:drift/native.dart'; import 'package:drift/remote.dart'; -import 'package:path/path.dart' as path; -import 'package:path_provider/path_provider.dart'; import 'package:sqlite3/common.dart'; part 'database.g.dart'; @@ -52,21 +47,10 @@ class UriConverter extends TypeConverter { @override Uri fromSql(String fromDb) => Uri.parse(fromDb); } -LazyDatabase _openConnection() { - return LazyDatabase(() async { - // TODO decide if this path is the right one to use - final dbFolder = await getApplicationDocumentsDirectory(); - final file = File(path.join(dbFolder.path, 'db.sqlite')); - return NativeDatabase.createInBackground(file); - }); -} - @DriftDatabase(tables: [Accounts]) class AppDatabase extends _$AppDatabase { AppDatabase(super.e); - AppDatabase.live() : this(_openConnection()); - // When updating the schema: // * Make the change in the table classes, and bump schemaVersion. // * Export the new schema and generate test migrations: From 5ad630ee1d5fcb7b94f1c1bdcc74b24bb1da9b8c Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 17 Dec 2024 11:45:01 -0500 Subject: [PATCH 2/8] db [nfc]: Mention build_runner for schema changes Signed-off-by: Zixuan James Li --- lib/model/database.dart | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/model/database.dart b/lib/model/database.dart index 6aa3309c6d..6f941c184b 100644 --- a/lib/model/database.dart +++ b/lib/model/database.dart @@ -53,8 +53,11 @@ class AppDatabase extends _$AppDatabase { // When updating the schema: // * Make the change in the table classes, and bump schemaVersion. - // * Export the new schema and generate test migrations: + // * Export the new schema and generate test migrations with drift: // $ tools/check --fix drift + // and generate database code with build_runner. + // See ../../README.md#generated-files for more + // information on using the build_runner. // * Write a migration in `onUpgrade` below. // * Write tests. @override From 9b2d580fa29eef8d7a46faad6a05dcaf6ffc9c9b Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 17 Dec 2024 19:09:47 -0500 Subject: [PATCH 3/8] db test: Add missing `after.close` call As we add more tests for future migrations, Drift will start complaining about opened databases that are not closed. Always remember doing this will ensure that we don't leak states to other database tests. Signed-off-by: Zixuan James Li --- test/model/database_test.dart | 1 + 1 file changed, 1 insertion(+) diff --git a/test/model/database_test.dart b/test/model/database_test.dart index cb3a7d299b..1cdc06251f 100644 --- a/test/model/database_test.dart +++ b/test/model/database_test.dart @@ -130,6 +130,7 @@ void main() { ...accountV1.toJson(), 'ackedPushToken': null, }); + await after.close(); }); }); } From d302584e53b332d6b503b4d0a9761d4b5c419069 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Sun, 29 Dec 2024 23:21:33 -0500 Subject: [PATCH 4/8] deps [nfc]: Set minimum version of drift to 2.23.0 We have already upgraded drift; this reflects that we rely on newer drift versions, but does not actually require changes to the lock files. Signed-off-by: Zixuan James Li --- pubspec.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pubspec.yaml b/pubspec.yaml index cc1d93cca8..c17bebe704 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -40,7 +40,7 @@ dependencies: convert: ^3.1.1 crypto: ^3.0.3 device_info_plus: ^11.2.0 - drift: ^2.5.0 + drift: ^2.23.0 file_picker: ^8.0.0+1 firebase_core: ^3.3.0 firebase_messaging: ^15.0.1 From 33da884303773fa12b5597433302698f46158df5 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Fri, 3 Jan 2025 11:30:52 +0800 Subject: [PATCH 5/8] db test: Test simple migrations without data programmatically This saves us from writing more simple migration tests between schema versions without data in the future. --- The tests are inspired by the test template generated from `dart run drift_dev make-migrations`. To reproduce the outputs, go through the following steps: Modify `build.yaml` by specifying the location of the database. This step is needed because `make-migrations` does not accept this through command line arguments. ``` targets: $default: builders: # ... drift_dev: options: databases: default: lib/model/database.dart ``` Then, run the following commands: ``` dart run drift_dev make-migrations cp test/model/schemas/*.json drift_schemas/default/ dart run drift_dev make-migrations ``` The first `make-migrations` run generates the initial schema and test files without looking at the versions we have in test/model/schemas. Copying the schema files and running `make-migrations` will end up creating `test/drift/default/migration_test.dart`, along with other generated files. See also: https://drift.simonbinder.eu/Migrations/#usage Signed-off-by: Zixuan James Li --- test/model/database_test.dart | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/test/model/database_test.dart b/test/model/database_test.dart index 1cdc06251f..8b177f9108 100644 --- a/test/model/database_test.dart +++ b/test/model/database_test.dart @@ -98,11 +98,30 @@ void main() { verifier = SchemaVerifier(GeneratedHelper()); }); - test('upgrade to v2, empty', () async { - final connection = await verifier.startAt(1); - final db = AppDatabase(connection); - await verifier.migrateAndValidate(db, 2); - await db.close(); + group('migrate without data', () { + const versions = GeneratedHelper.versions; + final latestVersion = versions.last; + + int fromVersion = versions.first; + for (final toVersion in versions.skip(1)) { + test('from v$fromVersion to v$toVersion', () async { + final connection = await verifier.startAt(fromVersion); + final db = AppDatabase(connection); + await verifier.migrateAndValidate(db, toVersion); + await db.close(); + }); + fromVersion = toVersion; + } + + for (final fromVersion in versions) { + if (fromVersion == latestVersion) break; + test('from v$fromVersion to latest (v$latestVersion)', () async { + final connection = await verifier.startAt(fromVersion); + final db = AppDatabase(connection); + await verifier.migrateAndValidate(db, latestVersion); + await db.close(); + }); + } }); test('upgrade to v2, with data', () async { From 3cff3960b93d174ff6df11246c7a2e89749055e5 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Wed, 18 Dec 2024 15:46:28 -0500 Subject: [PATCH 6/8] db: Start generating schema versions for migrations Generating schema_versions.g.dart is crucial because we otherwise only have access to the latest schema when running migrations. That would mean that if we later drop or alter a table or column mentioned in an existing migration, the meaning of the existing migration would change. The affected existing migration would then migrate to an unintended state that doesn't match what later migrations expect, and it or a later migration might fail. See also discussion: https://github.com/zulip/zulip-flutter/pull/1248#discussion_r1960988853 --- An alternative to using all these commands for generating files is `dart run drift_dev make-migrations`, which is essentially a wrapper for the `schema {dump,generate,steps}` subcommands. `make-migrations` let us manage multiple database schemas by configuring them with `build.yaml`, and it dictates the which subdirectories the generated files will be created at. Because `make-migrations` does not offer the same level of customizations to designate exactly where the output files will be, opting out from it for now. We can revisit this if it starts to offer features that are not available with the subcommands, or that we find the need for managing multiple databases. See also: https://drift.simonbinder.eu/migrations/step_by_step/#manual-generation Signed-off-by: Zixuan James Li --- lib/model/database.dart | 5 +- lib/model/schema_versions.g.dart | 112 +++++++++++++++++++++++++++++++ tools/check | 11 ++- 3 files changed, 124 insertions(+), 4 deletions(-) create mode 100644 lib/model/schema_versions.g.dart diff --git a/lib/model/database.dart b/lib/model/database.dart index 6f941c184b..2c48badc96 100644 --- a/lib/model/database.dart +++ b/lib/model/database.dart @@ -2,6 +2,8 @@ import 'package:drift/drift.dart'; import 'package:drift/remote.dart'; import 'package:sqlite3/common.dart'; +import 'schema_versions.g.dart'; + part 'database.g.dart'; /// The table of [Account] records in the app's database. @@ -85,7 +87,8 @@ class AppDatabase extends _$AppDatabase { assert(1 <= from && from <= to && to <= schemaVersion); if (from < 2 && 2 <= to) { - await m.addColumn(accounts, accounts.ackedPushToken); + final schema = Schema2(database: m.database); + await m.addColumn(schema.accounts, schema.accounts.ackedPushToken); } // New migrations go here. } diff --git a/lib/model/schema_versions.g.dart b/lib/model/schema_versions.g.dart new file mode 100644 index 0000000000..300813c53e --- /dev/null +++ b/lib/model/schema_versions.g.dart @@ -0,0 +1,112 @@ +// dart format width=80 +import 'package:drift/internal/versioned_schema.dart' as i0; +import 'package:drift/drift.dart' as i1; +import 'package:drift/drift.dart'; // ignore_for_file: type=lint,unused_import + +// GENERATED BY drift_dev, DO NOT MODIFY. +final class Schema2 extends i0.VersionedSchema { + Schema2({required super.database}) : super(version: 2); + @override + late final List entities = [ + accounts, + ]; + late final Shape0 accounts = Shape0( + source: i0.VersionedTable( + entityName: 'accounts', + withoutRowId: false, + isStrict: false, + tableConstraints: [ + 'UNIQUE(realm_url, user_id)', + 'UNIQUE(realm_url, email)', + ], + columns: [ + _column_0, + _column_1, + _column_2, + _column_3, + _column_4, + _column_5, + _column_6, + _column_7, + _column_8, + ], + attachedDatabase: database, + ), + alias: null); +} + +class Shape0 extends i0.VersionedTable { + Shape0({required super.source, required super.alias}) : super.aliased(); + i1.GeneratedColumn get id => + columnsByName['id']! as i1.GeneratedColumn; + i1.GeneratedColumn get realmUrl => + columnsByName['realm_url']! as i1.GeneratedColumn; + i1.GeneratedColumn get userId => + columnsByName['user_id']! as i1.GeneratedColumn; + i1.GeneratedColumn get email => + columnsByName['email']! as i1.GeneratedColumn; + i1.GeneratedColumn get apiKey => + columnsByName['api_key']! as i1.GeneratedColumn; + i1.GeneratedColumn get zulipVersion => + columnsByName['zulip_version']! as i1.GeneratedColumn; + i1.GeneratedColumn get zulipMergeBase => + columnsByName['zulip_merge_base']! as i1.GeneratedColumn; + i1.GeneratedColumn get zulipFeatureLevel => + columnsByName['zulip_feature_level']! as i1.GeneratedColumn; + i1.GeneratedColumn get ackedPushToken => + columnsByName['acked_push_token']! as i1.GeneratedColumn; +} + +i1.GeneratedColumn _column_0(String aliasedName) => + i1.GeneratedColumn('id', aliasedName, false, + hasAutoIncrement: true, + type: i1.DriftSqlType.int, + defaultConstraints: + i1.GeneratedColumn.constraintIsAlways('PRIMARY KEY AUTOINCREMENT')); +i1.GeneratedColumn _column_1(String aliasedName) => + i1.GeneratedColumn('realm_url', aliasedName, false, + type: i1.DriftSqlType.string); +i1.GeneratedColumn _column_2(String aliasedName) => + i1.GeneratedColumn('user_id', aliasedName, false, + type: i1.DriftSqlType.int); +i1.GeneratedColumn _column_3(String aliasedName) => + i1.GeneratedColumn('email', aliasedName, false, + type: i1.DriftSqlType.string); +i1.GeneratedColumn _column_4(String aliasedName) => + i1.GeneratedColumn('api_key', aliasedName, false, + type: i1.DriftSqlType.string); +i1.GeneratedColumn _column_5(String aliasedName) => + i1.GeneratedColumn('zulip_version', aliasedName, false, + type: i1.DriftSqlType.string); +i1.GeneratedColumn _column_6(String aliasedName) => + i1.GeneratedColumn('zulip_merge_base', aliasedName, true, + type: i1.DriftSqlType.string); +i1.GeneratedColumn _column_7(String aliasedName) => + i1.GeneratedColumn('zulip_feature_level', aliasedName, false, + type: i1.DriftSqlType.int); +i1.GeneratedColumn _column_8(String aliasedName) => + i1.GeneratedColumn('acked_push_token', aliasedName, true, + type: i1.DriftSqlType.string); +i0.MigrationStepWithVersion migrationSteps({ + required Future Function(i1.Migrator m, Schema2 schema) from1To2, +}) { + return (currentVersion, database) async { + switch (currentVersion) { + case 1: + final schema = Schema2(database: database); + final migrator = i1.Migrator(database, schema); + await from1To2(migrator, schema); + return 2; + default: + throw ArgumentError.value('Unknown migration from $currentVersion'); + } + }; +} + +i1.OnUpgrade stepByStep({ + required Future Function(i1.Migrator m, Schema2 schema) from1To2, +}) => + i0.VersionedSchema.stepByStepHelper( + step: migrationSteps( + from1To2: from1To2, + )); diff --git a/tools/check b/tools/check index fefcd514ed..68b7eb0782 100755 --- a/tools/check +++ b/tools/check @@ -378,13 +378,15 @@ run_l10n() { run_drift() { local schema_dir=test/model/schemas/ + local migration_helper_path=lib/model/schema_versions.g.dart + local outputs=( "${schema_dir}" "${migration_helper_path}" ) # Omitted from this check: # pubspec.{yaml,lock} tools/check - files_check lib/model/database{,.g}.dart "${schema_dir}" \ + files_check lib/model/database{,.g}.dart "${outputs[@]}" \ || return 0 - check_no_uncommitted_or_untracked "${schema_dir}" \ + check_no_uncommitted_or_untracked "${outputs[@]}" \ || return dart run drift_dev schema dump \ @@ -393,8 +395,11 @@ run_drift() { dart run drift_dev schema generate --data-classes --companions \ "${schema_dir}" "${schema_dir}" \ || return + dart run drift_dev schema steps \ + "${schema_dir}" "${migration_helper_path}" \ + || return - check_no_changes "schema updates" "${schema_dir}" + check_no_changes "schema or migration-helper updates" "${outputs[@]}" } filter_flutter_pub_run_output() { From 5fb8158999469674daf5b0980fd3847ca2de6fe8 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Wed, 19 Feb 2025 17:45:28 -0500 Subject: [PATCH 7/8] db [nfc]: Use step-by-step migration helper It is a thin wrapper that does what we are already doing in our migration code. While not required, an advantage of this is that it causes compilation errors for missing migration steps. --- lib/model/database.dart | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/model/database.dart b/lib/model/database.dart index 2c48badc96..b8a0f780fe 100644 --- a/lib/model/database.dart +++ b/lib/model/database.dart @@ -86,13 +86,13 @@ class AppDatabase extends _$AppDatabase { } assert(1 <= from && from <= to && to <= schemaVersion); - if (from < 2 && 2 <= to) { - final schema = Schema2(database: m.database); - await m.addColumn(schema.accounts, schema.accounts.ackedPushToken); - } - // New migrations go here. - } - ); + await m.runMigrationSteps(from: from, to: to, + steps: migrationSteps( + from1To2: (m, schema) async { + await m.addColumn(schema.accounts, schema.accounts.ackedPushToken); + }, + )); + }); } Future createAccount(AccountsCompanion values) async { From 601936da7fd147d040063168187984a7e87dc72b Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Fri, 3 Jan 2025 09:54:19 +0800 Subject: [PATCH 8/8] db: Drop all tables on downgrade We previously missed tables that are not known to the schema. This becomes an issue if a new table is added at a newer schema level. When we go back to an earlier schema, that new table remains in the database, so subsequent attempts to upgrade to the later schema level that adds the table will fail, because it already exists. The test for this relies on some undocumented drift internals to modify the schema version it sees when running the migration. References: https://github.com/simolus3/drift/blob/18cede15/drift/lib/src/runtime/executor/helpers/engines.dart#L459-L495 https://github.com/simolus3/drift/blob/18cede15/drift/lib/src/sqlite3/database.dart#L198-L211 https://github.com/simolus3/sqlite3.dart/blob/4de46afd/sqlite3/lib/src/implementation/database.dart#L69-L85 Fixes: #1172 Signed-off-by: Zixuan James Li --- lib/model/database.dart | 51 ++++++++++++++++++++++++++++++----- test/model/database_test.dart | 23 ++++++++++++++++ 2 files changed, 67 insertions(+), 7 deletions(-) diff --git a/lib/model/database.dart b/lib/model/database.dart index b8a0f780fe..f9000f5cf5 100644 --- a/lib/model/database.dart +++ b/lib/model/database.dart @@ -1,7 +1,9 @@ import 'package:drift/drift.dart'; +import 'package:drift/internal/versioned_schema.dart'; import 'package:drift/remote.dart'; import 'package:sqlite3/common.dart'; +import '../log.dart'; import 'schema_versions.g.dart'; part 'database.g.dart'; @@ -49,6 +51,19 @@ class UriConverter extends TypeConverter { @override Uri fromSql(String fromDb) => Uri.parse(fromDb); } +// TODO(drift): generate this +VersionedSchema _getSchema({ + required DatabaseConnectionUser database, + required int schemaVersion, +}) { + switch (schemaVersion) { + case 2: + return Schema2(database: database); + default: + throw Exception('unknown schema version: $schemaVersion'); + } +} + @DriftDatabase(tables: [Accounts]) class AppDatabase extends _$AppDatabase { AppDatabase(super.e); @@ -60,11 +75,37 @@ class AppDatabase extends _$AppDatabase { // and generate database code with build_runner. // See ../../README.md#generated-files for more // information on using the build_runner. + // * Update [_getSchema] to handle the new schemaVersion. // * Write a migration in `onUpgrade` below. // * Write tests. @override int get schemaVersion => 2; // See note. + Future _dropAndCreateAll(Migrator m, { + required int schemaVersion, + }) async { + await m.database.transaction(() async { + final query = m.database.customSelect( + "SELECT name FROM sqlite_master WHERE type='table'"); + for (final row in await query.get()) { + final data = row.data; + final tableName = data['name'] as String; + // Skip sqlite-internal tables. See for comparison: + // https://www.sqlite.org/fileformat2.html#intschema + // https://github.com/simolus3/drift/blob/0901c984a/drift_dev/lib/src/services/schema/verifier_common.dart#L9-L22 + if (tableName.startsWith('sqlite_')) continue; + // No need to worry about SQL injection; this table name + // was already a table name in the database, not something + // that should be affected by user data. + await m.database.customStatement('DROP TABLE $tableName'); + } + final schema = _getSchema(database: m.database, schemaVersion: schemaVersion); + for (final entity in schema.entities) { + await m.create(entity); + } + }); + } + @override MigrationStrategy get migration { return MigrationStrategy( @@ -73,15 +114,11 @@ class AppDatabase extends _$AppDatabase { }, onUpgrade: (Migrator m, int from, int to) async { if (from > to) { - // TODO(log): log schema downgrade as an error // This should only ever happen in dev. As a dev convenience, // drop everything from the database and start over. - for (final entity in allSchemaEntities) { - // This will miss any entire tables (or indexes, etc.) that - // don't exist at this version. For a dev-only feature, that's OK. - await m.drop(entity); - } - await m.createAll(); + // TODO(log): log schema downgrade as an error + assert(debugLog('Downgrading schema from v$from to v$to.')); + await _dropAndCreateAll(m, schemaVersion: to); return; } assert(1 <= from && from <= to && to <= schemaVersion); diff --git a/test/model/database_test.dart b/test/model/database_test.dart index 8b177f9108..0f8b21297c 100644 --- a/test/model/database_test.dart +++ b/test/model/database_test.dart @@ -98,6 +98,29 @@ void main() { verifier = SchemaVerifier(GeneratedHelper()); }); + test('downgrading', () async { + final schema = await verifier.schemaAt(2); + + // This simulates the scenario during development when running the app + // with a future schema version that has additional tables and columns. + final before = AppDatabase(schema.newConnection()); + await before.customStatement('CREATE TABLE test_extra (num int)'); + await before.customStatement('ALTER TABLE accounts ADD extra_column int'); + await check(verifier.migrateAndValidate( + before, 2, validateDropped: true)).throws(); + // Override the schema version by modifying the underlying value + // drift internally keeps track of in the database. + // TODO(drift): Expose a better interface for testing this. + await before.customStatement('PRAGMA user_version = 999;'); + await before.close(); + + // Simulate starting up the app, with an older schema version that + // does not have the extra tables and columns. + final after = AppDatabase(schema.newConnection()); + await verifier.migrateAndValidate(after, 2, validateDropped: true); + await after.close(); + }); + group('migrate without data', () { const versions = GeneratedHelper.versions; final latestVersion = versions.last;