Migration to kotlinx json serialization #5279
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
common/src/main/kotlin/io/homeassistant/companion/android/common/util/JsonUtil.kt
Fixed
Show resolved
Hide resolved
Test Results0 tests 0 ✅ 0s ⏱️ Results for commit 5c515a1. ♻️ This comment has been updated with latest results. |
4 tasks
TimoPtr
force-pushed
the
feature/migration_kotlinx_serialization
branch
2 times, most recently
from
5c515a1 to
2d2e374
Compare
TimoPtr
changed the title
TimoPtr
force-pushed
the
feature/migration_kotlinx_serialization
branch
from
cad6361 to
44dfc43
Compare
Migrate from java.util.Calendar to java.time.LocalDate.Time Replace generic type for attributes by map string any Remove context
TimoPtr
force-pushed
the
feature/migration_kotlinx_serialization
branch
from
9a0d755 to
6a842d6
Compare
TimoPtr
commented
May 15, 2025
| org.jetbrains.kotlinx:kotlinx-serialization-core-jvm:1.4.1=ktlintReporter | ||
| org.jetbrains.kotlinx:kotlinx-serialization-core-jvm:1.6.3=_agp_internal_javaPreCompileDebug_kspClasspath,_agp_internal_javaPreCompileRelease_kspClasspath,hiltAnnotationProcessorDebugAndroidTest,hiltAnnotationProcessorDebugUnitTest,hiltAnnotationProcessorReleaseUnitTest,kotlinCompilerPluginClasspathDebug,kotlinCompilerPluginClasspathDebugAndroidTest,kotlinCompilerPluginClasspathDebugUnitTest,kotlinCompilerPluginClasspathRelease,kotlinCompilerPluginClasspathReleaseUnitTest,kspDebugAndroidTestKotlinProcessorClasspath,kspDebugKotlinProcessorClasspath,kspDebugUnitTestKotlinProcessorClasspath,kspPluginClasspath,kspPluginClasspathNonEmbeddable,kspReleaseKotlinProcessorClasspath,kspReleaseUnitTestKotlinProcessorClasspath | ||
| org.jetbrains.kotlinx:kotlinx-serialization-core-jvm:1.7.3=debugAndroidTestCompileClasspath,debugAndroidTestRuntimeClasspath,debugCompileClasspath,debugRuntimeClasspath,debugUnitTestCompileClasspath,debugUnitTestRuntimeClasspath,releaseCompileClasspath,releaseRuntimeClasspath,releaseUnitTestCompileClasspath,releaseUnitTestRuntimeClasspath | ||
| org.jetbrains.kotlinx:kotlinx-serialization-core-jvm:1.8.1=debugAndroidTestCompileClasspath,debugAndroidTestRuntimeClasspath,debugCompileClasspath,debugRuntimeClasspath,debugUnitTestCompileClasspath,debugUnitTestRuntimeClasspath,releaseCompileClasspath,releaseRuntimeClasspath,releaseUnitTestCompileClasspath,releaseUnitTestRuntimeClasspath |
There was a problem hiding this comment.
Side note, while working on this PR and adding the kotlinx-serialization dependency. I was not able to run lint tasks, I had an error on a conflicting version of the new library 1.8.1 and 1.7.3 both were present in the lockfile. I spent a lot of time finding a way to force the new version but it was not working.
In the end I found out that I had to remove the lockfile and regenerate it. I suppose that the lock task in some specify cases keep some old artifacts in the file, probably when updating Gradle or a plugin.
1 task
jpelgrom
reviewed
May 20, 2025
common/src/main/kotlin/io/homeassistant/companion/android/common/util/JsonUtil.kt
Outdated
Show resolved
Hide resolved
common/src/main/kotlin/io/homeassistant/companion/android/common/util/JsonUtil.kt
Outdated
Show resolved
Hide resolved
common/src/main/kotlin/io/homeassistant/companion/android/common/util/JsonUtil.kt
Outdated
Show resolved
Hide resolved
common/src/main/kotlin/io/homeassistant/companion/android/common/util/JsonUtil.kt
Outdated
Show resolved
Hide resolved
...src/main/kotlin/io/homeassistant/companion/android/common/data/integration/ZoneAttributes.kt
Outdated
Show resolved
Hide resolved
.../homeassistant/companion/android/common/data/integration/impl/entities/IntegrationRequest.kt
Outdated
Show resolved
Hide resolved
...meassistant/companion/android/common/data/integration/impl/entities/RegisterDeviceRequest.kt
Outdated
Show resolved
Hide resolved
...in/io/homeassistant/companion/android/common/data/integration/impl/entities/SensorRequest.kt
Outdated
Show resolved
Hide resolved
...meassistant/companion/android/common/data/integration/impl/entities/UpdateLocationRequest.kt
Outdated
Show resolved
Hide resolved
...n/src/main/kotlin/io/homeassistant/companion/android/database/sensor/SensorWithAttributes.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: Joris Pelgröm <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
Migrate JSON serialization from Jackson to Kotlinx and adjust models and control providers accordingly.
- Replace Jackson
ObjectMappercalls withkotlinJsonMapperfor JSON de/serialization - Remove Jackson dependencies and annotate models for Kotlinx serialization
- Update control-related
Entity<Map<String, Any>>usages to rawEntity, and switch fromCalendartoLocalDateTime
Reviewed Changes
Copilot reviewed 161 out of 161 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/main/kotlin/.../ManageControlsViewModel.kt | Changed entitiesList type from List<Entity<*>> to raw List<Entity> |
| app/src/main/kotlin/.../notifications/MessagingManager.kt | Replaced Jackson parsing with kotlinJsonMapper.decodeFromString |
| app/src/main/kotlin/.../controls/HaControlsProviderService.kt | Updated generics on Entity and swapped to LocalDateTime |
| app/src/full/.../settings/wear/SettingsWearViewModel.kt | Swapped Jackson to kotlinJsonMapper and updated exception handling |
| app/src/full/.../sensors/LocationSensorManager.kt | Adjusted Entity<ZoneAttributes> to raw Entity and updated attribute casts |
Comments suppressed due to low confidence (3)
app/src/main/kotlin/io/homeassistant/companion/android/controls/HaControl.kt:30
- The interface now takes a raw
Entityparameter; consider usingEntity<Any>orEntity<*>to retain generic type information and suppress raw-type warnings.
fun createControl(..., entity: Entity, ...)
app/src/main/kotlin/io/homeassistant/companion/android/settings/controls/ManageControlsViewModel.kt:47
- Using the raw type
Entityloses compile-time type safety. Consider specifying a generic parameter likeEntity<Any>orEntity<*>to preserve intent and avoid unchecked usages.
val entitiesList = mutableStateMapOf<Int, List<Entity>>()
app/src/main/kotlin/io/homeassistant/companion/android/qs/TileExtensions.kt:150
- Switched to raw
Entityhere; specifying a generic such asEntity<Any>orEntity<*>will make the code clearer and safer against unchecked casts.
val state: Entity? =
common/src/main/kotlin/io/homeassistant/companion/android/common/util/JsonUtil.kt
Fixed
Show fixed
Hide fixed
|
Running this on my device, it does not like getting zones. |
Good catch thanks. It's because it use |
|
Another exception while editing settings for a Wear device (easiest to trigger if you add a template tile on the watch and try to edit it from the phone, the phone app crashes the moment you start typing text)
|
|
You mentioned adding a lint check to catch issues like above on Discord. Do you plan to add that in this PR, or later? |
I'm polishing it with some tests, and I'm opening a PR that we can merge on main without this PR and I will merge main into this PR. |
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
To continue updating Retrofit while maintaining our minSDK at
21, we need to migrate away from Jackson for JSON serialization. As proposed in #3518, we can migrate to Kotlinx serialization, which is now mature enough and fully integrated with Retrofit since version 2.10.0.Kotlinx serialization does not use reflection like Jackson, which requires some adjustments. First, all models must be annotated with
@Serializableso that the Gradle plugin can generate the necessary serializers. By default, it does not support the Any type or anything containing it. To address this limitation, I implemented a custom serializer that supports Any, but it has some restrictions—particularly when serializing to JSON, where Any must be a primitive and cannot be an object.To avoid using
SimpleDateFormatwith a format that does not fully support ISO8601, I migrated fromjava.util.Calendartojava.time.LocalDateTimein the models being serialized. This required ensuring that the:commonmodule includes the desugar dependency and has it enabled.For the
Entitymodel, I removed the context attribute since it was not used anywhere. If needed, it can be reintroduced later. The generic type used in theEntitymodel made the migration complex, especially for cases where<Any>was not used. To simplify this, I replaced it withAnyand adjusted the single case where it was not used. Alternatively, we could create a custom serializer and use a sealed class to handle specific entities, similar to what I did inSocketResponse, but this is outside the scope of this PR.To facilitate the review process, I structured the changes into distinct commits with no overlap, allowing them to be reviewed individually for clarity. I had to make one PR since everything is coupled here, so you can't build the individual commits it won't compile.
This is a significant and impactful change to the application, which may introduce regressions, particularly in how default parameters are handled. Since reflection is no longer used, the serializer enforces that all fields without default values must have a value, which could lead to exceptions in cases where none were thrown before. We need to be especially vigilant during the beta phase to identify and address any crashes or issues.
Checklist
Any other notes
Close #3518 and we can close #5114 and #5102 after the merge of this PR.
It remains one JSON serialization in the app that doesn't use Kotlinx serialization, but it is not using Jackson but
org.jsonso we can live with it for a while. I created a small issue for that #5279.I have made some unit tests that a bit verbose on purpose to showcase how the adjustment is working.