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

Add support for @ConstructorProperties #905

Closed
yawkat opened this Issue Aug 24, 2015 · 21 comments

Comments

Projects
None yet
8 participants
@yawkat

yawkat commented Aug 24, 2015

ConstructorProperties provide a standard way to communicate constructor properties. Currently, you have to use JsonCreator with JsonProperty to achieve the same behavior with jackson.

Libraries like lombok generate constructors using the standard annotation, which makes it necessary to manually annotate the constructor when using jackson. There are already libraries that add support for this annotation but I believe it could be added to the main project without problems.

I believe ConstructorProperties is absent on android so it might be necessary to handle that gracefully.

@cowtowncoder

This comment has been minimized.

Show comment
Hide comment
@cowtowncoder

cowtowncoder Sep 10, 2015

Member

Sounds like a useful improvement, thank you for suggestion. I wasn't aware of existence of @ConstructorProperties, which is the reason this feature doesn't yet exist. But it should be possible to add for 2.7.

Member

cowtowncoder commented Sep 10, 2015

Sounds like a useful improvement, thank you for suggestion. I wasn't aware of existence of @ConstructorProperties, which is the reason this feature doesn't yet exist. But it should be possible to add for 2.7.

@Shredder121

This comment has been minimized.

Show comment
Hide comment
@Shredder121

Shredder121 Nov 2, 2015

Just wanted to let you know I am also looking forward to this feature.

Shredder121 commented Nov 2, 2015

Just wanted to let you know I am also looking forward to this feature.

@cowtowncoder cowtowncoder added this to the 2.7.0 milestone Nov 8, 2015

@cowtowncoder

This comment has been minimized.

Show comment
Hide comment
@cowtowncoder

cowtowncoder Nov 8, 2015

Member

Support added for 2.7.0.

Member

cowtowncoder commented Nov 8, 2015

Support added for 2.7.0.

@Shredder121

This comment has been minimized.

Show comment
Hide comment
@Shredder121

Shredder121 Nov 8, 2015

Thanks, I'll try it out at least! 👍

Shredder121 commented Nov 8, 2015

Thanks, I'll try it out at least! 👍

@cowtowncoder

This comment has been minimized.

Show comment
Hide comment
@cowtowncoder

cowtowncoder Nov 9, 2015

Member

Official 2.7.0 will take some time, but code is in master branch now.

Member

cowtowncoder commented Nov 9, 2015

Official 2.7.0 will take some time, but code is in master branch now.

@Athaphian

This comment has been minimized.

Show comment
Hide comment
@Athaphian

Athaphian Apr 1, 2016

It seems, (in 2.7.3) that the constructorProperties are not correctly parsed when using a custom namingstrategy... still in the progress of figuring out if this is actually the case, but sure looks like it.

Athaphian commented Apr 1, 2016

It seems, (in 2.7.3) that the constructorProperties are not correctly parsed when using a custom namingstrategy... still in the progress of figuring out if this is actually the case, but sure looks like it.

@cowtowncoder

This comment has been minimized.

Show comment
Hide comment
@cowtowncoder

cowtowncoder Apr 11, 2016

Member

@Athaphian there are two possible interpretations for meaning of @ConstructorProperties:

  1. Assume they are same as "implicit names"; that is, similar to finding names of getters or fields, and may be renamed with naming strategy
  2. Assume they are "explicit names", similar to using @JsonProperty: these are NOT subject to renaming

My assumption was that (2) is the proper usage, since this is for @JsonCreator with @JsonProperty works. However, it seems that in many cases (1) would be desired instead.
I am not quite sure how this conflict could be resolved, given that there is no way to add anything in @ConstructorProperties (for example to explicitly allow choosing mode).

Member

cowtowncoder commented Apr 11, 2016

@Athaphian there are two possible interpretations for meaning of @ConstructorProperties:

  1. Assume they are same as "implicit names"; that is, similar to finding names of getters or fields, and may be renamed with naming strategy
  2. Assume they are "explicit names", similar to using @JsonProperty: these are NOT subject to renaming

My assumption was that (2) is the proper usage, since this is for @JsonCreator with @JsonProperty works. However, it seems that in many cases (1) would be desired instead.
I am not quite sure how this conflict could be resolved, given that there is no way to add anything in @ConstructorProperties (for example to explicitly allow choosing mode).

@Athaphian

This comment has been minimized.

Show comment
Hide comment
@Athaphian

Athaphian Apr 12, 2016

@cowtowncoder Maybe it can be made configurable as a config option on the mapper? setConstructorPropertiesImplicit or something... and later on ignore the explicitly set constructor properties names? I have not looked at the Jackson code, so don't know if this approach is even possible.

Athaphian commented Apr 12, 2016

@cowtowncoder Maybe it can be made configurable as a config option on the mapper? setConstructorPropertiesImplicit or something... and later on ignore the explicitly set constructor properties names? I have not looked at the Jackson code, so don't know if this approach is even possible.

@cowtowncoder

This comment has been minimized.

Show comment
Hide comment
@cowtowncoder

cowtowncoder Apr 12, 2016

Member

@Athaphian right, I am thinking along same lines. Could you file a new issue for such a MapperFeature (has to be there as it can not be enabled on per-call basis).

Fwtw, there is already MapperFeature.ALLOW_EXPLICIT_PROPERTY_RENAMING (added in 2.7), which does allow addressing this, but it has effect on all annotations, so not side-effect-free.

Member

cowtowncoder commented Apr 12, 2016

@Athaphian right, I am thinking along same lines. Could you file a new issue for such a MapperFeature (has to be there as it can not be enabled on per-call basis).

Fwtw, there is already MapperFeature.ALLOW_EXPLICIT_PROPERTY_RENAMING (added in 2.7), which does allow addressing this, but it has effect on all annotations, so not side-effect-free.

@cowtowncoder

This comment has been minimized.

Show comment
Hide comment
@cowtowncoder

cowtowncoder Apr 20, 2016

Member

Actually, I think that I will make the change for 2.7.4, since this would solve #1122. The only (?) open question would be whether to:

  1. Make @ConstructorProperties indicate similarity to @JsonCreator in indicating that annotated constructor is to be used as Creator or not; either by default, or via configuration, and
  2. Allow use of names as explicit names, not implicit

I think I'm ok without (2), but (1) is an open question.

Member

cowtowncoder commented Apr 20, 2016

Actually, I think that I will make the change for 2.7.4, since this would solve #1122. The only (?) open question would be whether to:

  1. Make @ConstructorProperties indicate similarity to @JsonCreator in indicating that annotated constructor is to be used as Creator or not; either by default, or via configuration, and
  2. Allow use of names as explicit names, not implicit

I think I'm ok without (2), but (1) is an open question.

@Shredder121

This comment has been minimized.

Show comment
Hide comment
@Shredder121

Shredder121 Apr 20, 2016

What would you do about multiple annotated constuctors then?

Shredder121 commented Apr 20, 2016

What would you do about multiple annotated constuctors then?

@cowtowncoder

This comment has been minimized.

Show comment
Hide comment
@cowtowncoder

cowtowncoder Apr 20, 2016

Member

@Shredder121 good question. At this point, it'd likely be "throw an exception to indicate ambiguity". One could also consider simple resolution, and perhaps configurable resolution handler. But simplest heuristic would probably be "use one with most parameters".

Member

cowtowncoder commented Apr 20, 2016

@Shredder121 good question. At this point, it'd likely be "throw an exception to indicate ambiguity". One could also consider simple resolution, and perhaps configurable resolution handler. But simplest heuristic would probably be "use one with most parameters".

@Shredder121

This comment has been minimized.

Show comment
Hide comment
@Shredder121

Shredder121 Apr 20, 2016

"use one with most parameters"

That's indeed one heuristic.

Maybe with ambiguity require @JsonCreator again?
Although it sounds like that would create more problems than it solves.

Shredder121 commented Apr 20, 2016

"use one with most parameters"

That's indeed one heuristic.

Maybe with ambiguity require @JsonCreator again?
Although it sounds like that would create more problems than it solves.

@cowtowncoder

This comment has been minimized.

Show comment
Hide comment
@cowtowncoder

cowtowncoder Apr 20, 2016

Member

This is a recurring concern, and aside from this annotation, also for full auto-detection.
In many other cases users can just take care not to annotate multiple ones, but with frameworks like Lombok and auto-values there isn't that control.

Member

cowtowncoder commented Apr 20, 2016

This is a recurring concern, and aside from this annotation, also for full auto-detection.
In many other cases users can just take care not to annotate multiple ones, but with frameworks like Lombok and auto-values there isn't that control.

@janzyka

This comment has been minimized.

Show comment
Hide comment
@janzyka

janzyka Sep 8, 2016

Yep, same problem here. Using lombok and Jackson suddenly started to use the all-arguments constructor even in the case not all fields are set in the JSON object. This results in the unset fields becoming null which overrides their default values which are set on object level default 👎

janzyka commented Sep 8, 2016

Yep, same problem here. Using lombok and Jackson suddenly started to use the all-arguments constructor even in the case not all fields are set in the JSON object. This results in the unset fields becoming null which overrides their default values which are set on object level default 👎

@oliverlockwood

This comment has been minimized.

Show comment
Hide comment
@oliverlockwood

oliverlockwood Nov 1, 2016

For anyone else who stumbles upon this - as a result of this change, on upgrading to Jackson 2.7, trying to deserialize a class which is

  • annotated with Lombok's @AllArgsConstructor annotation, and
  • has a constructor annotated with @JsonCreator

I started having errors along the following lines:

com.fasterxml.jackson.databind.JsonMappingException: Conflicting property-based creators: already had explicitly marked
[constructor for com.package.class, annotations: {interface java.beans.ConstructorProperties=@java.beans.ConstructorProperties(value=[all, my, instance, variables)}], encountered
[constructor for com.package.class, annotations: {interface com.fasterxml.jackson.annotation.JsonCreator=@com.fasterxml.jackson.annotation.JsonCreator(mode=DEFAULT)}]

This is because under the covers Lombok's constructors use the ConstructorProperties annotation by default. The problem can be fixed by setting the following value.
lombok.anyConstructor.suppressConstructorProperties = true.

Hope that's useful...

oliverlockwood commented Nov 1, 2016

For anyone else who stumbles upon this - as a result of this change, on upgrading to Jackson 2.7, trying to deserialize a class which is

  • annotated with Lombok's @AllArgsConstructor annotation, and
  • has a constructor annotated with @JsonCreator

I started having errors along the following lines:

com.fasterxml.jackson.databind.JsonMappingException: Conflicting property-based creators: already had explicitly marked
[constructor for com.package.class, annotations: {interface java.beans.ConstructorProperties=@java.beans.ConstructorProperties(value=[all, my, instance, variables)}], encountered
[constructor for com.package.class, annotations: {interface com.fasterxml.jackson.annotation.JsonCreator=@com.fasterxml.jackson.annotation.JsonCreator(mode=DEFAULT)}]

This is because under the covers Lombok's constructors use the ConstructorProperties annotation by default. The problem can be fixed by setting the following value.
lombok.anyConstructor.suppressConstructorProperties = true.

Hope that's useful...

@cowtowncoder

This comment has been minimized.

Show comment
Hide comment
@cowtowncoder

cowtowncoder Nov 1, 2016

Member

Thank you @oliverlockwood. Also: it is really important to use latest 2.7 version; currently 2.7.8. There have been many fixes, and at least one directly affects handling of creator properties for static factory methods.

Member

cowtowncoder commented Nov 1, 2016

Thank you @oliverlockwood. Also: it is really important to use latest 2.7 version; currently 2.7.8. There have been many fixes, and at least one directly affects handling of creator properties for static factory methods.

@ericxiong

This comment has been minimized.

Show comment
Hide comment
@ericxiong

ericxiong Dec 19, 2016

Any MapperFeature to disable using of ConstructorProperties ?
AUTO_DETECT_CREATORS seems not working. @cowtowncoder

ericxiong commented Dec 19, 2016

Any MapperFeature to disable using of ConstructorProperties ?
AUTO_DETECT_CREATORS seems not working. @cowtowncoder

@tsabirgaliev

This comment has been minimized.

Show comment
Hide comment
@tsabirgaliev

tsabirgaliev Jan 9, 2017

Unfortunately, this breaks the @JsonSetter feature:

class Pojo {
    @JsonRawValue
    String json;

    @ConstructorProperties({"json"})
    public Pojo(String json) {
        this.json = json;
    }

    public Pojo(){}

    public String getJson() {
        return json;
    }

    @JsonSetter("json")
    public void setJson(JsonNode node) {
        this.json = node.toString();
    }

    public void setJson(String json) {
        this.json = json;
    }
}

public class JsonRawValueDeserializationTest {
    ObjectMapper mapper = new ObjectMapper();

    @Test
    public void test() throws IOException {
        Pojo pojo = new Pojo("{\"foo\":18}");

        String output = mapper.writeValueAsString(pojo);
        assertEquals("{\"json\":{\"foo\":18}}", output);

        // The following throws Exception
        Pojo deserialized = mapper.readValue(output, Pojo.class);
        assertEquals("{\"foo\":18}", deserialized.getJson());
    }
}

tsabirgaliev commented Jan 9, 2017

Unfortunately, this breaks the @JsonSetter feature:

class Pojo {
    @JsonRawValue
    String json;

    @ConstructorProperties({"json"})
    public Pojo(String json) {
        this.json = json;
    }

    public Pojo(){}

    public String getJson() {
        return json;
    }

    @JsonSetter("json")
    public void setJson(JsonNode node) {
        this.json = node.toString();
    }

    public void setJson(String json) {
        this.json = json;
    }
}

public class JsonRawValueDeserializationTest {
    ObjectMapper mapper = new ObjectMapper();

    @Test
    public void test() throws IOException {
        Pojo pojo = new Pojo("{\"foo\":18}");

        String output = mapper.writeValueAsString(pojo);
        assertEquals("{\"json\":{\"foo\":18}}", output);

        // The following throws Exception
        Pojo deserialized = mapper.readValue(output, Pojo.class);
        assertEquals("{\"foo\":18}", deserialized.getJson());
    }
}
@cowtowncoder

This comment has been minimized.

Show comment
Hide comment
@cowtowncoder

cowtowncoder Jan 10, 2017

Member

@tsabirgaliev If there is a problem, please file a new issue, referencing back to this issue as a root cause. Due to length of discussion it is not easy to see how things fit together.
In addition to test (which is useful, thank you!), issue should state version affected.

Member

cowtowncoder commented Jan 10, 2017

@tsabirgaliev If there is a problem, please file a new issue, referencing back to this issue as a root cause. Due to length of discussion it is not easy to see how things fit together.
In addition to test (which is useful, thank you!), issue should state version affected.

@cowtowncoder

This comment has been minimized.

Show comment
Hide comment
@cowtowncoder

cowtowncoder Jan 16, 2017

Member

@ericxiong Yes, MapperFeature.INFER_CREATOR_FROM_CONSTRUCTOR_PROPERTIES -- auto-detection is somewhat orthogonal here.

Member

cowtowncoder commented Jan 16, 2017

@ericxiong Yes, MapperFeature.INFER_CREATOR_FROM_CONSTRUCTOR_PROPERTIES -- auto-detection is somewhat orthogonal here.

alek-sys added a commit to alek-sys/spring-functional-microframework that referenced this issue May 20, 2018

Remove Lombok from dependencies
This removal happened because of breaking changes in patch versions
(rzwitserloot/lombok#1563) and in the light of
potential removal of Lombok from Spring Boot dependencies
(spring-projects/spring-boot#12576).

Hello data class now implements all the attributes of data class
(constructor, getter, equals, hashCode) explicitely.

@ConstructorProperties annotation is required for Jackson to be able to
create an objects using constructor, see
FasterXML/jackson-databind#905
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment