CborArray bug with default values

[Plegbot]

Describe the bug When a class with multiple default values is encoded into a CborArray with encodeDefaults = false, the information about which value has changed from the default isn't preserved.

To Reproduce

@file:OptIn(ExperimentalSerializationApi::class)

import kotlinx.serialization.ExperimentalSerializationApi
import kotlinx.serialization.Serializable
import kotlinx.serialization.cbor.Cbor
import kotlinx.serialization.cbor.CborArray
import kotlinx.serialization.decodeFromByteArray
import kotlinx.serialization.encodeToByteArray

@Serializable
@CborArray
data class A(val a: Int = 0, val b: Int = 0)

fun main() {
    val instance = A(b = 1)
    val cbor = Cbor // Default settings
    println(cbor.decodeFromByteArray<A>(cbor.encodeToByteArray(instance))) // prints A(a=1, b=0)
}

@file:OptIn(ExperimentalSerializationApi::class)

import kotlinx.serialization.ExperimentalSerializationApi
import kotlinx.serialization.Serializable
import kotlinx.serialization.cbor.Cbor
import kotlinx.serialization.cbor.CborArray
import kotlinx.serialization.decodeFromByteArray
import kotlinx.serialization.encodeToByteArray

@Serializable
@CborArray
data class A(val a: Int = 0, val b: String = "")

fun main() {
    val instance = A(b = "Changed")
    val cbor = Cbor // Default settings
    println(cbor.decodeFromByteArray<A>(cbor.encodeToByteArray(instance))) // throws kotlinx.serialization.cbor.internal.CborDecodingException: Expected start of string, but found 43
}

Both tests work properly when I change val cbor = Cbor to val cbor = Cbor { encodeDefaults = true }

Expected behavior The first test should print A(a=0, b=1) The second test should print A(a=0, b=Changed)

Possible Solution Always encode all default values except (maybe) the last one when encoding to CborArray. Maybe (to save space) instead encode some kind of placeholder? I don't know cbor well enough to know if it's possible.

Environment

• Kotlin version: 2.1.20
• Library version: 1.7.3
• Kotlin platforms: Only tested on JVM
• Gradle version: Not sure, 8+

[pdvrieze]

There is some issue with the documentation that doesn't realise that for formats without value markers it is not valid to omit defaults as the reader will not be able to know the absence of such marker. (The never annotation is probably invalid in its semantics).

[sandwwraith]

I guess the only reasonable fix is to force writing of the default value if the class has @CborArray annotation

[fzhinkin]

Maybe (to save space) instead encode some kind of placeholder?

That's another option. There's a special pair of tag and value representing a hole inside CBOR array: 31(undefined), see https://www.iana.org/assignments/cbor-tags/cbor-tags.xhtml

I'm not sure how widespread this notation is and how such arrays will be interpreted by other CBOR parsers.

That way, resulting sequence size will be smaller (for every property with a default value wider than Short)

[sandwwraith]

@JesusMcCloud contributed @CborArray to us, maybe he'll know what approach is better

[JesusMcCloud]

I was facing the same issue with preparing an ASN.1 KxS format. Everything is done EXCEPT this precise issue.

The approach I am pursuing for ASN.1 (albeit without having the time to implement it) is conceptually simple, but a bit tricky to get right: Outright refuse encoding that results in ambiguous data.
I think this is the only valid strategy for the following reasons:

• You will know at compile-time whether encoding will work or not: Compilation will fail and you will never face runtime errors, wich makes everything bullet-proof (given the heuristics that decide whether you'll get ambiguous data is bullet-proof). This, to me is the killer argument here, because pretty much every software stack has been haunted by deserialization bugs with catastrophic consequences.
• No surprises: Instead of diluting the semantics of annotations, or introducing more (complicated) Annotations that just add complexity, you know exactly how your wire format will look like. This directly ties into the first bullet.
• Encapsulation: We only need a single detour in the encoding path that checks and everything else stays as it it. Hence, much less potential for bugs in the encoder. This also ties into the first bullet: The check is generic for all formats that share the same conceptual encoding format, so we have a single source of truth.
• Reusability/Composability: This checker logic should be exposed as part of the public API for such formats. Yes, it will require some extensibility and the possibility to override the provided functions, but when we try this with CBOR and ASN.1 (which causes this issue in a slightly different way), I think we have what we need to get it done. Also: Some ExperimentalInternalFoo opt-in annotation can guard it, so you know that it has the potential to break if you use that from community formats.

@wkornewald taggig you here as well, since we just talked about this issue recently

[pdvrieze]

@JesusMcCloud Am I understanding you correctly if you want to standardize some sort of verification logic for particular formats. It would have to be something done as a separate verifier as there is no way for the compiler (plugin) to know the specific format used, and this is an issue with the serializer/format combination.

As to how this could be done, a combination of a standard verification gradle plugin with service loaders. Even then the plugin would need to allow for some configuration that helps decide which serializer/format combinations should actually be verified.

[JesusMcCloud]

I suspect you are right! and during compilation it's impossible to know which format will be used as the generated serializer code is format-independent. I guess we're back to a runtime error then. Still, I thing we should outright refuse to even serialize something ambiguous.

[pdvrieze]

@JesusMcCloud It is fundamentally a format level decision on whether to pre-verify the serializers. I would say that build time verification is mostly possible with a single restriction: contextual resolvers (and somewhat polymorphic resolving). This can be specified to the verifier though, and it would also be possible to identify all (de)serialization entrypoints using a compiler plugin. Will this make it impossible to work around build-time verification, no, but it should automatically catch most cases (and force specification of them).

As to runtime-pre-verification, this is expensive (and also hard) as it requires verifying the entire potential serializer graph (XML builds this lazily as it needs it, but even then only partially). As to not serializing ambiguous data, that is ideally one of the design goals of a format, but it may either be hard to implement, or expensive to check.

[JesusMcCloud]

that is the "tricky to get right" part I have not spent time on and I don't know enough about how KxS works internally. I have only ever worked on formats. It's entirely possible that this problem is not just a bit tricky, but infeasible to solve in practice. Which would then mean that this bug is no bug at all, because you will always be able to generate a borked definition of a serializable class that will result in ambiguities.

[wkornewald]

The compiler plugin approach should be fine though. I'm just not sure if that's sufficient to catch all cases. And even a runtime check could come with a one-time init cost per class which shouldn't be too bad.

[pdvrieze]

@wkornewald Runtime checks are a bit more complex. Format configuration can have an influence, as well as containing classes, as such it would need to be per formant instance per root serializer.

@JesusMcCloud If we take custom serializers, there are many ways to get those wrong. But a build time analyser doesn't need to solve every case, only common ones.

But thinking about it again, maybe a test framework / and pedantic mode may be a better approach (as it allows normal code)