Lombok: Builder.Default causes fields to be null when instantiating with new

[kevcodez]

When using the @Builder.Default annotation on a list/set/map and instantiating the object using the new keyword, the field is null.

@Data
@Builder
@AllArgsConstructor
@NoArgsConstructor
@Entity
public class MyClass {

   @OneToMany(mappedBy = "orderItem", cascade = CascadeType.ALL)
   @Builder.Default
   private List<Entitlement> entitlements = new ArrayList<>();

}

// entitlements is null, should be empty ArrayList
MyClass myClass = new MyClass();

[heapifyman]

This issue also occures with any other non-primitive type, not only list/set/map.

```#java
@Getter
@Setter
@AllArgsConstructor
@NoArgsConstructor
@Builder
@ToString(callSuper = true)
public class Test {

public static final int DEFAULT_INT = 42;

public enum Status {
    STATUS_ONE,
    STATUS_TWO;
}

@Builder.Default
private Integer someInt = DEFAULT_INT;

@Builder.Default
private Status currentStatus = Status.STATUS_ONE;

public static void main(String[] args) {
    Test test = new Test();
    System.out.println("test = " + test);
}

}
```

[TokenR1ng]

This Issue makes the Release 1.16.16 totally unusable for us. Hope this gets fixed in the next Release since we could really use @Builder.Default Annotation.

[wgorski-antrego]

Are there any plans for fixing this? To be honest, it's a HUGE problem as it's super unintuitive and introduces bugs that are really hard to find. To be honest, the most intuitive way of generating a default value for the builder would be to take the value from the field initialization, instead of requiring an additional Builder.Default annotation.

[joaodelgado]

From a quick look at the source code, I found that the @Builder.Default is clearing the initialization part of every annotated field. Line 585:
https://github.com/rzwitserloot/lombok/blob/e92e285d95e0fb08c32e7059f85806a30985084b/src/core/lombok/javac/handlers/HandleBuilder.java#L580-L590

What's the purpose of clearing this initialization? Is it because of potential side effects of the initialization expression?

[omega09]

@joaodelgado See here. Initialization has been moved to the builder so if you don't nullify the field it will be initialized twice.

[davidje13]

@omega09 in that case I would say the @NoArgsConstructor (and similar) must be converted to set the fields too. As it is, this causes bizarre bugs and makes the @Builder annotation harmful. It's a clear violation of least-surprise, since it's removing a commonly used language-level feature.

I'm guessing builders have only been considered in isolation, but it is often useful to have @NoArgsConstructor and a builder. One use case is when working with ORM or JSON (where libraries often need no-args constructors). Another is testing (where the main code may use constructors but the tests use builders).

For now, it looks like the only workaround is to manually write a no-args-constructor, which is a pain.

[mimfgg]

writing the constructors manually also means that the constraints annotation like @Nonnull have to be processed manually there as well, which is not really elegant.

Any plan to fix the @Builder.default current behaviour?

[omega09]

@davidje13 In that case go write your opinion on the PR to the person who has the authority and not to me, who just explained the reason for this to someone who asked about the implementation detail.

[aburmeis]

To clean the initialization should be done in combination with final fields (like when applying Value) but not for non-final members like when combining Builder with Data, imho.

[dahn-zk]

Any updates?

[TokenR1ng]

We really need this issue to be solved. We cannot update to newer versions because of this and now we are also getting conflicts with other newer libs, e.g. Jackson >2.9. So we cannot update jackson because we cannot update lombok and so on.

It really is a problem for us since we have a lot of classes, e.g. value objects with some fields initialized, that use builder as well as constructor annotations. Even writing the constructors manually would not fix this problem, since @Builder.Default seems to remove the initialization.

Can we please have any feedback on this?
thx

[FerrariBruno]

It seems to work if you write the no args constructor manually. I haven't had any issues. I agree this should be resolved ASAP though.
For me it caused a lot of trouble when paired with DBFlow v3, as it uses constructors to create objects in the generated classes.

[mkeller-ergon]

But only if you duplicate the field defaults again in the default constructor... An empty default constructor won't work.

[FerrariBruno]

Yeah, that's true. If you use the @NoArgsConstructor annotation (or write the no args constructor manually but don't initialize the fields) you will face a ton of trouble.

[palme1337]

Is there any update on this? It seems like the easiest solution is to not remove the original instantiation. That way a default NoArgsConstructor will correctly pick up your field variable instantiation and your builder.build() method will just call the generated "default" targets.

[schuettec]

Is there anyone working on this one? We have very strange side effects when using Lombok in conjunction with JPA. The list references are nulled when using @Builder.Default.

[rspilker]

Yes, we're looking into this.

[xak2000]

@rspilker The reason to make the change, leading to this issue is to get rid of possible double-initialization, right? It is desribed here #1429.

But, really, the double-initialization is a way way less a problem than this totally unexpected behavior of modifying default initialization of POJO fields. Of course, the best way to solve this is somehow get rid of double-initialization and still not break field initializers. But if this is not possible, or too hard, or too long, then, just make this double-intialization happen! Then document it into @Builder.Default javadocs in bold and many people will say thanks you!

Yes, this can lead to problems if someone uses some sofisticated field initializers, but it is much less common, than problems from unexpectedly nullified fields. IMHO.

At least, document this nullifying-field-behavior in @Builder.Default javadocs in big red warning font. This behavior is really, really surprising and hard to found when a NPE thrown.

[rainerhahnekamp]

I totally agree to @xak2000.

I would even go further and say that @Builder.Default is dangerous because the nullification is not documented enough and it is completely non-intuitive.

Lombok is really great but this needs to be fixed. Is there already a pull request? Otherwise I can volunteer for one.

[NeveHanter]

The point is that @Builder.Default is nested class of the @Builder annotation, why do you even consider it working with i.e. @NoArgsConstructor as it's not an @Builder annotation and they're not even related?

It's just obvious, so there are only two solutions:

• Refactor @Builder.Default as @Default and add support for all constructor classes with the same effect of removing field initialization breaking current code,
• Stop relying on @Builder.Default in constructors case which wasn't probably even designed to interop with them

[shssoichiro]

Obvious? Why would it ever be obvious that two annotations don't work with each other, when it's not documented? It's trivial for an annotation processor to check if another annotation exists and carry over its value. It's not like @Builder.Default is private.

That being said, I'll take a shot at fixing this when I get to a computer.

[yquemener]

@NeveHanter this is not even an incompatibility with the @NoArgsConstructor but simply with any manually declared constructor as well. If I instantiate a class without going through the builder, the default values are ignored. That's problematic in any case where a class can be instantiated without a builder.

[goostleek]

@NeveHanter In that case I use this approach.

[janrieke]

@rspilker, do you already have a solution? If not, I also vote for abandoning the current approach of moving the init expression to a method (also considering #1749), because in most cases the memory/performance impact should be minimal.

If you really want to stick to the current approach, maybe this could be a way to go:

• Instead of moving the initialization expression to a static method, remove it from the variable declaration and add an assignment to the top of constructors (using a very low priority Handler, so other lombok annotation already finished generating their constructors). Also add it to manually implemented constructors.
• Do _not_ add this assignment to constructors that always explicitly assign a value to the instance variable.
• Let @Builder generate a (private) constructor with a builder instance as sole parameter. Use private boolean var$set to decide whether to use the init expression or the value in the builder instance. Also do not add the assignment to this constructor.

Drawbacks:

• @Builder would not generate an @AllArgsConstructor any more. This could break existing code. However, I consider this nicer, anyway.
• It may not be easy to find out if a constructor always assigns a value (ifs, early exits, etc.).
• This approach would change the order of initializations if there are only some instance variables annotated with @Builder.Default. (Note that this is also the case right now.) In the following example, the value of k would be 0 (instead of 1, as you'd expect), because the initialization of k would be executed before the initialization of j in constructors:

class Test {
    @Builder.Default
    int j = 1;
    int k = j;
}

[rzwitserloot]

@janrieke not implementable; we do not have the ability to make a complete copy of an arbitrary node. Any given expression node can be arbitrarily complex:

Object x = new Object() {
    /* absolutely all valid java nodes imaginable can go in here */
};

so making an 'expression duplicator' is a few manmonths work.

You'd think the ASTs themselves have the ability to cleanly copy any node, but... they don't. We've tried it before and it seems to work, but then it breaks in weird, unexpected ways because the nodes themselves also carry some resolution and line number info and those should NOT be duplicated.

So, you know. Great idea. We had it too. But... :(

[rzwitserloot]

Addressed by explicitly generating something like: "this.x = TypeName.$default$x();" in any generated no-args constructor, which is the best we can do here. Should address all the issues, right?

[rzwitserloot]

edge release available with this update: https://projectlombok.org/download-edge

[citDennis]

Thanks Ronald!

[janrieke]

Great, that should fix most of the problems. I'll adapt it to @SuperBuilder (which currently keeps the init expression on the instance variable).

What remains is that it still might be surprising (although now documented) that the initialization will not take place for manual constructors.
So could injecting an assignment with the method call as rhs at the beginning of each manual constructor be a solution (of course only for constructors that do not assign a value themselves)?

[PSanetra]

@rzwitserloot This fix is an improvment to the current situation, but as @janrieke states, the issue still exists for manual constructors. @rspilker mentions in https://github.com/rzwitserloot/lombok/pull/1429#issuecomment-312095098 that the original issue, why the initializer code is removed in the first place, is about possible expensive double initializations. I think these complex operations on initialization, while also using the builder pattern, should be considered rare edge cases and in most situations these double initializations should not be a problem. Maybe you can introduce an annotation parameter on Builder.Default to opt-out of or (to prevent breaking changes) to opt-in to double initialization.

[BGehrels]

@rzwitserloot Great, thank you! How long does it typically take for such an edge release to be available on maven central (aka stable)?

[flomin]

This issue doesn't seem to be fixed in the last 1.18.2 release ?
Using IntelliJ lombok plugin version 0.18-2018-1 or 0.19-2018-1
Using org.projectlombok:lombok-maven-plugin:1.18.0.0
Using maven 3.5.3

Doesn't work neither in IntelliJ nor directly in mvn command line.
Generated class as empty constructor with Builder.Default used on a list = new ArrayList<>();

[tyrion9]

This issue is fixed in version 1.18.2

<dependency> <groupId>org.projectlombok</groupId> <artifactId>lombok</artifactId> <version>1.18.2</version> </dependency>

`import lombok.AllArgsConstructor;
import lombok.Builder;
import lombok.Data;
import lombok.NoArgsConstructor;

@Data
@Builder
@NoArgsConstructor
@AllArgsConstructor
public class Test {
public static final int DEFAULT_INT = 42;
public enum Status {
STATUS_ONE,
STATUS_TWO;
}
@Builder.Default
private Integer someInt = DEFAULT_INT;

@Builder.Default
private Status currentStatus = Status.STATUS_ONE;

public static void main(String[] args) {
    Test test = new Test();
    System.out.println("test = " + test);
}

}`

Result:
test = Test(someInt=42, currentStatus=STATUS_ONE)

[alturkovic]

I believe this still does not work as intended.

Here is a simple Person class:

@Data
@Builder
@NoArgsConstructor
@AllArgsConstructor
public class Person {

  @Builder.Default
  private String name = "John Doe";

  @Builder.Default
  private int age = 25;

  public Person(final String name) {
    this.name = name;
  }
}

And a simple class to print out the possible combinations:

public class PersonTest {
  public static void main(final String[] args) {
    System.out.println(new Person()); // Person(name=John Doe, age=25)
    System.out.println(new Person("John Watson")); // Person(name=John Watson, age=0)
    System.out.println(new Person("Sherlock Holmes", 30)); // Person(name=Sherlock Holmes, age=30)
    System.out.println(Person.builder().build()); // Person(name=John Doe, age=25)
    System.out.println(Person.builder().name("Jane Doe").build()); // Person(name=Jane Doe, age=25)
    System.out.println(Person.builder().name("Jane Doe").age(18).build()); // Person(name=Jane Doe, age=18)
  }
}

Notice the second example where age is 0. I believe this is a bug and it should be 25. If you remove @Builder and @Builder.Default annotations (and of course the non-compiling builder examples) the second example will have age set to 25 as expected.

I have run these examples with the 1.18.2 version.

[tyrion9]

@alturkovic, you're right. It's another obvious bug.
At least now, it solved NoArgsConstructor annotation.

[eekboom]

Gah. I am going crazy.
We decided against using @Builder.Default for the strange bugs it introduces.
So instead we are using a skelton for the builder class that initializes the fields.

Now lombok emits a warning " @Builder will ignore the initializing expression entirely. If you want the initializing expression to serve as default, add @Builder.Default. If it is not supposed to be settable during building, make the field final."
Because we have configured our builds to fail on warnings (to keep our code clean) this is a real problem for us.
Any way to disable that warning?

[goostleek]

Yes @eekboom. The best option is to not use Builder.Default ATM. However, you can achieve the same functionality (as we did) with the approach described here.

[janrieke]

I am aware that this is still a problem for manual constructors (now for both @Builder and @SuperBuilder). In a previous post I suggested a possible solution, which I think is implementable and does not have too many drawbacks.
We need a word from the maintainers here on how to proceed.

[EvanNGoldstein]

Any update on this? Spent entirely too much time today trying to figure out why my @Builder.Default field was null when using my own constructor.

[goostleek]

You can try this workaround as a remedy until the fix is released.

[twilco]

@goostleek - I tried that workaround, but then realized that our classes that use @Builder also use @Singular, which breaks our existing code that assumes the existence of singular builder fields. Is there some workaround that accounts for the existence of @Singular fields?

[goostleek]

I cannot see the issue. Could you please provide an MWE (with a unit test) for the problem?

[twilco]

Oops, the solution is easy...

For anyone who needs to use @Singular in conjunction with the workaround proposed in the previously linked StackOverflow comment, do this:

@Getter
@Setter
@NoArgsConstructor
public class SearchRequest {
    // This is the field I want to default
    private Context context = Context.default;

    // @Singular
    // We had a singular here, but it will no longer work since the @Builder annotation 
    // was moved from the `public class` above to the below private constructor
    private List<SearchFilter> filters = new ArrayList<>();

    @Builder
    private SearchRequest(SearchContext context,
                          // move your @Singular annotation here
                          @Singular List<RequestFilter> filters) { 
        this.searchContext = Optional.ofNullable(context).orElse(this.searchContext);
        this.filters = filters;
    }
}

[goostleek]

~Nice! I have updated the SO answer by referencing your solution in the comment 👍 .~

I saw you already did it, removing... 🤡

[MT-Jacobs]

Is the issue found in https://github.com/rzwitserloot/lombok/issues/1347#issuecomment-415328497 being tracked in this closed ticket, or was there a separate ticket made for the new issue?

[janrieke]

There was a discussion in the forum on how manual constructors could be modified to mitigate that issue: https://groups.google.com/forum/#!topic/project-lombok/e9PzKXRlXXA
In it, @rzwitserloot gave some examples where there simply is no nice solution. Quoting him: "doing it right seems utterly impossible to me".

Thus, I vote for just emitting a warning if there is no assignment to that field in the manual constructor. You would have to fix the warning by adding an assignment. In my opinion, this would prevent most bugs. A drawback is that if you _intentionally_ want it uninitialized, you'd have to add an otherwise unnecessary line; however, this line makes the code easier to understand for non-expert users of lombok.
But everything is up to the lombok maintainers here.

[alturkovic]

It would be best to remove the feature alltogether, it is dangerous and causes a really hard to track bug. Best bet is to avoid it IMHO.

[gavenkoa]

@Builder.Default and custom constructor are incompatible ((

I don't need verification that all field of @Builder are set.

But I'd like to have blueprint method with @Builder(toBuilder = true).

So I rewrote code with constructors replaced by of with builders:

@Builder
public class Connection {
    private String user;
    private String pass;

    @Builder.Default
    private long timeout = 10_000;

    @Builder.Default
    private String port = "8080";

    public static Connection of(String user, String pass) {
        return Connection.builder()
            .user(user)
            .pass(pass)
            .build();
    }

    public static Connection of(String user, String pass, String port) {
        return Connection.builder()
            .user(user)
            .pass(pass)
            .port(port)
            .build();
    }

    public static Connection of(String user, String pass, String port, long timeout) {
        return Connection.builder()
            .user(user)
            .pass(pass)
            .port(port)
            .timeout(timeout)
            .build();
    }
}

It's a shame that I forced to create extra Builder object instead of single one with native new ((

[Maaartinus]

@gavenkoa I surely agree that there's something wrong, but your solution seems to be too complicated as for n variables, you need O(n²) lines. I thought, you could define a private @Wither and use

    public static Connection of(String user, String pass, String port, long timeout) {
        return Connection.of(user, pass).withTimeout(timeout);
    }

but this would create even more garbage. But what about chained setters like

    public static Connection of(String user, String pass, String port, long timeout) {
        return Connection.of(user, pass).setTimeout(timeout);
    }

? Sure, it's still O(n²), but not lines.

Actually, with mutable objects, I always wonder what anybody needs builder for?

[gavenkoa]

@Maaartinus @Wither is still experimental feature. Docs says that they can even changes name ((

I don't need builders, I just like chaining and setters chaining breaks Bean API ((

[Maaartinus]

@gavenkoa You're right concerning @Wither being experimental. But it's experimental since 2012 and nobody has come with a better name since then. It's one of my favorites.

@rzwitserloot IMHO, @Wither should be promoted out of experimental, WDYT?

setters chaining breaks Bean API

I see. With the beanscrap, there's no solution. The Lombok team refuses to implement both kinds of setters as this is an ugly API (and I agree; cure worse than the disease). With the Builder, it's ugly in a different, maybe nicer, way.

[rspilker]

We plan to do some work on @Wither soon. So it's going to stay in experimental for a while.

The idea is to improve the usability for Persistent Data Structures.

We're considering the following alternatives, possible we can support all of the,

@RequiredArgsConstructor
class Point {
  @Wither final int x;
  final int y;
}

class Point {
  final int x;
  final int y;

  Point(int x, int y) {
    this.x = x;
    this.y = y;
  }

  // Current implementation
  Point withX(int x) {
    return new Point(x, this.y);
  }

  // Alternative 1
  Point withX(Function<Integer, Integer> xFunction) {
    return new Point(xFunction.apply(this.x), this.y);
  }

  // Alternative 2
  Point withX(Function<Point,Integer> xFunction) {
    return new Point(xFunction.apply(this) this.y);
  }
}

[jasonCodesAway]

This is a long and complicated comment history. Was this issue closed as will not be fixed? I'm definitely hitting this issue and currently trying this workaround from SO, which indicates this is broken

[asserebryanskiy]

I currently can use both @NoArgsConstructor and @Builder with @Builder.Default and some other constructor this way:

@NoArgsConstructor
@AllArgsConstructor
@Builder
public SomeClass {
  @Builder.Default
  private int someInt = 0;
  @Builder.Default
  private String someString = "initial value";

  public SomeClass(int someInt) {
    this();
    this.someInt = someInt;
  }
}

This way all three constructors and builder correctly initialise fields.

My lombok version is 1.18.2

[rzwitserloot]

@asserebryanskiy cool trick – unfortunately, does not work for final fields.

[rzwitserloot]

@jasonCodesAway no, it was closed because it is fixed. However, right now manually written constructors do still run into the lack of default value issue. The only fix is for us to go in there and manually add a line which is complicated, but @janrieke has a plan. We'll talk about it next time we triage bugs and features. Re-opening, specifically for __MANUALLY__ written constructors.

It'll be hard to do this properly, see commentary on https://groups.google.com/forum/#!topic/project-lombok/e9PzKXRlXXA – but at the very least we should put in some effort to detect that this is happening (a manual constructor referring to a field with a defaulting annotation on there) and emit a warning.

[randakar]

How about just creating a new bug for that? Neither the title not the
problem description really match the issue left over ;)

On Fri, Jan 17, 2020, 04:24 Reinier Zwitserloot [e-mail]
wrote:

@jasonCodesAway https://github.com/jasonCodesAway no, it was closed
because it is fixed. However, right now manually written constructors do
still run into the lack of default value issue. The only fix is for us to
go in there and manually add a line which is complicated, but @janrieke
https://github.com/janrieke has a plan. We'll talk about it next time
we triage bugs and features. Re-opening, specifically for MANUALLY
written constructors.

—
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/1347?email_source=notifications&email_token=AABIERLIPOJ7YSZEIEROM73Q6EQINA5CNFSM4DFS7BUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJGKE3I#issuecomment-575447661,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AABIERMMFBWMZKOXK4GE65TQ6EQINANCNFSM4DFS7BUA
.

[rzwitserloot]

Moved to new feature #2340