From e041fc1e682dad0dbaa094d874b3dc69db647e5b Mon Sep 17 00:00:00 2001 From: Katherine Blizard <414924+kabliz@users.noreply.github.com> Date: Fri, 9 Aug 2024 14:03:56 -0700 Subject: [PATCH 1/2] [APT-9578] Improve Exception Handling ArmadilloException needs to support `cause` and `message` in the way `Exception` and most programmers expect them to be used. Avoids initializing new `Exception()` wherever possible to prevent obfuscating the origin of an error. Never uses `object` for Exceptions, as each carry specific information on their stacktrace. Exception messages need to be written in plain English, with indication of what component is failing when it fails. And ideally, with a hint on how to fix these errors. --- .../armadillo/ArmadilloPlayerChoreographer.kt | 32 ++++--- .../main/java/com/scribd/armadillo/Reducer.kt | 2 +- .../java/com/scribd/armadillo/StateStore.kt | 2 +- .../main/java/com/scribd/armadillo/Util.kt | 2 + .../armadillo/download/DownloadEngine.kt | 7 +- .../download/ExoplayerDownloadTracker.kt | 4 +- .../armadillo/error/ArmadilloException.kt | 91 ++++++++++++------- .../scribd/armadillo/models/ArmadilloState.kt | 3 +- .../playback/ExoPlaybackExceptionExt.kt | 4 +- .../playback/MediaSessionConnection.kt | 2 +- .../armadillo/playback/PlaybackEngine.kt | 2 +- .../armadillo/playback/PlaybackService.kt | 4 +- .../download/ExoplayerDownloadTrackerTest.kt | 7 +- RELEASE.md | 3 + gradle.properties | 2 +- 15 files changed, 105 insertions(+), 62 deletions(-) diff --git a/Armadillo/src/main/java/com/scribd/armadillo/ArmadilloPlayerChoreographer.kt b/Armadillo/src/main/java/com/scribd/armadillo/ArmadilloPlayerChoreographer.kt index 45d6a94..a90744b 100644 --- a/Armadillo/src/main/java/com/scribd/armadillo/ArmadilloPlayerChoreographer.kt +++ b/Armadillo/src/main/java/com/scribd/armadillo/ArmadilloPlayerChoreographer.kt @@ -241,7 +241,7 @@ internal class ArmadilloPlayerChoreographer : ArmadilloPlayer { override fun beginDownload(audioPlayable: AudioPlayable) { if (!isDownloadEngineInit) { - stateModifier.dispatch(ErrorAction(EngineNotInitialized("download engine not init"))) + stateModifier.dispatch(ErrorAction(EngineNotInitialized("download engine cannot start download."))) return } downloadEngine.download(audioPlayable) @@ -253,7 +253,7 @@ internal class ArmadilloPlayerChoreographer : ArmadilloPlayer { override fun removeAllDownloads() { if (!isDownloadEngineInit) { - stateModifier.dispatch(ErrorAction(EngineNotInitialized("download engine not init"))) + stateModifier.dispatch(ErrorAction(EngineNotInitialized("Cannot remove all the downloads."))) return } downloadEngine.removeAllDownloads() @@ -308,7 +308,10 @@ internal class ArmadilloPlayerChoreographer : ArmadilloPlayer { PlaybackState.PLAYING -> controls.pause() PlaybackState.PAUSED -> controls.play() else -> { - stateModifier.dispatch(ErrorAction(UnexpectedException("Unknown state: $playbackState"))) + stateModifier.dispatch(ErrorAction( + UnexpectedException(cause = IllegalStateException("Neither playing nor paused"), + actionThatFailedMessage = "Trying to play or pause media.")) + ) } } } @@ -333,7 +336,10 @@ internal class ArmadilloPlayerChoreographer : ArmadilloPlayer { override fun seekWithinChapter(percent: Int) { val position = stateProvider.currentState.positionFromChapterPercent(percent) ?: run { - stateModifier.dispatch(ErrorAction(UnexpectedException("No audiobook playing"))) + stateModifier.dispatch(ErrorAction( + UnexpectedException(cause = KotlinNullPointerException("Current state's position is null"), + actionThatFailedMessage = "seeking within chapter")) + ) return } seekTo(position) @@ -341,7 +347,7 @@ internal class ArmadilloPlayerChoreographer : ArmadilloPlayer { override fun removeDownload(audioPlayable: AudioPlayable) { if (!isDownloadEngineInit) { - stateModifier.dispatch(ErrorAction(EngineNotInitialized("DownloadEngine"))) + stateModifier.dispatch(ErrorAction(EngineNotInitialized("Cannot remove a download."))) return } downloadEngine.removeDownload(audioPlayable) @@ -412,18 +418,16 @@ internal class ArmadilloPlayerChoreographer : ArmadilloPlayer { lambda.invoke(controls, playbackState) } else { val error = if (controls == null) { - Log.e(TAG, "transportControls null") - TransportControlsNull + Log.e(TAG, "The transport controls are null. Are the controls initialized?") + TransportControlsNull() } else if (playbackState == null) { - Log.e(TAG, "playbackState is null") - NoPlaybackInfo + Log.e(TAG, "The playbackState is null.") + NoPlaybackInfo() } else if (PlaybackState.NONE == playbackState) { - Log.e(TAG, "playbackState == PlaybackState.NONE") - InvalidPlaybackState - } else if (!playbackReady) { - EngineNotInitialized("isPlaybackEngineReady == false") + Log.e(TAG, "There is no playback State.") + InvalidPlaybackState() } else { - UnexpectedException("unknown error in $TAG.doIfPlaybackReady") + EngineNotInitialized("Playback is not ready.") } stateModifier.dispatch(ErrorAction(error)) diff --git a/Armadillo/src/main/java/com/scribd/armadillo/Reducer.kt b/Armadillo/src/main/java/com/scribd/armadillo/Reducer.kt index 5f60b77..76aba82 100644 --- a/Armadillo/src/main/java/com/scribd/armadillo/Reducer.kt +++ b/Armadillo/src/main/java/com/scribd/armadillo/Reducer.kt @@ -282,7 +282,7 @@ internal object Reducer { private fun checkBounds(timeToCheck: () -> Interval): ArmadilloException? { return if (maxDurationDiscrepancy >= 0 && timeToCheck.invoke().inSeconds.longValue > maxDurationDiscrepancy) { Log.e(TAG, "Content metadata is incorrect") - IncorrectChapterMetadataException + IncorrectChapterMetadataException() } else { null } diff --git a/Armadillo/src/main/java/com/scribd/armadillo/StateStore.kt b/Armadillo/src/main/java/com/scribd/armadillo/StateStore.kt index 0920de5..1b05638 100644 --- a/Armadillo/src/main/java/com/scribd/armadillo/StateStore.kt +++ b/Armadillo/src/main/java/com/scribd/armadillo/StateStore.kt @@ -49,5 +49,5 @@ internal class ArmadilloStateStore(private val reducer: Reducer) : get() = armadilloStateObservable override val currentState: ArmadilloState - get() = armadilloStateObservable.value ?: throw MissingDataException("armadilloState should never be null") + get() = armadilloStateObservable.value ?: throw MissingDataException("Armadillo's State should never be null") } \ No newline at end of file diff --git a/Armadillo/src/main/java/com/scribd/armadillo/Util.kt b/Armadillo/src/main/java/com/scribd/armadillo/Util.kt index 268c1f7..9ccb077 100644 --- a/Armadillo/src/main/java/com/scribd/armadillo/Util.kt +++ b/Armadillo/src/main/java/com/scribd/armadillo/Util.kt @@ -2,6 +2,7 @@ package com.scribd.armadillo import android.content.Context import android.os.Build +import androidx.annotation.ChecksSdkIntAtLeast import com.scribd.armadillo.models.Chapter import com.scribd.armadillo.time.Interval import com.scribd.armadillo.time.Millisecond @@ -46,4 +47,5 @@ fun sanitizeChapters(chapters: List): List { } } +@ChecksSdkIntAtLeast(api = Build.VERSION_CODES.S) fun hasSnowCone() = Build.VERSION.SDK_INT >= Build.VERSION_CODES.S \ No newline at end of file diff --git a/Armadillo/src/main/java/com/scribd/armadillo/download/DownloadEngine.kt b/Armadillo/src/main/java/com/scribd/armadillo/download/DownloadEngine.kt index 685cb77..fafda84 100644 --- a/Armadillo/src/main/java/com/scribd/armadillo/download/DownloadEngine.kt +++ b/Armadillo/src/main/java/com/scribd/armadillo/download/DownloadEngine.kt @@ -17,6 +17,7 @@ import com.scribd.armadillo.StateStore import com.scribd.armadillo.actions.ErrorAction import com.scribd.armadillo.download.drm.OfflineDrmManager import com.scribd.armadillo.error.ArmadilloException +import com.scribd.armadillo.error.ArmadilloIOException import com.scribd.armadillo.error.DownloadServiceLaunchedInBackground import com.scribd.armadillo.error.UnexpectedDownloadException import com.scribd.armadillo.extensions.encodeInByteArray @@ -80,15 +81,15 @@ internal class ExoplayerDownloadEngine @Inject constructor( startDownload(context, request) } catch (e: Exception) { if (hasSnowCone() && e is ForegroundServiceStartNotAllowedException) { - stateModifier.dispatch(ErrorAction(DownloadServiceLaunchedInBackground(audioPlayable.id))) + stateModifier.dispatch(ErrorAction(DownloadServiceLaunchedInBackground(audioPlayable.id, e))) } else { - stateModifier.dispatch(ErrorAction(com.scribd.armadillo.error.ArmadilloIOException(e))) + stateModifier.dispatch(ErrorAction(ArmadilloIOException(cause = e, whatActionFailedMessage = "Can't prepare download."))) } } } override fun onPrepareError(helper: DownloadHelper, e: IOException) = - stateModifier.dispatch(ErrorAction(com.scribd.armadillo.error.ArmadilloIOException(e))) + stateModifier.dispatch(ErrorAction(ArmadilloIOException(cause = e, whatActionFailedMessage = "Can't report download error."))) }) } } diff --git a/Armadillo/src/main/java/com/scribd/armadillo/download/ExoplayerDownloadTracker.kt b/Armadillo/src/main/java/com/scribd/armadillo/download/ExoplayerDownloadTracker.kt index 9952d56..b200cd3 100644 --- a/Armadillo/src/main/java/com/scribd/armadillo/download/ExoplayerDownloadTracker.kt +++ b/Armadillo/src/main/java/com/scribd/armadillo/download/ExoplayerDownloadTracker.kt @@ -96,7 +96,7 @@ internal class ExoplayerDownloadTracker @Inject constructor( } catch (e: IOException) { Log.e(TAG, "Failed to load downloads", e) withContext(Dispatchers.Main) { - stateModifier.dispatch(ErrorAction(UnableToLoadDownloadInfo)) + stateModifier.dispatch(ErrorAction(UnableToLoadDownloadInfo())) } } } @@ -166,7 +166,7 @@ internal class ExoplayerDownloadTracker @Inject constructor( } if (taskFailed) { - actions.add(ErrorAction(DownloadFailed)) + actions.add(ErrorAction(DownloadFailed())) } actions.forEach { diff --git a/Armadillo/src/main/java/com/scribd/armadillo/error/ArmadilloException.kt b/Armadillo/src/main/java/com/scribd/armadillo/error/ArmadilloException.kt index f7c53d9..e28d062 100644 --- a/Armadillo/src/main/java/com/scribd/armadillo/error/ArmadilloException.kt +++ b/Armadillo/src/main/java/com/scribd/armadillo/error/ArmadilloException.kt @@ -2,84 +2,103 @@ package com.scribd.armadillo.error import com.scribd.armadillo.actions.Action -sealed class ArmadilloException(exception: Exception) : Exception(exception) { +sealed class ArmadilloException(cause: Throwable? = null, + message: String = "Unhandled Armadillo Error") + : Exception(message, cause) { abstract val errorCode: Int } /** * Errors in Armadillo */ -data class EngineNotInitialized(val reason: String) : ArmadilloException(exception = Exception(reason)) { +class EngineNotInitialized(message: String) + : ArmadilloException(message = "Armadillo Engine is not initialized: $message") { override val errorCode = 100 } -object TransportControlsNull : ArmadilloException(Exception()) { +class TransportControlsNull + : ArmadilloException(message = "Internal error, the transport controls are null. Are the controls being initialized?") { override val errorCode = 101 } -data class MissingDataException(val reason: String) : ArmadilloException(exception = Exception(reason)) { +class MissingDataException(message: String) + : ArmadilloException(message = "Internal error, unexpectedly missing internal data: $message") { override val errorCode = 102 } -data class ActionBeforeSetup(val action: Action) : ArmadilloException(exception = Exception(action.name)) { +data class ActionBeforeSetup(val action: Action) + : ArmadilloException(message = "Internal Error, using an Action before setup is finished: ${action.name}") { override val errorCode = 103 } -data class UnrecognizedAction(val action: Action) : ArmadilloException(exception = Exception(action.name)) { +data class UnrecognizedAction(val action: Action) + : ArmadilloException(message = "Internal error, unknown action being taken: ${action.name}") { override val errorCode = 104 } -object NoPlaybackInfo : ArmadilloException(Exception()) { +class NoPlaybackInfo : ArmadilloException(message = "Internal error, playback info is missing. Is the player initialized?") { override val errorCode = 105 } -object InvalidPlaybackState : ArmadilloException(Exception()) { +class InvalidPlaybackState + : ArmadilloException(message = "Internal Error, playback state is missing. Has the player been initialized or destroyed?") { override val errorCode = 106 } -data class InvalidRequest(val reason: String) : ArmadilloException(exception = Exception(reason)) { +class InvalidRequest(message: String) + : ArmadilloException(message = "Internal Error, certain media requests cannot be taken: $message") { override val errorCode: Int = 107 } /** * Playback Errors */ -data class HttpResponseCodeException(val responseCode: Int, val url: String?, val exception: Exception) : ArmadilloException(exception) { +data class HttpResponseCodeException(val responseCode: Int, val url: String?, override val cause: Exception) + : ArmadilloException(cause = cause, message = "HTTP Error $responseCode.") { override val errorCode: Int = 200 } -data class ArmadilloIOException(val exception: Exception) : ArmadilloException(exception) { +class ArmadilloIOException(cause: Exception, whatActionFailedMessage: String) + : ArmadilloException(cause = cause, message = "A critical playback issue occurred: $whatActionFailedMessage") { override val errorCode = 201 } -data class UpdateProgressFailureException(val exception: Exception) : ArmadilloException(exception) { +class UpdateProgressFailureException(cause: Exception) + : ArmadilloException(cause = cause, message = "Progress failed to update suddenly during playback. Download progress or playback " + + "progress may not be reporting correctly.") { override val errorCode = 202 } -data class PlaybackStartFailureException(val exception: Exception) : ArmadilloException(exception) { +class PlaybackStartFailureException(cause: Exception) + : ArmadilloException(cause = cause, message = "The player is initialized, but it failed to begin.") { override val errorCode = 203 } -data class ActionListenerException(val exception: Exception) : ArmadilloException(exception) { +class ActionListenerException(cause: Exception) + : ArmadilloException(cause = cause, message = "A problem occurred processing state updates.") { override val errorCode = 204 } -object IncorrectChapterMetadataException : ArmadilloException(Exception()) { +class IncorrectChapterMetadataException + : ArmadilloException(message = "The metadata for chapter durations are wrong and cannot be used for this content.") { override val errorCode = 205 } /** * Download Errors */ -data class MissingInfoDownloadException(val reason: String) : ArmadilloException(exception = Exception(reason)) { +class MissingInfoDownloadException(message: String) + : ArmadilloException(message = "Download info is missing: $message") { override val errorCode = 301 } -object DownloadFailed : ArmadilloException(Exception()) { +class DownloadFailed + : ArmadilloException(message = "The download has failed to finish.") { override val errorCode = 302 } -object UnableToLoadDownloadInfo : ArmadilloException(Exception()) { +class UnableToLoadDownloadInfo + : ArmadilloException(message = "Failed to load download metadata, queued downloads may fail to resume.") { override val errorCode = 303 } @@ -89,25 +108,28 @@ object UnableToLoadDownloadInfo : ArmadilloException(Exception()) { * the background before DownloadService.sendAddDownload */ -data class DownloadServiceLaunchedInBackground(val id: Int) : ArmadilloException(Exception()) { +data class DownloadServiceLaunchedInBackground(val id: Int, override val cause: Exception) + : ArmadilloException(cause = cause, + message = "Android 12 compliance problem: ${cause.message}") { override val errorCode = 304 } -data class UnexpectedDownloadException(val throwable: Throwable): ArmadilloException(exception = Exception(throwable)){ +class UnexpectedDownloadException(throwable: Throwable) + : ArmadilloException(cause = throwable, message = "Unknown problem while downloading."){ override val errorCode = 305 } /** * Misc Errors */ -data class UnexpectedException(val exception: Exception) : ArmadilloException(exception) { - constructor(reason: String) : this(Exception(reason)) - +class UnexpectedException(cause: Exception, actionThatFailedMessage: String) + : ArmadilloException(cause = cause, message = "Unanticipated error: $actionThatFailedMessage.") { override val errorCode = 400 } /** Media Browse Errors */ -data class BadMediaHierarchyException(val reason: String) : ArmadilloException(exception = Exception(reason)) { +class BadMediaHierarchyException(cause: Exception?, message: String) + : ArmadilloException(cause = cause, message = message) { override val errorCode: Int = 501 } @@ -115,19 +137,23 @@ data class BadMediaHierarchyException(val reason: String) : ArmadilloException(e * Audio Renderer Errors */ -data class RendererConfigurationException(val exception: Exception) : ArmadilloException(exception) { +class RendererConfigurationException(cause: Exception) + : ArmadilloException(cause = cause, message = "The audio data buffer (AudioSink) has been misconfigured.") { override val errorCode: Int = 601 } -data class RendererInitializationException(val exception: Exception) : ArmadilloException(exception) { +class RendererInitializationException(cause: Exception) + : ArmadilloException(cause = cause, message = "The audio data buffer (AudioSink) isn't initialized.") { override val errorCode: Int = 602 } -data class RendererWriteException(val exception: Exception) : ArmadilloException(exception) { +class RendererWriteException(cause: Exception) + : ArmadilloException(cause = cause, message = "Cannot write data into the audio data buffer (AudioSink).") { override val errorCode: Int = 603 } -data class UnknownRendererException(val exception: Exception) : ArmadilloException(exception) { +class UnknownRendererException(cause: Exception) + : ArmadilloException(cause = cause, message = "Unknown problem with the audio data buffer (AudioSink).") { override val errorCode: Int = 604 } @@ -135,15 +161,18 @@ data class UnknownRendererException(val exception: Exception) : ArmadilloExcepti * DRM errors */ -data class DrmContentTypeUnsupportedException(val contentType: Int) : ArmadilloException(exception = Exception()) { +data class DrmContentTypeUnsupportedException(val contentType: Int) + : ArmadilloException(message = "This DRM content type is not yet supported. Please consider opening a pull request.") { override val errorCode = 700 } -data class DrmDownloadException(val exception: Exception) : ArmadilloException(exception) { +class DrmDownloadException(cause: Exception) + : ArmadilloException(cause = cause, "Failed to process DRM license for downloading.") { override val errorCode = 701 } -data class DrmPlaybackException(val exception: Exception) : ArmadilloException(exception) { +class DrmPlaybackException(cause: Exception) + : ArmadilloException(cause = cause, message = "Cannot play DRM content.") { override val errorCode = 702 } diff --git a/Armadillo/src/main/java/com/scribd/armadillo/models/ArmadilloState.kt b/Armadillo/src/main/java/com/scribd/armadillo/models/ArmadilloState.kt index b51fb44..937f481 100644 --- a/Armadillo/src/main/java/com/scribd/armadillo/models/ArmadilloState.kt +++ b/Armadillo/src/main/java/com/scribd/armadillo/models/ArmadilloState.kt @@ -31,7 +31,8 @@ data class ArmadilloState( return null } if (percent > 100 || percent < 0) { - throw UnexpectedException(Exception("Invalid argument: $percent")) + throw UnexpectedException(cause = IllegalArgumentException("Invalid argument: $percent"), + actionThatFailedMessage = "Calculating seekbar progress from percentage.") } val currentChapter = playbackInfo.audioPlayable.chapters[playbackInfo.progress.currentChapterIndex] val decimal = percent / 100.toDouble() diff --git a/Armadillo/src/main/java/com/scribd/armadillo/playback/ExoPlaybackExceptionExt.kt b/Armadillo/src/main/java/com/scribd/armadillo/playback/ExoPlaybackExceptionExt.kt index 3771541..7648402 100644 --- a/Armadillo/src/main/java/com/scribd/armadillo/playback/ExoPlaybackExceptionExt.kt +++ b/Armadillo/src/main/java/com/scribd/armadillo/playback/ExoPlaybackExceptionExt.kt @@ -27,7 +27,7 @@ internal fun ExoPlaybackException.toArmadilloException(): ArmadilloException { is SocketTimeoutException -> HttpResponseCodeException(0, null, source) is UnknownHostException -> HttpResponseCodeException(0, source.message, source) // Message is supposed to be the host for UnknownHostException - else -> ArmadilloIOException(this) + else -> ArmadilloIOException(cause = this, whatActionFailedMessage = "Exoplayer error.") } } } else if (TYPE_RENDERER == type) { @@ -40,6 +40,6 @@ internal fun ExoPlaybackException.toArmadilloException(): ArmadilloException { } } } else { - UnexpectedException(this) + UnexpectedException(cause = this, actionThatFailedMessage = "Exoplayer error") } } \ No newline at end of file diff --git a/Armadillo/src/main/java/com/scribd/armadillo/playback/MediaSessionConnection.kt b/Armadillo/src/main/java/com/scribd/armadillo/playback/MediaSessionConnection.kt index 6b7e7d5..18f8510 100644 --- a/Armadillo/src/main/java/com/scribd/armadillo/playback/MediaSessionConnection.kt +++ b/Armadillo/src/main/java/com/scribd/armadillo/playback/MediaSessionConnection.kt @@ -48,7 +48,7 @@ internal class MediaSessionConnection( * completed. */ override fun onConnected() { - val mediaBrowserCompat = mediaBrowser ?: throw MissingDataException("media browser should be init") + val mediaBrowserCompat = mediaBrowser ?: throw MissingDataException("Media Browser needs to be initialized.") val mediaControllerCompat = MediaControllerCompat(context, mediaBrowserCompat.sessionToken) mediaController = mediaControllerCompat connectionListener.onConnectionCallback(mediaControllerCompat.transportControls) diff --git a/Armadillo/src/main/java/com/scribd/armadillo/playback/PlaybackEngine.kt b/Armadillo/src/main/java/com/scribd/armadillo/playback/PlaybackEngine.kt index 9bd94b9..6094200 100644 --- a/Armadillo/src/main/java/com/scribd/armadillo/playback/PlaybackEngine.kt +++ b/Armadillo/src/main/java/com/scribd/armadillo/playback/PlaybackEngine.kt @@ -109,7 +109,7 @@ internal class ExoplayerPlaybackEngine(private var audioPlayable: AudioPlayable) private val currentChapter: Chapter get() = audioPlayable.getChapterAtOffset(exoPlayer.currentPositionInDuration()) - ?: throw MissingDataException("currentChapter null") + ?: throw MissingDataException("The current chapter is missing.") override var offloadAudio: Boolean = false set(value) { diff --git a/Armadillo/src/main/java/com/scribd/armadillo/playback/PlaybackService.kt b/Armadillo/src/main/java/com/scribd/armadillo/playback/PlaybackService.kt index aa3646f..4e77a60 100644 --- a/Armadillo/src/main/java/com/scribd/armadillo/playback/PlaybackService.kt +++ b/Armadillo/src/main/java/com/scribd/armadillo/playback/PlaybackService.kt @@ -227,7 +227,7 @@ class PlaybackService : MediaBrowserServiceCompat() { private inner class PlaybackServiceManager : ServiceManager { override fun startService(audiobook: AudioPlayable, currentChapterIndex: Int) { - val token = sessionToken ?: throw MissingDataException("token should not be null") + val token = sessionToken ?: throw MissingDataException("The session's token is missing. Can't begin service.") if (!isInForeground) { ContextCompat.startForegroundService( this@PlaybackService, @@ -241,7 +241,7 @@ class PlaybackService : MediaBrowserServiceCompat() { } override fun updateNotificationForPause(audiobook: AudioPlayable, currentChapterIndex: Int) { - val token = sessionToken ?: throw MissingDataException("token should not be null") + val token = sessionToken ?: throw MissingDataException("The session's token is missing. Cannot pause the notification.") stopForeground(false) val notification = playbackNotificationManager.getNotification(audiobook, currentChapterIndex, false, token) notificationDeleteReceiver.setDeleteIntentOnNotification(notification) diff --git a/Armadillo/src/test/java/com/scribd/armadillo/download/ExoplayerDownloadTrackerTest.kt b/Armadillo/src/test/java/com/scribd/armadillo/download/ExoplayerDownloadTrackerTest.kt index 33d1af5..da14219 100644 --- a/Armadillo/src/test/java/com/scribd/armadillo/download/ExoplayerDownloadTrackerTest.kt +++ b/Armadillo/src/test/java/com/scribd/armadillo/download/ExoplayerDownloadTrackerTest.kt @@ -6,13 +6,15 @@ import com.scribd.armadillo.StateStore import com.scribd.armadillo.actions.ErrorAction import com.scribd.armadillo.actions.StopTrackingDownloadAction import com.scribd.armadillo.actions.UpdateDownloadAction -import com.scribd.armadillo.error.DownloadFailed import com.scribd.armadillo.models.DownloadProgressInfo import com.scribd.armadillo.models.DownloadState import org.junit.Before import org.junit.Test import org.junit.runner.RunWith +import org.mockito.kotlin.any +import org.mockito.kotlin.isA import org.mockito.kotlin.mock +import org.mockito.kotlin.times import org.mockito.kotlin.verify import org.mockito.kotlin.verifyNoMoreInteractions import org.mockito.kotlin.whenever @@ -46,8 +48,9 @@ class ExoplayerDownloadTrackerTest { downloadInfo = DownloadProgressInfo(ID, URL, DownloadState.FAILED) exoplayerDownloadTracker.dispatchActionsForProgress(downloadInfo) verify(stateModifier).dispatch(UpdateDownloadAction(downloadInfo)) - verify(stateModifier).dispatch(ErrorAction(DownloadFailed)) + verify(stateModifier).dispatch(isA()) verify(stateModifier).dispatch(StopTrackingDownloadAction(downloadInfo)) + verify(stateModifier, times(3)).dispatch(any()) verifyNoMoreInteractions(stateModifier) } diff --git a/RELEASE.md b/RELEASE.md index 23b17e7..67f8099 100644 --- a/RELEASE.md +++ b/RELEASE.md @@ -1,5 +1,8 @@ # Project Armadillo Release Notes +## 1.5 +- Fixes Error and Exception handling to not hide underlying exceptions, and to clearly explain the nature of errors. + ## 1.4.1 - Fixes Download Service issues on Android 14. - Removes the Exoplayer 2.19.1 `core` module that does not support Android 14. This module must now be included separately by your diff --git a/gradle.properties b/gradle.properties index a1997b5..2f7ee6e 100644 --- a/gradle.properties +++ b/gradle.properties @@ -13,7 +13,7 @@ org.gradle.jvmargs=-Xmx1536m # org.gradle.parallel=true PACKAGE_NAME=com.scribd.armadillo GRADLE_PLUGIN_VERSION=7.2.0 -LIBRARY_VERSION=1.4.1 +LIBRARY_VERSION=1.5 EXOPLAYER_VERSION=2.19.1 RXJAVA_VERSION=2.2.4 RXANDROID_VERSION=2.0.1 From 6155a7913644259636e4f6f7a93b325f0cfa321f Mon Sep 17 00:00:00 2001 From: Katherine Blizard <414924+kabliz@users.noreply.github.com> Date: Wed, 14 Aug 2024 09:58:37 -0700 Subject: [PATCH 2/2] [APT-9578] Improve Exception Handling ArmadilloException needs to support `cause` and `message` in the way `Exception` and most programmers expect them to be used. Avoids initializing new `Exception()` wherever possible to prevent obfuscating the origin of an error. Never uses `object` for Exceptions, as each carry specific information on their stacktrace. Exception messages need to be written in plain English, with indication of what component is failing when it fails. And ideally, with a hint on how to fix these errors. --- .../main/java/com/scribd/armadillo/download/DownloadEngine.kt | 4 ++-- .../java/com/scribd/armadillo/error/ArmadilloException.kt | 4 ++-- .../com/scribd/armadillo/playback/ExoPlaybackExceptionExt.kt | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Armadillo/src/main/java/com/scribd/armadillo/download/DownloadEngine.kt b/Armadillo/src/main/java/com/scribd/armadillo/download/DownloadEngine.kt index fafda84..c3206ec 100644 --- a/Armadillo/src/main/java/com/scribd/armadillo/download/DownloadEngine.kt +++ b/Armadillo/src/main/java/com/scribd/armadillo/download/DownloadEngine.kt @@ -83,13 +83,13 @@ internal class ExoplayerDownloadEngine @Inject constructor( if (hasSnowCone() && e is ForegroundServiceStartNotAllowedException) { stateModifier.dispatch(ErrorAction(DownloadServiceLaunchedInBackground(audioPlayable.id, e))) } else { - stateModifier.dispatch(ErrorAction(ArmadilloIOException(cause = e, whatActionFailedMessage = "Can't prepare download."))) + stateModifier.dispatch(ErrorAction(ArmadilloIOException(cause = e, actionThatFailedMessage = "Can't prepare download."))) } } } override fun onPrepareError(helper: DownloadHelper, e: IOException) = - stateModifier.dispatch(ErrorAction(ArmadilloIOException(cause = e, whatActionFailedMessage = "Can't report download error."))) + stateModifier.dispatch(ErrorAction(ArmadilloIOException(cause = e, actionThatFailedMessage = "Can't report download error."))) }) } } diff --git a/Armadillo/src/main/java/com/scribd/armadillo/error/ArmadilloException.kt b/Armadillo/src/main/java/com/scribd/armadillo/error/ArmadilloException.kt index e28d062..86b8db3 100644 --- a/Armadillo/src/main/java/com/scribd/armadillo/error/ArmadilloException.kt +++ b/Armadillo/src/main/java/com/scribd/armadillo/error/ArmadilloException.kt @@ -58,8 +58,8 @@ data class HttpResponseCodeException(val responseCode: Int, val url: String?, ov override val errorCode: Int = 200 } -class ArmadilloIOException(cause: Exception, whatActionFailedMessage: String) - : ArmadilloException(cause = cause, message = "A critical playback issue occurred: $whatActionFailedMessage") { +class ArmadilloIOException(cause: Exception, actionThatFailedMessage: String) + : ArmadilloException(cause = cause, message = "A critical playback issue occurred: $actionThatFailedMessage") { override val errorCode = 201 } diff --git a/Armadillo/src/main/java/com/scribd/armadillo/playback/ExoPlaybackExceptionExt.kt b/Armadillo/src/main/java/com/scribd/armadillo/playback/ExoPlaybackExceptionExt.kt index 7648402..41c5c70 100644 --- a/Armadillo/src/main/java/com/scribd/armadillo/playback/ExoPlaybackExceptionExt.kt +++ b/Armadillo/src/main/java/com/scribd/armadillo/playback/ExoPlaybackExceptionExt.kt @@ -27,7 +27,7 @@ internal fun ExoPlaybackException.toArmadilloException(): ArmadilloException { is SocketTimeoutException -> HttpResponseCodeException(0, null, source) is UnknownHostException -> HttpResponseCodeException(0, source.message, source) // Message is supposed to be the host for UnknownHostException - else -> ArmadilloIOException(cause = this, whatActionFailedMessage = "Exoplayer error.") + else -> ArmadilloIOException(cause = this, actionThatFailedMessage = "Exoplayer error.") } } } else if (TYPE_RENDERER == type) {