Skip to content

Prepare schema migration/testing code for upcoming database changes #1248

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Feb 20, 2025
Merged

Prepare schema migration/testing code for upcoming database changes #1248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d7fbff2
db [nfc]: Remove dead code
PIG208 Dec 13, 2024
5ad630e
db [nfc]: Mention build_runner for schema changes
PIG208 Dec 17, 2024
9b2d580
db test: Add missing `after.close` call
PIG208 Dec 18, 2024
d302584
deps [nfc]: Set minimum version of drift to 2.23.0
PIG208 Dec 30, 2024
33da884
db test: Test simple migrations without data programmatically
PIG208 Jan 3, 2025
3cff396
db: Start generating schema versions for migrations
PIG208 Dec 18, 2024
5fb8158
db [nfc]: Use step-by-step migration helper
PIG208 Feb 19, 2025
601936d
db: Drop all tables on downgrade
PIG208 Jan 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 55 additions & 28 deletions lib/model/database.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
import 'dart:io';

import 'package:drift/drift.dart';
import 'package:drift/native.dart';
import 'package:drift/internal/versioned_schema.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';

import '../log.dart';
import 'schema_versions.g.dart';

part 'database.g.dart';

/// The table of [Account] records in the app's database.
Expand Down Expand Up @@ -52,30 +51,61 @@ class UriConverter extends TypeConverter<Uri, String> {
@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);
});
// TODO(drift): generate this
VersionedSchema _getSchema({
required DatabaseConnectionUser database,
required int schemaVersion,
}) {
switch (schemaVersion) {
case 2:
return Schema2(database: database);
default:
Comment on lines +59 to +62
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a mention in the "When updating the schema" comment below.

throw Exception('unknown schema version: $schemaVersion');
}
}

@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:
// * Export the new schema and generate test migrations with drift:
// $ tools/check --fix drift
// and generate database code with build_runner.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific command we can put here, or perhaps refer the reader to some documentation, even if it's just dart run build_runner --help? (The dart run seems helpful to a contributor arriving here for the first time.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't include the command for this as there are different ways to generate them, and some are more efficient the the others. We should point to the relevant piece of documentation ("Generated files" in README.md), or move this section to the README.

// 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<void> _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(
Expand All @@ -84,25 +114,22 @@ 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);

if (from < 2 && 2 <= to) {
await m.addColumn(accounts, 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);
Comment on lines +126 to +129
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change is ultimately useful, but it took me a bit to see why. We should make this point clear in the commit message.

Changing from the explicit if (from < 2 && 2 <= to) form to these driver functions runMigrationSteps and migrationSteps isn't all that useful in itself. It feels to me a lot like the testWithDataIntegrity helper I commented on in a preceding comment. There's very little to the implementation of these functions; and if it were just a matter of giving structure, I think the if (from < 2 && 2 <= to) structure is already pretty clear and is more transparent.

There's one key thing this is doing, though: the schema argument here is the schema for version 2, rather than the latest schema (once we add a version 3 so that those become different).

For a lot of migrations that may not matter. But the point is that if in the future we e.g. alter ackedPushToken, or drop it, then this migration from 1 to 2 should continue to mean what it originally did (so that it produces the right starting point for subsequent migrations, including the one that made that further change to ackedPushToken). And it looks like in Drift the only good scalable way to do that is to record historical schemas in these generated files.

(That isn't the only way to do it in principle — Django takes a different approach, seen in the Zulip server's zerver/migrations/, where each migration records all its details explicitly and independently without reference to the app's models. I might even like that way better, but it doesn't look like it's an approach that Drift currently supports.)

So that's why it's essential that we start generating this schema_versions.g.dart file, and using it in the migrations.

At that point the use of runMigrationSteps and migrationSteps is still pretty optional — the key is that we switch from saying just accounts on this line, referring to the current latest schema, to instead saying Schema2.accounts. Or something equivalent to that, like this schema parameter that gets passed to from1To2.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, and the commit message can be clearer if we move the step-by-step helper change to an NFC commit.

},
));
});
}

Future<int> createAccount(AccountsCompanion values) async {
Expand Down
112 changes: 112 additions & 0 deletions lib/model/schema_versions.g.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

Generating schema_versions.g.dart is crucial because we
otherwise only have access to the latest schema when running migrations.

Migrations that require dropping columns will be problematic
because the latest schema will then have no information about the
dropped column.

s/will be/would be/ — otherwise it reads like a warning about something that's still the case after this commit

Or clarifying another aspect too:

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separately, let's include a link to this comment:
#1248 (comment)

because there's additional discussion there.

Schema2({required super.database}) : super(version: 2);
@override
late final List<i1.DatabaseSchemaEntity> 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<int> get id =>
columnsByName['id']! as i1.GeneratedColumn<int>;
i1.GeneratedColumn<String> get realmUrl =>
columnsByName['realm_url']! as i1.GeneratedColumn<String>;
i1.GeneratedColumn<int> get userId =>
columnsByName['user_id']! as i1.GeneratedColumn<int>;
i1.GeneratedColumn<String> get email =>
columnsByName['email']! as i1.GeneratedColumn<String>;
i1.GeneratedColumn<String> get apiKey =>
columnsByName['api_key']! as i1.GeneratedColumn<String>;
i1.GeneratedColumn<String> get zulipVersion =>
columnsByName['zulip_version']! as i1.GeneratedColumn<String>;
i1.GeneratedColumn<String> get zulipMergeBase =>
columnsByName['zulip_merge_base']! as i1.GeneratedColumn<String>;
i1.GeneratedColumn<int> get zulipFeatureLevel =>
columnsByName['zulip_feature_level']! as i1.GeneratedColumn<int>;
i1.GeneratedColumn<String> get ackedPushToken =>
columnsByName['acked_push_token']! as i1.GeneratedColumn<String>;
}

i1.GeneratedColumn<int> _column_0(String aliasedName) =>
i1.GeneratedColumn<int>('id', aliasedName, false,
hasAutoIncrement: true,
type: i1.DriftSqlType.int,
defaultConstraints:
i1.GeneratedColumn.constraintIsAlways('PRIMARY KEY AUTOINCREMENT'));
i1.GeneratedColumn<String> _column_1(String aliasedName) =>
i1.GeneratedColumn<String>('realm_url', aliasedName, false,
type: i1.DriftSqlType.string);
i1.GeneratedColumn<int> _column_2(String aliasedName) =>
i1.GeneratedColumn<int>('user_id', aliasedName, false,
type: i1.DriftSqlType.int);
i1.GeneratedColumn<String> _column_3(String aliasedName) =>
i1.GeneratedColumn<String>('email', aliasedName, false,
type: i1.DriftSqlType.string);
i1.GeneratedColumn<String> _column_4(String aliasedName) =>
i1.GeneratedColumn<String>('api_key', aliasedName, false,
type: i1.DriftSqlType.string);
i1.GeneratedColumn<String> _column_5(String aliasedName) =>
i1.GeneratedColumn<String>('zulip_version', aliasedName, false,
type: i1.DriftSqlType.string);
i1.GeneratedColumn<String> _column_6(String aliasedName) =>
i1.GeneratedColumn<String>('zulip_merge_base', aliasedName, true,
type: i1.DriftSqlType.string);
i1.GeneratedColumn<int> _column_7(String aliasedName) =>
i1.GeneratedColumn<int>('zulip_feature_level', aliasedName, false,
type: i1.DriftSqlType.int);
i1.GeneratedColumn<String> _column_8(String aliasedName) =>
i1.GeneratedColumn<String>('acked_push_token', aliasedName, true,
type: i1.DriftSqlType.string);
i0.MigrationStepWithVersion migrationSteps({
required Future<void> 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<void> Function(i1.Migrator m, Schema2 schema) from1To2,
}) =>
i0.VersionedSchema.stepByStepHelper(
step: migrationSteps(
from1To2: from1To2,
));
2 changes: 1 addition & 1 deletion pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

Need help checking if podfiles need an update; deps: Upgrade drift to 2.23.0

Confirmed by running tools/upgrade pod at this commit, on a Mac, that no changes to ios/Podfile.lock or macos/Podfile.lock are needed.

Copy link
Member Author

@PIG208 PIG208 Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for confirming!

file_picker: ^8.0.0+1
firebase_core: ^3.3.0
firebase_messaging: ^15.0.1
Expand Down
53 changes: 48 additions & 5 deletions test/model/database_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,11 +98,53 @@ 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();
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<SchemaMismatch>();
// 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', () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit in commit message:

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
```

should use # for comment, not // — the latter isn't YAML comment syntax

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separately in same commit message:

This saves us from writing the simple migration tests between schemas
without data in the future. For the test that upgrade the schema with
data, with the helper, while it ended up having more lines than the
original, the test becomes more structured this way.

The second sentence is explaining why you did do something that this version doesn't do, right? (after #1248 (comment) )

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 {
Expand Down Expand Up @@ -130,6 +172,7 @@ void main() {
...accountV1.toJson(),
'ackedPushToken': null,
});
await after.close();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain a bit more why this change is needed?

db test: Add missing `after.close` call

Signed-off-by: Zixuan James Li <[email protected]>

What goes wrong if this line is left out?

Copy link
Member Author

@PIG208 PIG208 Feb 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It addresses a latent warning that happens when we have multiple tests that use the same AppDatabase class:

WARNING (drift): It looks like you've created the database class DatabaseAtV2 multiple times. When these two databases use the same QueryExecutor, race conditions will occur and might corrupt the database. 
Try to follow the advice at https://drift.simonbinder.eu/faq/#using-the-database or, if you know what you're doing, set driftRuntimeOptions.dontWarnAboutMultipleDatabases = true
Here is the stacktrace from when the database was opened a second time:
#0      GeneratedDatabase._handleInstantiated (package:drift/src/runtime/api/db_base.dart:96:30)
#1      GeneratedDatabase._whenConstructed (package:drift/src/runtime/api/db_base.dart:73:12)

which comes from this method:

  bool _handleInstantiated() {
    if (!_openedDbCount.containsKey(runtimeType) ||
        driftRuntimeOptions.dontWarnAboutMultipleDatabases) {
      _openedDbCount[runtimeType] = 1;
      return true;
    }

    final count =
        _openedDbCount[runtimeType] = _openedDbCount[runtimeType]! + 1;
    if (count > 1) {
      driftRuntimeOptions.debugPrint(
        'WARNING (drift): It looks like you\'ve created the database class '
        '$runtimeType multiple times. When these two databases use the same '
        'QueryExecutor, race conditions will occur and might corrupt the '
        'database. \n'
        'Try to follow the advice at https://drift.simonbinder.eu/faq/#using-the-database '
        'or, if you know what you\'re doing, set '
        'driftRuntimeOptions.dontWarnAboutMultipleDatabases = true\n'
        'Here is the stacktrace from when the database was opened a second '
        'time:\n${StackTrace.current}\n'
        'This warning will only appear on debug builds.',
      );
    }

    return true;
  }

_openedDbCount is a global variable.

It is not an issue at this commit because 1 out of the 2 tests calls AppDatabase.close. While tests don't run concurrently, it is still better to avoid leaking states like this.

With awaitFakeAsync and testWidgets we have some checks/assertions to detect this as soon as the test completes, but I'm not sure if the need to check for this sort of thing justifies having a dedicated test helepr for migrations, unless there are other functionalities to be offered.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks. Yeah, just mention in the commit message that this otherwise leaks state, and would produce a warning if we had another test after this that also created a database instance.

});
});
}
Expand Down
11 changes: 8 additions & 3 deletions tools/check
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The files_check line here needs an update — see that function's doc.

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 \
Expand All @@ -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() {
Expand Down