WIP: #2092 merge unmappedSourceProperties of BeanMappings

[chris922]

I tackled #2092 and it was more complicated than I thought. Maybe it is even though not a bug, but now an enhancement.

Starting point are the changes in MappingMethodOptions.

I noticed that @BeanMapping will not be merged at all and in case a method is annotated with @BeanMapping and @InheritConfiguration the @BeanMapping will not be inherited and/or merged.

Therefore I implemented inheritance by following the pattern used for DelegatingOptions. The main idea is: If a method inherits the configuration and both methods are annotated with @BeanMapping I set the inherited @BeanMapping as the next DelegatingOptions.
This will ensure that all options of the other @BeanMapping will be inherited.

For the ignoreUnmappedSourceProperties stuff I had to implement a logic to join the lists.

I added the same logic for @MapMapping and @IterableMapping as well.

The following questions came up during the analyze and implementation:

1. Should we join ignoreUnmappedSourceProperties at all? Imho we should do this because I probably would also expect this. Nevertheless there might be some reasons that the method inheriting the configuration should have an own configuration of these properties
2. I am not sure if I understood the SelectionParameters correctly, right now I try to join the qualifiers, but maybe that is not the right way and they shouldn't be joined
3. Is it fine to change the next of the BeanMappingOptions? Could it be that there will then be a totally different inheritance flow? Looked for me that the delegating options works in the way that next in this case is the @Mapper config, then the @MapperConfig and so on. Now I change next to the other @BeanMapping and maybe this will lead to a different @Mapper config? Could this be?!

I marked this PR as WIP to get some first feedback from you, if everything is fine I would like to update the documentation to highlight that the properties will be merged

[filiphr]

Thanks for taking over this one @chris922.

Let me try to answer the points one by one. I also haven't looked at the code yet. Talking conceptually right now.

1. Should we join ignoreUnmappedSourceProperties at all? Imho we should do this because I probably would also expect this. Nevertheless there might be some reasons that the method inheriting the configuration should have an own configuration of these properties

I would say we shouldn't join. We have never joined anything up to now. Usually what you define in your current config overrides what is defined in the next config. For example you can override a @Mapping by defining it again for the same target. The use case here is that they want to merge unmapped source properties, but it is also completely valid not do it and to have to explicitly not ignore the inherited unmapped source properties.

2. I am not sure if I understood the SelectionParameters correctly, right now I try to join the qualifiers, but maybe that is not the right way and they shouldn't be joined

What would the backwards compatibility be here. Up to now we only took over if there was not @BeanMapping on the inherited method. This means that things might work differently after this PR.

3. Is it fine to change the next of the BeanMappingOptions? Could it be that there will then be a totally different inheritance flow? Looked for me that the delegating options works in the way that next in this case is the @Mapper config, then the @MapperConfig and so on. Now I change next to the other @BeanMapping and maybe this will lead to a different @Mapper config? Could this be?!

Yes it can be completely different mapper config. Perhaps this is even wrong now, because if there is not @BeanMapping and we inherit it then it would use the @MapperConfig from where that @BeanMapping is defined.

[drahkrub]

Hi @chris922 @filiphr,

as I wrote in #2092 we have a fairly complex hibernate domain model containing multi-level inheritance, some beans implement several interfaces and there are quite a few @Transient getter methods around which I do not want to map with mapstruct.

On the other hand I don't want to miss changes in the domain model (I'm not the only one working on it), therefore I use a strict MapperConfig regarding source and target properties:

@MapperConfig(
        unmappedSourcePolicy = ReportingPolicy.ERROR,
        unmappedTargetPolicy = ReportingPolicy.ERROR
)
public interface CentralConfig {}

Because of this, I have to use @BeanMapping(ignoreUnmappedSourceProperties = {...}) a lot.

And because of the afore mentioned complexity of the domain model I have to ignore the same source properties (which are often just @Transient getters) over and over and over again (I just found #1152 which looks related).

Since my DTOs are structured similar to the domain model (in terms of inheritance) this could be avoided if @BeanMapping would be inherited with unmappedSourceProperties being merged.

In my opinion this would feel natural. ;-)

Or how about an additional parameter to tackle this use case (override, inheritanceOverride, inheritUnmappedSourceProperties or ...)?

[filiphr]

@drahkrub just out of curiosity are your @Transient getters annotated with some annotation. If they are you can do something like proposed in #1152. You can have a custom AccessorNamingStrategy which will override getMethodType and return MethodType.OTHER for your transient getters. That way you won't need to ignore the properties explicitly in the mappers.

[drahkrub]

@filiphr Good point. In my case the getters are indeed annotated (with @Transient), but in Hibernate one can choose between annotating the getters (properties) or the fields, see e.g. this epic discussion on SO. In the latter case one does not have to use the transient annotation on "utility getters". So in my case the workaround you've proposed would probably work, on the other hand it would be anything but flexible, because maybe it could sometimes be desirable to include transient getters into some mapping...

[drahkrub]

@filiphr First of all: Congratulations on the release of 1.4.0.Final! :-)

Any chance that #2092 / #2096 will be tackled in 1.4.x?

[filiphr]

We've had a discussion about this functionality with the team and we stand by my previous comment

I would say we shouldn't join. We have never joined anything up to now. Usually what you define in your current config overrides what is defined in the next config. For example you can override a @Mapping by defining it again for the same target. The use case here is that they want to merge unmapped source properties, but it is also completely valid not do it and to have to explicitly not ignore the inherited unmapped source properties.

The entire pattern in MapStruct is that if you override a value in an annotation like @BeanMapping then the new value takes precedence. However, a new annotation can be introduced e.g. @IgnoreSourceMapping that would be merged in the same way that @Mapping is done. We would appreciate someone doing a new PR for this, because the MapStruct team is focused on something else.

@chris922 thanks anyways for the work and sorry for closing it, but sometimes we have to take decisions like this 😢