refactor(data): tighten data-core atomicity, chunking, and dead code (#313)

Addresses the below-cut data-core review nits from #313:

- SignatureRepository.delete: wrap delete + default-promotion in one SignatureDao
  @Transaction (deletePromotingDefault) so a crash between them can't leave an
  account with signatures but no default; log the promotion (PII-free).
- SignatureRepository.create: move the count-then-default check-then-act into a
  SignatureDao @Transaction (insertMakingFirstDefault) so two concurrent
  first-creates can't both become default.
- AccountSettingsRepository.update: route the read-modify-write through an
  AccountSettingsDao @Transaction (readModifyWrite) so concurrent per-field
  setters can't clobber each other.
- MailRepositoryImpl expunge/move/move-by-role: chunk the unbounded
  getRoutingByIds/deleteByIds IN(:ids) queries (500/chunk) like MailPruner,
  removing the latent SQLITE_MAX_VARIABLE_NUMBER crash.
- MessageDao.observeSummaries: remove the dead whole-table projection (superseded
  by Paging #124/#214); migrate test/debug-probe callers to getById or the paged
  query (which now guards the #51 CursorWindow regression).
- AccountDataMigrator: fix stale KDoc (schema is v2 with sortOrder, not v1).
- DatabaseEncryption.migrate: also sweep the stale -journal sidecar (journal_mode
  = DELETE), matching AccountDataMigrator's sweep.

Unit tests updated for the repository delegations; instrumented DAO tests cover
the new @Transaction behaviour; MailRepositoryImplTest covers the chunk split;
DatabaseEncryptionTest covers the -journal sweep.

Closes #313
This commit is contained in:
2026-07-08 08:09:12 -05:00
parent 4d724a52a8
commit d0ed168fcb
21 changed files with 348 additions and 114 deletions
@@ -88,4 +88,29 @@ class AccountSettingsDaoTest {
assertEquals(500, stored?.retentionCount)
assertEquals(6, stored?.retentionMonths)
}
@Test
fun readModifyWriteAppliesTheTransformToTheStoredRow() = runBlocking {
insertAccount()
dao.upsert(AccountSettingsEntity("acct", signature = "old", notificationsEnabled = false))
// The read + transform + write run in one transaction (issue #313); the transform gets the stored
// row and changes one field, so the un-touched fields are carried forward.
dao.readModifyWrite("acct") { stored -> stored!!.copy(signature = "new") }
val result = dao.get("acct")
assertEquals("new", result?.signature)
assertEquals(false, result?.notificationsEnabled)
}
@Test
fun readModifyWriteTransformsANullRowForAnUnconfiguredAccount() = runBlocking {
insertAccount()
// No settings row yet: the transform receives null and builds the first row.
dao.readModifyWrite("acct") { stored ->
stored?.copy(signature = "x") ?: AccountSettingsEntity("acct", signature = "seeded")
}
assertEquals("seeded", dao.get("acct")?.signature)
}
}
@@ -5,7 +5,6 @@ import android.content.Context
import androidx.room.Room
import androidx.test.core.app.ApplicationProvider
import androidx.test.ext.junit.runners.AndroidJUnit4
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.runBlocking
import net.zetetic.database.sqlcipher.SQLiteDatabase
import net.zetetic.database.sqlcipher.SupportOpenHelperFactory
@@ -56,7 +55,7 @@ class DatabaseEncryptionTest {
DatabaseEncryption.ensureEncrypted(dbFile, passphrase)
assertTrue("file must not read as plaintext once encrypted", DatabaseEncryption.isEncrypted(dbFile))
openEncrypted().apply {
assertEquals(listOf("acct:1"), messageDao().observeSummaries().first().map { it.id })
assertEquals("acct:1", messageDao().getById("acct:1")?.id)
close()
}
@@ -64,7 +63,7 @@ class DatabaseEncryptionTest {
DatabaseEncryption.ensurePlaintext(dbFile, passphrase)
assertFalse("file must be plaintext again after decrypt", DatabaseEncryption.isEncrypted(dbFile))
openPlaintext().apply {
assertEquals(listOf("acct:1"), messageDao().observeSummaries().first().map { it.id })
assertEquals("acct:1", messageDao().getById("acct:1")?.id)
close()
}
}
@@ -105,7 +104,7 @@ class DatabaseEncryptionTest {
DatabaseEncryption.ensureEncrypted(dbFile, passphrase)
assertTrue("the file stays encrypted", DatabaseEncryption.isEncrypted(dbFile))
openEncrypted().apply {
assertEquals(listOf("acct:1"), messageDao().observeSummaries().first().map { it.id })
assertEquals("acct:1", messageDao().getById("acct:1")?.id)
close()
}
}
@@ -122,7 +121,29 @@ class DatabaseEncryptionTest {
assertFalse(DatabaseEncryption.isEncrypted(dbFile))
openPlaintext().apply {
assertEquals(listOf("acct:1"), messageDao().observeSummaries().first().map { it.id })
assertEquals("acct:1", messageDao().getById("acct:1")?.id)
close()
}
}
@Test
fun conversionSweepsAStaleRollbackJournalSidecar() = runBlocking<Unit> {
openPlaintext().apply {
messageDao().insertNew(listOf(message("acct:1")))
close()
}
// A stray `-journal` left next to the file by an interrupted rollback-journal-mode session. The
// conversion runs in journal_mode = DELETE, so `-journal` is the sidecar that can actually linger
// (the pre-existing sweep only removed `-wal`/`-shm`) — issue #313.
val staleJournal = File(dbFile.parentFile, "$dbName-journal")
staleJournal.outputStream().use { it.write(0) }
assertTrue("precondition: a stale journal exists", staleJournal.exists())
DatabaseEncryption.ensureEncrypted(dbFile, passphrase)
assertFalse("the conversion must sweep the stale -journal sidecar", staleJournal.exists())
openEncrypted().apply {
assertEquals("the data still round-trips", "acct:1", messageDao().getById("acct:1")?.id)
close()
}
}
@@ -16,7 +16,6 @@ import io.mockk.unmockkAll
import io.mockk.unmockkObject
import io.mockk.verify
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.flow.flowOf
import kotlinx.coroutines.runBlocking
import net.zetetic.database.sqlcipher.SupportOpenHelperFactory
@@ -137,7 +136,7 @@ class DatabaseProvisionerInstrumentedTest {
// The keyed open the provisioner reported must actually succeed on real SQLCipher (no crash).
openEncrypted().apply {
assertEquals(listOf("acct:1"), messageDao().observeSummaries().first().map { it.id })
assertEquals("acct:1", messageDao().getById("acct:1")?.id)
close()
}
}
@@ -164,7 +163,7 @@ class DatabaseProvisionerInstrumentedTest {
// whatever ABI / page size this device or emulator image uses.
openEncrypted().apply {
messageDao().insertNew(listOf(message("acct:1")))
assertEquals(listOf("acct:1"), messageDao().observeSummaries().first().map { it.id })
assertEquals("acct:1", messageDao().getById("acct:1")?.id)
close()
}
assertTrue("the fresh cache was created in SQLCipher (encrypted) form", DatabaseEncryption.isEncrypted(dbFile))
@@ -182,7 +181,7 @@ class DatabaseProvisionerInstrumentedTest {
assertEquals(CacheOpenMode.Plaintext, mode)
assertFalse("the cache must be decrypted so the unkeyed open works", DatabaseEncryption.isEncrypted(dbFile))
openPlaintext().apply {
assertEquals(listOf("acct:1"), messageDao().observeSummaries().first().map { it.id })
assertEquals("acct:1", messageDao().getById("acct:1")?.id)
close()
}
}
@@ -198,7 +197,7 @@ class DatabaseProvisionerInstrumentedTest {
assertEquals(CacheOpenMode.Plaintext, mode)
assertFalse("a plaintext-with-encryption-off start converts nothing", DatabaseEncryption.isEncrypted(dbFile))
openPlaintext().apply {
assertEquals(listOf("acct:1"), messageDao().observeSummaries().first().map { it.id })
assertEquals("acct:1", messageDao().getById("acct:1")?.id)
close()
}
}
@@ -2,6 +2,7 @@
package org.libremail.data.local
import android.content.Context
import androidx.paging.PagingSource
import androidx.room.Room
import androidx.test.core.app.ApplicationProvider
import androidx.test.ext.junit.runners.AndroidJUnit4
@@ -9,6 +10,7 @@ import kotlinx.coroutines.flow.first
import kotlinx.coroutines.runBlocking
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Assert.assertNull
import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Test
@@ -16,6 +18,7 @@ import org.junit.runner.RunWith
import org.libremail.data.local.entity.AttachmentEntity
import org.libremail.data.local.entity.FolderEntity
import org.libremail.data.local.entity.MessageEntity
import org.libremail.data.local.entity.MessageSummary
/**
* Schema-behavior tests on a fresh in-memory database at the current version. The migration DDL
@@ -52,6 +55,12 @@ class LibreMailDatabaseTest {
isStarred = false,
)
/** Refreshes a [PagingSource] and returns the first loaded page's ids in order. */
private suspend fun PagingSource<Int, MessageSummary>.refreshIds(loadSize: Int = 20): List<String> {
val result = load(PagingSource.LoadParams.Refresh(key = null, loadSize = loadSize, placeholdersEnabled = false))
return (result as PagingSource.LoadResult.Page).data.map { it.id }
}
@Test
fun observeUnreadCountsAggregatesUnreadSyncedRowsPerAccountAndFolder() = runBlocking {
val messageDao = db.messageDao()
@@ -124,17 +133,17 @@ class LibreMailDatabaseTest {
assertEquals(listOf("acct:1"), messageDao.getSyncedIds("acct", "INBOX"))
messageDao.deleteSearchRows()
val remaining = messageDao.observeSummaries().first().map { it.id }
assertEquals(listOf("acct:1"), remaining)
assertEquals("the synced inbox row survives", "acct:1", messageDao.getById("acct:1")?.id)
assertNull("the transient search-only row is cleared", messageDao.getById("acct:2"))
}
@Test
fun observeSummariesReadsRowsWhoseBodiesExceedTheCursorWindow() = runBlocking {
fun pagedSummariesReadRowsWhoseBodiesExceedTheCursorWindow() = runBlocking {
val messageDao = db.messageDao()
// Each body is larger than SQLite's shared (~2 MB) CursorWindow. The old list query did
// SELECT * and dragged these bodies through the window, overflowing it with
// "Couldn't read row … from CursorWindow" (issue #51). observeSummaries omits body, so the
// rows stay tiny and read fine.
// Each body is larger than SQLite's shared (~2 MB) CursorWindow. A `SELECT *` list query
// dragged these bodies through the window, overflowing it with "Couldn't read row … from
// CursorWindow" (issue #51). The paged mailbox projection omits body, so the rows stay tiny
// and read fine — asserted against the real production query (issue #124/#214).
val hugeBody = "x".repeat(3 * 1024 * 1024)
messageDao.insertNew(
listOf(
@@ -143,7 +152,7 @@ class LibreMailDatabaseTest {
),
)
val ids = messageDao.observeSummaries().first().map { it.id }.toSet()
val ids = messageDao.pagingUnifiedFolderSummaries("INBOX").refreshIds().toSet()
assertEquals(setOf("acct:1", "acct:2"), ids)
}
@@ -194,9 +203,8 @@ class LibreMailDatabaseTest {
// Reconciling the inbox must not touch other folders' rows (windowed reconcile; whole-inbox
// window since these rows have uid 0).
messageDao.deleteSyncedInWindowNotIn("acct", "INBOX", minWindowUid = 0, keepIds = listOf("acct:INBOX:1"))
assertEquals(
setOf("acct:INBOX:1", "acct:Archive:1"),
messageDao.observeSummaries().first().map { it.id }.toSet(),
)
assertEquals("the kept inbox row survives", "acct:INBOX:1", messageDao.getById("acct:INBOX:1")?.id)
assertNull("the reconciled-away inbox row is deleted", messageDao.getById("acct:INBOX:2"))
assertEquals("the other folder is untouched", "acct:Archive:1", messageDao.getById("acct:Archive:1")?.id)
}
}
@@ -6,7 +6,6 @@ import androidx.paging.PagingSource
import androidx.room.Room
import androidx.test.core.app.ApplicationProvider
import androidx.test.ext.junit.runners.AndroidJUnit4
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.runBlocking
import org.junit.After
import org.junit.Assert.assertEquals
@@ -422,7 +421,9 @@ class MessageDaoTest {
dao.deleteByIds(listOf("a", "c"))
assertEquals(listOf("b"), dao.observeSummaries().first().map { it.id })
assertNull("a is deleted", dao.getById("a"))
assertNull("c is deleted", dao.getById("c"))
assertEquals("b survives", "b", dao.getById("b")?.id)
}
@Test
@@ -437,7 +438,9 @@ class MessageDaoTest {
dao.deleteByAccount("acct")
assertEquals(listOf("acct2:1"), dao.observeSummaries().first().map { it.id })
assertNull("the account's INBOX row is deleted", dao.getById("acct:1"))
assertNull("the account's other-folder row is deleted", dao.getById("acct:Archive:1"))
assertEquals("the other account survives", "acct2:1", dao.getById("acct2:1")?.id)
}
@Test
@@ -150,4 +150,47 @@ class SignatureDaoTest {
// clearDefault in setDefault only touches the target account; acct2's default is untouched.
assertEquals("b1", dao.getDefault("acct2")?.id)
}
@Test
fun insertMakingFirstDefaultMakesOnlyTheAccountsFirstSignatureDefault() = runBlocking {
insertAccount()
// The passed isDefault is a placeholder; the transaction decides it from the current count (#313).
dao.insertMakingFirstDefault(signature("s-1", "First", isDefault = false))
dao.insertMakingFirstDefault(signature("s-2", "Second", isDefault = true))
assertEquals(true, dao.getById("s-1")?.isDefault)
assertEquals(false, dao.getById("s-2")?.isDefault)
assertEquals("s-1", dao.getDefault("acct")?.id)
}
@Test
fun deletePromotingDefaultPromotesTheFirstRemainingWhenTheDefaultIsRemoved() = runBlocking {
insertAccount()
dao.upsert(signature("s-default", "Zeta", isDefault = true))
dao.upsert(signature("s-other", "alpha")) // name-first among the remaining rows
val promoted = dao.deletePromotingDefault("s-default")
assertEquals("s-other", promoted)
assertNull("the deleted default is gone", dao.getById("s-default"))
assertEquals("the first remaining becomes default", "s-other", dao.getDefault("acct")?.id)
}
@Test
fun deletePromotingDefaultPromotesNothingForANonDefaultOrTheLastRow() = runBlocking {
insertAccount()
dao.upsert(signature("s-default", "Default", isDefault = true))
dao.upsert(signature("s-plain", "Plain"))
// Deleting a non-default leaves the account's default untouched — nothing to promote.
assertNull(dao.deletePromotingDefault("s-plain"))
assertEquals("s-default", dao.getDefault("acct")?.id)
// Deleting the last (default) signature has no remaining row to promote.
assertNull(dao.deletePromotingDefault("s-default"))
assertNull(dao.getDefault("acct"))
// A missing id is a no-op.
assertNull(dao.deletePromotingDefault("absent"))
}
}
@@ -15,7 +15,6 @@ import io.mockk.mockkObject
import io.mockk.unmockkAll
import io.mockk.verify
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.flow.flowOf
import kotlinx.coroutines.runBlocking
import org.junit.After
@@ -138,7 +137,7 @@ class DatabaseModuleInstrumentedTest {
// ever opened this genuinely-encrypted file with the plaintext framework helper instead of
// SQLCipher's, this would throw (a plaintext driver can't parse SQLCipher ciphertext) rather
// than return the seeded row.
assertEquals(listOf("acct:1"), database.messageDao().observeSummaries().first().map { it.id })
assertEquals("acct:1", database.messageDao().getById("acct:1")?.id)
} finally {
database.close()
}
@@ -157,7 +156,7 @@ class DatabaseModuleInstrumentedTest {
val database = DatabaseModule.provideDatabase(context, provisioner())
try {
database.messageDao().insertNew(listOf(message("acct:1")))
assertEquals(listOf("acct:1"), database.messageDao().observeSummaries().first().map { it.id })
assertEquals("acct:1", database.messageDao().getById("acct:1")?.id)
} finally {
database.close()
}
@@ -182,7 +181,7 @@ class DatabaseModuleInstrumentedTest {
val database = DatabaseModule.provideDatabase(context, provisioner())
try {
assertThrows(Throwable::class.java) {
runBlocking { database.messageDao().observeSummaries().first() }
runBlocking { database.messageDao().getById("acct:1") }
}
} finally {
runCatching { database.close() }
@@ -10,7 +10,6 @@ import android.os.Bundle
import androidx.room.Room
import androidx.sqlite.db.SupportSQLiteDatabase
import androidx.sqlite.db.SupportSQLiteOpenHelper
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.runBlocking
import net.zetetic.database.sqlcipher.SupportOpenHelperFactory
import org.libremail.data.local.DatabaseEncryption
@@ -112,8 +111,8 @@ class ColdOpenCacheProbe : ContentProvider() {
)
.build()
try {
val ids = runBlocking { database.messageDao().observeSummaries().first().map { it.id } }
if (ids == listOf(EXPECTED_ROW_ID)) OPEN_OK else "$OPEN_ROWS$ids"
val id = runBlocking { database.messageDao().getById(EXPECTED_ROW_ID)?.id }
if (id == EXPECTED_ROW_ID) OPEN_OK else "$OPEN_ROWS$id"
} finally {
database.close()
}
@@ -98,10 +98,11 @@ class AccountDataMigrator @Inject constructor(
private val TABLES = listOf("accounts", "credentials", "account_settings", "signatures")
/**
* DDL for the account tables in [AccountDatabase] v1, copied verbatim from the exported Room
* schema (`schemas/org.libremail.data.local.AccountDatabase/1.json`). It MUST stay byte-for-byte
* identical to what Room generates for those entities, or Room silently accepts a subtly wrong
* schema (its identity check only compares the hash it writes, not the pre-existing tables).
* DDL for the account tables in [AccountDatabase] v2, copied verbatim from the exported Room
* schema (`schemas/org.libremail.data.local.AccountDatabase/2.json` — v2 added `accounts.sortOrder`,
* issue #164). It MUST stay byte-for-byte identical to what Room generates for those entities, or
* Room silently accepts a subtly wrong schema (its identity check only compares the hash it writes,
* not the pre-existing tables).
* `AccountDataMigratorTest.migratorDdlMatchesExportedAccountDatabaseSchema` guards it against the
* exported schema; `internal` only so that test can read it.
*/
@@ -80,10 +80,14 @@ object DatabaseEncryption {
target.close()
}
// Swap the converted file into place; drop any stale WAL/SHM sidecars from either file first.
// Swap the converted file into place; drop any stale sidecars from either file first. Both files
// are in rollback-journal mode (journal_mode = DELETE), so the sidecar that can actually linger
// after an interrupted attempt is the `-journal`; the `-wal`/`-shm` deletes are belt-and-suspenders
// for a file left in WAL mode by an older build (mirrors AccountDataMigrator's sweep).
listOf(dbFile.name, tmp.name).forEach { base ->
File(dir, "$base-wal").delete()
File(dir, "$base-shm").delete()
File(dir, "$base-journal").delete()
}
if (!tmp.renameTo(dbFile)) {
tmp.copyTo(dbFile, overwrite = true)
@@ -5,6 +5,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.AccountSettingsEntity
@@ -18,4 +19,15 @@ interface AccountSettingsDao {
@Insert(onConflict = OnConflictStrategy.REPLACE)
suspend fun upsert(settings: AccountSettingsEntity)
/**
* Reads [accountId]'s row (or null when absent), applies [transform], and writes the result — all in
* one transaction so a per-field setter's read-modify-write can't interleave with a concurrent
* setter and clobber the other field (issue #313). [transform] receives the stored entity, or null
* when no row exists yet.
*/
@Transaction
suspend fun readModifyWrite(accountId: String, transform: (AccountSettingsEntity?) -> AccountSettingsEntity) {
upsert(transform(get(accountId)))
}
}
@@ -15,18 +15,6 @@ import org.libremail.data.local.entity.MessageSummary
@Dao
interface MessageDao {
/**
* Mailbox-list projection ordered newest-first. Deliberately omits the large `body`/`isHtml`
* columns: the list observes every cached message at once, and pulling full bodies through
* SQLite's shared ~2 MB CursorWindow overflows it once enough large bodies are cached
* (issue #51). Bodies are loaded lazily per-message via [getById] when a message is opened.
*/
@Query(
"SELECT id, accountId, sender, senderEmail, subject, snippet, timestampMillis, " +
"isRead, isStarred, folder, inInbox, bodyFetched FROM messages ORDER BY timestampMillis DESC",
)
fun observeSummaries(): Flow<List<MessageSummary>>
/**
* Paged unified-inbox projection: folder-synced rows of [folder] across every account,
* newest-first, as a Paging 3 [PagingSource] (issue #124). Room loads only the requested window
@@ -44,4 +44,32 @@ interface SignatureDao {
clearDefault(accountId)
markDefault(id)
}
/**
* Inserts [signature], making it the account's default when it is the account's first — the count
* and the insert run in one transaction so two concurrent first-creates can't both read "count 0"
* and both become default (issue #313). [signature]'s own `isDefault` is ignored: this method
* decides it from the current count.
*/
@Transaction
suspend fun insertMakingFirstDefault(signature: SignatureEntity) {
upsert(signature.copy(isDefault = countForAccount(signature.accountId) == 0))
}
/**
* Deletes [id] and, when it was the account's default, promotes the account's first remaining
* signature — both in one transaction so a crash between the delete and the promote can't leave an
* account with signatures but no default (issue #313). No-op when [id] is absent. Returns the id of
* the signature promoted to default, or null when nothing was promoted (id absent, the deleted row
* wasn't the default, or no signatures remain).
*/
@Transaction
suspend fun deletePromotingDefault(id: String): String? {
val existing = getById(id) ?: return null
delete(id)
if (!existing.isDefault) return null
val promoted = firstForAccount(existing.accountId) ?: return null
markDefault(promoted.id)
return promoted.id
}
}
@@ -337,16 +337,16 @@ class MailRepositoryImpl @Inject constructor(
moveByRole(ids, FolderRole.TRASH, fallbackExpunge = true)
override suspend fun expunge(ids: List<String>): Result<Unit> = runCatching {
val routings = messageDao.getRoutingByIds(ids)
messageDao.deleteByIds(ids) // optimistic
val routings = messageDao.getRoutingByIdsChunked(ids)
messageDao.deleteByIdsChunked(ids) // optimistic
forEachAccountFolder(routings) { params, folder, group ->
imapClient.deleteMessages(params, folder, group.map { uidOf(it.id) })
}
}
override suspend fun moveToFolder(ids: List<String>, destFolderFullName: String): Result<Unit> = runCatching {
val routings = messageDao.getRoutingByIds(ids)
messageDao.deleteByIds(ids) // optimistic
val routings = messageDao.getRoutingByIdsChunked(ids)
messageDao.deleteByIdsChunked(ids) // optimistic
forEachAccountFolder(routings) { params, folder, group ->
if (folder != destFolderFullName) {
imapClient.moveMessages(params, folder, group.map { uidOf(it.id) }, destFolderFullName)
@@ -393,8 +393,8 @@ class MailRepositoryImpl @Inject constructor(
*/
private suspend fun moveByRole(ids: List<String>, role: FolderRole, fallbackExpunge: Boolean): Result<Unit> =
runCatching {
val routings = messageDao.getRoutingByIds(ids)
messageDao.deleteByIds(ids) // optimistic
val routings = messageDao.getRoutingByIdsChunked(ids)
messageDao.deleteByIdsChunked(ids) // optimistic
val destByAccount = routings.map { it.accountId }.distinct()
.associateWith { resolveRoleFolder(it, role) }
forEachAccountFolder(routings) { params, folder, group ->
@@ -556,6 +556,12 @@ private const val NANOS_PER_MS = 1_000_000L
/** Rows per page for the unified inbox (issue #124) — a page is a few screenfuls of message rows. */
private const val MAILBOX_PAGE_SIZE = 40
/**
* Ids per `IN (:ids)` query in the batch move/delete/expunge paths, kept under SQLite's 999
* host-parameter limit on older Android (matches [org.libremail.data.sync.MailPruner]'s DELETE chunk).
*/
private const val SQL_IN_CHUNK = 500
/** Attempts for the background best-effort SEEN-flag push before giving up silently (issue #148). */
private const val SEEN_FLAG_PUSH_MAX_ATTEMPTS = 3
@@ -565,6 +571,20 @@ private const val SEEN_FLAG_RETRY_BACKOFF_MS = 2_000L
/** Message id is "<accountId>:<uid>"; the uid is the trailing segment. */
private fun uidOf(id: String): String = id.substringAfterLast(':')
/**
* [MessageDao.getRoutingByIds] over an arbitrarily large [ids] list, chunked so the expanded `IN (:ids)`
* never exceeds SQLite's host-parameter limit (999 on older Android). The batch move/delete/expunge
* callers are bounded by the multi-select cap today, but chunking removes the latent
* `SQLITE_MAX_VARIABLE_NUMBER` crash the same way [org.libremail.data.sync.MailPruner] does (issue #313).
*/
private suspend fun MessageDao.getRoutingByIdsChunked(ids: List<String>): List<MessageRouting> =
ids.chunked(SQL_IN_CHUNK).flatMap { getRoutingByIds(it) }
/** [MessageDao.deleteByIds] chunked under SQLite's host-parameter limit (see [getRoutingByIdsChunked]). */
private suspend fun MessageDao.deleteByIdsChunked(ids: List<String>) {
ids.chunked(SQL_IN_CHUNK).forEach { deleteByIds(it) }
}
/**
* Builds the SQL `LIKE` pattern the paged-search DAO queries take (issue #214), preserving the old
* `matchesSearch` literal-substring semantics: escape the LIKE metacharacters (`\ % _`) — the `\`
@@ -49,7 +49,14 @@ class AccountSettingsRepository @Inject constructor(private val dao: AccountSett
it.copy(retentionMonths = months?.coerceAtLeast(0))
}
private suspend inline fun update(accountId: String, transform: (AccountSettings) -> AccountSettings) {
dao.upsert(transform(get(accountId)).toEntity())
/**
* Read-modify-writes an account's settings row through the DAO's single-transaction helper so a
* concurrent per-field setter can't clobber the read-modify-write (issue #313). A missing row is
* transformed from the account's defaults, preserving the "not configured yet = defaults" contract.
*/
private suspend fun update(accountId: String, transform: (AccountSettings) -> AccountSettings) {
dao.readModifyWrite(accountId) { stored ->
transform(stored?.toDomain() ?: AccountSettings(accountId)).toEntity()
}
}
}
@@ -6,6 +6,7 @@ import kotlinx.coroutines.flow.map
import org.libremail.data.local.dao.SignatureDao
import org.libremail.data.local.entity.SignatureEntity
import org.libremail.domain.model.Signature
import org.libremail.reporting.AppLog
import java.util.UUID
import javax.inject.Inject
import javax.inject.Singleton
@@ -28,8 +29,9 @@ class SignatureRepository @Inject constructor(private val dao: SignatureDao) {
/** Creates a signature; makes it the default when it is the account's first. Returns its id. */
suspend fun create(accountId: String, name: String, html: String): String {
val id = UUID.randomUUID().toString()
val isFirst = dao.countForAccount(accountId) == 0
dao.upsert(SignatureEntity(id, accountId, name, html, isDefault = isFirst))
// The count-then-default decision runs atomically in the DAO so two concurrent first-creates
// can't both become default (issue #313); the isDefault passed here is a placeholder.
dao.insertMakingFirstDefault(SignatureEntity(id, accountId, name, html, isDefault = false))
return id
}
@@ -39,15 +41,20 @@ class SignatureRepository @Inject constructor(private val dao: SignatureDao) {
}
suspend fun delete(id: String) {
val existing = dao.getById(id) ?: return
dao.delete(id)
// If we removed the default, promote the account's first remaining signature.
if (existing.isDefault) {
dao.firstForAccount(existing.accountId)?.let { dao.markDefault(it.id) }
// Delete + promote-a-new-default run in one DAO transaction so a crash between them can't strand
// the account with signatures but no default (issue #313).
val promotedId = dao.deletePromotingDefault(id)
if (promotedId != null) {
// PII-free: no signature content, name, or account address — just the state transition.
AppLog.i(TAG, "promoted a replacement default signature after deleting the previous default")
}
}
suspend fun setDefault(accountId: String, id: String) = dao.setDefault(accountId, id)
private fun SignatureEntity.toDomain() = Signature(id, accountId, name, contentHtml, isDefault)
private companion object {
const val TAG = "LibreMailSignatures"
}
}
@@ -445,6 +445,30 @@ class MailRepositoryImplTest {
coVerify { imapClient.deleteMessages(any(), "Trash", listOf("3")) }
}
@Test
fun `expunge chunks the id queries under SQLite's host-parameter limit`() = runTest {
// 501 ids force a split: an unchunked IN (:ids) would bind 501 host parameters, latent-crashing
// near SQLite's 999 limit on older Android (issue #313). Chunked at 500 -> a 500 + 1 split.
val ids = (1..501).map { "acct:INBOX:$it" }
coEvery { messageDao.getRoutingByIds(any()) } coAnswers {
firstArg<List<String>>().map { messageRouting(it, "INBOX") }
}
coEvery { messageDao.deleteByIds(any()) } just Runs
coEvery { accountDao.getById("acct") } returns accountEntity()
coEvery { connectionFactory.imapParamsFor(any()) } returns imapParams()
val result = repository.expunge(ids)
assertTrue(result.isSuccess)
// Both the routing read and the optimistic delete run one query per <=500-id chunk.
coVerify(exactly = 1) { messageDao.getRoutingByIds(match { it.size == 500 }) }
coVerify(exactly = 1) { messageDao.getRoutingByIds(match { it.size == 1 }) }
coVerify(exactly = 1) { messageDao.deleteByIds(match { it.size == 500 }) }
coVerify(exactly = 1) { messageDao.deleteByIds(match { it.size == 1 }) }
// Chunking is only a DB concern: all 501 UIDs still reach the server EXPUNGE in one grouped call.
coVerify { imapClient.deleteMessages(any(), "INBOX", match { it.size == 501 }) }
}
@Test
fun `moveToFolder moves messages to the chosen destination`() = runTest {
val id = "acct:INBOX:11"
@@ -2,6 +2,7 @@
package org.libremail.data.settings
import app.cash.turbine.test
import io.mockk.CapturingSlot
import io.mockk.Runs
import io.mockk.coEvery
import io.mockk.coVerify
@@ -24,6 +25,20 @@ class AccountSettingsRepositoryTest {
private val dao = mockk<AccountSettingsDao>()
private val repository = AccountSettingsRepository(dao)
/**
* Stubs the DAO's single-transaction read-modify-write (issue #313) to apply the repository's
* transform against [current] (the stored row, or null) and capture the entity it would persist — so
* these setter tests still assert the transformed row directly, while AccountSettingsDaoTest covers
* the real transaction.
*/
private fun captureUpdate(current: AccountSettingsEntity?): CapturingSlot<AccountSettingsEntity> {
val saved = slot<AccountSettingsEntity>()
coEvery { dao.readModifyWrite(any(), any()) } coAnswers {
saved.captured = secondArg<(AccountSettingsEntity?) -> AccountSettingsEntity>().invoke(current)
}
return saved
}
@Test
fun `get returns defaults when no row exists`() = runTest {
coEvery { dao.get("acct") } returns null
@@ -38,10 +53,9 @@ class AccountSettingsRepositoryTest {
@Test
fun `setSignature reads, modifies, and writes the row`() = runTest {
coEvery { dao.get("acct") } returns
AccountSettingsEntity("acct", signature = "old", signatureEnabled = true, notificationsEnabled = false)
val saved = slot<AccountSettingsEntity>()
coEvery { dao.upsert(capture(saved)) } just Runs
val saved = captureUpdate(
AccountSettingsEntity("acct", signature = "old", signatureEnabled = true, notificationsEnabled = false),
)
repository.setSignature("acct", "new")
@@ -101,10 +115,9 @@ class AccountSettingsRepositoryTest {
@Test
fun `setSignatureEnabled reads, modifies, and writes the row`() = runTest {
coEvery { dao.get("acct") } returns
AccountSettingsEntity("acct", signature = "keep", signatureEnabled = true, notificationsEnabled = true)
val saved = slot<AccountSettingsEntity>()
coEvery { dao.upsert(capture(saved)) } just Runs
val saved = captureUpdate(
AccountSettingsEntity("acct", signature = "keep", signatureEnabled = true, notificationsEnabled = true),
)
repository.setSignatureEnabled("acct", false)
@@ -115,9 +128,7 @@ class AccountSettingsRepositoryTest {
@Test
fun `setNotificationsEnabled reads, modifies, and writes the row`() = runTest {
coEvery { dao.get("acct") } returns AccountSettingsEntity("acct", notificationsEnabled = true)
val saved = slot<AccountSettingsEntity>()
coEvery { dao.upsert(capture(saved)) } just Runs
val saved = captureUpdate(AccountSettingsEntity("acct", notificationsEnabled = true))
repository.setNotificationsEnabled("acct", false)
@@ -126,9 +137,7 @@ class AccountSettingsRepositoryTest {
@Test
fun `setRetentionCount clamps a negative override to zero`() = runTest {
coEvery { dao.get("acct") } returns AccountSettingsEntity("acct")
val saved = slot<AccountSettingsEntity>()
coEvery { dao.upsert(capture(saved)) } just Runs
val saved = captureUpdate(AccountSettingsEntity("acct"))
repository.setRetentionCount("acct", -5)
@@ -137,9 +146,7 @@ class AccountSettingsRepositoryTest {
@Test
fun `setRetentionCount preserves null as inherit-the-global-default`() = runTest {
coEvery { dao.get("acct") } returns AccountSettingsEntity("acct", retentionCount = 10)
val saved = slot<AccountSettingsEntity>()
coEvery { dao.upsert(capture(saved)) } just Runs
val saved = captureUpdate(AccountSettingsEntity("acct", retentionCount = 10))
repository.setRetentionCount("acct", null)
@@ -148,9 +155,7 @@ class AccountSettingsRepositoryTest {
@Test
fun `setRetentionMonths clamps a negative override to zero`() = runTest {
coEvery { dao.get("acct") } returns AccountSettingsEntity("acct")
val saved = slot<AccountSettingsEntity>()
coEvery { dao.upsert(capture(saved)) } just Runs
val saved = captureUpdate(AccountSettingsEntity("acct"))
repository.setRetentionMonths("acct", -3)
@@ -159,12 +164,23 @@ class AccountSettingsRepositoryTest {
@Test
fun `setRetentionMonths keeps a positive override as given`() = runTest {
coEvery { dao.get("acct") } returns AccountSettingsEntity("acct")
val saved = slot<AccountSettingsEntity>()
coEvery { dao.upsert(capture(saved)) } just Runs
val saved = captureUpdate(AccountSettingsEntity("acct"))
repository.setRetentionMonths("acct", 6)
assertEquals(6, saved.captured.retentionMonths)
}
@Test
fun `a setter transforms the account defaults when no row exists yet`() = runTest {
// The DAO hands the transform a null stored row for a never-configured account; the repository
// must transform from the account's defaults so the "not configured = defaults" contract holds.
val saved = captureUpdate(current = null)
repository.setSignature("acct", "first")
assertEquals("acct", saved.captured.accountId)
assertEquals("first", saved.captured.signature)
assertTrue(saved.captured.signatureEnabled, "defaults carry through the transform")
}
}
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-3.0-or-later
package org.libremail.data.settings
import android.util.Log
import app.cash.turbine.test
import io.mockk.Runs
import io.mockk.coEvery
@@ -8,12 +9,18 @@ import io.mockk.coVerify
import io.mockk.every
import io.mockk.just
import io.mockk.mockk
import io.mockk.mockkStatic
import io.mockk.slot
import io.mockk.unmockkAll
import kotlinx.coroutines.flow.flowOf
import kotlinx.coroutines.test.runTest
import org.junit.After
import org.junit.Before
import org.junit.Test
import org.libremail.data.local.dao.SignatureDao
import org.libremail.data.local.entity.SignatureEntity
import org.libremail.reporting.AppLog
import org.libremail.reporting.RingLogBuffer
import kotlin.test.assertEquals
import kotlin.test.assertFalse
import kotlin.test.assertNull
@@ -24,51 +31,70 @@ class SignatureRepositoryTest {
private val dao = mockk<SignatureDao>(relaxed = true)
private val repository = SignatureRepository(dao)
// delete() breadcrumbs a promotion via AppLog; android.util.Log is an unmocked stub in plain JVM
// tests, so mock it class-wide (mirrors MailRepositoryImplTest).
@Before
fun setUp() {
mockkStatic(Log::class)
every { Log.i(any(), any()) } returns 0
}
@After
fun tearDown() = unmockkAll()
private fun entity(id: String, isDefault: Boolean) =
SignatureEntity(id, accountId = "acct", name = "N", contentHtml = "<p>x</p>", isDefault = isDefault)
@Test
fun `the first signature for an account becomes its default`() = runTest {
coEvery { dao.countForAccount("acct") } returns 0
fun `create routes through the atomic first-default insert with the given fields`() = runTest {
// The first-becomes-default decision now lives in the DAO transaction (issue #313); the repository
// just forwards the new row (isDefault a placeholder) and returns its generated id.
val saved = slot<SignatureEntity>()
coEvery { dao.upsert(capture(saved)) } just Runs
coEvery { dao.insertMakingFirstDefault(capture(saved)) } just Runs
repository.create("acct", "Work", "<p>hi</p>")
val id = repository.create("acct", "Work", "<p>hi</p>")
assertTrue(saved.captured.isDefault)
assertEquals("acct", saved.captured.accountId)
assertEquals("Work", saved.captured.name)
assertEquals("<p>hi</p>", saved.captured.contentHtml)
assertEquals(id, saved.captured.id)
assertFalse(saved.captured.isDefault, "the DAO decides the default flag, not the repository")
}
@Test
fun `later signatures are not made default`() = runTest {
coEvery { dao.countForAccount("acct") } returns 2
val saved = slot<SignatureEntity>()
coEvery { dao.upsert(capture(saved)) } just Runs
repository.create("acct", "Personal", "<p>hey</p>")
assertFalse(saved.captured.isDefault)
}
@Test
fun `deleting the default promotes the first remaining signature`() = runTest {
coEvery { dao.getById("s1") } returns entity("s1", isDefault = true)
coEvery { dao.firstForAccount("acct") } returns entity("s2", isDefault = false)
fun `delete routes through the atomic delete-and-promote`() = runTest {
coEvery { dao.deletePromotingDefault("s1") } returns null
repository.delete("s1")
coVerify { dao.delete("s1") }
coVerify { dao.markDefault("s2") }
coVerify { dao.deletePromotingDefault("s1") }
}
@Test
fun `deleting a non-default signature promotes nothing`() = runTest {
coEvery { dao.getById("s2") } returns entity("s2", isDefault = false)
fun `deleting a default that promotes a replacement logs a breadcrumb`() = runTest {
val buffer = RingLogBuffer()
AppLog.install(buffer)
coEvery { dao.deletePromotingDefault("s1") } returns "s2"
repository.delete("s1")
assertTrue(
buffer.snapshot().any { it.message.contains("promoted a replacement default signature") },
"the promotion is recorded for a debug report",
)
}
@Test
fun `deleting without a promotion logs nothing`() = runTest {
val buffer = RingLogBuffer()
AppLog.install(buffer)
coEvery { dao.deletePromotingDefault("s2") } returns null
repository.delete("s2")
coVerify { dao.delete("s2") }
coVerify(exactly = 0) { dao.firstForAccount(any()) }
coVerify(exactly = 0) { dao.markDefault(any()) }
assertFalse(
buffer.snapshot().any { it.message.contains("promoted a replacement default") },
)
}
@Test
+3
View File
@@ -73,6 +73,9 @@ style:
# Reader-path perf logging (issue #358): the repository's openMessage and the reader ViewModel
# log via AppLog, so their unit tests mockkStatic(Log) too.
- '**/data/repository/MailRepositoryImplTest.kt'
# SignatureRepository.delete breadcrumbs a default-promotion via AppLog (issue #313), so its unit
# test mockkStatic(Log) — it does not bypass the facade.
- '**/data/settings/SignatureRepositoryTest.kt'
- '**/data/repository/MailRepositoryImplCoverageTest.kt'
# Account-add breadcrumb (issue #403): addImapAccount/addOutlookAccount log via AppLog, so this
# suite mockkStatic(Log) so the calls don't crash on the throwing JVM stub.
+2 -1
View File
@@ -110,7 +110,8 @@ UNIFIED folder-only : SCAN TABLE messages USING INDEX index_messages_timestampMi
search (`matchesSearch`) over the small folder-scoped set — never the whole cache. `StateFlow`'s
built-in equality de-dup means an unrelated write now costs one cheap scoped re-query and no
recomposition.
- `observeSummaries()` is retained as the #51 CursorWindow regression-guard target in the DB tests.
- The #51 CursorWindow regression guard now targets the paged `pagingUnifiedFolderSummaries()`
projection in the DB tests; the superseded whole-table `observeSummaries()` was removed (issue #313).
## Deferred follow-ups