Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Builder should support fields from parent classes #853

Open
lombokissues opened this issue Jul 14, 2015 · 61 comments
Open

Builder should support fields from parent classes #853

lombokissues opened this issue Jul 14, 2015 · 61 comments

Comments

@lombokissues
Copy link
Collaborator

@lombokissues lombokissues commented Jul 14, 2015

Migrated from Google Code (issue 818)

@lombokissues

This comment has been minimized.

Copy link
Collaborator Author

@lombokissues lombokissues commented Jul 14, 2015

👤 elreydetodo   🕗 May 29, 2015 at 21:54 UTC

In a project I'm working on, I have an abstract base class with three sub-classes. Let's say that the base class has "id", "name", and "type" fields. I would expect one of the subclasses annotated with @ Builder to be able to set those fields in its builder implementation.

On a related note (though probably warranting a separate bug), @ AllArgsConstructor should probably take fields from parent classes into account.

I think it would be reasonable for this to be an option I have to pass to the annotation (i.e. @ Builder(includeFromParent=true)).

@lombokissues

This comment has been minimized.

Copy link
Collaborator Author

@lombokissues lombokissues commented Jul 14, 2015

👤 Maaartinus   🕗 May 30, 2015 at 07:15 UTC

For Builder, see
https://groups.google.com/d/msg/project-lombok/1S7Y7sHXZS8/Rdzj_U4Tvi0J

For why it's not implemented yet, see
https://groups.google.com/d/msg/project-lombok/-6b9dPH8qAw/0rzZW6ZAgJIJ

Related problem
issue #740

@lombokissues

This comment has been minimized.

Copy link
Collaborator Author

@lombokissues lombokissues commented Jul 14, 2015

End of migration

@smougenot

This comment has been minimized.

Copy link

@smougenot smougenot commented Oct 2, 2015

Would be nice to include the workaround in the official documentation (on @builder)
ex :https://www.donneo.de/2015/09/16/lomboks-builder-annotation-and-inheritance/

@AllArgsConstructor
public class Parent {
  private String a;
}

public class Child extends Parent {
  private String b;

  @Builder
  public Child(String a, String b){
    super(a);
    this.b = b;
  }
}

public static void main(String[] ...){
  Child.builder().a("testA").b("testB").build();
}
@msarniak

This comment has been minimized.

Copy link

@msarniak msarniak commented Oct 15, 2015

Builder pattern that supports inheritance without the chicken and egg problem - not the cleanest code, cluttered with generics, but since this is to be generated, maybe worth considering?

public class Parent {

    private final int a;

    protected Parent(final AbstractParentBuilder builder) {
        this.a = builder.a;
    }

    public static final class Builder extends AbstractParentBuilder<Parent, Builder> {
        public Parent build() {
            return new Parent(this);
        }
    }

    protected static abstract class AbstractParentBuilder<T extends Parent, B extends AbstractParentBuilder<T, ?>> {

        private int a;

        public B withA(int a) {
            this.a = a;
            return (B) this;
        }
    }
}


public class Child extends Parent {

    private final int b;

    protected Child(final AbstractChildBuilder builder) {
        super(builder);
        this.b = builder.b;
    }

    public static final class Builder extends AbstractChildBuilder<Child, Builder> {
        public Child build() {
            return new Child(this);
        }
    }

    protected static abstract class AbstractChildBuilder<T extends Child, B extends AbstractChildBuilder<T, ?>> extends Parent.AbstractParentBuilder<T, B> {

        private int b;

        public B withB(int b) {
            this.b = b;
            return (B) this;
        }
    }
}

With that, code below is valid:

Child child = new Child.Builder().withB(1).withA(2).build();

We may get rid of casts, adding some abstract self() method, implemented in concrete Builders.

@shakuzen

This comment has been minimized.

Copy link

@shakuzen shakuzen commented Dec 10, 2015

I came here from peichhorn/lombok-pg#78, which has a ton of +1s. I hope the maintainers here are aware of this.

@fahran

This comment has been minimized.

Copy link

@fahran fahran commented Jan 7, 2016

+1

9 similar comments
@afedulov

This comment has been minimized.

Copy link

@afedulov afedulov commented Jan 29, 2016

+1

@ywilkof

This comment has been minimized.

Copy link

@ywilkof ywilkof commented Jan 29, 2016

+1

@electrobabe

This comment has been minimized.

Copy link

@electrobabe electrobabe commented Feb 1, 2016

+1

@gaetancollaud

This comment has been minimized.

Copy link

@gaetancollaud gaetancollaud commented Feb 1, 2016

+1

@troymolnar

This comment has been minimized.

Copy link

@troymolnar troymolnar commented Feb 9, 2016

+1

@Lactem

This comment has been minimized.

Copy link

@Lactem Lactem commented Feb 16, 2016

+1

@laxika

This comment has been minimized.

Copy link

@laxika laxika commented Mar 2, 2016

+1

@jkocot

This comment has been minimized.

Copy link

@jkocot jkocot commented Mar 2, 2016

+1

@Yeggstry

This comment has been minimized.

Copy link

@Yeggstry Yeggstry commented Mar 11, 2016

+1

@rspilker

This comment has been minimized.

Copy link
Collaborator

@rspilker rspilker commented Mar 25, 2016

includeFromParent=true is not trivial since it requires resolution. From now on, please use the thumbs up button under the first comment to share your vote.

@marcuss

This comment has been minimized.

Copy link

@marcuss marcuss commented Apr 13, 2016

@rspilker there is a lot of other upvotes for this, but people made those in the wrong repo: peichhorn/lombok-pg#78 (comment)

@Insomnium

This comment has been minimized.

Copy link

@Insomnium Insomnium commented May 25, 2016

+1

7 similar comments
@hokkun-dayo

This comment has been minimized.

Copy link

@hokkun-dayo hokkun-dayo commented Jun 2, 2016

+1

@christopherbaek

This comment has been minimized.

Copy link

@christopherbaek christopherbaek commented Jun 13, 2016

+1

@iwaltgen

This comment has been minimized.

Copy link

@iwaltgen iwaltgen commented Jun 21, 2016

+1

@rmordillo-witbooking

This comment has been minimized.

Copy link

@rmordillo-witbooking rmordillo-witbooking commented Jul 6, 2016

+1

@xiemeilong

This comment has been minimized.

Copy link

@xiemeilong xiemeilong commented Jul 8, 2016

+1

@jeffochen

This comment has been minimized.

Copy link

@jeffochen jeffochen commented Jul 19, 2016

+1

@leanrobot

This comment has been minimized.

Copy link

@leanrobot leanrobot commented Jul 29, 2016

+1

@driverpt

This comment has been minimized.

Copy link

@driverpt driverpt commented Aug 8, 2016

+1.

Workaround

@gaetancollaud

This comment has been minimized.

Copy link

@gaetancollaud gaetancollaud commented Aug 8, 2016

@driverpt Thanks for the workaround. But you have to generate the constructor for all fields, which can be huge for some entity classes! Too bad the @AllArgConstructor doesn't support inheritance either. 😝

@witek1902

This comment has been minimized.

Copy link

@witek1902 witek1902 commented Sep 7, 2016

+1

@aonischenko

This comment has been minimized.

Copy link

@aonischenko aonischenko commented Nov 8, 2016

+1

1 similar comment
@wingsofovnia

This comment has been minimized.

Copy link

@wingsofovnia wingsofovnia commented Nov 30, 2016

+1

@janrieke

This comment has been minimized.

Copy link
Contributor

@janrieke janrieke commented Dec 9, 2016

This issue cannot be fixed without major changes to Lombok, i.e., deferring Lombok processing to after the sematic analysis of the compiler. Thus, we have investigated on workarounds to this problem. This is the solution we came up with.

Let's assume we have two classes: a superclass Parent, and a subclass Child.
It is impossible to access ANY kind of type information from the superclass while generating the Builder for the subclass. Therefore, we generate a builder for the superclass as well.
The idea is that we use this superclass builder to inject its values into the subclass instance.

Superclass:

@NoArgsConstructor
@Builder(builderMethodName="parentBuilder", applyOn=true) 
public abstract class Parent {
	@Getter @Setter
	private String parentString;

	@Getter @Setter @Singular
	private List<String> parentValues;
}

Subclass:

@Builder
public class Child extends Parent {
	@Getter @Setter
	private String childString;

	@Getter @Setter @Singular
	private List<String> childValues;
}

How to use:

Child child = Child.builder()
		.childString("childValue")
		.childValue("childValue1")
		.childValue("childValue2")
		.build();
Parent.parentBuilder()
		.parentString("parentValue")
		.parentValue("parentValue1")
		.parentValue("parentValue2")
		.applyOn(child);

Remarks:

  • Immediate consequence of having builders for each class in the inheritance hierarchy is that only one builder can be used as the real builder that creates the instance using a constructor. All other builders just have to be "applied on" the existing instance.
  • The applyOn() method relies on assigning values to the instance fields directly. If @Builder is placed on a constructor whose parameters are named differently than the class' fields, the applyOn() method cannot be generated.
  • Builders must have different builderMethodNames in each class in the hierachy.
  • We changed that @Builder also compiles on abstract classes, simply by omitting including the build() method.
  • The name of the applyOn() method can be customized via annotation parameter.

We implemented this for the Eclipse builder, and it works quite well. :-)
@rspilker, @rzwitserloot: Is this something you would consider to include? If this is the case, we would continue working on that, especially by providing a javac implementation.

@el-dot

This comment has been minimized.

Copy link

@el-dot el-dot commented Dec 29, 2016

You can invert Builder pattern and take builder instance as the param of constructor, this way Builder can be easily inherited, with added params in child class. So you could add @InheriteBuilder, some annotation field, or something like that to handle inheritace 'resolution' explicitly by user not compiler.

class A {
    final int x;
    A(Builder b) {
        this.x = b.x;
    }

    static class Builder {
        public int x;
        A build() {
            return new A(this);
        }
    }
}

class B extends A {
    final int y;
    B(Builder b) {
        super(b);
        this.y = b.y;
    }

    static class Builder extends A.Builder {
        public int y;

        @Override B build() {
            return new B(this);
        }
    }
}

I removed setters on builder for brevity;

@jonas-lauber

This comment has been minimized.

Copy link

@jonas-lauber jonas-lauber commented Jan 23, 2017

+1

2 similar comments
@disciple97

This comment has been minimized.

Copy link

@disciple97 disciple97 commented Feb 17, 2017

+1

@zhxiaogg

This comment has been minimized.

Copy link

@zhxiaogg zhxiaogg commented Feb 28, 2017

+1

@thekalinga

This comment has been minimized.

Copy link

@thekalinga thekalinga commented Mar 2, 2017

Can you guys stop posting these useless +1 at the bottom, wasting both your and everyone else's time who have subscribed to this page.

If you want the issue to get more attention, post a thumbs up for the first post, not at the bottom

@janrieke

This comment has been minimized.

Copy link
Contributor

@janrieke janrieke commented Mar 15, 2017

I created a pull-request (#1337, hehe) that addresses this issue.

I followed the idea of @Krzychek mentioned before: Generate a constructor on the type that takes a builder as a parameter and uses the fields from the builder to set the fields of the new instance. The Builder.build() method calls this constructor.

The feature only works for type builders and must be explicitly switched on, in order to not break existing implementations, which may rely on the all-args constructor generated by @Builder.

Example:

@Builder(extendable = true)
public class Parent {
	@Getter
	private String parentString;
}

@Builder(inherit = true)
public class Child extends Parent {
	@Getter
	private String childString;
}

public class TestBuilder {
	public static void main(String[] args) {
		Child child = (Child) Child.builder()
				.childString("childStringValue")
				.parentString("parentStringValue")
				.build();
		assert("childStringValue".equals(child.getChildString()));
		assert("parentStringValue".equals(child.getParentString()));
	}
}
@janrieke

This comment has been minimized.

Copy link
Contributor

@janrieke janrieke commented Apr 20, 2017

It would be great to get some feedback from the project developers. If my PR has a chance of getting included, I'll fix the current conflicts in the PR to make it mergeable again, and/or improve the code if you have any remarks.
Furthermore, I could try implementing @msarniak's generics idea to get rid of the cast.

@thekalinga

This comment has been minimized.

Copy link

@thekalinga thekalinga commented May 13, 2017

@rzwitserloot Any response to the above query?

@Komdosh

This comment has been minimized.

Copy link

@Komdosh Komdosh commented Jun 14, 2017

+1

1 similar comment
@kevcodez

This comment has been minimized.

Copy link

@kevcodez kevcodez commented Jun 16, 2017

+1

@nedkoh

This comment has been minimized.

Copy link

@nedkoh nedkoh commented Aug 29, 2017

Well the workaround(s) provided so far are not that useful nor great. Here is an example:
I'm trying to create inheritance with a contract from an interface and default implementation for properties to share and at the end have an immutable object. 3 things that help me accomplish that are: the inheritance (for providing the fields/constructor), the builder pattern (making things immutable), and lombok (elimination of writing boilerplate code).
What has been suggested is basically breaking at least one of my goals (and more likely 2). If I am going to write my own constructors I am not making things clean, saving much time with lombok, and I'm also defeating the purpose of inheritance.

@Lactem

This comment has been minimized.

Copy link

@Lactem Lactem commented Aug 30, 2017

Agreed. There are still no ideal workarounds.

@nedkoh

This comment has been minimized.

Copy link

@nedkoh nedkoh commented Sep 27, 2017

Please stop submitting +1 to the request - it only results in unnecessary comments and pollutes the post. If you agree/disagree please use proper comment/reaction/emoji above.

@zygimantus

This comment has been minimized.

Copy link

@zygimantus zygimantus commented Dec 15, 2017

Still no progress in this issue?

@derfloh205

This comment has been minimized.

Copy link

@derfloh205 derfloh205 commented Jan 9, 2018

+1

@faxxe

This comment has been minimized.

Copy link

@faxxe faxxe commented Jan 9, 2018

+2 ;)

@xwhfcenter

This comment has been minimized.

Copy link

@xwhfcenter xwhfcenter commented Jan 16, 2018

+1

@sumeettakalkar

This comment has been minimized.

Copy link

@sumeettakalkar sumeettakalkar commented Feb 2, 2018

I see a lot of +1s, but is this issue fixed?

bascarsija pushed a commit to bascarsija/grafana-api-java-client that referenced this issue Feb 7, 2018
bascarsija pushed a commit to bascarsija/grafana-api-java-client that referenced this issue Feb 7, 2018
…t/lombok#853 and rzwitserloot/lombok#1337) to ensure fluent usage patterns illustrated in GrafanaClientDashboardIntegrationTest continue to work
@nsidhaye

This comment has been minimized.

Copy link

@nsidhaye nsidhaye commented Apr 2, 2018

@sumeettakalkar Issue is still open. #1337 having good discussion & solutions.

I am not seeing any clean solution. May be I need to use lombok without builder pattern as I my project is using heavy use of inheritance.

Previously I thought it would be easy or may be I can create some hack around @builder. Any way it is worth to watch #1337

@janrieke Thanks for PR.

@timothyBrake

This comment has been minimized.

Copy link

@timothyBrake timothyBrake commented Apr 10, 2018

+1 please support this

@janrieke

This comment has been minimized.

Copy link
Contributor

@janrieke janrieke commented Apr 10, 2018

There is a new PR coming with a modified approach, as my first approach still required casting, which simply doesn't look good.
Posting those "+1" will not speed things up. Please just be patient for a little more.

@janrieke

This comment has been minimized.

Copy link
Contributor

@janrieke janrieke commented Jun 1, 2018

New pull request #1711 with an improved approach.

@vadipp

This comment has been minimized.

Copy link

@vadipp vadipp commented Oct 4, 2019

Seems like #1711 is released as experimental in version 1.18.2, so this issue may be closed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.