Android-components: Detect when we change a database schema without adding a migration for it

[Amejia481]

Let's discuss about which possible solutions we can have to prevent early this issue.

[Amejia481]

cc @pocmo @jonalmeida @rocketsroger @csadilek @psymoon @grigoryk

[Amejia481]

I was thinking about a possible lint rule that help us with that, it is weird that there is not something already in room for that.

[pocmo]

A lint rule would be nice. But I assume writing a lint rule is pretty hard since it is hard to know whether a migration will be needed by just looking at the code?

Room supports writing schema migration tests, e.g.:

• https://github.com/mozilla-mobile/android-components/blob/master/components/feature/pwa/src/androidTest/java/mozilla/components/feature/pwa/db/ManifestDatabaseMigrationTest.kt
• https://developer.android.com/training/data-storage/room/migrating-db-versions

I wonder if we can use that to write a more generic test that performs all the migrations? And then tries to read/write data?

Or maybe a combined approach. If the tests follow a strict naming convention then a lint rule could be triggered if there is no migration test for every schema version?

[Amejia481]

Or maybe a combined approach. If the tests follow a strict naming convention then a lint rule could be triggered if there is no migration test for every schema version?

Yes, I think have a generic test that read and write could be a good alternative!
I will also take a look to the "Android Study Group" slack group to see if there is something mentioned there too.

[Amejia481]

Another thing that I noticed that could help us to spot a missing migration is that file schemas/#.json has changed but file is not committed.

Every time, we change the schema the schemas/#.json gets updated by room. When we have this file updated but not committed, it will be very likely that we are missing a migration. We can have as part of our test suit a task that verify if there are schemas/#.json files that are not committed if so we can fail indicating that a possible migration have been skipped.

[Amejia481]

There is a way to run all the migrations to check for integrity https://developer.android.com/training/data-storage/room/migrating-db-versions#all-migrations-test I'm not sure how we can make that generic for all components that are using room, unless we enforce some strict naming convention or an is a relationship between all the classes that use room.

[Amejia481]

After we build every component we can run a script that verifies if there are none untracked files, if there are we fail the tests.

set -ex
OUTPUT=$(git status) 

if [[ "$OUTPUT" == *"Untracked"* && "$OUTPUT" == *".json"* ]]; then
    echo '"Possible missing migration, there is a room untracked file'
    exit 0
fi

Another line of defence is to create a test that test run all the migrations as shown here. We can have common base class or interface that provide us all the info that the test needs.

What do you think? @pocmo

[grigoryk]

After we build every component we can run a script that verifies if there are none untracked files, if there are we fail the tests.

Would that live in a task that's similar to complete-pr?

This would make a good pre-push hook (and have it emit a warning?), to help shorten the cycle a bit. Locally, I think you'd want a slightly more specific check than *".json"* though.

[Amejia481]

Would that live in a task that's similar to complete-pr?

Yes.

This would make a good pre-push hook (and have it emit a warning?), to help shorten the cycle a bit. Locally, I think you'd want a slightly more specific check than ".json" though.

Having it as a pre-push will be a great idea, additionally I think we could need a way to verify on CI too :)
Yes we can improve the check by adding a number as part of the file *"([0-9]*).json"*.
If we have it on CI any untracked file could be a possible red flag.

[pocmo]

If you want to be super strict then we probably should also not allow the modification of an already commited schema. That would imply that someone modified the schema and did not set a new version and create a migration. We probably should consider everything that landed in the repo as published (which with our nightly builds is not that unlikely) and therefore requiring a new schema version and migration when changed.

[Amejia481]

If you want to be super strict then we probably should also not allow the modification of an already commited schema. That would imply that someone modified the schema and did not set a new version and create a migration. We probably should consider everything that landed in the repo as published (which with out nightly builds is not that unlikely) and therefore requiring a new schema version and migration when changed.

I like the idea, this way we can enforce that every time someone modifies the schema MUST update the db version and subsequently create a migration.

[Amejia481]

@pocmo I'm working on generic test to run all the migrations but I noticed that we are not running Android tests on CI this is one prerequisite for this ticket, maybe this is one of the reasons why we didn't caught some issues before. I thought we were already doing that, do you know why it is that?

[pocmo]

We are running Android instrumentation tests on Firebase for components configured here:
https://github.com/mozilla-mobile/android-components/blob/9987b74993340250e88bfa7fc11530d07c2caf08/taskcluster/ci/test/kind.yml#L51-L72

If there are more/new components with tests to run there then let's add them. :)

[Amejia481]

Oh I didn't know that, thanks :)
There are other components that have android instrumentation tests that are not added there, this is the reason why there are some test that are failing locally.