-
Notifications
You must be signed in to change notification settings - Fork 2
Feature/Jackson clean up #1550
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
base: main
Are you sure you want to change the base?
Feature/Jackson clean up #1550
Conversation
4236c79 to
5a3b442
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
This pull request migrates the codebase from Jackson to Kotlin Serialization by introducing a new :infra:serialization module with a global SimJson instance. The PR removes Jackson dependencies, deletes Jackson-specific code, and updates all usages throughout the codebase to use the new SimJson object.
Changes:
- Creates new
:infra:serializationmodule withSimJsonconfiguration - Removes Jackson library dependencies from gradle files and proguard rules
- Migrates all Jackson serialization calls to use
SimJsonwith Kotlin Serialization - Deletes Jackson-specific serialization classes and updates related tests
Reviewed changes
Copilot reviewed 81 out of 81 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| infra/serialization/* | New module with SimJson configuration for Kotlin Serialization |
| settings.gradle.kts | Adds new serialization module to project structure |
| infra/*/build.gradle.kts | Removes Jackson dependencies from multiple modules |
| infra/network/src/main/java/com/simprints/infra/network/json/JsonHelper.kt | Deleted internal JsonHelper that used Jackson |
| infra/core/src/main/java/com/simprints/core/domain/tokenization/serialization/EventDatabaseSerialization.kt | Deleted Jackson serializers for TokenizableString |
| infra/events/src/main/java/com/simprints/infra/events/event/domain/models/EnrolmentRecordEvent.kt | Removed Jackson annotations |
| infra//src/main/java/**/.kt | Updated imports and usages from JsonHelper to SimJson across ~40+ files |
| infra//src/test/java/**/.kt | Updated test files to use SimJson and removed JsonHelper mocks |
| infra/core/src/main/java/com/simprints/core/CoreModule.kt | Removed JsonHelper from dependency injection |
| feature/orchestrator/src/main/java/com/simprints/feature/orchestrator/tools/OrchestrationJsonHelper.kt | Updated to extend SimJson configuration |
| gradle/libs.versions.toml | Removed Jackson version and library references |
| id/proguard-rules.pro | Removed Jackson-specific proguard rules |
Comments suppressed due to low confidence (1)
infra/core/src/main/java/com/simprints/core/tools/json/JsonHelper.kt:22
- The JsonHelper object is still present in the codebase with duplicate configuration to SimJson. This creates inconsistency and confusion. Consider either removing this entirely or having it delegate to SimJson to maintain a single source of truth for JSON configuration.
val json: Json by lazy {
Json {
ignoreUnknownKeys = true
explicitNulls = false
encodeDefaults = true
coerceInputValues = true
}
}
}
| * Global Json instance. | ||
| * Defined as a top-level variable to allow "One Dot" usage: SimJson.encodeToString(...) | ||
| * | ||
| * TODO: Should me moved to a Hilt DI module and inject it into classes |
Copilot
AI
Jan 21, 2026
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.
There is a typo in the TODO comment: "Should me moved" should be "Should be moved".
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.
Any reason not do it in this PR?
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 found it very complex
infra/core/src/main/java/com/simprints/core/tools/json/JsonHelper.kt
Outdated
Show resolved
Hide resolved
| @Test | ||
| fun `serialize Raw produces className Raw and value`() { | ||
| val raw = TokenizableString.Raw("person") | ||
|
|
||
| val result = json.parseToJsonElement( | ||
| json.encodeToString(raw), | ||
| val result = SimJson.parseToJsonElement( | ||
| SimJson.encodeToString(raw), | ||
| ) | ||
|
|
||
| val expected = JsonObject( |
Copilot
AI
Jan 21, 2026
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.
The removed test validates Jackson-specific class name serialization behavior which is no longer relevant. However, this test removal appears intentional as part of the Jackson cleanup. Ensure equivalent behavior is tested with the new Kotlin Serialization implementation if this serialization format is still being used.
...chestrator/src/main/java/com/simprints/feature/orchestrator/tools/OrchestrationJsonHelper.kt
Outdated
Show resolved
Hide resolved
5a3b442 to
d17df3e
Compare
d17df3e to
176750a
Compare
|
|
|
||
| @Test | ||
| fun `returns event if biometrics coSync enabled`() = runTest { | ||
| mockkObject(SimJson) |
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 do 2-way approach with the helper being injected where possible and a fallback with the direct object access?
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.
Would this be for the sole purpose of injecting mocks in those classes instead of mocking objects?
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.
More for the purpose of maintaining the inversion of control whereever possible.
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.
Then I don't really see the benefit if it's hardcoded in half of the classes - it's highly unlikely we decide to use one lib for the injection and another for the hardcoded cases.
| * Global Json instance. | ||
| * Defined as a top-level variable to allow "One Dot" usage: SimJson.encodeToString(...) | ||
| * | ||
| * TODO: Should me moved to a Hilt DI module and inject it into classes |
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.
Any reason not do it in this PR?
| * TODO: Should me moved to a Hilt DI module and inject it into classes | ||
| * | ||
| */ | ||
| val SimJson: Json by lazy { |
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.
Does this really warrant a full module? Do you plan to move any additional stuff in here?
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 wanted this in a separate module because it is needed by every other modules.
My first thought was to add this utility class to infra/core. However, infra/network also requires it, and it can't depend on core.
The second option was infra/logging, but that is not a good fit conceptually.
For that reason, I chose to place the SimJson in a new module: infra/serialization.
|
|
||
| @Provides | ||
| @Singleton | ||
| fun provideJsonHelper(): JsonHelper = JsonHelper |
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.
We cannot do the same for the new "helper"?
| class TokenizationSerializerTest { | ||
| // Todo remove this test once removing all the jackson code | ||
| @Test | ||
| fun `class name tokenization serialization and deserialization should produce same result`() { |
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.
While the implementation details were Jackson specific, the test purpose seems still valid?



JIRA ticket
Will be released in: 2026.1.0
Notable changes
There were several ways to centralize the Kotlin JSON serialization configuration. I chose to make it a global lazy variable to minimize refactoring.
The ideal approach would be to inject it using Hilt. However, it is mainly used in data-class converters and extension functions, and introducing DI would require threading the
Jsoninstance through all converters.For example:
would need to become:
Given the scope and risk of such changes, a global instance is the more pragmatic choice here.
Testing guidance
Additional work checklist