Skip to content

Conversation

@rozza
Copy link
Member

@rozza rozza commented Jun 22, 2023

rozza added 2 commits June 22, 2023 17:02
The kotlinx codec was picking up the double array as
it was serializable. However, it should be limited
to top level data, value or sealed kotlin classes.

JAVA-5035
@rozza rozza requested review from jyemin, katcharov and vbabanin June 22, 2023 16:06
return try {
create(kClass, kClass.serializer(), serializersModule, bsonConfiguration)
} catch (exception: SerializationException) {
return if (!(kClass.isData || kClass.isValue || kClass.isSealed)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Which of the assertions below test which of the tests here? It seems that at least one test is missing since there are three conditions here and two assertions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah that test ensures the two cases (double & CharSequence) that we knew were being picked up now return null.

All three types are already tested by the other tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like before this change this method was returning a codec for classes that were not data or value or sealed classes, and now it returns null for those. So I'm not clear how an assertion that it now returns null for all of these could already be in the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah - KotlinSerializerCodecTest has existing tests for data, value and sealed classes which we want to continue to support.

The shouldReturnNullForSerializableButNotValueClassOrSealedOrDataClassTypes adds a test to ensure other serializable types (that we know about) fail.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I'm dumb. Since it's a ! for all three then this is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Merged as is in 4.10.x branch

But added 28b9727 to the master branch to improve the readability of this check and the same for the DataclassCodec.

@jyemin jyemin self-requested a review June 22, 2023 16:50
@rozza rozza merged commit a7b9ce0 into mongodb:4.10.x Jun 23, 2023
@rozza rozza deleted the JAVA-5035 branch June 23, 2023 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants