diff --git a/app/src/main/java/de/softwareapp_hb/filemanager/data/SqliteTrashIndex.kt b/app/src/main/java/de/softwareapp_hb/filemanager/data/SqliteTrashIndex.kt index 8a94689..269d569 100644 --- a/app/src/main/java/de/softwareapp_hb/filemanager/data/SqliteTrashIndex.kt +++ b/app/src/main/java/de/softwareapp_hb/filemanager/data/SqliteTrashIndex.kt @@ -85,7 +85,33 @@ class SqliteTrashIndex(context: Context) : TrashIndex { private class Helper(context: Context) : SQLiteOpenHelper(context, DATABASE_NAME, null, DATABASE_VERSION) { - override fun onCreate(db: SQLiteDatabase) { + override fun onCreate(db: SQLiteDatabase) = create(db) + + override fun onUpgrade(db: SQLiteDatabase, oldVersion: Int, newVersion: Int) = + upgrade(db, oldVersion, newVersion) + } + + /** + * The table's shape, and how that shape moves forward. + * + * `internal` rather than `private` so a test can drive `create` and `upgrade` against a + * throwaway database, which is the only way to prove the upgrade path without waiting for + * a real version 2 to exist. + */ + internal companion object { + const val DATABASE_NAME = "trash-index.db" + const val DATABASE_VERSION = 1 + const val TABLE = "trashed" + const val COLUMN_STAGED_PATH = "staged_path" + const val COLUMN_BATCH_ID = "batch_id" + const val COLUMN_BATCH_FOLDER = "batch_folder" + const val COLUMN_ORIGINAL_PATH = "original_path" + const val COLUMN_NAME = "name" + const val COLUMN_IS_DIRECTORY = "is_directory" + const val COLUMN_SIZE_BYTES = "size_bytes" + const val COLUMN_STAGED_AT = "staged_at" + + fun create(db: SQLiteDatabase) { db.execSQL( """ CREATE TABLE $TABLE ( @@ -104,27 +130,33 @@ class SqliteTrashIndex(context: Context) : TrashIndex { db.execSQL("CREATE INDEX trashed_staged_at ON $TABLE ($COLUMN_STAGED_AT)") } - override fun onUpgrade(db: SQLiteDatabase, oldVersion: Int, newVersion: Int) { - // Dropping rather than migrating, which is only acceptable while nothing has - // shipped: unlike every other kind of cache here, this one cannot be rebuilt - // from the device, because the device never recorded where the items came from. - // A released version needs a real migration. - db.execSQL("DROP TABLE IF EXISTS $TABLE") - onCreate(db) + /** + * Bring [db] from [oldVersion] up to [newVersion], one version at a time. + * + * Version 1 is the first version this app has shipped, so there is nothing to migrate + * yet — the loop is empty and [DATABASE_VERSION] is still 1. The *shape* is the point. + * When version 2 arrives, the step for version 1 becomes the work that gets there: an + * `ALTER TABLE` plus a backfill for an added column, or a copy into a new table and a + * swap for a change SQLite cannot make in place. + * + * **A bare `DROP` must never appear here.** This is the one cache in the app that + * cannot be rebuilt from the device. The staged files are still on the disk, but only + * this table knows where they came from, so emptying it does not lose a cache — it + * turns every staged delete into a permanent one, silently, with every row still + * present on disk and no error raised anywhere. The old code did exactly that, which + * was defensible only while nothing had shipped. + * + * A version with no step is therefore a bug to be reported, not an excuse to wipe the + * table, which is why this ends in an `error` rather than in [create]. + */ + fun upgrade(db: SQLiteDatabase, oldVersion: Int, newVersion: Int) { + for (version in oldVersion until newVersion) { + error( + "No migration from trash-index schema version $version to ${version + 1}. " + + "Add one to SqliteTrashIndex.upgrade and bump DATABASE_VERSION; " + + "do not drop the table.", + ) + } } } - - private companion object { - const val DATABASE_NAME = "trash-index.db" - const val DATABASE_VERSION = 1 - const val TABLE = "trashed" - const val COLUMN_STAGED_PATH = "staged_path" - const val COLUMN_BATCH_ID = "batch_id" - const val COLUMN_BATCH_FOLDER = "batch_folder" - const val COLUMN_ORIGINAL_PATH = "original_path" - const val COLUMN_NAME = "name" - const val COLUMN_IS_DIRECTORY = "is_directory" - const val COLUMN_SIZE_BYTES = "size_bytes" - const val COLUMN_STAGED_AT = "staged_at" - } } diff --git a/app/src/test/java/de/softwareapp_hb/filemanager/data/SqliteTrashIndexTest.kt b/app/src/test/java/de/softwareapp_hb/filemanager/data/SqliteTrashIndexTest.kt index d283242..fb83969 100644 --- a/app/src/test/java/de/softwareapp_hb/filemanager/data/SqliteTrashIndexTest.kt +++ b/app/src/test/java/de/softwareapp_hb/filemanager/data/SqliteTrashIndexTest.kt @@ -1,6 +1,8 @@ package de.softwareapp_hb.filemanager.data +import android.database.sqlite.SQLiteDatabase import de.softwareapp_hb.filemanager.core.StoragePath +import java.io.File import kotlinx.coroutines.runBlocking import org.junit.Assert.assertEquals import org.junit.Assert.assertTrue @@ -85,4 +87,58 @@ class SqliteTrashIndexTest { fun `an empty index reads back empty rather than throwing`() = runBlocking { assertTrue(index().records().isEmpty()) } + + /** + * The upgrade path used to `DROP` the table and recreate it. That loses more than a cache: + * the staged files stay on disk, but nothing knows where they came from any more, so every + * staged delete silently becomes permanent — which is the one thing the trash exists to + * prevent. A version with no migration must therefore report itself and leave the rows be. + * + * Version 2 does not exist yet, which is exactly why this is worth pinning: the guard has + * to hold *before* the first real migration is written, not after. + */ + @Test + fun `a schema bump with no migration reports itself instead of emptying the index`() { + val db = scratchDatabase("migration-undefined") + SqliteTrashIndex.create(db) + insertOneRow(db) + + val failure = runCatching { SqliteTrashIndex.upgrade(db, 1, 2) } + + assertTrue("expected the missing step to be reported", failure.isFailure) + assertEquals("a version bump must not empty the index", 1, rowCount(db)) + } + + @Test + fun `an already current schema is left alone`() { + val db = scratchDatabase("migration-current") + SqliteTrashIndex.create(db) + insertOneRow(db) + + SqliteTrashIndex.upgrade(db, SqliteTrashIndex.DATABASE_VERSION, SqliteTrashIndex.DATABASE_VERSION) + + assertEquals(1, rowCount(db)) + } + + /** A throwaway database, so a migration can be driven without disturbing the real index. */ + private fun scratchDatabase(name: String): SQLiteDatabase = + SQLiteDatabase.openOrCreateDatabase( + File(RuntimeEnvironment.getApplication().cacheDir, "$name.db").apply { delete() }, + null, + ) + + private fun insertOneRow(db: SQLiteDatabase) { + db.execSQL( + "INSERT INTO ${SqliteTrashIndex.TABLE} " + + "(${SqliteTrashIndex.COLUMN_STAGED_PATH}, ${SqliteTrashIndex.COLUMN_BATCH_ID}, " + + "${SqliteTrashIndex.COLUMN_BATCH_FOLDER}, ${SqliteTrashIndex.COLUMN_ORIGINAL_PATH}, " + + "${SqliteTrashIndex.COLUMN_NAME}, ${SqliteTrashIndex.COLUMN_IS_DIRECTORY}, " + + "${SqliteTrashIndex.COLUMN_SIZE_BYTES}, ${SqliteTrashIndex.COLUMN_STAGED_AT}) " + + "VALUES ('/t/batch-1/0/a.txt', 'batch-1', '/t/batch-1', " + + "'/storage/emulated/0/Documents/a.txt', 'a.txt', 0, 42, 1000)", + ) + } + + private fun rowCount(db: SQLiteDatabase): Int = + db.query(SqliteTrashIndex.TABLE, null, null, null, null, null, null).use { it.count } }