Structured cbor #3036

JesusMcCloud · 2025-07-05T05:03:02Z

(not yet ready for review, as an internal review, cleanups and more thorough test cases are still pending; hence draft)

Discussion points @sandwwraith :

Which values (if any) of the Cbor configuration should be respected? My take:
- useDefiniteLengthEncoding: Yes
- preferCborLabelsOverNames: Probably no. Rather: if a label is present on a CborElement, use that over the name
- all others: make no sense, so ignore

Still TODO:

Label support
Review internally, cleanup and expand test suite
decode from/to CborElement
ApiDump
Documentation
Changelog

JesusMcCloud · 2025-07-08T10:28:59Z

Full disclosure: This PR incorporates code from a draft generated by Junie (albeit an impressive draft that saved a day of work). This is not a dumb copypasta of AI-generated code. Even if it were already feature-complete It would still not yet be marked ready for review because we have yet to review everything internally. I also want to stress that "we" is not a euphemism. There will be at least two of us reviewing and discussing internally, almost certainly with additional input from other humans in the process of readying this PR.

pdvrieze

I've reviewed the code at a general level. There is a lot of repetition, so I've only commented on the first case, not every one. I guess some of the AI generation is visible in the details.

pdvrieze · 2025-07-08T20:12:11Z

formats/cbor/commonMain/src/kotlinx/serialization/cbor/Cbor.kt

+                encodeKeyTags = true,
+                encodeValueTags = true,
+                encodeObjectTags = true,
+                verifyKeyTags = true,
+                verifyValueTags = true,
+                verifyObjectTags = true,


As these parameters change the behaviour of the system you should not change the default. Instead you could provide a way for users to choose a newer/different configuration.

thanks! definitely not intended

pdvrieze · 2025-07-08T20:15:40Z

formats/cbor/commonMain/src/kotlinx/serialization/cbor/CborElement.kt

+     * See [RFC 8949 3.4. Tagging of Items](https://datatracker.ietf.org/doc/html/rfc8949#name-tagging-of-items).
+     */
+    @OptIn(ExperimentalUnsignedTypes::class)
+    public val tags: ULongArray = ulongArrayOf()


You probably want to keep out the default initializer to ensure that any concrete subclass passes the value (it is sealed so it is not a usability issue to require it to be passed along).

Note that if tags are uncommon you should probably make it nullable to avoid the overhead of the empty array. Alternatively you could create an internal empty array value that is reused.

i'd go for an internal empty array. It does not make sense to differentiate between a null value and an empty array

pdvrieze · 2025-07-08T20:16:06Z

formats/cbor/commonMain/src/kotlinx/serialization/cbor/CborElement.kt

+@Serializable(with = CborIntSerializer::class)
+public class CborNegativeInt(
+    public val value: Long,
+    tags: ULongArray = ulongArrayOf()


See above, remove the default

Well actually, I am confident that all empty arrays of primitives delegate to a Singleton, so no premature optimisation planned here and in all other places

pdvrieze · 2025-07-08T20:19:08Z

formats/cbor/commonMain/src/kotlinx/serialization/cbor/CborElement.kt

+ * Class representing CBOR `null` value
+ */
+@Serializable(with = CborNullSerializer::class)
+public class CborNull(tags: ULongArray=ulongArrayOf()) : CborPrimitive(tags) {


The default value should remain, but use an internal array (or null) to avoid extensive copies of empty arrays.

pdvrieze · 2025-07-08T20:21:28Z

formats/cbor/commonMain/src/kotlinx/serialization/cbor/internal/CborElementSerializers.kt

+        }
+
+    override fun serialize(encoder: Encoder, value: CborElement) {
+        encoder.asCborEncoder()


Ideally you allow serializing to other formats with the "normal" behaviour (see the serializers for JsonElement etc.)

doesn't the JsonElement serializer have a verify method that ensures it can only be used with the JSON format?

turns out: it does

@JesusMcCloud You are right. I had missed the check because it was encapsulated. The worst part is that it would actually work without that verify check.

it does make life easier for those implementing the serializers. for me at least, i don't have to think about how this will behave with other formats, esp. wrt. tagging.

pdvrieze · 2025-07-08T20:23:52Z

formats/cbor/commonMain/src/kotlinx/serialization/cbor/internal/CborElementSerializers.kt

+    }
+}
+
+public object CborIntSerializer : KSerializer<CborNegativeInt> {


Should be internal like the other specialist serializers. They are accessible on the type itself.

pdvrieze · 2025-07-08T20:25:57Z

formats/cbor/commonMain/src/kotlinx/serialization/cbor/CborElement.kt

+ * Class representing signed CBOR integer (major type 1).
+ */
+@Serializable(with = CborIntSerializer::class)
+public class CborNegativeInt(


The name NegativeInt is misleading as it appears to mean a value that can only be negative, not a signed value (that can be positive or negative). Given this name is poor, you also want to rename the counterpart from PositiveInt.

That was intentional. IIRC only negative ints are encoded as signed int in cbor, so it really must be a negative int

But what if they are created programmatically? If you have this value based type you need a factory that creates the correct version on demand (and probably have an "CborInt" parent type that is sign independent)

a sealed parent makes sense (with a custom invoke function that returns the subtype based on sign), but the name for this class is the only one i could think of that is actually accurate

@JesusMcCloud With the explanation it certainly does make sense to call it positive/negative, although you could also just have a single type and make the distinction only on writing (as it is value dependent and deterministic anyway). Maybe the single type is best as that also avoids instances being created with invalid values.

it definitely makes life easier when you don't parse, but create. when parsing, though, I am slightly leaning towards being very explicit and very close to the wire format (which is what we're representing). I'll let @sandwwraith decide

pdvrieze · 2025-07-08T20:28:18Z

formats/cbor/commonMain/src/kotlinx/serialization/cbor/internal/CborElementSerializers.kt

+private fun defer(deferred: () -> SerialDescriptor): SerialDescriptor = object : SerialDescriptor {
+    private val original: SerialDescriptor by lazy(deferred)
+
+    override val serialName: String
+        get() = original.serialName
+    override val kind: SerialKind
+        get() = original.kind
+    override val elementsCount: Int
+        get() = original.elementsCount
+
+    override fun getElementName(index: Int): String = original.getElementName(index)
+    override fun getElementIndex(name: String): Int = original.getElementIndex(name)
+    override fun getElementAnnotations(index: Int): List<Annotation> = original.getElementAnnotations(index)
+    override fun getElementDescriptor(index: Int): SerialDescriptor = original.getElementDescriptor(index)
+    override fun isElementOptional(index: Int): Boolean = original.isElementOptional(index)
+}


Rather than this, it may be better to have the descriptor property itself be lazy, rather than each of its elements.

JesusMcCloud added 4 commits July 5, 2025 05:02

WIP strucutred cbor

c744df7

proper when for decoding

0a5fd66

baseline tagging

8709218

some cleanups

b94a742

pdvrieze reviewed Jul 8, 2025

View reviewed changes

Structured cbor #3036

Are you sure you want to change the base?

Structured cbor #3036

Uh oh!

Conversation

JesusMcCloud commented Jul 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JesusMcCloud commented Jul 8, 2025

Uh oh!

pdvrieze left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JesusMcCloud commented Jul 5, 2025 •

edited

Loading