test: cover the offline queue, account wipe, importers, and wear codecs

CHANGELOG.md

@@ -22,7 +22,14 @@ uses semantic versioning (`MAJOR.MINOR.PATCH`).
   Signing in or out now clears the local response cache and the offline action
   queue, so a new account can't briefly see the prior account's members, fronts
   or history, and actions queued while offline under one account can no longer
-  replay under another's credentials.
+  replay under another's credentials. The clear is now resilient: if wiping the
+  cache errors, the offline queue is still cleared (previously one failure
+  skipped the rest).
+
+- **Importing with a token that has a stray line break no longer fails.** Tokens
+  pasted from a chat client often pick up a trailing newline; the import request
+  now escapes it correctly instead of sending malformed JSON that the server
+  rejected.
 
 - **Wear: concurrent token refreshes no longer invalidate the session.** The
   watch app, its tiles, and its complications each build their own API client;

sheaf/app/src/main/java/systems/lupine/sheaf/data/repository/AccountDataWiper.kt

@@ -21,11 +21,21 @@ class AccountDataWiper @Inject constructor(
     private val cache: LocalCache,
     private val pendingOps: PendingOperationsDao,
 ) {
+    /**
+     * Best-effort, but each step is attempted independently. A single
+     * runCatching around all three meant a throw from [LocalCache.clearAll]
+     * skipped both queue deletes, leaving the previous account's queued front
+     * switches to replay against the new account's credentials: exactly the
+     * thing this class exists to prevent, silently swallowed.
+     */
     suspend fun wipe() {
-        runCatching {
-            cache.clearAll()
-            pendingOps.deleteAllSwitches()
-            pendingOps.deleteAllRemovals()
-        }.onFailure { Log.w("AccountDataWiper", "failed to wipe account data", it) }
+        step("cache") { cache.clearAll() }
+        step("pending switches") { pendingOps.deleteAllSwitches() }
+        step("pending removals") { pendingOps.deleteAllRemovals() }
+    }
+
+    private suspend fun step(what: String, block: suspend () -> Unit) {
+        runCatching { block() }
+            .onFailure { Log.w("AccountDataWiper", "failed to wipe $what", it) }
     }
 }

sheaf/app/src/main/java/systems/lupine/sheaf/data/sync/SyncWorker.kt

@@ -40,10 +40,7 @@ class SyncWorker @AssistedInject constructor(
         // mid-flight cancellation can still duplicate a front; a true
         // exactly-once guarantee needs a server-side Idempotency-Key on
         // createFront, tracked as a backend follow-up.
-        val ops: List<QueuedOp> = buildList {
-            dao.getAllRemovals().forEach { add(QueuedOp.Removal(it)) }
-            dao.getAllSwitches().forEach { add(QueuedOp.Switch(it)) }
-        }.sortedBy { it.createdAt }
+        val ops = mergeQueuedOps(dao.getAllRemovals(), dao.getAllSwitches())
 
         for (op in ops) {
             val retry = when (op) {
@@ -71,7 +68,7 @@ class SyncWorker @AssistedInject constructor(
         }.fold(
             onSuccess = { dao.deleteRemoval(removal); false },
             onFailure = { e ->
-                if (isPermanent(e)) {
+                if (isPermanentHttpFailure(e)) {
                     // Front already gone / ended on another device: drop it so
                     // it can't wedge the queue behind an endless retry.
                     Log.w(WORK_NAME, "dropping un-replayable removal (${reason(e)})")
@@ -103,7 +100,7 @@ class SyncWorker @AssistedInject constructor(
         }.fold(
             onSuccess = { dao.deleteSwitch(switch); false },
             onFailure = { e ->
-                if (isPermanent(e)) {
+                if (isPermanentHttpFailure(e)) {
                     // Un-replayable: a member was deleted server-side (404) or
                     // the payload is rejected (422). Drop so the rest isn't stuck.
                     Log.w(WORK_NAME, "dropping un-replayable switch (${reason(e)})")
@@ -113,21 +110,6 @@ class SyncWorker @AssistedInject constructor(
         )
     }
 
-    private sealed class QueuedOp(val createdAt: Long) {
-        class Removal(val row: PendingFrontRemoval) : QueuedOp(row.createdAt)
-        class Switch(val row: PendingFrontSwitch) : QueuedOp(row.createdAt)
-    }
-
-    // A failure is "permanent" when the server says the request itself is bad
-    // and replaying it can't help: a 4xx other than auth (401/403, handled by
-    // the token authenticator and recoverable on re-login), request timeout
-    // (408) and rate limiting (429), which are worth retrying. Network / IO
-    // errors and 5xx aren't HttpExceptions or are >=500, so they retry.
-    private fun isPermanent(t: Throwable): Boolean {
-        val code = (t as? HttpException)?.code() ?: return false
-        return code in 400..499 && code !in setOf(401, 403, 408, 429)
-    }
-
     private fun reason(t: Throwable): String =
         (t as? HttpException)?.let { "HTTP ${it.code()}" } ?: (t::class.simpleName ?: "error")
 
@@ -147,3 +129,44 @@ class SyncWorker @AssistedInject constructor(
         }
     }
 }
+
+/**
+ * One queued offline mutation, tagged with when the user actually made it.
+ * Top-level (rather than nested in the worker) so the ordering and the
+ * failure classification below can be unit-tested without a WorkManager.
+ */
+internal sealed class QueuedOp(val createdAt: Long) {
+    class Removal(val row: PendingFrontRemoval) : QueuedOp(row.createdAt)
+    class Switch(val row: PendingFrontSwitch) : QueuedOp(row.createdAt)
+}
+
+/**
+ * Interleave removals and switches by the time the user made them, rather than
+ * replaying all removals and then all switches, which rebuilds the wrong
+ * timeline whenever the queue holds both.
+ */
+internal fun mergeQueuedOps(
+    removals: List<PendingFrontRemoval>,
+    switches: List<PendingFrontSwitch>,
+): List<QueuedOp> = buildList {
+    removals.forEach { add(QueuedOp.Removal(it)) }
+    switches.forEach { add(QueuedOp.Switch(it)) }
+}.sortedBy { it.createdAt }
+
+/**
+ * A failure is "permanent" when the server says the request itself is bad and
+ * replaying it can't help: a 4xx other than auth (401/403, handled by the token
+ * authenticator and recoverable on re-login), request timeout (408) and rate
+ * limiting (429), which are all worth retrying. Network / IO errors and 5xx
+ * aren't HttpExceptions, or are >= 500, so they retry.
+ *
+ * Getting this wrong is expensive in both directions: call a transient failure
+ * permanent and the user's offline switch is silently dropped; call a permanent
+ * one transient and it wedges the whole queue behind an endless retry.
+ *
+ * The wear app mirrors this in WearStore.isPermanentSwitchError.
+ */
+internal fun isPermanentHttpFailure(t: Throwable): Boolean {
+    val code = (t as? HttpException)?.code() ?: return false
+    return code in 400..499 && code !in setOf(401, 403, 408, 429)
+}

sheaf/app/src/main/java/systems/lupine/sheaf/ui/importcommon/JsonString.kt

@@ -0,0 +1,38 @@
+package systems.lupine.sheaf.ui.importcommon
+
+/**
+ * Escape a string for embedding in a hand-built JSON body.
+ *
+ * The importers assemble their submit bodies as strings (the options blob is
+ * already-encoded JSON, so a typed data class would mean encoding it twice).
+ * That means every interpolated value has to be escaped here, and the escapers
+ * were doing quotes and backslashes only. A pasted PluralKit token with a
+ * newline in it (trivially easy: copy a token out of a chat client and pick up
+ * the trailing line break) produced a body with a literal newline inside a JSON
+ * string, which is invalid JSON, so the whole import 422'd with nothing useful
+ * to show the user.
+ *
+ * Handles the full set JSON requires: backslash, quote, the named control
+ * escapes, and \\u00xx for everything else below 0x20.
+ */
+internal fun jsonEscape(value: String): String {
+    val sb = StringBuilder(value.length + 8)
+    for (c in value) {
+        when (c) {
+            '\\' -> sb.append("\\\\")
+            '"' -> sb.append("\\\"")
+            '\b' -> sb.append("\\b")
+            '\n' -> sb.append("\\n")
+            '\r' -> sb.append("\\r")
+            '\t' -> sb.append("\\t")
+            '\u000C' -> sb.append("\\f")
+            else ->
+                if (c < ' ') sb.append("\\u%04x".format(c.code))
+                else sb.append(c)
+        }
+    }
+    return sb.toString()
+}
+
+/** [jsonEscape]d and wrapped in quotes, ready to drop into a JSON body. */
+internal fun jsonQuote(value: String): String = "\"${jsonEscape(value)}\""

sheaf/app/src/main/java/systems/lupine/sheaf/ui/importsp/ImportViewModel.kt

@@ -1,5 +1,6 @@
 package systems.lupine.sheaf.ui.importsp
 
+import systems.lupine.sheaf.ui.importcommon.jsonQuote
 import android.content.Context
 import android.net.Uri
 import android.provider.OpenableColumns
@@ -225,7 +226,7 @@ private fun buildSpOptionsJson(opts: ImportOptions, memberIds: List<String>?): S
     parts += "\"groups\":${opts.groups}"
     parts += "\"front_history\":${opts.frontHistory}"
     if (memberIds != null) {
-        val ids = memberIds.joinToString(",") { "\"${it.replace("\"", "\\\"")}\"" }
+        val ids = memberIds.joinToString(",") { jsonQuote(it) }
         parts += "\"member_ids\":[$ids]"
     }
     return parts.joinToString(",", prefix = "{", postfix = "}")

sheaf/app/src/main/java/systems/lupine/sheaf/ui/pkapiimport/PluralKitApiImportViewModel.kt

@@ -1,5 +1,6 @@
 package systems.lupine.sheaf.ui.pkapiimport
 
+import systems.lupine.sheaf.ui.importcommon.jsonQuote
 import androidx.lifecycle.ViewModel
 import androidx.lifecycle.viewModelScope
 import dagger.hilt.android.lifecycle.HiltViewModel
@@ -195,7 +196,7 @@ internal fun buildPkApiOptionsJson(opts: PKApiImportOptions, memberIds: List<Str
     parts += "\"groups\":${opts.groups}"
     parts += "\"front_history\":${opts.frontHistory}"
     if (memberIds != null) {
-        val ids = memberIds.joinToString(",") { "\"${it.replace("\"", "\\\"")}\"" }
+        val ids = memberIds.joinToString(",") { jsonQuote(it) }
         parts += "\"member_ids\":[$ids]"
     }
     return parts.joinToString(",", prefix = "{", postfix = "}")
@@ -216,11 +217,8 @@ internal fun buildApiImportBodyJson(
     // strings that wouldn't normally contain quotes or backslashes, but
     // we still escape defensively against a pasted token with whitespace
     // or odd characters.
-    val escapedToken = token
-        .replace("\\", "\\\\")
-        .replace("\"", "\\\"")
     return """{"source":"${ImportJobSource.PLURALKIT_API}",""" +
         """"idempotency_key":"$idempotencyKey",""" +
-        """"pk_token":"$escapedToken",""" +
+        """"pk_token":${jsonQuote(token)},""" +
         """"options":$options}"""
 }

sheaf/app/src/main/java/systems/lupine/sheaf/ui/pkimport/PluralKitFileImportViewModel.kt

@@ -1,5 +1,6 @@
 package systems.lupine.sheaf.ui.pkimport
 
+import systems.lupine.sheaf.ui.importcommon.jsonQuote
 import android.content.Context
 import android.net.Uri
 import android.provider.OpenableColumns
@@ -222,7 +223,7 @@ internal fun buildPkOptionsJson(opts: PKFileImportOptions, memberIds: List<Strin
     parts += "\"groups\":${opts.groups}"
     parts += "\"front_history\":${opts.frontHistory}"
     if (memberIds != null) {
-        val ids = memberIds.joinToString(",") { "\"${it.replace("\"", "\\\"")}\"" }
+        val ids = memberIds.joinToString(",") { jsonQuote(it) }
         parts += "\"member_ids\":[$ids]"
     }
     return parts.joinToString(",", prefix = "{", postfix = "}")

sheaf/app/src/main/java/systems/lupine/sheaf/ui/pluralspaceimport/PluralSpaceImportViewModel.kt

@@ -1,5 +1,6 @@
 package systems.lupine.sheaf.ui.pluralspaceimport
 
+import systems.lupine.sheaf.ui.importcommon.jsonQuote
 import android.content.Context
 import android.net.Uri
 import android.provider.OpenableColumns
@@ -214,7 +215,7 @@ private fun buildOptionsJson(opts: PluralSpaceImportOptions, memberIds: List<Str
     parts += "\"chat_messages\":${opts.chatMessages}"
     parts += "\"polls\":${opts.polls}"
     if (memberIds != null) {
-        val ids = memberIds.joinToString(",") { "\"${it.replace("\"", "\\\"")}\"" }
+        val ids = memberIds.joinToString(",") { jsonQuote(it) }
         parts += "\"member_ids\":[$ids]"
     } else {
         parts += "\"member_ids\":null"

sheaf/app/src/main/java/systems/lupine/sheaf/ui/prismimport/PrismImportViewModel.kt

@@ -1,5 +1,6 @@
 package systems.lupine.sheaf.ui.prismimport
 
+import systems.lupine.sheaf.ui.importcommon.jsonQuote
 import android.content.Context
 import android.net.Uri
 import android.provider.OpenableColumns
@@ -239,7 +240,7 @@ private fun buildOptionsJson(opts: PrismImportOptions, memberIds: List<String>?)
     parts += "\"member_board_posts\":${opts.memberBoardPosts}"
     parts += "\"media_attachments\":${opts.mediaAttachments}"
     if (memberIds != null) {
-        val ids = memberIds.joinToString(",") { "\"${it.replace("\"", "\\\"")}\"" }
+        val ids = memberIds.joinToString(",") { jsonQuote(it) }
         parts += "\"member_ids\":[$ids]"
     } else {
         parts += "\"member_ids\":null"

sheaf/app/src/main/java/systems/lupine/sheaf/ui/tbimport/TupperboxImportViewModel.kt

@@ -1,5 +1,6 @@
 package systems.lupine.sheaf.ui.tbimport
 
+import systems.lupine.sheaf.ui.importcommon.jsonQuote
 import android.content.Context
 import android.net.Uri
 import android.provider.OpenableColumns
@@ -209,7 +210,7 @@ internal fun buildTbOptionsJson(opts: TBImportOptions, memberIds: List<String>?)
     val parts = mutableListOf<String>()
     parts += "\"groups\":${opts.groups}"
     if (memberIds != null) {
-        val ids = memberIds.joinToString(",") { "\"${it.replace("\"", "\\\"")}\"" }
+        val ids = memberIds.joinToString(",") { jsonQuote(it) }
         parts += "\"member_ids\":[$ids]"
     }
     return parts.joinToString(",", prefix = "{", postfix = "}")

sheaf/app/src/test/java/systems/lupine/sheaf/MainDispatcherRule.kt

@@ -0,0 +1,21 @@
+package systems.lupine.sheaf
+
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.test.StandardTestDispatcher
+import kotlinx.coroutines.test.TestDispatcher
+import kotlinx.coroutines.test.resetMain
+import kotlinx.coroutines.test.setMain
+import org.junit.rules.TestWatcher
+import org.junit.runner.Description
+
+/**
+ * viewModelScope dispatches on Dispatchers.Main, which does not exist in a JVM
+ * unit test. Swap it for a TestDispatcher the test controls, so a ViewModel's
+ * coroutines run when the test advances them.
+ */
+class MainDispatcherRule(
+    val dispatcher: TestDispatcher = StandardTestDispatcher(),
+) : TestWatcher() {
+    override fun starting(description: Description) = Dispatchers.setMain(dispatcher)
+    override fun finished(description: Description) = Dispatchers.resetMain()
+}