fix(mail): below-cut mail/richtext perf & correctness nits (#298) #435
@@ -7,6 +7,7 @@ import kotlinx.coroutines.withContext
|
||||
import org.json.JSONArray
|
||||
import org.json.JSONObject
|
||||
import org.libremail.domain.model.OutgoingMessage
|
||||
import org.libremail.reporting.AppLog
|
||||
import java.io.IOException
|
||||
import java.net.HttpURLConnection
|
||||
import java.net.URL
|
||||
@@ -34,6 +35,14 @@ class GraphSender @Inject constructor() {
|
||||
message: OutgoingMessage,
|
||||
attachments: List<SendableAttachment> = emptyList(),
|
||||
) = withContext(Dispatchers.IO) {
|
||||
// Guard before any attachment is read into memory: Graph sendMail carries attachment bytes inline
|
||||
// (base64) in a single ~4 MB request, so an oversized file would blow that request limit and risk
|
||||
// an OOM from readBytes(). Fail with mayHaveSent=false so the outbox falls back to SMTP, which
|
||||
// streams attachments and handles far larger files (#298).
|
||||
attachments.firstOrNull { it.file.length() > MAX_ATTACHMENT_BYTES }?.let {
|
||||
AppLog.w(TAG, "Attachment over Graph sendMail size limit; not sending via Graph")
|
||||
throw GraphSendException("Attachment exceeds the Graph sendMail size limit", mayHaveSent = false)
|
||||
}
|
||||
val payload = buildSendMailPayload(message, attachments)
|
||||
val connection = (URL(SEND_MAIL_URL).openConnection() as HttpURLConnection).apply {
|
||||
requestMethod = "POST"
|
||||
@@ -76,8 +85,13 @@ class GraphSender @Inject constructor() {
|
||||
}
|
||||
|
||||
private companion object {
|
||||
const val TAG = "GraphSender"
|
||||
const val SEND_MAIL_URL = "https://graph.microsoft.com/v1.0/me/sendMail"
|
||||
const val TIMEOUT_MS = 15_000
|
||||
|
||||
// Per-file ceiling kept below Graph sendMail's ~4 MB whole-request cap, so one attachment can never
|
||||
// exceed the request limit or OOM when read into the base64 payload; larger files fall back to SMTP.
|
||||
const val MAX_ATTACHMENT_BYTES = 3L * 1024 * 1024
|
||||
const val HTTP_OK_MIN = 200
|
||||
const val HTTP_OK_MAX = 299
|
||||
const val ERROR_BODY_LIMIT = 500
|
||||
|
||||
@@ -12,23 +12,29 @@ package org.libremail.mail
|
||||
*/
|
||||
object HtmlToText {
|
||||
|
||||
// Hoisted out of convert() so each pattern is compiled once, not four times per call — convert()
|
||||
// runs once per fetched HTML body during sync, so this is a hot path (#298).
|
||||
private val SCRIPT_STYLE = Regex("(?is)<(script|style)\\b[^>]*>.*?</\\1>")
|
||||
private val LIST_ITEM = Regex("(?i)<li\\b[^>]*>")
|
||||
private val BLOCK_BREAK = Regex(
|
||||
"(?i)</?(p|div|tr|table|ul|ol|h[1-6]|blockquote)\\b[^>]*>|<br\\s*/?>",
|
||||
)
|
||||
private val LIST_ITEM = Regex("(?i)<li\\b[^>]*>")
|
||||
private val TAG = Regex("<[^>]*>")
|
||||
private val SPACES_AND_TABS = Regex("[ \\t]+")
|
||||
private val BLANK_LINES = Regex("\n{3,}")
|
||||
|
||||
fun convert(html: String): String {
|
||||
var s = html
|
||||
// Drop script/style contents outright so their text never leaks into the output.
|
||||
s = s.replace(Regex("(?is)<(script|style)\\b[^>]*>.*?</\\1>"), "")
|
||||
s = SCRIPT_STYLE.replace(s, "")
|
||||
s = LIST_ITEM.replace(s, "\n• ")
|
||||
s = BLOCK_BREAK.replace(s, "\n")
|
||||
s = s.replace(Regex("<[^>]*>"), "")
|
||||
s = TAG.replace(s, "")
|
||||
s = decodeEntities(s)
|
||||
// Collapse runs of spaces/tabs, then trim trailing spaces and cap consecutive blank lines.
|
||||
s = s.replace(Regex("[ \\t]+"), " ")
|
||||
s = SPACES_AND_TABS.replace(s, " ")
|
||||
s = s.lineSequence().joinToString("\n") { it.trim() }
|
||||
s = s.replace(Regex("\n{3,}"), "\n\n")
|
||||
s = BLANK_LINES.replace(s, "\n\n")
|
||||
return s.trim()
|
||||
}
|
||||
|
||||
|
||||
@@ -99,14 +99,24 @@ private fun providerLabel(account: Account): String = when (account.authType) {
|
||||
AuthType.PASSWORD_IMAP -> imapProviderLabel(account.imap.host)
|
||||
}
|
||||
|
||||
/**
|
||||
* Buckets an IMAP host to a coarse provider by matching brand tokens at DNS-label boundaries rather
|
||||
* than as raw substrings, so a custom domain that merely contains a brand name — e.g.
|
||||
* `mail.notgmail.example` — is no longer mislabeled (here it would have read as Gmail) (#298). The
|
||||
* short, common tokens (`me`/`mac`/`live`) match only as a registrable-domain suffix, never as a bare
|
||||
* label, so an innocent `me.company.example` doesn't read as iCloud either.
|
||||
*/
|
||||
private fun imapProviderLabel(host: String): String {
|
||||
val h = host.lowercase()
|
||||
val labels = h.split('.')
|
||||
fun hasLabel(vararg brands: String) = brands.any { it in labels }
|
||||
fun hasDomain(vararg domains: String) = domains.any { h == it || h.endsWith(".$it") }
|
||||
return when {
|
||||
"gmail" in h || "googlemail" in h -> "Gmail"
|
||||
"yahoo" in h -> "Yahoo"
|
||||
"icloud" in h || "me.com" in h || "mac.com" in h -> "iCloud"
|
||||
"outlook" in h || "office365" in h || "hotmail" in h || "live.com" in h -> "Outlook"
|
||||
"aol" in h -> "AOL"
|
||||
hasLabel("gmail", "googlemail") -> "Gmail"
|
||||
hasLabel("yahoo") -> "Yahoo"
|
||||
hasLabel("icloud") || hasDomain("me.com", "mac.com") -> "iCloud"
|
||||
hasLabel("outlook", "office365", "hotmail") || hasDomain("live.com") -> "Outlook"
|
||||
hasLabel("aol") -> "AOL"
|
||||
else -> "Other"
|
||||
}
|
||||
}
|
||||
|
||||
@@ -9,6 +9,9 @@ import kotlinx.coroutines.flow.StateFlow
|
||||
import kotlinx.coroutines.flow.asStateFlow
|
||||
import kotlinx.coroutines.launch
|
||||
import java.io.File
|
||||
import java.nio.file.AtomicMoveNotSupportedException
|
||||
import java.nio.file.Files
|
||||
import java.nio.file.StandardCopyOption
|
||||
|
||||
/**
|
||||
* File-backed store of pending [DebugReport]s — one JSON file per report under [directory].
|
||||
@@ -52,7 +55,7 @@ class ReportStore(
|
||||
synchronized(lock) {
|
||||
val serialized = serializeForDisk(report) ?: return
|
||||
directory.mkdirs()
|
||||
File(directory, fileName(report.id)).writeText(serialized)
|
||||
writeAtomically(File(directory, fileName(report.id)), serialized)
|
||||
_reports.value = scan()
|
||||
}
|
||||
}
|
||||
@@ -69,7 +72,7 @@ class ReportStore(
|
||||
val report = _reports.value.firstOrNull { it.id == id } ?: return
|
||||
if (report.surfaced) return
|
||||
val serialized = serializeForDisk(report.copy(surfaced = true)) ?: return
|
||||
File(directory, fileName(id)).writeText(serialized)
|
||||
writeAtomically(File(directory, fileName(id)), serialized)
|
||||
_reports.value = scan()
|
||||
}
|
||||
}
|
||||
@@ -135,10 +138,38 @@ class ReportStore(
|
||||
return runCatching { DebugReport.fromStorageJson(json) }.getOrNull()
|
||||
}
|
||||
|
||||
/**
|
||||
* Writes [content] to [target] via a temp file + atomic rename, so a process death mid-write — the
|
||||
* crash path that saves a report while the app is dying — can never leave a torn `.json` that [scan]
|
||||
* would fail to parse and silently drop (#298). The temp file uses a non-`.json` suffix so [scan]
|
||||
* ignores it (and any orphan left by an interrupted write), and the rename replaces an existing file
|
||||
* (the [markSurfaced] rewrite) atomically. Falls back to a plain replace on the rare filesystem
|
||||
* without atomic rename — still safer than an in-place truncate-then-write.
|
||||
*/
|
||||
private fun writeAtomically(target: File, content: String) {
|
||||
val tmp = File(directory, target.name + TMP_SUFFIX)
|
||||
tmp.writeText(content)
|
||||
try {
|
||||
Files.move(
|
||||
tmp.toPath(),
|
||||
target.toPath(),
|
||||
StandardCopyOption.ATOMIC_MOVE,
|
||||
StandardCopyOption.REPLACE_EXISTING,
|
||||
)
|
||||
} catch (e: AtomicMoveNotSupportedException) {
|
||||
AppLog.w(TAG, "Atomic report write unsupported here; falling back to a non-atomic replace", e)
|
||||
Files.move(tmp.toPath(), target.toPath(), StandardCopyOption.REPLACE_EXISTING)
|
||||
}
|
||||
}
|
||||
|
||||
private fun fileName(id: String) = "$id$SUFFIX"
|
||||
|
||||
private companion object {
|
||||
const val SUFFIX = ".json"
|
||||
|
||||
// Suffix for the write-and-rename temp file. Deliberately NOT ending in [SUFFIX] so scan() never
|
||||
// treats a half-written or orphaned temp as a report (#298).
|
||||
const val TMP_SUFFIX = ".tmp"
|
||||
const val TAG = "ReportStore"
|
||||
|
||||
/**
|
||||
|
||||
@@ -109,11 +109,17 @@ internal fun lineMarker(line: String): String? = when {
|
||||
*/
|
||||
internal fun mergeSameValueSpans(spans: List<RichSpan>): List<RichSpan> {
|
||||
val merged = ArrayList<RichSpan>()
|
||||
// Index of the right-most merged run for each style value. Spans are processed in ascending start
|
||||
// order, and runs of one value stay non-overlapping with strictly increasing ends, so only that
|
||||
// value's last run can touch the next span — track it directly instead of re-scanning `merged` for
|
||||
// every span (the old O(n^2) indexOfLast). The produced list is byte-for-byte identical (#298).
|
||||
val lastRunByStyle = HashMap<RichStyle, Int>()
|
||||
for (span in spans.sortedWith(compareBy({ it.start }, { it.end }))) {
|
||||
val i = merged.indexOfLast { it.style == span.style && span.start <= it.end }
|
||||
if (i >= 0) {
|
||||
val i = lastRunByStyle[span.style]
|
||||
if (i != null && span.start <= merged[i].end) {
|
||||
merged[i] = merged[i].copy(end = maxOf(merged[i].end, span.end))
|
||||
} else {
|
||||
lastRunByStyle[span.style] = merged.size
|
||||
merged.add(span)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -33,10 +33,15 @@ object RichTextEditing {
|
||||
return content.copy(spans = (otherKinds + updated).sortedBy { it.start })
|
||||
}
|
||||
|
||||
/** Links [[start], [end]) to [url], replacing any links that overlap the range. */
|
||||
/**
|
||||
* Links [[start], [end]) to [url]. A link that only partially overlaps the range keeps its
|
||||
* non-overlapping remainder — the same split [toggleStyle]/[subtractRange] does for spans — instead
|
||||
* of being dropped whole, so relinking part of a longer link no longer silently un-links the rest
|
||||
* of it (#298). A link fully inside the range is replaced outright.
|
||||
*/
|
||||
fun applyLink(content: RichTextContent, start: Int, end: Int, url: String): RichTextContent {
|
||||
if (start >= end || url.isBlank()) return content
|
||||
val kept = content.links.filter { it.end <= start || it.start >= end }
|
||||
val kept = subtractLinkRange(content.links, start, end)
|
||||
return content.copy(links = (kept + RichLink(start, end, url)).sortedBy { it.start })
|
||||
}
|
||||
|
||||
@@ -221,6 +226,17 @@ private fun subtractRange(spans: List<RichSpan>, start: Int, end: Int): List<Ric
|
||||
}
|
||||
}
|
||||
|
||||
/** Links analogue of [subtractRange]: drops the [[start], [end]) slice of each link, keeping the rest. */
|
||||
private fun subtractLinkRange(links: List<RichLink>, start: Int, end: Int): List<RichLink> = links.flatMap { link ->
|
||||
when {
|
||||
link.end <= start || link.start >= end -> listOf(link)
|
||||
else -> buildList {
|
||||
if (link.start < start) add(link.copy(end = start))
|
||||
if (link.end > end) add(link.copy(start = end))
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// --- block marker helpers ---
|
||||
|
||||
private val ORDERED = Regex("^\\d+\\. ")
|
||||
|
||||
@@ -1,14 +1,20 @@
|
||||
// SPDX-License-Identifier: GPL-3.0-or-later
|
||||
package org.libremail.mail
|
||||
|
||||
import io.mockk.every
|
||||
import io.mockk.mockkStatic
|
||||
import io.mockk.unmockkAll
|
||||
import kotlinx.coroutines.test.runTest
|
||||
import org.junit.After
|
||||
import org.junit.Before
|
||||
import org.junit.Test
|
||||
import org.libremail.domain.model.OutgoingMessage
|
||||
import java.io.ByteArrayOutputStream
|
||||
import java.io.File
|
||||
import java.io.IOException
|
||||
import java.io.InputStream
|
||||
import java.io.OutputStream
|
||||
import java.io.RandomAccessFile
|
||||
import java.net.HttpURLConnection
|
||||
import java.net.URL
|
||||
import java.net.URLConnection
|
||||
@@ -18,6 +24,7 @@ import java.util.concurrent.atomic.AtomicReference
|
||||
import kotlin.test.assertEquals
|
||||
import kotlin.test.assertFailsWith
|
||||
import kotlin.test.assertFalse
|
||||
import kotlin.test.assertNull
|
||||
import kotlin.test.assertTrue
|
||||
|
||||
/**
|
||||
@@ -30,8 +37,19 @@ import kotlin.test.assertTrue
|
||||
*/
|
||||
class GraphSenderSendTest {
|
||||
|
||||
@Before
|
||||
fun setUp() {
|
||||
// send() now breadcrumbs through AppLog on the oversized-attachment guard; android.util.Log is a
|
||||
// no-op stub under plain JVM tests, so mock it (fully qualified, so this file never imports it).
|
||||
mockkStatic(android.util.Log::class)
|
||||
every { android.util.Log.w(any<String>(), any<String>()) } returns 0
|
||||
}
|
||||
|
||||
@After
|
||||
fun tearDown() = armed.set(null)
|
||||
fun tearDown() {
|
||||
armed.set(null)
|
||||
unmockkAll()
|
||||
}
|
||||
|
||||
private val message =
|
||||
OutgoingMessage(accountId = "outlook:me@x.com", to = "bob@example.org", subject = "Hi", body = "Body")
|
||||
@@ -84,6 +102,26 @@ class GraphSenderSendTest {
|
||||
assertFalse(ex.mayHaveSent, "the request never reached Graph, so a retry is safe")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `an oversized attachment fails safe-to-fall-back before opening a connection`() = runTest {
|
||||
val last = arm() // armed, but the guard must trip before any connection is opened
|
||||
val big = File.createTempFile("graph-big", ".bin")
|
||||
try {
|
||||
// 4 MiB, over the 3 MiB per-file cap. setLength allocates the size without writing the bytes,
|
||||
// so the guard (which reads file.length()) trips without the test materializing 4 MiB.
|
||||
RandomAccessFile(big, "rw").use { it.setLength(4L * 1024 * 1024) }
|
||||
|
||||
val ex = assertFailsWith<GraphSendException> {
|
||||
GraphSender().send("token", message, listOf(SendableAttachment(big)))
|
||||
}
|
||||
|
||||
assertFalse(ex.mayHaveSent, "oversized never reached Graph, so SMTP fallback is safe")
|
||||
assertNull(last.get(), "the guard must trip before any connection is opened")
|
||||
} finally {
|
||||
big.delete()
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `GraphSendException carries its message, flag and cause`() {
|
||||
val cause = IOException("boom")
|
||||
|
||||
@@ -169,6 +169,26 @@ class DiagnosticsCollectorTest {
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `provider label matches brand tokens at label boundaries, not as substrings`() = runTest {
|
||||
every { settingsRepository.settings } returns flowOf(AppSettings())
|
||||
// Each custom host merely CONTAINS a brand name inside a longer DNS label; the old substring match
|
||||
// mislabeled them (notgmail→Gmail, yahooligans→Yahoo, me.company→iCloud, kaolin→AOL). They must all
|
||||
// bucket to "Other" now (#298).
|
||||
every { accountRepository.observeAccounts() } returns flowOf(
|
||||
listOf(
|
||||
account("1@x", AuthType.PASSWORD_IMAP, "mail.notgmail.example"),
|
||||
account("2@x", AuthType.PASSWORD_IMAP, "imap.yahooligans.example"),
|
||||
account("3@x", AuthType.PASSWORD_IMAP, "me.company.example"),
|
||||
account("4@x", AuthType.PASSWORD_IMAP, "kaolin.example"),
|
||||
),
|
||||
)
|
||||
|
||||
val report = collector.collectManual()
|
||||
|
||||
assertEquals(List(4) { "Other (PASSWORD_IMAP)" }, report.accounts)
|
||||
}
|
||||
|
||||
private fun account(email: String, authType: AuthType, imapHost: String) = Account(
|
||||
id = "id:$email",
|
||||
email = email,
|
||||
|
||||
@@ -111,6 +111,29 @@ class ReportStoreTest {
|
||||
assertEquals(listOf("valid"), store.reports.value.map { it.id })
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `save leaves no temporary file behind (atomic write renames it into place)`() {
|
||||
val store = newStore()
|
||||
|
||||
store.save(report("a"))
|
||||
|
||||
// The write-and-rename temp must not linger: only the final ".json" remains on disk (#298).
|
||||
assertEquals(listOf("a.json"), tempFolder.root.listFiles()?.map { it.name }.orEmpty())
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a stray temp file from an interrupted write is never scanned as a report`() {
|
||||
// A process death mid-write leaves a ".json.tmp" file, never a torn ".json". scan() filters on
|
||||
// ".json", so the orphan is ignored and a valid report saved alongside still lists cleanly — the
|
||||
// old in-place write could instead leave a truncated ".json" that scan() silently dropped (#298).
|
||||
File(tempFolder.root, "torn.json.tmp").writeText("{ half-written")
|
||||
val store = newStore()
|
||||
|
||||
store.save(report("valid"))
|
||||
|
||||
assertEquals(listOf("valid"), store.reports.value.map { it.id })
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `purgeOlderThan deletes reports strictly older than the cutoff`() {
|
||||
val store = newStore()
|
||||
|
||||
@@ -301,18 +301,53 @@ class RichTextEditingTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `applyLink keeps links wholly outside the range and replaces overlapping ones`() {
|
||||
fun `applyLink keeps links outside the range and splits partial overlaps, keeping the remainder`() {
|
||||
val content = RichTextContent(
|
||||
"0123456789",
|
||||
links = listOf(RichLink(0, 2, "a"), RichLink(3, 6, "b"), RichLink(7, 9, "c")),
|
||||
)
|
||||
val result = RichTextEditing.applyLink(content, 4, 7, "http://new")
|
||||
assertEquals(
|
||||
listOf(RichLink(0, 2, "a"), RichLink(4, 7, "http://new"), RichLink(7, 9, "c")),
|
||||
// "a" is wholly outside; "b" (3,6) overlaps [4,7) so only its (3,4) remainder survives (it is
|
||||
// no longer dropped whole); the new link takes [4,7); "c" starts at the range end, kept whole.
|
||||
listOf(RichLink(0, 2, "a"), RichLink(3, 4, "b"), RichLink(4, 7, "http://new"), RichLink(7, 9, "c")),
|
||||
result.links.sortedBy { it.start },
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `applyLink over the middle of a link relinks the middle and keeps both surrounding remainders`() {
|
||||
val content = RichTextContent("0123456789", links = listOf(RichLink(0, 8, "old")))
|
||||
val result = RichTextEditing.applyLink(content, 3, 5, "new")
|
||||
assertEquals(
|
||||
listOf(RichLink(0, 3, "old"), RichLink(3, 5, "new"), RichLink(5, 8, "old")),
|
||||
result.links.sortedBy { it.start },
|
||||
)
|
||||
}
|
||||
|
||||
// --- mergeSameValueSpans (shared merge used by toggleStyle and the HTML parser) ---
|
||||
|
||||
@Test
|
||||
fun `mergeSameValueSpans coalesces touching and overlapping runs of the same value only`() {
|
||||
val merged = mergeSameValueSpans(
|
||||
listOf(
|
||||
RichSpan(5, 8, RichStyle.Bold), // out of order, and overlaps the (3,6) run below
|
||||
RichSpan(0, 3, RichStyle.Bold),
|
||||
RichSpan(3, 6, RichStyle.Bold), // touches (0,3) and overlaps (5,8) → one 0..8 run
|
||||
RichSpan(0, 4, RichStyle.Italic), // a different value never folds into the Bold run
|
||||
RichSpan(10, 12, RichStyle.Bold), // a gap breaks the run into a fresh one
|
||||
),
|
||||
)
|
||||
assertEquals(
|
||||
listOf(
|
||||
RichSpan(0, 8, RichStyle.Bold),
|
||||
RichSpan(0, 4, RichStyle.Italic),
|
||||
RichSpan(10, 12, RichStyle.Bold),
|
||||
),
|
||||
merged,
|
||||
)
|
||||
}
|
||||
|
||||
// --- styleAt / isStyled caret edges ---
|
||||
|
||||
@Test
|
||||
|
||||
Reference in New Issue
Block a user