-
Notifications
You must be signed in to change notification settings - Fork 2
MS-1275 Minor networking improvements #1559
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request introduces significant improvements to the networking layer by centralizing backend API interactions and improving error handling.
Changes:
- Introduces a new
backend-apimodule withBackendApiClientas the single point of interaction for backend API calls - Wraps API call results in an
ApiResultsealed class withSuccessandFailurecases to make error handling more explicit - Migrates all existing API client code to use the new
BackendApiClientinstead of directly usingAuthStoreor factory classes
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| settings.gradle.kts | Adds the new :infra:backend-api module to the project |
| infra/backend-api/src/main/java/com/simprints/infra/backendapi/BackendApiClient.kt | New centralized client for backend API interactions |
| infra/backend-api/src/test/java/com/simprints/infra/backendapi/BackendApiClientTest.kt | Test coverage for the new BackendApiClient |
| infra/backend-api/build.gradle.kts | Build configuration for the new backend-api module |
| infra/network/src/main/java/com/simprints/infra/network/ApiResult.kt | New sealed class for wrapping API results |
| infra/network/src/test/java/com/simprints/infra/network/ApiResultTest.kt | Test coverage for ApiResult |
| infra/network/src/main/java/com/simprints/infra/network/SimNetwork.kt | Updated interface to return ApiResult instead of throwing |
| infra/network/src/main/java/com/simprints/infra/network/apiclient/SimApiClientImpl.kt | Updated implementation to return ApiResult |
| infra/network/src/test/java/com/simprints/infra/network/apiclient/SimApiClientImplTest.kt | Updated tests for new ApiResult return type |
| infra/auth-store/src/main/java/com/simprints/infra/authstore/AuthStore.kt | Removed buildClient method, added getFirebaseToken method |
| infra/auth-store/src/main/java/com/simprints/infra/authstore/AuthStoreImpl.kt | Updated implementation with getFirebaseToken |
| infra/auth-store/src/main/java/com/simprints/infra/authstore/network/SimApiClientFactory.kt | Removed - functionality moved to BackendApiClient |
| infra/auth-store/src/test/java/com/simprints/infra/authstore/AuthStoreImplTest.kt | Updated tests for removed buildClient method |
| infra/license/src/main/java/com/simprints/infra/license/remote/LicenseRemoteDataSourceImpl.kt | Migrated to use BackendApiClient with ApiResult error handling |
| infra/license/src/test/java/com/simprints/infra/license/remote/LicenseRemoteDataSourceImplTest.kt | Updated tests for new API client |
| infra/license/build.gradle.kts | Added backendApi plugin, removed direct dependencies |
| infra/event-sync/src/main/java/com/simprints/infra/eventsync/event/remote/EventRemoteDataSource.kt | Migrated to use BackendApiClient |
| infra/event-sync/src/test/java/com/simprints/infra/eventsync/event/remote/EventRemoteDataSourceTest.kt | Updated tests for new API client |
| infra/event-sync/build.gradle.kts | Added backendApi plugin |
| infra/enrolment-records/repository/src/main/java/com/simprints/infra/enrolment/records/repository/remote/EnrolmentRecordRemoteDataSourceImpl.kt | Migrated to use BackendApiClient, refactored constructor |
| infra/enrolment-records/repository/src/test/java/com/simprints/infra/enrolment/records/repository/remote/EnrolmentRecordRemoteDataSourceImplTest.kt | Updated tests for new API client |
| infra/enrolment-records/repository/build.gradle.kts | Added backendApi plugin |
| infra/config-store/src/main/java/com/simprints/infra/config/store/remote/ConfigRemoteDataSourceImpl.kt | Migrated to use BackendApiClient |
| infra/config-store/src/test/java/com/simprints/infra/config/store/remote/ConfigRemoteDataSourceImplTest.kt | Updated tests for new API client |
| infra/config-store/build.gradle.kts | Added backendApi plugin |
| infra/images/src/main/java/com/simprints/infra/images/remote/signedurl/usecase/*.kt | Migrated image upload use cases to use BackendApiClient |
| infra/images/src/test/java/com/simprints/infra/images/remote/signedurl/usecase/*.kt | Updated tests for new API client |
| infra/images/build.gradle.kts | Added backendApi plugin |
| infra/auth-logic/src/main/java/com/simprints/infra/authlogic/authenticator/remote/AuthenticationRemoteDataSource.kt | Migrated to use BackendApiClient (affected by critical bug) |
| infra/auth-logic/src/main/java/com/simprints/infra/authlogic/authenticator/remote/UnauthenticatedClientFactory.kt | Removed - functionality moved to BackendApiClient |
| infra/auth-logic/src/test/java/com/simprints/infra/authlogic/authenticator/remote/AuthenticationRemoteDataSourceTest.kt | Updated tests for new API client |
| infra/auth-logic/build.gradle.kts | Added backendApi plugin |
| fingerprint/infra/scanner/src/main/java/com/simprints/fingerprint/infra/scanner/data/remote/network/*.kt | Migrated to use BackendApiClient, removed factory classes |
| fingerprint/infra/scanner/src/test/java/com/simprints/fingerprint/infra/scanner/data/remote/network/*.kt | Updated tests for new API client |
| fingerprint/infra/scanner/build.gradle.kts | Added backendApi plugin |
| build-logic/convention/src/main/kotlin/LibraryBackendConventionPlugin.kt | New Gradle convention plugin for backend API dependencies |
| build-logic/convention/build.gradle.kts | Registered the new backendApi plugin |
| .github/workflows/pr-checks.yml | Added backend-api module to CI workflow |
...a/com/simprints/infra/images/remote/signedurl/usecase/UploadSampleWithTrackingUseCaseTest.kt
Show resolved
Hide resolved
...com/simprints/fingerprint/infra/scanner/data/remote/network/FingerprintFileDownloaderTest.kt
Outdated
Show resolved
Hide resolved
infra/backend-api/src/main/java/com/simprints/infra/backendapi/BackendApiClient.kt
Outdated
Show resolved
Hide resolved
infra/backend-api/src/main/java/com/simprints/infra/backendapi/BackendApiClient.kt
Outdated
Show resolved
Hide resolved
...tore/src/test/java/com/simprints/infra/config/store/remote/ConfigRemoteDataSourceImplTest.kt
Outdated
Show resolved
Hide resolved
...ava/com/simprints/infra/authlogic/authenticator/remote/AuthenticationRemoteDataSourceTest.kt
Outdated
Show resolved
Hide resolved
4c37de3 to
5301f2c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 40 out of 40 changed files in this pull request and generated 1 comment.
infra/backend-api/src/main/java/com/simprints/infra/backendapi/BackendApiClient.kt
Outdated
Show resolved
Hide resolved
5301f2c to
d646bac
Compare
BurningAXE
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems contained enough and simplifies things a bit.
infra/backend-api/src/main/java/com/simprints/infra/backendapi/BackendApiClient.kt
Show resolved
Hide resolved
d646bac to
89eff7e
Compare
…to a backend client # Conflicts: # infra/auth-store/src/main/java/com/simprints/infra/authstore/AuthStore.kt # infra/auth-store/src/main/java/com/simprints/infra/authstore/AuthStoreImpl.kt
… the exception throwing explicit
89eff7e to
d1c9a23
Compare
|



JIRA ticket
Will be released in: 2026.2.0
Notable changes
After spending way too much time trying out different approaches, I landed on 2 small improvements (in relative terms) that are worth doing. The rest of experimentation provided almost zero ROI.
simprints.library.backendApiplugin to the module (instead of separateauth-store,networkand retrofit dependencies)BackendApiClientinto your VM/use case/manager (instead of AuthStore or some sort of client factory)apiClient.executeCall(MyNewRemoteInterface::class) { api -> api.getStuff() }to do the thing (no need explicitly create a client on caller side).But doesn't it remove an option to "cache" the client if I need multiple calls on same interface, you might ask. In practice we have not had an case like that in the current code base, so likely YAGNI. Additionally, creation of new Retrofit API instance is relatively low cost (compared to the network call itself) since we are re-using the actual HTTP client instance and the clear abstraction of the
BackendApiClientcould allow us to implement some more complicated caching in the future without significant changes on the caller side.ApiResultsealed class to avoid throwing the exception deep into the networking stack. This result class has a couple of convenience getters to start with:result.getOrThrow()- replicates the old behaviour, but it moves the decision to throw closer to the caller. This makes it more explicit that the exception must be handled in one way or another.result.getOrMapFailure {}- provides a convenient way to either simply return the fallback value or map the exact networking exception to a specific value (e.g. to an error message based on the exception type)Testing guidance
Additional work checklist