Feature Proposal: Support for Bidirectional References in Builders

[Jan Rieke]

Consider you want to have two immutable value classes that should have a bidirectional association between them; let's call them Container and Content:

public class Container {
    private final Content content;
    // ... more fields ...
}

public class Content {
    private final Container container;
    // ... more fields ...
}

Using @AllArgsConstructors for these classes, you have a problem: You need a reference to the other instance as argument for both constructors, but one instance has to be created before the other, so that's impossible.

If you manually write one of your constructors, you could create the other instance within this constructor. However, then you have to put all fields of both classes into that constructor. That may lead to a large number of parameters, and could quickly get confusing (which parameter belongs to which class?).

A possible solution is to use builders. As a builder represents the future state of the object to be, you can simply pass a builder as argument to one of the constructors. For instance, the constructor for Container gets a ContentBuilder as parameter. The constructor fills in the "back reference" in that builder, calls build() to create the Content instance, and set the result as its "forward reference".

I propose the following syntax (all names preliminary and subject to discussion):

@SuperBuilder
public class Container {
    @SuperBuilder.BidirectionalReference(backReferenceSetterName = "container")
    private final Content content;
}

@SuperBuilder
public class Content {
    private final Container container;
}

Lombok will generate the following code for Container (for simplicity, I ommitted the type params and ContainerBuilderImpl class stuff):
   

public class Container {
    public static class ContainerBuilder {
        private Content.ContentBuilder contentBuilder;

        public ContainerBuilder content(Content.ContentBuilder contentBuilder) {
            this.contentBuilder = contentBuilder;
            return this;
        }

        public Container build() {
            return new Container(this);
        }
    }

    public Container(ContainerBuilder builder) {
        this.content = builder.contentBuilder != null ? builder.contentBuilder.container(this).build() : null;
    }

    private final Content content;
}

The generated builder code for Content remains as it is right now.

What's happening is that the builder's setter method for content now has a ContentBuilder as parameter. (As @SuperBuilder builder classes cannot be renamed, lombok knows the name of the builder class without resolution.)
In the constructor, the instance-in-creation (this) is set as "back reference" within the ContentBuilder. Because lombok cannot look into the other class, it cannot know the name of this back reference. Thus, this needs to be a parameter of the new @SuperBuilder.BidirectionalReference annotation.
Finally, the Container constructor creates the new Content instance using the build() method and sets the result as "forward reference".

This approach could also be extended to support 1-to-n relationships. The builder's setter will then get a list of ContentBuilders instead of a list of Contents. It may also be combinable with @Singular, but this will certainly be a later stage of development. Note that implementing it for @Builder will be problematic though, mainly because it would require changing from an all-args constructor to a custom constructor, which could easily break existing code.

What do you think? Is that a use-case worth supporting? Are there any problems with it I don't see?

[Martin Grajcar]

There's a lengthy conversion about this in #1623. My personal conclusion was that it's far too complicated to be worth the effort, but I may be wrong.

 

This approach could also be extended to support 1-to-n relationships.

Immutables are nice, but you can have an arbitrary complicated and huge graph and when you want to change anything, then you have to clone the whole graph. So when I add an item to a catalog, I have to clone all items (because of their back-references) and the catalog (container) having then an immutable set of n+1 items. This sounds pretty inefficient and cloning of the items can't be avoided (the immutable set can be implemented efficiently using a "persistent" data structure like in clojure).

As withing this feature, Lombok creates a circular structure, it should also generate non-stack-overflowing @EqualsAndHashCode and @ToString out of the box. So I'd call @SuperBuilder.BidirectionalReference just @BidirectionalReference (or maybe @Backreference) as it'd influence other annotations too. See this comment of mine.

The builder's setter will then get a list of ContentBuilders instead of a list of Contents. It may also be combinable with @Singular, but this will certainly be a later stage of development. Note that implementing it for @Builder will be problematic though, mainly because it would require changing from an all-args constructor to a custom constructor, which could easily break existing code.

What do you think? Is that a use-case worth supporting? Are there any problems with it I don't see?

I'm sceptical of the usability because of the potentially huge inefficiency (something like @With would have to clone the whole object graph). Anyway, a support for @EqualsAndHashCode and @ToString for circular structures will be needed and that's what I'd start with (maybe based on @BidirectionalReference, maybe automated). Just my €0.018456.

[Jan Rieke]

On Wednesday, April 22, 2020 at 12:38:10 AM UTC+2, Maaartin G wrote:

There's a lengthy conversion about this in #1623. My personal conclusion was that it's far too complicated to be worth the effort, but I may be wrong.

Oh boy, I totally missed that. :)

One point from that discussion is whether the builder should accept both instance and builder for the same attribute. That makes it more complex, but I doubt that it's really necessary. If you want a bidirectional association, why would you allow it to be inconsistent?

The other thing is the stuff with $lombokLastInstance. That means that the builder will _not_ be reusable to create a second similar instance, although that's only relevant for non-immutable classes (so probably a minor issue in this case). However, the advantage of that approach is that it allows circles of more than two instances (a -> b -> c -> a). That's nice, but that approach is really tedious to implement. I'll think about that.

 

 

This approach could also be extended to support 1-to-n relationships.

Immutables are nice, but you can have an arbitrary complicated and huge graph and when you want to change anything, then you have to clone the whole graph. So when I add an item to a catalog, I have to clone all items (because of their back-references) and the catalog (container) having then an immutable set of n+1 items. This sounds pretty inefficient and cloning of the items can't be avoided (the immutable set can be implemented efficiently using a "persistent" data structure like in clojure).

True. Both approaches won't work with the current @With implementation anyway. It's currently only for use-cases where you just want immutability, but don't want to modify anything.

I did not think through the toBuilder() part of it, but my impression is that it's possible. That could be a solution for @With, too. However, you are right that this will a inefficient solution for larger graphs, but that will be every solution with immutable value classes. So in such a case developers will always have to trade off immutability against efficiency.

 

As withing this feature, Lombok creates a circular structure, it should also generate non-stack-overflowing @EqualsAndHashCode and @ToString out of the box. So I'd call @SuperBuilder.BidirectionalReference just @BidirectionalReference (or maybe @Backreference) as it'd influence other annotations too. See this comment of mine.

Yep, it's definitely necessary to cover @EqualsAndHashCode and @ToString. Problem is that you'd probably want to exclude the back reference in most cases, but the annotation is only on the forward reference. Resolution, you know... ;)

[Martin Grajcar]

On Thu, Apr 23, 2020 at 11:51 PM Jan Rieke <jan.rieke...@gmail.com> wrote:

On Wednesday, April 22, 2020 at 12:38:10 AM UTC+2, Maaartin G wrote:

There's a lengthy conversion about this in #1623. My personal conclusion was that it's far too complicated to be worth the effort, but I may be wrong.

Oh boy, I totally missed that. :)

One point from that discussion is whether the builder should accept both instance and builder for the same attribute. That makes it more complex, but I doubt that it's really necessary. If you want a bidirectional association, why would you allow it to be inconsistent? 

You could accept an instance and use a fixed copy of it, so consistency is restored. But then you could let the user call toBuilder() instead.

 

The other thing is the stuff with $lombokLastInstance. That means that the builder will _not_ be reusable to create a second similar instance, although that's only relevant for non-immutable classes (so probably a minor issue in this case). However, the advantage of that approach is that it allows circles of more than two instances (a -> b -> c -> a). That's nice, but that approach is really tedious to implement. I'll think about that.

IIUIC (which I probably don't), the last instance is only relevant during some build(). Afterwards you can reset it.

This approach could also be extended to support 1-to-n relationships.

Immutables are nice, but you can have an arbitrary complicated and huge graph and when you want to change anything, then you have to clone the whole graph. So when I add an item to a catalog, I have to clone all items (because of their back-references) and the catalog (container) having then an immutable set of n+1 items. This sounds pretty inefficient and cloning of the items can't be avoided (the immutable set can be implemented efficiently using a "persistent" data structure like in clojure).

True. Both approaches won't work with the current @With implementation anyway. It's currently only for use-cases where you just want immutability, but don't want to modify anything.

I did not think through the toBuilder() part of it, but my impression is that it's possible. That could be a solution for @With, too. However, you are right that this will a inefficient solution for larger graphs, but that will be every solution with immutable value classes. So in such a case developers will always have to trade off immutability against efficiency.

I'm afraid, immutability has to lose: I could imagine staring with a small object graph, where the cost is tiny and later switching to something big, where I'll find out that it was a mistake. This feels like a time-bomb...

 

As withing this feature, Lombok creates a circular structure, it should also generate non-stack-overflowing @EqualsAndHashCode and @ToString out of the box. So I'd call @SuperBuilder.BidirectionalReference just @BidirectionalReference (or maybe @Backreference) as it'd influence other annotations too. See this comment of mine.

Yep, it's definitely necessary to cover @EqualsAndHashCode and @ToString. Problem is that you'd probably want to exclude the back reference in most cases, but the annotation is only on the forward reference. Resolution, you know... ;)

IMHO you shouldn't exclude either, as any exclusion may lead to conflicting hashCode. Imagine a bunch of trivial two-object cycles A<->B1, A<->B2, A<->B3, .... All the As are actually different, as they contain a B each. However, with an exclusion, you'll get the same hashCode. Even worse, you'll get them equal, albeit they are not (the user may consider them equal or not, it depends). Note that I didn't say which reference is the forward one.

Therefore, I'd surely go for the ThreadLocal-solution. When the user considers the different As to be equal, they can exclude the B field manually. When they consider the As to be different, they need to visit the B, but only once. A naive implementation using a global variable would go like this:

public int hashCode() {

    if (!theGlobalIdentityHashSet.add(this)) return 42;
    try {

        process as usual

    } finally {

        theGlobalIdentityHashSet.remove(this);

    }

}

This is pretty universal, but not thread-safe. Adding a ThreadLocal solves it trivially. However, there are no globals in Java, just static fields of a class and there are no lombok classes at runtime. So the user would have to provide a configuration like

globalCycleProtection=my.packa.ge.MyLombokHelper

and Lombok would add its

public static ThreadLocal<IdentityHashSet> theGlobalIdentityHashSet = new ThreadLocal<>()

to the class and use it for the above code.

I guess, a variant of this could also automatically exclude back-references without resolution....