extraPrivate=true from 1.16.22 breaks Jackson creator detection

[bcalmac]

In our project I use value objects like the one below. The only addition to a vanilla @Value + @Builder is overriding the all-args constructor so that I initialize the fields to a default value irrespective of how the object is built: through the all-args constructor (Jackson deserialization) or the builder (my code)

@Value
@Builder
public class Subscription {

    String organizationKey;
    String productKey;
    boolean enabled;
    Throttling throttling;
    ImmutableList<Endpoint> endpoints;
    ImmutableList<Constraint> constraints;

    private Subscription(String organizationKey, String productKey, Boolean enabled, Throttling throttling,
            ImmutableList<Endpoint> endpoints, ImmutableList<Constraint> constraints) {

        this.organizationKey = organizationKey;
        this.productKey = productKey;
        this.enabled = defaultIfNull(enabled, TRUE);
        this.throttling = throttling;
        this.endpoints = defaultIfNull(endpoints, ImmutableList.of());
        this.constraints = defaultIfNull(constraints, ImmutableList.of());
    }
}

This worked fine prior to 1.16.22, Jackson automatically detects the constructor as @JsonCreator and invokes it with all known properties and null for the rest. It also looks nice, the class has no Jackson annotations.

However, after the private no-args constructor added in 1.16.22, Jackson will by default instantiate the object using the no-args constructor and then set the individual fields one by one. Without the all-args constructor, my defaults will no longer be enforced.

This can be fixed by explicitly adding the @JsonCreator to the all-args constructor. Leaving aside the extra annotation that's needed now, this sneaky change will silently break a lot of code that relies on an explicit constructor.

Please evaluate the "noArgsConstructor.extraPrivate" new feature against side-effects like the one mentioned here. Jackson was working fine for me without any extra annotation. What is the "killer use-case" for the new feature?

Also, @Builder.Default would NOT work with Jackson deserialization (before or after this change).

    @Builder.Default
    ImmutableList<Constraint> constraints = ImmutableList.of();

[randakar]

Seconding this one. Having working lombok annotated code break in subtle ways that are undetectable by the compiler is not acceptable, imho. Famous example of other projects' policy here: https://unix.stackexchange.com/questions/235335/why-is-there-a-linux-kernel-policy-to-never-break-user-space The alternative that could be done in this example would be to define the default values with the field declarations (```private ImmutableList< Endpoint> endpoints = ImmutableList.of())```). That *should* work, if lombok doesn't mess with it. But see above. Aside from that: I don't think a no args constructor makes sense if there are other constructors available - like the requiredargs constructor implied by Data and Value annotations. I've been waging a silent war on no args constructors in old projects, replacing them (and setters) with actual constructors and JsonCreator / JsonPojoBuilder annotations for a while now. The whole "instantiate with a no constructor, then call a ton of setters" pattern needs to die imho, for a bunch of reasons. This quiet adding back of a no args constructor stands in the way of that. All in all: Please don't.

…

On Wed, May 30, 2018 at 2:28 AM, Bogdan Calmac ***@***.***> wrote: In our project I use value objects like the one below. The only addition to a vanilla @value + @builder is overriding the all-args constructor so that I initialize the fields to a default value irrespective of how the object is built: through the all-args constructor (Jackson deserialization) or the builder (my code) @***@***.*** class Subscription { String organizationKey; String productKey; boolean enabled; Throttling throttling; ImmutableList<Endpoint> endpoints; ImmutableList<Constraint> constraints; private Subscription(String organizationKey, String productKey, Boolean enabled, Throttling throttling, ImmutableList<Endpoint> endpoints, ImmutableList<Constraint> constraints) { this.organizationKey = organizationKey; this.productKey = productKey; this.enabled = defaultIfNull(enabled, TRUE); this.throttling = throttling; this.endpoints = defaultIfNull(endpoints, ImmutableList.of()); this.constraints = defaultIfNull(constraints, ImmutableList.of()); } } This worked fine prior to 1.16.22, Jackson automatically detects the constructor as @JsonCreator and invokes it with all known properties and null for the rest. It also looks nice, the class has no Jackson annotations. However, after the private no-args constructor added in 1.16.22, Jackson will by default instantiate the object using the no-args constructor and then set the individual fields one by one. Without the all-args constructor, my defaults will no longer be enforced. This can be fixed by explicitly adding the @JsonCreator to the all-args constructor. Leaving aside the extra annotation that's needed new, this sneaky change will silently break a lot of code that relies on an explicit constructor. Please evaluate the "noArgsConstructor.extraPrivate" new feature against side-effects like the one mentioned here. Jackson was working fine for me without any extra annotation. What is the "killer use-case" for the new feature? Also, @Builder.Default would NOT work with Jackson deserialization (before or after this change). @Builder.Default ImmutableList<Constraint> constraints = ImmutableList.of(); — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1708>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKCRfQlYch_pJGvYLBJzBqXK1uJ-87Gks5t3efDgaJpZM4USalT> .

-- "Don't only practice your art, but force your way into it's secrets, for it and knowledge can raise men to the divine." -- Ludwig von Beethoven

[soberich]

@bcalmac You say that extraPrivate=true breaks Jackson, but is't it what it should happen if there is an empty constructor and your visibility is set to any?
I mean why would you create an empty constructor for a Value object? And why are you setting extraPrivate to true??
I have a similar case, and it's fine, so maybe you should first (if you have all you dtos/value objects armed like this one) set Jackson visibility to

        objectMapper
                .setVisibility(PropertyAccessor.FIELD, JsonAutoDetect.Visibility.PUBLIC_ONLY)
                .setVisibility(PropertyAccessor.GETTER, JsonAutoDetect.Visibility.PUBLIC_ONLY)
                .setVisibility(PropertyAccessor.IS_GETTER, JsonAutoDetect.Visibility.PUBLIC_ONLY)
                .setVisibility(PropertyAccessor.SETTER, JsonAutoDetect.Visibility.PUBLIC_ONLY)
                .setVisibility(PropertyAccessor.CREATOR, JsonAutoDetect.Visibility.PUBLIC_ONLY)

or just

        objectMapper
                .setVisibility(PropertyAccessor.SETTER, JsonAutoDetect.Visibility.PUBLIC_ONLY)
                .setVisibility(PropertyAccessor.CREATOR, JsonAutoDetect.Visibility.PUBLIC_ONLY)

since the others are defaults,,
so that regardless if there is private no arg ctor - Jackson will not see it.
(The advantages are obvious, since less reflection is used, etc.)

[bcalmac]

@randakar Initializing the fields would not work, Jackson and the Lombok @Builder would set them to null. For Lombok, there's the @Builder.Default that I mentioned, but this doesn't cover Jackson.

@soberich extraPrivate=true is not my choice. The functionality was added in 1.16.22 and is enabled by default. Now, all the code that gets compiled after the upgrade would get an extra no-args constructor. Scary.

I thought some more about this since yesterday and I'm even more concerned. In the Jackson world, 2 constructors instead of one means more ambiguity for deserialization. I was looking through the documentation yesterday and I couldn't even find a place that details the rules for resolving the ambiguity (that is, which constructor will be used). From unit tests, it looks like it prefers the no-args constructor.

Both Jackson and Lombok do a lot of magic behind the scenes. But there's a threshold from which more magic means chaos and people stop being able to reason about their code. I think the extra no-args constructor just crossed that threshold.

[bcalmac]

Also, consider that a no-args constructor even violates the definition of immutable / value objects. Immutable objects are supposed to be fully initialized at construction time.

Also, the Lombok documentation states the following (and I think it makes total sense):

Also, any explicit constructor, no matter the arguments list, implies lombok will not generate a constructor. If you do want lombok to generate the all-args constructor, add AllArgsConstructor to the class. You can mark any constructor or method with @lombok.experimental.Tolerate to hide them from lombok.

[soberich]

@bcalmac

extraPrivate=true is not my choice. The functionality was added in 1.16.22 and is enabled by default.

Yes, I know. I mean, yes there is that new setting and whether it is enabled by default or manually by developer, all you need to do to neutralize relevant effects is, well, to turn it OFF. I understand that you might not turn it ON, but why don't just turn it OFF now?
It works perfectly.

I thought some more about this since yesterday and I'm even more concerned. In the Jackson world, 2 constructors instead of one means more ambiguity for deserialization. I was looking through the documentation yesterday and I couldn't even find a place that details the rules for resolving the ambiguity (that is, which constructor will be used). From unit tests, it looks like it prefers the no-args constructor.

Yes, sure no-arg.
There is no ambiguity. It chooses explicit @JsonCreator if there is first or no-arg as a second priority.

[soberich]

Just put lombok.noArgsConstructor.extraPrivate = false and there will be no difference with using 1.16.20

[bcalmac]

I know I can get the code to work. I just wanted to raise a flag about what seems to me a dubious feature that's enabled by default, and can easily break your code if you just casually upgrade Lombok. I can very well be wrong, I haven't seen a description of this feature other than "to enable deserialization frameworks (like Jackson) to operate out-of-the-box". For me it was already working out of the box, without any Jackson annotations.

@soberich Do you usually perform a regression test of third-party libraries used in your projects when making a minor version upgrade? That would be the only way to catch breaking changes like this one.

[soberich]

@bcalmac

@soberich Do you usually perform a regression test of third-party libraries used in your projects when making a minor version upgrade? That would be the only way to catch breaking changes like this one.

No I don't, so it's about bad semantic versioning in Lombok and not the new ctor feature. So not extraPrivate=true from 1.16.22 breaks Jackson creator detection but Lombok starting 1.16.22 breaks proper semantic versioning

[bcalmac]

@soberich All right, fair enough. Are you familiar with the reasoning behind adding the extra no-args constructor? Can you explain or point me to the relevant info?

There may be something to it, but adding a no-args constructor to an immutable class is inconceivable (from my outsider perspective).

[Maaartinus]

There may be something to it, but adding a no-args constructor to an immutable class is inconceivable (from my outsider perspective).

The reasoning is very simple: A dumb deserializer first creates an empty instance using the no-args constructor and then it sets the fields using reflection (ignoring private and hacking away final). Apart from Unsafe hacks, it's the only way how to create an object without understanding how to use a different constructor.

The no-args constructor is the traditional way as before @ConstructorProperties, there was no way how to find out what the constructor arguments mean. It used to be the only way and it had to be used also for immutables. Even the JLS accounts for it.

[bcalmac]

OK, so there are some cases when serialization needs to be hacked with a private no-args constructor. This still doesn’t justify having the hack as the default behavior.

As of today, Jackson can successfully deserialize a Value based either on ConstructorProperties or parameters-module.

The way it is, developers need to explicitly opt out of a hack needed only for old code instead of just enabling it when needed. Does that make sense?

[bcalmac]

Also consider that this hack is originally intended for Jackson. How can we evaluate the impact of this breaking change on other libraries and frameworks that do similar introspection? An extra constructor out of the blue is a big deal.

[andrebrait]

As a workaround for now, you can also put a @NoArgsConstructor before the @Data like this (this will make it generate the constructor instead of @Data):

@NoArgsConstructor
@Data
public class Foo {
}

As then you can use the access attribute set to NONE to make it not generate any no-args constructors.

@NoArgsConstructor(access = AccessLevel.NONE)
@Data
public class Foo {
}

[randakar]

The order matters? Wow, did not know that.. Op zo 3 jun. 2018 21:08 schreef André Brait <[email protected]>:

…

As a workaround for now, you can also put a @NoArgsConstructor *before* the @DaTa like this (this will make it generate the constructor instead of @DaTa): @NoArgsConstructor @DaTa public class Foo { } As then you can use the access attribute set to NONE to make it not generate any no-args constructors. @NoArgsConstructor(access = AccessLevel.NONE) @DaTa public class Foo { } — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1708 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKCRcBf9sT0_CiUEopJq20HOSK2yKqCks5t5DQxgaJpZM4USalT> .

[andrebrait]

@randakar me neither, I just found out when I bumped into this issue when upgrading from 1.16.20 to 1.16.22 and then I tried fixing it.

I did take a look into Lombok's code a while back, though, so I thought it could work. I tested and it did lol

[rspilker]

I'll try to address all things mentioned.

First, regarding (semantic) versioning, we underestimated the impact of this change. We thought adding a private no-args constructor would NOT break any backwards compatibility.

The reason we added this feature was that since java9 @ConstructorProperties moved to the module java.desktop, which is not added by default. This let us change the our code generation to switch from opt-out to opt-in.

We got a lot of feedback that that version broke a lot of projects, for instance #1563. So we looked for an alternative.

Another reason to add this feature is to create a stepping stone to allow default values for fields created via the constructor AND via a builder.

Regarding the order of the annotations, I didn't expect that the order would matter, but it does make sense now. Also, we obviously didn't have enough test cases for the interactions between annotations.

[rspilker]

We're going to release 1.18.0 very soon. This will be a breaking change, in the way that we're going to change the opt-out into an opt-in.

[bcalmac]

@rspilker, thank you for chiming in. Isn't jackson-module-parameter-names an adequate alternative to @ConstructorProperties?

From what I understand, this is a Jackson / Java 9 issue, I wouldn't expect Lombok to automatically fix it for me. Different projects would probably what to go with different solutions. IMHO Lombok should stay true to what a @Value is, and that does not include a no-args constructor. By getting in the middle of this, Lombok stops being a helper for generating boilerplate code and starts prescribing how objects should be designed.

Now, if this feature is here to stay, how would you reconcile it with the statement below? It's a guarantee that once you step in by defining a constructor, Lombok gets out of the way.

Also, any explicit constructor, no matter the arguments list, implies lombok will not generate a constructor. If you do want lombok to generate the all-args constructor, add AllArgsConstructor to the class. You can mark any constructor or method with @lombok.experimental.Tolerate to hide them from lombok.

[bcalmac]

@rspilker I've just seen you previous message. Changing this to an opt-in is completely satisfying to me.

[lpandzic]

Hello,

@rspilker, thank you for chiming in. Isn't jackson-module-parameter-names an adequate alternative to @ConstructorProperties?

It is, unless you have a class with one constructor parameter. In that case jackson would fail (it would detect it as a delegating creator instead of a property one) unless you have @JsonCreator or @ConstructorProperties. See FasterXML/jackson-databind#1498 for details.

[bcalmac]

Correct. I do have some of those cases and use @JsonCreator. This also means that I need to define an explicit constructor so I can attach the annotation.