-
Notifications
You must be signed in to change notification settings - Fork 2
Refactor the Orchestrator module to use Kotlin serialization for caching steps. #1548
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
feature/consent/src/main/java/com/simprints/feature/consent/ConsentParams.kt
Show resolved
Hide resolved
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 refactors the Orchestrator module to migrate from Jackson serialization to Kotlin serialization for caching steps during orchestration flows. The main goal is to remove the Jackson dependency entirely from the project.
Changes:
- Removed Jackson library dependencies and replaced them with Kotlin serialization
- Added
@Serializableannotations to all step params, results, and related data classes - Replaced Jackson's
@JsonTypeInfoand@JsonSubTypeswith Kotlin serialization'sSerializersModuleand polymorphic serialization - Updated
OrchestratorCacheto use Kotlin serialization instead of Jackson for serializing/deserializing steps - Removed
Serializableinterface constraint from navigation result handler methods - Changed
ActionRequest.unknownExtrastype fromMap<String, Any?>toMap<String, String?>
Reviewed changes
Copilot reviewed 76 out of 76 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| infra/ui-base/.../NavigationResultExt.kt | Removed Serializable type constraint from generic result handlers |
| infra/orchestrator-data/.../ActionRequest.kt | Migrated to Kotlin serialization, changed from sealed class inheritance to composition, converted unknownExtras to String map |
| infra/orchestrator-data/.../ActionRequestIdentifier.kt | Added @Serializable annotation |
| infra/core/.../JsonHelper.kt | Removed Jackson-based methods, kept only Kotlin serialization utility |
| infra/core/.../AgeGroup.kt | Removed Jackson annotations, added Kotlin @Serializable |
| infra/config-store/.../ModalitySdkType.kt | Added @Serializable, removed StepParams interface |
| infra/config-store/.../ProjectConfiguration.kt | Removed redundant else branch from exhaustive when expression |
| feature/orchestrator/.../OrchestratorCache.kt | Replaced Jackson serialization with Kotlin serialization for step caching |
| feature/orchestrator/.../Step.kt | Replaced Jackson mixins with Kotlin SerializersModule for polymorphic serialization |
| feature/orchestrator/.../OrcJsonHelper.kt | New helper object providing configured Json instance for orchestrator serialization |
| feature/orchestrator/.../OrchestratorViewModel.kt | Updated to use Kotlin serialization for ActionRequest serialization |
| feature/consent/.../ConsentParams.kt | Renamed field from type to consentType |
| feature/consent/.../ConsentFragment.kt | Updated to use renamed field consentType |
| feature/client-api/.../ActionRequestExtractor.kt | Updated getUnknownExtras() to return Map<String, String?> |
| feature/select-subject/.../SelectSubjectResult.kt | Added default value for savedCredential parameter |
| feature/external-credential/.../ExternalCredentialParams.kt | Added default values for nullable parameters |
| Various Params/Result classes | Added @Serializable and @SerialName annotations to all step params and results |
| Various build.gradle.kts files | Added Kotlin serialization plugin to affected modules |
| Test files | Updated tests to use Kotlin serialization APIs |
...ure/orchestrator/src/main/java/com/simprints/feature/orchestrator/cache/OrchestratorCache.kt
Outdated
Show resolved
Hide resolved
...in/java/com/simprints/feature/clientapi/mappers/request/extractors/ActionRequestExtractor.kt
Outdated
Show resolved
Hide resolved
...orchestrator/src/test/java/com/simprints/feature/orchestrator/cache/OrchestratorCacheTest.kt
Outdated
Show resolved
Hide resolved
...orchestrator/src/test/java/com/simprints/feature/orchestrator/cache/OrchestratorCacheTest.kt
Outdated
Show resolved
Hide resolved
feature/consent/src/main/java/com/simprints/feature/consent/ConsentParams.kt
Show resolved
Hide resolved
feature/login-check/src/test/java/com/simprints/feature/logincheck/usecases/ActionFactory.kt
Outdated
Show resolved
Hide resolved
...ure/orchestrator/src/main/java/com/simprints/feature/orchestrator/cache/OrchestratorCache.kt
Outdated
Show resolved
Hide resolved
feature/orchestrator/src/main/java/com/simprints/feature/orchestrator/tools/OrcJsonHelper.kt
Outdated
Show resolved
Hide resolved
feature/orchestrator/src/main/java/com/simprints/feature/orchestrator/OrchestratorViewModel.kt
Outdated
Show resolved
Hide resolved
infra/ui-base/src/main/java/com/simprints/infra/uibase/navigation/NavigationResultExt.kt
Outdated
Show resolved
Hide resolved
infra/orchestrator-data/src/main/java/com/simprints/infra/orchestration/data/ActionRequest.kt
Show resolved
Hide resolved
feature/exit-form/src/main/java/com/simprints/feature/exitform/ExitFormResult.kt
Show resolved
Hide resolved
alex-vt
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.
Jackson is still used in a few places, mainly in EventDatabaseSerialization.kt and TokenizationSerializerTest. Just in case checking if that's intentional, then the goal is to remove it entirely from the project.
...ure/orchestrator/src/main/java/com/simprints/feature/orchestrator/cache/OrchestratorCache.kt
Show resolved
Hide resolved
They are now unused files. I planned to remove them as part of the cleanup PR, Also I will completely remove the Jackson library and encapsulate all JSON handling inside |
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 78 out of 78 changed files in this pull request and generated 8 comments.
feature/orchestrator/src/main/java/com/simprints/feature/orchestrator/steps/Step.kt
Show resolved
Hide resolved
infra/config-store/src/main/java/com/simprints/infra/config/store/models/ModalitySdkType.kt
Show resolved
Hide resolved
infra/orchestrator-data/src/main/java/com/simprints/infra/orchestration/data/ActionRequest.kt
Show resolved
Hide resolved
...ial/src/main/java/com/simprints/feature/externalcredential/model/ExternalCredentialParams.kt
Show resolved
Hide resolved
...chestrator/src/main/java/com/simprints/feature/orchestrator/tools/OrchestrationJsonHelper.kt
Outdated
Show resolved
Hide resolved
...ure/orchestrator/src/main/java/com/simprints/feature/orchestrator/cache/OrchestratorCache.kt
Outdated
Show resolved
Hide resolved
...e/orchestrator/src/test/java/com/simprints/feature/orchestrator/OrchestratorViewModelTest.kt
Outdated
Show resolved
Hide resolved
...e/orchestrator/src/test/java/com/simprints/feature/orchestrator/OrchestratorViewModelTest.kt
Show resolved
Hide resolved
| params = MatchParams( | ||
| flowType = FlowType.IDENTIFY, | ||
| queryForCandidates = EnrolmentRecordQuery(), | ||
| biometricDataSource = BiometricDataSource.Simprints, // Test Simprints variant |
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.
Can we also test with CommCare as it also contains callingPackageName
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.
I think BiometricDataSource.CommCare is covered in line 287
...orchestrator/src/test/java/com/simprints/feature/orchestrator/cache/OrchestratorCacheTest.kt
Show resolved
Hide resolved
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.
A few comments (including a few good catches from Copilot) but otherwise looks good.
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 78 out of 78 changed files in this pull request and generated 5 comments.
feature/consent/src/main/java/com/simprints/feature/consent/ConsentParams.kt
Show resolved
Hide resolved
...e/orchestrator/src/test/java/com/simprints/feature/orchestrator/OrchestratorViewModelTest.kt
Show resolved
Hide resolved
feature/select-subject/src/main/java/com/simprints/feature/selectsubject/SelectSubjectResult.kt
Show resolved
Hide resolved
...ure/orchestrator/src/main/java/com/simprints/feature/orchestrator/cache/OrchestratorCache.kt
Show resolved
Hide resolved
...nfig-store/src/main/java/com/simprints/infra/config/store/models/FingerprintConfiguration.kt
Outdated
Show resolved
Hide resolved
|



JIRA ticket
Will be released in: 2026.1.0
Notable changes
Testing guidance
Additional work checklist