Add ability to globally configure @Mapping for properties in reused super classes

[agudian]

With #72 being resolved now, we need some means to allow ignoring certain properties from being mapped / reported as unmapped on a global scope.

I could think of something along the following lines (with much room for improvement for the naming):

@GlobalMapping(
   sourceType=BaseEntity.class, 
    targetType=BaseDto.class, 
    mappings={ 
      @Mapping(target="uuid", ignore=true),
      @Mapping(source="id", target="technicalId") 
   }
)

That annotation (or a multiple of it) would be allowed at mapper level (e.g. in @Mapper) and in the @MapperConfig annotation.

sourceType and targetType would be optional, and the specified @Mapping configuration would be applied for each mapping method that has source / target types that fit (as in "assignable from").

[gunnarmorling]

add ability to ignore a property from being mapped at mapping-method level

For this there is #72 already.

[agudian]

Oh, right. 😀

[agudian]

As #72 is now resolved, I've updated this issue to something that builds upon it and fits my requirements.

@gunnarmorling, @sjaakd: what do you think?

[agudian]

Btw, Sjaak already mentioned somewhere the idea to specify a global "dictionary" for source-to-target property names, this proposal could be a solution for this as well.

[sjaakd]

Btw, Sjaak already mentioned somewhere the idea to specify a global "dictionary" for source-to-target property names, this proposal could be a solution for this as well.

We are set to go for this. We have a @MapperConfig now as place were we could do this.

However, I also think we need to be careful with automation. From my own experience, sometimes unpredictable behavior is occurring because Mapstruct is using BuiltIn or TypeConversions (or in 2 steps :) iso selecting an (for instance) hand-written method. Now we add another dimension. So, I think we should also look into #177 when adding more automation.

Next, I experienced last friday the hard-way (while integrating beta2), I did need to copy a lot of mappings due to reverse mapping. I'll open a new issue for that.

[gunnarmorling]

From my own experience, sometimes unpredictable behavior is occurring because Mapstruct is using BuiltIn or TypeConversions (or in 2 steps :) iso selecting an (for instance) hand-written method

@sjaakd Do you have an example for this? Predictablility is one of the core values in MapStruct, so if there are issues around this, we need to look into ways of improving this.

Built-in methods and type conversions should only be used if no other hand-written (or generated) method is found. Are there situations where this is not the case?

[sjaakd]

Are there situations where this is not the case?

No.. The mechanism is clear. And after all: you can always check the generated code (so there you have an implicit logging mechanism).

However, I try to build a library of reusable [GML](http://www.opengeospatial.org/standards/gml specific) hand-written mappers, typically related GIS applications. Were these fail, I switch to more domain specific HW mappers. Typically I have a list of used mappers constituting both in a mapper. If I - for instance - forget to include a domain specific mapper, I'm surprised in how MapStruct uses type conversions and built-in methods to select a GML mapper. Usually these are fixed quite fast. I know were to look. But I noticed that other users in my surrounding sometimes find it harder to fix it.

I'm not such a good documentation reader myself. You know, don't read the manual of a new device, just try :) . So one idea could be to include some logging on how MapStruct came to the selection of a mapping method or type conversion. This would than also include which mapping is applied (global / local / implicit / reverse), which selection rules are used: (direct, used, built-in, type-conversion) and which configuration option is used (e.g. CollectionMappingStrategy).

[gunnarmorling]

@agudian What are the use cases for this feature of global mappings? I feel it adds quite some complexity and makes mapper interfaces potentially much harder to grasp.

E.g. couldn't the ignored audit trail property just be solved by not having that attribute in the target objects?

[agudian]

E.g. couldn't the ignored audit trail property just be solved by not having that attribute in the target objects?

Why didn't I think of that? 😉

No, seriously: we have some properties in our base entity types that are handled and populated / updated only by entity listeners (audit stuff, when was something last updated by whom and so on). On the DTO side, we have a UUID in the base class that has no representation in the entities. We need that UUID in the DTO for objects created on the client side that don't yet have a representing entity instance, but for which we need some identity still: for change detection, for visualizing validation errors on the right rows in a table, etc.

So for me personally, I'm only interested in ignoring target properties for certain base types. This was the easiest way to implement it (easy in terms of coding - I spent the most time with thinking about the unit test 😉).

[sjaakd]

I have similar problems: also need to populate an id, which are not in my entities but are in my JAXB objects. I use an expression for that (UUID). So it would be nice if that is supported as well.

[sjaakd]

Hey guys.. I finally had some time to look at this in more detail and like to move forward with this one. Especially the Any as type for target and source makes it very usable.

@gunnarmorling : do you think this is ready to be merged?

[sjaakd]

@agudian What are the use cases for this feature of global mappings? I feel it adds quite some complexity and makes mapper interfaces potentially much harder to grasp.

@gunnarmorling : I've quite some use cases for this as well. For instance, I've got a lot of (JAXB) base classes that always need to be populated, with the same constants and expressions (e.g. for ID generation).

I spent last saturday getting Netbeans to work with giving in-IDE feedback, just as is possible with Eclipse and I like it a lot. This takes a way a lot of the pain I mentioned before. I still think its a good idea to add some logging in MapStruct, for instance triggered by the --debug option in Maven somehow.

[maksim-kashynski]

Voting for this feature. As of 1.0.0.Beta3 it is no longer possible to get update- methods generated from a @Mapping annotations on create-method. The related issue is #380
Without the way to customize inherit/reference mappings, it becomes extremely tedious to copy and paste them from method to method.

[gunnarmorling]

@agudian, I've been thinking a bit more about this. There is basically two things I'm concerned about with the proposed approach:

• The irregularity of global definitions: source and target types need to be specified within the annotations rather then method parameters / return types as with the existing means of mapping definitions; It also doesn't work for parameterized types (I don't think that's supported yet in general, but any solution we come up with should leave the door open)
• When looking at a mapping method definition, not all its property mappings are easily visible; You need to take global definitions possibly defined somewhere else into account

So what would you think about re-using @InheritConfiguration alternatively:

@Mapping(source = "id", target = "primaryKey")
BaseEntity dtoToEntity(BaseDto dto);

@InheritConfiguration(name="dtoToEntity")
CarEntity toCarEntity(CarDto carDto);

This would address the concerns above:

• "Global" (or basic) mappings would just be defined as any other mappings, it works for parameterized types.
• When looking at a specific method such as toCarEntity() there is a link to the method with the basic mappings, making things much easier to grasp

We'd also re-use an existing construct rather than defining a new one. Now some downsides:

• It doesn't allow for the definition of mappings where source or target is "any type". Personally I don't think it's a huge issue.
• It doesn't work if the target base type (e.g. BaseDto) is an abstract class as we cannot generate an implementation of the base mapping method then. That's a problem indeed. One solution I can think of is to define the base method on another mapper and have a flag in @Mapper which marks that mapper as not to be implemented, i.e. it's only supposed to serve as base mapping source. That'd require the inherit configuration mechanism to work across mappers. Maybe you have another, better idea for that?

All in all, that approach seems beneficial to me due to its regularity, explicitness and re-use of existing constructs. WDYT?

[gunnarmorling]

It doesn't allow for the definition of mappings where source or target is "any type"

I guess that can be done by using Object as source/target type.

Another advantage of the inheritance approach is that it avoids any mismatches between conflicting global mappings for the same types as a method can only its configuration from exactly one other method.

[gunnarmorling]

One solution I can think of is to define the base method on another mapper and have a flag in @Mapper...

Or, we take methods defined on the conflig class into account for this purpose. The nice thing about that is that it doesn't really change the scope of @InheritConfiguration (I find inheriting from methods from the config class less invasive than from other mappers) and the base mapping method is not exposed to clients (which is a good thing if the return type is abstract as its not useful anyways). And if you want to expose the base method, just define it in the mapper interface itself.

[sjaakd]

And thus, my favourites are:

Drums.. TADA.. You've got a strange form of favorites 😄..

Anyway. To list the remaining topics to be solved:

1. Place of the flag (I don't have a strong preference, so I guess you guys need to settle that one).
2. The annotations @InheritConfiguraiton, @InheritBaseConfiguration and I guess also the @InheritReverseConfiguration. Personally: I'm concerned on these ones. I've seen what happens in the automated case (cyclic relations, unpredictable behavior). I added checks to circumvent those. @gunnarmorling : could it be that a matter of taste, whether you add elements to one annotation or you have multiple of them? @agudian : what's your take on this?
3. .. any more?

[agudian]

Place of the flag (I don't have a strong preference, so I guess you guys need to settle that one).

Ok then, I've placed my bet. Let the BDOL decide... 😉

On 2. I'm kind of out - I also have a feeling that re-using InheritConfiguraiton and InheritReverseConfiguration could cause some trouble/inconsistencies and that we might better add some new and specific annotation there. But that's really just a feeling based on no hands-on experience with that stuff. You two are much deeper into that, so you decide, I'll implement 😀

1. would be to reconsider the namings, but we need to pin down 1 and 2 first, I'd say.

Something else missing?

[gunnarmorling]

But I agree with Sjaak, that until now we mirrored all the options of @Mapper in @MapperConfig, so I'd also find it logical to do the same thing here.

Yes, ok, that make sense. Have an option on @Mapper and @MapperConfig, and giving the former precedence.

[gunnarmorling]

In a new @MappingPrototype annotation on the dummy mapping method declaration

Where would one use that? As I see it, all the method definitions on the @MapperConfig class are "mapping prototypes", so there is no need to mark them again as such via the annotation. Or where did you mean to use it?

There are two cases:

• You want to be the "mapping prototype" method be usable itself -> declare it on an actual mapper interface
• You don't want to be it usable (e.g. because it declares an abstract return type) -> declare it on a @MapperConfig-annotated interface

[gunnarmorling]

Regaring having a specific @InheritBaseConfiguration, I do not yet understand in which way it would differ from @InheritConfiguration. In both cases it's an advice to apply the configuration from one other method to the annotated method. I don't think it makes a difference whether the parameter/return types are exactly the same or super types. They must be the same or super-types, but in both cases the configuration can be inherited. Thus I don't think a new annotation is needed for that.

[gunnarmorling]

could it be that a matter of taste, whether you add elements to one annotation or you have multiple of them?

Generally I prefer to re-use (and possibly enhance/generify) existing annotations and constructs rather than adding new ones. But if a construct is too generic, it often gets too verbose to use it. Thus I think it's fine to have the explicit @InheritReverseConfiguration instead of @InheritConfiguration(reverse=true).

But for the case of inheriting base configurations re-using @InheritConfiguration seems good to me as it can be use as is, there is no need to parameterize it.

[sjaakd]

But for the case of inheriting base configurations re-using @InheritConfiguration seems good to me as it can be use as is, there is no need to parameterize it.

Ok. Lets go ahead and try. I do think we need to think a bit more on ruling. Examples:

I want to avoid relations as A -> B -> A . So 'A' inherits from 'B' inherits from 'A' again. I avoided this by simply not allowing A to inherit its configuration from B if B itself inherits also from something. I think we need to fine-tune that.

1. InheritConfiguration is not allowed on the @MapperConfig interface (prototype methods)
2. InheritConfiguraiton is allowed from A to be, as long as B InheritConfiguration from a method in the @MapperConfig annotated class.
3. But then, how do we resolve conflicts in the mapper class with methods in the class with the one marked with @MapperConfig? We need to indicate somehow a class as well in the @MapperConfig#name ..

If we can work the above out, then we're there.

Generally I prefer to re-use (and possibly enhance/generify) existing annotations and constructs rather than adding new ones. But if a construct is too generic, it often gets too verbose to use it. Thus I think it's fine to have the explicit @InheritReverseConfiguration instead of @InheritConfiguration(reverse=true).

That's good. I think we can close that discussion (in my mind this was still an open end somewhere 😄 ) ).

[gunnarmorling]

I want to avoid relations as A -> B -> A . So 'A' inherits from 'B' inherits from 'A' again. I avoided this by simply not allowing A to inherit its configuration from B if B itself inherits also from something. I think we need to fine-tune that.

+1 This situation must be detected and prohibited. It's easy when @InheritConfiguration is given explicitly, but when mapping methods for super-types may be inherited automatically, it may become more tricky.

Also some ordering must be applied (in case there is a mapping method for super-types and another one for super-super types).

InheritConfiguration is not allowed on the @MapperConfig interface (prototype methods)

Why would that be needed? Couldn't one base configuration method inherit the config from another one?

But then, how do we resolve conflicts in the mapper class with methods in the class with the one marked with @MapperConfig?

Could you give an example for the situation you have in mind?

[gunnarmorling]

Regarding the referencing of methods in the mapper config via @InheritConfiguration, we may add an optional member Class<?> declaring class for that purpose. By default, the named method would be searched on the "local" class.

[sjaakd]

Regarding the referencing of methods in the mapper config via @InheritConfiguration, we may add an optional member Class<?> declaring class for that purpose. By default, the named method would be searched on the "local" class.

Ah.. I think you already know what situation i have in mind 😄 . But this is why I had a special annotation @InheritBaseConfiguration in mind (this is only searched in the @MapperConfig class) whereas @InheritConfiguration is only searched in the mapper class itself.

[sjaakd]

By default, the named method would be searched on the "local" class.

You mean prefer the "local" class over the @MapperConfig class? There could be conflict in the @MapperConfig class as well. If there's no matching class in the "local" class, it still could be resolved with @InheritConfiguration#name only.

we may add an optional member Class<?> declaring class for that purpose.

Isn't that overkill? There are only 2 options, right "local" or @MapperConfig? I think we are making this over complicated.

[sjaakd]

we may add an optional member Class<?> declaring class for that purpose.

I've been thinking a bit more on this. Perhaps we should use an enum iso the class. Or do you want to open up the possibility as well to refer to entirely different mappers?

So:

@Target( ElementType.METHOD )
@Retention( RetentionPolicy.SOURCE )
public @interface InheritConfiguration {

    public enum Scope { LOCAL, MAPPER_CONFIG, BOTH }

    String name() default "";

    boolean inverse() default false;

    Scope scope() default Scope.BOTH;
}

Note:
*. You can't combine inheritance from prototype (mapper config) methods and local methods on one method

*. WHEN_ALL_TYPES_ARE_APPLICABLE / WHEN_EXPLICITLY_INHERITED . Perhaps obvious, but defining an @InheritConfiguration overrules 'WHEN_ALL_TYPES_ARE_APPLICABLE`

*. @agudian: Have you thought about update / create methods (in the @MapperConfig), or do we both match up with one signature? I think we should.

[gunnarmorling]

Or do you want to open up the possibility as well to refer to entirely different mappers?

Yes, I think some flexibility may be beneficial here. Not necessarily for mapper classes (at least for now), but I could imagine cascades of global config classes (i.e. one global config class extending another one).

[gunnarmorling]

boolean inverse() default false;

Ah, do you prefer that flag now? Didn't we agree on the separate annotation being nicer to read?

[sjaakd]

Ah, do you prefer that flag now? Didn't we agree on the separate annotation being nicer to read?

I was exploring the possibility 😄 .. Actually, the same arguments as you apply to having separate annotations for InheritBaseConfiguration and InheritConfiguration. So perhaps we should.. That opens up the possibility to inherit reverse configurations as well from base. I'm still worried that we open up a can of worms.

Anyway.. Perhaps we should leave it up to @agudian at this point. We have discussed it sufficiently (at least I think), and perhaps we should see what implementation yields. WDYT?

[gunnarmorling]

Yes, I think the general direction is clear, so I think we can can continue (or just finish ;) ) the discussion once Andreas has updated the PR based on our latest line of thought. Being able to specify base mapping methods in config classes is the key piece which was missing before.

[gunnarmorling]

@agudian The PR is merged. I'll leave the issue open until the doc update is there. Thx!

[agudian]

Doc update pushed.