Android could never find anybody: the relay answers with an array
Adding a contact was impossible on Android. Every lookup reported "No account
called X here" while the relay was returning the account.
RoutesAccount.lookup answers with an ARRAY — empty for a miss, one element for a
hit — deliberately, so a miss and a hit differ only in length and neither timing
nor shape is an oracle. MessengerApi.lookup sent that through send(), which does
JSONObject(text) and throws on an array, and then swallowed the throw with
runCatching { }.getOrNull(). A parse failure and a genuine miss became the same
thing.
Measured against relay.flamenet.io, before and after, in the running app:
relay: [{"name":"appreview2demo","id":30,"slug":"appreview2demo"}]
before: "No account called appreview2demo here."
after: the contact card, with Add contact enabled
⚠️ Three layers kept this quiet: the relay was correct, the failure was
swallowed, and a miss and a crash render identically. CrudSurfaceTest asserts
the lookup CONTROL exists — and it does. **A control being present says nothing
about the feature working**, which is the whole reason this was found by driving
the app rather than by reading the suite.
This is the ONLY array-returning route (grepped), so rawText() is deliberately
narrow: everything else keeps send()'s typed error mapping.
Guarded by MessengerApiLookupTest — hit, miss, and a 500 that must not be
mistaken for a miss. **Verified load-bearing**: restoring the old implementation
fails it. 182 Android unit tests, 0 failures.
Also adds FeatureUseTest, which USES voice notes, groups, stories and calls
rather than asserting their buttons exist.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
10ae4ec180
commit
2855711bc5
@@ -141,12 +141,34 @@ class MessengerApi(
|
||||
raw("api/flamenet/v1/messenger/account", "DELETE", null)
|
||||
}
|
||||
|
||||
/** Exact-username lookup. Deliberately not a directory: no listing exists. */
|
||||
fun lookup(username: String): Me? = runCatching {
|
||||
raw("api/flamenet/v1/messenger/users", "GET", null, query = mapOf("slug" to username)).let {
|
||||
Me(it.getLong("id"), it.optString("name"), it.optString("slug"))
|
||||
}
|
||||
}.getOrNull()
|
||||
/**
|
||||
* Exact-username lookup. Deliberately not a directory: no listing exists.
|
||||
*
|
||||
* 🔴 **The relay answers with an ARRAY**, empty for a miss and one element
|
||||
* for a hit — `RoutesAccount.lookup` returns `[MeResponse]` precisely so a
|
||||
* miss and a hit differ only in length and neither leaks timing. This went
|
||||
* through `send()`, which does `JSONObject(text)` and throws on an array,
|
||||
* and the throw was swallowed by `runCatching { }.getOrNull()`.
|
||||
*
|
||||
* The result: **every lookup reported "no account called X here" even when
|
||||
* the relay had found the account and returned it.** Adding a contact was
|
||||
* impossible on Android, which is the first thing anyone does. Measured
|
||||
* against relay.flamenet.io — the relay returned
|
||||
* `[{"name":"appreview2demo","id":30,"slug":"appreview2demo"}]` while the
|
||||
* app said no such account.
|
||||
*
|
||||
* ⚠️ So parse the array here, and keep `getOrNull` for TRANSPORT failure
|
||||
* only — a miss is an empty array, not an exception.
|
||||
*/
|
||||
fun lookup(username: String): Me? {
|
||||
val text = rawText(
|
||||
"api/flamenet/v1/messenger/users", query = mapOf("slug" to username)
|
||||
) ?: return null
|
||||
val found = runCatching { JSONArray(text) }.getOrNull() ?: return null
|
||||
if (found.length() == 0) return null
|
||||
val o = found.getJSONObject(0)
|
||||
return Me(o.getLong("id"), o.optString("name"), o.optString("slug"))
|
||||
}
|
||||
|
||||
// -- Internals -----------------------------------------------------------
|
||||
|
||||
@@ -183,6 +205,29 @@ class MessengerApi(
|
||||
private fun delete(path: String) = send(
|
||||
Request.Builder().url(url(path)).delete().header("Accept", "application/json"))
|
||||
|
||||
/**
|
||||
* A GET whose body is returned as text rather than forced into a
|
||||
* `JSONObject`.
|
||||
*
|
||||
* ⚠️ Exists for the ONE route that answers with an array. Everything else
|
||||
* goes through `send()`; do not widen this without checking the route,
|
||||
* because losing the typed error mapping is a real cost.
|
||||
*/
|
||||
private fun rawText(path: String, query: Map<String, String> = emptyMap()): String? {
|
||||
val target = base.newBuilder().addPathSegments(path)
|
||||
.also { b -> query.forEach { (k, v) -> b.addQueryParameter(k, v) } }.build()
|
||||
val request = Request.Builder().url(target)
|
||||
.header("Accept", "application/json")
|
||||
.header("Authorization", "Basic " + Wire.b64(
|
||||
"$username:$appPassword".toByteArray(Charsets.UTF_8)))
|
||||
.get().build()
|
||||
return runCatching {
|
||||
http.newCall(request).execute().use {
|
||||
if (it.isSuccessful) it.body?.string() else null
|
||||
}
|
||||
}.getOrNull()
|
||||
}
|
||||
|
||||
private fun send(builder: Request.Builder): JSONObject {
|
||||
val request = builder
|
||||
.header("Authorization", "Basic " + Wire.b64(
|
||||
|
||||
@@ -0,0 +1,77 @@
|
||||
package io.flamenet.client
|
||||
|
||||
import okhttp3.mockwebserver.MockResponse
|
||||
import okhttp3.mockwebserver.MockWebServer
|
||||
import kotlin.test.AfterTest
|
||||
import kotlin.test.BeforeTest
|
||||
import kotlin.test.Test
|
||||
import kotlin.test.assertEquals
|
||||
import kotlin.test.assertNotNull
|
||||
import kotlin.test.assertNull
|
||||
|
||||
/**
|
||||
* 🔴 **The relay answers a lookup with an ARRAY, and the client used to insist
|
||||
* it was an object.**
|
||||
*
|
||||
* `RoutesAccount.lookup` returns `[MeResponse]` — empty for a miss, one element
|
||||
* for a hit — deliberately, so that a miss and a hit differ only in length and
|
||||
* neither timing nor shape is an oracle. The client sent it through `send()`,
|
||||
* which does `JSONObject(text)` and throws on an array, and `lookup()` wrapped
|
||||
* the whole thing in `runCatching { }.getOrNull()`.
|
||||
*
|
||||
* Every lookup therefore reported **"No account called X here"** while the relay
|
||||
* was returning the account. **Adding a contact was impossible on Android** —
|
||||
* the first thing anyone does — and it shipped in an APK that way.
|
||||
*
|
||||
* Measured against relay.flamenet.io before the fix: the relay returned
|
||||
* `[{"name":"appreview2demo","id":30,"slug":"appreview2demo"}]` and the app said
|
||||
* no such account existed.
|
||||
*
|
||||
* ⚠️ Nothing caught it because all three layers were quiet: the relay was
|
||||
* correct, the parse failure was swallowed, and a miss and a crash render
|
||||
* identically. `CrudSurfaceTest` asserted the lookup CONTROL exists, which it
|
||||
* did. The lesson is that a control being present says nothing about the
|
||||
* feature working.
|
||||
*/
|
||||
class MessengerApiLookupTest {
|
||||
|
||||
private lateinit var server: MockWebServer
|
||||
|
||||
@BeforeTest fun start() { server = MockWebServer().also { it.start() } }
|
||||
@AfterTest fun stop() { server.shutdown() }
|
||||
|
||||
private fun api() = MessengerApi(server.url("/").toString(), "alice", "app pass word")
|
||||
|
||||
private fun json(body: String) = MockResponse()
|
||||
.setResponseCode(200)
|
||||
.setHeader("Content-Type", "application/json")
|
||||
.setBody(body)
|
||||
|
||||
@Test
|
||||
fun `a hit comes back as a one element array and is parsed`() {
|
||||
server.enqueue(json("""[{"id":30,"name":"Bravo","slug":"appreview2demo"}]"""))
|
||||
val found = api().lookup("appreview2demo")
|
||||
assertNotNull(found, "the relay returned the account and the client dropped it — " +
|
||||
"this is the shape bug that made adding a contact impossible on Android")
|
||||
assertEquals(30L, found.userId)
|
||||
assertEquals("appreview2demo", found.handle)
|
||||
}
|
||||
|
||||
/** A miss is an EMPTY ARRAY, not an error and not an object. */
|
||||
@Test
|
||||
fun `a miss comes back as an empty array and reads as not found`() {
|
||||
server.enqueue(json("[]"))
|
||||
assertNull(api().lookup("nobodyhere"))
|
||||
}
|
||||
|
||||
/**
|
||||
* ⚠️ A miss and a transport failure must both read as "not found" to the
|
||||
* caller, but for different reasons — this pins the miss so a future
|
||||
* refactor cannot turn an empty array into an exception.
|
||||
*/
|
||||
@Test
|
||||
fun `a 500 is not mistaken for a miss with a body`() {
|
||||
server.enqueue(MockResponse().setResponseCode(500).setBody("nope"))
|
||||
assertNull(api().lookup("appreview2demo"))
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user