#2239 matching generics - #2320

Thans for the review @filiphr . I'll get back on that later this week.

Regarding the failing test. I think that the test should pass. What I am having troubles to see is which change is causing this test to fail? When looking up the lifecycle methods we are passing the correct type I presume (the Order.Builder).

My initial idea was:

Look at L67 in MethodMatcher.

I need to match the sourceTypes with the targetType. That is all info I've got, right?

The method I need to match is public Order afterWithBuilderTargetReturningTarget(@MappingTarget Order.Builder orderBuilder), right?

It contains both:

1. ReturnType: Order
2. MappingTarget Order.Builder

How can I match and the returntype and the mappingtarget from a single targetType?

But, I'll take some distance and see if I miss something.. (Probably 😄 ) I'll get back on this one.

The remainder of your comments have been processed.

I did some further analysis.. MethodMatcher works according to spec. So, it does not look anymore what the source types actually are. It just tries to line up:

1. targetType with return type
2. sourceType[] with source types

There's no magic here.

What it receives is, in this case:

1. targetType = Order.builder
2. sourceTypes[0] = Order.builder
   The latter one is a @MappingTarget

It tries to match this with:

public Order afterWithBuilderTargetReturningTarget(@MappingTarget Order.Builder orderBuilder) which breaks on the no-match onthe returntype.

So, if we want to succeed, the targetType needs to be Order

What I did to make it more easy, remove all other stuff:

@Mapper
public interface OrderMapper {

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

    @Mapping( target = "items", ignore = true )
    Order map(OrderDto source, @Context MappingContext context);

  //  Item map(ItemDto source, @Context MappingContext context);
}

public class MappingContext {

    private final List<String> invokedMethods = new ArrayList<String>();
//
//    @BeforeMapping
//    public void beforeWithoutParameters() {
//        invokedMethods.add( "beforeWithoutParameters" );
//    }
//
//    @BeforeMapping
//    public void beforeWithTargetType(OrderDto source, @TargetType Class<Order> orderClass) {
//        invokedMethods.add( "beforeWithTargetType" );
//    }
//
//    @BeforeMapping
//    public void beforeWithBuilderTargetType(OrderDto source, @TargetType Class<Order.Builder> builderClass) {
//        invokedMethods.add( "beforeWithBuilderTargetType" );
//    }
//
//    @BeforeMapping
//    public void beforeWithTarget(OrderDto source, @MappingTarget Order order) {
//        invokedMethods.add( "beforeWithTarget" );
//    }
//
//    @BeforeMapping
//    public void beforeWithBuilderTarget(OrderDto source, @MappingTarget Order.Builder orderBuilder) {
//        invokedMethods.add( "beforeWithBuilderTarget" );
//    }
//
//    @AfterMapping
//    public void afterWithoutParameters() {
//        invokedMethods.add( "afterWithoutParameters" );
//    }
//
//    @AfterMapping
//    public void afterWithTargetType(OrderDto source, @TargetType Class<Order> orderClass) {
//        invokedMethods.add( "afterWithTargetType" );
//    }
//
//    @AfterMapping
//    public void afterWithBuilderTargetType(OrderDto source, @TargetType Class<Order.Builder> builderClass) {
//        invokedMethods.add( "afterWithBuilderTargetType" );
//    }
//
//    @AfterMapping
//    public void afterWithTarget(OrderDto source, @MappingTarget Order order) {
//        invokedMethods.add( "afterWithTarget" );
//    }
//
//    @AfterMapping
//    public void afterWithBuilderTarget(OrderDto source, @MappingTarget Order.Builder orderBuilder) {
//        invokedMethods.add( "afterWithBuilderTarget" );
//    }

    @AfterMapping
    public Order afterWithBuilderTargetReturningTarget(@MappingTarget Order.Builder orderBuilder) {
        invokedMethods.add( "afterWithBuilderTargetReturningTarget" );

        return orderBuilder.create();
    }

    public List<String> getInvokedMethods() {
        return invokedMethods;
    }
}

Put a debug statement on the MethodMatcher L 85 (my branch)

And just inspect, targetType, sourceTypes

You can step back in the stack to see what happens.

OK I see it now. Currently the MethodMatcher calls Type candidateResultType = candidateMethod.getResultType(); when evaluating whether the result type matches. The source method returns the type from the @MappingTarget here and therefore it works.

If you replace the getReturnType with getResultType in GenericAnalyser then the test is green.

I think that what you are doing is correct there. However, we need to provide this information when we are gathering the @BeforeMapping and @AfterMapping lifecycle methods and call it twice, once with the final type and once with the builder. However, I think that this is not that easy with our current setup.

If you replace the getReturnType with getResultType in GenericAnalyser then the test is green.

Yup.. But test: ExternalSelectionTest#shouldSelectGeneratedExternalMapperWithImportForPropertyType then fails..

Yup.. But test: ExternalSelectionTest#shouldSelectGeneratedExternalMapperWithImportForPropertyType then fails..

That's interesting. I see positionMappingTargetType in the new code. What is that used for? Not sure what we should do for that

Let's take one step back to the generated code:

`public class OrderMapperImpl implements OrderMapper {

    @Override
    public Order map(OrderDto source, MappingContext context) {
        if ( source == null ) {
            return null;
        }

        Builder order = Order.builder();

        Order target = context.afterWithBuilderTargetReturningTarget( order );
        if ( target != null ) {
            return target;
        }

        return order.create();
    }
}

What is actually the reason that we distinguish here? Why is the return type unequal to the result type (@MappingTarget). .Is that because the context itself could be generated (and hence we exclude that possibility)?

So why couldn't we do the:

if (target != null ) {
  return target.create();
}

Like said.. generating a context might be the reason..

That's interesting. I see positionMappingTargetType in the new code. What is that used for? Not sure what we should do for that

That's a reminiscence of an older version of the solution.. It can be removed.. I'll do so.

@filiphr I've provided a fix.. pursue a solution on different spots than the lifecycle methods.. But that could be done in new PRs with additional testcases. For the time being this goes far enough.. Have a look if you agree with the approach.

Really good job with the tests in this one @sjaakd, really thorough 😄

Really good job with the tests in this one @sjaakd, really thorough 😄

Thanks. 😄

I'll still add some testcases in the future probably to add some more realistic scenarios I found when implementing this. Just to make sure they work as well. But first lets get the bulk out.

I answered some comments I missed in earlier review. Have a look if you agree. If you agree, I'll do the merge.

I answered some comments I missed in earlier review. Have a look if you agree. If you agree, I'll do the merge.

From your 3 comments, I agree with one and resolve it. There are 2 more left.

• The enum one. This is not that big of a deal
• The one with the Ambiguous method mapping. This is a big deal since the test there doesn't have any raw Set so we shouldn't give such an error.

I'll still add some testcases in the future probably to add some more realistic scenarios I found when implementing this. Just to make sure they work as well. But first lets get the bulk out.

Once this is done, check https #2377, I think that it is solved by this PR, but it would be good to add that test case as well.