MS-1382 Correct Response handling in backend api client

infra/backend-api/src/main/java/com/simprints/infra/backendapi/BackendApiClient.kt

@@ -5,6 +5,9 @@ import com.simprints.core.PackageVersionName
 import com.simprints.infra.authstore.AuthStore
 import com.simprints.infra.network.SimNetwork
 import com.simprints.infra.network.SimRemoteInterface
+import com.simprints.infra.network.exceptions.SyncCloudIntegrationException
+import retrofit2.HttpException
+import retrofit2.Response
 import javax.inject.Inject
 import javax.inject.Singleton
 import kotlin.coroutines.cancellation.CancellationException
@@ -15,7 +18,7 @@ import kotlin.reflect.KClass
  * This is the single entry point for obtaining API clients in the infra layer.
  */
 @Singleton
-class BackendApiClient @Inject internal constructor(
+class BackendApiClient @Inject constructor(
     private val simNetwork: SimNetwork,
     private val authStore: AuthStore,
     @param:DeviceID private val deviceId: String,
@@ -30,7 +33,7 @@ class BackendApiClient @Inject internal constructor(
         remoteInterface: KClass<T>,
         block: suspend (T) -> V,
     ): ApiResult<V> = try {
-        ApiResult.Success(getApiClient(remoteInterface, authStore.getFirebaseToken()).executeCall(block))
+        wrapInApiResponse(getApiClient(remoteInterface, authStore.getFirebaseToken()).executeCall(block))
     } catch (t: Throwable) {
         wrapException(t)
     }
@@ -44,7 +47,7 @@ class BackendApiClient @Inject internal constructor(
         remoteInterface: KClass<T>,
         block: suspend (T) -> V,
     ): ApiResult<V> = try {
-        ApiResult.Success(getApiClient(remoteInterface, null).executeCall(block))
+        wrapInApiResponse(getApiClient(remoteInterface, null).executeCall(block))
     } catch (t: Throwable) {
         wrapException(t)
     }
@@ -54,6 +57,20 @@ class BackendApiClient @Inject internal constructor(
         authToken: String?,
     ): SimNetwork.SimApiClient<T> = simNetwork.getSimApiClient(remoteInterface, deviceId, versionName, authToken)
 
+    private fun <V> wrapInApiResponse(data: V): ApiResult<V> = if (data is Response<*>) {
+        // In cases where requests meta-data is required, the API client will return a Response object,
+        // such requests will return the Response with the error code instead of throwing exception,
+        // so we need to check the request was successful manually and wrap it in ApiResult accordingly.
+        if (data.isSuccessful) {
+            ApiResult.Success(data)
+        } else {
+            wrapException(SyncCloudIntegrationException(cause = HttpException(data)))
+        }
+    } else {
+        // Non-response types will throw directly
+        ApiResult.Success(data)
+    }
+
     private fun <V> wrapException(t: Throwable): ApiResult.Failure<V> = when (t) {
         is CancellationException -> throw t // Maintain the coroutine control flow by rethrowing the cancellation exception
         else -> ApiResult.Failure(t)

infra/backend-api/src/test/java/com/simprints/infra/backendapi/BackendApiClientTest.kt

@@ -8,8 +8,10 @@ import io.mockk.*
 import io.mockk.impl.annotations.MockK
 import kotlinx.coroutines.CancellationException
 import kotlinx.coroutines.test.runTest
+import okhttp3.ResponseBody
 import org.junit.Before
 import org.junit.Test
+import retrofit2.Response
 import java.io.IOException
 
 internal class BackendApiClientTest {
@@ -57,6 +59,32 @@ internal class BackendApiClientTest {
         }
     }
 
+    @Test
+    fun `executeCall returns Success when api client returns successful response`() = runTest {
+        coEvery { apiClient.executeCall(any<suspend (TestRemoteInterface) -> Response<ResponseBody>>()) } returns
+            Response<ResponseBody>.success(ResponseBody.EMPTY)
+
+        val result = subject.executeCall(TestRemoteInterface::class) { "ignored" }
+
+        assertThat(result).isInstanceOf(ApiResult.Success::class.java)
+        coVerify(exactly = 1) {
+            simNetwork.getSimApiClient(TestRemoteInterface::class, deviceId, versionName, "token")
+        }
+    }
+
+    @Test
+    fun `executeCall returns Failure when api client returns failed response`() = runTest {
+        coEvery { apiClient.executeCall(any<suspend (TestRemoteInterface) -> Response<ResponseBody>>()) } returns
+            Response<ResponseBody>.error(426, ResponseBody.EMPTY)
+
+        val result = subject.executeCall(TestRemoteInterface::class) { "ignored" }
+
+        assertThat(result).isInstanceOf(ApiResult.Failure::class.java)
+        coVerify(exactly = 1) {
+            simNetwork.getSimApiClient(TestRemoteInterface::class, deviceId, versionName, "token")
+        }
+    }
+
     @Test
     fun `executeCall returns Failure when api client returns throws exception`() = runTest {
         val throwable = IOException("boom")

infra/event-sync/src/main/java/com/simprints/infra/eventsync/event/remote/EventRemoteDataSource.kt

@@ -125,7 +125,6 @@ internal class EventRemoteDataSource @Inject constructor(
         val response = executeCall { remoteInterface ->
             remoteInterface.uploadEvents(requestId, projectId, acceptInvalidEvents, body)
         }
-
         return EventUpSyncResult(
             status = response.code(),
         )

infra/event-sync/src/test/java/com/simprints/infra/eventsync/event/remote/EventRemoteDataSourceTest.kt

@@ -1,7 +1,6 @@
 package com.simprints.infra.eventsync.event.remote
 
 import com.google.common.truth.Truth.*
-import com.simprints.infra.backendapi.ApiResult
 import com.simprints.infra.backendapi.BackendApiClient
 import com.simprints.infra.config.store.models.Project
 import com.simprints.infra.events.event.domain.EventCount
@@ -18,6 +17,7 @@ import com.simprints.infra.events.sampledata.createSessionScope
 import com.simprints.infra.eventsync.event.remote.exceptions.TooManyRequestsException
 import com.simprints.infra.eventsync.event.remote.models.session.ApiEventScope
 import com.simprints.infra.eventsync.event.usecases.MapDomainEventScopeToApiUseCase
+import com.simprints.infra.network.SimNetwork
 import com.simprints.infra.network.exceptions.BackendMaintenanceException
 import com.simprints.infra.network.exceptions.SyncCloudIntegrationException
 import com.simprints.testtools.common.syntax.assertThrows
@@ -38,7 +38,6 @@ import retrofit2.Response
 import kotlin.test.assertEquals
 
 class EventRemoteDataSourceTest {
-    @MockK
     lateinit var backendApiClient: BackendApiClient
 
     @MockK
@@ -67,13 +66,21 @@ class EventRemoteDataSourceTest {
         MockKAnnotations.init(this, relaxed = true)
         mockkStatic("kotlinx.coroutines.channels.ProduceKt")
 
-        coEvery { backendApiClient.executeCall<EventRemoteInterface, Any>(any(), any()) } coAnswers {
-            try {
-                ApiResult.Success(secondArg<suspend (EventRemoteInterface) -> Any>()(eventRemoteInterface))
-            } catch (e: Exception) {
-                ApiResult.Failure(e)
-            }
-        }
+        // Faking is simpler than mocking in this case
+        backendApiClient = BackendApiClient(
+            simNetwork = mockk {
+                coEvery {
+                    getSimApiClient(EventRemoteInterface::class, any(), any(), any())
+                } returns mockk<SimNetwork.SimApiClient<EventRemoteInterface>> {
+                    coEvery { executeCall(any<suspend (EventRemoteInterface) -> Any>()) } coAnswers {
+                        firstArg<suspend (EventRemoteInterface) -> Any>()(eventRemoteInterface)
+                    }
+                }
+            },
+            authStore = mockk { coEvery { getFirebaseToken() } returns "token" },
+            deviceId = "deviceId",
+            versionName = "versionName",
+        )
 
         every { mapDomainEventScopeToApiUseCase(any(), any(), any()) } returns apiEventScope
         eventRemoteDataSource = EventRemoteDataSource(backendApiClient)
@@ -85,7 +92,7 @@ class EventRemoteDataSourceTest {
     }
 
     @Test
-    fun count_shouldMakeANetworkRequest() = runTest {
+    fun `count should make a network request`() = runTest {
         coEvery {
             eventRemoteInterface.countEvents(any(), any(), any(), any(), any())
         } returns Response.success(
@@ -108,7 +115,7 @@ class EventRemoteDataSourceTest {
     }
 
     @Test
-    fun errorForCountRequestFails_shouldThrowAnException() = runTest {
+    fun `count request fails should throw an exception`() = runTest {
         coEvery {
             eventRemoteInterface.countEvents(
                 any(),
@@ -125,7 +132,7 @@ class EventRemoteDataSourceTest {
     }
 
     @Test
-    fun downloadEvents_shouldParseStreamAndEmitEventsIncrementally() = runTest {
+    fun `parseStreamAndEmitEvents should parse stream and emit events incrementally`() = runTest {
         val stream =
             javaClass.classLoader!!
                 .getResourceAsStream("responses/down_sync_8events.json")
@@ -156,7 +163,7 @@ class EventRemoteDataSourceTest {
     }
 
     @Test
-    fun `Get events should throw the exception received when downloading events`() = runTest {
+    fun `getEvents should throw the exception received when downloading events`() = runTest {
         val exception = BackendMaintenanceException(estimatedOutage = 100)
         coEvery {
             eventRemoteInterface.downloadEvents(
@@ -176,7 +183,7 @@ class EventRemoteDataSourceTest {
     }
 
     @Test
-    fun `Get events should map a SyncCloudIntegrationException with the status 429 to a TooManyRequestException`() = runTest {
+    fun `getEvents should map a SyncCloudIntegrationException with the status 429 to a TooManyRequestException`() = runTest {
         val exception = SyncCloudIntegrationException(
             cause = HttpException(
                 Response.error<Event>(
@@ -202,7 +209,7 @@ class EventRemoteDataSourceTest {
     }
 
     @Test
-    fun getEvents_shouldMakeTheRightRequest() = runTest {
+    fun `getEvents should make the right request`() = runTest {
         coEvery {
             eventRemoteInterface.downloadEvents(
                 any(),
@@ -233,7 +240,7 @@ class EventRemoteDataSourceTest {
     }
 
     @Test
-    fun getEvents_shouldReturnCorrectTotalHeader() = runTest {
+    fun `getEvents should return correct total header`() = runTest {
         coEvery {
             eventRemoteInterface.downloadEvents(any(), any(), any(), any(), any(), any())
         } returns Response.success(
@@ -250,7 +257,7 @@ class EventRemoteDataSourceTest {
     }
 
     @Test
-    fun getEvents_shouldReturnCorrectStatus() = runTest {
+    fun `getEvents should return correct status`() = runTest {
         coEvery {
             eventRemoteInterface.downloadEvents(any(), any(), any(), any(), any(), any())
         } returns Response.success(205, "".toResponseBody())
@@ -262,7 +269,7 @@ class EventRemoteDataSourceTest {
     }
 
     @Test
-    fun getEvents_shouldNotReturnTotalHeaderWhenLowerBound() = runTest {
+    fun `getEvents should not return total header when lower bound`() = runTest {
         coEvery {
             eventRemoteInterface.downloadEvents(any(), any(), any(), any(), any(), any())
         } returns Response.success(
@@ -277,7 +284,7 @@ class EventRemoteDataSourceTest {
     }
 
     @Test
-    fun postEvent_shouldUploadEvents() = runTest {
+    fun `post should upload events`() = runTest {
         coEvery {
             eventRemoteInterface.uploadEvents(
                 any(),
@@ -317,7 +324,7 @@ class EventRemoteDataSourceTest {
     }
 
     @Test
-    fun postEventFails_shouldThrowAnException() = runTest {
+    fun `post fails should throw an exception`() = runTest {
         coEvery {
             eventRemoteInterface.uploadEvents(
                 any(),
@@ -337,7 +344,7 @@ class EventRemoteDataSourceTest {
     }
 
     @Test
-    fun dumpInvalidEvents_shouldDumpEvents() = runTest {
+    fun `dumpInvalidEvents should dump events`() = runTest {
         coEvery { eventRemoteInterface.dumpInvalidEvents(any(), any(), any()) } returns mockk()
 
         val events = listOf("anEventJson")
@@ -347,4 +354,60 @@ class EventRemoteDataSourceTest {
             eventRemoteInterface.dumpInvalidEvents(DEFAULT_PROJECT_ID, events = events)
         }
     }
+
+    @Test
+    fun `getEvents should throw an exception received on failure response`() = runTest {
+        coEvery {
+            eventRemoteInterface.downloadEvents(
+                any(),
+                any(),
+                any(),
+                any(),
+                any(),
+                any(),
+            )
+        } returns Response.error(426, "".toResponseBody())
+
+        assertThrows<SyncCloudIntegrationException> {
+            eventRemoteDataSource.getEvents(GUID1, query, this)
+        }
+    }
+
+    @Test
+    fun `getEvents should throw an exception received on too many requests response`() = runTest {
+        coEvery {
+            eventRemoteInterface.downloadEvents(
+                any(),
+                any(),
+                any(),
+                any(),
+                any(),
+                any(),
+            )
+        } returns Response.error(429, "".toResponseBody())
+
+        assertThrows<TooManyRequestsException> {
+            eventRemoteDataSource.getEvents(GUID1, query, this)
+        }
+    }
+
+    @Test
+    fun `post failure response should throw an exception`() = runTest {
+        coEvery {
+            eventRemoteInterface.uploadEvents(
+                any(),
+                any(),
+                any(),
+                any(),
+            )
+        } returns Response.error(426, "".toResponseBody())
+
+        assertThrows<SyncCloudIntegrationException> {
+            eventRemoteDataSource.post(
+                GUID1,
+                DEFAULT_PROJECT_ID,
+                ApiUploadEventsBody(),
+            )
+        }
+    }
 }

infra/network/src/main/java/com/simprints/infra/network/apiclient/SimApiClientImpl.kt

@@ -15,6 +15,7 @@ import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.withContext
 import okhttp3.MediaType.Companion.toMediaType
 import retrofit2.HttpException
+import retrofit2.Response
 import retrofit2.Retrofit
 import retrofit2.converter.kotlinx.serialization.asConverterFactory
 import retrofit2.converter.scalars.ScalarsConverterFactory
@@ -62,7 +63,12 @@ internal class SimApiClientImpl<T : SimRemoteInterface>(
             runBlock = {
                 return@retryIO try {
                     withContext(Dispatchers.IO) {
-                        networkBlock(api)
+                        val result = networkBlock(api)
+                        if (result is Response<*> && !result.isSuccessful) {
+                            throw HttpException(result)
+                        } else {
+                            result
+                        }
                     }
                 } catch (e: Exception) {
                     throw transformExceptionIfNeeded(e)

infra/network/src/test/java/com/simprints/infra/network/FakeRetrofitInterface.kt

@@ -1,11 +1,16 @@
 package com.simprints.infra.network
 
 import kotlinx.serialization.Serializable
+import okhttp3.ResponseBody
+import retrofit2.Response
 import retrofit2.http.GET
 
 interface FakeRetrofitInterface : SimRemoteInterface {
     @GET("/path")
     suspend fun get(): Fake
+
+    @GET("/response")
+    suspend fun getResponse(): Response<ResponseBody>
 }
 
 @Serializable