Skip to content

Commit

Permalink
Fix alterTable without legacy alter table
Browse files Browse the repository at this point in the history
Closes #3088
  • Loading branch information
simolus3 committed Nov 30, 2024
1 parent 955f234 commit 3581453
Show file tree
Hide file tree
Showing 3 changed files with 56 additions and 4 deletions.
1 change: 1 addition & 0 deletions drift/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
## 2.22.1

- Fix generated SQL for `insertFromSelect` statements with upserts.
- Fix `alterTable` for databases where `legacy_alter_table` is not writable.

## 2.22.0

Expand Down
31 changes: 27 additions & 4 deletions drift/lib/src/runtime/query_builder/migration.dart
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ class Migrator {
final foreignKeysEnabled =
(await database.customSelect('PRAGMA foreign_keys').getSingle())
.read<bool>('foreign_keys');
final legacyAlterTable =
bool? legacyAlterTable =
(await database.customSelect('PRAGMA legacy_alter_table').getSingle())
.read<bool>('legacy_alter_table');

Expand Down Expand Up @@ -255,16 +255,39 @@ class Migrator {
// we've just dropped the original table), we need to enable the legacy
// option which skips the integrity check.
// See also: https://sqlite.org/forum/forumpost/0e2390093fbb8fd6
if (!legacyAlterTable) {
await _issueCustomQuery('pragma legacy_alter_table = 1;');
if (legacyAlterTable == false) {
try {
await _issueCustomQuery('pragma legacy_alter_table = 1;');
} on Object {
// On some databases like Turso, legacy_alter_table is not writable.
legacyAlterTable = null;

// A workaround is to drop all views and to re-create them later.
// We're not doing this by default to ensure we're not breaking
// existing users (e.g. if the new table references a view somehow).
final allViews = await database.customSelect(
'SELECT name, sql FROM sqlite_master WHERE type = ?;',
variables: [Variable<String>('view')],
).get();

for (final row in allViews) {
final sql = row.read<String>('sql');
if (!createAffected.contains(sql)) {
createAffected.add(sql);
}

final name = row.read<String>('name');
await database.customStatement('DROP VIEW "$name";');
}
}
}

// Step 7: Rename the new table to the old name
await _issueCustomQuery(
'ALTER TABLE ${context.identifier(temporaryName)} '
'RENAME TO ${context.identifier(tableName)}');

if (!legacyAlterTable) {
if (legacyAlterTable == false) {
await _issueCustomQuery('pragma legacy_alter_table = 0;');
}

Expand Down
28 changes: 28 additions & 0 deletions drift/test/integration_tests/migrations_integration_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,19 @@ void main() {

expect(underlying.userVersion, 3);
});

test("alterTable works for databases that can't set legacy alter table",
() async {
final interceptor = _NoLegacyAlterTable();
final db = TodoDb(NativeDatabase.memory().interceptWith(interceptor));
addTearDown(db.close);

final user = await db.users.insertReturning(
UsersCompanion.insert(name: 'test user', profilePicture: Uint8List(0)));
await Migrator(db).alterTable(TableMigration(db.users));
expect(await db.users.all().get(), [user]);
expect(interceptor.didPreventLegacyAlterTable, isTrue);
});
}

class _TestDatabase extends GeneratedDatabase {
Expand All @@ -548,3 +561,18 @@ class _TestDatabase extends GeneratedDatabase {
@override
final MigrationStrategy migration;
}

class _NoLegacyAlterTable extends QueryInterceptor {
var didPreventLegacyAlterTable = false;

@override
Future<void> runCustom(
QueryExecutor executor, String statement, List<Object?> args) {
if (statement.contains('legacy_alter_table') && statement.contains('=')) {
didPreventLegacyAlterTable = true;
throw 'not allowed';
}

return super.runCustom(executor, statement, args);
}
}

0 comments on commit 3581453

Please sign in to comment.