Fix serializing nulls for a property of a parameterized type with a nullable upper bound with Protobuf

[ShreckYe]

No description provided.

[ShreckYe]

The checks on Linux failed because "Gradle build daemon disappeared unexpectedly (it may have been killed or may have crashed)". Is there a way to rerun it?

[ShreckYe]

Here is some brief information for this PR.

See such a class definition with a type parameter T which has a nullable upper bound:

@Serializable
data class Box<T>(val value: T)

ProtoBuf.encodeToByteArray(Box(null)) fails with an exception:

kotlinx.serialization.SerializationException: 'null' is not allowed for not-null properties

While for JSON, Json.encodeToString(Box(null)) runs successfully with the result {"value":null}.

[pdvrieze]

It looks like what you are trying to do is to omit the box if the member is empty. Is that not more of a case for either a custom serializer or a value class (which formats can handle specially). In terms of your tests, you at least need to test for correct behaviour if there are more members than just 1.
Beyond that, this behaviour would need to be configurable (and not default) as you are making an incompatible change to the format.

[ShreckYe]

It looks like what you are trying to do is to omit the box if the member is empty.

No I don't think this is what I am doing. I mean it's probably a bug that, when the upper bound of a type of a property is nullable instead of the type itself, serializing a null fails for Protobuf. I will change the verb in the subject of this PR to "fix".

[pdvrieze]

It looks like what you are trying to do is to omit the box if the member is empty.

No I don't think this is what I am doing. I mean it's probably a bug that, when the upper bound of a type of a property is nullable instead of the type itself, serializing a null fails for Protobuf. I will change the verb in the subject of this PR to "fix".

I was looking at the test data. The data is empty when the member is null, and not empty if there is a member. This looks like what happens for inline structs. Looking back it may be because you directly serialize the struct so the box is not tagged (as it is the outer type).

[ShreckYe]

The data is empty when the member is null, and not empty if there is a member.

I think this is the correct behavior of Protobuf here. In the Protobuf Java library, writeTo() serializes a message just by serializing all its fields, and if all its optional fields are empty, the result is an empty message. There is a writeDelimitedTo() which writes the message size in varint encoding before the message (which is also how it serializes the data of a field of bytes, a nested message, or packed data), but it seems it's not supported here in this library yet.

If you run the code below on the dev branch the result is 0, which means the result is also an empty string.

class NullableIntBox(val value: Int?)
println(ProtoBuf.encodeToHexString(NullableIntBox(null)).length)

[cliffred]

I came across the same exception, and also another very similar one in org.mongodb:bson-kotlinx. But I hadn't seen this PR. I actually think the root cause is in the kotlinx.serialization compiler plugin. Because the default upper bound for a type parameter is Any?, they are nullable. Therefore I would expect that the write$Self$main static method generated by the kotlinx.serialization compiler plugin would invoke CompositeEncoder.encodeNullableSerializableElement, but instead it invokes CompositeEncoder.encodeSerializableElement. I've created JetBrains/kotlin#5271 to address this. This will also automatically fix this problem with the Protobuf encoder.

[ShreckYe]

@cliffred Cool. I also played with the compiler plugin for a while and later found that this could be resolved without changing the code in the compiler plugin. I think both are viable solutions.

[cliffred]

Me too, it's indeed a good idea to also deal with this within the individual encoders, therefore I also submitted a PR to the MongoDB BSON kotlinx encoder to fix it also within that encoder.

[sandwwraith]

Thanks for your PR! I believe this is the correct way to fix this in the encoder.

[sandwwraith]

Do you mind also adding test for class Foo<T: Any>(val t: T) to check that non-nullable type parameters still reject nulls?

[ShreckYe]

I'm sorry I am not sure how to do this. I could add some tests for class Foo<T: Any>(val t: T) but it's impossible to set a null for the member t, unless I go for some hacks like calling the constructor in Java and passing a null argument (I am not completely sure this works though).

[sandwwraith]

Yeah, you're right, that's impossible for Kotlin's type system. Never mind :)

[sandwwraith]

I've also taken a look at the PR by @cliffred. It seems to be the correct approach to my intuition, but I should re-check if it doesn't break anything.