Skip to content

Commit

Permalink
db: Drop all tables on downgrade
Browse files Browse the repository at this point in the history
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: zulip#1172

Signed-off-by: Zixuan James Li <zixuan@zulip.com>
  • Loading branch information
PIG208 authored and gnprice committed Feb 20, 2025
1 parent 5fb8158 commit 601936d
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 7 deletions.
51 changes: 44 additions & 7 deletions lib/model/database.dart
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -49,6 +51,19 @@ class UriConverter extends TypeConverter<Uri, String> {
@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);
Expand All @@ -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<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 @@ -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);
Expand Down
23 changes: 23 additions & 0 deletions test/model/database_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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<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', () {
const versions = GeneratedHelper.versions;
final latestVersion = versions.last;
Expand Down

0 comments on commit 601936d

Please sign in to comment.