Skip to content

Ensure bson-kotlin handles possible NPE #1395

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

Closed
wants to merge 1 commit into from
Closed

Ensure bson-kotlin handles possible NPE #1395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions bson-kotlin/src/main/kotlin/org/bson/codecs/kotlin/DataClassCodec.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,8 @@ internal data class DataClassCodec<T : Any>(
} else {
this.get(clazz, types)
}
codec ?: throw CodecConfigurationException("Can't find a codec for $clazz.")
Copy link
Member

Choose a reason for hiding this comment

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

I think we should not do this ad hoc check. Both of the CodecRegistry.get methods don't allow returning nulls, so when a custom CodecRegistry.get returns null, it violates its contract. There are three ways we can go about that:

  1. [the current approach in the driver, though I can't guarantee that it is used consistently] Do nothing, assuming nulls can't be returned, but also not asserting that values are non-null, because we use assertions only for driver bugs, and here a bug may be in the application code.
  2. [the approach proposed in this PR, but the PR applies it to a single place of many] Everywhere we call CodecRegistry.get, we check if the result is null, and do something with it. This approach is error-prone, and we should avoid it.
  3. [the approach I propose] Wrap all application-provided CodecRegistry objects in CheckedCodecRegistry, which checks the return value of the get methods for null, and either converts it into the CodecConfigurationException with accordance to the contract, or throws an NPE, informing an application that it violated the contract at runtime.
    3.1. If we were just now designing this API, I would have suggested the "throw NPE from CheckedCodecRegistry" approach. However, since we already have it implemented, and there is a chance we sometimes may do the ad hoc checks somewhere in the driver, I propose to use the "convert null into the CodecConfigurationException" approach.
    3.2 Following are all the places where a user may specify a custom CodecRegistry:
    • MongoClientSettings.Builder.codecRegistry
    • MongoCollection.withCodecRegistry
    • CodecRegistries.fromRegistries
      3.3 We can optimize wrapping by omitting it when the instance given to us is of the concrete type we own and know users may not extend: ChildCodecRegistry, ProvidersCodecRegistry, CheckedCodecRegistry.

Copy link
Member Author

Choose a reason for hiding this comment

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

The adhoc check is needed to inform the user if they are able to encode / decode the Kotlin DataClass and all its properties (fields). If it were to return null it means that there is no Codec available in the registry for the type.

Note: CodecRegistry.getCodec here is an extension function to CodecRegistry and is only for the use within the DataClassCodec. The call to this.get is a call to codecRegistry#get which uses the users configured CodecRegistry.


return kParameter.findAnnotation<BsonRepresentation>()?.let {
if (codec !is RepresentationConfigurable<*>) {
throw CodecConfigurationException(
Expand Down
17 changes: 17 additions & 0 deletions bson-kotlin/src/test/kotlin/org/bson/codecs/kotlin/DataClassCodecTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,12 @@ import kotlin.test.assertEquals
import org.bson.BsonDocument
import org.bson.BsonDocumentReader
import org.bson.BsonDocumentWriter
import org.bson.codecs.Codec
import org.bson.codecs.DecoderContext
import org.bson.codecs.EncoderContext
import org.bson.codecs.configuration.CodecConfigurationException
import org.bson.codecs.configuration.CodecRegistries.fromProviders
import org.bson.codecs.configuration.CodecRegistry
import org.bson.codecs.kotlin.samples.DataClassEmbedded
import org.bson.codecs.kotlin.samples.DataClassListOfDataClasses
import org.bson.codecs.kotlin.samples.DataClassListOfListOfDataClasses
Expand All @@ -42,6 +44,7 @@ import org.bson.codecs.kotlin.samples.DataClassWithBsonExtraElements
import org.bson.codecs.kotlin.samples.DataClassWithBsonId
import org.bson.codecs.kotlin.samples.DataClassWithBsonIgnore
import org.bson.codecs.kotlin.samples.DataClassWithBsonProperty
import org.bson.codecs.kotlin.samples.DataClassWithBsonRepresentation
import org.bson.codecs.kotlin.samples.DataClassWithCollections
import org.bson.codecs.kotlin.samples.DataClassWithDataClassMapKey
import org.bson.codecs.kotlin.samples.DataClassWithDefaults
Expand Down Expand Up @@ -449,6 +452,9 @@ class DataClassCodecTest {
fun testSupportedAnnotations() {
assertRoundTrips("""{"_id": "a"}""", DataClassWithBsonId("a"))
assertRoundTrips("""{"_id": "a"}""", DataClassWithBsonProperty("a"))
assertRoundTrips(
"""{"_id": "a", "altId": {"${'$'}oid": "111111111111111111111111"}}""",
DataClassWithBsonRepresentation("a", "111111111111111111111111"))
}

@Test
Expand All @@ -468,6 +474,13 @@ class DataClassCodecTest {
}
}

@Test
fun testEmptyCodecRegistry() {
assertThrows<CodecConfigurationException> {
DataClassCodec.create(DataClassWithBsonRepresentation::class, EmptyCodecRegistry())
}
}

private fun <T : Any> assertRoundTrips(expected: String, value: T) {
assertDecodesTo(assertEncodesTo(expected, value), value)
}
Expand Down Expand Up @@ -496,4 +509,8 @@ class DataClassCodecTest {
}

private fun registry() = fromProviders(DataClassCodecProvider(), Bson.DEFAULT_CODEC_REGISTRY)
private class EmptyCodecRegistry : CodecRegistry {
override fun <T : Any?> get(clazz: Class<T>?): Codec<T>? = null
override fun <T : Any?> get(clazz: Class<T>?, registry: CodecRegistry?): Codec<T>? = null
}
}
5 changes: 5 additions & 0 deletions bson-kotlin/src/test/kotlin/org/bson/codecs/kotlin/samples/DataClasses.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,11 @@ data class DataClassWithBsonConstructor(val id: String, val count: Int) {
@BsonCreator constructor(id: String) : this(id, -1)
}

data class DataClassWithBsonRepresentation(
@BsonId val id: String,
@BsonRepresentation(BsonType.OBJECT_ID) val altId: String
)

data class DataClassWithInvalidBsonRepresentation(@BsonRepresentation(BsonType.STRING) val id: BsonMaxKey)

data class DataClassWithFailingInit(val id: String) {
Expand Down