Skip to content
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.

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

NonNull enum property has null value #373

Closed
rafalbednarczuk opened this issue Sep 20, 2020 · 15 comments
Closed

NonNull enum property has null value #373

rafalbednarczuk opened this issue Sep 20, 2020 · 15 comments
Assignees
Labels
Milestone

Comments

@rafalbednarczuk
Copy link

rafalbednarczuk commented Sep 20, 2020

Describe the bug
Deserializing "null" string as non-null enum gives null value.

To Reproduce

enum class SocialMediaPlatform {INSTAGRAM, FACEBOOK, YOUTUBE, TWITCH}
fun main() {
    val json = "null"
    val enum  : SocialMediaPlatform = ObjectMapper().readValue(json)
    println(enum == null)
}

this snippet prints true

Expected behavior
Snippet should throw exception

Versions
Kotlin: 1.3.72
Jackson-module-kotlin: 2.11.2
Jackson-databind: 2.11.0

@ragnese
Copy link

ragnese commented Sep 22, 2020

This happens with any type- not just enums. IIRC, Objectmapper::readValue always returns a "platform type" which can always be null. This is definitely unfortunate and I don't know what the most correct and least-suprising answer would be, but I suspect the best answer would be a split API where half the methods on ObjectMapper return T? and half return T and fail on "null"

@cowtowncoder
Copy link
Member

cowtowncoder commented Sep 22, 2020

If Kotlin types indicate nullability, this is something that Kotlin module has to provide support for: jackson-databind has no immediate visibility to (or often, concept of) aspects of Kotlin that extend beyond Java.
So this would (need to) be a new feature

Implementation-wise there are two parts to the problem:

  1. How to introspect additional requirement for type
  2. How to change handling to fail on null

It might be possible to implement both by overriding method

public JsonSetter.Value findSetterInfo(Annotated ann) { }

in [Jackson]AnnotationIntrospector implementation/subclass that Kotlin provides, so if and when (1) is known, it can by defauilt return Nulls.Fail for JsonSetter.Value.value().
I think this would actually achieve what is asked here, although obviously testing would be needed as root value / property value handling differs a bit (there may be gaps for root values, in particular)

Same approach could work for other types too, of course, not just Enums. It should work nicely with POJOs; some work might be needed for deserializers of JDK default types.

@rafalbednarczuk
Copy link
Author

fun main() {
    println(isNullable<String?>())
    println(isNullable<String>())

}

inline fun <reified T> isNullable(): Boolean = null is T

This is how to know if type is nullalble or not. Null check for non-nullable types can be implemented in ObjectMapper.readValue() functions in Extensions.kt file.
This will work, but I don't think that's the best solution.

@cowtowncoder
Copy link
Member

@rafalbednarczuk This is from source calls, but the challenge is how to introspect this for given type: starting point is likely just type-erased Class (or with TypeReference it'd be super-type information, java.lang.reflect.Type).

@rafalbednarczuk
Copy link
Author

@cowtowncoder Does this new feature require changes on jackson-core level or only jackson-module-kotlin level? TypeReference class is inside jackson-core. I think this feature is required only by Kotlin devs. Java devs are used to handling nullables by themselves.

@cowtowncoder
Copy link
Member

@rafalbednarczuk It really depends on exactly how it can be implemented, but ideally only in Kotlin module.
Core components can not really use kotlin-core in any form (nor features beyond JDK baseline); kotlin module can.

Ability to fail on nulls (or replace with "default" value) exists already and is usable from Java with @JsonSetter(Nulls.FAIL) added on property, or using config overrides. This was added 3 years ago in Jackson 2.9:

https://medium.com/@cowtowncoder/jackson-2-9-features-b2a19029e9ff

but the question here is that of trying to automatically apply it for non-nullable Kotlin types, so that users would not have to -- for example -- add annotations all over the place, or have to enumerate all enumerations to use config overrides.

@rafalbednarczuk
Copy link
Author

rafalbednarczuk commented Sep 24, 2020

@cowtowncoder What do you think of modifying readValue functions? Here is working example snippet. I can make pull request for all functions.

fun main() {
    val json = "null"
    val mapper = JsonMapper.builder().build()

    //not throwing error
    val nullableInt: Int? = mapper.readValue(json)
    //throws error
    val nonNullableInt: Int = mapper.readValue(json)
}

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

inline fun <reified T> T.throwIfNullableTypeIsNull(): T {
    if (null !is T && this == null) {
        throw Exception()
    }
    return this
}

@cowtowncoder
Copy link
Member

cowtowncoder commented Sep 24, 2020

@rafalbednarczuk Main problem is just non-scalability (and fragility of sub-classing in general) of this approach: ObjectMapper has many methods, but there is also ObjectReader (which can not really be easily sub-classed by modules), and some formats extend ObjectMapper (like XmlMapper, CsvMapper for 2.x, but in 3.0 all format types will do this).
That is why I would prefer finding other mechanism.

@rafalbednarczuk
Copy link
Author

@cowtowncoder Do you have any other solution? Do you plan to add this feature or should I compile my own fork?

@cowtowncoder
Copy link
Member

I have no plans to work on this myself, I am offering information that helps others to possibly implement it (and outline some challenges).
So if you want to work on this that would be great of course!

@rafalbednarczuk
Copy link
Author

Let me know where to put the code and I will work on it.

@cowtowncoder
Copy link
Member

Typically you would fork whatever you want to contribute fix/change for.

rafalbednarczuk added a commit to rafalbednarczuk/jackson-module-kotlin that referenced this issue Sep 25, 2020
dinomite pushed a commit that referenced this issue Jan 1, 2021
@dinomite
Copy link
Member

dinomite commented Jan 1, 2021

Opened PR 405 against 2.12 with @rafalbednarczuk's fix.

@dinomite dinomite self-assigned this Jan 1, 2021
dinomite added a commit that referenced this issue Jan 1, 2021
@dinomite dinomite added this to the 2.12.1 milestone Jan 2, 2021
@dinomite dinomite added the 2.12 label Jan 2, 2021
@davidmoshal
Copy link

Similar issue with Int:

val mapper = jacksonObjectMapper()

data class Car(val doors: Int)

val car: Car = mapper.readValue("{\"doors\":null }")
println(car)

Ideally this should ideally throw a MissingKotlinParameterException.
Instead it prints: Car(doors=0)

@dinomite dinomite modified the milestones: 2.12.1, 2.12.2 Jan 11, 2021
@dinomite dinomite modified the milestones: 2.12.2, 2.13.0 May 13, 2021
@k163377
Copy link
Contributor

k163377 commented Jul 8, 2023

This issue is closed as a duplicate of #399.

@k163377 k163377 closed this as completed Jul 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants