Fix bson-kotlinx encodeNullableSerializableValue null handling
#1453
Conversation
Ensures that the deferredElement name is reset correctly. Test case ported to bson-kotlin JAVA-5524
| @@ -156,6 +156,8 @@ internal class DefaultBsonEncoder( | |||
| if (value != null || configuration.explicitNulls) { | |||
| encodeName(it) | |||
| super.encodeNullableSerializableValue(serializer, value) | |||
| } else { | |||
There was a problem hiding this comment.
It seems worth it to add tests for the corresponding else clause in the encodeSerializableValue method just above (I removed the clause and no tests fails, so it's not covered).
There was a problem hiding this comment.
I can't concoct a test that triggers this. That said it doesn't guarantee that there isn't a scenario where it could be triggered.
As we are skipping this value it makes sense to reset the defferred name if it exists.
However, I think I can clean up the usage.
There was a problem hiding this comment.
@jyemin I cleaned up the usage and encapsulated the handling of deferred element names.
| @@ -133,6 +135,20 @@ class DataClassCodecTest { | |||
| assertDecodesTo(withStoredNulls, dataClass) | |||
| } | |||
|
|
|||
| @Test | |||
| fun testDataClassWithListThatLastItemDefaultsToNull() { | |||
There was a problem hiding this comment.
This is just a copy from bson-kotlinx - to ensure the bson-kotlin library also works as expected.
| } | ||
| writer.writeNull() | ||
| } | ||
| override fun encodeNull() = writer.writeNull() |
There was a problem hiding this comment.
Not sure how this was missed in a PR - but encoding null will never require the deferred element name - as encode(Nullable)SerializableValue will be used
| @@ -206,7 +200,6 @@ internal class DefaultBsonEncoder( | |||
|
|
|||
| private fun encodeName(value: Any) { | |||
| writer.writeName(value.toString()) | |||
| deferredElementName = null | |||
There was a problem hiding this comment.
No longer required - as its cleaned up in the SerializableValue usages
| } | ||
| } | ||
| ?: super.encodeSerializableValue(serializer, value) | ||
| deferredElementHandler.with( |
There was a problem hiding this comment.
It's unclear to me why value is
| ?: super.encodeSerializableValue(serializer, value) | ||
| deferredElementHandler.with( | ||
| { | ||
| if (value != null || configuration.explicitNulls) { |
There was a problem hiding this comment.
Just a point of clarification:
It's unclear to me why value is ever null here. If it is, why isn't encodeNullableSerializableValue called instead?
There was a problem hiding this comment.
Yes, this surprised me also. The only test that triggers this is via the use of generics which aren't marked as nullable.
@Serializable data class Box<T>(val boxed: T)
@Serializable data class DataClassWithNullableGeneric(val box: Box<String?>)
Encoding: DataClassWithNullableGeneric(Box(null)) causes this to branch to trigger.
Its questionable that this code is reasonable / bug free. However, it appears to be marked as designed: https://youtrack.jetbrains.com/issue/KT-66206
I've added a comment to the code.
…godb#1453) Ensures that the deferredElement name is reset correctly. Test case ported to bson-kotlin JAVA-5524
…godb#1453) Ensures that the deferredElement name is reset correctly. Test case ported to bson-kotlin JAVA-5524
Ensures that the
deferredElementname is reset as it does inencodeSerializableValue.Test case ported to bson-kotlin
JAVA-5524