diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoTest.kt index 0d875d9..fdb9956 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoTest.kt @@ -166,6 +166,41 @@ class MessageDaoTest { ) } + @Test + fun browsePagingBreaksTimestampTiesByAscendingIdForATotalPageOrder() = runBlocking { + // Bulk mail can share a timestamp (issue #311): without a unique tiebreaker, rows tied at a + // LIMIT/OFFSET page boundary can duplicate or skip. Insertion order is scrambled so the ORDER BY + // — not the storage order — must produce the result. + dao.insertNew( + listOf( + message("tie-c", timestampMillis = 1_000), + message("tie-a", timestampMillis = 1_000), + message("tie-b", timestampMillis = 1_000), + message("newer", timestampMillis = 2_000), + ), + ) + + // Newest timestamp first, then ties broken by ascending id — a deterministic total order. + val expected = listOf("newer", "tie-a", "tie-b", "tie-c") + assertEquals(expected, dao.pagingUnifiedFolderSummaries("INBOX").refreshIds()) + assertEquals(expected, dao.pagingFolderSummaries("acct", "INBOX").refreshIds()) + } + + @Test + fun searchPagingBreaksTimestampTiesByAscendingIdForATotalPageOrder() = runBlocking { + dao.insertNew( + listOf( + message("hit-c", subject = "report", timestampMillis = 1_000), + message("hit-a", subject = "report", timestampMillis = 1_000), + message("hit-b", subject = "report", timestampMillis = 1_000), + ), + ) + + val expected = listOf("hit-a", "hit-b", "hit-c") + assertEquals(expected, dao.pagingUnifiedFolderSearchSummaries("INBOX", "%report%").refreshIds()) + assertEquals(expected, dao.pagingFolderSearchSummaries("acct", "INBOX", "%report%").refreshIds()) + } + @Test fun getUnfetchedIdsReturnsOnlySyncedRowsMissingABody() = runBlocking { dao.insertNew( @@ -232,6 +267,59 @@ class MessageDaoTest { assertEquals(true, row.inInbox) } + @Test + fun updateHeaderContentsRefreshesEveryRowInTheBatchAndLeavesFlagsAndBodiesUntouched() = runBlocking { + dao.insertNew( + listOf( + message("acct:1", isRead = true, isStarred = false, body = "cached-1", uid = 0), + message("acct:2", isRead = false, isStarred = true, body = "cached-2", uid = 0), + ), + ) + + // The sync path (issue #310) refreshes a whole recent window at once via this single-transaction + // batch. Only the six header fields of each passed entity are applied; the rest are ignored. + dao.updateHeaderContents( + listOf( + message( + "acct:1", + sender = "Charles", + senderEmail = "charles@example.org", + subject = "One", + timestampMillis = 5_000L, + uid = 42L, + ), + message( + "acct:2", + sender = "Grace", + senderEmail = "grace@example.org", + subject = "Two", + timestampMillis = 6_000L, + uid = 43L, + ), + ), + ) + + val one = requireNotNull(dao.getById("acct:1")) + assertEquals("Charles", one.sender) + assertEquals("charles@example.org", one.senderEmail) + assertEquals("One", one.subject) + assertEquals(5_000L, one.timestampMillis) + assertEquals(42L, one.uid) + // The casefold search columns track the refreshed headers (issue #232). + assertEquals("charles", one.senderFold) + assertEquals("one", one.subjectFold) + // Flags and the cached body are deliberately left untouched. + assertTrue(one.isRead) + assertEquals("cached-1", one.body) + + val two = requireNotNull(dao.getById("acct:2")) + assertEquals("Grace", two.sender) + assertEquals("Two", two.subject) + assertEquals(43L, two.uid) + assertTrue(two.isStarred) + assertEquals("cached-2", two.body) + } + @Test fun markSyncedPromotesSearchOnlyRowsIntoTheFolder() = runBlocking { dao.insertNew( diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/MigrationTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/MigrationTest.kt index 4406100..95803c7 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/MigrationTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/MigrationTest.kt @@ -13,6 +13,7 @@ import org.junit.Assert.assertTrue import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith +import org.libremail.di.DatabaseModule import java.lang.reflect.Modifier /** @@ -306,6 +307,23 @@ class MigrationTest { ) } + /** + * The replay tests discover migrations reflectively, but nothing there checks that [DatabaseModule] + * actually *registers* them. With no destructive fallback, a migration authored, schema-committed, + * and replay-tested but omitted from `addMigrations` still crash-loops every upgrading user at first + * database open (issue #312). Assert the builder's registered set ([DatabaseModule.ALL_MIGRATIONS]) + * is exactly the reflectively-discovered set, so such an omission fails here instead. + */ + @Test + fun databaseModuleRegistersEveryDeclaredMigration() { + assertEquals( + "DatabaseModule.ALL_MIGRATIONS must register every Migration in Migrations.kt " + + "(no destructive fallback, so a forgotten step crash-loops upgrades)", + allAppMigrations.map { it.startVersion to it.endVersion }, + DatabaseModule.ALL_MIGRATIONS.sortedBy { it.startVersion }.map { it.startVersion to it.endVersion }, + ) + } + /** * Creates a database at v7 (the oldest exported schema), fills it like a used install, then * replays every migration one step at a time — `runMigrationsAndValidate` diffs the migrated diff --git a/app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt b/app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt index 23e3db7..97546cb 100644 --- a/app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt +++ b/app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt @@ -6,6 +6,7 @@ import androidx.room.Dao import androidx.room.Insert import androidx.room.OnConflictStrategy import androidx.room.Query +import androidx.room.Transaction import kotlinx.coroutines.flow.Flow import org.libremail.data.local.entity.FolderUnreadCount import org.libremail.data.local.entity.MessageEntity @@ -36,11 +37,14 @@ interface MessageDao { * by [pagingUnifiedFolderSearchSummaries] (issue #214). Profiling (see * `docs/perf/issue-124-unified-inbox-paging.md`) showed the first page loads flat regardless of * total cache size on the existing indices, so no `(folder, …)` index / schema migration is added. + * Breaks ties on the `id` primary key (issue #311): `timestampMillis` isn't unique — bulk mail + * shares a second — so without a unique tiebreaker two rows tied at a LIMIT/OFFSET page boundary + * could duplicate or skip across pages. `id` is in the projection, so the tiebreaker adds no index. */ @Query( "SELECT id, accountId, sender, senderEmail, subject, snippet, timestampMillis, " + "isRead, isStarred, folder, inInbox, bodyFetched FROM messages " + - "WHERE folder = :folder AND inInbox = 1 ORDER BY timestampMillis DESC", + "WHERE folder = :folder AND inInbox = 1 ORDER BY timestampMillis DESC, id", ) fun pagingUnifiedFolderSummaries(folder: String): PagingSource @@ -56,7 +60,7 @@ interface MessageDao { @Query( "SELECT id, accountId, sender, senderEmail, subject, snippet, timestampMillis, " + "isRead, isStarred, folder, inInbox, bodyFetched FROM messages " + - "WHERE accountId = :accountId AND folder = :folder AND inInbox = 1 ORDER BY timestampMillis DESC", + "WHERE accountId = :accountId AND folder = :folder AND inInbox = 1 ORDER BY timestampMillis DESC, id", ) fun pagingFolderSummaries(accountId: String, folder: String): PagingSource @@ -70,13 +74,14 @@ interface MessageDao { * server-search hits `MailRepository.searchServer` inserts — exactly what the old filter saw. * Matches the Unicode-casefolded `*Fold` columns (issue #232) with a pattern built from the * lowercased query, so search is case-insensitive beyond ASCII — unlike the old ASCII-only `LIKE`. + * Breaks ties on the `id` primary key for a total page order, like the browse pagers (issue #311). */ @Query( "SELECT id, accountId, sender, senderEmail, subject, snippet, timestampMillis, " + "isRead, isStarred, folder, inInbox, bodyFetched FROM messages " + "WHERE folder = :folder AND (senderFold LIKE :pattern ESCAPE '\\' OR " + "senderEmailFold LIKE :pattern ESCAPE '\\' OR subjectFold LIKE :pattern ESCAPE '\\' OR " + - "snippetFold LIKE :pattern ESCAPE '\\') ORDER BY timestampMillis DESC", + "snippetFold LIKE :pattern ESCAPE '\\') ORDER BY timestampMillis DESC, id", ) fun pagingUnifiedFolderSearchSummaries(folder: String, pattern: String): PagingSource @@ -91,7 +96,7 @@ interface MessageDao { "isRead, isStarred, folder, inInbox, bodyFetched FROM messages " + "WHERE accountId = :accountId AND folder = :folder AND (senderFold LIKE :pattern ESCAPE '\\' OR " + "senderEmailFold LIKE :pattern ESCAPE '\\' OR subjectFold LIKE :pattern ESCAPE '\\' OR " + - "snippetFold LIKE :pattern ESCAPE '\\') ORDER BY timestampMillis DESC", + "snippetFold LIKE :pattern ESCAPE '\\') ORDER BY timestampMillis DESC, id", ) fun pagingFolderSearchSummaries( accountId: String, @@ -190,6 +195,28 @@ interface MessageDao { uid: Long, ) + /** + * Applies [updateHeaderContent] to every [messages] row in a single transaction (issue #310). A + * foreground sync / pull-to-refresh refreshes a whole recent window at once; running each row's + * UPDATE in its own implicit transaction fsyncs the journal once per message — N commits per folder + * per sync, amplified on the encrypted cache — so this collapses them into one commit. Refreshes the + * same display fields (plus the materialized [MessageEntity.uid] and the casefold search columns) as + * the single-row overload, still leaving cached bodies and optimistic read/star flags untouched. + */ + @Transaction + suspend fun updateHeaderContents(messages: List) { + for (message in messages) { + updateHeaderContent( + id = message.id, + sender = message.sender, + senderEmail = message.senderEmail, + subject = message.subject, + timestampMillis = message.timestampMillis, + uid = message.uid, + ) + } + } + /** Marks rows as folder-synced (e.g. a former search-only row that the sync now returns). */ @Query("UPDATE messages SET inInbox = 1 WHERE id IN (:ids)") suspend fun markSynced(ids: List) diff --git a/app/src/main/kotlin/org/libremail/data/sync/MailSyncer.kt b/app/src/main/kotlin/org/libremail/data/sync/MailSyncer.kt index e701ae9..3b63219 100644 --- a/app/src/main/kotlin/org/libremail/data/sync/MailSyncer.kt +++ b/app/src/main/kotlin/org/libremail/data/sync/MailSyncer.kt @@ -122,18 +122,11 @@ class MailSyncer @Inject constructor( val ids = entities.map { it.id } messageDao.insertNew(entities) // Mark every fetched message as synced (upgrades any former search-only row) and refresh - // its display fields — without touching cached bodies or optimistic read/star flags. + // its display fields — without touching cached bodies or optimistic read/star flags. The + // per-row refreshes run in a single transaction (issue #310) so a whole recent window + // costs one commit instead of one fsync per message (amplified on the encrypted cache). messageDao.markSynced(ids) - entities.forEach { - messageDao.updateHeaderContent( - id = it.id, - sender = it.sender, - senderEmail = it.senderEmail, - subject = it.subject, - timestampMillis = it.timestampMillis, - uid = it.uid, - ) - } + messageDao.updateHeaderContents(entities) // Reconcile server-side deletions ONLY within the fetched recent-UID window, so older // history paged in by the background backfill (issue #12) survives each foreground sync // instead of being wiped by a whole-folder "not in the recent 50" delete. Bound the diff --git a/app/src/main/kotlin/org/libremail/di/DatabaseModule.kt b/app/src/main/kotlin/org/libremail/di/DatabaseModule.kt index 2c69a34..367ae82 100644 --- a/app/src/main/kotlin/org/libremail/di/DatabaseModule.kt +++ b/app/src/main/kotlin/org/libremail/di/DatabaseModule.kt @@ -3,6 +3,7 @@ package org.libremail.di import android.content.Context import androidx.room.Room +import androidx.room.migration.Migration import androidx.sqlite.db.framework.FrameworkSQLiteOpenHelperFactory import dagger.Module import dagger.Provides @@ -46,30 +47,39 @@ import javax.inject.Singleton @InstallIn(SingletonComponent::class) object DatabaseModule { + /** + * Every migration the Room builder registers, in one named list so [provideDatabase] and the + * "registered == declared" safety-net test (issue #312) read the same source. There is deliberately + * no destructive fallback (see below), so a migration authored + schema-committed but forgotten here + * passes every replay test yet crash-loops ALL upgrading users at first DB open; `MigrationTest` + * asserts this list equals the reflectively-discovered set of every `Migration` val in Migrations.kt. + */ + val ALL_MIGRATIONS: Array = arrayOf( + MIGRATION_1_2, + MIGRATION_2_3, + MIGRATION_3_4, + MIGRATION_4_5, + MIGRATION_5_6, + MIGRATION_6_7, + MIGRATION_7_8, + MIGRATION_8_9, + MIGRATION_9_10, + MIGRATION_10_11, + MIGRATION_11_12, + MIGRATION_12_13, + MIGRATION_13_14, + MIGRATION_14_15, + MIGRATION_15_16, + MIGRATION_16_17, + MIGRATION_17_18, + MIGRATION_18_19, + ) + @Provides @Singleton fun provideDatabase(@ApplicationContext context: Context, provisioner: DatabaseProvisioner): LibreMailDatabase = Room.databaseBuilder(context, LibreMailDatabase::class.java, DB_NAME) - .addMigrations( - MIGRATION_1_2, - MIGRATION_2_3, - MIGRATION_3_4, - MIGRATION_4_5, - MIGRATION_5_6, - MIGRATION_6_7, - MIGRATION_7_8, - MIGRATION_8_9, - MIGRATION_9_10, - MIGRATION_10_11, - MIGRATION_11_12, - MIGRATION_12_13, - MIGRATION_13_14, - MIGRATION_14_15, - MIGRATION_15_16, - MIGRATION_16_17, - MIGRATION_17_18, - MIGRATION_18_19, - ) + .addMigrations(*ALL_MIGRATIONS) // No destructive fallback: the migration chain is complete, and silently dropping the // mail/message tables would lose cached data. A missing migration should fail loudly in // testing instead. diff --git a/app/src/test/kotlin/org/libremail/data/sync/MailSyncConcurrencyTest.kt b/app/src/test/kotlin/org/libremail/data/sync/MailSyncConcurrencyTest.kt index 2bd2fc7..efe7d66 100644 --- a/app/src/test/kotlin/org/libremail/data/sync/MailSyncConcurrencyTest.kt +++ b/app/src/test/kotlin/org/libremail/data/sync/MailSyncConcurrencyTest.kt @@ -410,6 +410,11 @@ class MailSyncConcurrencyTest { coEvery { dao.updateHeaderContent(any(), any(), any(), any(), any(), any()) } answers { store.updateHeaderContent(firstArg(), secondArg(), thirdArg(), arg(3), arg(4), arg(5)) } + coEvery { dao.updateHeaderContents(any()) } answers { + firstArg>().forEach { + store.updateHeaderContent(it.id, it.sender, it.senderEmail, it.subject, it.timestampMillis, it.uid) + } + } coEvery { dao.deleteByIds(any()) } answers { store.deleteByIds(firstArg()) } coEvery { dao.deleteSyncedByAccountFolder(any(), any()) } answers { store.deleteSyncedByAccountFolder(firstArg(), secondArg())