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

Mapping of immutable objects with Builders #782

Closed
michalcholewinski opened this Issue Mar 14, 2016 · 51 comments

Comments

Projects
None yet
@michalcholewinski

michalcholewinski commented Mar 14, 2016

Would be very nice to have functionality to map immutable objects (without setters) which contains only Builder with methods consistent with the naming convention "withSth().withAnother()" or something like this.
Already discussed with Andreas Gudian.

@agudian agudian changed the title from Mapping of immutable objects to Mapping of immutable objects with Builders Mar 14, 2016

@agudian agudian added the feature label Mar 14, 2016

@agudian agudian added this to the Future planning milestone Mar 14, 2016

@gunnarmorling

This comment has been minimized.

Show comment
Hide comment
@gunnarmorling

gunnarmorling Mar 15, 2016

Member

@michalcholewinski, part of this already is there: You could configure MapStruct to rocognize your "with..." methods as setters. That way you could map between the source and a builder. But you'd have to invoke build() yourself, and it wouldn't work with nested objects. So so more advanced support for builder-like patterns would be great to have. Interested in helping out?

Member

gunnarmorling commented Mar 15, 2016

@michalcholewinski, part of this already is there: You could configure MapStruct to rocognize your "with..." methods as setters. That way you could map between the source and a builder. But you'd have to invoke build() yourself, and it wouldn't work with nested objects. So so more advanced support for builder-like patterns would be great to have. Interested in helping out?

@maposchmi

This comment has been minimized.

Show comment
Hide comment
@maposchmi

maposchmi Apr 6, 2016

This worked for us with Mapstruct 1.0.0.Final
Our Mappers are abstract and extend BeanZuBeanMitBuilder

BeanZuBeanMitBuilder.java.txt
BuilderNamingStrategy.java.txt

maposchmi commented Apr 6, 2016

This worked for us with Mapstruct 1.0.0.Final
Our Mappers are abstract and extend BeanZuBeanMitBuilder

BeanZuBeanMitBuilder.java.txt
BuilderNamingStrategy.java.txt

@ugrave

This comment has been minimized.

Show comment
Hide comment
@ugrave

ugrave Apr 15, 2016

Hi,
the Jakson api has a annotation called JsonPOJOBuilder. This can be used to define a builder to deserilize Jason to model object.
Mapstruct could also define such a annotation to define a builder.

public @interface MapstructObjectBuilder {
    Class<?> builder(); // The class of the builder
    String build() default "build"; // The name of the build method
    String prefix() default "with"; // prefix for set methods
}

ugrave commented Apr 15, 2016

Hi,
the Jakson api has a annotation called JsonPOJOBuilder. This can be used to define a builder to deserilize Jason to model object.
Mapstruct could also define such a annotation to define a builder.

public @interface MapstructObjectBuilder {
    Class<?> builder(); // The class of the builder
    String build() default "build"; // The name of the build method
    String prefix() default "with"; // prefix for set methods
}
@mvonrenteln

This comment has been minimized.

Show comment
Hide comment
@mvonrenteln

mvonrenteln Aug 4, 2016

How is this custom NamingStrategy registered with mapstruct? I could not find anything in the documentation..

mvonrenteln commented Aug 4, 2016

How is this custom NamingStrategy registered with mapstruct? I could not find anything in the documentation..

@agudian

This comment has been minimized.

Show comment
Hide comment
@agudian

agudian Aug 4, 2016

Member

@mvonrenteln Currently that's only possible through a ServiceLoader file in META-INF of a jar that you add to the processor classpath. There is an example below integration-tests/src/test/resources.

Member

agudian commented Aug 4, 2016

@mvonrenteln Currently that's only possible through a ServiceLoader file in META-INF of a jar that you add to the processor classpath. There is an example below integration-tests/src/test/resources.

@mvonrenteln

This comment has been minimized.

Show comment
Hide comment
@mvonrenteln

mvonrenteln Aug 5, 2016

Okay, thanks, I will look into it. The example in integration test worked but the adaptions seem to be a bit tricky... Java-Model-API seems to be a bit complicated. I will adapt it next iteration.

mvonrenteln commented Aug 5, 2016

Okay, thanks, I will look into it. The example in integration test worked but the adaptions seem to be a bit tricky... Java-Model-API seems to be a bit complicated. I will adapt it next iteration.

@egotsev

This comment has been minimized.

Show comment
Hide comment
@egotsev

egotsev Dec 29, 2016

Is anyone working on this issue? If not, I would be interested in implementing it. Can you point me to some documentation as a starting point?

egotsev commented Dec 29, 2016

Is anyone working on this issue? If not, I would be interested in implementing it. Can you point me to some documentation as a starting point?

@mvonrenteln

This comment has been minimized.

Show comment
Hide comment
@mvonrenteln

mvonrenteln Dec 29, 2016

It is in my backlog but I will not work on it in the next weeks. Let me know if you have got something. Perhaps we can discuss this later.

One tricky point is to get this working recursively...

mvonrenteln commented Dec 29, 2016

It is in my backlog but I will not work on it in the next weeks. Let me know if you have got something. Perhaps we can discuss this later.

One tricky point is to get this working recursively...

@egotsev

This comment has been minimized.

Show comment
Hide comment
@egotsev

egotsev Dec 29, 2016

Okay, I can start studying the code and working on this right after NYE. Any other hints and tricky points are welcome. 🙂

egotsev commented Dec 29, 2016

Okay, I can start studying the code and working on this right after NYE. Any other hints and tricky points are welcome. 🙂

@hakamairi

This comment has been minimized.

Show comment
Hide comment
@hakamairi

hakamairi Feb 22, 2017

Any updates? I would love to use this feature.

hakamairi commented Feb 22, 2017

Any updates? I would love to use this feature.

@rasheedamir

This comment has been minimized.

Show comment
Hide comment
@rasheedamir

rasheedamir Mar 4, 2017

@agudian any hopes for this to be in place soon? It will take away lot of donkey work we have to do!

rasheedamir commented Mar 4, 2017

@agudian any hopes for this to be in place soon? It will take away lot of donkey work we have to do!

@kerdosa

This comment has been minimized.

Show comment
Hide comment
@kerdosa

kerdosa Mar 14, 2017

Hi, which freemaker template is used to generate constructor() code? And which freemaker template is used to generate setter method?

kerdosa commented Mar 14, 2017

Hi, which freemaker template is used to generate constructor() code? And which freemaker template is used to generate setter method?

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Mar 14, 2017

Member

@kerdosa If you mean the constructor for the object that we are mapping it is generated in the BeanMappingMethod.

As for the setter methods the SetterWrapper and SetterWrapperForCollectionsAndMaps.

Maybe if you let us know why exactly you need them, we can help you out more specifically.

Member

filiphr commented Mar 14, 2017

@kerdosa If you mean the constructor for the object that we are mapping it is generated in the BeanMappingMethod.

As for the setter methods the SetterWrapper and SetterWrapperForCollectionsAndMaps.

Maybe if you let us know why exactly you need them, we can help you out more specifically.

@ericmartineau

This comment has been minimized.

Show comment
Hide comment
@ericmartineau

ericmartineau Jun 22, 2017

This is something I've been looking into. I have a very rough prototype working and the regression tests passing, but I'm not loving the implementation.

This was my thinking:

You could add an annotation onto a mapping method like this:

@Builder(builderClass = com.some.ImmutableType.Builder, buildMethod = "build", staticBuilderMethod = "builder")
public ImmutableType toImmutableType(MutableType source);

which would generate something like this:

@Override
    public ImmutableType toImmutable(MutableType source) {
        if ( source == null ) {
            return null;
        }

        ImmutableType.Builder immutableType = ImmutableType.builder();

        immutableType.name( source.getName() );
        immutableType.age( source.getAge() );

        return immutableParent.build();
    }

When an immutable is a mapping source, it all works great. When it's a target, you run into a few problems:

  1. Mapstruct wants to look for write accessors on the ImmutableType class, but in this case, it really needs to look at ImmutableType.Builder. Because the Type definitions in mapstruct are created independently of any particular method mapping, it makes it difficult to attach the concept of "Builder" to the target class, even though that's what makes the most sense to me. Personally, I'd probably prefer to move a @builder annotation to the mapping target class rather than the mapping method, but I'm not sure if that breaks any mapstruct best practices though.

Either way, to properly support Builder implementations, each Type should probably have a "mappingTargetType" property, that could store the builder class. Then, any write-based logic could be based on the mappingTargetType instead of the "this" type.

  1. There's not a great way to detect that methods are "setters", not without rolling a custom AccessorNamingStrategy. And, in the case of lombok, which I'm primarily solving for, you can't modify the setter methods in the builder anyway - they are always the same name as the property.

I'd prefer that builder setter names be treated specially, and that there was some built-in support for detecting builder setters based on common conventions. For example, any single-arg public method in the builder that has a single parameter and returns the builder type could be considered a "setter" method.

I'm happy to work on this, but could use some guidance from the pros on how this could best be achieved.

ericmartineau commented Jun 22, 2017

This is something I've been looking into. I have a very rough prototype working and the regression tests passing, but I'm not loving the implementation.

This was my thinking:

You could add an annotation onto a mapping method like this:

@Builder(builderClass = com.some.ImmutableType.Builder, buildMethod = "build", staticBuilderMethod = "builder")
public ImmutableType toImmutableType(MutableType source);

which would generate something like this:

@Override
    public ImmutableType toImmutable(MutableType source) {
        if ( source == null ) {
            return null;
        }

        ImmutableType.Builder immutableType = ImmutableType.builder();

        immutableType.name( source.getName() );
        immutableType.age( source.getAge() );

        return immutableParent.build();
    }

When an immutable is a mapping source, it all works great. When it's a target, you run into a few problems:

  1. Mapstruct wants to look for write accessors on the ImmutableType class, but in this case, it really needs to look at ImmutableType.Builder. Because the Type definitions in mapstruct are created independently of any particular method mapping, it makes it difficult to attach the concept of "Builder" to the target class, even though that's what makes the most sense to me. Personally, I'd probably prefer to move a @builder annotation to the mapping target class rather than the mapping method, but I'm not sure if that breaks any mapstruct best practices though.

Either way, to properly support Builder implementations, each Type should probably have a "mappingTargetType" property, that could store the builder class. Then, any write-based logic could be based on the mappingTargetType instead of the "this" type.

  1. There's not a great way to detect that methods are "setters", not without rolling a custom AccessorNamingStrategy. And, in the case of lombok, which I'm primarily solving for, you can't modify the setter methods in the builder anyway - they are always the same name as the property.

I'd prefer that builder setter names be treated specially, and that there was some built-in support for detecting builder setters based on common conventions. For example, any single-arg public method in the builder that has a single parameter and returns the builder type could be considered a "setter" method.

I'm happy to work on this, but could use some guidance from the pros on how this could best be achieved.

@mvonrenteln

This comment has been minimized.

Show comment
Hide comment
@mvonrenteln

mvonrenteln Jun 22, 2017

@smartytime Great to see you're interested in this, too!

Would be nice to see your actual solution!

These two Problems can be solved seperately. For your first point I have implemented a workaround that you can find on stackoverflow. The second one I have on my long time TODO list... ;)

mvonrenteln commented Jun 22, 2017

@smartytime Great to see you're interested in this, too!

Would be nice to see your actual solution!

These two Problems can be solved seperately. For your first point I have implemented a workaround that you can find on stackoverflow. The second one I have on my long time TODO list... ;)

@ericmartineau

This comment has been minimized.

Show comment
Hide comment
@ericmartineau

ericmartineau Jun 23, 2017

I submitted a PR to spark a discussion. I'm confident there's a better way to handle it, but it's technically working.

#1229

ericmartineau commented Jun 23, 2017

I submitted a PR to spark a discussion. I'm confident there's a better way to handle it, but it's technically working.

#1229

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Jun 24, 2017

Member

I am replying first here without looking at the code in the PR as not to have some bias from the code 😄.

First of all, how would this work with automatic sub method generation? i.e. recursively creating mapping methods between types?

Regarding the annotation, have you thought about reusing @BeanMapping and adding a new field there? We could have a field builderClass. I am not sure if you are going to need buildMethod and staticBuilderMethod. During the processing time one can easily find the build method.

As for the staticBuilderMethod, this method is on the ImmutableType right? How will this work for builders that are not nested classes in their types?

Regarding your problems:

  1. During the creation of the SourceMethod there is access to all the annotations of the method, so we can get the type that we need to use to map to, i.e. to get write accessors to. There are already some annotations that can be plugged into your types like @ObjectFactory, @AfterMapping, @BeforeMapping, @Named. However, currently there are no annotations that one needs to put in their types.

  2. We could provide an alternative AccessorNamingStrategy that is going to work for builders. Users will only have to make sure that they are using that one, this will be a non-breaking change. Alternatively, we could change the signatures of the AccessorNamingStrategy and send some more info, like are we in the context of builder or not. However, I am not sure how I feel about that (not because of the breaking change).

In any case for the second problem we should distinguish when we are in a context of a builder type so we can do some special logic.

Member

filiphr commented Jun 24, 2017

I am replying first here without looking at the code in the PR as not to have some bias from the code 😄.

First of all, how would this work with automatic sub method generation? i.e. recursively creating mapping methods between types?

Regarding the annotation, have you thought about reusing @BeanMapping and adding a new field there? We could have a field builderClass. I am not sure if you are going to need buildMethod and staticBuilderMethod. During the processing time one can easily find the build method.

As for the staticBuilderMethod, this method is on the ImmutableType right? How will this work for builders that are not nested classes in their types?

Regarding your problems:

  1. During the creation of the SourceMethod there is access to all the annotations of the method, so we can get the type that we need to use to map to, i.e. to get write accessors to. There are already some annotations that can be plugged into your types like @ObjectFactory, @AfterMapping, @BeforeMapping, @Named. However, currently there are no annotations that one needs to put in their types.

  2. We could provide an alternative AccessorNamingStrategy that is going to work for builders. Users will only have to make sure that they are using that one, this will be a non-breaking change. Alternatively, we could change the signatures of the AccessorNamingStrategy and send some more info, like are we in the context of builder or not. However, I am not sure how I feel about that (not because of the breaking change).

In any case for the second problem we should distinguish when we are in a context of a builder type so we can do some special logic.

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Jun 24, 2017

Member

I have seen the PR for this issue. Before going into more into depth of the review, I think that we need some answers to some more general questions. I am writing here as I think that there are more people watching this issue and we can get some feedback.

My points:

Member

filiphr commented Jun 24, 2017

I have seen the PR for this issue. Before going into more into depth of the review, I think that we need some answers to some more general questions. I am writing here as I think that there are more people watching this issue and we can get some feedback.

My points:

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Jun 24, 2017

Member
  • Most important how do we want the API to look like? What makes more sense? Having it on the types themselves, or somehow differently?
  • Do people always have control over the types and builders code?
  • Do we want to have interfaces created by builders? Should we go for a smaller experimental option and then extend it?
  • Starting with 1.2.0 MapStruct will create sub-mapping methods. Which means that there won't be a need to generate explicit mappings between 2 types. Will this work with builders? It seems that it'll work with the current implementation, but it will be nice to have some tests for it as well.
  • Do we expect that each builder will be within the class that it creates or there can be builders outside of this class?
  • Will it make sense for MapStruct to find the build method in the builder automatically? This can be done by looking for an empty method that returns a type equal to the target type.
  • Will it make sense for MapStruct to find the builderStaticMethod in the type being created or the builder automatically? This can be done by looking for an empty static method in the type being created or using an empty constructor of the builder
Member

filiphr commented Jun 24, 2017

  • Most important how do we want the API to look like? What makes more sense? Having it on the types themselves, or somehow differently?
  • Do people always have control over the types and builders code?
  • Do we want to have interfaces created by builders? Should we go for a smaller experimental option and then extend it?
  • Starting with 1.2.0 MapStruct will create sub-mapping methods. Which means that there won't be a need to generate explicit mappings between 2 types. Will this work with builders? It seems that it'll work with the current implementation, but it will be nice to have some tests for it as well.
  • Do we expect that each builder will be within the class that it creates or there can be builders outside of this class?
  • Will it make sense for MapStruct to find the build method in the builder automatically? This can be done by looking for an empty method that returns a type equal to the target type.
  • Will it make sense for MapStruct to find the builderStaticMethod in the type being created or the builder automatically? This can be done by looking for an empty static method in the type being created or using an empty constructor of the builder
@ericmartineau

This comment has been minimized.

Show comment
Hide comment
@ericmartineau

ericmartineau Jun 26, 2017

@filiphr - I just read through your comments here. I've updated my original comment on the PR with an updated approach that I think solves for many of the issues you raised above. Specifically, it removes any assumptions that builders will be nested, and provides support for abstracts/generics for targets and builders. I'm trying to get a passing build to push up the latest changes.

As for the API... I respect the notion that we should not have to annotate the domain. There are always cases where that is difficult or impossible because the domain is included as a library. I consider the Builder class to also be part of the domain.

So, even though I went the route of annotating the domain, I think we should also support annotating the mapping method. And, if we're going to support annotating the mapping method, maybe we just start there and remove the ability to map the domain. It seemed more intuitive to annotate the domain when I started, because that information was more closely tied to the implementation, not just with the mapping, and it seemed like there could be ambiguous/conflicting annotations on methods. But I didn't chase that thought process all the way down. And the more I think about it, I think the method annotation may alleviate some difficulties I ran into.

For example, one thing I wrestled with was needing to know which Type any given builder produces. The use case may not be required or desired, but it was to try to support something like the following. I'm going to pretend to use method mapping to show how it could be problematic:

AbstractTarget toAbstractTarget(ConcreteSource source);

/* 
    Assume ConcreteTargetBuilder is not an inner class of ConcreteTarget.  I needed to know that 
    ConcreteTarget.Builder "builds" ConcreteTarget, which can be used to satisfy AbstractTarget.
    But, by inspecting ConcreteTargetBuilder alone I cannot make that determination.  So I ended up 
    putting annotations on the builders that point back at the concrete implementation.

    If we annotated toAbstractTarget, it would solve the issue, right?
*/
default ConcreteTargetBuilder concreteTargetFactory() {
  return ConcreteTarget.builder();
}

I think your suggestions about autodetecting buildMethod and builderStaticMethod are great. I'll do some work on that shortly. Alternatively, you should be able to provide an @ObjectFactory for the builder as well - for cases where the builder doesn't have a special static getter on the target class.

I don't understand the question "Do we want to have interfaces created by builders" - are you asking whether a builder can return an interface (rather than a concrete type)? If so, then hopefully the latest changes will address that. If you meant something else, would you mind elaborating?

I need to look into submapping methods. I don't fully understand that yet.

Also, how should we handle @BeforeMapping and @AfterMapping? I assume we should be passing the builder instance into those methods instead of the target type. Especially if the method is void.

Great feedback - would welcome any other suggestions or comments.

ericmartineau commented Jun 26, 2017

@filiphr - I just read through your comments here. I've updated my original comment on the PR with an updated approach that I think solves for many of the issues you raised above. Specifically, it removes any assumptions that builders will be nested, and provides support for abstracts/generics for targets and builders. I'm trying to get a passing build to push up the latest changes.

As for the API... I respect the notion that we should not have to annotate the domain. There are always cases where that is difficult or impossible because the domain is included as a library. I consider the Builder class to also be part of the domain.

So, even though I went the route of annotating the domain, I think we should also support annotating the mapping method. And, if we're going to support annotating the mapping method, maybe we just start there and remove the ability to map the domain. It seemed more intuitive to annotate the domain when I started, because that information was more closely tied to the implementation, not just with the mapping, and it seemed like there could be ambiguous/conflicting annotations on methods. But I didn't chase that thought process all the way down. And the more I think about it, I think the method annotation may alleviate some difficulties I ran into.

For example, one thing I wrestled with was needing to know which Type any given builder produces. The use case may not be required or desired, but it was to try to support something like the following. I'm going to pretend to use method mapping to show how it could be problematic:

AbstractTarget toAbstractTarget(ConcreteSource source);

/* 
    Assume ConcreteTargetBuilder is not an inner class of ConcreteTarget.  I needed to know that 
    ConcreteTarget.Builder "builds" ConcreteTarget, which can be used to satisfy AbstractTarget.
    But, by inspecting ConcreteTargetBuilder alone I cannot make that determination.  So I ended up 
    putting annotations on the builders that point back at the concrete implementation.

    If we annotated toAbstractTarget, it would solve the issue, right?
*/
default ConcreteTargetBuilder concreteTargetFactory() {
  return ConcreteTarget.builder();
}

I think your suggestions about autodetecting buildMethod and builderStaticMethod are great. I'll do some work on that shortly. Alternatively, you should be able to provide an @ObjectFactory for the builder as well - for cases where the builder doesn't have a special static getter on the target class.

I don't understand the question "Do we want to have interfaces created by builders" - are you asking whether a builder can return an interface (rather than a concrete type)? If so, then hopefully the latest changes will address that. If you meant something else, would you mind elaborating?

I need to look into submapping methods. I don't fully understand that yet.

Also, how should we handle @BeforeMapping and @AfterMapping? I assume we should be passing the builder instance into those methods instead of the target type. Especially if the method is void.

Great feedback - would welcome any other suggestions or comments.

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Jul 1, 2017

Member

@smartytime first of all sorry for the time it took me to get back to you.

For example, one thing I wrestled with was needing to know which Type any given builder produces

If we know that a certain type is builder then we could theoretically look for methods that have no parameters, return a non-void Type and this return Type is not the Builder itself. MapStruct could assume that this type is the type that the builder builds (see my comments about using an SPI as well).

For example:

class Builder {

    Builder name(String name) {
        //set
        return this;
    }

    /** 
    * This method will be a candidate for the method I explained earlier.
    */
    BuildType build() {
        //Here the initializing is created
    }
}

It is not that complex to find such methods during compilation time.

I would say that for @ObjectFactory it should work out of the box like it works now. Once we know that for a certain Type we need to use some Builder than all the actions that MapStruct does know will be done with this Builder Type instead of the Type.

are you asking whether a builder can return an interface (rather than a concrete type)? If so, then hopefully the latest changes will address that. If you meant something else, would you mind elaborating?

Yes that is what I meant. The builder will have some interface as a return type. I think that with your latest comments in the PR (that it works for abstract classes) this should be resolved.

I need to look into submapping methods. I don't fully understand that yet.

Let me know if you have some problems with this one. It is quite new and will be part of the 1.2.0.Final release. In a nutshell MapStruct will automatically try to generate intermediary methods between different types. The approach with annotating the domain works perfectly for this as we have all the information there. However, until now we have tried to avoid needing to annotate it, as it is not always under your control.

Also, how should we handle @BeforeMapping and @AfterMapping? I assume we should be passing the builder instance into those methods instead of the target type. Especially if the method is void.

I would say yes, we would pass the builder instead of the target type (actually there won't be any target type until we do return). This is similar as the @ObjectFactory. I also think nothing will need to be changed here as we would be passing the Builder Type to all the discovery methods instead of the target type.

An alternative way to discover the builders (instead of annotating the domain) would be to use a Service Provider Interface (SPI). Within this SPI MapStruct can pass the Type that it wants to create and will ask for a Builder Type, and we would use that further down. With this one would not pass the builders in the mapping methods.

It could be something like:

import javax.lang.model.type.TypeMirror;
import javax.lang.model.util.Elements;
import javax.lang.model.util.Types;

public interface BuilderProvider {
    TypeMirror findBuilder(TypeMirror typeMirror, Elements elements, Types types);
}

With an SPI users can manually map types to builders, or even have an automatic way of discovering them. Theoretically it would be possible to reuse the Lombok, AutoValue annotations as well. We can even create SPIs for each of those projects in our examples repository and users will be able to use them, maybe even provide them as separate modules if people are interested.

I'll also address your edited message in the PR there.

Btw. We would not include this in the next 1.2.0 release, but schedule it for the 1.3.0. I hope that this would be OK for you. We are almost ready with 1.2.0 and we are planning to have a CR1 really soon. For us would be completely OK to do a 1.3.0 release only with this feature (in order to have it out faster). Thanks for the understanding

Member

filiphr commented Jul 1, 2017

@smartytime first of all sorry for the time it took me to get back to you.

For example, one thing I wrestled with was needing to know which Type any given builder produces

If we know that a certain type is builder then we could theoretically look for methods that have no parameters, return a non-void Type and this return Type is not the Builder itself. MapStruct could assume that this type is the type that the builder builds (see my comments about using an SPI as well).

For example:

class Builder {

    Builder name(String name) {
        //set
        return this;
    }

    /** 
    * This method will be a candidate for the method I explained earlier.
    */
    BuildType build() {
        //Here the initializing is created
    }
}

It is not that complex to find such methods during compilation time.

I would say that for @ObjectFactory it should work out of the box like it works now. Once we know that for a certain Type we need to use some Builder than all the actions that MapStruct does know will be done with this Builder Type instead of the Type.

are you asking whether a builder can return an interface (rather than a concrete type)? If so, then hopefully the latest changes will address that. If you meant something else, would you mind elaborating?

Yes that is what I meant. The builder will have some interface as a return type. I think that with your latest comments in the PR (that it works for abstract classes) this should be resolved.

I need to look into submapping methods. I don't fully understand that yet.

Let me know if you have some problems with this one. It is quite new and will be part of the 1.2.0.Final release. In a nutshell MapStruct will automatically try to generate intermediary methods between different types. The approach with annotating the domain works perfectly for this as we have all the information there. However, until now we have tried to avoid needing to annotate it, as it is not always under your control.

Also, how should we handle @BeforeMapping and @AfterMapping? I assume we should be passing the builder instance into those methods instead of the target type. Especially if the method is void.

I would say yes, we would pass the builder instead of the target type (actually there won't be any target type until we do return). This is similar as the @ObjectFactory. I also think nothing will need to be changed here as we would be passing the Builder Type to all the discovery methods instead of the target type.

An alternative way to discover the builders (instead of annotating the domain) would be to use a Service Provider Interface (SPI). Within this SPI MapStruct can pass the Type that it wants to create and will ask for a Builder Type, and we would use that further down. With this one would not pass the builders in the mapping methods.

It could be something like:

import javax.lang.model.type.TypeMirror;
import javax.lang.model.util.Elements;
import javax.lang.model.util.Types;

public interface BuilderProvider {
    TypeMirror findBuilder(TypeMirror typeMirror, Elements elements, Types types);
}

With an SPI users can manually map types to builders, or even have an automatic way of discovering them. Theoretically it would be possible to reuse the Lombok, AutoValue annotations as well. We can even create SPIs for each of those projects in our examples repository and users will be able to use them, maybe even provide them as separate modules if people are interested.

I'll also address your edited message in the PR there.

Btw. We would not include this in the next 1.2.0 release, but schedule it for the 1.3.0. I hope that this would be OK for you. We are almost ready with 1.2.0 and we are planning to have a CR1 really soon. For us would be completely OK to do a 1.3.0 release only with this feature (in order to have it out faster). Thanks for the understanding

@ericmartineau

This comment has been minimized.

Show comment
Hide comment
@ericmartineau

ericmartineau Sep 10, 2017

@filiphr - I'm picking this back up - would like to see if we can get it wrapped up...

The latest changes to the PR removed all class annotations and simply use @BeanMapping(builderType = Bean.Builder.class) - feels much more aligned.

As you mentioned, this doesn't solve intermediate mappings, so I'm going to take a stab at the SPI example from your last comment for builder discovery. And I really like the idea of using introspection to discover build/builder/accessors.

Please let me know if anything has changed from your end. I know it's been a while.

ericmartineau commented Sep 10, 2017

@filiphr - I'm picking this back up - would like to see if we can get it wrapped up...

The latest changes to the PR removed all class annotations and simply use @BeanMapping(builderType = Bean.Builder.class) - feels much more aligned.

As you mentioned, this doesn't solve intermediate mappings, so I'm going to take a stab at the SPI example from your last comment for builder discovery. And I really like the idea of using introspection to discover build/builder/accessors.

Please let me know if anything has changed from your end. I know it's been a while.

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Sep 12, 2017

Member

@smartytime great that you want to pick it back up. On our side nothing has changed. I think that this is an awesome feature. It will certainly make things easier for users using Builders and MapStruct.

By the way, you won't be using introspection, but the java annotation processor packages. This is much more powerful than introspection, because you are seeing what the compiler sees. With Introspection you have less information because it is during runtime 😄 .

If you need anything, just let us know. We are more than happy to help you out with this feature.

Btw, you might wanna rebase everything on master (if you didn't do already), as there might be some other changes that you might use.

Member

filiphr commented Sep 12, 2017

@smartytime great that you want to pick it back up. On our side nothing has changed. I think that this is an awesome feature. It will certainly make things easier for users using Builders and MapStruct.

By the way, you won't be using introspection, but the java annotation processor packages. This is much more powerful than introspection, because you are seeing what the compiler sees. With Introspection you have less information because it is during runtime 😄 .

If you need anything, just let us know. We are more than happy to help you out with this feature.

Btw, you might wanna rebase everything on master (if you didn't do already), as there might be some other changes that you might use.

@ericmartineau

This comment has been minimized.

Show comment
Hide comment
@ericmartineau

ericmartineau Sep 14, 2017

Thanks for the reply. You'll notice I closed the other PR. I'm going to start fresh because now that II've read over your comments a number of times I think I'm starting to understand what you've been telling me. This is what I'm thinking:

I'm going to start over, taking the SPI approach. I can see now how it will be much less intrusive. I'm sure I'll have a bunch of questions.

ericmartineau commented Sep 14, 2017

Thanks for the reply. You'll notice I closed the other PR. I'm going to start fresh because now that II've read over your comments a number of times I think I'm starting to understand what you've been telling me. This is what I'm thinking:

I'm going to start over, taking the SPI approach. I can see now how it will be much less intrusive. I'm sure I'll have a bunch of questions.

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Sep 14, 2017

Member

No problem @smartytime thanks for tackling this. It is really an interesting approach. I think that we can have a nice non-intrusive clean solution.

Looking forward to your next PR. If you have any questions feel free to ping me on our gitter chat. I am mostly available in the evenings or during the weekends.

Member

filiphr commented Sep 14, 2017

No problem @smartytime thanks for tackling this. It is really an interesting approach. I think that we can have a nice non-intrusive clean solution.

Looking forward to your next PR. If you have any questions feel free to ping me on our gitter chat. I am mostly available in the evenings or during the weekends.

@LucasAndersson

This comment has been minimized.

Show comment
Hide comment
@LucasAndersson

LucasAndersson Sep 15, 2017

This issue is the only thing keeping me (and I'm sure many others) from using MapStruct. Great to hear it's being worked on @smartytime

LucasAndersson commented Sep 15, 2017

This issue is the only thing keeping me (and I'm sure many others) from using MapStruct. Great to hear it's being worked on @smartytime

@kiamesdavies

This comment has been minimized.

Show comment
Hide comment
@kiamesdavies

kiamesdavies Sep 20, 2017

This is great. Waiting .......

kiamesdavies commented Sep 20, 2017

This is great. Waiting .......

@filiphr filiphr modified the milestones: Future planning, 1.3.x Oct 17, 2017

@leimer

This comment has been minimized.

Show comment
Hide comment
@leimer

leimer Nov 16, 2017

At my work we would love to use MapStruct. But we heavily use Lombok and its Builder annotation. So we have many immutable types with builders. This issue is now open for 1,5 years. I think it's time to get this working.

I read the conversation and currently trying to figure out the terminology and technologies used at MapStruct. @smartytime @filiphr do you currently put some effort in this issue? I so, can I help you to finish a working implementation? If not, I would love to take all the arguments above in account and try to create a PR for this.

leimer commented Nov 16, 2017

At my work we would love to use MapStruct. But we heavily use Lombok and its Builder annotation. So we have many immutable types with builders. This issue is now open for 1,5 years. I think it's time to get this working.

I read the conversation and currently trying to figure out the terminology and technologies used at MapStruct. @smartytime @filiphr do you currently put some effort in this issue? I so, can I help you to finish a working implementation? If not, I would love to take all the arguments above in account and try to create a PR for this.

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Nov 16, 2017

Member

@leimer there is already #1296 PR opened for this. However, I am not sure what is the status for @smartytime in it.

If you are interested have a look at the discussion in there as well.

We would definitely want to have this for our next 1.3 release (which we hope it would be soon). If you are willing to help us out we would very much appreciate the help. We can of course help you out in the process as well.

Just as info, we want to have something that would not be overly dependant on the different implementations of the builders. Ideally MapStruct shouldn't need to know about Lombok, AutoValue and all the other flavors.

Member

filiphr commented Nov 16, 2017

@leimer there is already #1296 PR opened for this. However, I am not sure what is the status for @smartytime in it.

If you are interested have a look at the discussion in there as well.

We would definitely want to have this for our next 1.3 release (which we hope it would be soon). If you are willing to help us out we would very much appreciate the help. We can of course help you out in the process as well.

Just as info, we want to have something that would not be overly dependant on the different implementations of the builders. Ideally MapStruct shouldn't need to know about Lombok, AutoValue and all the other flavors.

@atomcat1978

This comment has been minimized.

Show comment
Hide comment
@atomcat1978

atomcat1978 Nov 29, 2017

Actually, I think, that the current version of MapStruct can be used together quite well with immutables. It is a question of mapping architecture. I'm soon going to write a blog entry about the approach we use in our current project.

atomcat1978 commented Nov 29, 2017

Actually, I think, that the current version of MapStruct can be used together quite well with immutables. It is a question of mapping architecture. I'm soon going to write a blog entry about the approach we use in our current project.

@michaeljeanh

This comment has been minimized.

Show comment
Hide comment
@michaeljeanh

michaeljeanh Dec 8, 2017

+1
This feature is what's keeping me from using MapStruct. Any ETA?

michaeljeanh commented Dec 8, 2017

+1
This feature is what's keeping me from using MapStruct. Any ETA?

@atomcat1978

This comment has been minimized.

Show comment
Hide comment
@atomcat1978

atomcat1978 Dec 11, 2017

It seems like it takes a while to get the post out on our company website, so here is the basic idea:

Suppose that we have a SoapUser object that should be mapped to a User domain object (the domain object is immutable). It is possible to set up the mapper as an abstract class in Mapstruct, so we can create the following mapping definition:


@Mapper
public abstract class UserMapper {

	abstract User.Builder soapUserToUserBuilder(SoapUser soapUser);

	public User soapUserToUser(SoapUser soapUser) {
		User.Builder builder = soapUserToUserBuilder(soapUser);
		return builder != null ?
				builder.setType(getUserType(soapUser))
				.build() :
				null;
	}

	private UserType getUserType(SoapUser soapUser) {
                // implementation.... Not important
	}
}

The essence is, that the soapUserToUserBuilder is created by MapStruct, and is hidden from the world outside. It returns our configured builder. In the method soapUserToUser we just call the method, adjust on the builder if required, call build(), and return the created immutable object.

This way the codebase won't be full of .build() garbage, and I think, it is a pretty transparent solution.

One might combine it with immutables to save on required number of code lines when it comes to creating immutable classes.

atomcat1978 commented Dec 11, 2017

It seems like it takes a while to get the post out on our company website, so here is the basic idea:

Suppose that we have a SoapUser object that should be mapped to a User domain object (the domain object is immutable). It is possible to set up the mapper as an abstract class in Mapstruct, so we can create the following mapping definition:


@Mapper
public abstract class UserMapper {

	abstract User.Builder soapUserToUserBuilder(SoapUser soapUser);

	public User soapUserToUser(SoapUser soapUser) {
		User.Builder builder = soapUserToUserBuilder(soapUser);
		return builder != null ?
				builder.setType(getUserType(soapUser))
				.build() :
				null;
	}

	private UserType getUserType(SoapUser soapUser) {
                // implementation.... Not important
	}
}

The essence is, that the soapUserToUserBuilder is created by MapStruct, and is hidden from the world outside. It returns our configured builder. In the method soapUserToUser we just call the method, adjust on the builder if required, call build(), and return the created immutable object.

This way the codebase won't be full of .build() garbage, and I think, it is a pretty transparent solution.

One might combine it with immutables to save on required number of code lines when it comes to creating immutable classes.

filiphr added a commit to filiphr/mapstruct that referenced this issue Mar 31, 2018

#782 Add BuilderInfo to SPI
The implementors of the SPI should return all the required information

filiphr added a commit to filiphr/mapstruct that referenced this issue Mar 31, 2018

filiphr added a commit to filiphr/mapstruct that referenced this issue Mar 31, 2018

#782 Do not use JDT for the Lombok integration tests
Lombok uses internals of the Java and Eclipse compilers.
In order for it to work with the Eclipse compiler, we need to add some extra jar.
Therefore, we are only testing Lombok with the Java compiler

filiphr added a commit to filiphr/mapstruct that referenced this issue Mar 31, 2018

filiphr added a commit to filiphr/mapstruct that referenced this issue Mar 31, 2018

filiphr added a commit to filiphr/mapstruct that referenced this issue Mar 31, 2018

filiphr added a commit to filiphr/mapstruct that referenced this issue Mar 31, 2018

filiphr added a commit to filiphr/mapstruct that referenced this issue Mar 31, 2018

filiphr added a commit to filiphr/mapstruct that referenced this issue Mar 31, 2018

filiphr added a commit to filiphr/mapstruct that referenced this issue Mar 31, 2018

filiphr added a commit to filiphr/mapstruct that referenced this issue Mar 31, 2018

filiphr added a commit to filiphr/mapstruct that referenced this issue Mar 31, 2018

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Mar 31, 2018

Member

@milanov the support will land on our master really soon :)

Member

filiphr commented Mar 31, 2018

@milanov the support will land on our master really soon :)

@filiphr filiphr self-assigned this Apr 2, 2018

@filiphr filiphr closed this in #1373 Apr 4, 2018

filiphr added a commit that referenced this issue Apr 4, 2018

filiphr added a commit that referenced this issue Apr 4, 2018

filiphr added a commit that referenced this issue Apr 4, 2018

#782 Add BuilderInfo to SPI
The implementors of the SPI should return all the required information

filiphr added a commit that referenced this issue Apr 4, 2018

#782 Do not use JDT for the Lombok integration tests
Lombok uses internals of the Java and Eclipse compilers.
In order for it to work with the Eclipse compiler, we need to add some extra jar.
Therefore, we are only testing Lombok with the Java compiler

filiphr added a commit that referenced this issue Apr 4, 2018

filiphr added a commit that referenced this issue Apr 4, 2018

filiphr added a commit that referenced this issue Apr 4, 2018

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Apr 4, 2018

Member

I am happy to announce that the builder support is finally on master. We are going to do a 1.3.0 Beta release so you can all try it out. Thanks a lot for all the support and the patience with us.

Member

filiphr commented Apr 4, 2018

I am happy to announce that the builder support is finally on master. We are going to do a 1.3.0 Beta release so you can all try it out. Thanks a lot for all the support and the patience with us.

@ramses-gomez

This comment has been minimized.

Show comment
Hide comment
@ramses-gomez

ramses-gomez May 9, 2018

Hello when is the 1.3.0 beta being released?

ramses-gomez commented May 9, 2018

Hello when is the 1.3.0 beta being released?

@joedj

This comment has been minimized.

Show comment
Hide comment
@joedj

joedj May 30, 2018

Thanks for working on this!

I have tested it out on master with an immutable destination class with @lombok.Builder, and can confirm that it does not work when the destination class and the mapper are in the same compilation phase (i.e. the same src/main/java), because mapstruct can't yet see the builder() method (as per rzwitserloot/lombok#1538 ).

LombokMapperTest works because the mapper class is in src/test/java rather than src/main/java.

There is a comment in integrationtest/src/test/resources/lombokBuilderTest/src/test/java/org/mapstruct/itest/lombok/PersonMapper.java hinting at this "Therefore we put the mapper into a separate class.", but it took me a while to figure out because it doesn't just have to be in a separate class, it needs to be in a separate compiler invocation entirely.

joedj commented May 30, 2018

Thanks for working on this!

I have tested it out on master with an immutable destination class with @lombok.Builder, and can confirm that it does not work when the destination class and the mapper are in the same compilation phase (i.e. the same src/main/java), because mapstruct can't yet see the builder() method (as per rzwitserloot/lombok#1538 ).

LombokMapperTest works because the mapper class is in src/test/java rather than src/main/java.

There is a comment in integrationtest/src/test/resources/lombokBuilderTest/src/test/java/org/mapstruct/itest/lombok/PersonMapper.java hinting at this "Therefore we put the mapper into a separate class.", but it took me a while to figure out because it doesn't just have to be in a separate class, it needs to be in a separate compiler invocation entirely.

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Jul 9, 2018

Member

@ramses-gomez I hope we would be able to release by the end of this week 😄. Thanks a lot for the patience with us.

@joedj sorry for the misunderstanding. Maybe we need to change that hint. Yes Mappers and Lombok classes need to be in separate compilation units. In this case those are the sources and test-sources compilations. However, for real world use cases you would probably need to have a module with the lombok classes and another module with the MapStruct mappers (that is until rzwitserloot/lombok#1538 is not resolved)

Member

filiphr commented Jul 9, 2018

@ramses-gomez I hope we would be able to release by the end of this week 😄. Thanks a lot for the patience with us.

@joedj sorry for the misunderstanding. Maybe we need to change that hint. Yes Mappers and Lombok classes need to be in separate compilation units. In this case those are the sources and test-sources compilations. However, for real world use cases you would probably need to have a module with the lombok classes and another module with the MapStruct mappers (that is until rzwitserloot/lombok#1538 is not resolved)

@gaweL-

This comment has been minimized.

Show comment
Hide comment
@gaweL-

gaweL- Aug 27, 2018

Hi,

Thanks for this feature, helping a lot :)

The ImmutablesBuildProvider currently assumes Immutables genereted classes are available under the "Immutable*" pattern (ImmutablesBuildProvider:84). Is it possible for you to support the org.immutables.value.Style#typeImmutable annotation/method ?

gaweL- commented Aug 27, 2018

Hi,

Thanks for this feature, helping a lot :)

The ImmutablesBuildProvider currently assumes Immutables genereted classes are available under the "Immutable*" pattern (ImmutablesBuildProvider:84). Is it possible for you to support the org.immutables.value.Style#typeImmutable annotation/method ?

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Sep 14, 2018

Member

@gaweL- I don't see why something like this won't be possible. Can you please create a new issue for this?

Member

filiphr commented Sep 14, 2018

@gaweL- I don't see why something like this won't be possible. Can you please create a new issue for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment