FasterXML / jackson-databind

General data-binding package for Jackson (2.x): works on streaming API (core) implementation(s)
Apache License 2.0
3.52k stars 1.38k forks source link

Problem with multi-argument Creator with `@JsonBackReference` property #1516

Open sarahlikesglitter opened 7 years ago

sarahlikesglitter commented 7 years ago

For the error, please refer to this github example for a consistent repro: https://github.com/atribe/ReproducingBug

The short summary of the issue is that Jackson 2.7.+ - 2.8.6 is not correctly setting up the setter methods for properties defined in my class that I want to deserialize when those classes have managed/back references. Also, another issue is that @JsonProperty and @JsonSetter annotation is ignored, so there isn't a way to explicitly provide your own setter methods for the properties.

Below is the trace of debugging I have done on this issue.

I have a simple JSON file that shows a ParentObject and a ChildObject. The ParentObject has a managed reference that is a list of its children. The ChildObject has a back reference to its parent object.

In Jackson 2.6.7, when I run the deserializer for these objects, it will properly handle the references and generating mutators for those properties that have dependencies.

In Jackson 2.7.0 - 2.8.6, when I run the deserializer on the same code, it will fail with the error below if I use the managed/back reference annotation. When that managed/back annotation is removed, it will run just fine.

com.fasterxml.jackson.databind.JsonMappingException: Invalid definition for property "" (of type Lcom/atribe/reproducingbug/ChildObject;): No non-constructor mutator available
 at [Source: {
  "companyName": "My Famke Company",
  "companyLogoImageId": "29a8045e-3d10-4121-9f27-429aa74d00ad",
  "productId": "ABC-0003",
  "productName": "Engineering Test",
  "recordNumber": "01",
  "revisionNumber": "1.0",
  "procedureId": "6e6f607e-fb3f-4750-8a0a-2b38220e3328",
  "childSet": [
        {
            "title": "Child 1",
            "componentId": "3f7debe1-cddc-4b66-b7a7-49249e0c9d3e",
            "orderLabel": "1",
            "orderNumber": 1
        }
  ]
}; line: 1, column: 1]
    at com.fasterxml.jackson.databind.JsonMappingException.from(JsonMappingException.java:270)
    at com.fasterxml.jackson.databind.DeserializationContext.mappingException(DeserializationContext.java:1329)
    at com.fasterxml.jackson.databind.DeserializationContext.reportBadPropertyDefinition(DeserializationContext.java:1293)
    at com.fasterxml.jackson.databind.deser.BeanDeserializerFactory.constructSettableProperty(BeanDeserializerFactory.java:726)
    at com.fasterxml.jackson.databind.deser.BeanDeserializerFactory.addReferenceProperties(BeanDeserializerFactory.java:642)
    at com.fasterxml.jackson.databind.deser.BeanDeserializerFactory.buildBeanDeserializer(BeanDeserializerFactory.java:230)
    at com.fasterxml.jackson.databind.deser.BeanDeserializerFactory.createBeanDeserializer(BeanDeserializerFactory.java:141)
    at com.fasterxml.jackson.databind.deser.DeserializerCache._createDeserializer2(DeserializerCache.java:403)
    at com.fasterxml.jackson.databind.deser.DeserializerCache._createDeserializer(DeserializerCache.java:349)
    at com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCache2(DeserializerCache.java:264)
    at com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCacheValueDeserializer(DeserializerCache.java:244)
    at com.fasterxml.jackson.databind.deser.DeserializerCache.findValueDeserializer(DeserializerCache.java:142)
    at com.fasterxml.jackson.databind.DeserializationContext.findContextualValueDeserializer(DeserializationContext.java:443)
    at com.fasterxml.jackson.databind.deser.std.CollectionDeserializer.createContextual(CollectionDeserializer.java:206)
    at com.fasterxml.jackson.databind.deser.std.CollectionDeserializer.createContextual(CollectionDeserializer.java:26)
    at com.fasterxml.jackson.databind.DeserializationContext.handleSecondaryContextualization(DeserializationContext.java:681)
    at com.fasterxml.jackson.databind.DeserializationContext.findContextualValueDeserializer(DeserializationContext.java:445)
    at com.fasterxml.jackson.databind.deser.std.StdDeserializer.findDeserializer(StdDeserializer.java:964)
    at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.resolve(BeanDeserializerBase.java:501)
    at com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCache2(DeserializerCache.java:293)
    at com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCacheValueDeserializer(DeserializerCache.java:244)
    at com.fasterxml.jackson.databind.deser.DeserializerCache.findValueDeserializer(DeserializerCache.java:142)
    at com.fasterxml.jackson.databind.DeserializationContext.findRootValueDeserializer(DeserializationContext.java:476)
    at com.fasterxml.jackson.databind.ObjectReader._findRootDeserializer(ObjectReader.java:1859)
    at com.fasterxml.jackson.databind.ObjectReader._bindAndClose(ObjectReader.java:1621)
    at com.fasterxml.jackson.databind.ObjectReader.readValue(ObjectReader.java:1220)
    at com.atribe.reproducingbug.ParentObject.deserialize(ParentObject.java:61)
    at com.atribe.reproducingbug.ParentObjectTest.deserializeTest(ParentObjectTest.java:19)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:271)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:70)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:309)
    at org.junit.runner.JUnitCore.run(JUnitCore.java:160)
    at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
    at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:51)
    at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:237)
    at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at com.intellij.rt.execution.application.AppMain.main(AppMain.java:147)
Process finished with exit code -1

From the stacktrace, I found that when it sets up the managed/back references there is this code below in BeanDeserializerFactory.java. The line of interest is when it calls construct in the SimpleBeanPropertyDefinition

protected void addReferenceProperties(DeserializationContext ctxt,
            BeanDescription beanDesc, BeanDeserializerBuilder builder)
        throws JsonMappingException
    {
        // and then back references, not necessarily found as regular properties
        Map<String,AnnotatedMember> refs = beanDesc.findBackReferenceProperties();
        if (refs != null) {
            for (Map.Entry<String, AnnotatedMember> en : refs.entrySet()) {
                String name = en.getKey();
                AnnotatedMember m = en.getValue();
                JavaType type;
                if (m instanceof AnnotatedMethod) {
                    type = ((AnnotatedMethod) m).getParameterType(0);
                } else {
                    type = m.getType();
                }
                SimpleBeanPropertyDefinition propDef = SimpleBeanPropertyDefinition.construct(
                        ctxt.getConfig(), m);
                builder.addBackReferenceProperty(name, constructSettableProperty(ctxt,
                        beanDesc, propDef, type));
            }
        }
    }

The method call has this code from SimpleBeanPropertyDefinition.java, where it calls member.getName():

public static SimpleBeanPropertyDefinition construct(MapperConfig<?> config,
            AnnotatedMember member) {
        return new SimpleBeanPropertyDefinition(member, PropertyName.construct(member.getName()),
                (config == null) ? null : config.getAnnotationIntrospector(),
                        null, EMPTY_INCLUDE);
    }

member.getName() returns an empty string every single time (that is the current implementation and has been that way for years). Now after this member is instantiated, there is this call where the AnnotatedMember mutator variable is not correctly created:

protected SettableBeanProperty constructSettableProperty(DeserializationContext ctxt,
            BeanDescription beanDesc, BeanPropertyDefinition propDef,
            JavaType propType0)
        throws JsonMappingException
    {
        // need to ensure method is callable (for non-public)
        AnnotatedMember mutator = propDef.getNonConstructorMutator();
        // 08-Sep-2016, tatu: issues like [databind#1342] suggest something fishy
        //   going on; add sanity checks to try to pin down actual problem...
        //   Possibly passing creator parameter?
        if (mutator == null) {
            ctxt.reportBadPropertyDefinition(beanDesc, propDef, "No non-constructor mutator available");
        }
.....omitted rest of method for brevity...

If you go to the method propDef.getNonConstructorMutator(), the implementation is below:

@Override
    public AnnotatedMember getNonConstructorMutator() {
        AnnotatedMember acc = getSetter();
        if (acc == null) {
            acc = getField();
        }
        return acc;
    }

Go to getSetter() and this is the implementation below. The _member variable is always false and returns null.

 @Override
    public AnnotatedMethod getSetter() {
        if ((_member instanceof AnnotatedMethod)
                && ((AnnotatedMethod) _member).getParameterCount() == 1) {
            return (AnnotatedMethod) _member;
        }
        return null;
    }

When we bubble back up to the original caller in constructSettableProperty, it will be null and throw the error even if you explicitly write out a setter method associated with your property that has a managed/back reference.

protected SettableBeanProperty constructSettableProperty(DeserializationContext ctxt,
            BeanDescription beanDesc, BeanPropertyDefinition propDef,
            JavaType propType0)
        throws JsonMappingException
    {
        // need to ensure method is callable (for non-public)
        AnnotatedMember mutator = propDef.getNonConstructorMutator();
        // 08-Sep-2016, tatu: issues like [databind#1342] suggest something fishy
        //   going on; add sanity checks to try to pin down actual problem...
        //   Possibly passing creator parameter?
        if (mutator == null) {
            ctxt.reportBadPropertyDefinition(beanDesc, propDef, "No non-constructor mutator available");
        }
...omitted rest of method for brevity...
cowtowncoder commented 7 years ago

Yes, sounds like a bug. Thank you for thorough investigation and notes.

I haven't had to look into this one but hope to address it.

sarahlikesglitter commented 7 years ago

Thanks for the reply, would greatly appreciate further investigation into this issue and a fix (if any).

wheredevel commented 7 years ago

So, is it a confirmed bug?

cowtowncoder commented 7 years ago

@wheredevel I have not had time to look further, but sounds like legit bug.

cowtowncoder commented 7 years ago

Alas test uses Lombok. This is problematic as I do not have (nor plan to) install Lombok, as it does not (unlike most other libraries) work using just basic build tools and IDE.

cowtowncoder commented 7 years ago

... that aside I do appreciate reproductions, which are greatly appreciated! :-D

cowtowncoder commented 7 years ago

Unfortunately I am unable to reproduce the issue at this point. When simplifying this to just use fields, default constructor, problem does not occur. I assume it would be reproducible when Lombok has generated constructors and setters, but I don't have Lombok set up nor plan to do that.

Another thing to know is that @JsonBackReference and @JsonManagedReference can not be passed as Creator (constructor) argument: they must be handled using field or setter.

So, what I would need at this point is reproduction with explicitly defined setters/getters/constructors, with appropriate annotations. I assume Lombok gives access to generated source code, or allows sources to be generated for troubleshooting? If so, just including data classes ParentObject and ChildObject would be enough

atribe commented 7 years ago

Ok, I'll look at that tomorrow and see if we can reproduce without lombok. Thanks.

wheredevel commented 7 years ago

@cowtowncoder Lombok works with any Java IDE. For example, in Eclipse: 1) download the jar 2) in eclipse.ini add the line -javaagent:<path to lombok jar>/lombok.jar 3) restart Eclipse

Or follow installation instructions.

One more thing is to include the jar in your build, so that javac would hook it in during your classes compilation (a.k.a. lomboking). For example, in Maven:

...
        <dependency>
            <groupId>org.projectlombok</groupId>
            <artifactId>lombok</artifactId>
            <version>1.16.16</version>
            <scope>provided</scope>
        </dependency>
...
cowtowncoder commented 7 years ago

@wheredevel I know it is possible, but I do not want to install jars for arbitrary frameworks in my classpath. This is why from this point on no Lombok dependencies are to be added for any of Jackson projects: if I did, then everyone everywhere would have to do same installation just to build Jackson.

atribe commented 7 years ago

I tested some different configurations and found that you were correct, the code in my repo works with Lombok removed. So I dug a bit more and found that it only fails when the child is annotated with @AllArgsConstructor. The parent can have it and it works just fine.

I've branched myh repo and changed it so it passes while using lombok. The link shows the specific annotation that is causing the problem.

https://github.com/atribe/ReproducingBug/blob/TestPassesWithLombok/src/main/java/com/atribe/reproducingbug/ChildObject.java

Seems like I probably need to dig a bit more and maybe file a bug with lombok. Thanks for the help.

cowtowncoder commented 7 years ago

@atribe Thank you for digging this up. Just one follow-up question: do you know what @AllArgsConstructor would mean in terms of constructor added? I am not sure if Lombok could be adding Jackson annotations -- would seem unlikely, but on the other hand constructor wouldn't be auto-detected otherwise. Unless you are also using jackson-module-parameter-names with Java 8... which could auto-detect constructor. I am guessing this may be happening. Note that @AllArgsConstructor itself would have no effect on Jackson: it wouldn't be recognized. But it would materialize constructor in bytecode, which Jackson would notice.

One other note: code sample is using back references (@JsonBackReference) which is probably relevant as well. In fact that should not be passed through constructor because it, well, typically can not: it has to point to parent object, which can only be constructed when child object has been instantiated.... so that should remain a separate property (set via setter or field). I think some earlier versions may have handled things differently by using reverse order.

cowtowncoder commented 7 years ago

@shongywong If you are also using Lombok, you probably will need to avoid @ AllArgsConstructor. If not, it is necessary to avoid adding @JsonBackReference annotated parameter in @JsonCreator annotated (or, with Java 8, auto-detected) constructor.

I will see if I can construct a non-Lombok test case with this information.

wheredevel commented 7 years ago

@shongywong If you are also using Lombok, you probably will need to avoid @ AllArgsConstructor. If not, it is necessary to avoid adding @JsonBackReference annotated parameter in @JsonCreator annotated (or, with Java 8, auto-detected) constructor.

@shongywong could you elaborate more on this, please? Because, I use both annotations, and got the error.

atribe commented 7 years ago

@cowtowncoder This is the decompiled ChildObject class with @Data, @AllArgsConstructor, and @NoArgsConstructor:

//
// Source code recreated from a .class file by IntelliJ IDEA
// (powered by Fernflower decompiler)
//

package com.atribe.reproducingbug;

import com.fasterxml.jackson.annotation.JsonBackReference;
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import java.beans.ConstructorProperties;
import java.util.UUID;

@JsonIgnoreProperties(
    ignoreUnknown = true
)
public class ChildObject {
    private UUID id;
    @JsonBackReference
    private ParentObject parentObject;
    private UUID componentId;
    private int orderNumber;
    private String orderLabel;
    private String title;

    public UUID getId() {
        return this.id;
    }

    public ParentObject getParentObject() {
        return this.parentObject;
    }

    public UUID getComponentId() {
        return this.componentId;
    }

    public int getOrderNumber() {
        return this.orderNumber;
    }

    public String getOrderLabel() {
        return this.orderLabel;
    }

    public String getTitle() {
        return this.title;
    }

    public void setId(UUID id) {
        this.id = id;
    }

    public void setParentObject(ParentObject parentObject) {
        this.parentObject = parentObject;
    }

    public void setComponentId(UUID componentId) {
        this.componentId = componentId;
    }

    public void setOrderNumber(int orderNumber) {
        this.orderNumber = orderNumber;
    }

    public void setOrderLabel(String orderLabel) {
        this.orderLabel = orderLabel;
    }

    public void setTitle(String title) {
        this.title = title;
    }

    public boolean equals(Object o) {
        if(o == this) {
            return true;
        } else if(!(o instanceof ChildObject)) {
            return false;
        } else {
            ChildObject other = (ChildObject)o;
            if(!other.canEqual(this)) {
                return false;
            } else {
                label75: {
                    Object this$id = this.getId();
                    Object other$id = other.getId();
                    if(this$id == null) {
                        if(other$id == null) {
                            break label75;
                        }
                    } else if(this$id.equals(other$id)) {
                        break label75;
                    }

                    return false;
                }

                Object this$parentObject = this.getParentObject();
                Object other$parentObject = other.getParentObject();
                if(this$parentObject == null) {
                    if(other$parentObject != null) {
                        return false;
                    }
                } else if(!this$parentObject.equals(other$parentObject)) {
                    return false;
                }

                Object this$componentId = this.getComponentId();
                Object other$componentId = other.getComponentId();
                if(this$componentId == null) {
                    if(other$componentId != null) {
                        return false;
                    }
                } else if(!this$componentId.equals(other$componentId)) {
                    return false;
                }

                if(this.getOrderNumber() != other.getOrderNumber()) {
                    return false;
                } else {
                    Object this$orderLabel = this.getOrderLabel();
                    Object other$orderLabel = other.getOrderLabel();
                    if(this$orderLabel == null) {
                        if(other$orderLabel != null) {
                            return false;
                        }
                    } else if(!this$orderLabel.equals(other$orderLabel)) {
                        return false;
                    }

                    Object this$title = this.getTitle();
                    Object other$title = other.getTitle();
                    if(this$title == null) {
                        if(other$title != null) {
                            return false;
                        }
                    } else if(!this$title.equals(other$title)) {
                        return false;
                    }

                    return true;
                }
            }
        }
    }

    protected boolean canEqual(Object other) {
        return other instanceof ChildObject;
    }

    public int hashCode() {
        int PRIME = true;
        int result = 1;
        Object $id = this.getId();
        int result = result * 59 + ($id == null?43:$id.hashCode());
        Object $parentObject = this.getParentObject();
        result = result * 59 + ($parentObject == null?43:$parentObject.hashCode());
        Object $componentId = this.getComponentId();
        result = result * 59 + ($componentId == null?43:$componentId.hashCode());
        result = result * 59 + this.getOrderNumber();
        Object $orderLabel = this.getOrderLabel();
        result = result * 59 + ($orderLabel == null?43:$orderLabel.hashCode());
        Object $title = this.getTitle();
        result = result * 59 + ($title == null?43:$title.hashCode());
        return result;
    }

    @ConstructorProperties({"id", "parentObject", "componentId", "orderNumber", "orderLabel", "title"})
    public ChildObject(UUID id, ParentObject parentObject, UUID componentId, int orderNumber, String orderLabel, String title) {
        this.id = id;
        this.parentObject = parentObject;
        this.componentId = componentId;
        this.orderNumber = orderNumber;
        this.orderLabel = orderLabel;
        this.title = title;
    }

    public ChildObject() {
    }

    public String toString() {
        return "ChildObject(id=" + this.getId() + ", componentId=" + this.getComponentId() + ", orderNumber=" + this.getOrderNumber() + ", orderLabel=" + this.getOrderLabel() + ", title=" + this.getTitle() + ")";
    }
}
cowtowncoder commented 7 years ago

@atribe Thank you! Without having tested this, I think the problem really isn't Lombok, but combination of just two things:

  1. Constructor as creator (not a problem in itself)
  2. One of constructor parametrs is @JsonBackReference (specified in field but does not matter)

I think this should finally allow me to create test to see how to address this problem. It is bit tricky due to how managed/back references are handled: parent may well not even exist at the point where constructor is to be called (this depends, too, it may exist if it does not use constructors) However there should be a better way; especially since here we even have an actual setter to use, if it turned out difficult

cowtowncoder commented 7 years ago

In the meantime: couple of work-arounds that do exist:

  1. Disable MapperFeature.INFER_CREATOR_FROM_CONSTRUCTOR_PROPERTIES -- that would prevent use of Lombok-generated full constructors for construction and things would just work
  2. Remove @AllArgsConstructor: that will not create constructor to use

either of which would work around the issue.

cowtowncoder commented 7 years ago

Ok I can now reproduce the issue as reported.

cowtowncoder commented 7 years ago

So: for 2.8(.8) I am only able to improve error messaging -- passing of @JsonBackReference through constructors is not supported. I will see if I can actually fix this for 2.9, however: doing so will require bigger internal changes which is why it can not be done for 2.8 (too risky).

joshwand commented 7 years ago

Data point: I think I managed to avoid this bug by adding suppressConstructorProperties=true to my lombok @AllArgsConstructor annotation.

cowtowncoder commented 7 years ago

@joshwand Thanks! That makes sense, that would also prevent auto-discovery of creators.

kraiss commented 7 years ago

@cowtowncoder @joshwand We have faced the same issue and we found another possible workaround.

If you are only using @AllArgsConstructor for internal use in the class (for @Builder as instance), you can define the constructor private like this "@AllArgsConstructor(access = AccessLevel.PRIVATE)"

HSerg commented 7 years ago

Yet another workaround: @AllArgsConstructor(onConstructor_ = {@JsonIgnore}). JDK8, Jackson 2.9.1, Lombok 1.16.18.

guentherwieser commented 6 years ago

As suppressConstructorProperties is deprecated in lombok, using lombok.anyConstructor.suppressConstructorProperties=true in a lombok.config file worked for our project (all the other proposals didn't work).

Full lombok.config in the root of our parent project:

config.stopBubbling=true
lombok.anyConstructor.suppressConstructorProperties=true
xak2000 commented 6 years ago

Last lombok version v1.16.20 (January 9th, 2018) introduced a breaking change:

BREAKING CHANGE: lombok config key lombok.anyConstructor.suppressConstructorProperties is now deprecated and defaults to true, that is, by default lombok no longer automatically generates @ConstructorProperties annotations. New config key lombok.anyConstructor.addConstructorProperties now exists; set it to true if you want the old behavior. Oracle more or less broke this annotation with the release of JDK9, necessitating this breaking change.

So, suppressConstructorProperties is not required anymore!

But for me it caused another problem: now Jackson cannot deserialize @Value classes, generated by lombok, because auto-generated constructor not have @ConstructorProperties nor @JsonCreator annotation.

So I have to add this lombok.anyConstructor.addConstructorProperties = true to the lombok.config file to restore previous behaviour.

OndraZizka commented 6 years ago

Is the MapperFeature.INFER_CREATOR_FROM_CONSTRUCTOR_PROPERTIES related to that?

cowtowncoder commented 6 years ago

Yes, that is related: disabling of that feature prevents use of @ConstructorProperties as alias for @JsonCreator; that is, a marker for constructor explicitly marked to be used.