Imrprove Firestore integration error handling (#206)

Co-authored-by: Joseph Cooper <[email protected]>
OmriCat · Sep 16, 2023 · 62cec63 · 62cec63
1 parent 9ded416
commit 62cec63
Show file tree

Hide file tree

Showing 12 changed files with 154 additions and 166 deletions.
diff --git a/.github/workflows/data-integration-tests.yml b/.github/workflows/data-integration-tests.yml
@@ -112,7 +112,7 @@ jobs:
             adb uninstall "com.omricat.maplibrarian.integrationtesting.debug" || true
             adb uninstall "com.omricat.maplibrarian.debug" || true
             ./gradlew installDebug
-            ./scripts/capture-logcat-logs.sh adb shell am instrument -w com.omricat.maplibrarian.integrationtesting.debug/androidx.test.runner.AndroidJUnitRunner
+            ./scripts/capture-logcat-logs.sh ./scripts/run-instrumented-integration-tests.sh
 
       - name: Extract logs from firebase emulator container
         if: ${{ always() }}

diff --git a/app/src/main/kotlin/com/omricat/maplibrarian/chartlist/FirebaseChartsRepository.kt b/app/src/main/kotlin/com/omricat/maplibrarian/chartlist/FirebaseChartsRepository.kt
@@ -10,19 +10,33 @@ import com.github.michaelbull.result.onFailure
 import com.google.firebase.firestore.DocumentSnapshot
 import com.google.firebase.firestore.FirebaseFirestore
 import com.google.firebase.firestore.FirebaseFirestoreException
+import com.google.firebase.firestore.FirebaseFirestoreException.Code.ALREADY_EXISTS
+import com.google.firebase.firestore.FirebaseFirestoreException.Code.CANCELLED
+import com.google.firebase.firestore.FirebaseFirestoreException.Code.DATA_LOSS
+import com.google.firebase.firestore.FirebaseFirestoreException.Code.INTERNAL
+import com.google.firebase.firestore.FirebaseFirestoreException.Code.OK
+import com.google.firebase.firestore.FirebaseFirestoreException.Code.UNAVAILABLE
+import com.google.firebase.firestore.FirebaseFirestoreException.Code.UNKNOWN
 import com.google.firebase.firestore.QuerySnapshot
 import com.omricat.firebase.interop.runCatchingFirebaseException
 import com.omricat.logging.Loggable
 import com.omricat.logging.Logger
 import com.omricat.logging.log
+import com.omricat.maplibrarian.chartlist.ChartsRepository.AddNewChartError
+import com.omricat.maplibrarian.chartlist.ChartsRepository.AddNewChartError.Cancelled
+import com.omricat.maplibrarian.chartlist.ChartsRepository.AddNewChartError.ChartExists
+import com.omricat.maplibrarian.chartlist.ChartsRepository.AddNewChartError.OtherException
+import com.omricat.maplibrarian.chartlist.ChartsRepository.AddNewChartError.Unavailable
+import com.omricat.maplibrarian.chartlist.ChartsRepository.Error.ExceptionWrappingError
+import com.omricat.maplibrarian.chartlist.ChartsRepository.Error.MessageError
 import com.omricat.maplibrarian.model.ChartId
 import com.omricat.maplibrarian.model.DbChartModel
 import com.omricat.maplibrarian.model.DbChartModelFromMapDeserializer
 import com.omricat.maplibrarian.model.UnsavedChartModel
 import com.omricat.maplibrarian.model.User
 import com.omricat.maplibrarian.model.serializedToMap
 import com.omricat.maplibrarian.utils.DispatcherProvider
-import com.omricat.maplibrarian.utils.logErrorAndMap
+import com.omricat.maplibrarian.utils.logAndMapException
 import kotlinx.coroutines.tasks.await
 import kotlinx.coroutines.withContext
 
@@ -31,7 +45,6 @@ class FirebaseChartsRepository(
     private val dispatchers: DispatcherProvider = DispatcherProvider.Default,
     override val logger: Logger
 ) : ChartsRepository, Loggable {
-
     override suspend fun chartsListForUser(
         user: User
     ): Result<List<DbChartModel>, ChartsRepository.Error> =
@@ -40,19 +53,18 @@ class FirebaseChartsRepository(
                     db.mapsCollection(user).get().await()
                 }
             }
-            .mapError(ChartsServiceError::fromThrowable)
-            .onFailure { log(Warn) { "$it" } }
+            .logAndMapException(::ExceptionWrappingError)
             .andThen { snapshot ->
                 snapshot
                     .map { m -> m.parseMapModel() }
                     .combine()
-                    .mapError { e -> ChartsServiceError(e.message) }
+                    .mapError { e -> MessageError(e.message) }
             }
 
     override suspend fun addNewChart(
         user: User,
         newChart: UnsavedChartModel
-    ): Result<DbChartModel, ChartsRepository.Error> {
+    ): Result<DbChartModel, AddNewChartError> {
         require(user.id == newChart.userId) {
             "UserId of newMap (was ${newChart.userId}) must be " +
                 "same as userId of user (was ${user.id})"
@@ -62,22 +74,30 @@ class FirebaseChartsRepository(
                     db.mapsCollection(user).add(newChart.serializedToMap()).await()
                 }
             }
-            .logErrorAndMap(ChartsServiceError::fromThrowable)
+            .logAndMapException { exception ->
+                when (exception.code) {
+                    UNAVAILABLE -> Unavailable
+                    ALREADY_EXISTS -> ChartExists(newChart)
+                    CANCELLED -> Cancelled
+                    INTERNAL -> error("Firebase threw an internal error. This is unrecoverable.")
+                    DATA_LOSS -> error("Firebase indicated unrecoverable data loss or corruption")
+                    OK ->
+                        error(
+                            "FirebaseFirestoreException $exception had a status code of OK. " +
+                                "Docs say this should never happen."
+                        )
+                    UNKNOWN -> OtherException(exception)
+                    else -> OtherException(exception)
+                }
+            }
+            .onFailure { log(Warn) { "error Adding New Chart: ${it.message}" } }
             .map { ref -> newChart.withChartId(ChartId(ref.id)) }
     }
 
     private fun FirebaseFirestore.mapsCollection(user: User) =
         collection("users").document(user.id.value).collection("maps")
 }
 
-/*
-   It is always safe to upcast ChartModel<Nothing?> to ChartModel<ChartId?> since the only
-   possible value for a val of type Nothing? is null.
-
-   It is safe to cast ChartModel<ChartId?> to ChartModel<ChartId> immediately after setting
-   the chartId parameter to a non-null value.
-*/
-@Suppress("UNCHECKED_CAST")
 private fun UnsavedChartModel.withChartId(chartId: ChartId): DbChartModel =
     DbChartModel(userId = userId, title = title, chartId = chartId)
 

diff --git a/app/src/main/kotlin/com/omricat/maplibrarian/utils/LogErrorAndMap.kt b/app/src/main/kotlin/com/omricat/maplibrarian/utils/LogErrorAndMap.kt
@@ -9,5 +9,6 @@ import com.omricat.logging.log
 
 context(Loggable)
 
-inline fun <V, E> Result<V, Throwable>.logErrorAndMap(transform: (Throwable) -> E): Result<V, E> =
-    this.onFailure { logger.log(Warn, throwable = it) { "" } }.mapError(transform)
+inline fun <V, E, T : Exception> Result<V, T>.logAndMapException(
+    transform: (T) -> E
+): Result<V, E> = this.onFailure { logger.log(Warn, throwable = it) { "" } }.mapError(transform)
diff --git a/core/src/main/kotlin/com/omricat/maplibrarian/chartlist/AddNewChartWorkflow.kt b/core/src/main/kotlin/com/omricat/maplibrarian/chartlist/AddNewChartWorkflow.kt
@@ -9,6 +9,7 @@ import com.omricat.maplibrarian.chartlist.AddNewChartWorkflow.Event.Saved
 import com.omricat.maplibrarian.chartlist.AddNewChartWorkflow.State
 import com.omricat.maplibrarian.chartlist.AddNewChartWorkflow.State.Editing
 import com.omricat.maplibrarian.chartlist.AddNewChartWorkflow.State.Saving
+import com.omricat.maplibrarian.chartlist.ChartsRepository.AddNewChartError
 import com.omricat.maplibrarian.model.ChartModel
 import com.omricat.maplibrarian.model.DbChartModel
 import com.omricat.maplibrarian.model.UnsavedChartModel
@@ -77,17 +78,18 @@ public class AddNewChartWorkflow(private val chartsRepository: ChartsRepository)
 
     override fun snapshotState(state: State): Snapshot = state.toSnapshot()
 
-    internal fun onErrorSaving(chart: UnsavedChartModel, e: ChartsRepository.Error) = action {
-        state = Editing(chart, errorMessage = e.message)
-    }
+    internal fun onErrorSaving(chart: UnsavedChartModel, e: ChartsRepository.AddNewChartError) =
+        action {
+            state = Editing(chart, errorMessage = e.message)
+        }
 
     internal fun onNewItemSaved(savedChart: DbChartModel) = action { setOutput(Saved) }
 
     private fun saveNewItem(
         user: User,
         chart: UnsavedChartModel
-    ): Worker<Result<DbChartModel, ChartsRepository.Error>> =
-        resultWorker(ChartsServiceError::fromThrowable) {
+    ): Worker<Result<DbChartModel, ChartsRepository.AddNewChartError>> =
+        resultWorker({ e -> AddNewChartError.OtherException(e) }) {
             chartsRepository.addNewChart(user, chart)
         }
 

diff --git a/core/src/main/kotlin/com/omricat/maplibrarian/chartlist/ChartsRepository.kt b/core/src/main/kotlin/com/omricat/maplibrarian/chartlist/ChartsRepository.kt
@@ -4,7 +4,6 @@ import com.github.michaelbull.result.Result
 import com.omricat.maplibrarian.model.DbChartModel
 import com.omricat.maplibrarian.model.UnsavedChartModel
 import com.omricat.maplibrarian.model.User
-import kotlinx.serialization.Serializable
 
 public interface ChartsRepository {
     public suspend fun chartsListForUser(
@@ -14,20 +13,28 @@ public interface ChartsRepository {
     public suspend fun addNewChart(
         user: User,
         newChart: UnsavedChartModel
-    ): Result<DbChartModel, ChartsRepository.Error>
+    ): Result<DbChartModel, AddNewChartError>
 
-    public sealed interface Error {
+    public interface Error {
         public val message: String
+
+        public data class MessageError(override val message: String) : Error
+
+        public data class ExceptionWrappingError(public val exception: Throwable) : Error {
+            override val message: String
+                get() = exception.message ?: "No message in exception $exception"
+        }
     }
-}
 
-// public typealias ChartsServiceError = ChartsService.Error
-@Serializable
-public data class ChartsServiceError(override val message: String) : ChartsRepository.Error {
-    private constructor(throwable: Throwable) : this(throwable.message ?: "Unknown error")
+    public sealed class AddNewChartError(public val message: String) {
+        public data object Unavailable : AddNewChartError("Service temporarily unavailable")
+
+        public data object Cancelled : AddNewChartError("Operation ")
+
+        public data class ChartExists(public val unsavedChartModel: UnsavedChartModel) :
+            AddNewChartError("Chart already exists: $unsavedChartModel")
 
-    public companion object {
-        public fun fromThrowable(throwable: Throwable): ChartsServiceError =
-            ChartsServiceError(throwable)
+        public data class OtherException(val exception: Throwable) :
+            AddNewChartError(exception.message ?: "No message in $exception")
     }
 }
diff --git a/core/src/main/kotlin/com/omricat/maplibrarian/chartlist/ChartsWorkflow.kt b/core/src/main/kotlin/com/omricat/maplibrarian/chartlist/ChartsWorkflow.kt
@@ -5,6 +5,7 @@ import com.github.michaelbull.result.getOrElse
 import com.github.michaelbull.result.map
 import com.omricat.maplibrarian.chartlist.ActualChartsWorkflow.Props
 import com.omricat.maplibrarian.chartlist.ChartsListWorkflow.Event.SelectItem
+import com.omricat.maplibrarian.chartlist.ChartsRepository.Error.ExceptionWrappingError
 import com.omricat.maplibrarian.chartlist.ChartsScreen.Loading
 import com.omricat.maplibrarian.chartlist.ChartsScreen.ShowError
 import com.omricat.maplibrarian.chartlist.ChartsWorkflowState.AddingItem
@@ -85,9 +86,7 @@ public class ActualChartsWorkflow(
             chartsRepository: ChartsRepository,
             user: User
         ): Worker<Result<List<DbChartModel>, ChartsRepository.Error>> =
-            resultWorker(ChartsServiceError::fromThrowable) {
-                chartsRepository.chartsListForUser(user)
-            }
+            resultWorker(::ExceptionWrappingError) { chartsRepository.chartsListForUser(user) }
     }
 }
 

diff --git a/core/src/test/kotlin/com/omricat/maplibrarian/chartlist/ChartsWorkflowTest.kt b/core/src/test/kotlin/com/omricat/maplibrarian/chartlist/ChartsWorkflowTest.kt
@@ -33,7 +33,9 @@ internal class ChartsWorkflowTest :
 
                 "transform ErrorLoadingCharts to RequestData" {
                     val state =
-                        ChartsWorkflowState.ErrorLoadingCharts(ChartsServiceError("Error message"))
+                        ChartsWorkflowState.ErrorLoadingCharts(
+                            ChartsRepository.Error.MessageError("Error message")
+                        )
 
                     ChartsWorkflowState.fromSnapshot(state.toSnapshot()) shouldBe
                         ChartsWorkflowState.RequestData

diff --git a/...-tests/firebase/src/main/kotlin/com/omricat/maplibrarian/firebase/FirebaseEndToEndTest.kt b/...-tests/firebase/src/main/kotlin/com/omricat/maplibrarian/firebase/FirebaseEndToEndTest.kt
@@ -1,27 +1,25 @@
 package com.omricat.maplibrarian.firebase
 
-import android.annotation.SuppressLint
 import assertk.all
 import assertk.assertThat
 import assertk.assertions.hasSize
 import assertk.assertions.isEqualTo
 import assertk.assertions.prop
 import com.github.michaelbull.result.getOrThrow
-import com.google.firebase.auth.FirebaseAuth
-import com.google.firebase.firestore.FirebaseFirestore
 import com.omricat.logging.test.TestLogger
 import com.omricat.maplibrarian.auth.EmailPasswordCredential
 import com.omricat.maplibrarian.auth.FirebaseUserRepository
 import com.omricat.maplibrarian.chartlist.FirebaseChartsRepository
-import com.omricat.maplibrarian.firebase.auth.FirebaseAuthEmulatorRestApi
-import com.omricat.maplibrarian.firebase.charts.FirebaseFirestoreRestApi
+import com.omricat.maplibrarian.firebase.TestFixtures.authApi
+import com.omricat.maplibrarian.firebase.TestFixtures.firebaseAuthInstance
+import com.omricat.maplibrarian.firebase.TestFixtures.firestoreApi
+import com.omricat.maplibrarian.firebase.TestFixtures.firestoreInstance
 import com.omricat.maplibrarian.model.UnsavedChartModel
 import com.omricat.result.assertk.isOk
 import kotlin.time.ExperimentalTime
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.test.runTest
 import org.junit.Before
-import org.junit.BeforeClass
 import org.junit.Test
 
 @Suppress("FunctionName")
@@ -65,49 +63,4 @@ class FirebaseEndToEndTest {
             prop("First item title") { it.first().title }.isEqualTo("New map")
         }
     }
-
-    companion object Fixtures {
-
-        // Not a problem to leak a Context in an instrumented test
-        @SuppressLint("StaticFieldLeak")
-        @JvmStatic
-        lateinit var firestoreInstance: FirebaseFirestore
-
-        @JvmStatic lateinit var firestoreApi: FirebaseFirestoreRestApi
-
-        @JvmStatic lateinit var firebaseAuthInstance: FirebaseAuth
-
-        @JvmStatic lateinit var authApi: FirebaseAuthEmulatorRestApi
-
-        @JvmStatic
-        @BeforeClass
-        fun setup() {
-            firestoreInstance =
-                FirebaseFirestore.getInstance(TestFixtures.app).apply {
-                    useEmulator(
-                        FirebaseEmulatorConnection.HOST,
-                        FirebaseEmulatorConnection.FIRESTORE_PORT
-                    )
-                }
-
-            firestoreApi =
-                FirebaseFirestoreRestApi(
-                    TestFixtures.projectId,
-                    TestFixtures.emulatorBaseUrl(FirebaseEmulatorConnection.FIRESTORE_PORT)
-                )
-
-            firebaseAuthInstance =
-                FirebaseAuth.getInstance(TestFixtures.app).apply {
-                    useEmulator(
-                        FirebaseEmulatorConnection.HOST,
-                        FirebaseEmulatorConnection.AUTH_PORT
-                    )
-                }
-            authApi =
-                FirebaseAuthEmulatorRestApi(
-                    TestFixtures.projectId,
-                    TestFixtures.emulatorBaseUrl(FirebaseEmulatorConnection.AUTH_PORT)
-                )
-        }
-    }
 }