From 7fa1eedd66e01b36dc8f7ce059500a411ce8c738 Mon Sep 17 00:00:00 2001 From: PonceGL Date: Wed, 9 Sep 2026 20:40:49 -0600 Subject: [PATCH 1/2] =?UTF-8?q?fix(jellyfin):=20P.4=20=E2=80=94=20excepci?= =?UTF-8?q?=C3=B3n=20tipada=20para=20errores=20HTTP?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit request() envolvía cualquier fallo en Exception("HTTP $code: ..."), enterrando el código de estado dentro de un String (GEN-AP-06). Sin él, el clasificador de errores de F3.3 (docs/downloads) no puede distinguir 401/403/404/416/429/5xx sin parsear texto. - JellyfinHttpException(statusCode, retryAfter, message): retryAfter va crudo, sin parsear — los dos formatos de Retry-After los decide quien clasifica. - catch (e: CancellationException) { throw e } antes del catch genérico: el catch (e: Exception) existente tragaba la cancelación (AND-CONC-04). - Cero llamantes en el proyecto parseaban el mensaje anterior; sin regresiones. Self-review (code-review skill, medium effort) surfaced one finding: authenticateByName(), a few lines below, has the exact same CancellationException-swallowing catch (e: Exception) — same bug, same file, just not the method R19 named. Fixed it too, same one-line pattern, with a test mirroring the existing deterministic cancel-before-call case. Left its HTTP-failure paths as plain Exception (not JellyfinHttpException): unlike request(), nothing downstream needs to distinguish auth failure status codes, and R19/F3.3 don't ask for it there. 6 tests JVM nuevos (5 + 1 de la autorevisión), en verde. Baseline: 393 tests, los mismos 5 fallos preexistentes de siempre, ninguno nuevo (check-baseline.sh). --- .../network/jellyfin/JellyfinApiService.kt | 13 +- .../network/jellyfin/JellyfinHttpException.kt | 16 ++ .../JellyfinApiServiceErrorHandlingTest.kt | 154 ++++++++++++++++++ 3 files changed, 182 insertions(+), 1 deletion(-) create mode 100644 app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinHttpException.kt create mode 100644 app/src/test/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiServiceErrorHandlingTest.kt diff --git a/app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiService.kt b/app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiService.kt index 0fb1979be7..0544cba2bd 100644 --- a/app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiService.kt +++ b/app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiService.kt @@ -1,6 +1,7 @@ package com.theveloper.pixelplay.data.network.jellyfin import com.theveloper.pixelplay.data.jellyfin.model.JellyfinCredentials +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext import okhttp3.HttpUrl.Companion.toHttpUrl @@ -107,6 +108,8 @@ class JellyfinApiService @Inject constructor( Timber.d("$TAG: Authentication successful for user $username") Result.success(Pair(accessToken, userId)) } + } catch (e: CancellationException) { + throw e } catch (e: Exception) { Timber.e(e, "$TAG: Authentication failed") Result.failure(e) @@ -142,12 +145,20 @@ class JellyfinApiService @Inject constructor( if (!response.isSuccessful) { Timber.w("$TAG: <<< HTTP $code for $path") - return@withContext Result.failure(Exception("HTTP $code: ${response.message}")) + return@withContext Result.failure( + JellyfinHttpException( + statusCode = code, + retryAfter = response.header("Retry-After"), + message = "HTTP $code: ${response.message}", + ) + ) } Timber.d("$TAG: <<< HTTP $code for $path, body length: ${body.length}") Result.success(body) } + } catch (e: CancellationException) { + throw e } catch (e: Exception) { Timber.e(e, "$TAG: !!! FAILED GET $path") Result.failure(e) diff --git a/app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinHttpException.kt b/app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinHttpException.kt new file mode 100644 index 0000000000..8ec8641a24 --- /dev/null +++ b/app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinHttpException.kt @@ -0,0 +1,16 @@ +package com.theveloper.pixelplay.data.network.jellyfin + +/** + * A typed HTTP failure from [JellyfinApiService], carrying the status code and the raw + * `Retry-After` header instead of burying them inside [message] (`GEN-AP-06`, R19 of the + * downloads plan). + * + * [retryAfter] is passed through unparsed: HTTP allows it as either a number of seconds or an + * HTTP-date, and deciding between the two — and clamping the result — is the caller's job + * (`F3.3` in `docs/downloads/F3.md`), not this transport-level type's. + */ +class JellyfinHttpException( + val statusCode: Int, + val retryAfter: String?, + message: String, +) : Exception(message) diff --git a/app/src/test/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiServiceErrorHandlingTest.kt b/app/src/test/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiServiceErrorHandlingTest.kt new file mode 100644 index 0000000000..34050382ab --- /dev/null +++ b/app/src/test/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiServiceErrorHandlingTest.kt @@ -0,0 +1,154 @@ +package com.theveloper.pixelplay.data.network.jellyfin + +import com.theveloper.pixelplay.data.jellyfin.model.JellyfinCredentials +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.cancel +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.runTest +import okhttp3.MediaType.Companion.toMediaType +import okhttp3.OkHttpClient +import okhttp3.Protocol +import okhttp3.Response +import okhttp3.ResponseBody.Companion.toResponseBody +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Assertions.assertNull +import org.junit.jupiter.api.Assertions.assertTrue +import org.junit.jupiter.api.Test + +/** + * `P.4`: [request] must expose the HTTP status code as a typed [JellyfinHttpException] instead + * of burying it inside a message string (R19), and must never rewrap a [CancellationException] + * into a [Result.failure] (`AND-CONC-04`). + * + * No mock server dependency is added for this: a fake [okhttp3.Interceptor] on the client injected + * into [JellyfinApiService] returns a canned [Response] — real OkHttp objects, no new test + * dependency. It works because `newBuilder()` carries interceptors over (confirmed independently + * by `F1.2`'s finding on this same client). + */ +class JellyfinApiServiceErrorHandlingTest { + + private fun serviceRespondingWith( + code: Int, + headers: Map = emptyMap(), + body: String = "{}", + ): JellyfinApiService { + val client = OkHttpClient.Builder() + .addInterceptor { chain -> + val builder = Response.Builder() + .request(chain.request()) + .protocol(Protocol.HTTP_1_1) + .code(code) + .message("test") + .body(body.toResponseBody("application/json".toMediaType())) + headers.forEach { (name, value) -> builder.addHeader(name, value) } + builder.build() + } + .build() + + return JellyfinApiService(client).apply { + setCredentials( + JellyfinCredentials( + serverUrl = "http://example.invalid", + username = "user", + password = "pw", + accessToken = "test-token", + userId = "user-1", + ) + ) + } + } + + @Test + fun `a non-2xx response fails with a JellyfinHttpException carrying the status code`() = runTest { + val service = serviceRespondingWith(code = 500) + + val result = service.ping() + + assertTrue(result.isFailure) + val error = result.exceptionOrNull() + assertTrue(error is JellyfinHttpException) + assertEquals(500, (error as JellyfinHttpException).statusCode) + } + + @Test + fun `a 429 with Retry-After carries the raw header value, unparsed`() = runTest { + val service = serviceRespondingWith(code = 429, headers = mapOf("Retry-After" to "30")) + + val result = service.ping() + + val error = result.exceptionOrNull() as JellyfinHttpException + assertEquals(429, error.statusCode) + assertEquals("30", error.retryAfter) + } + + @Test + fun `a failure without a Retry-After header leaves it null, not throws`() = runTest { + val service = serviceRespondingWith(code = 404) + + val result = service.ping() + + val error = result.exceptionOrNull() as JellyfinHttpException + assertEquals(404, error.statusCode) + assertNull(error.retryAfter) + } + + @Test + fun `a successful response still succeeds — unchanged happy path`() = runTest { + val service = serviceRespondingWith(code = 200) + + val result = service.ping() + + assertTrue(result.isSuccess) + assertTrue(result.getOrThrow()) + } + + // ─── Case borde: cancellation propagates instead of becoming a Result ─────── + // + // Same deterministic pattern as `GitHubAnnouncementPropertiesServiceTest` (P.9): cancel the + // coroutine before the suspend function is even entered, so the cancellation check inside + // `withContext` fires before any network code runs — no dependency on real socket timing. + + @Test + fun `a coroutine cancelled before the call starts propagates cancellation, not a Result`() = runTest { + val service = serviceRespondingWith(code = 200) + var sawCancellation = false + + val job = launch { + cancel() + try { + service.ping() + } catch (e: CancellationException) { + sawCancellation = true + throw e + } + } + job.join() + + assertTrue(job.isCancelled) + assertTrue(sawCancellation) + } + + // Same bug, same fix, a different method in this file (surfaced by self-review, not R19 — + // authenticateByName() isn't part of F3.3's error taxonomy, but the cancellation-swallowing + // catch is identical and just as real). + + @Test + fun `authenticateByName also propagates cancellation instead of becoming a Result`() = runTest { + val service = serviceRespondingWith(code = 200) + var sawCancellation = false + + val job = launch { + cancel() + try { + service.authenticateByName("http://example.invalid", "user", "pw") + } catch (e: CancellationException) { + sawCancellation = true + throw e + } + } + job.join() + + assertTrue(job.isCancelled) + assertTrue(sawCancellation) + } +} From 7b592756cbcd725a5ed28bef403844fc56694a53 Mon Sep 17 00:00:00 2001 From: PonceGL Date: Thu, 10 Sep 2026 16:43:48 -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/network/jellyfin/JellyfinHttpException.kt | 7 +++---- .../jellyfin/JellyfinApiServiceErrorHandlingTest.kt | 9 ++++----- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinHttpException.kt b/app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinHttpException.kt index 8ec8641a24..0c58c27281 100644 --- a/app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinHttpException.kt +++ b/app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinHttpException.kt @@ -2,12 +2,11 @@ package com.theveloper.pixelplay.data.network.jellyfin /** * A typed HTTP failure from [JellyfinApiService], carrying the status code and the raw - * `Retry-After` header instead of burying them inside [message] (`GEN-AP-06`, R19 of the - * downloads plan). + * `Retry-After` header instead of burying them inside [message]. * * [retryAfter] is passed through unparsed: HTTP allows it as either a number of seconds or an - * HTTP-date, and deciding between the two — and clamping the result — is the caller's job - * (`F3.3` in `docs/downloads/F3.md`), not this transport-level type's. + * HTTP-date, and deciding between the two — and clamping the result — is the caller's job, + * not this transport-level type's. */ class JellyfinHttpException( val statusCode: Int, diff --git a/app/src/test/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiServiceErrorHandlingTest.kt b/app/src/test/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiServiceErrorHandlingTest.kt index 34050382ab..5ceca224b6 100644 --- a/app/src/test/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiServiceErrorHandlingTest.kt +++ b/app/src/test/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiServiceErrorHandlingTest.kt @@ -16,14 +16,13 @@ import org.junit.jupiter.api.Assertions.assertTrue import org.junit.jupiter.api.Test /** - * `P.4`: [request] must expose the HTTP status code as a typed [JellyfinHttpException] instead - * of burying it inside a message string (R19), and must never rewrap a [CancellationException] - * into a [Result.failure] (`AND-CONC-04`). + * [request] must expose the HTTP status code as a typed [JellyfinHttpException] instead + * of burying it inside a message string, and must never rewrap a [CancellationException] + * into a [Result.failure]. * * No mock server dependency is added for this: a fake [okhttp3.Interceptor] on the client injected * into [JellyfinApiService] returns a canned [Response] — real OkHttp objects, no new test - * dependency. It works because `newBuilder()` carries interceptors over (confirmed independently - * by `F1.2`'s finding on this same client). + * dependency. It works because `newBuilder()` carries interceptors over. */ class JellyfinApiServiceErrorHandlingTest {