You are right, the last property name or the source parameter name if there are no properties should be enough. The targetPropertyName is the target (name in the PropertyMapping) we need to source.

In the example the targetPropertyName is id in both mappings.

Maybe we can use the SourceRHS.getSourceReference() instead.

I think what you are explaining is in line with my suggestion in #1048.

And yes using the sourceRef it will work. I will push that.

By the way, you are commenting on my commit and not the PR :)

Just as info, I have not squashed because @sjaakd comments are on my commit in my branch and I think that they would be lost if I force push. When merging it can be squashed into one commit :)

So if I have two nested property mappings with different path-parts in the middle, this will still create two different methods? E.g. customer.billingAddress.street and customer.invoiceAddress.street? Just asking because of the possibility to only include the last part of the property path... and I'm not sure I understand what's really in those variables... 😁

Thanks for the example @agudian. In your example we need to actually create 4 methods
we also need to map the customer. However, the current implementation does not work for that. We need to rethink it, it doesn't work when I include all sourceProperties from the SourceReference as well.

@filiphr : I forgot why.. but why is it that we hand the complete source parameter instead of source properties in these scenarios? I'm not sure if that would matters simpler in all cases..

@sjaakd, sorry, but I don't quite understand what you mean. Can you maybe tell me where in the code I should look so I can understand it better?

@sjaakd, sorry, but I don't quite understand what you mean. Can you maybe tell me where in the code I should look so I can understand it better?

Well, consider the original problem:

@Mapper
public interface MyMapper {

@Mappings({
        @Mapping(source = "senderId", target = "sender.id"),
        @Mapping(source = "recipientId", target = "recipient.id")
    })
    Entity toEntity(Dto dto);
}

Now (prior to this PR) we create a mapping method:

    protected Client dtoToClient(Dto dto) {
        if ( dto == null ) {
            return null;
        }

        Client client = new Client();

        client.setId( dto.getRecipientId() );

        return client;
    }

But what if we would have generated the following

    protected Client stringToClient(String string) {
        if ( dto == null ) {
            return null;
        }

        Client client = new Client();

        client.setId( string );

        return client;
    }

So, when we cannot do symmetric stuff, we hand the source properties in stead of the source parameter.. The call site would then look something like this:

public class MyMapperImpl implements MyMapper {

    @Override
    public Entity toEntity(Dto dto) {
        if ( dto == null ) {
            return null;
        }

        Entity entity = new Entity();

        entity.setRecipient( stringToClient( dto.getRecepientId() ) );
        entity.setSender( stringToClient( dto.getSenderId() ) );

        return entity;
    }

Now I understand what you mean @sjaakd, thanks. You are saying that we do something similar to the NestedPropertyMappingMethod. I didn't think about that one, I can certainly try it out during the week 😄, or if you want to do it, feel free to work on the branch

or if you want to do it, feel free to work on the branch

I'll have a look and see if I can dig into this.. Not sure though whether I'll find some time 😄

@filiphr .. Did a first attempt... I ran into some issues..

First of all I changed the implementation on line 198 of NestedTargetPropertyMappingHolder

                   if ( !groupedSourceReferences.nonNested.isEmpty() ) {
                        for ( Mapping mapping : groupedSourceReferences.nonNested ) {

                            MappingOptions nonNestedOptions = MappingOptions.forMappingsOnly(
                                groupByTargetName( groupedSourceReferences.nonNested ),
                                true
                            );

                            PropertyMapping propertyMapping = createPropertyMappingForNestedTarget(
                                nonNestedOptions,
                                targetProperty,
                                mapping.getSourceReference(),
                                forceUpdateMethod
                            );

                            if ( propertyMapping != null ) {
                                propertyMappings.add( propertyMapping );
                            }

                            handledTargets.add( entryByTP.getKey().getName() );
                        }

                        unprocessedDefinedTarget.put( targetProperty, groupedSourceReferences.notProcessedAppliesToAll );
                    }
                }
            }

That fails on a an interesting line 642 in PropertyMapping, so I got curious.. I commented out those lines

//            if ( sourceType.isPrimitive() || targetType.isPrimitive() ) {
//                return null;
//            }

The result is quite surprising. The call site of the implementation works as a charm:

   @Override
    public Entity toEntity(Dto dto) {
        if ( dto == null ) {
            return null;
        }

        Entity entity = new Entity();

        entity.setSender( longToClient( dto.senderId ) );
        entity.client = clientDtoToClient( dto.sameLevel );
        entity.client2 = clientDtoToClient( dto.sameLevel2 );
        entity.nested2 = clientDtoToNestedClient( dto.level2 );
        entity.nested = clientDtoToNestedClient( dto.level );
        entity.setRecipient( longToClient1( dto.recipientId ) );
        long id = dtoNestedDto2Id( dto );
        entity.setId2( id );
        long id1 = dtoNestedDtoId( dto );
        entity.setId( id1 );

        return entity;
    }

(note I did not remove your forcing part ye, hence longToClient1 and longToClient.)..

however (of course I should say) MapStruct cannot (yet) generate a mapping from a primitive to a bean.

    protected Client longToClient(long long1) {
        if (  ) {
            return null;
        }

        Client client = new Client();

        client.nestedClient = longToNestedClient( long1.senderId );

        return client;
    }

So.. the question is now: is it a good idea to allow that and generate a proper mapping for such cases??

So.. the question is now: is it a good idea to allow that and generate a proper mapping for such cases??

To answer that.. It seems that on the source side we do this already 😄

        long id = dtoNestedDto2Id( dto );
        entity.setId2( id );

The empty null check can be fixed by adding an additional check on line30 in BeanMapping.ftl:

<#if !mapNullToDefault && !sourceParametersExcludingPrimitives.empty>

@sjaakd what if we generate something similar to the NestedProperryMappingMethod, instead of a Property mapping?

@sjaakd what if we generate something similar to the NestedProperryMappingMethod, instead of a Property mapping?

Thought about that as well. 2 reasons:

1. Re-use. If we generate a PropertyMapping calling ForgedBeanMapping, MapStruct will automatically generate a method calling another method. So in your example

protected Client longToClient(long long1) {

        Client client = new Client();

        client.nestedClient = longToNestedClient( long1.senderId );

        return client;
    }

2. We already have cases were we use an parameter as source. So it should be easy.

What's going wrong is that the MappingOptions should be poppedToParameter. That's what I still need to do: so the last remaining propertyEntry should become the parameter.

In the mean time I wrote a new issue.. This Map<String, List<Mapping>> is driving me crazy 😄

Completely agree with you.

How will this work when you have 2 properties that need to be non-nested mapped? Example:

See the fishTankToMaterialDto1 method in the FishTankToMaterialImpl fixture

@sjaakd the things that you were trying out, do you have them locally or somehwere in your branches (I couldn't find them). I think that it will be good if we can wrap this one up so we can do the release

see #1203 ..

@sjaakd I had a look at it and I also played around with it. We have the following problems:

First of all we shouldn't generate this:

 protected Client stringToClient(String string) {
    if ( dto == null ) {
        return null;
    }

    Client client = new Client();

    client.setId( string );

    return client;
}

But we should use update methods, because you can map multiple things into the client. This we can do and the generated code will look like:

 protected void stringToClient(String string, Client mappingTarget) {
    if ( string == null ) {
        return;
    }

    client.setId( string );
}

And of course the caller will create the client like we do in an update method. I already managed to do this. However, there are some issues when the source reference is null. Because we pop the source reference into a parameter, now we have to care about ignore, constant and expression in the NestedTargetPropertyMappingHolder#Builder as well.

Also the ReversingNestedSourcePropertiesTest is failing because some generated methods are reused and they shouldn't be reused.

What exactly don't you like with the current approach?

First of all we shouldn't generate this

I think we should. For 2 reasons..

1. We do already support this. If when we have no other "defined", "name-matched" @Mappings, we look for methods (select methods) which map the parameter itself into a target bean. This has been there for a long time. If we support that.. we should make the next step as well (generate methods for those).

2. Its the symmetric case for source method mapping. Here we step down the source until we reach the proper level.

But we should use update methods, because you can map multiple things into the client. This we can do and the generated code will look like:

This was my first thought as well. But now I don't think so. This would imply (in this case) that you have to pass multiple source parameters (and worse select them.).

protected void stringToClient(String string, Long lng, Client mappingTarget) {
   if ( string == null ) {
       return;
   }
  // what about null checking long?

   client.setId( string );
   client.setLong( lng );

}

Having said that, I think we need more time for this. Although this is a rather serious issue blocking a CR I would say. I also think we should fix #1197 first and then go back to this issue. When I started looking into this I got really confused by these constructs were you drag around mappings from multiple targets to multiple sources only for the sake of a deprecated enum mapping. We should re-map the old enum mapping asap to the new @ValueMapping and not bother all the other code with it or remove it...

1. We do already support this. If when we have no other "defined", "name-matched" @mappings, we look for methods (select methods) which map the parameter itself into a target bean. This has been there for a long time. If we support that.. we should make the next step as well (generate methods for those).

Are you talking about the NormalizingTest here? I think that we no longer do this, I think it is a bug, see my comment on the deNormalizeFont below.

2. Its the symmetric case for source method mapping. Here we step down the source until we reach the proper level.

It is similar but not the same. For the target we shouldn't create a new object all the time. Let's take our LetterMapper as an example. And the denormalization of the LetterEntity which by the way no longer uses the deNormalizeFont method (which might be a bug).

We generate this method:

protected FontDto letterEntityToFontDto(LetterEntity letterEntity) {
    if ( letterEntity == null ) {
        return null;
    }

    FontDto fontDto = new FontDto();

    fontDto.setSize( letterEntity.getFontSize() );
    fontDto.setType( letterEntity.getFontType() );

    return fontDto;
}

And we call it letterDto.setFont( letterEntityToFontDto( entity ) );

With the new proposal you are saying that we should call it like:

letterDto.setFont( fontSizeToFontDto( entity.getFontSize() ) );
letterDto.setFont( fontTypeToFontDto( entity.getFont() ) );

And we would generate will look like:

protected FontDto fontSizeToFontDto(int fontSize) {
    if ( fontSize== null ) {
        return null;
    }

    FontDto fontDto = new FontDto();

    fontDto.setSize( fontSize );

    return fontDto;
}

protected FontDto fontTypeToFontDto(String fontType) {
    if ( fontType== null ) {
        return null;
    }

    FontDto fontDto = new FontDto();

    fontDto.setFontType( fontType);

    return fontDto;
}

Why I thought by using update methods is not like in your example, but for each of the fontSizeToFontDto and fontTypeToFontDto we will pass the FontDto and the property that needs to be set, not all the mappings.

If you don't have time for #1197 I can have a go at it.

Are you talking about the NormalizingTest here? I think that we no longer do this, I think it is a bug, see my comment on the deNormalizeFont below.

No.. I meant: applyParameterNameBasedMapping l148, BeanMappingMethod.

We generate this method:

I don't think you're example is right.. In your example FontDto is already generated in the parent method before calling its siblings (type and size).

If there's no nesting anymore (no segments anymore) there's no more bean to map into.. The only thing you have to construct (and return) would be a FontSizeDto or FontTypeDto.. (iso FontDto), right?

I don't think you're example is right.. In your example FontDto is already generated in the parent method before calling its siblings (type and size).

The letterEntityToFontDto method is correct, I copied it from the generated code from this PR and we generate the same for 1.2.0.Beta2 as well. fontSize and fontType are int and String respectively.

Maybe we need to schedule a call and/or talk on a chat to clarify this PR better.

Thanks a lot for the review @sjaakd. We also have #1212 to reconsider the approach.