Moor: Testing Verifying migrations

[bdlindsay]

Hey there,

Thanks for merging in #908 so quickly. My team was trying out the experimental migration verification and ran into an unexpected issue. We've verified manually that our migration code is working, but when trying to write the automated test using Schema Verifier, it's showing some errors. Could the generated table SQL be incorrect, or maybe something stands out to you? The error does not occur for every table in our schema, but it is occurring for more tables than are affected by the migration.

Our build.yaml

```build.yaml. This file is quite powerful, see https://pub.dev/packages/build_config
targets:
$default:
builders:
moor_generator:
options:
write_from_json_string_constructor: true
generate_connect_constructor: true

The test code (Where Database is an @useMoor database):

import '../../generated_migrations/schema.dart';
import '../../generated_migrations/schema_v2.dart';
import 'package:moor_generator/api/migrations.dart';

void main() {
test('testing moor migration testing', () async {
final schemaVerifier = SchemaVerifier(GeneratedHelper());
final v2Connection = await schemaVerifier.startAt(2);
final v2database = Database.connect(v2Connection);

  try {
    await schemaVerifier.migrateAndValidate(v2database, 3);

    return;
  } on SchemaMismatch catch (error) {
    fail('$error');
  }
});

}

The error:

Schema does not match
plots:
Internal error: Could not parse CREATE TABLE plots (owner_id VARCHAR NOT NULL, deleted INTEGER NOT NULL, last_synced INTEGER NULL, last_modified INTEGER NOT NULL, id VARCHAR NOT NULL, name VARCHAR NOT NULL, crop VARCHAR NOT NULL, crop_variety VARCHAR NOT NULL, planting_date INTEGER NOT NULL, is_outdoor_trial INTEGER NOT NULL CHECK (is_outdoor_trial in (0, 1)), uses_crop_blanket INTEGER NOT NULL CHECK (uses_crop_blanket in (0, 1)), is_raised_bed INTEGER NOT NULL CHECK (is_raised_bed in (0, 1)), plant_spacing REAL NOT NULL, row_spacing REAL NOT NULL, irrigation_type VARCHAR NOT NULL, rows_per_bed REAL NULL, bed_spacing REAL NULL, coordinates VARCHAR NOT NULL, PRIMARY KEY (id))
lab_reports:
Internal error: Could not parse CREATE TABLE lab_reports (owner_id VARCHAR NOT NULL, deleted INTEGER NOT NULL, last_synced INTEGER NULL, last_modified INTEGER NOT NULL, id VARCHAR NOT NULL, sample_date INTEGER NOT NULL, source VARCHAR NOT NULL, PRIMARY KEY (id))
dissipated_active_ingredients:
Internal error: Could not parse CREATE TABLE dissipated_active_ingredients (id VARCHAR NOT NULL, lab_report_id VARCHAR NOT NULL, active_ingredient_id VARCHAR NULL, name VARCHAR NULL, residue REAL NOT NULL, PRIMARY KEY (id))
sprays:
Internal error: Could not parse CREATE TABLE sprays (owner_id VARCHAR NOT NULL, deleted INTEGER NOT NULL, last_synced INTEGER NULL, last_modified INTEGER NOT NULL, id VARCHAR NOT NULL, create_date INTEGER NOT NULL, application_method VARCHAR NOT NULL, growth_stage VARCHAR NOT NULL, PRIMARY KEY (id))
products:
Internal error: Could not parse CREATE TABLE products (id VARCHAR NOT NULL, trade_name VARCHAR NOT NULL, type VARCHAR NOT NULL, owner_id VARCHAR NULL, PRIMARY KEY (id))
markets:
Internal error: Could not parse CREATE TABLE markets (owner_id VARCHAR NOT NULL, deleted INTEGER NOT NULL, last_synced INTEGER NULL, last_modified INTEGER NOT NULL, id VARCHAR NOT NULL, name VARCHAR NOT NULL, mrl REAL NOT NULL, arfd REAL NOT NULL, adi REAL NOT NULL, total_active_ingredients INTEGER NOT NULL, PRIMARY KEY (id))
```

[simolus3]

Thanks for the report! This appears to be caused by the NULL constraints (e.g. last_synced INTEGER NULL). This syntax isn't listed in sqlite's syntax diagram, so I missed that case in the parser. Sqlite obviously supports that (source), so we should support it too.

I just adapted the parser to work with the statements you posted, you can try out the new version by adding this to your pubspec:

dev_dependencies:
  sqlparser:
    git:
      url: https://github.com/simolus3/moor.git
      ref: develop
      path: sqlparser

[bdlindsay]

Do I need a dev version of moor generator as well? I couldn't resolve both moor_generator and the sqlparser.

Because moor_generator 3.4.0 depends on sqlparser ^0.11.0 and no versions of moor_generator match >3.4.0 <4.0.0, moor_generator ^3.4.0 requires sqlparser from hosted.

The dev dependencies

moor_generator: ^3.4.0 # Generates our database
sqlparser:
    git:
      url: https://github.com/simolus3/moor.git
      ref: develop
      path: sqlparser

[simolus3]

Ah sorry, you'll actually need a dependency override. Remove sqlparser from your dev_dependencies and add it like this:

dependency_overrides:
  sqlparser:
    git:
      url: https://github.com/simolus3/moor.git
      ref: develop
      path: sqlparser

[bdlindsay]

So that fix does solve the parsing error - thanks for the quick fix! Reading the documentation of schemaVerifier.migrateAndValidate I would've expected it to call the onUpgrade function of the passed in database's migration strategy. Just running migrateAndValidate the error seems clear that the schema stays at the previous version, and calling the migration myself before migrateAndValidate I do get a passing schema verification. What did you intend the behavior to be?

[simolus3]

I would've expected it to call the onUpgrade function of the passed in database's migration strategy

It should do that! We call database.beforeOpen() with the appropriate versions, which should invoke the migration. Did you override that method by any chance? The default implementation should call onUpgrade. We don't call onUpgrade directly to closely match the behavior you'd see in a real implementation.

[kuhnroyal]

I have also started to experiment with this. I found a problem that prevents me from instantiating previous schemas when declaring the primary key in a moor file as table level constraint and not on the column directly:

CREATE TABLE foos(
    id INTEGER NOT NULL,
    name TEXT NOT NULL,
    deleted BOOLEAN DEFAULT FALSE,
    PRIMARY KEY (id)
) AS FooEntity;

This usually generates bool get dontWriteConstraints => true; in the table class but not for the schema_vx.dart files, resulting in SqliteException(1): table "foos" has more than one primary key.

The json for this table contains:

 "constraints": [
  "PRIMARY KEY (id)"
],
"explicit_pk": [
  "id"
]

[bdlindsay]

I would've expected it to call the onUpgrade function of the passed in database's migration strategy

It should do that! We call database.beforeOpen() with the appropriate versions, which should invoke the migration. Did you override that method by any chance? The default implementation should call onUpgrade. We don't call onUpgrade directly to closely match the behavior you'd see in a real implementation.

I'm not overriding beforeOpen, but I am overriding super.migration to set the Migration strategy. Does the beforeOpen in the migration strategy take place over the database's beforeOpen?

[simolus3]

I found a problem that prevents me from instantiating previous schemas when declaring the primary key in a moor file

Thanks for the report, I've fixed this in 95dacd986cec3d63e885485fa8beb9a3905099d0.

Does the beforeOpen in the migration strategy take place over the database's beforeOpen?

No, those are entirely unrelated, it's a bit unfortunate that they share their names. Overriding migration is the intended use and shouldn't break anything. I assume that your onUpgrade callback did get called though, as the default implementation for onUpgrade just throws. Can you verify that with a breakpoint or a print inside onUpgrade?

[kuhnroyal]

Same here. The parsing error is now solved, thanks! But the schema stays at the old version and the migration is not run, tested this with a print.

[kuhnroyal]

Well onUpgrade is called with 49 as from when I try to migrate 3 -> 4 😆
This is what my print logs:

print('### migrate $from $to');

### migrate 49 4

Need to investigate more tomorrow.

[bdlindsay]

The code actually isn't catching on breakpoints running through the debug mode in VSCode, but also printing I get a similar issue to kuhnroyal (but also opposite 'wrong' version). My migration 2 -> 3 is print('migrating! $fromVersion, $toVersion'); -> migrating! 11, 3

[kuhnroyal]

Seems like the runtimeSchemaVersion is wrong and is somehow related to the number of tables/triggers/views in the database. @bdlindsay Does 11 fit you table count?

[kuhnroyal]

This works in VerifierImplementation, setting the start version with a PRAGMA:

  @override
  Future<DatabaseConnection> startAt(int version) async {
    final executor = VmDatabase.memory();
    final db = helper.databaseForVersion(executor, version);
    await db.customStatement('PRAGMA schema_version = $version');

    // Opening the helper database will instantiate the schema for us
    await executor.ensureOpen(db);

    return DatabaseConnection.fromExecutor(executor);
  }

[kuhnroyal]

With above fix I can write complex tests, this is really great work!

One question, how can I avoid setting dontWarnAboutMultipleDatabases = true when writing multiple tests for one schema version?
I tried closing the database and connection but that doesn't seem to be enough.

  // TODO figure out how to get rid of this
  moorRuntimeOptions.dontWarnAboutMultipleDatabases = true;
  SchemaVerifier verifier;
  DatabaseConnection connection;
  AppDatabase db;

  setUp(() async {
    verifier = SchemaVerifier(GeneratedHelper());
    connection = await SchemaVerifier(GeneratedHelper()).startAt(3);
    db = AppDatabase.connect(connection);
  });

  tearDown(() async {
    await connection.executor.close();
    await db.close();
  });

[bdlindsay]

Yes, 11 matches my number of tables. Using a custom statement to set the schema version also works for me.

[simolus3]

Thanks again for your feedback! I've added the version pragma Peter suggested. The issue about duplicate databases actually came from the internal helper database which wasn't properly closed by the verifier. I've fixed this by using two separate executors in c7669a18a026b79ab9e643723d230203ea0615f0.
I had to add another VmDatabase factory for this, so you'll have to add a dependency_override for the core moor package as well if you're using this.

[kuhnroyal]

@simolus3 The dbForUse is empty when returned from startAt. The migration tests are only working because the database is migrated from 0 to X. Or am I missing something here?
I do some inserts before migrateAndValidate and the db has no tables there.

[simolus3]

That was a bug related to VmDatabase.opened - it should be fixed in 08470abd479ea5b752a990a4cb8aee2b1c5b5ac3

[kuhnroyal]

The database returned from startAt is now in the latest version but not in the requested one.

[simolus3]

Apologies for the slow and and forth progress here. However, I couldn't reproduce the issue about startAt already being at the latest version. Did you check the version by running a query via customSelect on your database class? That would open the database and run your regular migration (after which it would be at the latest version of course).
I did just fix a problem where migrateAndValidate would always migrate to the latest version, regardless of the expected version provided. Is that what you meant?

[kuhnroyal]

No worries. I didn't check the version but my renamed column (version 4) is already renamed when I use startAt(3). My just insert some data into the table at version 3 before running the migration and that part fails with no such column. This worked before.

        final verifier = SchemaVerifier(GeneratedHelper());
        final connection = await verifier.startAt(3);
        final db = AppDatabase.connect(connection);

        print((await db.customSelect('SELECT sql FROM sqlite_master WHERE name = "file_uploads";').getSingle()).data);

The print already outputs the DDL for version 4, which is my latest version.

[simolus3]

Yes, as soon as you use the database, it will be be opened which involves running the regular migration to the latest version. So if you call customSelect, we'd first run the migration and then run the query, which will then report the updated column name of course. Previously I made startAt return an opened connection so that the db wouldn't migrate when used. I no longer think that this is the right approach because

• there is only one connection. Closing a database closes the underlying connection, so we can't close the helper database used to instantiate the initial schema which causes duplicate instantiation warnings
• it doesn't really make sense for a database to operate on an older schema, this diverges from the behavior you'd see in a real implementation. This might be related to the "no such column" warning you were seeing, as the AppDatabase and its generated companions would always use the latest name.

Of course, we should allow users to insert custom data before a migration to verify that migrations don't destroy data, I'm just not sure about the right approach here. We could have something like this

final verifier = SchemaVerifier(GeneratedHelper());
final someStructure = await verifier.someMethodFor(3);

// Initialize data
final Database raw = someStructure.rawDb; // raw db from package:sqlite3
raw.execute('INSERT INTO ...');

// Proper test now
final db = AppDatabase.connect(someStructure.connection);
await verifier.migrateAndValidate(db, 4);

// Additional queries and assets to verify data integrity

That's a bit more complicated, but I assume something like this would enable your use case again? It's similar to Room's apis, which don't let you use DAOS for initialization either.

[kuhnroyal]

Right, I see you point. The older revision uses the newer generated database classes.
Raw statements is all that should be used anyways for these tests so using a plain sqlite3 db should be sufficent.

[simolus3]

I added the apis in in 6c913a481145019a8c37c3d6f2d4480ac884f83e. Thanks for all the suggestions and feedback!

[kuhnroyal]

This works now, thank you!