JAVA-6224 Ensure Kotlin can encode ByteArrays correctly - #2019
Conversation
Narrow the DataClassCodec array interception to exclude ByteArray so it falls through to ByteArrayCodec (BSON Binary), restoring pre-5.1.3 behavior. ArrayCodecProvider is registered before ByteArrayCodec in both the test registry and the production KotlinCodecProvider, so it must apply the same ByteArray exclusion and return null to let the registry fall through to ByteArrayCodec. Object arrays still route through ArrayCodec (JAVA-5122). JAVA-6224
kotlinx.serialization encodes a ByteArray as a BSON array of int32 elements by default, which differs from bson-kotlin's DataClassCodec (compact BSON Binary). Add an opt-in ByteArrayAsBsonBinary KSerializer so users can store ByteArray fields as BsonBinary via @serializable(with = ByteArrayAsBsonBinary::class) or @contextual plus its serializersModule. It is not registered in defaultSerializersModule, so existing behavior and on-disk format are unchanged. JAVA-6224
There was a problem hiding this comment.
Pull request overview
This PR addresses ByteArray encoding differences/regressions across the Kotlin codecs by ensuring bson-kotlin encodes ByteArray as compact BSON Binary and by adding an opt-in kotlinx.serialization serializer to encode ByteArray as BSON Binary when desired.
Changes:
- Update
bson-kotlincodec selection soByteArrayusesByteArrayCodec(BSON Binary) instead of the generic array codec. - Add an opt-in
kotlinx.serializationKSerializer<ByteArray>(ByteArrayAsBsonBinary) for compact BSON Binary encoding inbson-kotlinx. - Extend tests and sample data classes in both modules to cover
ByteArrayfields and nestedByteArrayarrays.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| bson-kotlinx/src/test/kotlin/org/bson/codecs/kotlinx/samples/DataClasses.kt | Adds sample serializable data classes exercising ByteArray and nested arrays with opt-in binary serialization. |
| bson-kotlinx/src/test/kotlin/org/bson/codecs/kotlinx/KotlinSerializerCodecTest.kt | Adds round-trip tests documenting default ByteArray behavior and verifying opt-in binary encoding via ByteArrayAsBsonBinary. |
| bson-kotlinx/src/main/kotlin/org/bson/codecs/kotlinx/ByteArrayAsBsonBinary.kt | Introduces a KSerializer<ByteArray> that encodes/decodes as compact BsonBinary and provides a contextual SerializersModule. |
| bson-kotlin/src/test/kotlin/org/bson/codecs/kotlin/samples/DataClasses.kt | Extends Kotlin (non-kotlinx) sample data classes to include ByteArray and nested ByteArray arrays with correct equality semantics. |
| bson-kotlin/src/test/kotlin/org/bson/codecs/kotlin/DataClassCodecTest.kt | Adds/updates tests to assert ByteArray encodes as BSON Binary and remains size-efficient; extends array test coverage. |
| bson-kotlin/src/main/kotlin/org/bson/codecs/kotlin/DataClassCodec.kt | Prevents ByteArray from being treated as a generic array so the registry can select ByteArrayCodec. |
| bson-kotlin/src/main/kotlin/org/bson/codecs/kotlin/ArrayCodecProvider.kt | Ensures ByteArray falls through to ByteArrayCodec by returning null for ByteArray rather than an ArrayCodec. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nhachicha
left a comment
There was a problem hiding this comment.
LGTM, Claude review revealed some minor things, the important one to consider is the migration/breaking existing users with this fix
Review — blocking & important items
bson-kotlin/src/main/kotlin/org/bson/codecs/kotlin/DataClassCodec.kt
- 🟡 [important] Decoding data written by the regressed versions is now a hard failure with no migration story. Affected releases persisted
ByteArrayfields as BSON arrays of int32; after this fix those documents decode viaByteArrayCodec, which throwsBsonInvalidOperationException("readBinaryData can only be called when CurrentBSONType is BINARY"). Either add a lenient fallback that reads a BSON array of int32 whencurrentBsonType == ARRAY, or explicitly decide against leniency and document the break in JAVA-6224 / release notes (naming which released versions produced the array form). A test pinning whichever behavior is chosen would document it.
bson-kotlin/src/test/kotlin/org/bson/codecs/kotlin/DataClassCodecTest.kt
- 🔴 [blocking]
testDataClassWithObjectArrayEncodesAsBsonArrayis a byte-for-byte duplicate of the updatedtestDataClassWithArrays— identicalDataClassWithArraysinstance and semantically identical expected JSON (differences are whitespace only). It adds no coverage and doubles maintenance. Suggest keeping one: either revert the byte-array additions totestDataClassWithArraysand keep the new focused test, or delete the new one. - 🟡 [important] In
testDataClassWithByteArrayDoesNotExpandDocumentSize,document.toBsonDocument()is redundant —documentis already aBsonDocument(the call returnsthis). Usedocument.getBinary("byteArray").data.sizedirectly.
bson-kotlin/src/main/kotlin/org/bson/codecs/kotlin/ArrayCodecProvider.kt
- 🟡 [important] Returning
nullforByteArrayassumes the registry contains aByteArrayCodec. Users composingArrayCodecProviderin a custom registry withoutValueCodecProviderpreviously got a (verbose) working codec and will now getCodecConfigurationException. This is the intended pre-regression behavior, but worth a line in the release notes.
bson-kotlinx/src/main/kotlin/org/bson/codecs/kotlinx/ByteArrayAsBsonBinary.kt
- 🟡 [important] The public
serializersModuleproperty has no KDoc. In anexplicitApi()module — and referenced from the class KDoc as one of the two opt-in mechanisms — it should document what registering it does (@Contextual ByteArrayfields use this serializer), with its own@since 5.10.
bson-kotlinx/src/test/kotlin/org/bson/codecs/kotlinx/KotlinSerializerCodecTest.kt
- 🟡 [important] The
@Contextual+ByteArrayAsBsonBinary.serializersModulepath advertised in the KDoc has no test. Add one (data class with@Contextual val byteArray: ByteArray, codec built with the module) — contextual resolution for primitive-array types has kotlinx-serialization edge cases worth pinning. Also missing: empty/nullableByteArrayand a negative test thatdeserializefails on a non-binary BSON value. - 🟡 [important] Comment wrapping in the two new tests is broken mid-clause ("(subType / 00),", "per-type-argument / annotation") far short of the 120-col limit, and much of the second comment narrates the change rather than stating a constraint. Suggest trimming to the one non-obvious point — the annotation inside
Array<@Serializable(...) ByteArray>applies per element — and reflowing the rest.
- Remove the duplicated array test from bson-kotlin's DataClassCodecTest - Document the ByteArrayAsBsonBinary.serializersModule opt-in - Cover the @contextual, empty, nullable and non-binary decode cases - Tidy the new test comments
|
@nhachicha updated based on feedback. Only issue not addressed because this was a regression is for |
JAVA-6224