MS-1382 Correct Response handling in backend api client

[luhmirin-s]

JIRA ticket
Will be released in: 2025.2.1

Root cause analysis (for bugfixes only)

First known affected version: 2024.1.5

• When retrofit interface returns Response<T> type, then it will never throw during call execution.

Notable changes

• Added an explicit check for Response<ResponseBody> type in the backend API client and unwrapping to follow the same rules as the regular responses - if not successful it is wrapped in ApiResponse.Failure() with the SyncCloudIntegrationException as the cause. This should remove the differences in handling of successful and unsuccessful calls in the domain layer.
• Just to be sure, added additional layer of unwrapping on the network layer.

Testing guidance

• Sync should work as expected.
• There isn't really a way to reliably reproduce the issue by hand, so we will have to rely on unit tests.

Additional work checklist

• Effect on other features and security has been considered
• Design document marked as "In development" (if applicable)
• External (Gitbook) and internal (Confluence) Documentation is up to date (or ticket created)
• Test cases in Testiny are up to date (or ticket created)
• Other teams notified about the changes (if applicable)

[Copilot]

Pull request overview

Fixes inconsistent error handling when Retrofit endpoints return Response<T> by ensuring non-2xx responses are treated like regular failed calls (throwing and being mapped consistently), improving reliability of sync/network error behavior across infra modules.

Changes:

• Updated SimApiClientImpl to throw HttpException when executeCall returns an unsuccessful retrofit2.Response<*>.
• Added/expanded unit tests covering Response<T> success/failure paths, retry behavior, and maintenance vs non-maintenance 503 handling.
• Updated event-sync remote data source tests to use a lightweight BackendApiClient + SimNetwork fake instead of wrapping results manually.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
infra/network/src/main/java/com/simprints/infra/network/apiclient/SimApiClientImpl.kt	Ensures unsuccessful Response<*> results throw, enabling consistent exception mapping + retry logic.
infra/network/src/test/java/com/simprints/infra/network/apiclient/SimApiClientImplTest.kt	Adds coverage for Response<ResponseBody> and retry/maintenance behavior.
infra/network/src/test/java/com/simprints/infra/network/FakeRetrofitInterface.kt	Adds a Retrofit endpoint returning Response<ResponseBody> for testing.
infra/backend-api/src/main/java/com/simprints/infra/backendapi/BackendApiClient.kt	Adds an extra response-unwrapping layer and changes constructor visibility.
infra/backend-api/src/test/java/com/simprints/infra/backendapi/BackendApiClientTest.kt	Adds tests for Response<ResponseBody> success/failure wrapping (currently with a type-mismatch issue).
infra/event-sync/src/test/java/com/simprints/infra/eventsync/event/remote/EventRemoteDataSourceTest.kt	Refactors tests to use a simple BackendApiClient fake path and adds failure-response assertions.
infra/event-sync/src/main/java/com/simprints/infra/eventsync/event/remote/EventRemoteDataSource.kt	Minor formatting-only change.

Comments suppressed due to low confidence (1)

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

• Changing BackendApiClient from @Inject internal constructor to a public @Inject constructor expands the module’s public API surface. Most infra-layer injected classes keep their constructors internal to discourage direct instantiation outside DI (e.g. infra/event-sync/.../DownSyncSubjectUseCase.kt:19, infra/images/.../ImageRepositoryImpl.kt:16). Consider keeping the constructor internal and adjusting tests to mock/fake at the SimNetwork/SimApiClient level (or provide a dedicated test helper) rather than making construction public.

@Singleton
class BackendApiClient @Inject constructor(
    private val simNetwork: SimNetwork,
    private val authStore: AuthStore,
    @param:DeviceID private val deviceId: String,
    @param:PackageVersionName private val versionName: String,
) {

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[BurningAXE]

Am I correct in my understanding that we shouldn't introduce another type similar to Response or we'll step on the same rake again?

[luhmirin-s]

@BurningAXE not exactly. The Response<ResponseBody> is the only option if you need to do anything more than just parse the request body to a specific type, so it has been added for very specific reasons and there is a chance that we will have to use it in the future. This is why I added such an aggressive handling for it.