Kotlin / kotlinx.serialization

Kotlin multiplatform / multi-format serialization
Apache License 2.0
5.4k stars 620 forks source link

[Protobuf] Respect default values from official documentation #2831

Open glureau opened 3 weeks ago

glureau commented 3 weeks ago

(I did some research but not sure if I missed some documentation or discussions somewhere, sorry if it's a duplicate.)

What is your use-case and why do you need this feature?

My use-case is to generate correct protobuf3 messages so that other services (that may use other tools than kotlinx) are able to read properly.

A successful test as introduction:

    enum class Foo { A, B }

    @Serializable
    data class Bar(val foo: Foo = Foo.B)

    @Test
    fun defaultTest() { // ✅ 
        val protobuf = ProtoBuf {
            encodeDefaults = false
        }
        val bar = Bar(Foo.B)
        val serialized = protobuf.encodeToByteArray(bar)
        assertEquals(0, serialized.size)
        val deserialized = protobuf.decodeFromByteArray<Bar>(serialized)
        assertEquals(bar, deserialized)
    }

This is not matching the official documentation as far as I understand: https://protobuf.dev/programming-guides/proto3/#default , especially those 2 quotes

For enums, the default value is the first defined enum value, which must be 0.

Also note that if a scalar message field is set to its default, the value will not be serialized on the wire.

It looks like we are mixing the Kotlin default values and what Protobuf considers the default values. If this example is able to produce an empty ByteArray, decoding with the same protobuf from Java will lead to a Bar(Foo.A) instead of Bar(Foo.B).

This error is not limited to enums but all scalar types as mentioned in protobuf documentation (strings, bytes, bools, numeric types and enums).

Describe the solution you'd like

I'd like to have a serialization that respects the proto 3 format:

I consider the current encodeDefaults = false to be the best solution, given official doc "the value will not be serialized on the wire", and the fact that encoding default make it unusable with nullable fields. I presume it's ok to keep this flag for retro-compatibility if needed, but having a proper proto3 support is higher in my priority list.

pdvrieze commented 1 day ago

@glureau It makes sense to add a mode to support it, but for compatibility reasons it wouldn't be valid to change the default behaviour. In terms of enums, the solution might be to "reorder" the enum fields so the default field will also be the one with index 0 (but again this changes the on-the-wire contents of the protobuf so may be incompatible).