diff options
17 files changed, 495 insertions, 84 deletions
diff --git a/.coverage-baseline b/.coverage-baseline index 7de30c1..b0b26f7 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -94.16 +94.23 diff --git a/app/src/main/java/org/terst/listen/download/EpisodeDownloader.kt b/app/src/main/java/org/terst/listen/download/EpisodeDownloader.kt index dfc4aad..04ca9a3 100644 --- a/app/src/main/java/org/terst/listen/download/EpisodeDownloader.kt +++ b/app/src/main/java/org/terst/listen/download/EpisodeDownloader.kt @@ -9,6 +9,7 @@ import org.terst.listen.data.EpisodeDao import org.terst.listen.data.EpisodeEntity import org.terst.listen.data.ProbedVideo import org.terst.listen.data.SubscriptionKind +import org.terst.listen.server.ServerAnswer import org.terst.listen.server.ServerEpisode import java.io.File import java.io.IOException @@ -40,8 +41,8 @@ interface ServerAudio { */ suspend fun canAsk(episode: EpisodeEntity): Boolean - /** What the server has for this episode, or null if it has no answer right now. */ - suspend fun resolve(episode: EpisodeEntity): ServerEpisode? + /** What the server has for this episode, or why it has no answer. */ + suspend fun resolve(episode: EpisodeEntity): ServerAnswer /** Records the server's offsets, once its audio is actually the file on disk. */ suspend fun record(episode: EpisodeEntity, info: ServerEpisode) @@ -157,9 +158,12 @@ class EpisodeDownloader( * The publisher serves a different file on every request, so skips measured on the server's * copy are wrong for anything else. Falling back silently therefore produced an episode whose * skips could never apply, and the owner has ruled that out: with a server configured, an - * episode it cannot supply yet (pending, unreachable, a bad token, an unreadable answer, or - * audio that then fails to arrive) fails with [WaitingForServerException] and is retried on - * the next refresh. Taking the publisher's copy is [downloadFromPublisher], on request. + * episode it cannot supply yet (pending, unreachable, an unreadable answer, or audio that + * then fails to arrive) fails with [WaitingForServerException] and is retried on the next + * refresh. Taking the publisher's copy is [downloadFromPublisher], on request. + * + * The exception is a token the server refuses (HTTP 401/403). That is not transient, so it + * is treated as no server at all and downloads from the publisher at once. * * With no server configured, or no feed to identify the episode to it, there is nothing to * wait for, and this downloads from the publisher exactly as the app did before the server @@ -171,7 +175,14 @@ class EpisodeDownloader( return downloadOverHttp(episode, episode.mediaUrl).map { AudioResult(it, fromServer = false) } } - val info = server.resolve(episode) + val answer = server.resolve(episode) + // A refused token is a configuration error and will be refused again on every refresh, so + // waiting for it would mean nothing ever downloads. It takes the same direct path as + // having no server, and is recorded as the publisher's audio like any other. + if (answer == ServerAnswer.Rejected) { + return downloadOverHttp(episode, episode.mediaUrl).map { AudioResult(it, fromServer = false) } + } + val info = (answer as? ServerAnswer.Found)?.episode val serverUrl = info?.audioUrl?.takeIf { info.ready } ?: return Result.failure(WaitingForServerException()) diff --git a/app/src/main/java/org/terst/listen/download/WaitingForServerText.kt b/app/src/main/java/org/terst/listen/download/WaitingForServerText.kt index 70c68c2..2b93e23 100644 --- a/app/src/main/java/org/terst/listen/download/WaitingForServerText.kt +++ b/app/src/main/java/org/terst/listen/download/WaitingForServerText.kt @@ -2,6 +2,7 @@ package org.terst.listen.download import org.terst.listen.data.DownloadState import org.terst.listen.data.EpisodeEntity +import org.terst.listen.server.SkipServerConfig /** * Everything the app says about an episode waiting for the skip server's copy, in one testable @@ -40,3 +41,23 @@ object WaitingForServerText { null } } + +/** + * What the settings screen says when the skip server has refused the token. + * + * Kept out of the composable for the same reason as [WaitingForServerText]. + */ +object RejectedTokenText { + + /** Said where the server is configured: what happened, what it means meanwhile, what to do. */ + const val NOTICE = + "The server rejected the token. Episodes are downloading from their publishers " + + "meanwhile, so skips will not apply to them. Check the token and save it again." + + /** + * The notice to show, or null. Shown only while the saved configuration is the very one the + * server refused, so saving a different token clears it at once, before any request is made. + */ + fun noticeFor(rejected: SkipServerConfig?, current: SkipServerConfig?): String? = + if (rejected != null && rejected == current) NOTICE else null +} diff --git a/app/src/main/java/org/terst/listen/server/OkHttpSkipTransport.kt b/app/src/main/java/org/terst/listen/server/OkHttpSkipTransport.kt index 1541dfb..acf310a 100644 --- a/app/src/main/java/org/terst/listen/server/OkHttpSkipTransport.kt +++ b/app/src/main/java/org/terst/listen/server/OkHttpSkipTransport.kt @@ -6,6 +6,7 @@ import okhttp3.MediaType.Companion.toMediaType import okhttp3.OkHttpClient import okhttp3.Request import okhttp3.RequestBody.Companion.toRequestBody +import okhttp3.Response /** * [SkipTransport] over the app's own OkHttp client. @@ -13,26 +14,27 @@ import okhttp3.RequestBody.Companion.toRequestBody * On the framework-adapter exclusion list: there is no decision in here, only the HTTP that * [SkipServer]'s logic is deliberately kept free of. * - * Every failure is a null or a false rather than an exception. The server is an improvement the - * app prefers, not something it depends on -- so unreachable, slow and broken all have to mean - * the same thing to the caller as not configured: carry on without it. + * Every failure is a [SkipReply] rather than an exception, and the adapter only reports what + * happened: an answer, a refusal of the credentials (401/403), or no answer. What each means is + * for [SkipServer] and its callers to decide. */ class OkHttpSkipTransport(private val client: OkHttpClient) : SkipTransport { - override suspend fun get(url: String, token: String): String? = withContext(Dispatchers.IO) { + override suspend fun get(url: String, token: String): SkipReply = withContext(Dispatchers.IO) { runCatching { val request = Request.Builder() .url(url) .header("Authorization", "Bearer $token") .build() client.newCall(request).execute().use { response -> - if (!response.isSuccessful) return@use null - response.body?.string() + if (response.isCredentialRefusal()) return@use SkipReply.Rejected + if (!response.isSuccessful) return@use SkipReply.NoAnswer + response.body?.string()?.let { SkipReply.Body(it) } ?: SkipReply.NoAnswer } - }.getOrNull() + }.getOrDefault(SkipReply.NoAnswer) } - override suspend fun post(url: String, token: String, body: String): Boolean = + override suspend fun post(url: String, token: String, body: String): SkipReply = withContext(Dispatchers.IO) { runCatching { val request = Request.Builder() @@ -40,11 +42,22 @@ class OkHttpSkipTransport(private val client: OkHttpClient) : SkipTransport { .header("Authorization", "Bearer $token") .post(body.toRequestBody(JSON)) .build() - client.newCall(request).execute().use { it.isSuccessful } - }.getOrDefault(false) + client.newCall(request).execute().use { response -> + when { + response.isCredentialRefusal() -> SkipReply.Rejected + response.isSuccessful -> SkipReply.Body("") + else -> SkipReply.NoAnswer + } + } + }.getOrDefault(SkipReply.NoAnswer) } + /** 401 and 403, which is what a refused token looks like. 404 and 5xx are not credentials. */ + private fun Response.isCredentialRefusal() = code == HTTP_UNAUTHORIZED || code == HTTP_FORBIDDEN + private companion object { + const val HTTP_UNAUTHORIZED = 401 + const val HTTP_FORBIDDEN = 403 val JSON = "application/json; charset=utf-8".toMediaType() } } diff --git a/app/src/main/java/org/terst/listen/server/ServerSkips.kt b/app/src/main/java/org/terst/listen/server/ServerSkips.kt index 2a182cf..608521a 100644 --- a/app/src/main/java/org/terst/listen/server/ServerSkips.kt +++ b/app/src/main/java/org/terst/listen/server/ServerSkips.kt @@ -33,9 +33,9 @@ class ServerSkips( * moved still matches on its enclosure URL, and failing that on its title. An episode that * loses its identity loses its offsets. */ - override suspend fun resolve(episode: EpisodeEntity): ServerEpisode? { - val feedUrl = subscriptions.getById(episode.subscriptionId)?.feedUrl ?: return null - return server.episode( + override suspend fun resolve(episode: EpisodeEntity): ServerAnswer { + val feedUrl = subscriptions.getById(episode.subscriptionId)?.feedUrl ?: return ServerAnswer.Unavailable + return server.ask( feedUrl = feedUrl, guid = episode.guid, enclosureUrl = episode.mediaUrl, diff --git a/app/src/main/java/org/terst/listen/server/SkipRefresher.kt b/app/src/main/java/org/terst/listen/server/SkipRefresher.kt index 4d490f6..12863f7 100644 --- a/app/src/main/java/org/terst/listen/server/SkipRefresher.kt +++ b/app/src/main/java/org/terst/listen/server/SkipRefresher.kt @@ -67,7 +67,7 @@ class SkipRefresher( // discarded, same reasoning as skipFurniture()'s catch just above. @Suppress("TooGenericExceptionCaught") try { - serverAudio.resolve(episode)?.let { info -> serverAudio.record(episode, info) } + (serverAudio.resolve(episode) as? ServerAnswer.Found)?.let { serverAudio.record(episode, it.episode) } } catch (e: CancellationException) { throw e } catch (e: Exception) { diff --git a/app/src/main/java/org/terst/listen/server/SkipServer.kt b/app/src/main/java/org/terst/listen/server/SkipServer.kt index 69dbeec..990b758 100644 --- a/app/src/main/java/org/terst/listen/server/SkipServer.kt +++ b/app/src/main/java/org/terst/listen/server/SkipServer.kt @@ -1,5 +1,7 @@ package org.terst.listen.server +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow import kotlinx.serialization.SerialName import kotlinx.serialization.Serializable import kotlinx.serialization.json.Json @@ -14,13 +16,40 @@ import kotlinx.serialization.json.JsonPrimitive */ data class SkipServerConfig(val baseUrl: String, val token: String) +/** + * What the transport saw, with no opinion about what it means. + * + * Three outcomes rather than a nullable body because the callers must treat two kinds of + * failure oppositely: a server that did not answer is worth waiting for, while one that + * rejected the credentials will reject them again on the next refresh. + */ +sealed interface SkipReply { + /** A successful response and its body (empty for a post that returns nothing). */ + data class Body(val text: String) : SkipReply + + /** The server answered HTTP 401 or 403: it heard us and refused the credentials. */ + data object Rejected : SkipReply + + /** Anything else: unreachable, timed out, TLS failure, any other status, no body. */ + data object NoAnswer : SkipReply +} + /** The HTTP the client needs, behind an interface so its logic is testable off-device. */ interface SkipTransport { - /** Returns the response body, or null for any failure. */ - suspend fun get(url: String, token: String): String? + suspend fun get(url: String, token: String): SkipReply - /** Returns whether the post succeeded. */ - suspend fun post(url: String, token: String, body: String): Boolean + suspend fun post(url: String, token: String, body: String): SkipReply +} + +/** What asking the server about one episode came to. */ +sealed interface ServerAnswer { + data class Found(val episode: ServerEpisode) : ServerAnswer + + /** The server refused the token. A configuration error, not a transient one. */ + data object Rejected : ServerAnswer + + /** No usable answer right now: not configured, unreachable, or a response that did not parse. */ + data object Unavailable : ServerAnswer } /** One stretch of an episode to skip. Deserialised straight off the wire -- the shape the @@ -102,6 +131,19 @@ class SkipServer( * and the download path must treat the two oppositely: no server means download from the * publisher as the app always did, while a server that merely did not answer means wait. */ + private val refused = MutableStateFlow<SkipServerConfig?>(null) + + /** + * The configuration the server last refused, or null. + * + * Remembered so the settings screen can say so: a refused token otherwise changes nothing + * visible, and episodes quietly come from publishers. Kept as the refused configuration + * rather than a flag so it stops applying the moment a different one is saved, and cleared + * by the next successful reply. Held in memory: it is rediscovered by the first request of + * the next refresh, which is soon enough for something that cannot fix itself. + */ + val rejected: StateFlow<SkipServerConfig?> = refused + suspend fun isConfigured(): Boolean = config() != null @@ -122,7 +164,7 @@ class SkipServer( } append("]}") } - return transport.post(base(target) + "/api/subscriptions", target.token, body) + return observe(target, transport.post(base(target) + "/api/subscriptions", target.token, body)) != null } /** @@ -137,8 +179,19 @@ class SkipServer( guid: String, enclosureUrl: String, title: String, - ): ServerEpisode? { - val target = config() ?: return null + ): ServerEpisode? = (ask(feedUrl, guid, enclosureUrl, title) as? ServerAnswer.Found)?.episode + + /** + * As [episode], but says why there was no answer when the reason is a refused token -- the + * one failure the download path must not wait out. + */ + suspend fun ask( + feedUrl: String, + guid: String, + enclosureUrl: String, + title: String, + ): ServerAnswer { + val target = config() ?: return ServerAnswer.Unavailable val query = buildString { append("?feed=").append(encode(feedUrl)) append("&guid=").append(encode(guid)) @@ -146,10 +199,16 @@ class SkipServer( append("&title=").append(encode(title)) } - val body = transport.get(base(target) + "/api/episode" + query, target.token) ?: return null - val parsed = runCatching { lenient.decodeFromString<EpisodeResponse>(body) }.getOrNull() ?: return null + val reply = transport.get(base(target) + "/api/episode" + query, target.token) + val body = observe(target, reply) ?: return if (reply == SkipReply.Rejected) { + ServerAnswer.Rejected + } else { + ServerAnswer.Unavailable + } + val parsed = runCatching { lenient.decodeFromString<EpisodeResponse>(body) }.getOrNull() + ?: return ServerAnswer.Unavailable - return ServerEpisode( + return ServerAnswer.Found(ServerEpisode( ready = parsed.status == "ready" && parsed.audioUrl != null, audioUrl = parsed.audioUrl?.let { absolute(base(target), it) }, durationMs = parsed.durationMs?.takeIf { it > 0 }, @@ -163,7 +222,22 @@ class SkipServer( foundAdMs = parsed.foundAdMs, insertedAdMs = parsed.insertedAdMs, adsRefused = parsed.adsRefused, - ) + )) + } + + /** + * The body of a successful reply, or null; and the one place a reply updates [rejected]. + * + * Only a refusal sets it and only a success clears it. No answer says nothing about the + * token, so it must not hide a refusal already seen, nor invent one. + */ + private fun observe(target: SkipServerConfig, reply: SkipReply): String? { + when (reply) { + is SkipReply.Rejected -> refused.value = target + is SkipReply.Body -> refused.value = null + is SkipReply.NoAnswer -> Unit + } + return (reply as? SkipReply.Body)?.text } private fun base(target: SkipServerConfig) = target.baseUrl.trimEnd('/') diff --git a/app/src/main/java/org/terst/listen/ui/SettingsScreen.kt b/app/src/main/java/org/terst/listen/ui/SettingsScreen.kt index 50a03cd..cf9ca00 100644 --- a/app/src/main/java/org/terst/listen/ui/SettingsScreen.kt +++ b/app/src/main/java/org/terst/listen/ui/SettingsScreen.kt @@ -35,6 +35,7 @@ import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember import androidx.compose.runtime.setValue import androidx.compose.ui.Modifier +import androidx.compose.ui.graphics.Color import androidx.compose.ui.unit.dp /** Offered speeds. 1.0x stays in the list as the way back to normal. */ @@ -117,6 +118,8 @@ fun SettingsScreen( data class SkipServerSettings( val url: String, val token: String, + /** Set while the server is refusing the saved token; see [RejectedTokenText]. */ + val rejectedNotice: String?, val onSave: (url: String, token: String) -> Unit, ) @@ -155,12 +158,14 @@ private fun SkipServerSection(settings: SkipServerSettings, furniture: Furniture ) Button(onClick = { settings.onSave(url, token) }) { Text("Save") } Text( - if (settings.url.isBlank()) { - "Not set. Episodes download from publishers and play without detected skips." - } else { - "Episodes download from the server, which is also what analysed them." - }, + settings.rejectedNotice + ?: if (settings.url.isBlank()) { + "Not set. Episodes download from publishers and play without detected skips." + } else { + "Episodes download from the server, which is also what analysed them." + }, style = MaterialTheme.typography.bodySmall, + color = if (settings.rejectedNotice != null) MaterialTheme.colorScheme.error else Color.Unspecified, ) SwitchRow( label = "Skip the show's own intro and credits", diff --git a/app/src/main/java/org/terst/listen/ui/SkipServerSettingsState.kt b/app/src/main/java/org/terst/listen/ui/SkipServerSettingsState.kt index 3dceb80..5aba893 100644 --- a/app/src/main/java/org/terst/listen/ui/SkipServerSettingsState.kt +++ b/app/src/main/java/org/terst/listen/ui/SkipServerSettingsState.kt @@ -8,6 +8,8 @@ import androidx.compose.runtime.rememberCoroutineScope import androidx.compose.ui.platform.LocalContext import kotlinx.coroutines.launch import org.terst.listen.data.Graph +import org.terst.listen.download.RejectedTokenText +import org.terst.listen.server.SkipServerConfig /** * Reads and writes the skip server's address and token. @@ -25,9 +27,13 @@ internal fun rememberSkipServerSettings(): SkipServerSettings { val url by preferences.serverUrl.collectAsState(initial = "") val token by preferences.serverToken.collectAsState(initial = "") + val rejected by Graph.get(context).skipServer.rejected.collectAsState() + val configured = if (url.isBlank() || token.isBlank()) null else SkipServerConfig(url, token) + return SkipServerSettings( url = url, token = token, + rejectedNotice = RejectedTokenText.noticeFor(rejected, configured), onSave = { newUrl, newToken -> scope.launch { preferences.setServer(newUrl, newToken) } }, diff --git a/app/src/test/java/org/terst/listen/download/EpisodeDownloaderTest.kt b/app/src/test/java/org/terst/listen/download/EpisodeDownloaderTest.kt index 46e15c0..bbb29af 100644 --- a/app/src/test/java/org/terst/listen/download/EpisodeDownloaderTest.kt +++ b/app/src/test/java/org/terst/listen/download/EpisodeDownloaderTest.kt @@ -17,6 +17,7 @@ import org.terst.listen.data.DownloadState import org.terst.listen.data.EpisodeEntity import org.terst.listen.data.FakeEpisodeDao import org.terst.listen.data.SubscriptionKind +import org.terst.listen.server.ServerAnswer import org.terst.listen.server.ServerEpisode import java.io.File @@ -175,10 +176,16 @@ class EpisodeDownloaderTest { private val info: ServerEpisode?, /** Whether a server is configured at all -- a different thing from it having no answer. */ private val configured: Boolean = true, + /** The server refusing the token, which is not the same as having no answer. */ + private val rejected: Boolean = false, ) : ServerAudio { var recorded: Pair<String, ServerEpisode>? = null override suspend fun canAsk(episode: EpisodeEntity) = configured - override suspend fun resolve(episode: EpisodeEntity) = info + override suspend fun resolve(episode: EpisodeEntity): ServerAnswer = when { + rejected -> ServerAnswer.Rejected + info != null -> ServerAnswer.Found(info) + else -> ServerAnswer.Unavailable + } override suspend fun record(episode: EpisodeEntity, info: ServerEpisode) { recorded = episode.id to info } @@ -310,6 +317,55 @@ class EpisodeDownloaderTest { assertNull("offsets describe the server's copy, which never landed", audio.recorded) } + // ---- a refused token is a configuration error, not a wait ---- + + private fun assertDownloadedFromPublisher(result: Result<File>) { + assertTrue(result.isSuccess) + val stored = dao.rows.single() + assertEquals(DownloadState.DOWNLOADED, stored.downloadState) + assertFalse("the publisher's copy must not carry the server's skips", stored.audioFromServer) + assertEquals("/original.mp3", server.takeRequest().path) + } + + @Test + fun `a rejected token downloads from the publisher at once and is not marked as the server's`() = runTest { + server.enqueue(MockResponse().setBody("publisher-bytes")) + val audio = FakeServerAudio(null, rejected = true) + val ep = episode(url = server.url("/original.mp3").toString()).also { dao.rows += it } + + assertDownloadedFromPublisher(withServer(audio).download(ep, SubscriptionKind.PODCAST)) + + assertNull("nothing from the server may be recorded against the publisher's audio", audio.recorded) + } + + @Test + fun `a rejected token clears an earlier server provenance claim`() = runTest { + server.enqueue(MockResponse().setBody("publisher-bytes")) + val ep = episode(url = server.url("/original.mp3").toString()).copy(audioFromServer = true) + .also { dao.rows += it } + + withServer(FakeServerAudio(null, rejected = true)).download(ep, SubscriptionKind.PODCAST) + + assertFalse(dao.rows.single().audioFromServer) + } + + @Test + fun `an extracted video does not consult a rejected server either`() = runTest { + val extractor = object : ExtractedAudioSource { + override suspend fun download(url: String, targetDir: File) = + File(targetDir, "v.m4a").also { it.writeText("video-audio") } + + override suspend fun probe(url: String): org.terst.listen.data.ProbedVideo? = null + } + val youtube = EpisodeDownloader( + OkHttpClient(), dao, library, extractor, FakeServerAudio(null, rejected = true), + ) { 1_000L } + val ep = episode(url = "https://www.youtube.com/watch?v=abc").also { dao.rows += it } + + assertTrue(youtube.download(ep, SubscriptionKind.YOUTUBE_CHANNEL).isSuccess) + assertFalse(dao.rows.single().audioFromServer) + } + @Test fun `an extracted video never waits even when a server is configured`() = runTest { val extractor = object : ExtractedAudioSource { diff --git a/app/src/test/java/org/terst/listen/download/WaitForServerWiringTest.kt b/app/src/test/java/org/terst/listen/download/WaitForServerWiringTest.kt index 6162cae..74c04da 100644 --- a/app/src/test/java/org/terst/listen/download/WaitForServerWiringTest.kt +++ b/app/src/test/java/org/terst/listen/download/WaitForServerWiringTest.kt @@ -23,6 +23,7 @@ import org.terst.listen.data.SubscriptionKind import org.terst.listen.server.ServerSkips import org.terst.listen.server.SkipServer import org.terst.listen.server.SkipServerConfig +import org.terst.listen.server.SkipReply import org.terst.listen.server.SkipTransport /** @@ -41,14 +42,16 @@ class WaitForServerWiringTest { private lateinit var dao: FakeEpisodeDao private lateinit var subscriptions: FakeSubscriptionDao - private class Transport(private val body: String?) : SkipTransport { + private class Transport(private val reply: SkipReply) : SkipTransport { + constructor(body: String?) : this(body?.let { SkipReply.Body(it) } ?: SkipReply.NoAnswer) + var gets = 0 - override suspend fun get(url: String, token: String): String? { + override suspend fun get(url: String, token: String): SkipReply { gets++ - return body + return reply } - override suspend fun post(url: String, token: String, body: String) = true + override suspend fun post(url: String, token: String, body: String) = SkipReply.Body("") } @Before @@ -113,6 +116,86 @@ class WaitForServerWiringTest { assertEquals("the publisher must not be contacted", 0, publisher.requestCount) } + private fun assertDownloadsFromPublisher(reply: SkipReply) = runTest { + publisher.enqueue(MockResponse().setBody("publisher-bytes")) + val transport = Transport(reply) + + val result = downloader(transport, configured = true).download(episode(), SubscriptionKind.PODCAST) + + assertTrue(result.isSuccess) + val stored = dao.rows.single() + assertEquals(DownloadState.DOWNLOADED, stored.downloadState) + assertEquals(false, stored.audioFromServer) + assertEquals("/orig.mp3", publisher.takeRequest().path) + assertEquals(1, transport.gets) + } + + @Test fun `a rejected token downloads from the publisher`() = assertDownloadsFromPublisher(SkipReply.Rejected) + + // The same thing through the real transport, so the status-to-outcome mapping and the + // decision are tested together: 401 and 403 download, every other failure waits. + + private fun skipServerAnswering(vararg codes: Int): Pair<MockWebServer, EpisodeDownloader> { + val skip = MockWebServer().apply { start() } + codes.forEach { skip.enqueue(MockResponse().setResponseCode(it)) } + val skipServer = SkipServer( + { SkipServerConfig(skip.url("/").toString(), "tok") }, + org.terst.listen.server.OkHttpSkipTransport(OkHttpClient()), + ) + val fingerprints = FakeShowFingerprintDao().apply { episodeRows = dao.rows } + val serverSkips = ServerSkips(skipServer, subscriptions, fingerprints, FakePlaybackStateDao(dao.rows), FakeMidrollDao()) + return skip to EpisodeDownloader(OkHttpClient(), dao, temp.newFolder(), null, serverSkips) { 1L } + } + + private fun assertStatusDownloadsFromPublisher(code: Int) = runTest { + publisher.enqueue(MockResponse().setBody("publisher-bytes")) + val (skip, downloader) = skipServerAnswering(code) + try { + assertTrue(downloader.download(episode(), SubscriptionKind.PODCAST).isSuccess) + assertEquals(DownloadState.DOWNLOADED, dao.rows.single().downloadState) + assertEquals(false, dao.rows.single().audioFromServer) + } finally { + skip.shutdown() + } + } + + private fun assertStatusWaits(code: Int) = runTest { + val (skip, downloader) = skipServerAnswering(code) + try { + assertTrue(downloader.download(episode(), SubscriptionKind.PODCAST).isFailure) + assertEquals(DownloadState.WAITING_FOR_SERVER, dao.rows.single().downloadState) + assertEquals("the publisher must not be contacted", 0, publisher.requestCount) + } finally { + skip.shutdown() + } + } + + @Test fun `HTTP 401 downloads from the publisher`() = assertStatusDownloadsFromPublisher(401) + + @Test fun `HTTP 403 downloads from the publisher`() = assertStatusDownloadsFromPublisher(403) + + @Test fun `HTTP 404 waits`() = assertStatusWaits(404) + + @Test fun `HTTP 500 waits`() = assertStatusWaits(500) + + @Test fun `HTTP 503 waits`() = assertStatusWaits(503) + + @Test fun `a connection failure waits`() = runTest { + val skipServer = SkipServer( + { SkipServerConfig("http://127.0.0.1:1", "tok") }, + org.terst.listen.server.OkHttpSkipTransport(OkHttpClient()), + ) + val fingerprints = FakeShowFingerprintDao().apply { episodeRows = dao.rows } + val serverSkips = ServerSkips(skipServer, subscriptions, fingerprints, FakePlaybackStateDao(dao.rows), FakeMidrollDao()) + + val result = EpisodeDownloader(OkHttpClient(), dao, temp.newFolder(), null, serverSkips) { 1L } + .download(episode(), SubscriptionKind.PODCAST) + + assertTrue(result.isFailure) + assertEquals(DownloadState.WAITING_FOR_SERVER, dao.rows.single().downloadState) + assertEquals(0, publisher.requestCount) + } + @Test fun `an unreachable server waits`() = assertWaitsWithoutPublisher(null) @Test fun `a response that will not parse waits`() = assertWaitsWithoutPublisher("<html>502 bad gateway") diff --git a/app/src/test/java/org/terst/listen/download/WaitingDownloadsTest.kt b/app/src/test/java/org/terst/listen/download/WaitingDownloadsTest.kt index 465af46..ef37cfb 100644 --- a/app/src/test/java/org/terst/listen/download/WaitingDownloadsTest.kt +++ b/app/src/test/java/org/terst/listen/download/WaitingDownloadsTest.kt @@ -24,6 +24,7 @@ import org.terst.listen.data.FakeEpisodeDao import org.terst.listen.data.FakeSubscriptionDao import org.terst.listen.data.SubscriptionEntity import org.terst.listen.data.SubscriptionKind +import org.terst.listen.server.ServerAnswer import org.terst.listen.server.ServerEpisode import java.io.File @@ -67,7 +68,8 @@ class WaitingDownloadsTest { private class ScriptedServer(private val answers: MutableMap<String, ServerEpisode?>) : ServerAudio { val recorded = mutableListOf<String>() override suspend fun canAsk(episode: EpisodeEntity) = true - override suspend fun resolve(episode: EpisodeEntity) = answers[episode.id] + override suspend fun resolve(episode: EpisodeEntity): ServerAnswer = + answers[episode.id]?.let { ServerAnswer.Found(it) } ?: ServerAnswer.Unavailable override suspend fun record(episode: EpisodeEntity, info: ServerEpisode) { recorded += episode.id } diff --git a/app/src/test/java/org/terst/listen/download/WaitingForServerTextTest.kt b/app/src/test/java/org/terst/listen/download/WaitingForServerTextTest.kt index ed06106..8ceac26 100644 --- a/app/src/test/java/org/terst/listen/download/WaitingForServerTextTest.kt +++ b/app/src/test/java/org/terst/listen/download/WaitingForServerTextTest.kt @@ -6,6 +6,7 @@ import org.junit.Assert.assertTrue import org.junit.Test import org.terst.listen.data.DownloadState import org.terst.listen.data.EpisodeEntity +import org.terst.listen.server.SkipServerConfig class WaitingForServerTextTest { @@ -32,4 +33,34 @@ class WaitingForServerTextTest { assertNull("no offer in $it", WaitingForServerText.publisherOffer(waitingEpisode().copy(downloadState = it))) } } + + private val saved = SkipServerConfig("https://listen.terst.org", "stale") + + @Test + fun theRejectedTokenNoticeSaysWhatHappenedWhatItMeansAndWhatToDo() { + val notice = RejectedTokenText.noticeFor(rejected = saved, current = saved)!! + + assertEquals(RejectedTokenText.NOTICE, notice) + assertEquals( + "The server rejected the token. Episodes are downloading from their publishers " + + "meanwhile, so skips will not apply to them. Check the token and save it again.", + notice, + ) + } + + @Test + fun noNoticeWhenNothingWasRejected() { + assertNull(RejectedTokenText.noticeFor(rejected = null, current = saved)) + } + + @Test + fun savingADifferentTokenClearsTheNoticeAtOnce() { + assertNull(RejectedTokenText.noticeFor(rejected = saved, current = saved.copy(token = "fresh"))) + assertNull(RejectedTokenText.noticeFor(rejected = saved, current = saved.copy(baseUrl = "https://other"))) + } + + @Test + fun clearingTheServerClearsTheNotice() { + assertNull(RejectedTokenText.noticeFor(rejected = saved, current = null)) + } } diff --git a/app/src/test/java/org/terst/listen/server/OkHttpSkipTransportTest.kt b/app/src/test/java/org/terst/listen/server/OkHttpSkipTransportTest.kt index 335b68d..47f9da8 100644 --- a/app/src/test/java/org/terst/listen/server/OkHttpSkipTransportTest.kt +++ b/app/src/test/java/org/terst/listen/server/OkHttpSkipTransportTest.kt @@ -6,8 +6,6 @@ import okhttp3.mockwebserver.MockResponse import okhttp3.mockwebserver.MockWebServer import org.junit.After import org.junit.Assert.assertEquals -import org.junit.Assert.assertFalse -import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test @@ -15,11 +13,10 @@ import org.junit.Test /** * The transport's whole contract is that nothing it does can break the app. * - * The skip server is an improvement on playback, not a precondition for it: unreachable, slow, - * misconfigured and returning nonsense all have to mean the same thing to the caller as "not - * configured" -- carry on downloading from publishers. An exception escaping from here would - * turn a reachable episode into a failed download, which is worse than never having had a - * server at all. + * An exception escaping from here would turn a reachable episode into a failed download, which + * is worse than never having had a server at all. So every failure is a [SkipReply]; and the + * one distinction worth drawing is a refused credential (401/403) versus everything else, + * because only the former is a configuration error rather than a wait. */ class OkHttpSkipTransportTest { @@ -36,60 +33,89 @@ class OkHttpSkipTransportTest { @After fun tearDown() = server.shutdown() + private fun getUrl() = server.url("/api/episode").toString() + @Test fun `returns the body of a successful get`() = runTest { server.enqueue(MockResponse().setBody("""{"status":"ready"}""")) - val got = transport.get(server.url("/api/episode").toString(), "tok") - - assertEquals("""{"status":"ready"}""", got) + assertEquals(SkipReply.Body("""{"status":"ready"}"""), transport.get(getUrl(), "tok")) } @Test fun `sends the token as a bearer header`() = runTest { server.enqueue(MockResponse().setBody("{}")) - transport.get(server.url("/api/episode").toString(), "hunter2") + transport.get(getUrl(), "hunter2") assertEquals("Bearer hunter2", server.takeRequest().getHeader("Authorization")) } + /** The two statuses that mean "the credentials were refused" and nothing else does. */ @Test - fun `an unauthorised response is nothing rather than an error`() = runTest { + fun `401 is a rejection`() = runTest { server.enqueue(MockResponse().setResponseCode(401).setBody("""{"error":"unauthorized"}""")) - assertNull(transport.get(server.url("/api/episode").toString(), "wrong")) + assertEquals(SkipReply.Rejected, transport.get(getUrl(), "wrong")) + } + + @Test + fun `403 is a rejection`() = runTest { + server.enqueue(MockResponse().setResponseCode(403)) + + assertEquals(SkipReply.Rejected, transport.get(getUrl(), "wrong")) + } + + @Test + fun `404 is no answer, not a rejection`() = runTest { + server.enqueue(MockResponse().setResponseCode(404)) + + assertEquals(SkipReply.NoAnswer, transport.get(getUrl(), "tok")) + } + + @Test + fun `500 is no answer, not a rejection`() = runTest { + server.enqueue(MockResponse().setResponseCode(500)) + + assertEquals(SkipReply.NoAnswer, transport.get(getUrl(), "tok")) } @Test - fun `a server error is nothing rather than an error`() = runTest { + fun `502 is no answer, not a rejection`() = runTest { server.enqueue(MockResponse().setResponseCode(502)) - assertNull(transport.get(server.url("/api/episode").toString(), "tok")) + assertEquals(SkipReply.NoAnswer, transport.get(getUrl(), "tok")) } /** A misconfigured address must not throw out of the download path. */ @Test - fun `an unreachable host is nothing rather than an error`() = runTest { - assertNull(transport.get("http://127.0.0.1:1/api/episode", "tok")) + fun `an unreachable host is no answer rather than an error`() = runTest { + assertEquals(SkipReply.NoAnswer, transport.get("http://127.0.0.1:1/api/episode", "tok")) } @Test - fun `a malformed url is nothing rather than an error`() = runTest { - assertNull(transport.get("not a url at all", "tok")) + fun `a connection dropped mid-response is no answer rather than an error`() = runTest { + server.enqueue(MockResponse().setSocketPolicy(okhttp3.mockwebserver.SocketPolicy.DISCONNECT_AFTER_REQUEST)) + + assertEquals(SkipReply.NoAnswer, transport.get(getUrl(), "tok")) + } + + @Test + fun `a malformed url is no answer rather than an error`() = runTest { + assertEquals(SkipReply.NoAnswer, transport.get("not a url at all", "tok")) } @Test fun `posts the body as json`() = runTest { server.enqueue(MockResponse().setResponseCode(200)) - val ok = transport.post( + val reply = transport.post( server.url("/api/subscriptions").toString(), "tok", """{"feeds":["https://a"]}""", ) - assertTrue(ok) + assertTrue(reply is SkipReply.Body) val request = server.takeRequest() assertEquals("POST", request.method) assertEquals("""{"feeds":["https://a"]}""", request.body.readUtf8()) @@ -99,15 +125,27 @@ class OkHttpSkipTransportTest { ) } + private fun postUrl() = server.url("/api/subscriptions").toString() + @Test - fun `a rejected post reports false`() = runTest { - server.enqueue(MockResponse().setResponseCode(400)) + fun `a post answered 401 or 403 is a rejection`() = runTest { + server.enqueue(MockResponse().setResponseCode(401)) + server.enqueue(MockResponse().setResponseCode(403)) - assertFalse(transport.post(server.url("/api/subscriptions").toString(), "tok", "{}")) + assertEquals(SkipReply.Rejected, transport.post(postUrl(), "tok", "{}")) + assertEquals(SkipReply.Rejected, transport.post(postUrl(), "tok", "{}")) + } + + @Test + fun `a post answered 400, 404 or 500 is no answer`() = runTest { + listOf(400, 404, 500).forEach { code -> + server.enqueue(MockResponse().setResponseCode(code)) + assertEquals("HTTP $code", SkipReply.NoAnswer, transport.post(postUrl(), "tok", "{}")) + } } @Test - fun `a post to an unreachable host reports false`() = runTest { - assertFalse(transport.post("http://127.0.0.1:1/api/subscriptions", "tok", "{}")) + fun `a post to an unreachable host is no answer`() = runTest { + assertEquals(SkipReply.NoAnswer, transport.post("http://127.0.0.1:1/api/subscriptions", "tok", "{}")) } } diff --git a/app/src/test/java/org/terst/listen/server/ServerSkipsTest.kt b/app/src/test/java/org/terst/listen/server/ServerSkipsTest.kt index 5d069df..56c0bf0 100644 --- a/app/src/test/java/org/terst/listen/server/ServerSkipsTest.kt +++ b/app/src/test/java/org/terst/listen/server/ServerSkipsTest.kt @@ -24,12 +24,12 @@ class ServerSkipsTest { private class StubTransport(private val body: String?) : SkipTransport { val gets = mutableListOf<String>() - override suspend fun get(url: String, token: String): String? { + override suspend fun get(url: String, token: String): SkipReply { gets += url - return body + return body?.let { SkipReply.Body(it) } ?: SkipReply.NoAnswer } - override suspend fun post(url: String, token: String, body: String) = true + override suspend fun post(url: String, token: String, body: String) = SkipReply.Body("") } private val show = SubscriptionEntity( @@ -87,7 +87,7 @@ class ServerSkipsTest { val got = subject(transport, subscriptions = FakeSubscriptionDao()).resolve(episode) - assertNull(got) + assertEquals(ServerAnswer.Unavailable, got) assertTrue("asked about a show that is not subscribed", transport.gets.isEmpty()) } diff --git a/app/src/test/java/org/terst/listen/server/SkipRefresherTest.kt b/app/src/test/java/org/terst/listen/server/SkipRefresherTest.kt index ebeec3f..28e0e03 100644 --- a/app/src/test/java/org/terst/listen/server/SkipRefresherTest.kt +++ b/app/src/test/java/org/terst/listen/server/SkipRefresherTest.kt @@ -67,10 +67,10 @@ class SkipRefresherTest { override suspend fun canAsk(episode: EpisodeEntity) = true - override suspend fun resolve(episode: EpisodeEntity): ServerEpisode? { + override suspend fun resolve(episode: EpisodeEntity): ServerAnswer { resolved += episode.id check(episode.id !in failing) { "server unreachable: ${episode.id}" } - return infoFor[episode.id] + return infoFor[episode.id]?.let { ServerAnswer.Found(it) } ?: ServerAnswer.Unavailable } override suspend fun record(episode: EpisodeEntity, info: ServerEpisode) { @@ -167,12 +167,12 @@ class SkipRefresherTest { // Blocks forever until cancelled, so a test that lets this complete normally proves // the sweep waited for the cancelled work instead of abandoning it. - val stuck = CompletableDeferred<ServerEpisode?>() + val stuck = CompletableDeferred<ServerAnswer>() val started = mutableListOf<String>() val audio = object : ServerAudio { override suspend fun canAsk(episode: EpisodeEntity) = true - override suspend fun resolve(episode: EpisodeEntity): ServerEpisode? { + override suspend fun resolve(episode: EpisodeEntity): ServerAnswer { started += episode.id return stuck.await() } @@ -209,7 +209,7 @@ class SkipRefresherTest { val audio = object : ServerAudio { override suspend fun canAsk(episode: EpisodeEntity) = true - override suspend fun resolve(episode: EpisodeEntity): ServerEpisode? { + override suspend fun resolve(episode: EpisodeEntity): ServerAnswer { throw CancellationException("cancelled mid-query") } @@ -258,8 +258,8 @@ class SkipRefresherTest { val transport = object : SkipTransport { var next = """{"status":"ready","breaks":[{"startMs":0,"endMs":60000}]}""" - override suspend fun get(url: String, token: String) = next - override suspend fun post(url: String, token: String, body: String) = true + override suspend fun get(url: String, token: String) = SkipReply.Body(next) + override suspend fun post(url: String, token: String, body: String) = SkipReply.Body("") } val serverSkips = ServerSkips( SkipServer({ SkipServerConfig("https://listen.terst.org", "tok") }, transport), diff --git a/app/src/test/java/org/terst/listen/server/SkipServerTest.kt b/app/src/test/java/org/terst/listen/server/SkipServerTest.kt index 32d13e0..c54ed35 100644 --- a/app/src/test/java/org/terst/listen/server/SkipServerTest.kt +++ b/app/src/test/java/org/terst/listen/server/SkipServerTest.kt @@ -23,21 +23,24 @@ class SkipServerTest { private class FakeTransport : SkipTransport { var response: String? = null + /** Overrides [response] when set: what the transport reports instead of a body. */ + var reply: SkipReply? = null var failPost = false + var postReply: SkipReply? = null val gets = mutableListOf<String>() val posts = mutableListOf<Pair<String, String>>() var seenToken: String? = null - override suspend fun get(url: String, token: String): String? { + override suspend fun get(url: String, token: String): SkipReply { gets += url seenToken = token - return response + return reply ?: response?.let { SkipReply.Body(it) } ?: SkipReply.NoAnswer } - override suspend fun post(url: String, token: String, body: String): Boolean { + override suspend fun post(url: String, token: String, body: String): SkipReply { posts += url to body seenToken = token - return !failPost + return postReply ?: if (failPost) SkipReply.NoAnswer else SkipReply.Body("") } } @@ -182,6 +185,74 @@ class SkipServerTest { assertEquals(false, server(transport).pushSubscriptions(listOf("https://a"))) } + // ---- a refused token, which is not the same as no answer ---- + + private suspend fun SkipServer.askAboutOne() = ask("https://f", "g1", "", "") + + @Test + fun `a rejected reply is reported as rejected, not as unavailable`() = runTest { + val transport = FakeTransport().apply { reply = SkipReply.Rejected } + + assertEquals(ServerAnswer.Rejected, server(transport).askAboutOne()) + } + + @Test + fun `no answer, rubbish and not configured are unavailable, never rejected`() = runTest { + assertEquals(ServerAnswer.Unavailable, server(FakeTransport().apply { reply = SkipReply.NoAnswer }).askAboutOne()) + assertEquals( + ServerAnswer.Unavailable, + server(FakeTransport().apply { response = "<html>502" }).askAboutOne(), + ) + assertEquals(ServerAnswer.Unavailable, server(FakeTransport(), baseUrl = null).askAboutOne()) + } + + @Test + fun `a rejected episode request is remembered against the configuration that was refused`() = runTest { + val subject = server(FakeTransport().apply { reply = SkipReply.Rejected }, token = "stale") + + subject.askAboutOne() + + assertEquals(SkipServerConfig("https://listen.terst.org", "stale"), subject.rejected.value) + } + + @Test + fun `a rejected push is remembered too, which is the earliest point it can be noticed`() = runTest { + val subject = server(FakeTransport().apply { postReply = SkipReply.Rejected }, token = "stale") + + assertEquals(false, subject.pushSubscriptions(listOf("https://a"))) + + assertEquals(SkipServerConfig("https://listen.terst.org", "stale"), subject.rejected.value) + } + + @Test + fun `a later success clears the remembered rejection`() = runTest { + val transport = FakeTransport().apply { reply = SkipReply.Rejected } + val subject = server(transport) + subject.askAboutOne() + + transport.reply = null + transport.response = ready + subject.askAboutOne() + + assertNull(subject.rejected.value) + } + + @Test + fun `an unreachable server neither sets nor clears the rejection`() = runTest { + val transport = FakeTransport().apply { reply = SkipReply.Rejected } + val subject = server(transport) + subject.askAboutOne() + + transport.reply = SkipReply.NoAnswer + subject.askAboutOne() + + assertEquals(SkipServerConfig("https://listen.terst.org", "tok"), subject.rejected.value) + + val fresh = server(FakeTransport().apply { reply = SkipReply.NoAnswer }) + fresh.askAboutOne() + assertNull(fresh.rejected.value) + } + /** A trailing slash on the configured URL must not produce a double slash in the path. */ @Test fun `tolerates a trailing slash on the base url`() = runTest { |