Fix database corruption dialog#19692
Conversation
david-allison
left a comment
There was a problem hiding this comment.
I feel this PR mixes two concepts:
launchCatchingTaskdoes not handle a database corruption error properly- (maybe)
repairCollectiondoes not work if the database is corrupt
We should firstly fix the linked issue: showing the correct dialog via launchCatchingTask: anywhere in the app can throw due to a corrupt collection, and launchCatchingTask is our standard error handler which should handle this.
If repairCollection is broken, let's open a separate issue for it. This fix is likely correct, but I'd like to see a regression test here.
This comment was marked as outdated.
This comment was marked as outdated.
|
Potential solution? Perhaps there is a cleaner way... but this works without making too many changes. |
david-allison
left a comment
There was a problem hiding this comment.
Thanks! Do you want me to submit the issue with repairCollection, or do you want to submit a separate PR.
That fix looked good
david-allison
left a comment
There was a problem hiding this comment.
OK, this looks good. Needs:
- automated test (ideally with a corrupt collection, rather than just throwing the exception)
- correcting the
catchblock - setting
databaseCorruptFlag
Ensures `launchCatchingTask` and `InitialActivity` handle `SQLiteDatabaseCorruptException` with correct dialog
19273c7 to
44949e5
Compare
|
Found out there was another area that needs to handle the exception in |
david-allison
left a comment
There was a problem hiding this comment.
One query on the test, the rest looks good to go
You created a file, deleteOnExit() for normal handling, and deleting it manually in the teardown method seems appropriate
AnkiDroid/src/androidTest/java/com/ichi2/anki/dialogs/DatabaseCorruptDialogTest.kt
Outdated
Show resolved
Hide resolved
44949e5 to
74accdc
Compare
Maybe good to add |
74accdc to
3c7be78
Compare
Thank you! |
This avoids a
SQLiteDatabaseCorruptExceptionescaping and ensures the correct dialog is displayed rather than a generic error dialogPurpose / Description
Accessing the collection with methods like
CollectionManger::withColorCollectionManager::getColUnsafewill attempt to open the database. When the file is corrupt this throws aSQLiteDatabaseCorruptException. InDeckPicker,withColthrows the error which is caught by alaunchCatchingTaskand a generic error dialog is displayed rather than the intended one.Fixes
DIALOG_LOAD_FAILED:SqliteFailure;DatabaseCorrupt;database disk image is malformed#19092Approach
Rather than accessing the collection object directly, to avoid opening the database the collection file is accessed from
CollectionManager, andBackupManager::repairCollectionwas changed slightly to accommodate this.Note that the logic in
repairCollectiondoes not guarantee the dialog will always appear even if the file is still corrupt (in my testing at least). For example ifreturn repairedFile.renameTo(colFile)succeeds and returns true, no dialog message will appear but the database could still be corrupt and prevent the decks from loading.I am not sure what the course of action is here, maybe a stronger condition for
repairCollection?How Has This Been Tested?
I am not sure if there is a better way to do this, there probably is. I couldn't find an integration test that corrupts the database, just unit tests (if there isn't maybe this is something worth having).
I copied
Anki-Android-Backend/rsdroid-instrumented/src/androidTest/assets/initial_version_2_12_1_corrupt_regular.anki2into
AnkiDroid/src/main/assets/.This file is a corrupt collection used for a backend unit test. Then I modified
DeckPicker::handleStartuptemporarily for testing:private fun handleStartup() { val context = AnkiDroidApp.instance val environment: AnkiDroidEnvironment = object : AnkiDroidEnvironment { private val folder = selectAnkiDroidFolder(context) override fun hasRequiredPermissions(): Boolean = folder.hasRequiredPermissions(context) override val requiredPermissions: PermissionSet get() = folder.permissionSet override fun initializeAnkiDroidFolder(): Boolean = CollectionHelper.isCurrentAnkiDroidDirAccessible(context) } + val dir = getCurrentAnkiDroidDirectory(context) + val targetDb = File(dir, "collection.anki2") + this.assets.open("initial_version_2_12_1_corrupt_regular.anki2").use { input -> + targetDb.outputStream().use { output -> + input.copyTo(output) + } + } viewModel.handleStartup(environment = environment) }and temporarily modified
BackupManager::repairCollectionto always return false to guarantee the dialog would appear during testing.Screencast.from.12-05-2025.05.57.47.AM.webm
Learning (optional, can help others)
Describe the research stage
Links to blog posts, patterns, libraries or addons used to solve this problem
Checklist
Please, go through these checks before submitting the PR.