From aef58f7e5486133d33a0695b5b35cc69d651483b Mon Sep 17 00:00:00 2001 From: PonceGL Date: Mon, 7 Sep 2026 07:23:24 -0600 Subject: [PATCH 1/2] refactor(github): generalize property fetching, fix cancellation swallowing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitHubAnnouncementPropertiesService only knew how to fetch the Play Store announcement config. Split it into three pieces so any future consumer (this repo's downloads feature, upstream or not) can read its own properties file without knowing anything about Play Store announcements: - buildRawContentUrl: pure URL construction. - fetchRawProperties(rawUrl): the actual HTTP GET + Properties parsing, given any URL — not just github.com. This is what makes the 404/error paths testable at all: fetchProperties() always builds a real GitHub URL by design, so there is no seam to intercept it in a unit test without hitting the network for real. fetchRawProperties has none of that restriction, so tests point it at a local com.sun.net.httpserver.HttpServer instead (JDK-included, no new dependency). - Properties.toPlayStoreAnnouncementRemoteConfig(): the mapping, pulled out so it's testable directly against a hand-built Properties with no network involved at all. fetchPlayStoreAnnouncement() is now just fetchProperties(...).mapCatching { it.toPlayStoreAnnouncementRemoteConfig() } — same signature, same defaults (including the stale theovilardo/PixelPlay repo; that's a separate fix), same behavior on 200/404/error, verified with a characterization test written before touching the mapping logic. Also fixed: the original catch (e: Exception) silently turned a cancelled coroutine into a Result.failure instead of letting CancellationException propagate (AND-CONC-04). Worth being explicit about what this does and doesn't achieve: HttpURLConnection is a classic blocking-socket API that does not react to Thread.interrupt(), which I confirmed empirically — neither runInterruptible nor closing the connection from a cancellation handler actually interrupts a call that's genuinely blocked in getResponseCode(). So a cancellation that arrives while the network call is in flight still has to wait for that call to finish on its own (success, error, or its 10s timeout) before it can surface. What the fix does guarantee: once a CancellationException does reach this code, it is never swallowed. Making the call itself promptly interruptible would mean moving off HttpURLConnection entirely (OkHttp, e.g.) — out of scope here, noted in the KDoc for whoever touches this file next. 10 new JVM tests, stable across repeated runs: the URL builder, the mapping (including every boolean spelling and blank-vs-absent values), fetchRawProperties against a real local server for 200/404/500/unreachable-host, and a deterministic cancellation test (cancel before the call starts, rather than racing real socket timing — which the empirical finding above shows doesn't actually cancel anyway). No consumer changes: MainActivity's fetchPlayStoreAnnouncement() call is untouched. Preparatory task for the downloads feature (P.9, cut from master — generic, unrelated to that feature's UI). --- .../GitHubAnnouncementPropertiesService.kt | 141 ++++++++++---- ...GitHubAnnouncementPropertiesServiceTest.kt | 182 ++++++++++++++++++ 2 files changed, 283 insertions(+), 40 deletions(-) create mode 100644 app/src/test/java/com/theveloper/pixelplay/data/github/GitHubAnnouncementPropertiesServiceTest.kt diff --git a/app/src/main/java/com/theveloper/pixelplay/data/github/GitHubAnnouncementPropertiesService.kt b/app/src/main/java/com/theveloper/pixelplay/data/github/GitHubAnnouncementPropertiesService.kt index d540ad1cae..e855585beb 100644 --- a/app/src/main/java/com/theveloper/pixelplay/data/github/GitHubAnnouncementPropertiesService.kt +++ b/app/src/main/java/com/theveloper/pixelplay/data/github/GitHubAnnouncementPropertiesService.kt @@ -6,6 +6,7 @@ import java.net.URL import java.util.Properties import javax.inject.Inject import javax.inject.Singleton +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext import timber.log.Timber @@ -24,7 +25,28 @@ data class PlayStoreAnnouncementRemoteConfig( class GitHubAnnouncementPropertiesService @Inject constructor() { /** - * Reads announcement flags from a raw properties file in GitHub. + * Reads a raw `.properties` file from a GitHub repo over its raw-content CDN. Generic on + * purpose (`GEN-DES-01`): this class has no idea what a "Play Store announcement" or a + * "downloads feature flag" is — each caller maps the result to its own type, the way + * [fetchPlayStoreAnnouncement] does below. + * + * A 404 is **not** a failure: it means the file doesn't exist (yet, or was deliberately + * removed), and the caller gets an empty [Properties] back to interpret however its own + * defaults say to — this is what makes a remote kill switch fail safe instead of fail + * enabled. Any other non-2xx status, a network error, or an unreadable response is a + * [Result.failure]. Cancelling the calling coroutine while this suspends propagates as + * [CancellationException], never as a [Result.failure] (`AND-CONC-04`). + */ + suspend fun fetchProperties( + owner: String, + repo: String, + branch: String, + configPath: String, + ): Result = fetchRawProperties(buildRawContentUrl(owner, repo, branch, configPath)) + + /** + * Reads announcement flags from a raw properties file in GitHub. A mapping over + * [fetchProperties]; the network call and its error handling live there now. * * Expected keys: * - play_store_announcement_enabled @@ -41,48 +63,87 @@ class GitHubAnnouncementPropertiesService @Inject constructor() { branch: String = "master", configPath: String = "remote-config/app-announcements.properties", ): Result { - return withContext(Dispatchers.IO) { - var connection: HttpURLConnection? = null - try { - val rawUrl = "https://raw.githubusercontent.com/$owner/$repo/$branch/$configPath" - connection = (URL(rawUrl).openConnection() as HttpURLConnection).apply { - requestMethod = "GET" - connectTimeout = 10_000 - readTimeout = 10_000 - addRequestProperty("Accept", "text/plain") - } + return fetchProperties(owner, repo, branch, configPath) + .mapCatching { it.toPlayStoreAnnouncementRemoteConfig() } + } +} - when (val code = connection.responseCode) { - HttpURLConnection.HTTP_OK -> { - val response = connection.inputStream.bufferedReader().use { it.readText() } - val props = Properties().apply { load(StringReader(response)) } - val config = PlayStoreAnnouncementRemoteConfig( - enabled = props.booleanFlag("play_store_announcement_enabled"), - playStoreUrl = props.stringValue("play_store_url"), - title = props.stringValue("play_store_announcement_title"), - body = props.stringValue("play_store_announcement_body"), - primaryActionLabel = props.stringValue("play_store_primary_action"), - dismissActionLabel = props.stringValue("play_store_dismiss_action"), - linkPendingMessage = props.stringValue("play_store_link_pending_message"), - ) - Result.success(config) - } - HttpURLConnection.HTTP_NOT_FOUND -> { - Timber.i("Remote announcement properties file not found. Keeping announcement disabled.") - Result.success(PlayStoreAnnouncementRemoteConfig()) - } - else -> { - val errorMessage = connection.errorStream?.bufferedReader()?.use { it.readText() } - Result.failure( - IllegalStateException("Failed to fetch remote announcement properties: $code - $errorMessage"), - ) - } +/** + * The mapping [fetchPlayStoreAnnouncement] applies to whatever [fetchProperties] returns. + * `internal` and pulled out on its own so it's testable directly with a hand-built + * [Properties] — no network involved — which is most of what "same behavior as before" + * actually means here. + */ +internal fun Properties.toPlayStoreAnnouncementRemoteConfig(): PlayStoreAnnouncementRemoteConfig = + PlayStoreAnnouncementRemoteConfig( + enabled = booleanFlag("play_store_announcement_enabled"), + playStoreUrl = stringValue("play_store_url"), + title = stringValue("play_store_announcement_title"), + body = stringValue("play_store_announcement_body"), + primaryActionLabel = stringValue("play_store_primary_action"), + dismissActionLabel = stringValue("play_store_dismiss_action"), + linkPendingMessage = stringValue("play_store_link_pending_message"), + ) + +/** Where [fetchProperties] actually points. Pure string-building, no I/O — `internal` so a + * test can verify it without touching the network. */ +internal fun buildRawContentUrl(owner: String, repo: String, branch: String, configPath: String): String = + "https://raw.githubusercontent.com/$owner/$repo/$branch/$configPath" + +/** + * Does the actual HTTP GET against [rawUrl] and parses the response as [Properties]. `internal` + * so a test can point it at a local server instead of `raw.githubusercontent.com` — the public + * [fetchProperties] always builds a real GitHub URL, so this is the only seam that makes the + * 404 / error behaviors verifiable without a live network call. + * + * The `catch (e: CancellationException) { throw e }` below is the actual fix this task exists + * for (`AND-CONC-04`): the original code caught a broad `Exception`, which would have silently + * turned a coroutine cancellation into an ordinary [Result.failure] instead of letting it + * propagate. It's worth being honest about what this does and doesn't guarantee: + * `HttpURLConnection` is a classic blocking-socket API that does **not** react to + * `Thread.interrupt()`, so cancelling the caller while this is blocked in + * [HttpURLConnection.getResponseCode] does not interrupt that call — it still runs to + * completion (or its own timeout) before this function gets a chance to notice anything. What + * this fix guarantees is narrower but real: cancellation is never *swallowed* once it does + * reach this code, at whichever point that happens to be. Making the blocking call itself + * promptly interruptible would mean replacing `HttpURLConnection` with something that supports + * it (OkHttp, e.g.) — out of scope for a "fix the catch" task; noted for whoever touches this + * next. + */ +internal suspend fun fetchRawProperties(rawUrl: String): Result { + return withContext(Dispatchers.IO) { + var connection: HttpURLConnection? = null + try { + val conn = (URL(rawUrl).openConnection() as HttpURLConnection).apply { + requestMethod = "GET" + connectTimeout = 10_000 + readTimeout = 10_000 + addRequestProperty("Accept", "text/plain") + } + connection = conn + + when (val code = conn.responseCode) { + HttpURLConnection.HTTP_OK -> { + val response = conn.inputStream.bufferedReader().use { it.readText() } + Result.success(Properties().apply { load(StringReader(response)) }) + } + HttpURLConnection.HTTP_NOT_FOUND -> { + Timber.i("Remote properties file not found at $rawUrl. Returning empty properties.") + Result.success(Properties()) + } + else -> { + val errorMessage = conn.errorStream?.bufferedReader()?.use { it.readText() } + Result.failure( + IllegalStateException("Failed to fetch remote properties: $code - $errorMessage"), + ) } - } catch (e: Exception) { - Result.failure(e) - } finally { - connection?.disconnect() } + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + Result.failure(e) + } finally { + connection?.disconnect() } } } diff --git a/app/src/test/java/com/theveloper/pixelplay/data/github/GitHubAnnouncementPropertiesServiceTest.kt b/app/src/test/java/com/theveloper/pixelplay/data/github/GitHubAnnouncementPropertiesServiceTest.kt new file mode 100644 index 0000000000..bffa037583 --- /dev/null +++ b/app/src/test/java/com/theveloper/pixelplay/data/github/GitHubAnnouncementPropertiesServiceTest.kt @@ -0,0 +1,182 @@ +package com.theveloper.pixelplay.data.github + +import com.sun.net.httpserver.HttpServer +import java.net.InetSocketAddress +import java.util.Properties +import java.util.concurrent.Executors +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.cancel +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.runTest +import org.junit.jupiter.api.AfterEach +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Assertions.assertFalse +import org.junit.jupiter.api.Assertions.assertNull +import org.junit.jupiter.api.Assertions.assertTrue +import org.junit.jupiter.api.BeforeEach +import org.junit.jupiter.api.Test + +class GitHubAnnouncementPropertiesServiceTest { + + private lateinit var server: HttpServer + + @BeforeEach + fun startServer() { + server = HttpServer.create(InetSocketAddress("localhost", 0), 0) + server.executor = Executors.newCachedThreadPool() + server.start() + } + + @AfterEach + fun stopServer() { + server.stop(0) + } + + private fun url(path: String) = "http://localhost:${server.address.port}$path" + + private fun respond(path: String, code: Int, body: String) { + server.createContext(path) { exchange -> + val bytes = body.toByteArray() + exchange.sendResponseHeaders(code, bytes.size.toLong()) + exchange.responseBody.use { it.write(bytes) } + } + } + + // ─── buildRawContentUrl: pure, no I/O ─────────────────────────────────────── + + @Test + fun `buildRawContentUrl points at raw githubusercontent com with the given path`() { + val url = buildRawContentUrl("owner", "repo", "main", "config/flags.properties") + + assertEquals("https://raw.githubusercontent.com/owner/repo/main/config/flags.properties", url) + } + + // ─── fetchRawProperties: real HTTP behavior against a local server ────────── + + @Test + fun `a 200 response is parsed into Properties`() = runTest { + respond("/ok", 200, "key1=value1\nkey2 = value2\n") + + val result = fetchRawProperties(url("/ok")) + + assertTrue(result.isSuccess) + val props = result.getOrThrow() + assertEquals("value1", props.getProperty("key1")) + assertEquals("value2", props.getProperty("key2")) + } + + @Test + fun `a 404 is success with empty Properties, not a failure`() = runTest { + respond("/missing", 404, "") + + val result = fetchRawProperties(url("/missing")) + + assertTrue(result.isSuccess) + assertTrue(result.getOrThrow().isEmpty) + } + + @Test + fun `a server error is a failure`() = runTest { + respond("/broken", 500, "internal error") + + val result = fetchRawProperties(url("/broken")) + + assertTrue(result.isFailure) + } + + @Test + fun `an unreachable host is a failure, not a thrown exception`() = runTest { + // Nothing is listening on this port. + val result = fetchRawProperties("http://localhost:1/nothing-here") + + assertTrue(result.isFailure) + } + + // ─── Case borde: cancellation propagates instead of becoming a Result ─────── + // + // `HttpURLConnection` is a classic blocking-socket API and does not react to + // `Thread.interrupt()`: cancelling the caller while it's genuinely blocked inside + // `getResponseCode()` does not interrupt that call (verified empirically — attempts at + // forcing it, including closing the connection from a completion handler, could not make + // it happen reliably; documented as a known limitation on `fetchRawProperties`'s own KDoc). + // What *is* real, and what this task's fix actually targets, is that a `CancellationException` + // reaching this function's try/catch — however it gets there — is never rewrapped into a + // `Result.failure`. A coroutine cancelled before the call even starts exercises exactly that + // catch path deterministically, without depending on real socket timing. + + @Test + fun `a coroutine cancelled before the fetch starts propagates cancellation, not a Result`() = runTest { + var sawCancellation = false + + val job = launch { + cancel() + try { + fetchRawProperties(url("/never-reached")) + } catch (e: CancellationException) { + sawCancellation = true + throw e + } + } + job.join() + + assertTrue(job.isCancelled) + assertTrue(sawCancellation) + } + + // ─── toPlayStoreAnnouncementRemoteConfig: the mapping, with no network at all ─ + + @Test + fun `empty Properties maps to the same defaults as PlayStoreAnnouncementRemoteConfig()`() { + val config = Properties().toPlayStoreAnnouncementRemoteConfig() + + assertEquals(PlayStoreAnnouncementRemoteConfig(), config) + } + + @Test + fun `a fully populated properties file maps every field`() { + val props = Properties().apply { + setProperty("play_store_announcement_enabled", "true") + setProperty("play_store_url", "https://play.google.com/store/apps/details?id=x") + setProperty("play_store_announcement_title", "New version!") + setProperty("play_store_announcement_body", "Check it out") + setProperty("play_store_primary_action", "Update") + setProperty("play_store_dismiss_action", "Later") + setProperty("play_store_link_pending_message", "Opening store…") + } + + val config = props.toPlayStoreAnnouncementRemoteConfig() + + assertTrue(config.enabled) + assertEquals("https://play.google.com/store/apps/details?id=x", config.playStoreUrl) + assertEquals("New version!", config.title) + assertEquals("Check it out", config.body) + assertEquals("Update", config.primaryActionLabel) + assertEquals("Later", config.dismissActionLabel) + assertEquals("Opening store…", config.linkPendingMessage) + } + + @Test + fun `booleanFlag accepts the documented truthy spellings and defaults everything else to false`() { + listOf("true", "1", "yes", "on", "TRUE", " true ").forEach { value -> + val props = Properties().apply { setProperty("play_store_announcement_enabled", value) } + assertTrue(props.toPlayStoreAnnouncementRemoteConfig().enabled, "expected '$value' to be truthy") + } + listOf("false", "0", "no", "off", "garbage", "").forEach { value -> + val props = Properties().apply { setProperty("play_store_announcement_enabled", value) } + assertFalse(props.toPlayStoreAnnouncementRemoteConfig().enabled, "expected '$value' to be falsy") + } + } + + @Test + fun `a blank string value is treated the same as an absent one`() { + val props = Properties().apply { setProperty("play_store_url", " ") } + + assertNull(props.toPlayStoreAnnouncementRemoteConfig().playStoreUrl) + } + + // fetchProperties itself is a one-line composition of buildRawContentUrl (tested above, + // pure) and fetchRawProperties (tested above, against a local server): it always builds a + // real GitHub URL by design, so there's no seam to verify it end to end without a live + // network call — which would make this suite flaky and offline-hostile for no real + // coverage gain (GEN-TEST-01/04). Not tested separately on purpose. +} From 5b4d95de05b35f097da8c424a423e2b1b017eee6 Mon Sep 17 00:00:00 2001 From: PonceGL Date: Thu, 10 Sep 2026 16:47:31 -0600 Subject: [PATCH 2/2] docs: strip internal-planning-doc references from code comments Comments cited local-only planning files and rule IDs (docs/downloads/*.md, GEN-/AND- rule docs) that don't exist outside this machine and are meaningless to anyone reviewing the PR. Reworded to keep the same technical reasoning without the dangling references. --- .../data/github/GitHubAnnouncementPropertiesService.kt | 6 +++--- .../data/github/GitHubAnnouncementPropertiesServiceTest.kt | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/com/theveloper/pixelplay/data/github/GitHubAnnouncementPropertiesService.kt b/app/src/main/java/com/theveloper/pixelplay/data/github/GitHubAnnouncementPropertiesService.kt index e855585beb..f5e8d12e54 100644 --- a/app/src/main/java/com/theveloper/pixelplay/data/github/GitHubAnnouncementPropertiesService.kt +++ b/app/src/main/java/com/theveloper/pixelplay/data/github/GitHubAnnouncementPropertiesService.kt @@ -26,7 +26,7 @@ class GitHubAnnouncementPropertiesService @Inject constructor() { /** * Reads a raw `.properties` file from a GitHub repo over its raw-content CDN. Generic on - * purpose (`GEN-DES-01`): this class has no idea what a "Play Store announcement" or a + * purpose: this class has no idea what a "Play Store announcement" or a * "downloads feature flag" is — each caller maps the result to its own type, the way * [fetchPlayStoreAnnouncement] does below. * @@ -35,7 +35,7 @@ class GitHubAnnouncementPropertiesService @Inject constructor() { * defaults say to — this is what makes a remote kill switch fail safe instead of fail * enabled. Any other non-2xx status, a network error, or an unreadable response is a * [Result.failure]. Cancelling the calling coroutine while this suspends propagates as - * [CancellationException], never as a [Result.failure] (`AND-CONC-04`). + * [CancellationException], never as a [Result.failure]. */ suspend fun fetchProperties( owner: String, @@ -97,7 +97,7 @@ internal fun buildRawContentUrl(owner: String, repo: String, branch: String, con * 404 / error behaviors verifiable without a live network call. * * The `catch (e: CancellationException) { throw e }` below is the actual fix this task exists - * for (`AND-CONC-04`): the original code caught a broad `Exception`, which would have silently + * for: the original code caught a broad `Exception`, which would have silently * turned a coroutine cancellation into an ordinary [Result.failure] instead of letting it * propagate. It's worth being honest about what this does and doesn't guarantee: * `HttpURLConnection` is a classic blocking-socket API that does **not** react to diff --git a/app/src/test/java/com/theveloper/pixelplay/data/github/GitHubAnnouncementPropertiesServiceTest.kt b/app/src/test/java/com/theveloper/pixelplay/data/github/GitHubAnnouncementPropertiesServiceTest.kt index bffa037583..42d774ff91 100644 --- a/app/src/test/java/com/theveloper/pixelplay/data/github/GitHubAnnouncementPropertiesServiceTest.kt +++ b/app/src/test/java/com/theveloper/pixelplay/data/github/GitHubAnnouncementPropertiesServiceTest.kt @@ -178,5 +178,5 @@ class GitHubAnnouncementPropertiesServiceTest { // pure) and fetchRawProperties (tested above, against a local server): it always builds a // real GitHub URL by design, so there's no seam to verify it end to end without a live // network call — which would make this suite flaky and offline-hostile for no real - // coverage gain (GEN-TEST-01/04). Not tested separately on purpose. + // coverage gain. Not tested separately on purpose. }