[BUG] Builder Constructors are protected when should be public in 1.18.18

[icrss]

Describe the bug
The constructor method to obtain a builder does not have public accessibility even though this is default. It does not respect the accessibility parameter either. The Class.builder() method works as expected.

To Reproduce

package a;
import lombok.Builder;
@Builder
public class Data {
}

package b;
import a.Data;
public class Usage {
    public void use() {
        Data.builder().build(); // OK
        new Data.DataBuilder().build(); // Doesn't compile
    }
}

Expected behavior
new Data.DataBuilder() should be public. Explicitly specifying the access as public doesn't have an effect.

Version info (please complete the following information):

• 1.18.16, 1.18.18
• Multiple JDKs, command line and IntelliJ

[icrss]

Just FYI, I need to use the constructor method to avoid massive code changes to legacy code which already use this.

[Rawi01]

I think that this is intentional, at least the documentation states that it generates a package private no-args empty constructor. If you need an additional constructor you can simply add one.

@Builder
public class Data {
    @NoArgsConstructor
    public static class DataBuilder {
    }
}

[rzwitserloot]

Yup, it was intentional (and the access level thing is referring to the access level of the static builder method). For API purposes, having both Person.builder() as well as new Person.PersonBuilder() as different ways to accomplish the identical end result (namely: a new builder) is bad - TIOOWTDI (There should be one— and preferably only one —obvious way to do it. - Cribbing from python here, but it is an excellent API design principle).

But, I do get your pain. I'd like to cater to it if I can, but adding a permanent ugly, hard to fathom wart in the form of an option in the @Builder(params_go_here) paramset of Builder, or even a config key - that is likely a bridge or two too far, so I'm not sure this is solvable.

However, @Rawi01 's fix should work for you, no? If that suffices, that'll be the way to go, I think.

[SebestyenBartha]

I understand the desire to have a clean API. However, @Rawi01's suggested fix doesn't work because in that case lombok tries to create two empty constructors with only the visibility being different which obviously doesn't compile, so to make it work, it would have to be written like so:

@Builder public class Data { public static class DataBuilder { public DataBuilder(){ } } }
Instead of:

@Builder(constructorAccess=AccessLevel.PUBLIC) public class Data { }
Even if the suggestion would work, it would be a lot of unnecessary boilerplate while lombok's whole thing is to reduce boilerplate... :\ And all this just to be able to use a static 'builder()' method which I fail to see the real benefit of compared to just using the regular constructor TBH. OK, it can reduce the number of imports by one, and in theory it has other benefits, but not in practice in this case, what am I missing? According to the doc '.builder()' does nothing but creates a builder instance... I don't see why you would want to hide the constructor behind that method in this use-case. I'd rather have a public constructor and only that - because I agree that it's preferable to have a one way of doing something, but in this case it would break existing code, hence my proposal for the additional workaround. Ideally no argument would be needed for @builder because it would just generate the public constructor by default.

[janrieke]

The duplicate constructor problem could be fixed, I guess.

Factory methods like builder() have advantages over constructors. For instance, I often manually add additional builderXYZ() methods that return pre-filled builder instances for specific purposes. That won't be possible with constructors.

Furthermore, it is a well-established idiom in the builder pattern. Adding a public constructor could confuse users, because they don't know if there is a semantic difference between the constructor and the factory method.

[SebestyenBartha]

Yes, that's what I said - in theory it can have benefits, however, in the current implementation it does not - or am I missing something? To my knowledge the builder() method just does this when compiled to Vanilla and nothing else:
public static builder() {return new DataBuilder();}
So theory sounds fine, but the practice is different.

[SebestyenBartha]

OK, so new suggestion: what if we make it configurable to have a public builder constructor and no factory method OR have a factory method and a hidden builder constructor? That way you have a single option for each class at any given time to create the builder.

[janrieke]

As I explained, the factory method does have real advantages in practice right now. That's not theoretical, I used that approach of adding additional builder methods 2 times in the last 3 weeks.
That won't be possible with constructors only.

As @rzwitserloot said, making this configurable is not a good solution either, given the rare use cases where this is needed. So IMO the way to go is fixing the problem with @NoArgsConstructor on the builder.

[SebestyenBartha]

I can't comment on how "rare" our use case is I have no data to confirm or deny.
This is a legitimate scenario I'm talking about and there is a workaround already, which is just using a lot more boilerplate, IMO unnecessarily, and we're limiting the power of lombok here because of viewpoints of cleanliness and best practices, which I appreciate, but do not share as I can't see the harm the changes would do.
But it's not my call to make, we'll just continue to use our fork I guess.

[rzwitserloot]

because of viewpoints of cleanliness and best practices, which I appreciate, but do not share as I can't see the harm the changes would do.

Let's think this through. Pick a stylistic choice; pick any. You'll always be able to find someone who will say "It is legitimate" and "I prefer it this way", for both available choices, even if the choice is incredibly exotic.

That isn't trying to indicate that your preference here is exotic or nuts. Not at all. I'm merely pointing at that 'random joe said so' is a meaningless indicator.

Thus, your request boils down to three options:

1. We ignore any '... but... we would like it!' style commentary out of principle, or
2. Lombok balloons to support every imaginable style, and between the parameters on our annotations and the settings available in lombok.config, there are literally 200+ different settings available even for something as simple as @Getter. The learning curve for lombok becomes ridiculous, and the ability for us to actually test the combinatory explosion of options to ensure they all make sense and work as one might expect becomes a herculean task.
3. We make a judgement for each request and try to take into account some combination of how much boilerplate is saved, how common the scenario is, how understandable the documentation would be, and contrast it all to the inherent cost of adding a parameter or even a lombok.config setting. And then we make a call. Because we are just humans and not some encyclopedia that is acutely aware of all the java code ever written out there, there's a lot of personal preference and experience involved in this process. We'd prefer it if there wasn't, but I don't know how.

I hope it is obvious that the second option, even though it's clearly what you want here, just isn't on the table, and cannot be. It's not a matter of us trying to hamfistedly force our style preferences on you. Or at least, that's not the intent.

We try to go by the third option, but right now I'm quite convinced that the right move is to close this issue:

• We've gone with the static builder. Even if, in retrospect, it was the wrong call (I don't think it was), changing it now incurs the additional cost of either introducing a breaking change (obviously, forget about that - not going to happen). Thus, it doesn't matter, even if you convince us that the static builder was wrong. Too late now.
• We can, of course, introduce a configuration, to solve the problem instead, but as explained above, adding a configuration incurs its own cost and I'm not sure it's worthwhile here.
• The primary problem with your approach is not so much a code style issue, it's an idiomacy issue: In my experience, the vast majority of builder APIs (no doubt helped along by the fact that lombok also does this), work with a static method. It's like writingYourVariablesLikeThis instead of writing_them_like_this. In java, option 1 is correct. Why? Because the community does it that way, and there is no point holding a debate on what is more readable. Even if there is universal agreement the underscores are more readable, any effort to ask lombok to e.g. start generating identifiers using underscores would be straight up denied. Consistency and following along with community consensus is important.

Now, I'm not sure that 'static methods' are significantly more common than 'public constructors in the builder class', but from my experience I think that's the case, and that's primarily why I don't want to add a feature for this, especially considering that there is a workaround.

Having said all that, I'd still like to fix the @NoArgsConstructor issue, and that will make the additional boilerplate slightly less painful for you.

Given the traffic on this issue so far, here's my proposal:

Without further significant feedback (e.g. in the vein of: Look at all these very commonly used APIs that have public builder constructors! - that might change things), this issue is to be closed, and a new issue created to deal with the @NoArgs issue. Let's wait for feedback until June 20th, 2021.

[bogdanb]

Note that the private constructor causes this Jackson issue.

Basically, you can tell Jackson to use a builder (maybe using @Jacksonized), but it only knows to instantiate the builder via constructor. There is no way to tell it to use a factory method. When using the AfterBurner module this doesn’t work due to module access rules, at least in some cases on newer Java versions. (I’ve no idea exactly how that part works, it may depend on how the dependencies are loaded. I hit the bug above in my app; we’re not using modules, but we’re using Java 16.)

For the record, I could make it work using SebestyenBartha’s method, but only with a hand-written public constructor for the builder, not with @NoArgsConstructor.

(An unrelated issue is that if you set builderMethodName to the empty string, the builder is only accessible via the constructor, which means only from within the class itself. I’m not sure if that is intended or accidental behavior.)

[janrieke]

I'd say: The issue is with Jackson for not supporting a well-established builder idiom.

Don't get me wrong: I don't want to blame Jackson. They need to make choices similar to the one the lombok maintainers had to make, and they chose differently (at least until now). But only because one particular library (although it is quite popular) did so, this does not change the point @rzwitserloot was describing: The maintenance burden for such a feature is probably much higher for a code-generator like lombok than for a "regular" library like Jackson.

So I'd try convincing the Jackson folks to add support for specifying a static builder method name. That has advantages even without lombok (e.g., you can use a different builder for Jackson than for regular programmatic use).

[bogdanb]

I see your point. But I do think that just turning a constructor from private to public is not that much of a burden. I mean, Lombok will always need to generate a constructor for the builder, it’s not like that’s going to ever change. (In principle even adding an option for it is not really necessary, since it wouldn’t be a breaking change.)

On the other hand, there is the issue that in some situations library consumers can upgrade Lombok much easier than Jackson. Jackson is a common shared library, all sorts of other libraries I use depend on specific versions of it. Even if I send Jackson a PR and they release a new version tomorrow I may not be able to actually use it for years. (Most of my job is making Jira add-ons, and in that Jackson is provided by Jira, and the next time it’ll upgrade to a version released today will probably be years from now.) In contrast, Lombok is pretty much compile-time only, I can just upgrade it whenever and get the benefit right away.

(Also, note that the principle says “preferably only one”. I agree that APIs should be as simple and clear as possible, but right now the API is so simple I just can’t use it, and the distance to an API I could use is exactly one replaced word. Besides, it’s not like constructors are a weird and obscure Java feature.)

[arnljot]

I think it would be very valuable to be able to have the lombok generate the builder constructor public.
The reason for this is that when working with mapstruct, and using "ADDER_PREFERRED" collection mapping strategy, mapstruct assumes to find a constructor to pass the builder to. If not found, it'll use the builders fields to set collections to, and ADDER_PREFERRED will not work as intended.

As it stands now, Mapstruct and Lombok will not play nice together since they assume differently about how builders work.

Given the variety of how various projects assumes object creation is handled, I think it would be beneficial if lombok could provide the task of easily providing these strategies.

In my current project we've got objects with inheritance and need to use SuperBuilder, having lombok set that objects constructor to public, so that also Mapstruct could reach it would make our need to provide hand written boilerplate code much less.

[rzwitserloot]

@arnljot Then tell mapstruct. Their library sucks because it can't deal with a very common pattern. And I'm not just saying common because 'lombok works like this' - it's in many projects.

[bogdanb]

I agree that both Jackson and Mapstruct suck because they can’t deal with a common pattern. But also Lombok sucks because it can’t provide a common pattern, used by many projects. Can’t we make Lombok not suck while we’re working on making other libraries suck less?

[rzwitserloot]

No, because catering to multiple common strategies that broadly to the same thing is ordinarily irrelevant unless trying to lombokize existing things without wanting to break backwards compatibility, or work around limitations in other projects that require a specific pattern to function properly. That's all too exotic, because the problem is: How do we 'document' and manage not to asplode settings into a combinatory disaster where most options are complete gobbledygook to most users?

The answer is to aggressively prune that tree and pick preferred patterns.

You've misjudged lombok. Its purpose is not to replace all imaginable boilerplate. Merely some boilerplate. On the flipside, surely mapstruct's purpose is to deal with all (common) patterns of structures that mapstruct can apply to. Hence, it's on them, not on us.