@chris922 You also had a look at the issue, if you have time you can also review this PR. Thanks 😄

@filiphr I just had a quick look:

1. In the reference documentation you are mentioning the new exceptions. As a user of mapstruct this would be a bit confusing for me, because - as far as I got it - those exceptions are used internally and are only exposed to the user as warnings/errors during processing and nothing I can catch. So is it really helpful to mention these exception names? Maybe for a custom SPI? (haven't yet checked the builder SPI)
2. This will prevent implementing an own @AfterMapping that is calling the correct builder method in case one of the new exceptions would be thrown, isn't it? I still think as a user writing a custom SPI is rather complex. You may use different builder patterns in case you are using different libraries and the SPI must support everything together. So using @AfterMapping is for me still a good and easy solution to select the correct build method.

Thanks a lot @chris922.

1. I mentioned the exceptions because they are in the public SPI package and they can be thrown by a custom SPI implementations. However, you are right, maybe I can remove them from the documentation and just mention what happens when that happens (no reference to exceptions).

2. You might be right. I think that we need to move that to a separate issue. What we can do is to allow setting the methods to null and just passing a type. In such case we can even use an @ObjectFactory to create a builder instance and we probably need something like @ObjectCreator or whatever that can be invoked to produce a target instance. In any case I think that we need to do that in a new issue and target it for Beta2.

MapStruct should alleviate the user from too complicated stuff like SPI's ..

I totally agree with this point. SPI should only be required for really complex stuff and not for basic stuff like deciding which build method should be called.

Having a new parameter for @Mapper/@MapperConfig sounds good to me.
Maybe this value should also be available in @BeanMapping to be able to decide this per mapping method and not just per mapping class?

@chris922 and @sjaakd I hear you 😄. I will try to find a better approach for this.

I agree that we need to make it easier for the users. The only issue is that the Type is created really early on and it's properties are required really early on, maybe even before we even touch the methods. Therefore it is a bit difficult too handle the more than one builder creation exception. Without an SPI there is no way for us to know the type for the builder and thus no way to know how to get the properties. I am open to ideas 😄

As for the build method, I would propose that we introduce some new annotation like @BuilderFinisher or whatever and we can then filter in the methods that we have and if we find one we invoke that method instead.

For example instead of doing return builder.build() we would do return builderFinisher(builder)/

Btw, I am travelling until Sunday so won't be able to work on this

@sjaakd and @chris922 I've thought a bit about this one. I will add a new annotation so people can hook in custom build method.

However, I am still struggling when there is more than one builder creation method. We need the type of the builder really early on in the compilation and we can use a method to get it as it would be too late. What is your suggestion about this? Is it OK to leave this for a custom SPI?

What is your suggestion about this? Is it OK to leave this for a custom SPI?

Is there a way to automatically detect what kind of builder pattern is present given the builder technology used? I think we did this recently as well, right?

Is there a way to automatically detect what kind of builder pattern is present given the builder technology used? I think we did this recently as well, right?

By builder technology do you mean Immutables, FreeBuilder etc, or something else?

The current mechanism works in the following way:

• Find all public static methods on the type being build that have no parameters
• For each of those methods get the return type and in that type look for a public method without parameters that has a return type same as the type being build

What we can actually work with is when there is more that one builder creation method to not use builders. However, if there is a single builder creation method and multiple build methods than we would assume the user has defined a BuilderFinisher or BuildFactory and we would invoke that to finish the building. The BuildFactory can always be used, in the same way the @ObjectFactory is used.

By builder technology do you mean Immutables, FreeBuilder etc, or something else?

or even protobuf (believe it uses a builder pattern as well, right?

What we can actually work with is when there is more that one builder creation method to not use builders. However, if there is a single builder creation method and multiple build methods than we would assume the user has defined a BuilderFinisher or BuildFactory and we would invoke that to finish the building. The BuildFactory can always be used, in the same way the @objectfactory is used.

So.. If i understand you correctly, in case of conflicts (multiple qualifying methods), you leave it to the user to point out the right "factory" for creation, and the right "finisher" so the user could delegate to the generated code, right?

Could we also use the qualifier approach? Because we normally use that to resolve conflicts somehow..?

So.. If i understand you correctly, in case of conflicts (multiple qualifying methods), you leave it to the user to point out the right "factory" for creation, and the right "finisher" so the user could delegate to the generated code, right?

Factory for creation would be to implement an SPI as we need the builder really early, even before we work with the methods. The finisher can be done with an annotation.

Could we also use the qualifier approach? Because we normally use that to resolve conflicts somehow..?

Not really, most of the time the builder code is generated, so there is no way for users to put the qualifiers. At least I don't see it, If you have some idea please do share it.

Example for the more than one builder creation method:

public Person {

    public static PersonBuilder builder() {}

    public static SecondPersonBuilder secondBuilder() {}
}

In the case of the builder creator for Person without an SPI we cannot figure out what is the type of the builder that we need, PersonBuilder or SecondPersonBuilder. If such case occurs the user will need to implement an SPI.

For the more than one build methods:

public class Vehicle {

    public static VehicleBuilder builder(){}

    public static class VehicleBuilder {

        public Vehicle create() {}
   
        public Vehicle otherCreate() {}
    }
}

In this case we can find the type of the builder (VehicleBuilder). However, the user will need to define something like:

@Mapper
public interface MyMapper {

    @BuilderFinisher
    default Vehicle build(VehicleBuilder builder) {
        return builder.otherCreate();
    }
}

With @BuilderFinisher the users would be able to define their own way of invoking the builder.

How does this sound?

@filiphr .. I understand your approach.

However it would still take a considerable amount of work to do this for each method. If you only get 3 or 4 properties to map in a bean, you now need to implement possibly a creater and a finisher method. Do you know if the builders predominantly use a consistent naming convention that we could use? E.g. if its always createXYZ this could be considered a pattern. You could also do this on the @MappingConfig, @Mapping and @BeanMapping as two properties. Like builderCreatetor = "^createSecond.*$", builderFinisher = "^buildSecond.*$".

@sjaakd, @chris922 I've changed the approach a bit. I took ideas from you, @sjaakd . There is now @BuilderMapping in which you can set the pattern for the finisher method.

I still need to update the Javadoc and the reference documentation, but wanted to get your opinion first. I went with a standalone annotation as that opens the path to meta annotations for 2.0 (perhaps).

WDYT?

@sjaakd it might actually make sense to apply it on MapMapping or IterableMapping. The reason is that we are constructing intermediary methods in there, so if they need builders we can use that from there (although I think it is not passed appropriately now).

2. Yes my idea was to go with meta annotations for the future. In any case I can change this and apply the BuilderMapping as a field in the Mapper, MapperConfig and BeanMapping in order to be consistent with the current paradigm.

How should I proceed with this one?

How should I proceed with this one?

I would prefer to have a consists meta annotation interface in 2.0 and (a consistent one now) 😄

I've finally found the time to go over the changes in this PR 😄.

Thank you @chris922 and @sjaakd for the reviews.

I've applied both of your comments.

• We now have the @BuilderMapping as part of the @BeanMapping, @Mapper and/or @MapperConfig
• I've adapted the error messages a bit, they should be more descriptive now.

If you have time let me know what you think.

Hey Filip.. Looks more consistent.

I'm still a bit struggling whether we want to have annotations within annotations to solve this problem. I know you are considering some kind of meta annotation mechanism for 2.0.

However, It would be more usable (and step-up to 2.0) if you could define an annotation as a user (a bit like we do with qualifiers), grouping specific @BuilderMapping properties for reuse and mark that annotation with @BuilderMapping

So:

@Retention(RetentionPolicy.CLASS)
@Target(ElementType.METHOD)
@BuilderMapping(builderFinisher = "create")
public @interface BuilderMappingForFrameworkX { }

@Retention(RetentionPolicy.CLASS)
@Target(ElementType.METHOD)
@BuilderMapping(builderFinisher = "wrongCreate")
public @interface BuilderMappingForFrameworkY { }

You should then also be able to use the @BuilderMappingForFrameworkX and @BuilderMappingForFrameworkY directly :

@Mapper(builder = @BuilderMappingForFrameworkX)
public interface BuilderMappingDefinedMapper {

    BuilderMappingDefinedMapper INSTANCE = Mappers.getMapper( BuilderMappingDefinedMapper.class );

    Process map(Source source);

    @BeanMapping(builder = @BuilderMappingForFrameworkY)
    Process wrongMap(Source source);
}

If this is possible, we could:

1. Extend this mechanism to other places (e.g. reusable pieces of mappings instead of the @InheritConfiguration / @InheritInverseConfiguration
2. Define our own extra's... e.g. @BuilderMappingForProtoBuf, @BuilderMappingForLombok

Note: Having only one property in @BuilderMapping makes it a bit hard to see where you are heading for.

Shall we discuss this in our Tuesday meeting?

EDIT:

1. if possible, in 2.0, we could have a generic handling of the stuff above. Possibly resolving configuration annotations on the same level also we do @Mapper
2. next to the options above it should be possible to define the mapping in line as well (your approach). So we could see the above as an extension.
3. if we are going for meta annotations, providing this approach does work, we should do this for all configuration stuff, in order to keep the api consistent. This means that 2.0 will not be backward compatible with 1.x
4. I don't want to make the new way of configuring blocking for 1.3: going for a non-annotation property would also be fine for me.

EDIT II:

• you should probably squash and adapt the commit name. You don't throw any exceptions anymore 😄

Rename @BuilderMapping should be renamed to @Builder and builderFinisher into buildMethod

I've squashed all commits, did the rename, update the commit message to:

• Add new @builder annotation for defining a build method
• When there are multiple build methods look for a method named build and if found use it
• If @builder is defined than look for a build method with the defined method
• When a type has multiple builder creation methods throw an exception and don't use the builder

Defaulting to a method named build will make sure that a correct method is selected for:

• FreeBuilder - it has two methods: build and buildPartial
• Protobuf - it has three methods: getDefaultInstanceForType, build and buildPartial

I also updated the documentation. Once the build is done, I'll do the merge

Thanks for the final checks @chris922. I'll commit those directly on master 😄