-
Notifications
You must be signed in to change notification settings - Fork 1
[MS-1284] Refactor config store models to use Kotlin Serialization #1529
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
| fun addInvalidIntentEvent( | ||
| action: String, | ||
| extras: Map<String, Any>, | ||
| extras: Map<String, String>, |
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 am still verifying this part, as I believe using String alone should be sufficient for the extras.
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.
Tested and it is working as expected. below an example event
{
"id": "ebc63e69-3360-4071-a1d5-da10da55f719",
"type": "InvalidIntent",
"version": 2,
"payload": {
"startTime": {
"unixMs": 1768329314909,
"isUnixMsTrustworthy": true,
"elapsedSinceBoot": 85249056
},
"action": "com.simprints.id.REGISTER",
"extras": {
"test": "newExtra",
"projectId": "NDAdcsfcentNWOGsIkqC",
"userId": "1",
"oduleId": "1",
"callerPackageName": "com.simprints.intentlauncher"
}
},
"tokenizedFields": []
}
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 that this might only be used in the dashboard reporting and values are only looked at in case of deep investigation, so just strings would be fine.
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.
What happens if other types are passed?
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.
Ah, you are explicitly converting it. Then I guess it's fine.
| */ | ||
| data class ExperimentalProjectConfiguration( | ||
| private val customConfig: Map<String, Any>?, | ||
| private val customConfig: Map<String, JsonElement>?, |
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.
As per the design, JsonElement is sufficient here, since Vulcan only allows JSON content for custom configuration.
| projectConfiguration.copy(custom = null), | ||
| ) | ||
|
|
||
| assertThat( |
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.
This is an invalid test case, as projectConfiguration.custom is now a Map<String, JsonElement>.
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.
But the test is about handling a broken json string value in the protobuf store and has nothing to do with the type of experimental/custom config property.
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 old test case was verifying whether values in custom: Map<String, Any> could be converted to a string to be stored as protopuf. Now that custom is Map<String, JsonElement>, this verification will always succeed. Every value can be converted
19f1f5d to
cbe32d1
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
cbe32d1 to
4c37d17
Compare
28ad523 to
9e98591
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 35 out of 35 changed files in this pull request and generated 4 comments.
.../simprints/face/capture/screens/livefeedback/LiveFeedbackAutoCaptureFragmentViewModelTest.kt
Outdated
Show resolved
Hide resolved
.../simprints/face/capture/screens/livefeedback/LiveFeedbackAutoCaptureFragmentViewModelTest.kt
Outdated
Show resolved
Hide resolved
fb0ae29 to
be2bf86
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
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 35 out of 35 changed files in this pull request and generated 6 comments.
...e/src/main/java/com/simprints/infra/config/store/local/migrations/models/OldProjectConfig.kt
Show resolved
Hide resolved
.../simprints/face/capture/screens/livefeedback/LiveFeedbackAutoCaptureFragmentViewModelTest.kt
Show resolved
Hide resolved
...tor/src/test/java/com/simprints/feature/orchestrator/usecases/steps/BuildStepsUseCaseTest.kt
Show resolved
Hide resolved
infra/config-store/src/test/java/com/simprints/infra/config/store/testtools/Models.kt
Show resolved
Hide resolved
...tore/src/test/java/com/simprints/infra/config/store/local/models/ProjectConfigurationTest.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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
22d129c to
4a74bca
Compare
|
| fun addInvalidIntentEvent( | ||
| action: String, | ||
| extras: Map<String, Any>, | ||
| extras: Map<String, String>, |
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 that this might only be used in the dashboard reporting and values are only looked at in case of deep investigation, so just strings would be fine.
|
|
||
| @Keep | ||
| @Serializable | ||
| @ExcludedFromGeneratedTestCoverageReports("Data class") |
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.
Is there any chance that we could get rid of this migration all together?
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.
Once all users migrate to SID 2022.4.0
.../java/com/simprints/infra/config/store/local/migrations/ProjectConfigSharedPrefsMigration.kt
Show resolved
Hide resolved
...re/src/main/java/com/simprints/infra/config/store/models/ExperimentalProjectConfiguration.kt
Show resolved
Hide resolved
...re/src/main/java/com/simprints/infra/config/store/models/ExperimentalProjectConfiguration.kt
Show resolved
Hide resolved
| projectConfiguration.copy(custom = null), | ||
| ) | ||
|
|
||
| assertThat( |
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.
But the test is about handling a broken json string value in the protobuf store and has nothing to do with the type of experimental/custom config property.
4a74bca to
879aa81
Compare
| ) | ||
| private val JSON_FACE_CONFIGURATION_WITH_UNEXPECTED_FIELD = | ||
| JsonHelper.fromJson<Map<String, String>>( | ||
| JsonHelper.fromJson<Map<String, JsonElement>>( |
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.
Should we change the type 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.
Yes we had to change the type. Jackson is lenient by default. It silently coerces: numbers → strings and booleans → strings while kotlinx.serialization is strict by design. No implicit coercion. if you check the json string you will see that it contains numbers and booleans



JIRA ticket
Will be released in: 2026.1.0
Notable changes
AnyPrimitiveSerializeras it did not behave as expected.AnywithString.infra/config-storemodule.Testing guidance
Additional work checklist