Added kotlinx.json JsonElement serialization support #1459
Conversation
JAVA-5239 --------- Co-authored-by: Mark <[email protected]>
bson-kotlinx/build.gradle.kts
Outdated
| @@ -38,19 +38,27 @@ description = "Bson Kotlinx Codecs" | |||
|
|
|||
| ext.set("pomName", "Bson Kotlinx") | |||
|
|
|||
| java { | |||
| registerFeature("jsonSupport") { | |||
There was a problem hiding this comment.
Required to make the dependency optional - see: JAVA-5540
| @@ -45,34 +50,93 @@ import org.bson.types.ObjectId | |||
| * | |||
| * For custom serialization handlers | |||
| */ | |||
| public sealed interface BsonDecoder { | |||
| @ExperimentalSerializationApi | |||
| internal sealed interface BsonDecoder : Decoder, CompositeDecoder { | |||
There was a problem hiding this comment.
As this is sealed there is no need for it to have been public - as all implementations are private. So its effectively private.
| @@ -37,31 +38,57 @@ import org.bson.types.ObjectId | |||
| * | |||
| * For custom serialization handlers | |||
| */ | |||
| public sealed interface BsonEncoder { | |||
| @ExperimentalSerializationApi | |||
| internal sealed interface BsonEncoder : Encoder, CompositeEncoder { | |||
There was a problem hiding this comment.
Same as the BsonDecoder interface - As this is sealed there is no need for it to have been public - as all implementations are private. So its effectively private.
| import org.bson.internal.UuidHelper | ||
|
|
||
| @OptIn(ExperimentalSerializationApi::class) | ||
| internal interface JsonBsonDecoder : BsonDecoder, JsonDecoder { |
There was a problem hiding this comment.
Provides Json decoding support
| import org.bson.types.Decimal128 | ||
|
|
||
| @OptIn(ExperimentalSerializationApi::class) | ||
| internal class JsonBsonEncoder( |
There was a problem hiding this comment.
Provides Json encoding support
| @@ -172,13 +172,13 @@ private constructor( | |||
| } | |||
|
|
|||
| override fun encode(writer: BsonWriter, value: T, encoderContext: EncoderContext) { | |||
| serializer.serialize(DefaultBsonEncoder(writer, serializersModule, bsonConfiguration), value) | |||
| serializer.serialize(BsonEncoder.createBsonEncoder(writer, serializersModule, bsonConfiguration), value) | |||
There was a problem hiding this comment.
Use the factory to check which encoder / decoder to load.
| } | ||
|
|
||
| @Test | ||
| fun testDataClassWithNestedJsonElements() { |
There was a problem hiding this comment.
Added a test to ensure complex nested types also support json elements.
gradle/publish.gradle
Outdated
| @@ -100,6 +100,7 @@ configure(javaProjects) { project -> | |||
| artifact sourcesJar | |||
| artifact javadocJar | |||
|
|
|||
| suppressAllPomMetadataWarnings() | |||
There was a problem hiding this comment.
A side effect of using gradle features. We dont need to publish the metadata in the pom at this time - so I've disabled the warnings.
There was a problem hiding this comment.
[Optional] We could suppress warnings for specific variants using suppressPomMetadataWarningsFor(variant) instead of all at once to ensure that we don't overlook any warnings unrelated to this change in the future.
gradle/publish.gradle
Outdated
| @@ -100,6 +100,7 @@ configure(javaProjects) { project -> | |||
| artifact sourcesJar | |||
| artifact javadocJar | |||
|
|
|||
| suppressAllPomMetadataWarnings() | |||
There was a problem hiding this comment.
[Optional] We could suppress warnings for specific variants using suppressPomMetadataWarningsFor(variant) instead of all at once to ensure that we don't overlook any warnings unrelated to this change in the future.
There was a problem hiding this comment.
It appears that the current tests may not be covering the JsonBsonEncoder, as the coverage analysis indicates that the class has not been accessed. Are these tests intended to evaluate the JsonBsonEncoder? If not, could we consider adding tests to ensure this functionality is covered by tests?
There was a problem hiding this comment.
Thanks @vbabanin turns out that wasn't really being used at all, rather it was left to some other implementation of the JsonEncoder. I overrode encodeSerializableValue to handle JsonElements and added extra tests for coverage.
I also tested to make sure the encoding configuration was honored.
Types from Json are now converted to the lowest Bson size for their type instead of all being Doubles. Also, improve test coverage
| put("string", JsonPrimitive("the fox ...")) | ||
| put("timestamp", JsonPrimitive(1311768464867721221)) | ||
| }) | ||
| assertDecodesTo("""{"value": $jsonAllSupportedTypesDocument}""", dataClassWithAllSupportedJsonTypes) |
There was a problem hiding this comment.
It seems we are missing a test case for encoding jsonAllSupportedTypesDocument, which includes a more comprehensive nested structure within JsonElement.
There was a problem hiding this comment.
Done - it is not roundtrippable so encodes and decodes can be different.
JAVA-5239