Remove orphan syncstats entries referencing non-existent collections

[sunkup]

Purpose

Remove orphan syncstats entries referencing non-existent collections in database migration 17 to 18.

Short description

For some reason it did not work with the existing auto migration so I changed it into a manual one. I suspect the deletion of orphan entries needs to happen before the column name is changed although I do not understand why.

• use a manual migration from 17 to 18
• delete orphan syncstats entries (referencing non-existing collections) in the migration
• test the migration deletes the orphan entries

Checklist

• The PR has a proper title, description and label.
• I have self-reviewed the PR.
• I have added documentation to complex functions and functions that can be used by other modules.
• I have added reasonable tests or consciously decided to not add tests.

[Copilot]

Pull Request Overview

This PR replaces an automatic database migration with a manual one to properly handle orphan syncstats entries that reference non-existent collections during the migration from database version 17 to 18.

• Converts AutoMigration18 to a manual Migration18 to ensure orphan deletion occurs before column renaming
• Adds explicit deletion of orphan syncstats entries before performing the migration
• Includes comprehensive test coverage for the orphan removal functionality

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
Migration18.kt	New manual migration that deletes orphan syncstats entries before renaming authority column to dataType
AutoMigration18.kt	Removed auto migration file that couldn't handle orphan deletion properly
AppDatabase.kt	Updated to use manual migration instead of auto migration for version 17→18
Migration18Test.kt	Added test to verify orphan syncstats entries are properly removed during migration

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.}

[ArnyminerZ]

Looks good to me. I would not bother on what Copilot said.

[rfc2822]

How can that work when there is a ForeignKey(childColumns = arrayOf("collectionId"), entity = Collection::class, parentColumns = arrayOf("id"))?

Is there something wrong with the foreign key definition?

[sunkup]

No I don't think so. SQLite foreign key constraints are disabled by default (See also this reddit post) and I think room will enable them internally only, but not for our SupportSQLiteDatabase executed queries in tests. :)

[rfc2822]

Ok this is extremly important to know (and to document).

We have to make sure to understand when these constraints are active because our code relies on them.

[sunkup]

Right. I will try to verify that and add an elaborate comment

[sunkup]

Hmm. Unfortunately the documentation on ForeignKey does not mention this at all ...

So there is PRAGMA foreign_keys (=ON/OFF) enable/disable foreign key constraint enforcement in SQLite. In room there is a wrapper method setForeignKeyConstraintsEnabled() which executes the db query to alter the state.

I would imagine the call to setForeignKeyConstraintsEnabled(true) to be in RoomOpenHelper.onConfigure, since it extends SupportSQLiteOpenHelper.Callback which states in the kdoc for it's onConfigure callback, (here as well) that:

  * This method should only call methods that configure the parameters of the database
  * connection, such as [SupportSQLiteDatabase.enableWriteAheadLogging]
  * [SupportSQLiteDatabase.setForeignKeyConstraintsEnabled],
  * [SupportSQLiteDatabase.setLocale], [SupportSQLiteDatabase.setMaximumSize], or executing
  * PRAGMA statements.

Then there is also this query setting it directly which I don't really understand though.

So I can't find the exact place ... but maybe you or @ArnyminerZ know a bit better where to dig deeper?

[sunkup]

I tested inserting an invalid syncstats row in App.onCreate(). Once via our syncstats DAO and once using a direct query. They both fail on a foreign key constraint exception, so I am pretty sure room enables it somehow.

Since the SQLite documenation on enabling foreign key support says that it needs to be done per database connection it could very well be the last link I mentioned where it's being set directly using pragma before writing in createOnOpen. There probably is some way to verify that easily without cloning the whole AOSP, but I don't know how and I don't really think it's super necessary ...

[rfc2822]

I read that foreign keys are by default disabled in testing SQLite, but not for the production app. However I couldn't verify that yet.

They both fail on a foreign key constraint exception, so I am pretty sure room enables it somehow.

But then there's the question how the original issue could occur? This is the thing that irritates me most.

Maybe we should enable it explicitly just to be sure? However if it was previously not enabled for some reason (like in the original issue), would that cause foreign-key exceptions?

[sunkup]

It should always have been enabled in production, otherwise I would strongly expect them to say so in the dev docs for room. They don't mention it because everyone expects foreign key constraint enforcement to be enabled by default.

I think we can enable it ourselves once more too, just to make sure, and if it does actually cause foreign-key exceptions (user or play console reports), then we know that it was not properly enforced before and we kind of know where the issue came from :)

[rfc2822]

It should always have been enabled in production,

I think so too. But how could it be disabled in the original issue?

Maybe the user has edited the database manually previously with some tool that didn't set the PRAGMA?

@steelman Is it possible that you edited the database manually already some time before?