Unexpected behavior of customized @Builder when @Builder.Default is used

[michal-kaciuba]

When customizing @builder like this:

@Getter
@AllArgsConstructor
@Builder
public class MessagePublishRequest {
	private String recipient;
	private Map<String, Object> payload;
	private String control;
	private MessageType type;
	@Builder.Default
	private LocalDateTime scheduledFor = LocalDateTime.now();

	public static class MessagePublishRequestBuilder {
		private final static ScheduleTimeFactory scheduleTimeFactory = new ScheduleTimeFactory();

		public MessagePublishRequestBuilder between(LocalDateTime from, LocalDateTime to) {
			scheduledFor = scheduleTimeFactory.scheduleBetween(from, to);
			return this;
		}
	}
}

The following test fails:

def "should schedule within given period"() {
        given:
        def from = LocalDateTime.now().plusDays(2)
        def to = LocalDateTime.now().plusDays(3)

        def request = MessagePublishRequest.builder()
                .between(from, to)
                .build()


        expect:
        request.scheduledFor.isAfter(from) || request.scheduledFor.isEqual(from)
        request.scheduledFor.isBefore(to)
    }

One might be surprised by an unexpected behavior of the code above. The reason is that between() is not setting scheduledFor$set = true so scheduledFor is overwritten by the default value on build(). The correct usage would be to use MessagePublishRequestBuilder::scheduledFor() instead of setting the field directly like this:

public MessagePublishRequestBuilder between(LocalDateTime from, LocalDateTime to) {
	scheduledFor(scheduleTimeFactory.scheduleBetween(from, to));
	return this;
}

I wonder if such problem could be avoided or at least better documented.

[rzwitserloot]

Just FYI, you can make that between method a line shorter: return scheduledFor(scheduleTimeFactory.scheduleBetween(from, to)); :)

[rzwitserloot]

It's not a matter of documentation. It is a matter of making a distinction between 'feel free to mess with these' and 'hands off / don't mess with these unless you really know what you are doing', and then documenting how you can recognize the 'dont touch those' fields. Which kinda takes care of itself if we chuck a $ in there, because the (hopefully?) universally agreed upon java-ese for 'no touchy'.

Any field marked with @Singular or indeed @Builder.Default has custom behaviour associated with it, and should result in the fields used for these to be hands-off!-ized with a dollar. @Singular already does this for maps. It should also do so for sets and lists. Any field protected by @Builder.Default already generated a dollarized $set boolean field, but should also dollarize the value field, presumably by appending $value to it.

If we apply this change it's gonna mess with all existing code that touches these fields directly, which is backwards incompatible, but the odds are increased that such code is doing the wrong thing already. Which makes the breaking change warranted.

I'm positively inclined to dollarizing ALL fields involved in special treatment, be it singular, or default.

The upcoming release doesn't have any breaking changes yet. Let's push this forward to 1.18.10.

[michal-kaciuba]

Good point @rzwitserloot. I imagine the perfect solution to make such mistakes impossible altogether would be to wrap the fields of the builder in a bean and use setters. That would make the delomboked code unreadable or at least overcomplicated. That's why I suggested just to put some documentation on this. I can agree that dollarizing the value field is a better idea and should solve the problem.

[Sam-Kruglov]

Just bumped into this breaking change. I thought you are supposed to increase major version for those, not patch version, or are you not following semver?
Also, IDEA plugin doesn't see those $value and $set fields

[EugenCepoi]

Would it be possible to set the default value of the field in the Builder to the same value as what's defined for the field annotated with Builder.Default? Instead of having a marker to check if it was set or not.

I find that it's super easy to fall into the trap when you add custom variants in the generated builder.
And there are no good ways around it. The only one would be to use a different name for the custom method, but that's not always great from an API perspective.

Another issue that makes it hard to have builder customizations+default values is because Lombok doesn't generate the different setters in the builder if there is a custom variant with the same name. If this was taking in consideration the full signature then people could delegate to the generated version in most situations.

[rzwitserloot]

@Sam-Kruglov project lombok does not believe semver as an absolutist concept makes sense. Almost any change is 'breaking', provided a sufficiently academic case.

We bump minor version if we feel the cases where things do break are plausible if exotic and it's not in an experimental feature (we don't as a rule break common stuff at all, of course). We did not find messing with the singular-generated field as plausible, but evidently some are doing this. We did consider that, but then realized that, as this bug report shows, most of those uses are probably broken already; making your compiler now error out on this is in fact doing you a favour.

Perhaps you disagree with our assessments, but no going back now.

Not seeing those fields is the point. You're not supposed to interact with these fields, what we do with em is too tricky to try to encode in a set-in-stone spec. If we did that, people would start relying on that behaviour. It's unfortunate we were unclear about how that's a bad idea, but we've fixed that now.

@EugenCepoi no, because the field(s) used in the builder may not even be of the same type. You can use @Tolerate to 'hide' a method from lombok.

[EugenCepoi]

Even if they are not of the same type, this doesn't necessarily prevent you from doing the defaulting in a different way that plays more nicely with custom code.

One problem is that you seem to do the defaulting in the generated build method, perhaps you could instead do it at initialization time.

Thanks for the pointer to @Tolerate, it does help with some of these problems.

[mikehalmamoj]

This has caused an incompatibility issue with Jackson. Jackson's BeanDeserializer looks for the $set version of the property at this line, doesn't find it and so doesn't override the default.

[mikehalmamoj]

My apologies ^^^ - turns out the test had a bug and the upgrade merely highlighted that bug. Nothing to see here, move along.

[rzwitserloot]

@EugenCepoi We can fix your case but then we break another case.

Imagine this scenario:

class Foo {
    private static final AtomicInteger UNIQUE_ID_COUNTER = new AtomicInteger(1000);
    @Builder.Default int id = UNIQUE_ID_COUNTER.incrementAndGet();
}

If lombok initializes the builder field with the default value, then we'd be incrementing that counter every time you invoke builder(), even if you then explicitly set the id counter.

We can do one of four things here:

1. Break the above case and fix your case.
2. Find a voodoo magic unicorn that backwards-compatibly fixes all concerns. I don't think it exists here.
3. Keep things as is (the above case continues to work, your case will continue to lead to confusion and bugs due to lack of awareness of the $set variable's existence).
4. Rejigger a few things in the API design. For example, dollarize ALL the fields and in general take away the ability to extensively mess with what builder makes, then find ways to restore needed functionality, such as: Add an explicit way to add alternative call scenarios. This would break backwards compatibility and be quite an undertaking.

Perhaps if we did it all over again we'd have shot down the 'the default expression has side effects' case and thus opened the door to ditch the $set variable. But we didn't. The decision is made now, so now unfortunately making the case for catering to your case and ditching the 'side effects' case has to bear the additional burden of justifying the backwards break. Whilst I can probably be convinced your case is more important than the side effects case, I don't think it's more important enough to warrant the break.

But here's one idea: It's a bit of work and therefore may not make it (there's so much to do, and not enough time to do it all, but we would accept PRs if this idea ends up being worthwhile), but what if lombok 'scans' the explicitly written builder class for any attempt to set that variable that has an associated $set boolean, and effectively demands that any write to that field is either immediately preceded by, or succeeded by, a write to the $set field, and failure to do so generates a compiler warning? So if you wrote this:

@Value @Builder class Whatever {
    @Builder.Default long x = System.currentTimeMillis();

    public static class WhateverBuilder {
        public WhateverBuilder x(Instant i) {
            this.x = i.toEpochMilli();
        }
    }
}

That lombok will generate a warning on the this.x = line. This warning goes away if that assignment has this.x$set = true either directly above or directly below it (in fact, this.x$set = [i do not care what you write here] is good enough, if you go out of your way to mess with it, I assume the author is aware of how lombok works and knows what they are doing, it doesn't have to be a true literal.. or should we demand a true literal?

It's a bit complicated (we'd have to scan for this.x, and in theory, WhateverBuilder.this.x, and x = but not if there is a local var shadowing x, etc - we don't have the benefit of the compiler having already sorted out what x refers to, so we'd have to write all that. That's why I'm pretty sure the value-for-cost analysis on this upgrade implies this upgrade is way down the list, but, assuming it's a good idea, at least we can then endorse 'PRs accepted' on it ;)

What do you think?

[rzwitserloot]

Thinking on it, there is a magic unicorn, but I'm not sure we want it: Instead of detecting if you fail to invoke $set = true, what if we... generated it for you? This is trickier than it seems like, e.g. we'd have to turn:

for (int i = 0; i < 10; i++) if (i == 5) x = 10;

into:
for (int i = 0; i < 10; i++) if (i == 5) {x = 10; x$set = true;}

and we'd have to flag as error something like:

for (x = 0; x <10; x++) { /* error - you can't set x in one of the 3 'for' components */)

as well as:

while (x = 0 != 10) /* error, can't use the assignment as expression */

but we could theoretically do all that.

[rspilker]

I would still be worried about the different ways to access the field (false negatives) and shadowing (false positives).

   foo.x = 7;

Should we fix that? Well, we might.

...
FooBuilder copyXInto(FooBuilder foo) {
    foo.x = this.x;
    foo.x$set = true;
}
...
void resetFoo() {
   x = 0;
   foo.x$set = false;
}
...

The only thing I can image is to disallow direct field modification, and generate private setters.

[EugenCepoi]

Hey thanks for giving this a second thought :)

Regarding the side effect use case, this feels somewhat unnatural to me. I'd expect it to have similar semantics as default values for attributes in Java, which does run that code anyway even if you provide a different value through the constructor or some other mean. I wonder in practice how common are either use cases. I can definitely say that the issue I mention occurred in the project I'm working on several times, to my self and colleagues of mine. And is not related to some technical aspect in our code base, but mostly just about how we imagine this API would work.

From an outside perspective the different options sound quite complex, I really would just imagine something that sets the default value "statically" which stays close to std Java and keep generating the setter if there isn't another one with the same signature.

That said, I've been using the Tolerate workaround so far and delegated to the generated setter which does the job, so I'm fine with that :) Thank you!