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] Jackson annotations are added in Builder even without @Jacksonized #3186

Closed
adriil opened this issue Apr 28, 2022 · 8 comments
Closed

Comments

@adriil
Copy link

adriil commented Apr 28, 2022

Describe the bug
After upgrading from 1.18.12 to 1.18.16, Jackson annotations like JsonFormat, JsonIgnore, ... are copied from Class members to setter and the according Builder fields.

This looks a lot like the @Jacksonized feature introduced in 1.18.16 with #2387, but the problem is that I don't use this annotation, and the logic seems to be applied anyway.

For instance, for this initial java file...

@JsonDeserialize(builder = Expense.ExpenseBuilder.class)
@JsonInclude(JsonInclude.Include.NON_NULL)
@Getter
@Setter
@Builder
@JsonFilter("myFilter)
public class Expense {

    @JsonFormat(shape = JsonFormat.Shape.STRING, pattern = ISO_8601_WITH_MILLIS)
    @JsonSerialize(using = LocalDateTimeSerializer.class)
    private LocalDateTime budgetAccrualDate;

    @JsonPOJOBuilder(withPrefix = "")
    public static final class ExpenseBuilder {
    }

}

I have the following vanilla java files :

  • With 1.18.16 :
@JsonDeserialize(
    builder = ExpenseBuilder.class
)
@JsonInclude(Include.NON_NULL)
@JsonPropertyOrder(
    alphabetic = true,
    value = {"id"}
)
@JsonFilter("myFilter")
public class Expense {

    @JsonFormat(
        shape = Shape.STRING,
        pattern = "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'"
    )
    @JsonSerialize(
        using = LocalDateTimeSerializer.class
    )
    private LocalDateTime budgetAccrualDate;

    Expense(final LocalDateTime budgetAccrualDate) {
        this.budgetAccrualDate = budgetAccrualDate;
    }

    public static ExpenseBuilder builder() {
        return new ExpenseBuilder();
    }

    public LocalDateTime getBudgetAccrualDate() {
        return this.budgetAccrualDate;
    }

    @JsonFormat(
        shape = Shape.STRING,
        pattern = "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'"
    )
    public void setBudgetAccrualDate(final LocalDateTime budgetAccrualDate) {
        this.budgetAccrualDate = budgetAccrualDate;
    }

    @JsonPOJOBuilder(
        withPrefix = ""
    )
    public static final class ExpenseBuilder {

        private LocalDateTime budgetAccrualDate;

        ExpenseBuilder() {
        }

        @JsonFormat(
            shape = Shape.STRING,
            pattern = "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'"
        )
        public ExpenseBuilder budgetAccrualDate(final LocalDateTime budgetAccrualDate) {
            this.budgetAccrualDate = budgetAccrualDate;
            return this;
        }

        public Expense build() {
            return new Expense(this.budgetAccrualDate);
        }

        public String toString() {
            return "Expense.ExpenseBuilder(budgetAccrualDate=" + this.budgetAccrualDate + ")";
        }
    }
}
  • With 1.18.12 :
@JsonDeserialize(
    builder = ExpenseBuilder.class
)
@JsonInclude(Include.NON_NULL)
@JsonPropertyOrder(
    alphabetic = true,
    value = {"id"}
)
@JsonFilter("myFilter")
public class Expense {
    @JsonFormat(
        shape = Shape.STRING,
        pattern = "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'"
    )
    @JsonSerialize(
        using = LocalDateTimeSerializer.class
    )
    private LocalDateTime budgetAccrualDate;

    Expense(final LocalDateTime budgetAccrualDate) {
        this.budgetAccrualDate = budgetAccrualDate;
    }

    public static ExpenseBuilder builder() {
        return new ExpenseBuilder();
    }

    public LocalDateTime getBudgetAccrualDate() {
        return this.budgetAccrualDate;
    }

    public void setBudgetAccrualDate(final LocalDateTime budgetAccrualDate) {
        this.budgetAccrualDate = budgetAccrualDate;
    }

    @JsonPOJOBuilder(
        withPrefix = ""
    )
    public static final class ExpenseBuilder {
        private LocalDateTime budgetAccrualDate;

        ExpenseBuilder() {
        }

        public ExpenseBuilder budgetAccrualDate(final LocalDateTime budgetAccrualDate) {
            this.budgetAccrualDate = budgetAccrualDate;
            return this;
        }

        public Expense build() {
            return new Expense(this.budgetAccrualDate);
        }

        public String toString() {
            return "Expense.ExpenseBuilder(budgetAccrualDate=" + this.budgetAccrualDate + ")";
        }
    }
}

So with 1.18.16, Jackson annotations are now copied to Builder (and also Class setter), even without @Jacksonized annotation.

To Reproduce
Compile the following file :

$ curl -s https://repo1.maven.org/maven2/org/projectlombok/lombok/1.18.16/lombok-1.18.16.jar -O
$ curl -s https://repo1.maven.org/maven2/com/fasterxml/jackson/core/jackson-annotations/2.13.2/jackson-annotations-2.13.2.jar -O
$ curl -s https://repo1.maven.org/maven2/com/fasterxml/jackson/core/jackson-core/2.13.2/jackson-core-2.13.2.jar -O
$ curl -s https://repo1.maven.org/maven2/com/fasterxml/jackson/core/jackson-databind/2.13.2.2/jackson-databind-2.13.2.2.jar -O
$ javac -cp "*" TestJacksonLombok
import com.fasterxml.jackson.annotation.*;
import com.fasterxml.jackson.databind.*;
import com.fasterxml.jackson.databind.annotation.*;
import lombok.*;
import java.time.LocalDateTime;

public class TestJacksonLombok {

    public static void main(String[] args) {

    }

    @JsonDeserialize(builder = Expense.ExpenseBuilder.class)
    @Getter
    @Setter
    @Builder
    public static class Expense {

        @JsonFormat(shape = JsonFormat.Shape.STRING, pattern = "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'")
        private LocalDateTime date;

        @JsonPOJOBuilder(withPrefix = "")
        public static final class ExpenseBuilder {
        }

    }

}
  • If you compile it again but this time with curl -s https://repo1.maven.org/maven2/org/projectlombok/lombok/1.18.12/lombok-1.18.12.jar -O, you will see there is no issue with 1.18.12.

Expected behavior
If I'm correct assuming this is an issue with @Jacksonized, its logic shouldn't be applied if @Jacksonized isn't explicitly set.

Version info (please complete the following information):

  • Issue appears in 1.18.16
  • Platform : Java 11 (OpenJDK SAP Machine distribution)

Additional context
When JsonFormat is copied to the Builder, it makes the deserialization stricter as you can't rely on Jackson lenient logic if a pattern is defined.
So in the case you're maintaining an API, before 1.18.16 users are able to use slightly different format than the one you defined (slightly different but still valid for Jackson depending on the expected type, though), i.e. maybe by mistake they are not strictly using the milliseconds for a LocaleDateTime for instance.
But after 1.18.16, this tolerance is gone because the JsonFormat is defined at Builder level, and if a pattern is defined, enabling lenient setting has no effect. So this is a breaking change for your API users who don't have choice to use strictly the format you defined. We'd like to avoid having this consequence by default.

@rzwitserloot
Copy link
Collaborator

rzwitserloot commented Apr 29, 2022

The COPY_TO_SETTER_ANNOTATIONS lists in HandlerUtil.java includes various jackson annotations. That lombok copies these is intentional (it's not intended that @Jacksonized is required for copy annotation behaviour - that's not the bug).

I'm not quite sure if there is some logic to why jackson behaves like this or if this is a bug on their side; we're on v1.18.24, at this point removing this behaviour is just as disruptive, and you're the first to report it. Point being: There is no easy answer anymore.

Really starting to regret attempting to address copying of annotations at all :(

@adriil
Copy link
Author

adriil commented Apr 29, 2022

Thank you for your response.
I understand the situation, after all it makes sense to have the format applied at both serialization and deserialization.

Would there be a way to maybe add a configuration setting to disable the copy of annotations ? I admit this might sound overkill if we're the first reporting it, but from my side it looks like we'll have to keep 1.18.12 until we audited our incoming calls and detected how much customers are on thin ice and rely on Jackson leniency.

@adriil
Copy link
Author

adriil commented Apr 29, 2022

Actually I think a lighter solution like configuring a list of annotations we want to exclude from this logic would do the trick and would probably be more acceptable. I don't know if this is technically feasible though.

@rzwitserloot
Copy link
Collaborator

It keeps coming up but I don't want to add it. The 'other' one where this keeps coming up proves why: Because blindly copying it is perhaps not the right move in the first place.

I'm having a hard time accepting that there exists annotations for which it is logical to copy them exactly 95.000% of the time. Any less than that, and we shouldn't presume copying it was right in the first place. Any more than that, and whatever methods you want to exist that didn't get the copy are no longer boilerplate and you should just write it out by hand.

Lombok cannot become a mess o' config options, that is not a route towards a library that is easy to maintain, explain, and use.

This particula case sounds more like: You're doing something sufficiently bizarre that you should write the methods out long-form.

Have some patience with me - I don't use Jackson much. Is there a way to explain to me (as a non-jackson user) why the need to not copy these would plausibly come up?

@adriil
Copy link
Author

adriil commented May 2, 2022

I totally agree and understand why you don't want to add it.

Is there a way to explain to me (as a non-jackson user) why the need to not copy these would plausibly come up?

A problem will come up especially with @JsonFormat annotation.
When we use it to define a pattern for a LocalDatetime, for instance @JsonFormat(shape = JsonFormat.Shape.STRING, pattern = "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'"), then Jackson will strictly use this pattern to serialize the field. For now, no issue at all.
For deserialization, however, if you tell Jackson to use the Builder to deserialize the object , like the following @JsonDeserialize(builder = Expense.ExpenseBuilder.class), then you will have two different behaviours :

  • if the annotation @JsonFormat hasn't been copied to Builder field :
    • then Jackson, with default leniency enabled, will allow to deserialize a String that looks like a LocaleDateTime, but doesn't strictly match the ISO LocaleDateTime pattern. So for instance the following payload without milliseconds will work
      {
          "date": "2018-01-01T10:00:00Z"
      }
  • if the annotation @JsonFormat has been copied to Builder field :
    • then Jackson, like for serialization, will enforce that the deserialized String strictly matches the pattern defined in the annotation (whatever leniency is enabled or not, which is expected behaviour), so the payload above won't work anymore, and you need to update it like the following :
      {
          "date": "2018-01-01T10:00:00.000Z"
      }

Hence the breaking change implied by Lombok upgrade for users who might not correctly follow the expected format.

But to be honest, this is an issue because we thought Jackson was strict all along even for deserialization, but it appears that if you tell it to use the Builder to deserialize, then the enforcement is done only if annotations are copied to the Builder. In the end, having Lombok copying this annotation is not wrong to me. I'd just wish it wasn't, for this specific situation :)

This particula case sounds more like: You're doing something sufficiently bizarre that you should write the methods out long-form.

Is there a way, on our side, to "undo" the copy done by lombok for this specific annotation @JsonFormat ?

@janrieke
Copy link
Contributor

janrieke commented May 2, 2022

Just an idea (lacking a PC to test it at the moment):
Lombok has some kind of annotation duplication avoidance. I don't know if this logic applies here, but you could try manually implementing the affected setter method in the builder and put a @JsonFormat(lenient=true) on it.

If we're lucky, that already works. If not, I think that could be something to address in a new lombok release. It's a potential breaking change, but it's very unlikely, because it would affect only repeatable annotations, as this is the only way to allow multiple annotations of the same type. And even if it breaks someone's code, that's easily fixable by manually adding the annotation a second time. And I don't know if there are even repeatable annotations in the annotations-to-copy list.

@adriil
Copy link
Author

adriil commented May 3, 2022

Thank you very much, your suggestion seems to work as expected :

  1. added manually the method and annotation in @Builder
        @JsonPOJOBuilder(withPrefix = "")
        public static final class ExpenseBuilder {

            @JsonFormat(lenient = OptBoolean.TRUE)
            public ExpenseBuilder date(LocalDateTime date) {
                this.date = date;
                return this;
            }

        }
  1. after compilation, the annotation is still there and haven't been replaced by @JsonFormat(shape = JsonFormat.Shape.STRING, pattern = "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'")
        @JsonPOJOBuilder(
            withPrefix = ""
        )
        public static final class ExpenseBuilder {
            private LocalDateTime date;

            @JsonFormat(
                lenient = OptBoolean.TRUE
            )
            public ExpenseBuilder date(LocalDateTime var1) {
                this.date = var1;
                return this;
            }

            ExpenseBuilder() {
            }

            public Expense build() {
                return new Expense(this.date);
            }

            public String toString() {
                return "TestJacksonLombok.Expense.ExpenseBuilder(date=" + this.date + ")";
            }
        }

Then I guess we have a way to work around this, I'll close this issue.
Thank you very much for your time and support @rzwitserloot @janrieke .

@adriil adriil closed this as completed May 3, 2022
@rzwitserloot
Copy link
Collaborator

I'll make a separate issue for it, but I'm strongly considering a completely different approach to automatic annotation copying. The new plan is: We don't, instead, you add a single line to lombok.config naming your 'library' (not any individual annotation) and we do the right thing for it, automatically. This means it's now opt-in, there won't be 'surprises' (Where some point-release update of lombok causes some annotation to be silently copied whereas before we didn't), we can 'crash' if we don't recognize the key (that would indicate you need to upgrade lombok first / someone with a newer version added it), and we have a place to document it, at least.

With as one exception: Any annotations that are specifically about nullity at the language level (and not, say, at the 'when you make a table in some SQL database to model this type, stick a non-null constraint on it', so javax.validation doesn't count, as an example) - we support those inherently, including copying them, and will silently add new ones in newer lombok versions if some well meaning dweeb adds the 85th different take on nonnull annotations. Please don't do that, but if you must, let us know. It has to be public and meant for general consumption (not your own internal nonnull annotation used solely within a single project or company or team).

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

3 participants