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

[BUG] @Jacksonized does not copy @JsonUnwrapped to @Builder #2736

Closed
mjustin opened this issue Feb 2, 2021 · 8 comments
Closed

[BUG] @Jacksonized does not copy @JsonUnwrapped to @Builder #2736

mjustin opened this issue Feb 2, 2021 · 8 comments
Milestone

Comments

@mjustin
Copy link

mjustin commented Feb 2, 2021

Describe the bug

The @Jacksonized annotation says it "[copies] Jackson-related configuration annotations [...] from the class to the builder class." However, it does not copy the @JsonUnwrapped annotation, which results in incorrect deserialization behavior for objects that require it.

To Reproduce

ExampleFile.java

import com.fasterxml.jackson.annotation.JsonUnwrapped;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.*;
import com.fasterxml.jackson.databind.json.JsonMapper;
import lombok.*;
import lombok.extern.jackson.Jacksonized;

public class ExampleFile {
    public static void main(String[] args) throws JsonProcessingException {
        ObjectMapper objectMapper = JsonMapper.builder().disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES).build();
        WithUnwrapped expected = new WithUnwrapped(new NestedValue("A", "B"));

        String json = "{\"value1\":\"A\",\"value2\":\"B\"}";
        WithUnwrapped deserializedObject = objectMapper.readValue(json, WithUnwrapped.class);
        if (!expected.equals(deserializedObject)) {
            throw new IllegalStateException("Wrong value; was: " + deserializedObject);
        }
    }

    @Data
    @NoArgsConstructor
    @AllArgsConstructor
    @Builder     // Correct result if these two annotations are commented out
    @Jacksonized // Correct result if these two annotations are commented out
    public static class WithUnwrapped {
        @JsonUnwrapped
        private NestedValue nested;
    }

    @Data
    @NoArgsConstructor
    @AllArgsConstructor
    public static class NestedValue {
        private String value1;

        private String value2;
    }
}

Reproduction Steps

$ curl -s https://repo1.maven.org/maven2/org/projectlombok/lombok/1.18.18/lombok-1.18.18.jar -O
$ curl -s https://repo1.maven.org/maven2/com/fasterxml/jackson/core/jackson-annotations/2.12.1/jackson-annotations-2.12.1.jar -O
$ curl -s https://repo1.maven.org/maven2/com/fasterxml/jackson/core/jackson-core/2.12.1/jackson-core-2.12.1.jar -O
$ curl -s https://repo1.maven.org/maven2/com/fasterxml/jackson/core/jackson-databind/2.12.1/jackson-databind-2.12.1.jar -O
$ javac -cp "*" ExampleFile.java
$ java -cp ".:*" ExampleFile
Exception in thread "main" java.lang.IllegalStateException: Wrong value; was: ExampleFile.WithUnwrapped(nested=null)
	at ExampleFile.main(ExampleFile.java:17)

Expected behavior

The @JsonUnwrapped annotation should be copied to the builder method. In the specific example above, this should lead to the JSON value successfully parsing.

Version info (please complete the following information):

  • Lombok 1.18.18
  • javac 11.0.9.1
@snystedt
Copy link

snystedt commented Apr 21, 2021

I have been facing the same issue with my use case and I took a look at the JacksonizedHandler.

It uses the array defined in HandlerUtils with the following members:

JACKSON_COPY_TO_BUILDER_ANNOTATIONS = Collections.unmodifiableList(Arrays.asList(new String[] {
			"com.fasterxml.jackson.annotation.JsonAutoDetect",
			"com.fasterxml.jackson.annotation.JsonFormat",
			"com.fasterxml.jackson.annotation.JsonIgnoreProperties",
			"com.fasterxml.jackson.annotation.JsonIgnoreType",
			"com.fasterxml.jackson.annotation.JsonPropertyOrder",
			"com.fasterxml.jackson.annotation.JsonRootName",
			"com.fasterxml.jackson.annotation.JsonSubTypes",
			"com.fasterxml.jackson.annotation.JsonTypeInfo",
			"com.fasterxml.jackson.annotation.JsonTypeName",
			"com.fasterxml.jackson.annotation.JsonView",
			"com.fasterxml.jackson.databind.annotation.JsonNaming",
		}));

Wouldn't the fix be as simple as adding "com.fasterxml.jackson.annotation.JsonUnwrapped" to this array?

@janrieke
Copy link
Contributor

Probably not, because that list is for the builder class, not the setter methods. However, there's another list for that: COPY_TO_SETTER_ANNOTATIONS

@snystedt
Copy link

snystedt commented Apr 21, 2021

I just realized the sample above is a bit different from our case.

Our case is something like this:

@Builder
@Jacksonized
public class Foo {
    @JsonUnwrapped
    private Bar bar;
}

If I manually add a builder and annotate the Bar-method as JsonUnwrapped it works as expected.

@Builder
@JsonDeserialize(builder = Foo.FooBuilder.class)
public class Foo {
    @JsonUnwrapped
    private Bar bar;

    @JsonPOJOBuilder(withPrefix = "")
    public static class FooBuilder {
        private Bar bar;

        FooBuilder() {}
       
        @JsonUnwrapped
        public FooBuilder bar(Bar bar) {
            this.bar = bar;
            return this;
        }
        
        public Foo build() {
            return new Foo(bar);
        }
    }
}

In this case the suggestion above should be valid, right?

@snystedt
Copy link

snystedt commented Apr 21, 2021

Or is it that you mean to say that the JACKSON_COPY_TO_BUILDER_ANNOTATIONS enumerates the annotations that would end up on the actual Builder class and the COPY_TO_SETTER_ANNOTATIONS enumerates annotations that would end up on the actual setter method of the class (in the above case the public FooBuilder bar(Bar bar) {...})?

@janrieke
Copy link
Contributor

Exactly.

@snystedt
Copy link

So, should I just make a branch and a PR?

@j-klemm
Copy link

j-klemm commented Sep 17, 2021

Any updates on this @snystedt @janrieke ?

@rzwitserloot
Copy link
Collaborator

@snystedt I don't use any of these tools so I have to go by what you've all figured out. I don't know what @JsonUnwrapped is meant to do. However, if indeed it is the case that it should always be copied straight to the builder's 'setter' method if the field is annotated with it, then yeah, it's as simple as adding it to the list for the COPY_TO_SETTER_ANNOTATIONS file. If you're a new contributor, also add yourself to the AUTHORS file and that's all that's needed in the PR for this one I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants