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

There are cases where isRequired specifications are not properly merged. #668

Open
k163377 opened this issue Apr 28, 2023 · 3 comments
Open
Labels

Comments

@k163377
Copy link
Contributor

k163377 commented Apr 28, 2023

Jackson has a policy of merging annotations assigned to accessors, fields, or parameters into a single property.
ProjectMapK/jackson-module-kogera#101 (comment)

Therefore, the goal of this issue is to match the behavior of databind.

The following is an old explanation.


Describe the bug
For example, if a getter is given JsonProperty(required = true), the parameter nullability is ignored and overridden by required = true.
This appears to be an unexpected behavior, since getters are not involved in deserialization.

Also, since this override does not occur for setters, there is no consistency in behavior.

To Reproduce

import com.fasterxml.jackson.annotation.JsonProperty
import com.fasterxml.jackson.databind.BeanDescription
import com.fasterxml.jackson.databind.ObjectMapper
import com.fasterxml.jackson.module.kotlin.jacksonObjectMapper
import org.junit.Test

class PropRequiredTest {
    val mapper = jacksonObjectMapper()

    private inline fun <reified T> ObjectMapper.introspectDeser(): BeanDescription =
        deserializationConfig.introspect(deserializationConfig.constructType(T::class.java))

    private fun BeanDescription.isRequired(propertyName: String): Boolean =
        this.findProperties().find { it.name == propertyName }?.isRequired ?: false

    data class AffectByAccessor(
        @get:JsonProperty(required = true)
        var a: String?,
        @field:JsonProperty(required = true)
        var b: String?,
        @set:JsonProperty(required = true)
        var c: String?
    ) {
        @get:JsonProperty(required = true)
        var x: String? = null
        @field:JsonProperty(required = true)
        var y: String? = null
        @JvmField
        @field:JsonProperty(required = true)
        var z: String? = null
    }

    @Test
    fun affectByAccessorTestDeser() {
        val desc = mapper.introspectDeser<AffectByAccessor>()

        // The expected value of a ~ c is false, but the actual values are all true.
        val a = desc.isRequired("a")
        val b = desc.isRequired("b")
        val c = desc.isRequired("c")

        // x and y is false and z is true. These are the expected values.
        val x = desc.isRequired("x")
        val y = desc.isRequired("y")
        val z = desc.isRequired("z")
    }
}

Expected behavior
Annotations assigned to accessors should not affect the creator.

Versions
Kotlin: 1.5.32
Jackson-module-kotlin & Jackson-databind: 2.15.0

Additional context
If processing is performed without registering a KotlinModule, a ~ c and x ~ z will all be true.
Therefore, a possible policy is to match the KotlinModule to this result.

@k163377 k163377 added the bug label Apr 28, 2023
@k163377
Copy link
Contributor Author

k163377 commented Apr 28, 2023

@cowtowncoder
Can you please confirm this as I think it has something to do with databind as well?
Personally, I think there should be a strict distinction between serialization and deserialization contexts.

@cowtowncoder
Copy link
Member

@k163377 Yes and no: annotations are merged across accessors on purpose, to remove need to duplicate settings (i.e. can add @JsonProperty("name") on just getter, to affect both serialization and deserialization). But precedence does vary between ser/deser, so overrides should work as expected (getter overrides field for serialization, but setter would override getter, etc).

One problem, however, is that information from annotations is not merged as part of flattening: so all information from @JsonProperty comes from one specific annotation instance; it is not possible to override just one aspect. So required being part of @JsonProperty, settings comes from highest-precedence annotation, regardless of whether it explicitly defines it or not.

@k163377 k163377 changed the title The parameter's isRequired is overridden by the JsonProperty granted to accessor. There are cases where isRequired specifications are not properly merged. May 3, 2023
@k163377
Copy link
Contributor Author

k163377 commented May 3, 2023

@cowtowncoder
Thanks for the answer.
I have updated the description as it seems more correct to match the behavior of databind.

k163377 added a commit to ProjectMapK/jackson-module-kogera that referenced this issue May 6, 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

2 participants