Lombok: Mark fields as required for Builder

[lathspell]

It seems that the Builder from the @ Builder annotation does not check if all fields have been initialized before calling build(). It would be nice if one could mark certain fields as required.
Using final does not help (the objects field gets initialized with null by the private constructor) and the @ NonNull only works for objects and not primitives and also has the drawback that a value of null might even be acceptable, I just want that somebody explicitly called .name(null) and did not simply forget this attribute.

So maybe a @ BuilderRequired or @ Builder(required = true) would be nice?

[randakar]

Marking fields with a NonNull annotation is how you are supposed to do
that. This feature already exists.

Op vr 6 apr. 2018 18:24 schreef Jan Mewes [e-mail]:

Is this solution already sufficient to support this requirement?
https://stackoverflow.com/a/30867286/2339010

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/rzwitserloot/lombok/issues/1043#issuecomment-379305125,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAKCRWDJkmK2QeVOeYx5_hSpBtJ8Hxtuks5tl5bEgaJpZM4HsO0Q
.

[JoaoCamposFrom94]

NonNull does a runtime check, I would prefer something at compile time. It also doesn't prevent other developers from misusing your code. See the comment chain of the SO solution and in particular this comment: https://stackoverflow.com/questions/29885428/required-arguments-with-a-lombok-builder/30867286#comment68241859_30867286

[dlipofsky]

@AllArgsConstructor creates a constructor that takes all args, this would give you a compile-time check that all args are present, although it sure is clunky if you have a lot of args, and doesn't protect you from reversing the position of two args of the same type. @RequiredArgsConstructor would do similar for just the required args (those marked with final or @NonNull). I don't see how you could possibly get a compile-time check for something like a builder in a language like Java.

[janxb]

We are also facing this issue. A class with multiple properties and one of them is required. Currently you have to annotate the required property with @NonNull, but this only throws an exception at run-time. You could also make the field final and use @RequiredArgsConstructor, but then I can still create an empty object using the Builder.

I suggest the following: Instead of generating an empty builder() method, you could generate a builder(Long id) method, when the property id is final. In combination with the @RequiredArgsConstructor, this would remove the no-args constructor and forces you to set the required fields, also when using the builder.

[randakar]

Why is marking that field as Final and NonNull not enough? That should
work, right?

Op di 17 jul. 2018 10:35 schreef Jan Brodda [e-mail]:

We are also facing this issue. A class with multiple properties and one of
them is required. Currently you have to annotate the required property with
@NonNull, but this only throws an exception at run-time. You could also
make the field final and use @RequiredArgsConstructor, but then I can
still create an empty object using the Builder.

I suggest the following: Instead of generating an empty builder() method,
you could generate a builder(Long id) method, when the property id is
final. In combination with the @RequiredArgsConstructor, this would
remove the no-args constructor and forces you to set the required fields,
also when using the builder.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/rzwitserloot/lombok/issues/1043#issuecomment-405504405,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAKCRWiXF33EysVYNrIlDifnRo7kLdCcks5uHaHAgaJpZM4HsO0Q
.

[janxb]

When using your suggestion, builder throws a runtime exception. At compile-time I don’t get any hint that the property may be required, because I can call the builder without setting the affected properties. If the builder method would have required properties as arguments, I would be forced to set them.

[Maaartinus]

@janxb

I suggest the following: Instead of generating an empty builder() method, you could generate a builder(Long id) method, when the property id is final. In combination with the @RequiredArgsConstructor, this would remove the no-args constructor and forces you to set the required fields, also when using the builder.

IMHO this makes a lot of sense, as long as there are not too many required fields. Otherwise, you'd need another builder for building the argument list for the builder(.....) call. :rofl:

This should work independently of the nullability of the field. Something like

@Value @Builder
class X {
    @Builder.Required X parent;
    int x, y;
}

X.builder(null).x(1).y(2).build();

is strange but perfectly fine. It only guarantees that the field won't get forgotten.

[randakar]

Well, if you're going to do this you probably want it for all fields. In
cases like this you probably want your entire object to be immutable, not
just a single field.
Let's face it, null values are to be avoided as much as possible.

But if you want your entire object to be immutable you basically go back to
square one - the builder will have all arguments as an argument so you
might as well just call the constructor directly.

If there was a way to tell the java compiler that not calling a method on
an object before doing other things is an error we could solve this. But
there isn't, so we can't.
The best I can come up with is to embed this kind of rule in other tools -
Sonar, IntelliJ, eclipse .. those should be capable of at least warning
"hey, you're using this builder but you're not calling all methods on it".

But even then .. there may be situations where people pass a Builder around
between layers and incrementally set fields on it. Those use cases would
also fail that test and basically turn up as false positives.

So all in all .. I think it's not possible (enforcing method calls on the
builder), or not likely to be useful in practice (enforcing fields).

On Tue, Jul 17, 2018 at 12:43 PM, Maaartinus [e-mail]
wrote:

@janxb https://github.com/janxb

I suggest the following: Instead of generating an empty builder() method,
you could generate a builder(Long id) method, when the property id is
final. In combination with the @RequiredArgsConstructor, this would
remove the no-args constructor and forces you to set the required fields,
also when using the builder.

IMHO this makes a lot of sense, as long as there are not too many required
fields. Otherwise, you'd need another builder for building the argument
list for the builder(.....) call. 🤣

This should work independently of the nullability of the field. Something
like

@Value @Builder
class X {
@Builder.Required X parent;
int x, y;
}

X.builder(null).x(1).y(2).build();

is strange but perfectly fine. It only guarantees that the field won't get
forgotten.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/rzwitserloot/lombok/issues/1043#issuecomment-405539847,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAKCRaZZaVfcXawZAmx1HLKEXeab0Kt9ks5uHb_hgaJpZM4HsO0Q
.

--
"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

[janxb]

For my specific use-case I can say that we don’t require every field to be filled. We have a data object with ~10 properties, but only the ID is mandatory. So as you can see in my example above, the builder method would only require one additional argument.

Also I can’t really understand your point about passing around the builder. If required arguments are parameters of the initial builder call, there is no need for further evaluation.

[randakar]

If that happens to be the case, sure. But who guarantees that this is going
to be true?

And when is your rule going to trigger? When the builder is instantiated it
can't be checked yet. The most logical thing would be to check it when
.build() is being called, but that .build call might happen in a completely
different layer of the application so what are you going to warn on?
Either way, even if it's easy to build, the java compiler cannot really
help us, so you're going to have to look at Sonar and IDE rules for this.

On Tue, Jul 17, 2018 at 2:25 PM, Jan Brodda [e-mail]
wrote:

For my specific use-case I can say that we don’t require every field to be
filled. We have a data object with ~10 properties, but only the ID is
mandatory. So as you can see in my example above, the builder method would
only require one additional argument.

Also I can’t really understand your point about passing around the
builder. If required arguments are parameters of the initial builder call,
there is no need for further evaluation.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/rzwitserloot/lombok/issues/1043#issuecomment-405564003,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAKCRa8NAz-x_csOof2ynr-NQ6Z4u4loks5uHdfIgaJpZM4HsO0Q
.

--
"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

[Maaartinus]

@randakar

Let's face it, null values are to be avoided as much as possible.

Sure, my example was just a special case showing that this feature is unrelated to nullability.

And when is your rule going to trigger? When the builder is instantiated it
can't be checked yet.

I'm pretty sure that @janxb does not want any checks to happen. All what's needed is to make some fields (in his case just the id) of the builder required and this gets assured trivially. I guess, I understand what he means as I could use something similar:

Source:

@Value @Builder class X {
    @Builder.Required private final int id;
    private final int x, y, z;
}

Vanilla Java:

class X {
    static class Builder {
        private final int id;
        private int x, y, z;

        private Builder(int id) {
            this.id = id;
        }

         .... setters and build()
    }

    public Builder builder(int id) {
         return new Builder(id);
    }

    private final int id;
    private final int x, y, z;
}

All it does is to make Builder.id final and implement the consequences. Nothing has to be checked and nothing can go wrong. You must set the id immediately and it's excluded from further building. Add @NonNull at will.

[randakar]

Yeah, that is my understanding too. I just question how valuable this
really is. You're losing the documenting value that Builder is supposed to
bring that way, so why not just call the constructor in the first place?

On Tue, Jul 17, 2018 at 3:02 PM, Maaartinus [e-mail]
wrote:

@randakar https://github.com/randakar

Let's face it, null values are to be avoided as much as possible.

Sure, my example was just a special case showing that this feature is
unrelated to nullability.

And when is your rule going to trigger? When the builder is instantiated it
can't be checked yet.

I'm pretty sure that @janxb https://github.com/janxb does not want any
checks to happen. All what's needed is to make some fields (in his case
just the id) of the builder required and this gets assured trivially. I
guess, I understand what he means as I could use something similar:

Source:

@Value @Builder class X {
@Builder.Required private final int id;
private final int x, y, z;
}

Vanilla Java:

class X {
static class Builder {
private final int id;
private int x, y, z;

    private Builder(int id) {
        this.id = id;
    }

     .... setters and build()
}

public Builder builder(int id) {
     return new Builder(id);
}

private final int id;
private final int x, y, z;

}

All it does is to make Builder.id final and implement the consequences.
Nothing has to be checked and nothing can go wrong. You must set the id
immediately and it's excluded from further building. Add @NonNull at will.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/rzwitserloot/lombok/issues/1043#issuecomment-405573762,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAKCRbGCCTH6LPNghS4rg9C33hz2z55Nks5uHeBwgaJpZM4HsO0Q
.

--
"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

[Maaartinus]

@randakar 'cause they're maybe tens of -non-required fields and you really wouldn't like such a constructor.

Concerning the documenting value of the builder: When only a few fields are required, then this can't be any problem. These required fields are typically the most important ones and this makes being explicit with them unimportant. The builder(....) call ensures that they don't get forgotten.

Actually, this is a tiny step towards the readability of combined python's positional and keyword arguments (the examples there are mostly strange overuses, but the principle is very clean).

[randakar]

Ok, The python thing is fancy, I give you that. Would love to have this, at
least for the non-positional arguments.
Positional arguments - less so. If you're going to have an option to use
fields by name, I'd rather have it mandatory to be honest.

That aside:

It's probably just my own personal pet peeve getting in the way here.
I like immutable objects.
I also like banishing null as much as possible.
If you combine the two, a builder should always set all fields. No
exceptions.

Therefore, there should not be such a thing as 'optional' fields except in
the sense that they might contain an Optional<> of some kind.
Therefore, if I were to use this feature, it would turn basically turn
into a more complicated and slower version AllArgsConstructor.

Sure, there are frameworks (hibernate, jackson in some circumstances) that
do not yet support this style of building applications properly, but that
is just a matter of time in my opinion.

So, personally, long term, I'd rather see a solution that makes this work
with named fields. And since lombok itself is not in a position to create
such a solution, we have to turn to other tools to at least provide
warnings about it.
Still not ideal, still not exactly what the OP wants, but at least it would
be something.

On Tue, Jul 17, 2018 at 5:06 PM, Maaartinus [e-mail]
wrote:

@randakar https://github.com/randakar 'cause they're maybe tens of
-non-required fields and you really wouldn't like such a constructor.

Concerning the documenting value of the builder: When only a few fields
are required, then this can't be any problem. These required fields are
typically the most important ones and this makes being explicit with them
unimportant. The builder(....) call ensures that they don't get forgotten.

Actually, this is a tiny step towards the readability of combined
python's positional and keyword arguments
https://docs.python.org/dev/tutorial/controlflow.html#keyword-arguments
(the examples there are mostly strange overuses, but the principle is very
clean).

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/rzwitserloot/lombok/issues/1043#issuecomment-405616247,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAKCRVIZ-1J7H5PnBkNnVHSK4-a1seMoks5uHf2TgaJpZM4HsO0Q
.

--
"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

[janrieke]

I like the idea of having a new annotation @Builder.Required. Its behavior could be made working nicely together with @NonNull, e.g. like this:

If a field has @Builder.Required, lombok adds a parameter to the builder() method, and additionally skips the generation of the builder's field setter method. If the field also has @NonNull, lombok adds a null check to the builder() method (and, of course, keeps the null check in the constructor as it is right now). In this way, a builder instance has a non-null value for that field from the start, and that field value could not be removed from the builder later on.
For instance, as stated here, some users would like to use FindBugs to spot potential null values for @NonNull fields after compilation. This approach would allow FindBugs to do so. And as this feature explicitly requires adding the new annotation @Builder.Required, we would not break legitimate use cases like removing a field value from a builder by calling field(null).

[randakar]

Yeah, that's exactly the proposal that Maartinus did. ;-)

On Thu, Jul 19, 2018 at 11:24 AM, Jan Rieke [e-mail]
wrote:

I like the idea of having a new annotation @Builder.Required. Its
behavior could be made working nicely together with @NonNull, e.g. like
this:

If a field has @Builder.Required, lombok adds a parameter to the builder()
method, and additionally skips the generation of the builder's field setter
method. If the field also has @NonNull, lombok adds a null check to the
builder() method (and, of course, keeps the null check in the constructor
as it is right now). In this way, a builder instance has a non-null value
for that field from the start, and that field value could not be removed
from the builder.
For instance, as stated here
https://stackoverflow.com/questions/51324922/findbugs-detecter-for-nonnull-lombok-builder-attributes,
some users would like to use FindBugs to spot potential null values for
@NonNull fields after compilation. This approach would allow FindBugs to
do so. And as this feature explicitly requires adding the new annotation
@Builder.Required, we would not break legitimate use cases like removing
a field value from a builder by calling field(null).

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/rzwitserloot/lombok/issues/1043#issuecomment-406213569,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAKCRZq-o9u_g_NyRUaQwskfIwoWuzeAks5uIFBdgaJpZM4HsO0Q
.

--
"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

[janrieke]

Not exactly: He did not mention the additional null checks in the builder() method for @NonNull fields (which are important to satisfy the FindBugs issue, see stackoverflow).
But anyway: Take it as an upvote, then. :)

PS: We have to consider that toBuilder() will become more complex to generate, because right now it simply calls the builder's setter methods.

[randakar]

True. But that's sort of common sense, right?

On Thu, Jul 19, 2018 at 1:39 PM, Jan Rieke [e-mail] wrote:

Not exactly: He did not mention the additional null checks in the
builder() method for @NonNull fields.
But anyway: Take it as an upvote, then. :)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/rzwitserloot/lombok/issues/1043#issuecomment-406246890,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAKCRQNfUCdTOh6ydLjNArdFTGTJpUcGks5uIHACgaJpZM4HsO0Q
.

--
"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

[janrieke]

@rzwitserloot, @rspilker: What's your opinion on Maaartinus' idea? Is it worth implementing?

[Maaartinus]

@janrieke It's not really my idea, I've only re-formulated what has been requested.

PS: We have to consider that toBuilder() will become more complex to generate, because right now it simply calls the builder's setter methods.

The setter could stay and be made private and honor the @NonNull annotation.

[janrieke]

The setter could stay and be made private and honor the @NonNull annotation.

If we'd generate a null check in it (of course only if there is both @NonNull and @Builder.Required present), why not keeping it public?
If it's private, it will still be accessable from the @Builder-annotated class, so we do not win much with that.

[Maaartinus]

@janrieke Such a public setter would be only useful if you want to change the already set value. Which you rarely do, but yes, such cases do exist.

If it's private, it will still be accessable from the @Builder-annotated class, so we do not win much with that.

Sure, you can access it from the whole class, but so you can access the field itself. My reason was that it's improbably needed and therefore should be somewhat hidden.

• Maybe there should be @Builder.Required(setterAccessLevel=...)?
• The default should be NONE?

Another user wanting this.

[janrieke]

Yes, then we'd have enough flexibility.
In my opinion, this would make a nice extension to the @Builder annotation (which IMO really is the most useful lombok feature). Only that it won't work with @SuperBuilder. :-(

[rzwitserloot]

This idea is not good enough.

https://github.com/rzwitserloot/lombok/wiki/FEATURE-IDEA:-%22Mandatory%22-fields-with-@Builder

[banterly91]

I followed up on @rzwitserloot idea and implemented a plugin for this. It is just the first version so any feedback is appreciated. Check it out here:

https://github.com/banterly91/Java-Builder-Guided-Completion-Intellij-Plugin

Bad part is that for now it only supports builder classes whose source code you have access to. I do hope I can make it work in the future with generated builders.