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

Jackson skips isXXX properties with Int type #337

Closed
imanushin opened this issue May 11, 2020 · 9 comments
Closed

Jackson skips isXXX properties with Int type #337

imanushin opened this issue May 11, 2020 · 9 comments
Assignees
Labels
bug is-prefix Issues relating to is-prefixed fields

Comments

@imanushin
Copy link

Describe the bug
The Kotlin properties with signature isXXX: Int omits default values even with option JsonInclude.Include.ALWAYS

private data class ProblematicType(val id: Int, val default: Int)

To Reproduce

  1. Prepare mapper:
val mapper = jacksonObjectMapper().setSerializationInclusion(JsonInclude.Include.ALWAYS)
  1. Prepare classes:
private data class Correct(val id: Int, val default: Int)
private data class Problematic(val id: Int, val isDefault: Int)
  1. Serialize both of them via writeValueAsString:
mapper.writeValueAsString(...)
  1. Compare the results

Expected behavior
Both classes have the same result semantic, so jsons are like this:

  • {id: 1, default: 0}
  • {id: 1, isDefault: 0}

Actual result
isDefault is ignored when it has 0:

  • {id: 1, default: 0}
  • {id: 1} <--- 0 is ignored for Int type

Versions
Kotlin: 1.3.72
Jackson-module-kotlin: 2.11
Jackson-databind: 2.11

Additional context
This is issue of 2.11 only. I didn't see it with previous versions (2.10.1 - 2.10.4). Version downgrading helps.

Spring Boots also adds his dependencies. Probably, this is result of version conflict. Actual runtime dependencies are:

+--- com.fasterxml.jackson.core:jackson-databind:2.10.4 -> 2.11.0 (*)
+--- com.fasterxml.jackson.datatype:jackson-datatype-jdk8:2.10.4
|    +--- com.fasterxml.jackson.core:jackson-core:2.10.4 -> 2.11.0
|    \--- com.fasterxml.jackson.core:jackson-databind:2.10.4 -> 2.11.0 (*)
+--- com.fasterxml.jackson.datatype:jackson-datatype-jsr310:2.10.4
|    +--- com.fasterxml.jackson.core:jackson-annotations:2.10.4 -> 2.11.0
|    +--- com.fasterxml.jackson.core:jackson-core:2.10.4 -> 2.11.0
|    \--- com.fasterxml.jackson.core:jackson-databind:2.10.4 -> 2.11.0 (*)
@imanushin imanushin added the bug label May 11, 2020
@imanushin imanushin changed the title Jackson skips is* properties with Int type Jackson skips isXXX properties with Int type May 11, 2020
@viartemev viartemev self-assigned this May 13, 2020
@viartemev
Copy link
Member

@imanushin thanks for your contribution. Workaround for you case is:

private data class Problematic(val id: Int, @get:JsonProperty("isDefault") val isDefault: Int)

@cowtowncoder the problem related to #80. Based on Kotlin docs:

If the name of the property starts with is, a different name mapping rule is used: the name of the getter will be the same as the property name, and the name of the setter will be obtained by replacing is with set. For example, for a property isOpen, the getter will be called isOpen() and the setter will be called setOpen(). This rule applies for properties of any type, not just Boolean.

The current implementation doesn't take into account a case where a property has a non-boolean type. At this point, jackson-databind tries to fetch proper getter name, but BeanUtil.okNameForIsGetter can properly handle only cases where is applied only for boolean fields.
Could we add a method this check it to AnnotationIntrospector and let override it?

@cowtowncoder
Copy link
Member

Yeah to me using "is-getter" for non-boolean values seems kind of wrong, but I know Kotlin specifies that as legit use case.

I changed handling in 2.11 to be more configurable, but it is probably true that addition of JacksonAnnotationIntrospector.findRenameByField() would not fully work with non-boolean getter. Adding a MapperFeature would be reasonable, as BeanUtil can not really call AnnotationIntrospector (nor would I want it to be added). But caller would need to take that into accont.

But basically I would not object to having a setting to enable discovery of "other kinds" of is-getters.

@viartemev
Copy link
Member

viartemev commented May 15, 2020

@cowtowncoder adding this feature makes sense because for java it doesn't work also:

public class Github337Java {

    /* com.fasterxml.jackson.databind.exc.InvalidDefinitionException: 
         No serializer found for class com.fasterxml.jackson.module.kotlin.test.github.Correct and no properties discovered to create BeanSerializer 
        (to avoid exception, disable SerializationFeature.FAIL_ON_EMPTY_BEANS) */
    @Test
    public void serialization() throws JsonProcessingException {
        ObjectMapper mapper = new ObjectMapper();
        String value = mapper.writeValueAsString(new Correct());
        assertNotNull(value);
    }
}

class Correct {
    private final Integer def = 1;

    public Integer isDef() {
        return def;
    }
}

@cowtowncoder
Copy link
Member

Yes, but my understanding is that Bean specification would only consider boolean/Boolean valued is getters. But if you or anyone else have link(s) to different interpretation would be happy to be proven wrong?

But even if so, I guess I am not totally against detecting non-boolean-valued is-getters. Change could expose previously unseen properties, if changed for default handling so there's minor backwards compatibility concern.

@viartemev
Copy link
Member

Based on Java Beans specification (8.3 Design Patterns for Properties) is-getters are valid only for boolean properties.

Have you ever had a request for such a feature in Jackson for Java/Scala?
I don't think that this feature makes sense in Java/Scala (it's not covered by spec), it's only Kotlin-specific feature.

@cowtowncoder
Copy link
Member

@viartemev I do not remember having this request for Java or Scala. The reason I mention MapperFeature is simply as an easy integration point to let Kotlin module (or possibly user) change the setting.
However, I think it is a fair point that a global setting that would also affect non-Kotlin types could be confusing.

I am open to other styles of configuration and if refactoring in jackson-databind is needed that's fine too. Introspection of naming convention is too hard-coded at this point, being quite old cold from pre-1.0 days (added around 0.9.5 or so, when databind was created :) ).
So this might be just one aspect in need of improvements: other examples include (more) configurable "with"-methods for builders.

Unfortunately I haven't had time to dig deeper into this issue, so help would be appreciated.

@dinomite
Copy link
Member

dinomite commented Mar 6, 2021

@k163377
Copy link
Contributor

k163377 commented Mar 3, 2023

This issue will be fixed by #630 and therefore closed as a duplicate.

@k163377 k163377 closed this as completed Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug is-prefix Issues relating to is-prefixed fields
Projects
None yet
Development

No branches or pull requests

5 participants