#1135 added test and implementation for overloading by quantumlexa · Pull Request #1136 · mapstruct/mapstruct

quantumlexa · 2017-03-12T18:03:33Z

Mapstruct does support overloading of target setters(see: Issue892Test test) but choosing preferred type logic is based on getter type of target - not source, which looks a bit strange.
Here is straightforward implementation of this

mapstruct-robot · 2017-03-12T18:03:33Z

(Message from the pull-request-builder): Admins, please verify this patch for it to build in the pull-request-builder.

gunnarmorling · 2017-03-13T20:11:43Z

ok to test

filiphr

@quantumlexa a really good addition to the framework. Thanks a lot for doing the PR. In general it is good.

I have some comments inline.

Maybe it will be good to add some tests for which MapStruct will generate a forged method (see NestedBeanMappingsTest). You can add fields to those places as well if you want to.

filiphr · 2017-03-17T23:48:02Z

processor/src/main/java/org/mapstruct/ap/internal/model/BeanMappingMethod.java

-            Map<String, Accessor> accessors = method.getResultType().getPropertyWriteAccessors( cms );
+            Map<String, Accessor> srcReadAccessors = Collections.emptyMap();
+            if (sourceMethod instanceof SourceMethod) {
+                srcReadAccessors = ( (SourceMethod) sourceMethod ).getGetSourceReadAccessors();


I don't think that it is entirely correct to use this only for SourceMethod(s). We also have the ForgedMethod(s) that are used for nested target mappings, automatic name based methods creation etc. Maybe you can add a method to the new MappingMethodUtils (you'll have to rebase on top of the latest master).

On the other side, you won't be able to use memoization. You can add the getGetSourceReadAccessors() or even name it getSourceReadAccessors to the Method interface and do memoization where needed (SourceMethod and ForgedMethod, the others can just return empty map`).

filiphr · 2017-03-17T23:49:18Z

processor/src/main/java/org/mapstruct/ap/internal/model/common/Type.java

        return presenceCheckers;
    }

+    public Map<String, Accessor> getPropertyWriteAccessors( CollectionMappingStrategyPrism cmStrategy ) {


Can you copy the previous Javadoc here and add the sourceAccessor(s) parameter to the new method signature?

filiphr · 2017-03-18T00:01:08Z

processor/src/main/java/org/mapstruct/ap/internal/model/common/Type.java


-    private Type determinePreferredType(Accessor readAccessor) {
+    private TypeMirror extractTypeMirror( Accessor accessor ) {
+        return accessor.getExecutable().getParameters().get( 0 ).asType();


This is wrong it can lead to NPE. The accessor does not always have an executable. MapStruct can also map public field and that is why we have introduced the Accessor. I think that this can be done by only using the MapStruct Type and not the TypeMirror. Maybe you can also use Type in the isBetter check as well.

filiphr · 2017-03-18T00:01:58Z

processor/src/main/java/org/mapstruct/ap/internal/util/Executables.java

     */
-    public static List<Accessor> getAllEnclosedAccessors(Elements elementUtils, TypeElement element) {
+    public static List<Accessor> getAllEnclosedAccessors( Elements elementUtils, TypeElement element ) {
+        if (element == null) {


Did you have problems with an NPE here?

filiphr · 2017-03-18T00:04:45Z

processor/src/test/java/org/mapstruct/ap/test/overloading/OverloadingTest.java

+
+@WithClasses({
+        SourceTargetMapper.class,
+        SourceTargetMapperPropertyBean.class,


What is the difference between the SourceTargetMapper and the SourceTargetMapperPropertyBean is it only the @Mapping annotation? Btw. when the source and target have the same name you don't need it. If you want to test that it works for different name you can just add new fields :).

filiphr · 2017-03-18T00:06:20Z

processor/src/test/java/org/mapstruct/ap/test/overloading/OverloadingTest.java

+            Source.class,
+            Target.class
+    })
+    public void testShouldGenerateCorrectMapperImplementationJsr() {


Why do you need this test method? Also the @WithClasses on the method is not needed, because those classes are already added on the class level. We usually add @WithClasses to methods when we want to test errors within the mappers.

filiphr · 2017-03-18T00:08:23Z

processor/src/test/java/org/mapstruct/ap/test/overloading/OverloadingTest.java

+    public void testShouldGenerateCorrectMapperImplementation() {
+        Source source = new Source( new Date() );
+        Target target = SourceTargetMapper.INSTANCE.sourceToTarget( source );
+        Assert.assertTrue( target.getUpdatedOn() > 0 );


Can you use assertj Assertions here? I would also add the implementation class as a comparison fixture to the GeneratedSource rule. You can have a look at the ArrayMappingTest and the ScienceMapperImpl.java fixture for how it works.

As this PR adresses only a behavioral change and doesn't structurally change the generated code, I don't think we need that fixture here - we should try keep the usage of those to a minimum, as we'll always need to keep maintaining them in the future... 🙂

@agudian is right for the fixture. Just to make sure that we don't miss this in the future, maybe the setUpdateOn(long updateOn) in the Target should throw an exception. That method should not be called.

removed body of test at all. It should compile ant that's enough.

filiphr

There are still some issues with the PR:

I think that by mistake you have reverted the changes you had (the commits that moved the getGetSourceReadAccessors to the Method interface and it's renaming
This solution only works if the target and source properties have the same name. If they are different, but they are matched with @Mapping then it doesn't work

filiphr · 2017-03-31T19:18:33Z

processor/src/main/java/org/mapstruct/ap/internal/model/source/SourceMethod.java

        return getMappingTargetParameter() != null;
    }
+
+    public Map<String, Accessor> getGetSourceReadAccessors() {


It looks like your last version was reverted to before you moved the getSourceReadAccessors to the Method interface.

filiphr · 2017-03-31T19:20:07Z

processor/src/main/java/org/mapstruct/ap/internal/model/common/Type.java

     * @return an unmodifiable map of all write accessors indexed by property name
     */
-    public Map<String, Accessor> getPropertyWriteAccessors( CollectionMappingStrategyPrism cmStrategy ) {
+    public Map<String, Accessor> getPropertyWriteAccessors( CollectionMappingStrategyPrism cmStrategy,


This will only work when the source and target properties are the same.

If you modify the name in either Source or Target and accordingly use @Mapping then it'll fail.

filiphr · 2017-03-31T19:28:56Z

processor/src/main/java/org/mapstruct/ap/internal/model/common/Type.java

    }

+    private TypeMirror extractTypeMirror( Accessor accessor ) {
+        return accessor.getExecutable().getParameters().get( 0 ).asType();


In theory you should not be able to get up to here if your Accessor is field. However, I think that a null check needs to be done on the accessor.getExecutable().

filiphr · 2017-03-31T19:39:51Z

@agudian, I had another look at the PR. There are some issues which are addressed in my second review. However, I don't think that there is an issue with the selectors:

If none of the source accessors can be matched to the target then the type of the getter is used
If a source accessor is used, then a direct mapping will always be done, because the type of the source is either the same as the target, or it is assignable to the target.

@quantumlexa Can you perhaps add more properties and make sure that all the branches in Type#isBetter are tested?

quantumlexa · 2017-04-01T18:47:59Z

due to some errors with git branch - recreated it in: #1136

filiphr · 2017-04-02T07:06:08Z

PR moved to #1167

mapstruct#1135 added test and implementation for overloading

b099446

filiphr mentioned this pull request Mar 17, 2017

Ambiguous Mapping Migration Issues from CR1 to CR2/Final #1003

Closed

filiphr requested changes Mar 18, 2017

View reviewed changes

quantumlexa force-pushed the 1135 branch 2 times, most recently from a29f5ba to b099446 Compare March 31, 2017 02:49

filiphr requested changes Mar 31, 2017

View reviewed changes

quantumlexa force-pushed the 1135 branch from 4fdc103 to b099446 Compare April 1, 2017 18:31

filiphr closed this Apr 2, 2017

Uh oh!

Conversation

quantumlexa commented Mar 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mapstruct-robot commented Mar 12, 2017

Uh oh!

gunnarmorling commented Mar 13, 2017

Uh oh!

filiphr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filiphr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filiphr commented Mar 31, 2017

Uh oh!

quantumlexa commented Apr 1, 2017

Uh oh!

filiphr commented Apr 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

quantumlexa commented Mar 12, 2017 •

edited

Loading