Basic support for builder inheritance

[drekbour]

This allows public class AppleBuilder extends FruitBuilder to be generated and, provided all withMethods override, used as an abstract FruitBuilder.

It's ugly but effective. One day it can be followed up with a change that generates only an interface from an abstract pojo.

[Adrodoc]

Does this handle exclusion of properties properly? If you exclude a superclass property I suspect that the coresponding with-method will not be overriden in the subclass builder. This is a problem because then the return type is that of the superclass and the method hast no effect. I can't check that right now, but I don't see a test for this scenario, so I guess you missed this. Possible solutions are:

• Post a compiler error at the annotation if a with method of the superclass builder is not overriden in the subclass builder
• Override the excluded methods and throw an UnsupportedOperationException

[Adrodoc]

Although it is not really important it might be good to reuse the fields in the superclass builder, but that might be too difficult.

[drekbour]

a) I seem to broken a test in my final "this little tidy up can't break anything" commit. Will have to look into that in a day or so. b) you are entirely correct about about excluding superclass properties and the whole basis of this working just because of method overriding. This is why I have not added anything to the README stating you can now inherit builders. The fact is it will now work in some vanilla circumstances but not in others. Genuinely inheriting the parent builder (with knowledge that is a PB) is possible but beyond my remit for this PR. I have taken a solid step in the right direction only.

[mkarneim]

Finally I got into this since I am working on #133.

As you stated, it would be better to have an interface generated instead of an abstract builder. This would solve the ugly parts of the generated abstract builder, like shadowing existing properties. And I guess, since I don't know you motivating use case, that an interface would be OK for any use of this.

I'll see if I can come up with something usable today.

[mkarneim]

Well, regardless what I try, I can't solve this one.

For example, I just did a checkout of your branch 'clone-fix' and modified your test scenario like this:

package net.karneim.pojobuilder.processor.with.builderinheritance;

import net.karneim.pojobuilder.GeneratePojoBuilder;

@GeneratePojoBuilder(withBuilderInterface = Builder.class)
public class Fruit {
  public String colour;
}

@GeneratePojoBuilder(withBaseclass = FruitBuilder.class, withBuilderInterface = Builder.class)
class Apple extends Fruit {
  public String variety;
}

As you can see I added withBuilderInterface = Builder.class to both annotations.

This gives an compilation error:

The interface Builder cannot be implemented more than once with different arguments: Builder<Fruit> and Builder<Apple>
AppleBuilder.java
/pojobuilder/src/testdata/java/net/karneim/pojobuilder/processor/with/builderinheritance
line 6  

This error would be the same in PB 3.5.0. That's why it is not possible to create a working builder hierarchy. It is still not possible with this branch.

[Adrodoc]

You can achieve that by using an assertj style selftype generic. This would require all superclass builders to be generic and everyone who is already using PB would geht a lot of raw type warnings when updating.

[Adrodoc]

For the classes Fruit and Apple:

@GeneratePojoBuilder(withBuilderInterface = Builder.class, generateCommonBuilderInterface=true)
public class Fruit {
  public String colour;
}
@GeneratePojoBuilder(withBuilderInterface = Builder.class, generateCommonBuilderInterface=true)
class Apple extends Fruit {
  public String variety;
}

you could additionally generate a "common builder interface" IFruitBuilder:

public interface IFruitBuilder<T extends Fruit> implements Builder<T> {
  IFruitBuilder<T> withColour(String colour);
}
public class FruitBuilder implements IFruitBuilder<Fruit> { ... }
public class AppleBuilder implements IFruitBuilder<Apple> { ... }

This would avoid the issue of adding generics to old builder classes which would generate a lot of raw type warnings.

[mkarneim]

@Adrodoc55 I know that we could use the selftype generic style. Actually I described this solution in issue #55. I have not implemented it because of its own uglyness that you pointed out correctly: users must add genetic type arguments to all their builder usages.

The idea of generating separate interfaces for each builder looks promising, although I could not create a working scenario where all PB features would work. In your example both builders just implement the IFruitBuilder interfaces. The more real life scenario would be that both implement their own specific interface. But that does issue the same compilation error I mentioned earlier in this thread.

Edit: Well that is not completely true! See below.

[mkarneim]

The point is, that I think the only way we have to support builder hierarchies is to go with the selftype style, and if we want to release our users from specifying generic type arguments all over their code we must provide some more code that does this automatically, e.g. a factory or builder specific subclasses that just do that.

[mkarneim]

We actually could do this using interfaces if the concrete builders do not extend each other.

I mean this:

public interface IFruitBuilder<T extends Fruit> extends Builder<T> {
  IFruitBuilder<T> withColour(String colour);
}
public interface IAppleBuilder<T extends Apple> extends IFruitBuilder<T> {
  IAppleBuilder<T> withVariety(String variety);
}
public class FruitBuilder implements IFruitBuilder<Fruit> { ... }
public class AppleBuilder implements IAppleBuilder<Apple> { ... }

Edit To illustrate it I just created some playground repository: https://github.com/mkarneim/pojobuilder-playground

There is also a main method that shows some use case: https://github.com/mkarneim/pojobuilder-playground/blob/master/src/main/java/fruits/Main.java

What do you think? Would this work?

[drekbour]

I made some small changes that take a step forwards because the "proper fix" eludes us. We all know this by now :) Can my "baby steps" changes get into the build while we think further as it could take (more) years!

My thoughts (and I thought a lot on this) have been: Abstract pojos should generate abstract builders with generic-self-type. Concrete builders remain as-is. abstract class FruitBuilder<B extends FruitBuilder<B>> Allow inheritance from abstract-builder to concrete-builder via withBaseClass directive class AppleBuilder extends FruitBuilder<AppleBuilder>

Q Interaction with BuilderInterface is ugly but certainly possible. There are several alternatives that would need mocking up to discover which gives the cleanest solution. Q What to do if Fruit is not abstract? I think inheritance between concrete builders is simply not possible (without seriously compromising the simplicity of PB-generated code). AGain there are a few alternatives here

[mkarneim]

I think the best way to discuss different approaches is by showing complete examples like the one I created with https://github.com/mkarneim/pojobuilder-playground. This will show everyone of us how the generated builders could look like, and how they are meant to be used. Especially the usage is important, so that every variant could be checked for compatibility. @drekbour , could you please give some feedback about the 'playground'? Do you have further usages that are currently not covered by this approach?

I like the interface variant for it's simplicity and the fact that the user is not bothered with generic self types.

[drekbour]

Your playground example has the same issues, specifically AppleBuilder.withColour completely shadows the FruitBuilder.withColour (as it must because it needs to change the return type). (Apple also needs directive baseclass=FruitBuilder.class)

Extending from this PR, my proposals for next steps are: 1 AppleBuilder uses casting.

• AppleBuilder withColour(x) { return (AppleBuilder)super.withColour(x); }
• Though not beautiful, this is safe, would be optimised by the JRE and it covers all forms and depths of inheritance.
• Related fields can be omitted from AppleBuilder though they would need to be made accessible by AppleBuilder.build().
• This is pretty cheap to implement
  2. Use generic-self-type for abstract builders only.
• FruitBuilder<B extends FruitBuilder<B>, T> implements Builder<T>
• AppleBuilder can omit withColour() entirely
• This is large piece of work
  3. Think about mandating factoryMethods. These could construct an anonymous, typed subclass allowing self-type for all PBs provided you always access via said factory method
• I'm not sure there is any outcome here that doesn't make PB output much more complicated

Again though - this is conversation about the big picture. I'm really keen to contribute and make inheritable builders happen but could you review this PR as what it is not everything it could be. It is:

• A couple of tidying commits. These are fairly independent of the content but made it easier to implement.
• a bugfix for any baseclass with a clone method that doesn't throw CloneNotSupportedException. This is a code-generation issue not specifically related to PB inheritance.
• a bugfix for generating an abstract Builder from an abstract Pojo. Previously PB generated a build() method that would not compile.

[Adrodoc]

@drekbour The directive baseclass=FruitBuilder.class was avoided on purpuse in the playground as it is unnecessary to achieve type inheritence (interfaces are sufficient for that). Therefor there is no shadowing issue. Inheritence of implementation is not needed, because reusing generated code has no benefit over generating the same code twice. We are still missing an use case that does require inheritence via superclasses rather than interfaces. As for your changes as they are: The bugfix of the clone method is definitely useful and should be merged, but the change about abstract builders for abstract constructors is unnecessary, if inheritence via interfaces is the preffered solution.

[drekbour]

PB still doesn't support this! There is no logical reason to reject "abstract Builders" because you prefer interfaces. In our current code, that "builder interface" is hundreds of lines long and needs manual maintenance every time new fields are added to the pojo super-class. This is precisely the manual work PB is designed to avoid and negates much of its benefits :( I can fixup and reduce this PR to more precise changes if I get a nod from @mkarneim.