KTOR-7620 Make Url class @Serializable and JVM Serializable #4421

wkornewald · 2024-10-22T15:35:24Z

In our project we had to define our own UrlSerializer. It would be much nicer to have this in the Ktor library itself, so it works out of the box (similar to how Cookie was recently extended).

Also, types like Url and Cookie should be java.io.Serializable. Otherwise Android crashes when using those types as e.g. screen arguments. This happens very quickly when Url is used indirectly as part of a data class where we wanted type safety.

wkornewald · 2024-10-22T15:38:31Z

ktor-io/jvm/src/io/ktor/utils/io/JvmSerializable.jvm.kt

+@Suppress("UNCHECKED_CAST")
+public actual fun <T : Any> JvmSerializerReplacement(serializer: JvmSerializer<T>, value: T): Any =
+    DefaultJvmSerializerReplacement(serializer, value)
+
+@PublishedApi // IMPORTANT: changing the class name would result in serialization incompatibility
+internal class DefaultJvmSerializerReplacement<T : Any>(
+    private var serializer: JvmSerializer<T>?,
+    private var value: T?
+) : Externalizable {


Should such an API better be integrated as part of the Kotlin stdlib itself or at least in a separate library for reuse in other projects? It could also be useful in kotlinx-datetime: Kotlin/kotlinx-datetime#373
And I'll also add the same code to my open-source lib, too (https://github.com/ensody/ReactiveState-Kotlin).
That's unnecessary duplication in multiple places.

Regarding JvmSreializable interface, you can vote on this issue:

KT-48243 Publish kotlin.io.Serializable

I haven't found any issue proposing multiplatform Externalizable implementation, so feel free to create one!

wkornewald · 2024-10-27T06:37:16Z

ktor-http/common/src/io/ktor/http/Url.kt

+    }
+}
+
+internal object UrlJvmSerializer : JvmSerializer<Url> {


Just an idea: To make this slightly easier to implement one could also add a reusable serializer which utilizes kotlinx-serialization (e.g. using JSON or BSON or CBOR internally).

I like this idea! Would you like to implement it in this PR or in a separate one?

In our project we had to define our own UrlSerializer. It would be much nicer to have this in the Ktor library itself, so it works out of the box (similar to how Cookie was recently extended). Also, types like Url and Cookie should be java.io.Serializable. Otherwise Android crashes when using those types as e.g. screen arguments. This happens very quickly when Url is used indirectly as part of a data class where we wanted type safety.

e5l

Could you tell me why can't we mark it with @serializable from kotlinx.serialization?

wkornewald · 2024-11-07T08:29:05Z

Could you tell me why can't we mark it with @serializable from kotlinx.serialization?

This PR is doing that, but that's not enough.

On Android many APIs work with anything that's Parcelable/Serializable. Imagine you have some random data class which contains a Url. You pass that object into an Android API and that tries to parcel/serialize it. There is no kotlinx.serialization involved in this case.

It's very error-prone and inconvenient to force everyone to check all the attributes of every data class and manually use kotlinx.serialization in many different places to ensure that the data is compatible with the Android API. The better solution is to make all widely used types compatible with java.io.Serializable or at least Parcelable. It's the same issue with kotlinx-datetime where people also run into crashes because Instant is incompatible with Android and nobody wants to manually write serializers and remember to use JSON serialization in thousands of places. Everything should just work without crashes and without thinking.

For this to be possible, the types that are very commonly used in the UI like Url, Instant, etc. need to be compatible with java.io.Serializable. But we can also chat or have a quick Huddle on the JetBrains Slack. I'm available there under "Waldemar Kornewald".

e5l · 2024-11-07T09:06:38Z

That sounds good; let's mark parent interfaces with the @InternalApi annotation.

@osipxd, could you also check?

osipxd · 2024-11-07T09:13:56Z

ktor-http/common/src/io/ktor/http/Url.kt

@@ -254,3 +262,22 @@ internal val Url.encodedUserAndPassword: String
    get() = buildString {
        appendUserAndPassword(encodedUser, encodedPassword)
    }
+
+public class UrlSerializer : KSerializer<Url> {
+    override val descriptor: SerialDescriptor = PrimitiveSerialDescriptor("Url", PrimitiveKind.STRING)


Let's use a qualified name

Suggested change

override val descriptor: SerialDescriptor = PrimitiveSerialDescriptor("Url", PrimitiveKind.STRING)

override val descriptor: SerialDescriptor = PrimitiveSerialDescriptor("io.ktor.http.Url", PrimitiveKind.STRING)

osipxd · 2024-11-07T09:20:32Z

ktor-http/common/src/io/ktor/http/Url.kt

+    }
+}
+
+internal object UrlJvmSerializer : JvmSerializer<Url> {


I like this idea! Would you like to implement it in this PR or in a separate one?

osipxd · 2024-11-07T09:27:54Z

ktor-io/jvm/src/io/ktor/utils/io/JvmSerializable.jvm.kt

+@Suppress("UNCHECKED_CAST")
+public actual fun <T : Any> JvmSerializerReplacement(serializer: JvmSerializer<T>, value: T): Any =
+    DefaultJvmSerializerReplacement(serializer, value)
+
+@PublishedApi // IMPORTANT: changing the class name would result in serialization incompatibility
+internal class DefaultJvmSerializerReplacement<T : Any>(
+    private var serializer: JvmSerializer<T>?,
+    private var value: T?
+) : Externalizable {


Regarding JvmSreializable interface, you can vote on this issue:

KT-48243 Publish kotlin.io.Serializable

I haven't found any issue proposing multiplatform Externalizable implementation, so feel free to create one!

osipxd · 2024-11-07T09:36:19Z

ktor-shared/ktor-junit/jvm/src/io/ktor/junit/Assertions.kt

+    val encoded = ByteArrayOutputStream().also {
+        ObjectOutputStream(it).writeObject(obj)
+    }.toByteArray()
+    return ObjectInputStream(encoded.inputStream()).readObject() as T


Should the assertEquals call be right here?

Suggested change

return ObjectInputStream(encoded.inputStream()).readObject() as T

val decoded = ObjectInputStream(encoded.inputStream()).readObject() as T

assertEquals(obj, decoded)

return decoded

It would have to be optional (default true) because not all objects might be equal after deserialization, though in most cases they would be. But yes, that's actually how we do in our implementation. I was just afraid of the code looking 100% the same...

osipxd · 2024-11-07T09:43:04Z

ktor-http/common/src/io/ktor/http/Url.kt

@@ -254,3 +262,22 @@ internal val Url.encodedUserAndPassword: String
    get() = buildString {
        appendUserAndPassword(encodedUser, encodedPassword)
    }
+
+public class UrlSerializer : KSerializer<Url> {


It could be an object. And probably make it inner object Url.Serializer

Suggested change

public class UrlSerializer : KSerializer<Url> {

public object UrlSerializer : KSerializer<Url> {

wkornewald force-pushed the url-serializable branch from 7c7b289 to 62e380e Compare October 22, 2024 15:37

wkornewald commented Oct 22, 2024

View reviewed changes

wkornewald force-pushed the url-serializable branch from 62e380e to 8db09e6 Compare October 27, 2024 06:32

wkornewald commented Oct 27, 2024

View reviewed changes

wkornewald force-pushed the url-serializable branch from 8db09e6 to d217ee0 Compare October 27, 2024 06:49

e5l self-requested a review October 29, 2024 07:35

e5l changed the base branch from main to 3.1.0-eap October 29, 2024 07:36

osipxd force-pushed the 3.1.0-eap branch 2 times, most recently from a65ff10 to 9ae3d49 Compare October 31, 2024 10:15

wkornewald force-pushed the url-serializable branch from 92cfa95 to 6666ecf Compare October 31, 2024 13:41

wkornewald force-pushed the url-serializable branch from 6666ecf to 9057d9f Compare October 31, 2024 14:32

e5l requested a review from bjhham November 7, 2024 07:42

e5l requested changes Nov 7, 2024

View reviewed changes

e5l requested review from e5l and osipxd and removed request for bjhham November 7, 2024 09:06

e5l approved these changes Nov 7, 2024

View reviewed changes

osipxd reviewed Nov 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KTOR-7620 Make Url class @Serializable and JVM Serializable #4421

KTOR-7620 Make Url class @Serializable and JVM Serializable #4421

wkornewald commented Oct 22, 2024

wkornewald Oct 22, 2024 •

edited

Loading

osipxd Nov 7, 2024

wkornewald Oct 27, 2024 •

edited

Loading

osipxd Nov 7, 2024

e5l left a comment

wkornewald commented Nov 7, 2024

e5l commented Nov 7, 2024

osipxd Nov 7, 2024

osipxd Nov 7, 2024

osipxd Nov 7, 2024

osipxd Nov 7, 2024

wkornewald Nov 7, 2024

osipxd Nov 7, 2024

	override val descriptor: SerialDescriptor = PrimitiveSerialDescriptor("Url", PrimitiveKind.STRING)
	override val descriptor: SerialDescriptor = PrimitiveSerialDescriptor("io.ktor.http.Url", PrimitiveKind.STRING)

-    return ObjectInputStream(encoded.inputStream()).readObject() as T
+    val decoded = ObjectInputStream(encoded.inputStream()).readObject() as T
+    assertEquals(obj, decoded)
+    return decoded

	public class UrlSerializer : KSerializer<Url> {
	public object UrlSerializer : KSerializer<Url> {

KTOR-7620 Make Url class @Serializable and JVM Serializable #4421

Are you sure you want to change the base?

KTOR-7620 Make Url class @Serializable and JVM Serializable #4421

Conversation

wkornewald commented Oct 22, 2024

wkornewald Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wkornewald Oct 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e5l left a comment

Choose a reason for hiding this comment

wkornewald commented Nov 7, 2024

e5l commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wkornewald Oct 22, 2024 •

edited

Loading

wkornewald Oct 27, 2024 •

edited

Loading