refactor(github): generalize property fetching, fix cancellation swallowing

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: 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].
+     */
+    suspend fun fetchProperties(
+        owner: String,
+        repo: String,
+        branch: String,
+        configPath: String,
+    ): Result<Properties> = 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<PlayStoreAnnouncementRemoteConfig> {
-        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: 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<Properties> {
+    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()
         }
     }
 }

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. Not tested separately on purpose.
+}