Skip to content

Kotlinx codec provider limit to data, value or sealed classes. #1147

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 23, 2023
Merged

Kotlinx codec provider limit to data, value or sealed classes. #1147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4fca28f
Kotlin: Limit the KotlinSerializerCodec to Data, Value or Sealed Classes
rozza Jun 22, 2023
6f59811
Test: Update AggregatesTest to use List of Doubles
rozza Jun 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions bson-kotlinx/src/main/kotlin/org/bson/codecs/kotlinx/KotlinSerializerCodec.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,14 @@ private constructor(
serializersModule: SerializersModule = defaultSerializersModule,
bsonConfiguration: BsonConfiguration = BsonConfiguration()
): Codec<T>? {
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.

null
} else {
try {
create(kClass, kClass.serializer(), serializersModule, bsonConfiguration)
} catch (exception: SerializationException) {
null
}
}
}

Expand Down
6 changes: 6 additions & 0 deletions bson-kotlinx/src/test/kotlin/org/bson/codecs/kotlinx/KotlinSerializerCodecProviderTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@ class KotlinSerializerCodecProviderTest {
assertNull(codec)
}

@Test
fun shouldReturnNullForSerializableButNotValueClassOrSealedOrDataClassTypes() {
assertNull(KotlinSerializerCodecProvider().get(DoubleArray::class.java, Bson.DEFAULT_CODEC_REGISTRY))
assertNull(KotlinSerializerCodecProvider().get(CharSequence::class.java, Bson.DEFAULT_CODEC_REGISTRY))
}

@Test
fun shouldReturnKotlinSerializerCodecUsingDefaultRegistry() {
val codec = MongoClientSettings.getDefaultCodecRegistry().get(DataClassWithSimpleValues::class.java)
Expand Down
4 changes: 2 additions & 2 deletions driver-core/src/test/functional/com/mongodb/client/model/AggregatesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,11 +97,11 @@ public void shouldGroupWithQuantile(final BsonField quantileAccumulator,
private static Stream<Arguments> setWindowFieldWithQuantileSource() {
return Stream.of(
Arguments.of(null,
WindowOutputFields.percentile("result", "$num1", new double[]{0.1, 0.9}, QuantileMethod.approximate(),
WindowOutputFields.percentile("result", "$num1", asList(0.1, 0.9), QuantileMethod.approximate(),
documents(UNBOUNDED, UNBOUNDED)),
asList(asList(1.0, 3.0), asList(1.0, 3.0), asList(1.0, 3.0))),
Arguments.of("$partitionId",
WindowOutputFields.percentile("result", "$num1", new double[]{0.1, 0.9}, QuantileMethod.approximate(), null),
WindowOutputFields.percentile("result", "$num1", asList(0.1, 0.9), QuantileMethod.approximate(), null),
asList(asList(1.0, 2.0), asList(1.0, 2.0), asList(3.0, 3.0))),
Arguments.of(null,
WindowOutputFields.median("result", "$num1", QuantileMethod.approximate(), documents(UNBOUNDED, UNBOUNDED)),
Expand Down