Thanks for the PR @Srimathi-S. I wasn't expecting any changes in the public API, I was expecting only changes in the ImmutablesBuilderProvider. Perhaps the first step need to be improved a bit. The entire idea of this implementation is to provide the right builder.

Thanks for the PR @Srimathi-S. I wasn't expecting any changes in the public API, I was expecting only changes in the ImmutablesBuilderProvider. Perhaps the first step need to be improved a bit. The entire idea of this implementation is to provide the right builder.

Thanks for the clarification @filiphr . Will it help to delegate the superClass builderInfo finding to ImmutableProvider that way there will not be any public api change. In DefaultBuilderProvider I can add the below method

protected BuilderInfo findSuperClassBuilderInfo(TypeElement typeElement) {
        return findBuilderInfo(typeElement.getSuperclass());
    }

and implement in ImmutablesBuilderProvider with a custom call directed to super class

   @Override
    protected BuilderInfo findSuperClassBuilderInfo(TypeElement typeElement) {
        return super.findBuilderInfo(typeElement);
    }

Not sure how I can improvise the first step though.

In theory what you can try is to add

BuilderInfo findBuilderInfo( TypeMirror type, Set<TypeElement> processedTypeElementSet )

to the DefaultBuilderProvider and from the existing method call the new method.

However, looking at the test case

public abstract class Item {

    public abstract String getId();

    public abstract Map<String, String> getAttributes();

    public static Item.Builder builder() {
        return new Item.Builder();
    }

    public static class Builder extends ImmutableItem.Builder {

        public ImmutableItem.Builder addSomeData(String key, String data) {
            return super.attributes( Map.of( key, data ) );
        }
    }
}

To me it looks like that we should first check of the Item has builder before delegating to the ImmutableItem to check this. If we do that, then I don't think that we will have a stackoverflow error. That's what I meant with "The entire idea of this implementation is to provide the right builder.".

Currently, the assumption in ImmutablesBuilderProvider is that the builder is located in the Immutable<Type>. However, looking at the example it is also possible to extend the builder and / or to provide a builder method in the <Type> itself. Perhaps we first need to apply the logic for the Type and only then check (without checking parents) if the Immutable<Type> has an appropriate builder

@filiphr Sorry for the delayed response, I have thought about performing builder check on the super class of immutables first and then on immutables but there are 2 scenarios

1. There can be builders which doesn't extend from ImmutableItemDTO i.e something like

@Value.Immutable
public abstract class ItemDTO {



    @Value.Parameter(order = 1)
    public abstract String getId();

    @Value.Parameter(order = 2)
    public abstract Map<String, String> getAttributes();

    public static Builder builder(){
       return new Builder();
    }

    public static class Builder{

    }



}

If above is the class today there will be a builder generated inside immutable as well and till today we would have been using immutable class's builder but after this change we will start using the ItemDTO Builder. This sounds like an ideal behaviour though.

2. There can be multi level inheritance with builder neither present on ItemDTO nor on ImmutableItemDTO but on their base class , if we start from Item and then go to immutable item again we will be struck on the same infinite loop.
   For example in the below code

public class BaseItemDTO {

    public BaseItemDTO(String itemType) {
        this.itemType = itemType;
    }

    public BaseItemDTO() {
    }

    String itemType;


    public String getItemType() {
        return itemType;
    }

    public void setItemType(String itemType) {
        this.itemType = itemType;
    }

    public static Builder builder() {
        return new Builder();
    }

    public static final class Builder {

        private String itemType;

        private Builder() {
        }

        public BaseItemDTO.Builder itemType(String itemType) {
            this.itemType = Objects.requireNonNull(itemType, "itemType");
            return this;
        }

        public BaseItemDTO build() {
            return new BaseItemDTO(itemType);
        }


    }
}

@Value.Immutable(builder = false)
public abstract class ItemDTO extends BaseItemDTO{



    @Value.Parameter(order = 1)
    public abstract String getId();

    @Value.Parameter(order = 2)
    public abstract Map<String, String> getAttributes();



}

The only builder is there in BaseItemDTO it will be ideal for the code to switch over to this. In this case, we will end up encountering stack overflow error.

There are two solutions I see possible now

1. Suggested by you on the above thread

BuilderInfo findBuilderInfo( TypeMirror type, Set<TypeElement> processedTypeElementSet )

to the DefaultBuilderProvider and from the existing method call the new method. Upside is this will also prevent any stack overflow errors. Downside is the new Public API addition and there are two tries before deciding to fallback to next super class.

2. First solution I proposed.
   In DefaultBuilderProvider I can add the below method

protected BuilderInfo findSuperClassBuilderInfo(TypeElement typeElement) {
        return findBuilderInfo(typeElement);
    }

and implement in ImmutablesBuilderProvider with a custom call directed to super class

   @Override
    protected BuilderInfo findSuperClassBuilderInfo(TypeElement typeElement) {
        return super.findBuilderInfo(typeElement);
    }

This will take the control of what to do with the next processing and give it to their corresponding subclass. They can choose to call their own findBuilderInfo or super class findBuilderInfo or something else whenever call fails. Upside is this will make sure the call is linear. Downside is we will not make the code preventative of stack overflow.

Good points @Srimathi-S.

1. There can be builders which do not extend from ImmutableItemDTO.

For this I would indeed agree that it would be more correct to use the builders that are defined you your own ItemDTO.

2. There can be multi level inheritance with builder neither present on ItemDTO nor on ImmutableItemDTO but on their base class

This is a tricky one, as I'm not even sure that the builder on the base class will even be considered as a builder by MapStruct as we need to create ItemDTO and not BaseItemDTO. Does this work now?

I would prefer the second approach. You could even add a method

protected BuilderInfo findBuilderInfo(TypeMirror type, boolean checkParent)

that would be overwritten in the immutable implementation. My main thing is that we should only need to do small changes in the default implementation, in order for the immutables implementation to be able to overwrite certain methods. The public API of the SPI should not need changes I believe. I think that preventing a stackoverflow is on the implementation of the builder provider. Our current immutables implementation is not avoiding that as we always keep trying the existing class.

An alternative would also be to first check the existing hierarchy and then check for the child ImmutableXXX class without checking its parents (as we've already checked those)

@filiphr Sorry for the much delayed response. I have made the fixes to the MR as per your suggestions. Let me know if any further changes are needed.
FYI: According to the issue author, we should be able to use builders for the following case:

@Value.Immutable
public abstract class ValueObject {
    public abstract String getFoo();
    public abstract Long getId();

    public static class Builder extends ImmutableValueObject.Builder {
       //some additional builder methods here
    }
}

However, this will not work out-of-the-box as Immutables will generate the class without a builder() method in the above scenario. Hence, a "no builder found" exception will be thrown from map-struct. Ideally, in this case, the user should also implement their own builder() method as follows:

@Value.Immutable
public abstract class ValueObject {
    public abstract String getFoo();
    public abstract Long getId();

    public static ImmutableValueObject.Builder builder() {
        return new ValueObject.Builder();
    }


    public static class Builder extends ImmutableValueObject.Builder {
       //some additional builder methods here
    }
}

You can refer test for this scenario. This behaviour is retained in latest version of immutables as well.

Thanks for your work on this @Srimathi-S. I've pushed a small polish.

Regarding the need for the user to define a static builder method. I think that is OK. What we could do (as a separate PR and if you are interested) is to look for inner static classes with a public constructor that could be a potential builder.

Thanks @Srimathi-S