Moor: Framework support for implementing column alterations

[thetrav]

The official SQLite docs recommend implementing the following process to achieve column alterations that they do not support:
https://www.sqlite.org/lang_altertable.html#otheralter

While I believe I can implement this myself, it would be super handy if Moor had a utility function attached to the migrator that could spare me the need to dig around in the table definition bits and bobs

[kuhnroyal]

This is not easy to do, moor doesn't know how your database looked some schemaVersions ago.
If you remove columns, some old migration code may not even compile anymore if you don't write it as plain strings.
If you remove/alter columns and their types only you know how to select and convert the new data. There is no automatism for this.

The first step towards anything like this and some form of migration testing would need to be something like room's schema json files.
Every time the generator runs it would need to write the whole schema into some file, one for each schema version.
Then we could read those with the migrator and generate create statements at a certain schemaVersion. This would also make migrations testable.

final myTableCopy = migrator.getSchemaForVersion(3).getTable('my_table').copy()
await migrator.createTable(myTableCopy);
await migrator.issueCustomQuery('INSERT INTO my_table_copy (...) SELECT ... FROM my_table')
await migrator.deleteTable('my_table');
await migrator.issueCustomQuery('ALTER TABLE my_table_copy RENAME TO my_table');

This migration will still work and compile even if you remove the my_table descriptor completely from your moor database in schemaVersion 4, whereas migrator.createTable(myTable) would not compile anymore, because it's gone and you would have to rewrite your migration.

TestDatabase.createAtSchemaVersion(3);
// add test data
TestDatabase.runMigration(to: 4);
// verify data

Moor knowing the old schemas would also allow for testing.

But I don't think you get around writing most of the migration code for alterations yourself. Moor helping with creating a copy would be something at least.

[simolus3]

Yeah the current idea of migrations is that you take an old schema and always migrate it to the current version. So if you change a table at 2->3 but remove that table at 3->4, the migration code at version 4 would just remove that table. The column alteration from 2->3 doesn't matter at version 4, so that code can just be dropped.
This is pretty simple for most use cases, but the downsides are that this is hard to test.

If you remove columns, some old migration code may not even compile anymore if you don't write it as plain strings.

Right - that's why the remove functions take a table/column name instead of a table/column instance, which would likely not be available anymore.

If you remove/alter columns and their types only you know how to select and convert the new data. There is no automatism for this.

We could try to do some sql black magic like reading the old CREATE TABLE statement from the sqlite_master table. But that doesn't sound reliable at all. Since column alterations can't be supported without fixed schema files, let's expand the scope of this issue to find a more reliable way to do schema migrations with moor.

I've considered creating schema files that could be used to help with automated migrations. An alternative would be the approach described in #218, but that requires a significant change to your workflow. There are different approaches that come to my mind:

Manual migration procedures

Inspired from SQLDelight's sqm files and room migrators. For each schema migration, users write a file or a Dart function taking care of that migration.

@UseMoor(
  // tables and include describe the current schema
  tables: [...],
  include: {...},
  migrations: [
    Migration.dart(from: 1, to: 2, code: _firstMigration)
    Migration.file(from: 2, to: 3, include: 'migration_2_3.migrate.moor')
  ],
  schemaVersion: 3, // the schema version must be known at compile time
)
class Foo extends _$Foo {...}

Future<void> _firstMigration(Migrator m) async {
  await m.createTable(...);
}
````

```sql
-- migration_2_3.migrate.moor

ALTER TABLE foo ()

This would be the least work to implement in the generator, and it fixes the problem of poor testability. However, automated routines for complex alterations are stil hard to implement. But then on the other hand it looks like Room doesn't provide utilities for that either. Since the current version and all available migrations are constants, we can verify that a migration path exists for all old versions at compile time. We can also provide some test utils that applies schema migrations in series.

Generate schema dumps for tests

This would be very similar to what moor is doing at the moment - dump the schema as json that won't be shipped with the app. It's only used for testing, and only to verify that applying the migrations yields the correct schema. We have an in-memory representation of all tables in the generator (it's needed to generate code), so I imagine that this isn't all that complicated.

Generate old schema representations in Dart

Instead of just dumping schema as json, we create Dart classes that represent the schema at an old version. Ideally, this would allow apis like:
```dart
Future _migration1To2(Migrator m, Schema old) async {
final oldFooTable = old.table('foo');
final actions = [
old.dropColumn(oldFooTable.column('bar')),
old.addColumnConstraint(oldFooTable.column('baz'), 'UNIQUE'),
];
await m.applyActions(actions);
}
````

While I think that the last option is super awesome, it also requires a ton of work, both in the generator and in the runtime. Maybe we can implement this gradually, by first making the migration flow easier to test, then dumping json structures to better verify tests, and finally generating Dart code to represent a schema at a specific time.

[kuhnroyal]

I agree this is nothing that can be done in one go. My biggest pain is testability and rewriting the full table syntax for a table copy, but this seems different for @thetrav - I don't want to hijack this issue.

[Mike278]

FWIW, the approach in #218 is working well for us so far.

We have an add_migration.sh script that generates a migration skeleton and a migration test skeleton that look something like:

part of '../migration_handler.dart';
class _1573657230 implements Migration {

  @override
  int get migrationId => 1573657230;

  @override
  Future<void> execute(ProxyDb db) async {
    // Migration code goes here
  }
}

part of '../migration_test_list.dart';
class _1573657230 implements MigrationTest {

  @override
  int get migrationId => 1573657230;

  @override
  Future<void> prepareSnapshot(ProxyDb proxyDb) async {
    // Opportunity to execute any sql queries on the snapshot before the migration is run.
    // We use a ProxyDb since MyAppDb's generated code will not line up with the snapshot's database schema.
  }

  @override
  Future<void> verifyMigration(MyAppDb db) async {
    // Verify that the migration was successful
  }
}

Our CI saves a snapshot of the database schema as an sqlite file whenever the app is deployed:

/// Takes a snapshot of the current database schema and saves it to a file.
Future<void> main(List<String> args) async {
  String path = args.length == 1 ? args.single : '${secondsSinceEpoch()}.sqlite3';
  final file = File(path);
  if (file.existsSync()) file.deleteSync();
  final proxyDb = ProxyDb(VmDatabase(file));
  final migrationHandler = MigrationHandler(proxyDb);
  await migrationHandler.runMigrations();
  final List<Migration> pending = await migrationHandler.getPendingMigrations();
  if (pending.isNotEmpty) throw StateError('The following migrations failed to run: $pending');
  await proxyDb.close();
}

and we have a test that opens each schema snapshot and runs each pending MigrationTest:

  test("schema migrations", () async {
    List<File> snapshots = getSchemaSnapshots();
    for (final snapshotFile in snapshots) {
      // We don't want to run migration tests for migrations that have already
      // been applied to this snapshot, since the test's `prepareSnapshot()` would be
      // running on a post-migrated database.
      List<int> pendingMigrationIds = await getPendingMigrationIds(snapshotFile);
      for (MigrationTest test in migrationTests) {
        if (pendingMigrationIds.contains(test.migrationId)) {
          await runMigrationTest(snapshotFile, test);
        }
      }
    }
  });

[simolus3]

I just published moor_generator 2.3, which contains a first step towards this. The schema of a @UseMoor-class can be exported by running

pub run moor_generator schema dump lib/src/path/to/databases.dart schema.json

The first file should contain exactly one class annotated with @UseMoor - its schema will then be stored in schema.json. This is still experimental and some parts of the format might change in the future, but I hope that this approach can be used to eventually find schema differences and auto-generate migration code.

[rodydavis]

We can store this scheme.json in a version control as a top level file too. Then each time the build process runs we can check that file and diff the fields.

[kuhnroyal]

Hey @simolus3, I have thought about this for a while now and I am now getting to a point with 2 apps where I need some support for migration tests.

I compared the solutions from sqldelight and room and I think sqldelights approach could work pretty well here.
This is my idea:

1. Make schemaVersion statically known in @UseMoor
2. Add a generator switch that writes a versioned schema file every time the generator runs, this file needs to be committed to VCS
3. Add a helper that can create a database from such a schema version file
4. Add a schema crawler that uses sqlparser to parse all tables/indexes/trigger from sqlite_master
5. Add a way to compare two AST trees ignoring column order
6. Add some test tooling around it

To write a migration test:

1. Create a database from a schema version file
2. Get the schema AST tree of the source database version
3. Use the generated database to upgrade to the target version
4. Get the schema AST tree of the target database version
5. Use test tooling to compare the trees and pretty print the diffs

What do you think about this approach?

[simolus3]

Thanks for the ideas. This sounds like a good idea to make schema migrations testable at least. We can later build on that to also make them automated for simple scenarios.

Add a generator switch that writes a versioned schema file every time the generator runs

This probably won't work with build_runner since we need to have statically known filenames, so their name can't depend on the content of the input file. Adding a command for this to moor's CLI utils should work though.

schema crawler that uses sqlparser to parse all tables/indexes/trigger from sqlite_master

This sounds like a really cool idea!

Moor already has standalone tools to extract the schema from a database class. I'll work on utils to instantiate that schema at runtime and to create a diff. Let's say that we had an api like this:

abstract class SchemaVerifier {
  static Future<SchemaVerifier> fromDir(Directory directory) {...}

  // Creates a database with the selected schema instantiated
  Future<DatabaseConnection> startAt(int version);

  // Opens the database, then compares runtime schema to what's expected
  Future<void> verifySchema(GeneratedDatabase db, int expectedVersion);
}

Would that work for you? You could use it like:

// Assuming that the migrations/ contains generated schema dumps
final verifier = await SchemaVerifier.fromDir(Directory('migrations'));

final db = YourDatabase.connect(SchemaVerifier.startAt(3));
// you could also use the DatabaseConnection to insert data to later verify that it's still intact
await verifier.verifySchema(db, 4)

If you want to test schema migrations towards an older version (e.g. your current is 4, and you want to test from 2 to 3), you'd need to change the database class. Maybe moor can provide apis to make that easier too.

[kuhnroyal]

This probably won't work with build_runner since we need to have statically known filenames, so their name can't depend on the content of the input file.

That makes me sad 😢
But we don't really need a json file, we could generate this into a single Dart file that contains all the schemas, maybe as simple as global Strings or in some wrapper class. The filename would be known as there is only a single file.

This sounds like a really cool idea!

Thanks but this is not my idea. sqldelight uses https://github.com/schemacrawler/SchemaCrawler which does this

Would that work for you?

I think so, this looks about right. Pretty much the same API as https://developer.android.com/reference/android/arch/persistence/room/testing/MigrationTestHelper - could probably use that as guidance. The validateDroppedTables comes to mind for something that needs some special handling.

If you want to test schema migrations towards an older version (e.g. your current is 4, and you want to test from 2 to 3), you'd need to change the database class. Maybe moor can provide apis to make that easier too.

Yea there should probably be some simple non-generated database that only handles plain SQL. Dart classes will not match the historical version. This also implies that all migrations can always only be based on SQL statements and not on Dart entities.

I can invest a couple days into this if you need help and can give me some pointers.

[davidmartos96]

FWIW I can share some code I did on my project to have some basic tests around the migrations of the database. It makes use of sqlparser to compare the schemas of two versions of a databases. It's quite basic, but it's been useful for me, at least to check that the result of migrating a database matches a database created from scratch.

Future<void> compareSchemas(AppDatabase db1, AppDatabase db2) async {
  final master1 = await getSqliteMaster(db1);
  final master2 = await getSqliteMaster(db2);

  expect(master1.length, master2.length);
  for (var i = 0; i < master1.length; i++) {
    final rowDb1 = master1[i];
    final rowsDb2 = master2[i];
    expect(rowDb1["type"], rowsDb2["type"]);
    expect(rowDb1["tbl_name"], rowsDb2["tbl_name"]);
    expectSqlCreate(rowDb1["sql"] as String, rowsDb2["sql"] as String);
  }
  await db1.close();
  await db2.close();
}

Future<List<Map<String, dynamic>>> getSqliteMaster(AppDatabase db) {
  return db.executor.runSelect(
    "SELECT * FROM sqlite_master ORDER BY tbl_name",
    <dynamic>[],
  );
}

void expectSqlCreate(String actualCreateSql, String expectedCreateSql) {
  if (actualCreateSql == expectedCreateSql) {
    return;
  }
  expect(actualCreateSql != null && expectedCreateSql != null, isTrue);

  final engine = SqlEngine();

  final actualRes = engine.parse(actualCreateSql);
  final expectedRes = engine.parse(expectedCreateSql);

  final equals = astNodeEquals(actualRes.rootNode, expectedRes.rootNode);
  expect(equals, isTrue, reason: "$actualCreateSql\n!=\n$expectedCreateSql");
}

bool astNodeEquals(AstNode node1, AstNode node2) {
  if (!node1.contentEquals(node2)) {
    if (node1 is ColumnDefinition && node2 is ColumnDefinition) {
      if (node1.columnName != node2.columnName ||
          !areEquivalentSqlTypes(node1.typeName, node2.typeName)) {
        return false;
      }
    } else {
      return false;
    }
  }
  final childNodes1 = node1.childNodes.toList();
  final childNodes2 = node2.childNodes.toList();
  if (childNodes1.isEmpty && childNodes2.isEmpty) {
    return true;
  } else if (childNodes1.length != childNodes2.length) {
    return false;
  } else {
    for (var i = 0; i < childNodes1.length; i++) {
      final equals = astNodeEquals(childNodes1[i], childNodes2[i]);
      if (!equals) {
        return false;
      }
    }
    return true;
  }
}

List<Set<String>> _equivalentSqlTypes = [
  {"VARCHAR", "TEXT"},
  {"INTEGER", "BOOLEAN"},
];

bool areEquivalentSqlTypes(String type1, String type2) {
  if (type1 == type2) {
    return true;
  }

  for (var equivalences in _equivalentSqlTypes) {
    if (equivalences.contains(type1) && equivalences.contains(type2)) {
      return true;
    }
  }

  return false;
}

[kuhnroyal]

Thanks, this looks like a good starting point for a temporary solution. How do you create the database in the old revision? Do you store them in VCS?

[davidmartos96]

@kuhnroyal Yes, I have v1 of the database stored as a file in the tests folder. Then I have a AppDatabase factory constructor that receives a specific schemaVersion, to trigger the migrations.

[kuhnroyal]

@simolus3 What version of sqlite is currently bundled with ffi? Can I expect this to be available?

2018-09-15 (3.25.0)

1. Enhancements the ALTER TABLE command:
   a. Add support for renaming columns within a table using ALTER TABLE table RENAME COLUMN oldname TO newname.
   b. Fix table rename feature so that it also updates references to the renamed table in triggers and views.

[simolus3]

sqlite3_flutter_libs currently bundles 3.32.3 on both iOS and Android, so it should be available.

[kuhnroyal]

Hello @simolus3, I am playing around with the new TableMigration on develop.

I am trying something like this:

    await db.transaction(() async {
      await m.alterTable(
        TableMigration(db.uploads, newColumns: [
          db.uploads.tag
        ], columnTransformer: {
          db.uploads.tag: const Constant<String>('unused'),
          // db.uploads.tag: const Variable<String>('unused'),
        }),
      );
      await db.customStatement("DELETE FROM uploads WHERE state != 'uploaded'");
    });

But this throws an exception. Basically I want to fill the new tag (which is NOT NULL) column for all existing rows with some string value. This is what I get:

flutter: NoSuchMethodError: The getter 'length' was called on null.
flutter: Receiver: null
flutter: Tried calling: length
flutter: --------------------------------------------------------------------------
flutter: NoSuchMethodError: The getter 'length' was called on null.
flutter: Receiver: null
flutter: Tried calling: length
flutter: --------------------------------------------------------------------------
flutter: dart:core-patch/object_patch.dart 51:5                                              Object.noSuchMethod
flutter: dart:convert/utf.dart 88:31                                                         Utf8Encoder.convert
flutter: dart:convert/codec.dart 21:32                                                       Codec.encode
flutter: package:sqlite3/src/ffi/memory.dart 42:29                                           allocateZeroTerminated
flutter: package:sqlite3/src/impl/database.dart 87:20                                        DatabaseImpl.execute
flutter: package:moor/src/ffi/vm_database.dart 153:11                                        _VmDelegate._runWithArgs
flutter: package:moor/src/ffi/vm_database.dart 163:11                                        _VmDelegate.runCustom
flutter: package:moor/src/runtime/executor/helpers/engines.dart 79:19                        _ExecutorWithQueryDelegate.runCustom.<fn>
flutter: package:synchronized/src/basic_lock.dart 32:26                                      BasicLock.synchronized
flutter: package:moor/src/runtime/executor/helpers/engines.dart 22:26                        _ExecutorWithQueryDelegate._synchronized
flutter: package:moor/src/runtime/executor/helpers/engines.dart 76:12                        _ExecutorWithQueryDelegate.runCustom
flutter: package:moor/src/runtime/api/query_engine.dart 301:23                               QueryEngine.customStatement.<fn>
flutter: package:moor/src/runtime/api/query_engine.dart 98:64                                QueryEngine.doWhenOpened.<fn>
flutter: package:stack_trace/src/stack_zone_specification.dart 126:26                        StackZoneSpecification._registerUnaryCallback.<fn>.<fn>
flutter: package:stack_trace/src/stack_zone_specification.dart 208:15                        StackZoneSpecification._run
flutter: package:stack_trace/src/stack_zone_specification.dart 126:14                        StackZoneSpecification._registerUnaryCallback.<fn>
flutter: dart:async/zone.dart 1198:47                                                        _rootRunUnary
flutter: dart:async/zone.dart 1100:19                                                        _CustomZone.runUnary
flutter: dart:async/future_impl.dart 143:18                                                  _FutureListener.handleValue
flutter: dart:async/future_impl.dart 696:45                                                  Future._propagateToListeners.handleValueCallback
flutter: dart:async/future_impl.dart 725:32                                                  Future._propagateToListeners
flutter: dart:async/future_impl.dart 529:5                                                   Future._completeWithValue
flutter: dart:async-patch/async_patch.dart 40:15                                             _AsyncAwaitCompleter.complete
flutter: dart:async-patch/async_patch.dart 311:13                                            _completeOnAsyncReturn
flutter: package:moor/src/runtime/executor/helpers/engines.dart                              _TransactionExecutor.ensureOpen

Probably caused by the Constant expression. Some statement seems to be null but the trace is not very conclusive.
Any ideas?

[simolus3]

Thanks for the feedback, @kuhnroyal! In this case, I'm unable to properly reproduce this though. I added a test that's very similar to your snippet in 14aa07fffa58f6be9553e67ae9ce291dee2554d4, but it passes.

The stacktrace indicates that customStatement(null) was called. It think that's unlikely to happen in the migrator since we build all our queries from a StringBuffer or a direct string literal. Can you check whether that could happen anywhere in your code?

[kuhnroyal]

It is definitely coming from the migrator, can't seem to get the correct stacktrace in the console but here it is:

Edit:
Looking at the code I somehow think that createAffected somehow contains a null string.

Edit 2: Not sure how and why but there it is :)

There are no manual indexes created by me on this table.

Edit 3: There is an autoindex on the table, very likely due to a unique constraint on the table.

[kuhnroyal]

I created #870 to fix this.

However, thinking about this kind of table migration API a bit longer leads me to think that it might not be very practical in the future.

Say I have a migration for v2 that adds a column m.addColumn(foos, foos.bar).

Now a month later I decide that v4 should change the column to be nullable, or be removed or whatever. These changes automatically lead to changes to the behavior of the v2 migration. If there are still app users with v1 out there who wish to update, well good bye :) Or v2 doesn't even compile anymore.

This is probably part of the reason why other tools generate the actual required SQL statements when a migration is required or you have to write it manually as SQL.

[davidmartos96]

I agree with Peter.
The currently worked on migration API is simple and very nice to use. But at the same time can cause issues like the one described by Peter if you are not careful enough or if you don't have tests that cover migrating a database from version 1 to N.
What I do at the moment is use raw statements for my migrations since those will never be modified by the code generator.

[simolus3]

If there are still app users with v1 out there who wish to update, well good bye :) Or v2 doesn't even compile anymore.

I agree that this is a real issue. In practice most of these problems can be fixed by adapting the older migrations. Since a user would always migrate to the latest version, one could just remove the addColumn call if it's not needed for v4, and then drop the column for v3 -> v4.

Of course, I see how that's error-prone and annoying to maintain. All of the Migrator apis have this problem since they can only see the current state of the schema. They work great for simple migrations where you're mostly just adding columns, but they don't replace the advanced tooling we discussed here.

[simolus3]

FWIW, I started some work on this in the verify-migrations branch. You can view the initial documentation here, I also started a (for now, tiny) example project here.

My approach was to re-use most of the existing generator code to create non-abstract GeneratedDatabase classes for each schema version (so DatabaseAtV1, DatabaseAtV2 and so on). Each version will have its own (stripped down) table classes, so that we can use Migrator.createAll to instantiate the expected schema.

So if you wanted to test the migration from v1 to v2, you could

1. populate a database with DatabaseAtV1
2. connect your app-database to that and run a migration to v2
3. populate another database with DatabaseAtV2
4. compare the sqlite_schema table of those databases

That's pretty much what the new API in moor_generator is doing too.

The comparison algorithm is still incomplete and I'll need to make some changes to the schema export for virtual tables, but the general approach should work and I hope we can have an experimental version of this in the next update.

@davidmartos96 Since you already have some migration tests, I wonder how you instantiated the reference database so far? Did you create a new database file after making changes to your schema as reference? Are you storing them in your repository? I'd love to compare some approaches to spot potential problems. One issue with my approach is that it can't really reflect custom onCreate callbacks in the generated databases for instance.

[davidmartos96]

@simolus3 That is great news Simon!

Did you create a new database file after making changes to your schema as reference? Are you storing them in your repository?

That's correct. I have version 1 of the database checked out in the repository. I don't have it checked out for all the schema versions though. I use the v1 to create the latest version of the database via migration and compare it against creating it from scratch. The comparison I do it with the sqlite_master table + your SQLite parser package.

[simolus3]

I think this can be closed now as the TableMigration api and schema verifications have been released. Suggestions for further apis or tooling should be discussed in separate issues.