diff --git a/app/src/main/java/com/theveloper/pixelplay/data/jellyfin/JellyfinRepository.kt b/app/src/main/java/com/theveloper/pixelplay/data/jellyfin/JellyfinRepository.kt index 82f9d4acd1..355d1a9bdc 100644 --- a/app/src/main/java/com/theveloper/pixelplay/data/jellyfin/JellyfinRepository.kt +++ b/app/src/main/java/com/theveloper/pixelplay/data/jellyfin/JellyfinRepository.kt @@ -25,6 +25,7 @@ import com.theveloper.pixelplay.data.preferences.PlaylistPreferencesRepository import com.theveloper.pixelplay.data.stream.BulkSyncResult import com.theveloper.pixelplay.data.stream.CloudMusicUtils import dagger.hilt.android.qualifiers.ApplicationContext +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow @@ -160,6 +161,8 @@ class JellyfinRepository @Inject constructor( _isLoggedInFlow.value = true Timber.d("$TAG: Login successful for $username@$serverUrl") Result.success(username) + } catch (e: CancellationException) { + throw e } catch (e: Exception) { Timber.e(e, "$TAG: Login failed") api.clearCredentials() @@ -251,6 +254,8 @@ class JellyfinRepository @Inject constructor( Timber.d("$TAG: Synced ${entities.size} playlists") Result.success(entities) + } catch (e: CancellationException) { + throw e } catch (e: Exception) { Timber.e(e, "$TAG: Failed to sync playlists") Result.failure(e) @@ -294,6 +299,8 @@ class JellyfinRepository @Inject constructor( Timber.d("$TAG: Synced ${entities.size} songs for playlist $playlistId") Result.success(entities.size) + } catch (e: CancellationException) { + throw e } catch (e: Exception) { Timber.e(e, "$TAG: Failed to sync playlist songs") Result.failure(e) @@ -335,6 +342,8 @@ class JellyfinRepository @Inject constructor( Timber.d("$TAG: Synced ${entities.size} library songs") Result.success(entities.size) + } catch (e: CancellationException) { + throw e } catch (e: Exception) { Timber.e(e, "$TAG: Failed to sync library songs") Result.failure(e) @@ -356,6 +365,8 @@ class JellyfinRepository @Inject constructor( val playlistResult = syncPlaylists().getOrElse { try { syncUnifiedLibrarySongsFromJellyfin() + } catch (e: CancellationException) { + throw e } catch (e: Exception) { Timber.e(e, "$TAG: Failed to sync unified library after playlist fetch failure") } @@ -377,6 +388,8 @@ class JellyfinRepository @Inject constructor( try { syncUnifiedLibrarySongsFromJellyfin() + } catch (e: CancellationException) { + throw e } catch (e: Exception) { Timber.e(e, "$TAG: Failed to sync unified library") } @@ -426,6 +439,8 @@ class JellyfinRepository @Inject constructor( } val jellyfinSongs = JellyfinResponseParser.parseSongs(result.getOrThrow()) Result.success(jellyfinSongs.map { it.toDisplaySong() }) + } catch (e: CancellationException) { + throw e } catch (e: Exception) { Timber.e(e, "$TAG: Search failed") Result.failure(e) @@ -460,6 +475,8 @@ class JellyfinRepository @Inject constructor( return@withContext result } Result.failure(Exception("No lyrics found")) + } catch (e: CancellationException) { + throw e } catch (e: Exception) { Timber.e(e, "$TAG: Failed to get lyrics for song $songId") Result.failure(e) 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..26f5ea14de 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 @@ -92,7 +93,13 @@ class JellyfinApiService @Inject constructor( okHttpClient.newCall(request).execute().use { response -> if (!response.isSuccessful) { - return@withContext Result.failure(Exception("HTTP ${response.code}: ${response.message}")) + return@withContext Result.failure( + JellyfinHttpException( + statusCode = response.code, + retryAfter = response.header("Retry-After"), + message = "HTTP ${response.code}: ${response.message}", + ) + ) } val responseBody = response.body.string() @@ -107,6 +114,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 +151,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..0c58c27281 --- /dev/null +++ b/app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinHttpException.kt @@ -0,0 +1,15 @@ +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]. + * + * [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, + * 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..c05dcc4003 --- /dev/null +++ b/app/src/test/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiServiceErrorHandlingTest.kt @@ -0,0 +1,200 @@ +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 + +/** + * [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. + */ +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()) + } + + // ─── Cancellation before the call starts ──────────────────────────────────── + // + // Same deterministic pattern as `GitHubAnnouncementPropertiesServiceTest`: 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. + // Note: this exercises `withContext`'s own pre-flight `ensureActive()`, not the + // `catch (e: CancellationException) { throw e }` lines below it — those are covered + // separately further down, by throwing mid-call instead of cancelling before the call. + + @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) + } + + @Test + fun `authenticateByName also propagates cancellation before the call starts`() = 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) + } + + // ─── Cancellation surfacing mid-call ───────────────────────────────────────── + // + // The tests above cancel *before* `request()`/`authenticateByName()` are even entered, so + // they never actually reach the `catch (e: CancellationException) { throw e }` lines inside + // those functions — deleting those lines wouldn't fail either test above. This one makes + // the interceptor throw `CancellationException` from inside the blocking `execute()` call, + // landing directly in that catch block, so a regression (removing the rethrow, or + // reordering it after the generic `catch (e: Exception)`) actually fails a test. + // + // Only `request()` (via [ping]) is covered this way, not `authenticateByName()`: this + // module runs with `unitTests.isReturnDefaultValues = true`, so the unmocked + // `org.json.JSONObject` used to build `authenticateByName()`'s POST body returns `null` + // from `toString()` before the interceptor is ever reached — no JVM test can exercise + // that function's body beyond the pre-flight cancellation case above. Its catch-block + // ordering was fixed identically to `request()`'s and reviewed by eye; a real test needs + // either Robolectric or a JSON dependency for this module, out of scope here. + + private fun serviceThrowingMidCall(exception: Throwable): JellyfinApiService { + val client = OkHttpClient.Builder() + .addInterceptor { throw exception } + .build() + return JellyfinApiService(client).apply { + setCredentials( + JellyfinCredentials( + serverUrl = "http://example.invalid", + username = "user", + password = "pw", + accessToken = "test-token", + userId = "user-1", + ) + ) + } + } + + @Test + fun `a CancellationException thrown mid-call by request() propagates uncaught, not wrapped in a Result`() = runTest { + val service = serviceThrowingMidCall(CancellationException("simulated mid-call cancellation")) + + var caught: CancellationException? = null + try { + service.ping() + } catch (e: CancellationException) { + caught = e + } + + assertTrue(caught != null) + } +}