Type Introspection: non-nullable field with default is reported as required

[pepperbob]

Describe the bug

Type-Introspection of a data class reports its property as "required" though it has a default value. The reason - at least in my case - is that KotlinAnnotationIntrospector#requiredAnnotationOrNullability favors "required by nullability" over whatever the JsonProperty annotation says.

To Reproduce

data class TestData(
    @JsonProperty(value = "prop", required = false, defaultValue = "250")
    val someproperty: Int = 250
)

@Test
fun expectSomePropertyToNotBeRequired() {
    val javaType = mapper.constructType(TestData::class.java)
    val beandescription = mapper.getSerializationConfig().introspect(javaType)

    val def = beandescription.findProperties().first()
    
    // fails as the field is reported as required
    assert(!def.metadata.required, { "Property is expected to not be required" })
}

Expected behavior

It would be nice if the introspection could consider the propertys' default-value; however I would expect to overrule its decisions by being explicit via the JsonProperty-Annotation.

Versions
Kotlin: 1.7.0
Jackson-module-kotlin: 2.13.4
Jackson-databind: 2.13.4.2

Additional context

Frameworks that are using Jacksons introspection on Kotlin classes will come to wrong conclusions (as seen in swagger.io which produces kind of wrong schemas). Something similiar has been reported in issue #397

[k163377]

@pepperbob
If you want to check the reqirements during deserialization (i.e. parameters), you need to refer to deserializationConfig.

- val beandescription = mapper.getSerializationConfig().introspect(javaType)
+ val beandescription = mapper.deserializationConfig.introspect(javaType)

Can you please confirm if the problem occurs again after using this?

[cowtowncoder]

@k163377 would that change the original issue wrt preference of introspected information?

It is also worth noting that calls to introspect() should be done when constructing deserializers, and not during actual deserialization... because it is a rather expensive call to make (that is, reflection based introspection of a full type is a heavy-weight operation and the result cannot change between calls anyway)

[pepperbob]

@k163377 Quickly tried this and introspection via deserializationConfig reports the property as not required - and it seems I could just define the required-state via the annotation; however, dropping the annotation and the default value will still report "not required" though this wouldn't work obviously.

KotlinAnnotationIntrospector#requiredAnnotationOrNullability applies still the same rules but the path deserializationConfig takes comes to different conclusions about "required by nullability" as the way serializationConfig does it (the former inspects constructor params, the latter the getter methods).

[pepperbob]

@k163377 Sorry, I have to correct myself as this is wrong:

... dropping the annotation and the default value will still report "not required" though this wouldn't work obviously.

In my example the Int is a primitive and unless DeserializationFeature.FAIL_ON_NULL_FOR_PRIMITIVES is enabled it will simply default to 0 🙈

So after some tests I think that deserializationConfig behaves correct/as expected! I still cannot overrule with JsonProperty and preference is still the same but as the reflection has access to default values it comes to the right conclusions.

As I understood this cannot work for serializationConfig as in this case getters/fields are introspected and default values do not apply here.

[k163377]

@cowtowncoder

would that change the original issue wrt preference of introspected information?

I thought this was a deliberate override of priorities.

The required value of the JsonProperty annotation is false by default.
On the other hand, even if a parameter has JsonProperty set, it cannot ignore nullability on Kotlin or the presence of a default value.
Therefore, if required = false is set by annotation, it needs to be overridden.
Considering that there are fewer occasions where required = false is explicitly stated, it would appear to be an unintended behavior not to override.

[k163377]

@pepperbob
Thanks for the verification.

As I understood this cannot work for serializationConfig as in this case getters/fields are introspected and default values do not apply here.

Does this mean that you want serializationConfig to also take into account the presence or absence of default values?

[cowtowncoder]

@k163377 Ah yes, the defaulting of required to false is problematic. I wish we had OptBoolean enum in use back then (in which case default would be "undefined") since as things are it is impossible to tell explicit false from default.

[pepperbob]

@k163377 Considering the subtle differences between serialization/de-serialization everything works as expected and it's not a bug after all. Thank you for your time bringing this to my attention - #397 already had the answer somewhere but I didn't undertstand.

So as a conclusion:

• in the context of serialization non-nullable fields are required (no matter their default) meaning "a guarantee" that a value will be present
• at the same time in context of de-serialization non-nullable fields may have a default value and therefore are not required "to be provided"

So "required" has this further dimension that needs to be considered when building things upon it.