Skip to content

Mapping using target constructor - #2074

Merged
filiphr merged 2 commits into
mapstruct:masterfrom
filiphr:constructor
Apr 26, 2020
Merged

Mapping using target constructor#2074
filiphr merged 2 commits into
mapstruct:masterfrom
filiphr:constructor

Conversation

@filiphr

@filiphr filiphr commented Apr 19, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Fixes #73.

It would be good to get some feedback on it.

  • Have I missed some test cases?
  • Is the generated code acceptable?
  • Is the disambiguation for constructors acceptable?

@filiphr
filiphr requested a review from sjaakd April 19, 2020 15:56
@filiphr filiphr mentioned this pull request Apr 19, 2020
Merged
sjaakd
sjaakd reviewed Apr 21, 2020
View reviewed changes

@sjaakd sjaakd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Filip,

What a work 💯 ! I went through your PR. Loads of test 👍

Some of the things we should discuss:

  1. opt-out
  2. mixed scenarios (half constructor, half setters)
  3. name base mapping (I think its there, but doc needs update).
  4. what if you don't have control over the target?

testcases are really plentiful. I'll ponder on it a bit more.

Comment thread documentation/src/main/asciidoc/chapter-3-defining-a-mapper.asciidoc Outdated
Comment thread documentation/src/main/asciidoc/chapter-3-defining-a-mapper.asciidoc Outdated
Comment thread documentation/src/main/asciidoc/chapter-3-defining-a-mapper.asciidoc
Comment thread processor/src/main/java/org/mapstruct/ap/internal/model/CollectionAssignmentBuilder.java Outdated
Comment thread ...org/mapstruct/ap/test/constructor/constructorproperties/SimpleConstructorPropertiesTest.java
Comment thread ...src/test/java/org/mapstruct/ap/test/constructor/nestedsource/_target/AdderUsageObserver.java
@filiphr

filiphr commented Apr 23, 2020

Copy link
Copy Markdown
Member Author

Thanks a lot for the review @sjaakd.

Regarding your questions:

  1. opt-out

In theory adding a public empty constructor would make MapStruct use that one. If there is an @ObjectFactory we would use that. Otherwise if you opt out there will be a compile error anyways. So not sure from what you will be opting out.

  1. mixed scenarios (half constructor, half setters)

There are no tests for that, but it would work. Although if you have the same property in a constructor and a setter then the one from the constructor would be used. Will add tests for that as well.

  1. name base mapping (I think its there, but doc needs update).

Nice spot, will add that.

  1. what if you don't have control over the target?

Well you still have the option to add an @ObjectFactory which always wins. Otherwise if the target has multiple constructors and one of them has no parameters then that one would be used. I don't see a big difference compared to what is happening right now.

@filiphr

filiphr commented Apr 25, 2020

Copy link
Copy Markdown
Member Author

@sjaakd I added a new commit and applied your feedback on it. Let me know what you think

sjaakd
sjaakd requested changes Apr 25, 2020
View reviewed changes

@sjaakd sjaakd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some documentation nagging :)

Comment thread documentation/src/main/asciidoc/chapter-3-defining-a-mapper.asciidoc Outdated
Comment thread documentation/src/main/asciidoc/chapter-3-defining-a-mapper.asciidoc Outdated
Comment thread documentation/src/main/asciidoc/chapter-3-defining-a-mapper.asciidoc Outdated
Comment thread documentation/src/main/asciidoc/chapter-3-defining-a-mapper.asciidoc Outdated
@filiphr

filiphr commented Apr 25, 2020

Copy link
Copy Markdown
Member Author

I've added more commits:

  • Update the documentation with your suggestions @sjaakd
  • Add Kotlin Data class integration test

@filiphr filiphr mentioned this pull request Apr 26, 2020
Open
filiphr added 2 commits April 26, 2020 12:04
@filiphr
Accessor#getSimpleName should return a String
5ba1fcf
@filiphr
mapstruct#73 Add support for using constructor arguments when instant…
7bd5ca5 
…iating mapping targets

By default the constructor argument names are used to extract the target properties.
If a constructor is annotated with an annotation named `@ConstructorProperties` (from any package) then it would be used to extract the target properties.

If a mapping target has a parameterless empty constructor it would be used to instantiate the target.
When there are multiple constructors then an annotation named `@Default` (from any package) can be used to mark a constructor that should be used by default when instantiating the target.

Supports mapping into Java 14 Records and Kotlin data classes out of the box
@filiphr filiphr mentioned this pull request Apr 26, 2020
Open
@filiphr
filiphr merged commit 2b2299a into mapstruct:master Apr 26, 2020
@filiphr
filiphr deleted the constructor branch April 26, 2020 10:44
@filiphr filiphr mentioned this pull request Apr 27, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sjaakd sjaakd sjaakd approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Make use of constructor arguments when instantiating mapping targets

2 participants

@filiphr @sjaakd