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

ok to test

This is a comment from the previous PR:

@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.

I also forgot to write in my review comment:

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

@filiphr Could don't replay on your comment.

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

What kind of scenarios you are talking about?
For now I found only one possible issue (I don't think that somebody uses it)
in case of target has right public field but wrong setter. The setter has more priority on public field which is logical.

` static class Target {
public String a;

    public void setA(SomeType i) {
        //illegal one
    }

    static class Source {
        public String getA() {
            return "val";
        }
    }
`

What kind of scenarios you are talking about?

What I meant is to have a case when typeUtils.isSameType( previousType, preferredType ) evaluates to true, and typeUtils.isAssignable( nextType, preferredType ) && !typeUtils.isAssignable( previousType, preferredType ) evaluates to true. I can't really think of an example for this right now.

I set a breakpoint at those 2 places and they were never hit.

Also this won't work for @InheritInverseConfiguration and @InheritConfiguration. You can reproduce this by adding a public constructor to the Source and setter with long.

Also this to the SourceTargetMapper:

@InheritInverseConfiguration
Source targetToSource(Target target);

For @InheritConfiguration you need to use a @MapperConfig to be able to reproduce.

The reason for this is that the MappingOptions are mutable and are updated via the MappingOptions#applyInheritedOptions.

As an idea consider creating the sourceReadAccessors when you need them. There are 3 places where they are used:

• BeanMappingMethod.Builder#setupMethodWithMapping(Method) - here you can most probably generate them when you need them
• Mapping#hasPropertyInReverseMethod(String, SourceMethod) - here we only need to check if a write accessor exists for a property and the sourceReadAccessors are not important
• TargetReference.BuilderFromTargetMapping#getTargetEntries(Type, String[])` - Here you need to decide when it is best to create them, maybe when you set the method.

By the way @quantumlexa you can add an @author tag to the new classes that you are adding to the code if you want to 😉

Hi, @filiphr
Sorry for late reply.
unfortunately - reverse logic is not so easy as I expected :)
The problem is not only in generation sourceReadAccessors on the fly.
org.mapstruct.ap.internal.model.PropertyMapping.MappingBuilderBase#targetProperty method set's targetType to targetProp.getType(); from mappings.
when I changed code from
this.targetType = targetProp.getType();

to

            if ( method.getSourceReadAccessors().get( Executables.getPropertyName( targetWriteAccessor ) ) != null ) {
                TypeMirror accessedType = method.getSourceReadAccessors()
                    .get( Executables.getPropertyName( targetWriteAccessor ) )
                    .getAccessedType();
                Type type = context.getTypeFactory().getType( accessedType );
				this.targetType = type;
            }

my test with reverse mapping succeeded (~ 200 other test failed )
And place code here doesn't look as a right solution.
Any suggestion regarding possible fix?

@quantumlexa I think that you are looking at the wrong location. Maybe that needs to be done in the TargetReference#BuilderFromTargetMapping#getTargetEntries(), maybe the nextType needs to be addapted there. I think that you should try and use the isReverse that currently is just ignored in the Mapping#init(), it is also passed to the BuilderFromTargetMapping but it is not set and it is not used in there. @sjaakd do you perhaps know what was the idea behind that?

Can you perhaps push the test with the inverse?

@filiphr
Added failing test for reverse mapping.

@quantumlexa I know how you can approach the problem, and I think you can solve it :). The MappingOptions are not immutable, and they can be modified during the inheritance resolvation. The implementation will also not work when you are inheriting a configuration see my commit to your branch for an example.

MappingOptions are marked as fully initialized via MappingOptions#markAsFullyInitialized(). There is also Mapping#copyForInheritanceTo() which copies the mapping for the given method. The SourceReference also has method copyForInheritanceTo, I think that one should be added to the TargetReference and there we can create a new TargetReference. The only thing is that you shouldn't use method.getSourceReadAccessors() when you create copy for inheritance, if you do that, you will get the same result, the Mappings will not be complete until all the inheritance is done.

Maybe you can also see PR #1157 where I have added the reverse source parameter to the BuilderFromTargetMapping, maybe you could reuse the logic from it as well.

@quantumlexa I forgot to write. This should also work for nested target properties.

For example for the following mapping:

@Mapping(target = "nested.updateOn", source = "sourceNested.updateOnTarget");
Target map(Source source);

And the model looks like:

class Target {

    private NestedTarget nested;

    //the normal getters and setters
    public void setNested(NestedSource nestedSource) {
        //Here some custom conversion
    }
}
class Nested {
    private Date updateOn;
    //This is like your example in the PR
}

class Source {
    private NestedSource nestedSource;

    //getters and setters
}

class NestedSource { 
    private long updateOnTarget;

    //getters and setters
}

I think with the current state of the PR it won't work as it only considers the read accessors of the first source type.

@quantumlexa do you still have time for this PR? Is there something that we can help you out to bring it to the finish line?

We are planning to do a release next week, if we don't manage to resolve this PR until then, we'll merge it for another release

Hi, @filiphr
I was in quite long vacation and probably I'm not able to do it on time.
But give me couple of days please - I'll try to solve it.

@quantumlexa no problem, take your time. If you manage to do it by the beginning of next and it works OK, then we will include it in Beta3, otherwise we'll do it in the release after Beta3. We will also try to do the release after Beta3 faster so we can also have a Final release soon after that.

@quantumlexa do you need some help with the PR? Do you think that you will have time to finish it up?

hi, @filiphr
yep, I have some problems here.
the main problem - calculation of reverse mapping uses getPropertyWriteAccessors where argument of it: Map<String, Accessor> sourceAccessor.
To map target setter and source getter I need this mapping, so as a result - classic circular dependency.