The ObjectMapper.readValue extension methods need the type param to be bound to Any

[ragnese]

Describe the bug
One can violate the type system by deserializing null where it was not intended

To Reproduce

class Foo(val value: Int) // any class

val mapper = ObjectMapper().registerKotlinModule()

val deserialized = mapper.readValue<Foo>("null") // succeeds
println(deserialized) // null

Expected behavior
Deserialization should fail when the declared type parameter is not explicitly nullable

Versions
Kotlin: 1.4
Jackson-module-kotlin: 2.13
Jackson-databind: 2.13

Additional context
The fix is to add a type bound to the current methods and to add a new set of nullable methods:

inline fun <reified T: Any> ObjectMapper.readValue(content: String): T = readNullableValue(content)) ?:  throw SomeMappingException()

inline fun <reified T: Any> ObjectMapper.readNullableValue(content: String): T? = readValue(content, jacksonTypeRef<T>())

[dinomite]

I think this is the same as #399.

Adding that readNullableValue() seems like a good idea, would you make a PR?

[ragnese]

I'm working on the PR today.

One question is what exception should be thrown for the non-nullable versions when they deserialize a null?

[dinomite]

Hmm, I'm not sure what is thrown when the existing strictNullChecks is violated—throwing the same exception is probably the right approach, though you could also create a new exception in Exception.kt if there doesn't seem to be a good one that already exists.

[ragnese]

Okay. I'll look into what strictNullChecks throws. I was originally going to just throw a new DatabindException, but I wasn't sure how to implement it.

class DeserializedNullException() : DatabindException("Deserialized a null value") {
    override fun prependPath(referrer: Any?, fieldName: String?) {
        // TODO - I don't know what to do here.
    }

    override fun prependPath(referrer: Any?, index: Int) {
        // TODO - I don't know what to do here.
    }
}

Similarly, I didn't know how to throw a JsonMappingException because all of the non-deprecated ctors take a Closeable processor argument, which I won't have from just wrapping the readValue calls and checking for null.

[dinomite]

Hmm, that's a new exception in 2.13, so I'm not sure—it looks like perhaps implementations are supposed to add the information passed to those functions to the exception message?

The only usage in jackson-databind accumulates into a List and _buildMessage() adds it to the exception message through this call to getPathReference().

All that is to say, it looks like extending JsonMappingException would be best or perhaps using ValueInstantiationException is appropriate.

[cowtowncoder]

+1 for what @dinomite said.

One other quick note:

DatabindException is the "new" base type added to be used in 3.0: it will replace JsonMappingException there; with 2.x it is sort of redundant base type which CAN be used for receiving (that is, code may catch it), but CAN NOT be thrown (since it is abstract / incomplete)

So code in 2.x differs from 3.0 (master) here, unfortunately. The intent is, however, that the vast majority of code should not directly instantiate exceptions but either:

1. Use methods in base (de)serializer/handler implementations (when extending)
2. Use methods in context objects (DeserializationContext / SerializerProvider

which will properly attach necessary information, as well as have same (or, sometimes, similar) signature between 2.x and 3.x code bases.

I know this is a bit of hassle due to rather long development cycle for 3.0 (unintentional. due to 2.x lifetime exceeding initial expectations).

[revintec]

@dinomite Hi, I don't think adding a new extension method is the right approach, we should make the existing extension method work

1. the old code using the new jackson library would work without modification/recompilation
2. the new extension method is pointless, nullable/non-nullable is already baked in the language, we should not add user's mental burden

[k163377]

This issue is closed as a duplicate of #399.