Add support for using kotlinx-serialization rather than Jackson#2791
Add support for using kotlinx-serialization rather than Jackson#2791Luna712 wants to merge 5 commits into
Conversation
If accepted, I will slowly migrate the app and library to kotlinx-serialization, standardizing all the different JSON parsers we use as well. kotlinx-serialization is fully KMP and also more performant, because unlike Jackson, it doesn't use runtime reflection, it uses validation at compiler time.
fire-light43
left a comment
There was a problem hiding this comment.
A good start, but I want more insurance that this works when we migrate. Therefore I have created a comprehensive test to validate the move, please see if something like this can be included in androidTest in this pull request.
package com.lagradost.cloudstream3
import androidx.test.ext.junit.runners.AndroidJUnit4
import androidx.test.platform.app.InstrumentationRegistry
import com.lagradost.cloudstream3.utils.AppUtils.toJson
import dalvik.system.DexFile
import kotlinx.serialization.ExperimentalSerializationApi
import kotlinx.serialization.InternalSerializationApi
import kotlinx.serialization.KSerializer
import kotlinx.serialization.Serializable
import kotlinx.serialization.json.Json
import kotlinx.serialization.serializer
import kotlinx.serialization.serializerOrNull
import org.instancio.Instancio
import org.junit.Test
import org.junit.runner.RunWith
import kotlin.reflect.KClass
import kotlin.reflect.jvm.jvmName
import kotlin.test.assertEquals
import kotlin.test.assertNotNull
@RunWith(AndroidJUnit4::class)
class SerializationClassTester {
// Same as app, or using app reference
val jacksonMapper = mapper
val kotlinxMapper = Json {
ignoreUnknownKeys = true
}
@Test
fun isIdenticalSerialization() {
val serializableClasses = findSerializableClasses("com.lagradost")
println("Number of serializable classes: ${serializableClasses.size}")
serializableClasses.forEach { kClass ->
val instance = Instancio.create(kClass.java)
val jacksonJson = jacksonMapper.writeValueAsString(instance)
val kotlinxJson = serializeWithKotlinx(kClass, instance)
assertEquals(
jacksonJson,
kotlinxJson,
"""
Serialization mismatch for:
${kClass.qualifiedName}
Jackson:
$jacksonJson
Kotlinx:
$kotlinxJson
""".trimIndent()
)
println("Identical serialization for: ${kClass.jvmName}")
}
}
@OptIn(InternalSerializationApi::class, ExperimentalSerializationApi::class)
@Test
fun isIdenticalDeserialization() {
val serializableClasses = findSerializableClasses("com.lagradost")
println("Number of serializable classes: ${serializableClasses.size}")
serializableClasses.forEach { kClass ->
val instance = Instancio.create(kClass.java)
// Convert to JSON to get example JSON object
// We prefer jackson here because the app may have many jackson JSON strings in local storage
val originalJson = jacksonMapper.writeValueAsString(instance)
// Create an object from the JSON using kotlinx
val serializer =
kClass.serializerOrNull() ?: kotlinxMapper.serializersModule.getContextual(kClass)
assertNotNull(serializer, "The class: ${kClass.jvmName} must be serializable!")
val kotlinxDecoded = kotlinxMapper.decodeFromString(serializer, originalJson)
// Create an object from the JSON using jackson
val mapperDecoded = jacksonMapper.readValue(originalJson, kClass.java)
// Deep inspect both object using the mapper toJson function.
// This deep equality check can be performed using other methods, but this just works.
val jacksonJson = mapperDecoded.toJson()
val kotlinxJson = kotlinxDecoded.toJson()
assertEquals(
jacksonJson,
kotlinxJson,
"""
Serialization mismatch for:
${kClass.qualifiedName}
Jackson:
$jacksonJson
Kotlinx:
$kotlinxJson
""".trimIndent()
)
println("Identical deserialization for: ${kClass.jvmName}")
}
}
private fun findSerializableClasses(packageName: String): List<KClass<*>> {
val context = InstrumentationRegistry
.getInstrumentation()
.targetContext
val dexFile = DexFile(context.packageCodePath)
return dexFile.entries()
.toList()
.filter { it.startsWith(packageName) }
.mapNotNull {
runCatching { Class.forName(it).kotlin }
.onFailure { e ->
// Log.e("DEX", "Failed loading $it", e)
}
.getOrNull()
}.filter { kClass ->
kClass.java.annotations.any {
it is Serializable
}
}
}
@OptIn(InternalSerializationApi::class)
@Suppress("UNCHECKED_CAST")
private fun serializeWithKotlinx(
kClass: KClass<*>,
value: Any
): String {
val serializer = kClass.serializer() as KSerializer<Any>
return kotlinxMapper.encodeToString(serializer, value)
}
}I use
androidTestImplementation(kotlin("test"))
androidTestImplementation("org.instancio:instancio-core:5.5.1")
androidTestImplementation(libs.kotlinx.serialization.json)|
The two suggestions are done. I also preemptively fixed issues that could have been caused during migration including bugs that broke backup and other stuff. I also added support to the rest of the app by using helper methods rather than mapper directly etc... I will add the test later also. For the record, for testing this I literally converted every single aspect of the app to kotlinx serialization and nothing broke now, but I will do smaller PRs later to convert sections of the app incrementally to make things a little easier to test later on. |
|
@fire-light43 I added the test now with some minor changes to what you had above to fix some deprecation warnings and a couple other things. I did #2792 so these tests run when making PRs also, since I plan to do those to start migrating a lot of things I thought that would make things easier also. |
fire-light43
left a comment
There was a problem hiding this comment.
Looks good, just some minor things. It looks ready to merge very soon.
Good choice going with ClassGraph 👍
|
I will do changes tomorrow more than likely rather than tonight. Thanks for the review again! |
|
No problem, this is already very fast progress |
|
@fire-light43 done with all that now, I also added two more serializers to do things we currently do with Jackson, and also added another test to ensure all the serializers are properly working. P.S. I may be writing more tests in the future for things now. You kinda got me in to liking to write those lol. |
| val kotlinxMapper = json | ||
|
|
||
| @Test | ||
| fun isIdenticalSerialization() { |
There was a problem hiding this comment.
hmm... I just thought of something... unless I am missing something.... this may not actually match a lot of times because things will be different when we swap @JsonProperty with @SerialName and a couple of other changes.
There was a problem hiding this comment.
I guess we could keep @JsonProperty for now until migration is complete and then remove them all so we would have both @JsonProperty with @SerialName in the meantime, which this may be better anyway to ensure the fallback also properly works if something goes wrong with kotlinx serialization during migration as well...
If accepted, I will slowly migrate the app and library to kotlinx-serialization, standardizing all the different JSON parsers we use as well. kotlinx-serialization is fully KMP and also more performant, because unlike Jackson, it doesn't use runtime reflection, it uses validation at compiler time.