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

Make use of constructor arguments when instantiating mapping targets #73

Open
gunnarmorling opened this Issue Nov 24, 2013 · 38 comments

Comments

Projects
None yet
@gunnarmorling
Member

gunnarmorling commented Nov 24, 2013

Some thoughts:

  • Need a way to select the constructor to use if there are several ones
  • Properties not covered by the chosen constructor should be populated by setters
  • Optionally, the mechanism should take parameterized factory methods into account
@agudian

This comment has been minimized.

Show comment
Hide comment
@agudian

agudian Jan 2, 2014

Member

What kind of arguments do you mean?

Member

agudian commented Jan 2, 2014

What kind of arguments do you mean?

@gunnarmorling

This comment has been minimized.

Show comment
Hide comment
@gunnarmorling

gunnarmorling Jan 3, 2014

Member

I'd like to add some kind of support for immutable target types, which take their attribute values through a constructor:

public class Order {
    private final long id;
    private final String name;

    public Order(long id, String name) { ... }
 }

The generated code would then have to invoke this constructor instead of relying on the default constructor and setters. Generally this should be possbible as we can access the parameter names in the annotation processor.

An open question is how to chose a constructor in case there are several. Optionally existing setters could be invoked for attributes not present in the invoked constructor.

Member

gunnarmorling commented Jan 3, 2014

I'd like to add some kind of support for immutable target types, which take their attribute values through a constructor:

public class Order {
    private final long id;
    private final String name;

    public Order(long id, String name) { ... }
 }

The generated code would then have to invoke this constructor instead of relying on the default constructor and setters. Generally this should be possbible as we can access the parameter names in the annotation processor.

An open question is how to chose a constructor in case there are several. Optionally existing setters could be invoked for attributes not present in the invoked constructor.

@fcamblor

This comment has been minimized.

Show comment
Hide comment
@fcamblor

fcamblor Nov 18, 2014

👍 on this

We could introduce a @Constructor({"id","name"}) annotation allowing to bind mapping to a dedicated constructor, using named parameters (not types which could be ambiguous and less obvious regarding API readability)

WDYT ?

👍 on this

We could introduce a @Constructor({"id","name"}) annotation allowing to bind mapping to a dedicated constructor, using named parameters (not types which could be ambiguous and less obvious regarding API readability)

WDYT ?

@gunnarmorling

This comment has been minimized.

Show comment
Hide comment
@gunnarmorling

gunnarmorling Nov 18, 2014

Member

@fcamblor, yes, that sounds very reasonable. I first thought @ConstructorProperties from the JDK could be re-used, but that lacks the required element type. +1 for using names.

One thing though: I assume it's a common case that there is a default constructor (no args) and one constructor taking the bean properties. It'd be nice to globally configure which one to use in this case without the need to specify @Constructor for each single method.

Member

gunnarmorling commented Nov 18, 2014

@fcamblor, yes, that sounds very reasonable. I first thought @ConstructorProperties from the JDK could be re-used, but that lacks the required element type. +1 for using names.

One thing though: I assume it's a common case that there is a default constructor (no args) and one constructor taking the bean properties. It'd be nice to globally configure which one to use in this case without the need to specify @Constructor for each single method.

@gunnarmorling gunnarmorling modified the milestones: 1.0.0.RC1, 1.0.0.Beta3 Nov 25, 2014

@gunnarmorling gunnarmorling modified the milestones: 1.0.0.RC1, 1.0.0.Beta4 Mar 1, 2015

@pardom-zz

This comment has been minimized.

Show comment
Hide comment
@pardom-zz

pardom-zz Mar 7, 2015

Contributor

I would also like to see this, specifically for immutable objects. Any update?

Contributor

pardom-zz commented Mar 7, 2015

I would also like to see this, specifically for immutable objects. Any update?

@gunnarmorling

This comment has been minimized.

Show comment
Hide comment
@gunnarmorling

gunnarmorling Mar 8, 2015

Member

No update really, but it's one of the remaining things which I think should be done for 1.0. Would you be interested in giving this a try?

Member

gunnarmorling commented Mar 8, 2015

No update really, but it's one of the remaining things which I think should be done for 1.0. Would you be interested in giving this a try?

@pardom-zz

This comment has been minimized.

Show comment
Hide comment
@pardom-zz

pardom-zz Mar 8, 2015

Contributor

I might be. Can you point me in the right direction?

Contributor

pardom-zz commented Mar 8, 2015

I might be. Can you point me in the right direction?

@gunnarmorling

This comment has been minimized.

Show comment
Hide comment
@gunnarmorling

gunnarmorling Mar 9, 2015

Member

Some helpful docs to get started are:

For this issue, you'd probably have to adapt PropertyMapping (describs the "link" between a source and target property, so this would somehow have to express that a property is propagated by a constructor argument rather than a setter call) and BeanMappingMethod (describes one mapping method between two bean types, so this would have to express which constructor of the target type to invoke).

Hope this helps for a start, let me know in case you run into any road blocks. Thanks!

Member

gunnarmorling commented Mar 9, 2015

Some helpful docs to get started are:

For this issue, you'd probably have to adapt PropertyMapping (describs the "link" between a source and target property, so this would somehow have to express that a property is propagated by a constructor argument rather than a setter call) and BeanMappingMethod (describes one mapping method between two bean types, so this would have to express which constructor of the target type to invoke).

Hope this helps for a start, let me know in case you run into any road blocks. Thanks!

@pardom-zz

This comment has been minimized.

Show comment
Hide comment
@pardom-zz

pardom-zz Mar 12, 2015

Contributor

Hmm, not sure I have the time to spend on this right now, so I can't promise I'll get it done. This feature would satisfy my immutability requirement though, so kudos to whoever does it. 👍

Contributor

pardom-zz commented Mar 12, 2015

Hmm, not sure I have the time to spend on this right now, so I can't promise I'll get it done. This feature would satisfy my immutability requirement though, so kudos to whoever does it. 👍

@gunnarmorling gunnarmorling modified the milestones: 1.1-next, 1.0.0.CR1 May 29, 2015

@mozinrat

This comment has been minimized.

Show comment
Hide comment
@mozinrat

mozinrat Aug 14, 2015

I would love to use this feature if its there, can anyone point me about whats being done.

I would love to use this feature if its there, can anyone point me about whats being done.

@gunnarmorling

This comment has been minimized.

Show comment
Hide comment
@gunnarmorling

gunnarmorling Aug 14, 2015

Member

@mozinrat no one found the time to implement it yet unfortunately. If you are interested and would like to help out with building it, let me know and I will help you to get running with Hacking on MapStruct.

Member

gunnarmorling commented Aug 14, 2015

@mozinrat no one found the time to implement it yet unfortunately. If you are interested and would like to help out with building it, let me know and I will help you to get running with Hacking on MapStruct.

@mozinrat

This comment has been minimized.

Show comment
Hide comment
@mozinrat

mozinrat Aug 16, 2015

@gunnarmorling whats the plan for this, lets connect on some platform email etc and discuss the possibility. I can give it a week or so but its hard for me to give any more time. Forking the repo meanwhile...:)

@gunnarmorling whats the plan for this, lets connect on some platform email etc and discuss the possibility. I can give it a week or so but its hard for me to give any more time. Forking the repo meanwhile...:)

@gunnarmorling

This comment has been minimized.

Show comment
Hide comment
@gunnarmorling

gunnarmorling Aug 17, 2015

Member

@mozinrat It should be doable in a week :)

You can send a mail to mapstruct-users to get a discussion started on the feature and its implementation. To get started, you can checkout the information given here and the technical documentation hints given in the section after that.

I think the main issue to decide is how to select the constructor to be chosen in case there are several ones, e.g. a default (no-args) constructor and one or two constructors with arguments.

Many thanks for your help @mozinrat, looking forward to this feature very much! Release we are in a stabilization phase for the 1.0 Final release, this feature should go into 1.1 from my POV.

Member

gunnarmorling commented Aug 17, 2015

@mozinrat It should be doable in a week :)

You can send a mail to mapstruct-users to get a discussion started on the feature and its implementation. To get started, you can checkout the information given here and the technical documentation hints given in the section after that.

I think the main issue to decide is how to select the constructor to be chosen in case there are several ones, e.g. a default (no-args) constructor and one or two constructors with arguments.

Many thanks for your help @mozinrat, looking forward to this feature very much! Release we are in a stabilization phase for the 1.0 Final release, this feature should go into 1.1 from my POV.

@pjean

This comment has been minimized.

Show comment
Hide comment
@pjean

pjean Feb 3, 2016

👍 I would like to be able to use immutable object.
I think you should take a look at the Jackson project which uses the constructor and some annotations for serialization and deserialization. It should be quite similar to map the target.
This issue is a good start for investigation.

pjean commented Feb 3, 2016

👍 I would like to be able to use immutable object.
I think you should take a look at the Jackson project which uses the constructor and some annotations for serialization and deserialization. It should be quite similar to map the target.
This issue is a good start for investigation.

@gunnarmorling

This comment has been minimized.

Show comment
Hide comment
@gunnarmorling

gunnarmorling Feb 3, 2016

Member

@pjean, yes, support for this is very high on my wish list. It's basically a question of resources, so if someone from the community stepped up to give this a shot, this would be awesome. Maybe you'd be interested?

Member

gunnarmorling commented Feb 3, 2016

@pjean, yes, support for this is very high on my wish list. It's basically a question of resources, so if someone from the community stepped up to give this a shot, this would be awesome. Maybe you'd be interested?

@pjean

This comment has been minimized.

Show comment
Hide comment
@pjean

pjean Feb 3, 2016

Need to go deep into mapstruct code. I will take a look if I find enough time and will keep you in touch.

pjean commented Feb 3, 2016

Need to go deep into mapstruct code. I will take a look if I find enough time and will keep you in touch.

@afdia

This comment has been minimized.

Show comment
Hide comment
@afdia

afdia Feb 7, 2016

There is an alternative solution to the "which constructor to use" problem: Provider-Methods, which are used by Guice and Dagger (DI frameworks) when they should create classes without an @Inject annotation (I guess CDI uses the same concept but it's called Producers).

Instead of annotating a constructor, you would write a provider method. e.g.:

class Car {
    int doors;
    Color color;
    public Car(int doors, Color color) {
        this.doors = doors;
        this.color = color;
    }
}

The provider method could be located in the mapper class and would look like

@Provides
public Car createCar(int doors, Color color) {
    return new Car(doors, color);
}

This approach introduces some overhead (because the user has to map the method parameters to the appropriate constructor field), but is much more flexible and non invasive to the class which should be mapped.

This could be an advantage when classes should not or cannot be modified (e.g. generated classes or classes of a 3rd party lib) and would also enable alternative ways to create a class (e.g. calling a static creator method)

The disadvantage is that MapStruct can only verify the correctness of the provider method arguments, but not the correctness of the method argument -> constructor argument mapping.

I guess the best solution would be letting the user choose between both ways (80% of the time the annotation would be sufficient, but otherwise the more flexible approach would be nice to have)

Just some food for thought :)

afdia commented Feb 7, 2016

There is an alternative solution to the "which constructor to use" problem: Provider-Methods, which are used by Guice and Dagger (DI frameworks) when they should create classes without an @Inject annotation (I guess CDI uses the same concept but it's called Producers).

Instead of annotating a constructor, you would write a provider method. e.g.:

class Car {
    int doors;
    Color color;
    public Car(int doors, Color color) {
        this.doors = doors;
        this.color = color;
    }
}

The provider method could be located in the mapper class and would look like

@Provides
public Car createCar(int doors, Color color) {
    return new Car(doors, color);
}

This approach introduces some overhead (because the user has to map the method parameters to the appropriate constructor field), but is much more flexible and non invasive to the class which should be mapped.

This could be an advantage when classes should not or cannot be modified (e.g. generated classes or classes of a 3rd party lib) and would also enable alternative ways to create a class (e.g. calling a static creator method)

The disadvantage is that MapStruct can only verify the correctness of the provider method arguments, but not the correctness of the method argument -> constructor argument mapping.

I guess the best solution would be letting the user choose between both ways (80% of the time the annotation would be sufficient, but otherwise the more flexible approach would be nice to have)

Just some food for thought :)

@hyungsul

This comment has been minimized.

Show comment
Hide comment
@hyungsul

hyungsul Feb 10, 2016

I am currently investigating this issue. In my project, I modeled DTOs as immutable and have the same problem with this issue. Because I have a long list of parameters in DTOs, it is inefficient to list all the constructor parameters in the mapping annotation.

For some cases, it is good enough to have a boolean flag like "immutable" so that all the values are mapped via a constructor instead of setters without listing all the constructor parameters in the annotation.

I am currently investigating this issue. In my project, I modeled DTOs as immutable and have the same problem with this issue. Because I have a long list of parameters in DTOs, it is inefficient to list all the constructor parameters in the mapping annotation.

For some cases, it is good enough to have a boolean flag like "immutable" so that all the values are mapped via a constructor instead of setters without listing all the constructor parameters in the annotation.

@gunnarmorling

This comment has been minimized.

Show comment
Hide comment
@gunnarmorling

gunnarmorling Mar 15, 2016

Member

@afdia Thanks for your input! We already have "factory methods", but they don't they take arguments either. But we could enhance those, making them just like producer methods in CDI. Qualifiers could be used to resolve ambiguities.

In the end I'd like to see support for both: parameterized constructors and parameterized factory/producer methods.

Member

gunnarmorling commented Mar 15, 2016

@afdia Thanks for your input! We already have "factory methods", but they don't they take arguments either. But we could enhance those, making them just like producer methods in CDI. Qualifiers could be used to resolve ambiguities.

In the end I'd like to see support for both: parameterized constructors and parameterized factory/producer methods.

@gunnarmorling

This comment has been minimized.

Show comment
Hide comment
@gunnarmorling

gunnarmorling Mar 15, 2016

Member

@hyungsul, agreed some means of simplified configuration would be great, so to select between e.g. the default constructor and a single parameterized one.

Member

gunnarmorling commented Mar 15, 2016

@hyungsul, agreed some means of simplified configuration would be great, so to select between e.g. the default constructor and a single parameterized one.

@cliedeman

This comment has been minimized.

Show comment
Hide comment
@cliedeman

cliedeman Aug 8, 2016

Contributor

I have started work on this here #847

Contributor

cliedeman commented Aug 8, 2016

I have started work on this here #847

@sjaakd

This comment has been minimized.

Show comment
Hide comment
@sjaakd

sjaakd Aug 8, 2016

Contributor

👍 i'll take a look as soon as I'm back .

Op 8 aug. 2016 om 10:28 heeft Ciaran Liedeman notifications@github.com het volgende geschreven:

I have started work on this here #847


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Contributor

sjaakd commented Aug 8, 2016

👍 i'll take a look as soon as I'm back .

Op 8 aug. 2016 om 10:28 heeft Ciaran Liedeman notifications@github.com het volgende geschreven:

I have started work on this here #847


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@mattiasoamz

This comment has been minimized.

Show comment
Hide comment
@mattiasoamz

mattiasoamz Dec 14, 2016

Would you consider adding support for Guava immutable collections as well? Not sure if it should be handled as part of this issue - it does relate to support immutable beans but from an implementation perspective it's a different thing.

Would you consider adding support for Guava immutable collections as well? Not sure if it should be handled as part of this issue - it does relate to support immutable beans but from an implementation perspective it's a different thing.

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Dec 15, 2016

Member

@mattiasoamz This can possibly done via some conversions. Can you perhaps create a new issue for this and maybe specify how would you use them (the use case)? I have not used the Guava immutable collections, and an example bean with them would be much appreciated 😄

Member

filiphr commented Dec 15, 2016

@mattiasoamz This can possibly done via some conversions. Can you perhaps create a new issue for this and maybe specify how would you use them (the use case)? I have not used the Guava immutable collections, and an example bean with them would be much appreciated 😄

@mattiasoamz

This comment has been minimized.

Show comment
Hide comment
@mattiasoamz

mattiasoamz Dec 27, 2016

@filiphr I'm sorry for my late response - the holidays got in between. I have created #1025.

@filiphr I'm sorry for my late response - the holidays got in between. I have created #1025.

@hakamairi

This comment has been minimized.

Show comment
Hide comment
@hakamairi

hakamairi Feb 22, 2017

Hi,
I'm currently working with a lot of immutable classes and this could be the mapping solution.
What's the progress on the issue?

The initial approach is a bit similar to @JsonCreator @JsonProperty of com.fasterxml.jackson.

Hi,
I'm currently working with a lot of immutable classes and this could be the mapping solution.
What's the progress on the issue?

The initial approach is a bit similar to @JsonCreator @JsonProperty of com.fasterxml.jackson.

@jelmew

This comment has been minimized.

Show comment
Hide comment
@jelmew

jelmew Sep 27, 2017

Hi. Is any progress made on this issue for using allargsconstructors instead of the all args constructor? Is someone working on this? (If not, I might take this one up)

jelmew commented Sep 27, 2017

Hi. Is any progress made on this issue for using allargsconstructors instead of the all args constructor? Is someone working on this? (If not, I might take this one up)

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Oct 3, 2017

Member

@jelmew as far as I know no one is working on this one. If you want feel free to take it and try it out. There is already an old PR #847 in case you want to have a look there as well. That code probably needs to be rebased on the latest master, as there are probably quite a few changes

Member

filiphr commented Oct 3, 2017

@jelmew as far as I know no one is working on this one. If you want feel free to take it and try it out. There is already an old PR #847 in case you want to have a look there as well. That code probably needs to be rebased on the latest master, as there are probably quite a few changes

@wjbuys

This comment has been minimized.

Show comment
Hide comment
@wjbuys

wjbuys Oct 25, 2017

Could a similar approach to Jackson's JDK8 ParameterNames module be used here? That works pretty nicely for constructing annotation-less POJOs through their constructors.

wjbuys commented Oct 25, 2017

Could a similar approach to Jackson's JDK8 ParameterNames module be used here? That works pretty nicely for constructing annotation-less POJOs through their constructors.

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Oct 26, 2017

Member

@wjbuys thanks for the link. Actually MapStruct can read the constructor parameter names even on Java 6. MapStruct is an Annotation Processor and runs during the compilation phase, so we are using the javax.lang.model and the names are already present.

Member

filiphr commented Oct 26, 2017

@wjbuys thanks for the link. Actually MapStruct can read the constructor parameter names even on Java 6. MapStruct is an Annotation Processor and runs during the compilation phase, so we are using the javax.lang.model and the names are already present.

@wjbuys

This comment has been minimized.

Show comment
Hide comment
@wjbuys

wjbuys Oct 26, 2017

Ah, of course, that makes sense. Thanks for the clarification.

wjbuys commented Oct 26, 2017

Ah, of course, that makes sense. Thanks for the clarification.

@gunnarmorling

This comment has been minimized.

Show comment
Hide comment
@gunnarmorling

gunnarmorling Oct 27, 2017

Member
Member

gunnarmorling commented Oct 27, 2017

@mohamnag

This comment has been minimized.

Show comment
Hide comment
@mohamnag

mohamnag Mar 1, 2018

I think I'm a bit too late to the party, however I wanted to ask why you think the @ConstructorProperties can not be reused? If by type you mean the constructor's parameter type, then it should be available by reflection from method signature right?

The thing is that there are other tools outside generating and using this like lombok for generation and jackson for deserialization. This means if it can be reused, it will just fall into place even for existing libs.

mohamnag commented Mar 1, 2018

I think I'm a bit too late to the party, however I wanted to ask why you think the @ConstructorProperties can not be reused? If by type you mean the constructor's parameter type, then it should be available by reflection from method signature right?

The thing is that there are other tools outside generating and using this like lombok for generation and jackson for deserialization. This means if it can be reused, it will just fall into place even for existing libs.

@almothafar

This comment has been minimized.

Show comment
Hide comment
@almothafar

almothafar Apr 30, 2018

@gunnarmorling I was thinking about a solution for this like:

public class Order {
    private final long id;
    private final String name;

    public Order(@MapperConstructor long id, @MapperConstructor String name) { ... }
 }

Or as what @mohamnag suggested:

@Mapper(config = CentralConfig.class)
public interface OrderMapper {
    public static OrderMapper INSTANCE = Mappers.getMapper(OrderMapper.class);

    @ConstructorProperties(source = "name", target = "name")
    Order map(OrderDTO orderVO);

    @InheritInverseConfiguration
    OrderDTO map(Order order);
}

@gunnarmorling I was thinking about a solution for this like:

public class Order {
    private final long id;
    private final String name;

    public Order(@MapperConstructor long id, @MapperConstructor String name) { ... }
 }

Or as what @mohamnag suggested:

@Mapper(config = CentralConfig.class)
public interface OrderMapper {
    public static OrderMapper INSTANCE = Mappers.getMapper(OrderMapper.class);

    @ConstructorProperties(source = "name", target = "name")
    Order map(OrderDTO orderVO);

    @InheritInverseConfiguration
    OrderDTO map(Order order);
}
@ade90036

This comment has been minimized.

Show comment
Hide comment
@ade90036

ade90036 Jul 8, 2018

Any update on this?

Feature requrst opened since 2013 and no concrete solution as yet.

I'm on the same boat as others. Need to work with immutable objects. Hence mapping via constructor is a must.

Regards

ade90036 commented Jul 8, 2018

Any update on this?

Feature requrst opened since 2013 and no concrete solution as yet.

I'm on the same boat as others. Need to work with immutable objects. Hence mapping via constructor is a must.

Regards

@sjaakd

This comment has been minimized.

Show comment
Hide comment
@sjaakd

sjaakd Jul 8, 2018

Contributor

A possible constructor approach is not as simple as it seems. However support for immutables comes in the form of builders in the upcoming 1.3

Contributor

sjaakd commented Jul 8, 2018

A possible constructor approach is not as simple as it seems. However support for immutables comes in the form of builders in the upcoming 1.3

@ade90036

This comment has been minimized.

Show comment
Hide comment
@ade90036

ade90036 Jul 8, 2018

Hi, thanks for the prompt reply.

I have guess that. 😀

The constructor approach nicely fits in with other libraries such as lombok and Jackson.

Would you be so kind to point me to PR or Issue number to see if this is something that would work in my scenario?

I'm currently using mapstruct with lombok for standard bean / bean mapping but for entity beans (database) objects where IDs are immutable I'm using custom converter.

Regards

ade90036 commented Jul 8, 2018

Hi, thanks for the prompt reply.

I have guess that. 😀

The constructor approach nicely fits in with other libraries such as lombok and Jackson.

Would you be so kind to point me to PR or Issue number to see if this is something that would work in my scenario?

I'm currently using mapstruct with lombok for standard bean / bean mapping but for entity beans (database) objects where IDs are immutable I'm using custom converter.

Regards

@sjaakd

This comment has been minimized.

Show comment
Hide comment
@sjaakd

sjaakd Jul 8, 2018

Contributor

There's an old PR pending. We had much internal discussion on that one. The problem is that you need to relate constructor arguments with source accessors. Remember: they can have the same type, so you must have a (compile safe) mechanism to point which arg goes where... Also selecting such constructor is not easy.

So.. this particular PR kept on dangling. Its hard to think of all situations here. So be careful were you put your teeth in 😄. I can't guarantee it will be merged.

Lombok supports a builder pattern by the way.. Not sure about Jackson though.

Contributor

sjaakd commented Jul 8, 2018

There's an old PR pending. We had much internal discussion on that one. The problem is that you need to relate constructor arguments with source accessors. Remember: they can have the same type, so you must have a (compile safe) mechanism to point which arg goes where... Also selecting such constructor is not easy.

So.. this particular PR kept on dangling. Its hard to think of all situations here. So be careful were you put your teeth in 😄. I can't guarantee it will be merged.

Lombok supports a builder pattern by the way.. Not sure about Jackson though.

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