feat(reminders): ReviewReminder schema migration by ericli3690 · Pull Request #18856 · ankidroid/Anki-Android

ericli3690 · 2025-07-12T05:52:00Z

Purpose / Description

Added review reminder schema migration handling. If a future developer updates ReviewReminder, so long as they also update the schema migration code, data should be transferred safely on all user devices from the old schema to the new system. Without this code, that data transfer would result in SerializationExceptions and IllegalArgumentExceptions.

Changes, grouped by file:

ReviewReminder:

Fixed up a docstring explaining the migration process.
Made the ReviewReminder class implement the ReviewReminderSchema interface. This allows it to also be stored in the oldReviewReminderSchemasForMigration map. This also means ID now needs to become an override field; see ReviewReminderSchema in ReviewReminderMigrationSettings.

ReviewRemindersDatabase:

Converted it into an object. There was no need for it to be a class, I was always using val database = ReviewRemindersDatabase() everywhere anyways.
Added a StoredReviewRemindersMap data class. This is a wrapper around a serialized map of review reminder IDs to review reminders. Previously, these maps were written to SharedPreferences in single chunks; now, we serialize the map to a string and store it as a part of a StoredReviewReminderMap. The StoredReviewReminderMap has a version field, which allows us to determine which schema to use to deserialize the JSON string field of the StoredReviewReminderMap.
schemaVersion and oldReviewReminderSchemasForMigration. These hold the current and past review reminder schemas.
Includes a SchemaVersion inline value class.
Includes the ReviewReminderSchema interface. All schemas, both old and current, implement this interface. Objects implementing this interface are dynamically stored in the oldReviewReminderSchemasForMigration field so they can be used for the schema migration process. An ID is set here because it's necessary for the migration process: whenever a migration occurs from an old schema to a new schema, we need to store it in a map with the key set to the ID of the review reminder; hence all ReviewReminderSchemas must have an ID.
performSchemaMigration is the key method that performs a migration. It finds the schema version specified from a StoredReviewReminderMap in the oldReviewReminderSchemasForMigration, uses it to deserialize the JSON string of the map from review reminder IDs to review reminders, then runs the migrate method specified in the ReviewReminderSchema interface repeatedly until the current schema version is reached. That updated map is then re-written to SharedPreferences, completing the migration. This process differs slightly from my old process of performing review reminder migrations; whereas before the migration happened across all stored review reminders in SharedPreferences all at once, now we perform the migration for each individual read if an old schema is detected. I believe this makes the system more clean. It also means I can delete a bunch of the helper methods at the bottom of the file: getAllReviewReminderSharedPrefsAsMap, deleteAllReviewReminderSharedPrefs, and writeAllReviewReminderSharedPrefsAsMap. I previously only used these for the migration process and they are no longer needed, so I deleted them.
Modified decodeJson so it calls the performSchemaMigration method if it detects an old schema.

ReviewRemindersDatabaseTest:

Updated to support the fact that ReviewRemindersDatabase is now an object.
Includes a TestingReviewReminderMigrationSettings object. This serves two purposes. On one hand, it is tested by ReviewRemindersDatabaseTest to validate that the migration system works. On the other hand, it serves as an example class for future developers to read when figuring out how to use the migration framework I've constructed. See the example old schemas in this object for more details.
Added some new deserialization failure case tests to validate what happens if a StoredReviewRemindersMap is not stored correctly.
Created a new Hamcrest matcher. We need to check if, after a migration, the essential fields of the ReviewReminders are still the same. However, we don't really care if the IDs have changed, since the ID is an internal property of the data representation and not important to the user. In fact, due to the way I have constructed ReviewReminder, the only way to "edit" a ReviewReminder object is to... delete the old one and create a new one (which I think is a good thing! it increases data privacy). Hence, the migration process consumes some IDs, and the IDs are different afterwards.
Added a long and detailed test of the migration process.

ScheduleReminders:

Edited to reflect the fact that ReviewRemindersDatabase is now an object, not a class.

Fixes

For GSoC 2025: Review Reminders

Approach

We store a static list of the old schemas to attempt a migration from. As the app matures and ReviewReminder changes, more and more old schema classes can be added to this list.
Since this technique is complex, I created unit tests. However, in order to make this functionality unit-testable, I had to opt into an internal feature of the Kotlin serialization library.

How Has This Been Tested?

Tested on a physical Samsung S23, API 34.
Unit tests pass.

Checklist

You have a descriptive commit message with a short title (first line, max 50 chars).
You have commented your code, particularly in hard-to-understand areas
You have performed a self-review of your own code

david-allison · 2025-07-18T11:17:39Z

@ericli3690 Is this ready to be un-drafted?

ericli3690 · 2025-07-22T05:49:32Z

@david-allison Sorry for the delay!! Ready!

AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ScheduleRemindersTest.kt

GSoC 2025: Review Reminders - Added a `catchDatabaseExceptions` wrapper method to ScheduleReminders. By wrapping calls to the database with this function, any database errors caught fail gracefully and show an error dialog to the user. If the database access takes too long, a progress bar shows up, too. - Previously, this was a method that also included schema migration logic etc., but that's now been moved to ankidroid#18856, so it's now possible for me to move this small snippet here instead.

Copilot

Pull Request Overview

This PR implements schema migration functionality for the ReviewReminder system to handle database schema changes without user data loss. The implementation adds version tracking to stored review reminders and provides an automated migration system that can upgrade old schemas to new ones.

Key changes:

Added schema versioning system with ReviewReminderSchemaVersion and StoredReviewRemindersMap
Implemented step-by-step migration logic in ReviewRemindersDatabase.performSchemaMigration
Created comprehensive test suite covering migration scenarios and edge cases

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
ReviewRemindersDatabase.kt	Converted to object, added migration logic and schema versioning
ReviewReminderMigrationSettings.kt	New file defining schema versions and migration chains
ReviewReminder.kt	Updated to implement ReviewReminderSchema interface
ReviewRemindersDatabaseTest.kt	Enhanced tests with migration scenarios and custom matchers
ReviewReminderMigrationSettingsTest.kt	New test file validating schema version consistency

AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminderMigrationSettings.kt

AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabaseTest.kt

AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt

ericli3690 · 2025-08-04T06:06:39Z

See PR description for full changelog.

criticalAY

Looks fine thanks!

criticalAY · 2025-08-17T21:49:33Z

AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewReminderMigrationSettingsTest.kt

+@RunWith(JUnit4::class)
+class ReviewReminderMigrationSettingsTest {
+    @Test
+    fun `current schema version points to ReviewReminder`() {
+        assertThat(ReviewReminderMigrationSettings.SCHEMA_VERSION.value, equalTo(1))
+        assertThat(
+            ReviewReminderMigrationSettings.oldReviewReminderSchemasForMigration.keys
+                .last()
+                .value,
+            equalTo(1),
+        )
+        assertThat(ReviewReminderMigrationSettings.oldReviewReminderSchemasForMigration.values.last(), equalTo(ReviewReminder::class))
+    }
+}


GSoC 2025: Review Reminders - Added a `catchDatabaseExceptions` wrapper method to ScheduleReminders. By wrapping calls to the database with this function, any database errors caught fail gracefully and show an error dialog to the user. If the database access takes too long, a progress bar shows up, too. - Previously, this was a method that also included schema migration logic etc., but that's now been moved to ankidroid#18856, so it's now possible for me to move this small snippet here instead.

Other values than TimeManager may need to be set in test and reset otherwise. In particular it could be useful for ankidroid#18856. I tried to abtsract this class. As the stack always had a single value, I removed it. I don't expect that defining a child class will be useful all the time. But in this case, introducing `time` avoid changing many many files and keep the code more readble.

Arthur-Milchior

I need to go to sleep, I'll finish reviewing another time

Arthur-Milchior · 2025-08-20T23:41:01Z

AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminder.kt

- * version A -> B -> C but may sometimes jump from A -> C.
- * [ScheduleReminders.catchDatabaseExceptions] will attempt to migrate from all old schemas present in the list.
+ * so that [ReviewRemindersDatabase.performSchemaMigration] can migrate their reminders to the new schema.
+ * Use an [ReviewReminderSchema] to store the old schema and to define a method for migrating to the new schema.


"uses a reviewRereminderSchema" I think. I don't "an" is appropriate

Arthur-Milchior · 2025-08-20T23:47:21Z

AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminderMigrationSettings.kt

+ * Schema migration settings for testing purposes.
+ * Consult these as an example of how to save old schemas and define their migrate methods.
+ */
+object TestingReviewReminderMigrationSettings {


I'm sorry but I fail to understand why the Testing object is found in the main code and note the test part.

Fair enough. I was worried about that but initially decided against it because I was uncomfortable with making the schema version visible for testing. Since you advise it, though, I've gone ahead and done that instead. Thanks!

Arthur-Milchior · 2025-08-20T23:47:54Z

AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminderMigrationSettings.kt

+
+/**
+ * Schema migration settings for testing purposes.
+ * Consult these as an example of how to save old schemas and define their migrate methods.


"these" seems to be plural. I don't understand what this refer to. There is a single object below.

Arthur-Milchior · 2025-08-20T23:48:57Z

AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminderMigrationSettings.kt

+ * When [ReviewReminder] is updated by a developer, implement this interface in a new data class which
+ * has the same fields as the old version of [ReviewReminder], then implement the [migrate] method which
+ * transforms old [ReviewReminder]s to new [ReviewReminder]s. Also ensure that the previous [ReviewReminderSchema]
+ * in the migration version chain ([ReviewRemindersDatabase.oldReviewReminderSchemasForMigration]) has its migrate method


Here and above, you should add [migrate] to indicate it's the name of the method. I am confused about why you didn't do it when you use those brackets a lot in this file

Arthur-Milchior · 2025-08-20T23:51:09Z

AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt

+     *
+     * We assume that the version is accurate; e.x. if the version is 3, then the [ReviewReminder] stored is indeed
+     * of schema version 3. This should be safe to assume since writing this data class to SharedPreferences is an
+     * atomic operation: i.e., it is written all at once.


I am not certain that it's worth explaining what atomic mean. Hopefuly, the word is clear enough. It's a case where, if the reader don't know it, looking it up maybe better than looking at your small explanation.

Removed the unnecessary explanation.

Arthur-Milchior · 2025-08-21T00:32:17Z

AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt

+     * @see [ReviewReminderMigrationSettings.SCHEMA_VERSION]
+     */
+    val SCHEMA_VERSION =
+        if (isRobolectric) {


To be frank, I'm not a fan of having test code in non test folder if we can avoid it.

You can look at TimeManager to see how it's done. Even if honestly, I don't perfectly understand why it uses a stack.

I feel like it would be better to have a variable, visible for testing, and set it in tests when you need it.

As mentioned above, I agree and have now marked the fields as visible for testing instead. Thanks!

GSoC 2025: Review Reminders - Added a `catchDatabaseExceptions` wrapper method to ScheduleReminders. By wrapping calls to the database with this function, any database errors caught fail gracefully and show an error dialog to the user. If the database access takes too long, a progress bar shows up, too. - Previously, this was a method that also included schema migration logic etc., but that's now been moved to ankidroid#18856, so it's now possible for me to move this small snippet here instead.

Arthur-Milchior · 2025-08-23T23:42:35Z

Acttually I don't see anything else to comment heer. Please ping me on discord when my request are done

GSoC 2025: Review Reminders - Added a `catchDatabaseExceptions` wrapper method to ScheduleReminders. By wrapping calls to the database with this function, any database errors caught fail gracefully and show an error dialog to the user. If the database access takes too long, a progress bar shows up, too. - Previously, this was a method that also included schema migration logic etc., but that's now been moved to #18856, so it's now possible for me to move this small snippet here instead.

GSoC 2025: Review Reminders Added review reminder schema migration handling. If a future developer updates `ReviewReminder`, so long as they also update the schema migration code, data should be transferred safely on all user devices from the old schema to the new system. Without this code, that data transfer would result in SerializationExceptions and IllegalArgumentExceptions. **Changes, grouped by file:** ReviewReminder: - Fixed up a docstring explaining the migration process. - Made the ReviewReminder class implement the ReviewReminderSchema interface. This allows it to also be stored in the oldReviewReminderSchemasForMigration map. This also means ID now needs to become an override field; see ReviewReminderSchema in ReviewReminderMigrationSettings. ReviewRemindersDatabase: - Converted it into an object. There was no need for it to be a class, I was always using `val database = ReviewRemindersDatabase()` everywhere anyways. - Added a `StoredReviewRemindersMap` data class. This is a wrapper around a serialized map of review reminder IDs to review reminders. Previously, these maps were written to SharedPreferences in single chunks; now, we serialize the map to a string and store it as a part of a `StoredReviewReminderMap`. The `StoredReviewReminderMap` has a version field, which allows us to determine which schema to use to deserialize the JSON string field of the `StoredReviewReminderMap`. - `schemaVersion` and `oldReviewReminderSchemasForMigration`. These hold the current and past review reminder schemas. - Includes a SchemaVersion inline value class. - Includes the ReviewReminderSchema interface. All schemas, both old and current, implement this interface. Objects implementing this interface are dynamically stored in the oldReviewReminderSchemasForMigration field so they can be used for the schema migration process. An ID is set here because it's necessary for the migration process: whenever a migration occurs from an old schema to a new schema, we need to store it in a map with the key set to the ID of the review reminder; hence all ReviewReminderSchemas must have an ID. - `performSchemaMigration` is the key method that performs a migration. It finds the schema version specified from a `StoredReviewReminderMap` in the `oldReviewReminderSchemasForMigration`, uses it to deserialize the JSON string of the map from review reminder IDs to review reminders, then runs the `migrate` method specified in the `ReviewReminderSchema` interface repeatedly until the current schema version is reached. That updated map is then re-written to SharedPreferences, completing the migration. This process differs slightly from my old process of performing review reminder migrations; whereas before the migration happened across all stored review reminders in SharedPreferences all at once, now we perform the migration for each individual read if an old schema is detected. I believe this makes the system more clean. It also means I can delete a bunch of the helper methods at the bottom of the file: `getAllReviewReminderSharedPrefsAsMap`, `deleteAllReviewReminderSharedPrefs`, and `writeAllReviewReminderSharedPrefsAsMap`. I previously only used these for the migration process and they are no longer needed, so I deleted them. - Modified `decodeJson` so it calls the `performSchemaMigration` method if it detects an old schema. ReviewRemindersDatabaseTest: - Updated to support the fact that ReviewRemindersDatabase is now an object. - Includes a TestingReviewReminderMigrationSettings object. This serves two purposes. On one hand, it is tested by ReviewRemindersDatabaseTest to validate that the migration system works. On the other hand, it serves as an example class for future developers to read when figuring out how to use the migration framework I've constructed. See the example old schemas in this object for more details. - Added some new deserialization failure case tests to validate what happens if a StoredReviewRemindersMap is not stored correctly. - Created a new Hamcrest matcher. We need to check if, after a migration, the essential fields of the ReviewReminders are still the same. However, we don't really care if the IDs have changed, since the ID is an internal property of the data representation and not important to the user. In fact, due to the way I have constructed ReviewReminder, the only way to "edit" a ReviewReminder object is to... delete the old one and create a new one (which I think is a good thing! it increases data privacy). Hence, the migration process consumes some IDs, and the IDs are different afterwards. - Added a long and detailed test of the migration process. ScheduleReminders: - Edited to reflect the fact that ReviewRemindersDatabase is now an object, not a class.

Arthur-Milchior · 2025-08-26T22:01:25Z

AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminder.kt

    }
+
+    /**
+     * This is the up-to-date schema, we cannot migrate to a newer version.


I'd have phrased it as "there is no new version to migrate to". I feel like the phrasing here state that it's not possible to do it. The truth is that it would not even make sense to request it

Hm, fair enough. Since it's already merged, though, I'll leave it as is. Feel free to open a PR if you'd like! Thanks

ericli3690 added Needs Review GSoC Pull requests authored by a Google Summer of Code participant [Candidate/Selected], for GSoC mentors and removed Needs Review labels Jul 12, 2025

ericli3690 marked this pull request as draft July 13, 2025 01:09

This comment was marked as resolved.

Sign in to view

david-allison added Blocked by dependency Currently blocked by some other dependent / related change and removed Blocked by dependency Currently blocked by some other dependent / related change labels Jul 14, 2025

david-allison closed this Jul 18, 2025

david-allison reopened this Jul 18, 2025

ericli3690 force-pushed the ericli3690-review-reminders-schema-migration-in-schedule-reminders branch from 795f444 to ff9031f Compare July 22, 2025 05:28

ericli3690 requested a review from Copilot July 22, 2025 05:29

This comment was marked as outdated.

Sign in to view

ericli3690 force-pushed the ericli3690-review-reminders-schema-migration-in-schedule-reminders branch from ff9031f to 6198c94 Compare July 22, 2025 05:48

ericli3690 marked this pull request as ready for review July 22, 2025 05:49

ericli3690 added the Needs Review label Jul 22, 2025

sanjaysargam reviewed Jul 22, 2025

View reviewed changes

AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ScheduleRemindersTest.kt Outdated Show resolved Hide resolved

ericli3690 marked this pull request as draft July 28, 2025 03:31

ericli3690 removed the Needs Review label Jul 28, 2025

ericli3690 force-pushed the ericli3690-review-reminders-schema-migration-in-schedule-reminders branch from 6198c94 to ee8de53 Compare August 4, 2025 05:32

ericli3690 mentioned this pull request Aug 4, 2025

feat(reminders): review reminders RecyclerView #18964

Merged

3 tasks

ericli3690 force-pushed the ericli3690-review-reminders-schema-migration-in-schedule-reminders branch from ee8de53 to a21a1e3 Compare August 4, 2025 05:41

ericli3690 requested a review from Copilot August 4, 2025 05:41

Copilot AI reviewed Aug 4, 2025

View reviewed changes

ericli3690 force-pushed the ericli3690-review-reminders-schema-migration-in-schedule-reminders branch from a21a1e3 to 3566f0c Compare August 4, 2025 05:49

ericli3690 marked this pull request as ready for review August 4, 2025 06:06

ericli3690 added the Needs Review label Aug 4, 2025

criticalAY added the Needs Author Reply Waiting for a reply from the original author label Aug 6, 2025

ericli3690 added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Aug 7, 2025

ericli3690 requested a review from criticalAY August 7, 2025 02:53

ericli3690 mentioned this pull request Aug 9, 2025

feat(reminders): handle deck deletion #19065

Merged

5 tasks

criticalAY approved these changes Aug 17, 2025

View reviewed changes

criticalAY added the Needs Second Approval Has one approval, one more approval to merge label Aug 17, 2025

ericli3690 removed the Needs Review label Aug 19, 2025

Arthur-Milchior reviewed Aug 21, 2025

View reviewed changes

Arthur-Milchior mentioned this pull request Aug 21, 2025

Abstract TimeManager to create reusable resettable variable #19113

Closed

ericli3690 mentioned this pull request Aug 21, 2025

feat(reminders): AlarmManagerService and NotificationService #19118

Merged

3 tasks

ericli3690 mentioned this pull request Aug 21, 2025

feat(reminders): set alarms from ScheduleReminders #19119

Closed

3 tasks

ericli3690 marked this pull request as draft August 26, 2025 04:27

ericli3690 force-pushed the ericli3690-review-reminders-schema-migration-in-schedule-reminders branch from 3566f0c to bce77f7 Compare August 26, 2025 20:44

ericli3690 marked this pull request as ready for review August 26, 2025 21:00

ericli3690 requested a review from Arthur-Milchior August 26, 2025 21:00

Arthur-Milchior approved these changes Aug 26, 2025

View reviewed changes

Arthur-Milchior added this pull request to the merge queue Aug 26, 2025

Merged via the queue into ankidroid:main with commit 27fd44c Aug 26, 2025
10 checks passed

github-actions bot added this to the 2.23 release milestone Aug 26, 2025

github-actions bot removed the Needs Second Approval Has one approval, one more approval to merge label Aug 26, 2025

Uh oh!

Comments

Conversation

ericli3690 commented Jul 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose / Description

Fixes

Approach

How Has This Been Tested?

Checklist

Uh oh!

This comment was marked as resolved.

david-allison commented Jul 18, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

ericli3690 commented Jul 22, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ericli3690 commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

criticalAY left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Arthur-Milchior left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Arthur-Milchior commented Aug 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ericli3690 commented Jul 12, 2025 •

edited

Loading

ericli3690 commented Aug 4, 2025 •

edited

Loading