Implement java.io.Serializable for some of the classes #373

dkhalanskyjb · 2024-03-22T14:03:37Z

Implement java.io.Serializable for

Instant
LocalDate
LocalTime
LocalDateTime
UtcOffset

TimeZone is not Serializable because its behavior is system-dependent. We can make it java.io.Serializable later if there is demand.

We are using string representations instead of relying on Java's entities being java.io.Serializable so that we have more freedom to change our implementation later.

Fixes #143

Moozart · 2024-04-09T14:30:13Z

Can you please merge this commit if possible? @ilya-g

ilya-g

For better control over serialized representation I would suggest using writeReplace/readResolve with an Externalizable container.

ilya-g · 2024-04-19T15:07:56Z

core/jvm/src/Instant.kt

@@ -111,6 +113,25 @@ public actual class Instant internal constructor(internal val value: jtInstant)

        internal actual val MIN: Instant = Instant(jtInstant.MIN)
        internal actual val MAX: Instant = Instant(jtInstant.MAX)
+
+        @JvmStatic
+        private val serialVersionUID: Long = 1L


We usually use const val for that.

dkhalanskyjb · 2024-04-22T09:07:59Z

I don't understand the idea. Do you mean that LocalTime should be Externalizable?

From the docs (https://docs.oracle.com/javase%2F7%2Fdocs%2Fapi%2F%2F/java/io/Externalizable.html):

When an Externalizable object is reconstructed, an instance is created using the public no-arg constructor

But we don't have public no-arg constructors. I guess we could do something like class LocalTime { @PublishedApi internal constructor() }, which would make it a public constructor for the JVM, and trick Kotlin somehow to avoid initializing val value: java.time.LocalTime in that constructor, but is it worth it to make such intrusive changes just to support Java serialization?

akhbulatov · 2024-04-26T11:42:57Z

A very necessary implementation on Android to be able to pass LocalDate/LocalDateTime objects between screens

dkhalanskyjb · 2024-04-26T12:14:23Z

@akhbulatov, it's not "necessary", just convenient: see #143 (comment)

wkornewald · 2024-06-20T06:43:45Z

Do you have an estimate when this will get merged? It's a pretty huge inconvenience for us to wrap Instant because we want to export an SDK and the wrapping shouldn't be exposed to our users. It seems to be impossible without writing huge amounts of boilerplate.

ilya-g

Take a look at how the list and set builders are serialized in stdlib. I believe we'd better have something similar here.
If you want, I can take over this branch and implement it accordingly.

ilya-g · 2024-08-02T15:03:15Z

core/jvm/src/Instant.kt

+    }
+
+    private fun writeObject(oStream: java.io.ObjectOutputStream) {
+        oStream.defaultWriteObject()


I believe this would write all object fields first.

dkhalanskyjb · 2024-08-05T09:50:06Z

Do you mean https://github.com/JetBrains/kotlin/blob/3be0aa15f714726e4872c09b35f789652c46876d/libraries/stdlib/jvm/src/kotlin/collections/builders/ListBuilder.kt#L674? Ok, I think I got the idea, thanks. This does look better than reflection.

It's not important to me who does this.

ilya-g · 2024-08-15T15:03:57Z

core/jvm/src/internal/SerializedValue.kt

+import kotlinx.datetime.*
+import java.io.*
+
+internal class SerializedValue(var typeTag: Int, var value: Any?) : Externalizable {


Alternatively, we can place it in kotlinx.datetime package and name it shortly, e.g. Ser, to shave off some bytes.

Do you mean, shave off some bytes from the serialized representation? I'm okay with that, as long as it still stays in the internal/ directory.

Does it make sense to mark this with @PublishedApi to signal that it's an incompatible change to move/rename this class?

Does it make sense to mark this with @PublishedApi to signal that it's an incompatible change to move/rename this class?

Yes, I can do that

dkhalanskyjb · 2024-08-16T10:34:34Z

core/jvm/src/internal/SerializedValue.kt

+import kotlinx.datetime.*
+import java.io.*
+
+internal class SerializedValue(var typeTag: Int, var value: Any?) : Externalizable {


Can these values be private?

They can, but does it makes a difference for an internal class? Or you mean in order not to generate getters/setters?

Yep, for the latter.

dkhalanskyjb · 2024-08-16T10:34:36Z

core/jvm/src/internal/SerializedValue.kt

+import kotlinx.datetime.*
+import java.io.*
+
+internal class SerializedValue(var typeTag: Int, var value: Any?) : Externalizable {


Do you mean, shave off some bytes from the serialized representation? I'm okay with that, as long as it still stays in the internal/ directory.

Does it make sense to mark this with @PublishedApi to signal that it's an incompatible change to move/rename this class?

dkhalanskyjb · 2024-08-19T14:27:17Z

I think it's good to go, but can't approve a pull request that I myself opened.

dkhalanskyjb · 2024-11-13T13:33:29Z

The reason I'm not clicking the "merge" button is that we are going to move kotlinx.datetime.Instant to kotlin.time.Instant (#382), so I don't think it makes sense to make it java.io.Serializable. @ilya-g, WDYT about removing java.io.Serializable for Instant from this PR and merging the implementation for the rest of the classes?

wkornewald · 2024-11-13T15:17:45Z

Does this mean the stdlib will get a nice solution for adding Serializable support from within KMP code? I've contributed something to Ktor which could possibly serve as a starting point for a simple helper API in the stdlib: ktorio/ktor#4421

dkhalanskyjb · 2025-03-04T10:30:35Z

@ilya-g, FYI: only the last commit is new, the rest are only updated because I've had to rebase this PR.

dkhalanskyjb · 2025-03-17T13:32:14Z

Now there are two new commits.

Implement java.io.Serializable for * Instant * LocalDate * LocalTime * LocalDateTime * UtcOffset TimeZone is not `Serializable` because its behavior is system-dependent. We can make it `java.io.Serializable` later if there is demand. We are using string representations instead of relying on Java's entities being `java.io.Serializable` so that we have more freedom to change our implementation later. Fixes #143

Implement java.io.Serializable for * LocalDate * LocalTime * LocalDateTime * UtcOffset TimeZone is not `Serializable` because its behavior is system-dependent. We can make it `java.io.Serializable` later if there is demand. Instant is not `Serializable` because it is about to be removed. We are using string representations instead of relying on Java's entities being `java.io.Serializable` so that we have more freedom to change our implementation later. Fixes #143

dkhalanskyjb · 2025-03-20T11:33:47Z

Merged manually in 98c3e53 to preserve authorship.

lukellmann · 2025-03-22T20:12:21Z

core/jvm/src/LocalDate.kt

@@ -100,6 +102,8 @@ public actual class LocalDate internal constructor(internal val value: jtLocalDa
    @PublishedApi
    @JvmName("toEpochDays")
    internal fun toEpochDaysJvm(): Int = value.toEpochDay().clampToInt()
+
+    private fun writeReplace(): Any = Ser(Ser.DATE_TAG, this)


The java.time classes also have a readObject method that always throws an exception with a comment that this is about defending against malicious streams. I'm not very familiar with Java's serialization, so I don't know if this is needed.

I found this post about serialization via writeReplace/readResolve. This section is about the always throwing readObject method. Without it, a hand crafted byte stream could cause deserialization of e.g. LocalDate without going through Ser.readResolve. This could violate invariants of the classes. I think it could for example sneak in null to the non-nullable property LocalDate.value.

Thanks! Yes, deserialize("aced00057372001a6b6f746c696e782e6461746574696d652e4c6f63616c44617465618443f17dae33e70200014c00097472756556616c75657400154c6a6176612f74696d652f4c6f63616c446174653b787070" .hexToByteArray(HexFormat { bytes.byteSeparator = "" })) does indeed create a LocalDate whose value is null.

@lukellmann, would you like to make a PR that fixes (and tests!) this problem?

sure, but it will probably take a few days

I'm on it but it will take a bit (I'm going through the spec).

dkhalanskyjb force-pushed the add-java-serialization branch from e803d80 to 75a14a6 Compare March 22, 2024 14:03

dkhalanskyjb requested a review from ilya-g March 25, 2024 11:05

ilya-g requested changes Apr 19, 2024

View reviewed changes

dkhalanskyjb force-pushed the add-java-serialization branch from 75a14a6 to 9fc52b6 Compare May 13, 2024 12:15

dkhalanskyjb requested a review from ilya-g May 13, 2024 12:15

dkhalanskyjb force-pushed the add-java-serialization branch from 9fc52b6 to 32c9cc0 Compare May 23, 2024 12:21

ilya-g requested changes Aug 2, 2024

View reviewed changes

ilya-g reviewed Aug 15, 2024

View reviewed changes

ilya-g force-pushed the add-java-serialization branch from ce1dc4a to 42fa2a5 Compare August 15, 2024 15:10

dkhalanskyjb commented Aug 16, 2024

View reviewed changes

ilya-g approved these changes Aug 19, 2024

View reviewed changes

wkornewald mentioned this pull request Oct 23, 2024

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

Merged

dkhalanskyjb mentioned this pull request Nov 13, 2024

Implement YearMonth #457

Draft

dkhalanskyjb force-pushed the add-java-serialization branch from d6238ee to e876cfd Compare March 4, 2025 10:20

dkhalanskyjb requested a review from ilya-g March 4, 2025 10:29

dkhalanskyjb added this to the 0.7.0 milestone Mar 17, 2025

dkhalanskyjb force-pushed the add-java-serialization branch from 34da4d1 to 5df40c3 Compare March 17, 2025 13:12

ilya-g approved these changes Mar 19, 2025

View reviewed changes

dkhalanskyjb and others added 7 commits March 20, 2025 11:46

Fix compilation after a merge

3b94059

val serialVersionUID -> const val

a29c32a

Use externalizable replacement for serializable entities

17bac4b

Rename serialization surrogate and annotate with PublishedAPI

ca7d2d1

Remove some leftovers

a67b418

Do not implement Instant: java.io.Serializable

8c767ea

Add the API dump

1776d8c

dkhalanskyjb force-pushed the add-java-serialization branch from 5df40c3 to 1776d8c Compare March 20, 2025 11:02

dkhalanskyjb closed this Mar 20, 2025

dkhalanskyjb deleted the add-java-serialization branch March 20, 2025 11:33

lukellmann reviewed Mar 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement java.io.Serializable for some of the classes #373

Implement java.io.Serializable for some of the classes #373

dkhalanskyjb commented Mar 22, 2024

Moozart commented Apr 9, 2024

ilya-g left a comment

ilya-g Apr 19, 2024

dkhalanskyjb commented Apr 22, 2024

akhbulatov commented Apr 26, 2024

dkhalanskyjb commented Apr 26, 2024

wkornewald commented Jun 20, 2024

ilya-g left a comment

ilya-g Aug 2, 2024

dkhalanskyjb commented Aug 5, 2024

ilya-g Aug 15, 2024

dkhalanskyjb Aug 16, 2024

ilya-g Aug 19, 2024

dkhalanskyjb Aug 16, 2024

ilya-g Aug 19, 2024

dkhalanskyjb Aug 19, 2024

dkhalanskyjb Aug 16, 2024

dkhalanskyjb commented Aug 19, 2024

dkhalanskyjb commented Nov 13, 2024

wkornewald commented Nov 13, 2024

dkhalanskyjb commented Mar 4, 2025

dkhalanskyjb commented Mar 17, 2025

dkhalanskyjb commented Mar 20, 2025

lukellmann Mar 22, 2025

lukellmann Mar 22, 2025

dkhalanskyjb Mar 24, 2025

dkhalanskyjb Mar 24, 2025

lukellmann Mar 24, 2025

lukellmann Mar 30, 2025

Implement java.io.Serializable for some of the classes #373

Implement java.io.Serializable for some of the classes #373

Conversation

dkhalanskyjb commented Mar 22, 2024

Moozart commented Apr 9, 2024

ilya-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkhalanskyjb commented Apr 22, 2024

akhbulatov commented Apr 26, 2024

dkhalanskyjb commented Apr 26, 2024

wkornewald commented Jun 20, 2024

ilya-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkhalanskyjb commented Aug 5, 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

Choose a reason for hiding this comment

dkhalanskyjb commented Aug 19, 2024

dkhalanskyjb commented Nov 13, 2024

wkornewald commented Nov 13, 2024

dkhalanskyjb commented Mar 4, 2025

dkhalanskyjb commented Mar 17, 2025

dkhalanskyjb commented Mar 20, 2025

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