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

2.15.0 does not seem to honor PropertyAccessor.IS_GETTER visibility anymore #669

Open
fleiber opened this issue May 1, 2023 · 7 comments
Labels

Comments

@fleiber
Copy link

fleiber commented May 1, 2023

Here is a small main reproducing the behaviour change:

import com.fasterxml.jackson.annotation.JsonAutoDetect
import com.fasterxml.jackson.annotation.PropertyAccessor
import com.fasterxml.jackson.databind.ObjectMapper
import com.fasterxml.jackson.module.kotlin.KotlinModule


fun main() {
    val mapper = ObjectMapper().apply {
        registerModule(KotlinModule.Builder().build())
        setVisibility(PropertyAccessor.IS_GETTER, JsonAutoDetect.Visibility.NONE)
    }

    class Foo(val bar: Int) {
        val isZero get() = bar == 0
    }

    println(mapper.writeValueAsString(Foo(1)))
    // with Jackson 2.14.2, prints: {"bar":1}
    // with Jackson 2.15.0, prints: {"bar":1,"isZero":false}
}

We serialize these classes with setVisibility(PropertyAccessor.IS_GETTER, Visibility.NONE) to ignore the isXXX methods, and we cannot use @JsonIgnore ("This annotation is not applicable to target 'member property without backing field or delegate'").

Starting with Jackson 2.15.0, the isZero property is serialized, then the deserialization fails. Please also note that the issue only appears when we register KotlinModule.

The way we use the visibility feature looks fine to me, the property was correctly ignored at serialization with version 2.14.2, so I would say the 2.15 change does look like a regression.

@k163377
Copy link
Contributor

k163377 commented May 3, 2023

@cowtowncoder
I checked the POJOPropertiesCollector and it seems that if AI.findImplicitPropertyName returns a value, it is all checked for visibility as RegularGetter.
https://github.com/FasterXML/jackson-databind/blob/1def849c8b3ae868e4120c42f0f38c3c4b11c359/src/main/java/com/fasterxml/jackson/databind/introspect/POJOPropertiesCollector.java#L819

Is this expected behavior?


I will see if this can be accomplished by customizing the AccessorNamingStrategy.

@cowtowncoder
Copy link
Member

@cowtowncoder I checked the POJOPropertiesCollector and it seems that if AI.findImplicitPropertyName returns a value, it is all checked for visibility as RegularGetter. https://github.com/FasterXML/jackson-databind/blob/1def849c8b3ae868e4120c42f0f38c3c4b11c359/src/main/java/com/fasterxml/jackson/databind/introspect/POJOPropertiesCollector.java#L819

Is this expected behavior?

Hmmmh. I had to read that one couple of times through... I think the problem is that "implicit name" returned by AI does not denote whether it might be is-getter or regular getter. And then just proceeds to check for regular-getter rules.
Only naming convention gives is "is-getter" attribute.

It is bit of a short-coming, or oversight. I think that given how things are, this probably needs to be considered working as designed. It is not optimal but I am not sure it can be easily changed.

@k163377
Copy link
Contributor

k163377 commented May 4, 2023

@cowtowncoder
Thanks for the confirmation.

I think the problem is that "implicit name" returned by AI does not denote whether it might be is-getter or regular getter.

It is true that implName alone is not enough to determine if it is is-getter or not.

It is not optimal but I am not sure it can be easily changed.

Agreed.

By the way, is there a way to customize AccessorNamingStrategy from Module?
I looked for a while but could not find it.

I think that creating an AccessorNamingStrategy that wraps the default and replaces the findImplicitPropertyName would address this issue.

@cowtowncoder
Copy link
Member

Looking into where AccessorNamingStrategy makes sense: I think a challenge there is just that of how to allow multiple strategies, so that Kotlin module would only affect handling of Kotlin classes (and Scala for Scala classes) etc.
I don't remember if there is explicit support for Modules to set it, but technically Modules can call any method in ObjectMapper in setup method.

FWTW I think I was thinking of this extension point to help with Kotlin is-getters, but did not follow up on the idea.

@k163377
Copy link
Contributor

k163377 commented May 5, 2023

I have done some research and it seems to me that, assuming the current implementation, it would be quite difficult to solve this problem.
Below is a summary of the implementation methods and situations I have considered.

Customize AccessorNamingStrategy

This method is ideal because the scope of influence is limited to POJOPropertiesCollector::_addGetterMethod.
However, the following problems exist with customization

  • There is no set method in Module.SetupContext (it is necessary to assume that the content obtained by getOwner is an ObjectMapper)
  • Even if there is a set method, it always overwrites the existing settings because it cannot refer to them.
    • Also, it is not possible to set individual values for serialization and deserialization.

https://javadoc.io/static/com.fasterxml.jackson.core/jackson-databind/2.15.0/com/fasterxml/jackson/databind/Module.SetupContext.html

Customize PropertyNamingStrategy

As for PropertyNamingStrategy, it can be customized from Module.SetupContext.
It seems to be easier than AccessorNamingStrategy to coexist with existing settings.

However, there is a big concern because the scope of influence of this customization is different from that of findImplicitPropertyName and AccessorNamingStrategy.

Change from findImplicitPropertyName to findNameFor...

It cannot solve this problem because it is always treated as visible.

@k163377
Copy link
Contributor

k163377 commented May 5, 2023

In the meantime, I will also share my own workaround idea.
Please note that I have not studied this method in depth and have not confirmed that it works.

class IgnoreIsGetterAnnotationIntrospector : NopAnnotationIntrospector() {
    override fun hasIgnoreMarker(m: AnnotatedMember): Boolean = (m as? AnnotatedMethod)
        ?.let { _ -> m.name.startsWith("is") }
        ?: false
}

fun createMapper(): ObjectMapper {
    val module = object : SimpleModule() {
        override fun setupModule(context: SetupContext) {
            super.setupModule(context)
            context.appendAnnotationIntrospector(IgnoreIsGetterAnnotationIntrospector())
        }
    }

    return jacksonObjectMapper().registerModule(module)
}

@cowtowncoder
Copy link
Member

Thank you for the analysis @k163377, makes sense. Unfortunately there is no obvious easy solution.

Workaround might be useful for users in the meantime.

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

3 participants