Use the Google Books API key: the fallback source actually answers now

The user obtained a restricted Google Books key. Keyless requests 429 for every
caller on the internet — all anonymous traffic bills to one shared Google Cloud
project whose daily quota is permanently exhausted — so the documented fallback
has never once answered. Because MetadataRepository.combine turns "a source
failed, none found" into Unavailable, that standing failure meant every Open
Library hiccup reached the user as "couldn't be reached". The app has been
effectively single-sourced since it was written.

Build plumbing reads GOOGLE_BOOKS_API_KEY from local.properties (gitignored),
falling back to the environment and then to empty. A blank key is a supported
state: a fresh clone still builds a working app that falls back to the keyless
endpoint, rather than failing to build.

GoogleBooksClient appends the key only when non-blank, building the URL with
HttpUrl.Builder in a pure requestUrl() so it is testable without a socket. The
key is scrubbed from SourceResult.Failed.reason before that string can reach the
scan sheet — it is rendered to the user and is our only diagnostic channel from a
real phone, and some okhttp/JDK IOExceptions embed the full request URL in their
message. Defensive, not a response to an observed leak.

Resolves the RATE_LIMITED decision parked in RetryPolicy's KDoc: a keyed 429 is
the short per-user rate limit and gets exactly one retry, honouring Retry-After
capped at 2s. A keyless 429 is still the dead daily quota and is still never
retried.

Verified against the live API, not only offline: both ISBNs that failed on the
phone (9781883937386, 9781883937676) plus a control return HTTP 200, in the
percent-encoded URL shape HttpUrl actually produces. Both books are in Google
Books, so the restored fallback now covers precisely the Open Library TLS-reset
failure that broke those scans.

189 unit tests (was 172), 0 failures; Paparazzi unchanged; release APK builds.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J7WHnTx2Cso4VV245WDAJY
This commit is contained in:
Spriteandclaude committed 2026-09-11 23:34:43 +00:00
1 parent d6d02f788c
commit 486f6ebc48
11 files changed
+662 -37

No files matched your search

+21
View File
@@ -21,6 +21,24 @@ val keystoreProperties = Properties().apply {
}
val hasReleaseKeystore = keystorePropertiesFile.exists()
// Google Books API key. Lives in `local.properties` (gitignored) as
// GOOGLE_BOOKS_API_KEY=..., or in the environment for CI. Absent is a supported
// state: the build stays green and GoogleBooksClient falls back to the keyless
// endpoint, which is what a fresh clone without the key gets.
// The key is NOT a secret in the usual sense — it ships inside the APK and can be
// extracted — but it is restricted to the Books API, and it must never be
// committed. See docs/METADATA-SOURCES.md.
val localPropertiesFile = rootProject.file("local.properties")
val localProperties = Properties().apply {
if (localPropertiesFile.exists()) {
localPropertiesFile.inputStream().use { load(it) }
}
}
val googleBooksApiKey: String =
(localProperties["GOOGLE_BOOKS_API_KEY"] as String?)
?: System.getenv("GOOGLE_BOOKS_API_KEY")
?: ""
android {
namespace = "org.modg.bookshelf"
compileSdk = 37
@@ -34,6 +52,8 @@ android {
versionName = "1.0"
testInstrumentationRunner = "androidx.test.runner.AndroidJUnitRunner"
buildConfigField("String", "GOOGLE_BOOKS_API_KEY", "\"$googleBooksApiKey\"")
}
signingConfigs {
@@ -64,6 +84,7 @@ android {
buildFeatures {
compose = true
buildConfig = true
}
testOptions {
@@ -114,7 +114,7 @@ class AppContainer(private val context: Context) {
.build()
}
val metadataRepository by lazy { MetadataRepository(metadataHttpClient, json) }
val metadataRepository by lazy { MetadataRepository(metadataHttpClient, json, BuildConfig.GOOGLE_BOOKS_API_KEY) }
val syncEngine by lazy {
SyncEngine(
@@ -5,52 +5,83 @@ import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.withContext
import kotlinx.serialization.SerializationException
import kotlinx.serialization.json.Json
import okhttp3.HttpUrl
import okhttp3.OkHttpClient
import okhttp3.Request
/**
* Google Books lookup — SPEC.md "Book metadata lookup" fallback source. No API key.
* Google Books lookup — SPEC.md "Book metadata lookup" fallback source. Takes an
* [apiKey] (see `AppContainer` / `BuildConfig.GOOGLE_BOOKS_API_KEY`) because a
* keyless request is permanently rate-limited: every anonymous caller on the
* internet shares one exhausted daily quota (docs/METADATA-SOURCES.md). A blank
* key (the default) sends the request keyless, unchanged from before — this
* matters for a fresh clone with no key configured, which must still build a
* working app rather than a broken one.
* Never throws: every outcome, including transport failure, comes back as a
* [SourceResult] rather than a swallowed null.
*/
class GoogleBooksClient(
private val httpClient: OkHttpClient,
json: Json,
private val apiKey: String = "",
) {
private val json = Json(from = json) { ignoreUnknownKeys = true }
/**
* Retries transient failures per [RetryPolicy]. Note this source's standing
* failure — keyless requests share one exhausted global quota and answer 429,
* which [RetryPolicy] deliberately does NOT retry, so today this costs nothing
* and changes nothing here. See docs/METADATA-SOURCES.md.
* Retries transient failures per [RetryPolicy]. A keyless 429 is the exhausted
* global daily quota, which [RetryPolicy] deliberately does NOT retry — see its
* KDoc. A keyed 429 is the much shorter per-user rate limit and is worth one
* more ask, so [retryRateLimitedOnce] follows whether a key is configured.
*/
suspend fun lookup(isbn13: String): SourceResult =
withContext(Dispatchers.IO) { withRetry { fetch(isbn13) } }
withContext(Dispatchers.IO) {
withRetry(retryRateLimitedOnce = apiKey.isNotBlank()) { fetch(isbn13) }
}
/** Single un-retried attempt, for tests that need to count calls. */
internal suspend fun lookupOnce(isbn13: String): SourceResult =
withContext(Dispatchers.IO) { fetch(isbn13) }
private fun fetch(isbn13: String): SourceResult = try {
val request = Request.Builder()
.url("https://www.googleapis.com/books/v1/volumes?q=isbn:$isbn13")
.build()
httpClient.newCall(request).execute().use { response ->
classify(response.code, response.body.string())
private fun fetch(isbn13: String): SourceResult = redact(
try {
val request = Request.Builder().url(requestUrl(isbn13)).build()
httpClient.newCall(request).execute().use { response ->
classify(response.code, response.body.string(), response.header("Retry-After"))
}
} catch (e: IOException) {
SourceResult.fromException(e)
},
)
/**
* Package-visible pure function — no socket involved — so it's exhaustively
* unit-testable offline: the key is appended (URL-encoded) only when non-blank,
* and omitted entirely for the keyless default. Built with [HttpUrl.Builder]
* rather than string concatenation so query-parameter encoding is correct by
* construction rather than by hand.
*/
internal fun requestUrl(isbn13: String): String {
val builder = HttpUrl.Builder()
.scheme("https")
.host("www.googleapis.com")
.addPathSegments("books/v1/volumes")
.addQueryParameter("q", "isbn:$isbn13")
if (apiKey.isNotBlank()) {
builder.addQueryParameter("key", apiKey)
}
} catch (e: IOException) {
SourceResult.fromException(e)
return builder.build().toString()
}
/**
* Package-visible pure function — no socket involved — so it's exhaustively
* unit-testable offline (2xx-with-record, 2xx-without-record, 404, 429, 500,
* malformed body). [parseResponse] is defined in terms of this so the two
* can never disagree about what a body means.
* can never disagree about what a body means. [retryAfterHeader] is the raw
* `Retry-After` header value, if any — threaded through so the retry loop can
* see it, but only 429 ever consults it (via [SourceResult.fromHttpCode]).
*/
internal fun classify(httpCode: Int, body: String?): SourceResult {
if (httpCode !in 200..299) return SourceResult.fromHttpCode(httpCode)
internal fun classify(httpCode: Int, body: String?, retryAfterHeader: String? = null): SourceResult {
if (httpCode !in 200..299) return SourceResult.fromHttpCode(httpCode, retryAfterHeader)
if (body.isNullOrBlank()) return SourceResult.Failed("empty body", FailureKind.MALFORMED)
return try {
val dto = json.decodeFromString(GoogleBooksResponseDto.serializer(), body)
@@ -66,4 +97,20 @@ class GoogleBooksClient(
/** Package-visible for offline fixture tests — parses a raw response body with no network involved. */
internal fun parseResponse(body: String): BookMetadata? =
(classify(200, body) as? SourceResult.Found)?.metadata
/**
* [SourceResult.Failed.reason] is rendered on the scan sheet — our only
* diagnostic channel from a real phone (docs/METADATA-SOURCES.md) — so it must
* never carry the API key. Some okhttp/JDK IOExceptions embed the full request
* URL, key included, in their own message (`UnknownHostException`, SSL errors,
* and okhttp's own "Canceled" IOException variants differ by platform), so
* this scrubs defensively rather than trusting that no exception type ever
* will. Package-visible so the scrub itself is unit-testable without a socket.
*/
internal fun redact(result: SourceResult): SourceResult =
if (result is SourceResult.Failed && apiKey.isNotBlank() && result.reason.contains(apiKey)) {
result.copy(reason = result.reason.replace(apiKey, "[REDACTED]"))
} else {
result
}
}
@@ -16,9 +16,9 @@ class MetadataRepository(
private val openLibraryClient: OpenLibraryClient,
private val googleBooksClient: GoogleBooksClient,
) {
constructor(httpClient: OkHttpClient, json: Json) : this(
constructor(httpClient: OkHttpClient, json: Json, googleBooksApiKey: String = "") : this(
OpenLibraryClient(httpClient, json),
GoogleBooksClient(httpClient, json),
GoogleBooksClient(httpClient, json, googleBooksApiKey),
)
suspend fun lookup(isbn: String): LookupResult {
@@ -27,6 +27,15 @@ object RetryPolicy {
/** One original attempt plus two retries. Beyond this the marginal gain is noise. */
const val MAX_ATTEMPTS = 3
/**
* Cap on how long we'll wait on a keyed 429's `Retry-After` before treating it
* as not worth honouring, and also the fixed backoff used when that header is
* absent or unparseable. The user is standing at a bookshelf: a per-user rate
* limit clears fast, so there's no reason to wait longer than this for the one
* retry [withRetry] grants it (see [isRetryable]'s KDoc on RATE_LIMITED).
*/
const val RATE_LIMIT_RETRY_CAP_MILLIS = 2_000L
/**
* Stop starting NEW attempts once this much time has gone into a single source.
* A backstop against pathological cases (every attempt hitting the slow tail),
@@ -41,19 +50,37 @@ object RetryPolicy {
* [FailureKind.TRANSPORT] and [FailureKind.SERVER_ERROR] are transient and
* cheap to re-ask. The rest are not:
* - TIMEOUT — the budget is already spent; see the class KDoc.
* - RATE_LIMITED — the source is explicitly asking us to stop. Hammering a
* quota is how an intermittent block becomes a permanent one,
* and METADATA-SOURCES.md records that happening to this
* project's IP during research. When the Google Books API key
* lands, revisit this: a keyed 429 is a per-second rate limit
* and IS worth one Retry-After-respecting retry, unlike
* today's keyless daily-quota 429, which never clears.
* - RATE_LIMITED — the source is explicitly asking us to stop, and NOT
* retryable here either: a keyless 429 is the exhausted
* shared daily quota (METADATA-SOURCES.md), which never
* clears within a session, so hammering it is pure waste and
* risks turning an intermittent block into a permanent one.
* A KEYED 429 is a different animal — the much shorter
* per-user rate limit — and IS worth one retry, but that's a
* call only [GoogleBooksClient] can make (it knows whether a
* key is configured), so it opts in per-call via
* [withRetry]'s `retryRateLimitedOnce` rather than by
* changing this blanket answer.
* - CLIENT_ERROR — an identical request gets an identical answer.
* - MALFORMED — same bytes, same parse failure.
*/
fun isRetryable(kind: FailureKind): Boolean =
kind == FailureKind.TRANSPORT || kind == FailureKind.SERVER_ERROR
/**
* Parses an HTTP `Retry-After` header (the plain delta-seconds form; Google
* Books does not send the HTTP-date form) into a wait capped at
* [RATE_LIMIT_RETRY_CAP_MILLIS]. Returns null — "don't honour this" — if the
* header is missing, blank, negative, or not a plain integer; [withRetry]
* falls back to [RATE_LIMIT_RETRY_CAP_MILLIS] itself in that case, so either
* way the wait stays short and fixed rather than whatever the server asked for.
*/
fun retryAfterMillis(header: String?): Long? {
val seconds = header?.trim()?.toLongOrNull() ?: return null
if (seconds < 0) return null
return (seconds * 1000).coerceAtMost(RATE_LIMIT_RETRY_CAP_MILLIS)
}
/**
* Backoff before attempt number [nextAttempt] (2-based: the wait before the
* first retry is `backoffMillis(2)`). 250ms then 750ms, plus up to 40% jitter
@@ -82,10 +109,21 @@ object RetryPolicy {
*
* [sleep] and [nowMillis] are injectable purely so tests can run the real policy
* with no wall-clock delay; production callers use the defaults.
*
* [retryRateLimitedOnce] permits exactly one extra retry of a
* [FailureKind.RATE_LIMITED] failure, on top of whatever [RetryPolicy.isRetryable]
* already grants — see its KDoc. "Once" is enforced independently of
* [maxAttempts]: a second consecutive rate-limited failure always ends the loop,
* even if attempts remain in the budget. The wait before that one retry honours
* [SourceResult.Failed.retryAfterMillis] when the failure carries one, and
* [RetryPolicy.RATE_LIMIT_RETRY_CAP_MILLIS] otherwise — never the normal
* exponential [RetryPolicy.backoffMillis], which is tuned for transient transport
* errors, not for a server explicitly asking us to wait.
*/
suspend fun withRetry(
maxAttempts: Int = RetryPolicy.MAX_ATTEMPTS,
budgetMillis: Long = RetryPolicy.TOTAL_BUDGET_MILLIS,
retryRateLimitedOnce: Boolean = false,
random: Random = Random.Default,
nowMillis: () -> Long = { System.currentTimeMillis() },
sleep: suspend (Long) -> Unit = { delay(it) },
@@ -94,13 +132,21 @@ suspend fun withRetry(
val started = nowMillis()
var last: SourceResult = attempt()
var attemptsMade = 1
var rateLimitedRetryUsed = false
while (attemptsMade < maxAttempts) {
val failure = last as? SourceResult.Failed ?: return last
if (!RetryPolicy.isRetryable(failure.kind)) break
val rateLimitedRetry = failure.kind == FailureKind.RATE_LIMITED &&
retryRateLimitedOnce && !rateLimitedRetryUsed
if (!RetryPolicy.isRetryable(failure.kind) && !rateLimitedRetry) break
if (nowMillis() - started >= budgetMillis) break
sleep(RetryPolicy.backoffMillis(attemptsMade + 1, random))
if (rateLimitedRetry) {
rateLimitedRetryUsed = true
sleep(failure.retryAfterMillis ?: RetryPolicy.RATE_LIMIT_RETRY_CAP_MILLIS)
} else {
sleep(RetryPolicy.backoffMillis(attemptsMade + 1, random))
}
last = attempt()
attemptsMade++
}
@@ -53,9 +53,16 @@ sealed interface SourceResult {
* to the user as supplementary detail on the scan sheet and is our ONLY
* diagnostic channel from a real phone — so it names the specific failure, not
* a generic one. [kind] is the same fact in a form [RetryPolicy] can act on;
* nothing should ever parse [reason] to recover it.
* nothing should ever parse [reason] to recover it. [retryAfterMillis] is set
* only for a [FailureKind.RATE_LIMITED] failure whose response carried a usable
* `Retry-After` header (already parsed and capped — see [RetryPolicy]); a
* retryable failure without one falls back to a fixed backoff instead.
*/
data class Failed(val reason: String, val kind: FailureKind = FailureKind.TRANSPORT) : SourceResult
data class Failed(
val reason: String,
val kind: FailureKind = FailureKind.TRANSPORT,
val retryAfterMillis: Long? = null,
) : SourceResult
companion object {
/**
@@ -89,11 +96,19 @@ sealed interface SourceResult {
/**
* Maps a non-2xx HTTP status to a specific failure. 429 is called out
* separately from the rest of 4xx because it is the one client error that is
* about us rather than about the request, and because it is currently
* Google Books' permanent state — see docs/METADATA-SOURCES.md.
* about us rather than about the request, and because it is Google Books'
* permanent keyless state — see docs/METADATA-SOURCES.md. [retryAfterHeader]
* is the raw `Retry-After` header value, if any; only a 429 ever uses it, via
* [RetryPolicy.retryAfterMillis]. Passing it for another code is harmless
* (it's simply not consulted) — the parameter isn't restricted to 429 so
* callers don't need to know which codes care.
*/
fun fromHttpCode(code: Int): Failed = when {
code == 429 -> Failed("http 429 (rate limited)", FailureKind.RATE_LIMITED)
fun fromHttpCode(code: Int, retryAfterHeader: String? = null): Failed = when {
code == 429 -> Failed(
"http 429 (rate limited)",
FailureKind.RATE_LIMITED,
retryAfterMillis = RetryPolicy.retryAfterMillis(retryAfterHeader),
)
code in 500..599 -> Failed("http $code (server error)", FailureKind.SERVER_ERROR)
else -> Failed("http $code", FailureKind.CLIENT_ERROR)
}
@@ -3,7 +3,9 @@ package org.modg.bookshelf.data.metadata
import kotlinx.serialization.json.Json
import okhttp3.OkHttpClient
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertNull
import org.junit.Assert.assertTrue
import org.junit.Test
/**
@@ -87,6 +89,74 @@ class GoogleBooksClientTest {
assertEquals(SourceResult.Failed("malformed json", FailureKind.MALFORMED), result)
}
// --- requestUrl(): the key must be present (and encoded) only when configured. ---
@Test
fun `requestUrl omits the key entirely when it is blank -- the keyless default`() {
val url = client.requestUrl("9780134685991")
assertTrue(url.contains("9780134685991"))
assertFalse(url.contains("key="))
}
@Test
fun `requestUrl appends the key when one is configured`() {
val keyed = GoogleBooksClient(OkHttpClient(), Json, apiKey = "test-key-123")
val url = keyed.requestUrl("9780134685991")
assertTrue(url.contains("key=test-key-123"))
}
@Test
fun `requestUrl url-encodes a key that needs it`() {
val key = "a key/with&chars"
val keyed = GoogleBooksClient(OkHttpClient(), Json, apiKey = key)
val url = keyed.requestUrl("9780134685991")
assertFalse("raw key must not appear unescaped in the url", url.contains(key))
// "key=" is the last query parameter added, so everything after it is the
// encoded value -- decode it back and confirm it round-trips to the original,
// rather than pinning to one specific percent-encoding scheme.
val encodedKey = url.substringAfter("key=")
assertEquals(key, java.net.URLDecoder.decode(encodedKey, "UTF-8"))
}
// --- classify() threads the Retry-After header into a 429's retryAfterMillis. ---
@Test
fun `classify carries a parsed Retry-After into the 429 failure`() {
val result = client.classify(429, null, retryAfterHeader = "1") as SourceResult.Failed
assertEquals(1_000L, result.retryAfterMillis)
}
@Test
fun `classify leaves retryAfterMillis null when no header is given`() {
val result = client.classify(429, null) as SourceResult.Failed
assertNull(result.retryAfterMillis)
}
// --- redact(): the API key must never survive into a shown/logged reason. ---
@Test
fun `redact scrubs the api key out of a reason that leaked the full keyed request url`() {
val key = "test-key-123"
val keyed = GoogleBooksClient(OkHttpClient(), Json, apiKey = key)
val leakedUrl = keyed.requestUrl("9780134685991")
check(leakedUrl.contains(key)) { "fixture assumption broken: url doesn't contain the key" }
val leaking = SourceResult.Failed("network error: connect to $leakedUrl failed", FailureKind.TRANSPORT)
val redacted = keyed.redact(leaking) as SourceResult.Failed
assertFalse("literal key must not survive redaction", redacted.reason.contains(key))
assertTrue(redacted.reason.contains("[REDACTED]"))
}
@Test
fun `redact is a no-op when there is no key configured or nothing to scrub`() {
val clean = SourceResult.Failed("tls connection reset", FailureKind.TRANSPORT)
assertEquals(clean, client.redact(clean))
}
private fun fixture(name: String): String =
checkNotNull(javaClass.classLoader.getResourceAsStream("fixtures/$name")) { "missing fixture $name" }
.bufferedReader()
@@ -131,6 +131,101 @@ class RetryPolicyTest {
assertEquals(transport, result)
}
// --- a keyed 429 gets exactly one extra retry; a keyless one gets none ---
@Test
fun `a keyed 429 is retried exactly once`() = runTest {
var calls = 0
val result = withRetry(retryRateLimitedOnce = true, sleep = {}) {
calls++
if (calls == 1) SourceResult.fromHttpCode(429) else found
}
assertEquals(found, result)
assertEquals(2, calls)
}
@Test
fun `a keyless 429 is not retried at all`() = runTest {
var calls = 0
val result = withRetry(retryRateLimitedOnce = false, sleep = {}) { calls++; SourceResult.fromHttpCode(429) }
assertEquals(1, calls)
assertEquals(FailureKind.RATE_LIMITED, (result as SourceResult.Failed).kind)
}
@Test
fun `a second consecutive 429 ends it -- one retry means one, not up to MAX_ATTEMPTS`() = runTest {
var calls = 0
val result = withRetry(retryRateLimitedOnce = true, sleep = {}) { calls++; SourceResult.fromHttpCode(429) }
assertEquals(2, calls)
assertEquals(
SourceResult.Failed("http 429 (rate limited), 2 attempts", FailureKind.RATE_LIMITED),
result,
)
}
@Test
fun `the rate-limited retry waits on Retry-After when present and short`() = runTest {
val sleeps = mutableListOf<Long>()
var calls = 0
withRetry(retryRateLimitedOnce = true, sleep = { sleeps.add(it) }) {
calls++
if (calls == 1) SourceResult.fromHttpCode(429, "1") else found
}
assertEquals(listOf(1_000L), sleeps)
}
@Test
fun `the rate-limited retry caps a long Retry-After instead of honouring it`() = runTest {
val sleeps = mutableListOf<Long>()
var calls = 0
withRetry(retryRateLimitedOnce = true, sleep = { sleeps.add(it) }) {
calls++
if (calls == 1) SourceResult.fromHttpCode(429, "120") else found
}
assertEquals(listOf(RetryPolicy.RATE_LIMIT_RETRY_CAP_MILLIS), sleeps)
}
@Test
fun `the rate-limited retry falls back to the cap when Retry-After is absent`() = runTest {
val sleeps = mutableListOf<Long>()
var calls = 0
withRetry(retryRateLimitedOnce = true, sleep = { sleeps.add(it) }) {
calls++
if (calls == 1) SourceResult.fromHttpCode(429) else found
}
assertEquals(listOf(RetryPolicy.RATE_LIMIT_RETRY_CAP_MILLIS), sleeps)
}
@Test
fun `the rate-limited retry falls back to the cap when Retry-After is unparseable`() = runTest {
val sleeps = mutableListOf<Long>()
var calls = 0
withRetry(retryRateLimitedOnce = true, sleep = { sleeps.add(it) }) {
calls++
if (calls == 1) SourceResult.fromHttpCode(429, "not-a-number") else found
}
assertEquals(listOf(RetryPolicy.RATE_LIMIT_RETRY_CAP_MILLIS), sleeps)
}
// --- Retry-After parsing itself ---
@Test
fun `retryAfterMillis honours a short header exactly`() {
assertEquals(1_000L, RetryPolicy.retryAfterMillis("1"))
}
@Test
fun `retryAfterMillis caps a long header at the rate-limit cap`() {
assertEquals(RetryPolicy.RATE_LIMIT_RETRY_CAP_MILLIS, RetryPolicy.retryAfterMillis("120"))
}
@Test
fun `retryAfterMillis is null when the header is absent, unparseable, or negative`() {
assertEquals(null, RetryPolicy.retryAfterMillis(null))
assertEquals(null, RetryPolicy.retryAfterMillis("soon"))
assertEquals(null, RetryPolicy.retryAfterMillis("-5"))
}
@Test
fun `backoff is short and jittered, never zero and never seconds long`() {
val r = Random(1234)