Skip to content

Conversation

@rozza
Copy link
Member

@rozza rozza commented Mar 2, 2023

Library adds Bson support for Kotlin data classes

Follows the spirit of the bson-record-codec library

JAVA-4872

Library adds Bson support for Kotlin data classes

Follows the spirit of the bson-record-codec library

JAVA-4872
@rozza rozza requested a review from jyemin March 2, 2023 10:33
@rozza rozza marked this pull request as ready for review March 2, 2023 10:33
@rozza rozza requested a review from katcharov March 3, 2023 09:00
@rozza rozza added the kotlin label Mar 6, 2023
import org.bson.codecs.pojo.annotations.BsonRepresentation
import org.bson.diagnostics.Loggers

internal data class DataClassCodec<T : Any>(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You chose to restrict the classes supported by the codec to kotlin data classes. You will have feature request about sealed class support (often used in kotlin & supported in kmongo). Enums are already supported with the java codec.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - that can definitely be added to the backlog. We support sealed classes in Scala.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added JAVA-4905

types: List<Type> = emptyList()
): DataClassCodec<R> {
validateAnnotations(kClass)
val primaryConstructor = kClass.primaryConstructor!!
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usually I prefer to write: kClass.primaryConstructor ?: error("no primary constructor for $kClass")
(avoid !!)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

parameter.findAnnotation<BsonProperty>()!!.value
} else {
requireNotNull(parameter.name)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more idiomatic to write

return if (parameter.hasAnnotation()) {
idFieldName
} else {
parameter.findAnnotation()?.value ?: requireNotNull(parameter.name)
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

new ExpressionCodecProvider(),
new Jep395RecordCodecProvider()));
new Jep395RecordCodecProvider(),
new KotlinDataClassCodecProvider()));
Copy link

@zigzago zigzago Mar 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an issue here for the migration of kmongo: this is not backward compatible - and it prevents to add easily other kotlin codecs for data classes. I had to define my own default settings in kmongo because of an issue with EnumCodec (see https://github.com/Litote/kmongo/blob/master/kmongo-shared/src/main/kotlin/org/litote/kmongo/util/KMongoUtil.kt#L110 )

I would be great to add a way to configure the list of default CodecProviders (ServiceLoader ?)

By the way, you forget to update the javadoc of #getDefaultCodecRegistry()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extending codec registries is supported, either via MongoClientSettings or using withCodecRegistry. As they are registries, they are order dependent so users typically do something like: fromRegistries(fromCodecs(myCustomCodec), DefaultCodecRegsitry)

Will update the javadoc.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thank you. The issue with kmongo is that I have a codec to add at the end of the list of default providers - but I can manage that by removing EnumCodecProvider & KotlinDataClassCodecProvider ;)

@rozza rozza requested a review from zigzago March 8, 2023 14:42
Copy link
Collaborator

@katcharov katcharov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

codec.encode(writer, value, EncoderContext.builder().build())
assertEquals(expected, document)
if (expected.contains("_id")) {
assertEquals("_id", document.firstKey)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious: why is it important for the first key to be _id?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a micro-optimization that drivers do, since the server needs the _id value right off, and due to BSON structure it has to look for keys sequentially.

new ExpressionCodecProvider(),
new Jep395RecordCodecProvider()));
new Jep395RecordCodecProvider(),
new KotlinDataClassCodecProvider()));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

codec.encode(writer, value, EncoderContext.builder().build())
assertEquals(expected, document)
if (expected.contains("_id")) {
assertEquals("_id", document.firstKey)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a micro-optimization that drivers do, since the server needs the _id value right off, and due to BSON structure it has to look for keys sequentially.

@rozza rozza closed this Mar 21, 2023
@rozza
Copy link
Member Author

rozza commented Mar 21, 2023

Manually rebased and merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants