Thank you for your detailed and thorough review.

Alternatively we can also go with a new annotation for this since for thissource is mandatory, but nottarget.

Agreed with a new annotation (didn't think of thetarget not being mandatory btw).

I guess this should also be pre JDK-8 compatible? So there will be@MapKeyMappings and@MapKeyMapping. I personally like the name.
A new annotation might also lead to a better separation of code.

I am not saying that we should add all this customizations in this PR, but I would like to anticipate things and make sure that we are implementing this right.

Yeah. I will start with the "basic" key mapping feature viasource andtarget. We can then see how far I got, and then you can decide to give it another round or release it with the simple mapping.
You (or the team) can then prioritize the next features, maybe#3293 because it was a feature request.

Thank you for the detailled comments (especially about the type(s)), I will incorporate them as I refactor the code.

I guess this should also be pre JDK-8 compatible? So there will be@MapKeyMappings and@MapKeyMapping. I personally like the name.

The need of@MapKeyMappings is not for pre JDK-8 compatibility. It is needed for all JDKs, due to the need of@Repeatable.

Side note, can you please reach out to me on my email? You can find it in any of my commits.

@filiphr
Sorry for the wall of text 🙈

I made good progress but then I confused myself with the conversion ofMap<X, Y> toMap<String, ?>
Let's say, we have a map of Company-IDs (Long) with their amounf of employees (Long):

Scenario 1 Mapping Long to String without custom mapping method

Let's say that company 1 and 2 need to be switched. With the original implementation that would've looked like...

@MapKeyMapping(target ="1L",source ="2L" )@MapKeyMapping(target ="2L",source ="1L" )Map<String,Long>mapLongKeyMapToStringKeyMap(Map<Long,Long>source);// GENERATED CODEprivatefinalMap<String,String>mapLongKeyMapToStringKeyMapKeyMappings;publicMapKeyMappingCommonConversionMapperImpl() {this.mapLongKeyMapToStringKeyMapKeyMappings =newLinkedHashMap<String,String>();this.mapLongKeyMapToStringKeyMapKeyMappings.put("1L","2L" );this.mapLongKeyMapToStringKeyMapKeyMappings.put("2L","1L" );    }publicMap<String,Long>mapLongKeyMapToStringKeyMap(Map<Long,Long>source) {Map<String,Long>map =newLinkedHashMap<String,Long>();for (java.util.Map.Entry<Long,Long>entry :source.entrySet() ) {Stringkey =String.valueOf(entry.getKey() );// generates "1" instead of "1L"Longvalue =entry.getValue();map.put(this.mapLongKeyMapToStringKeyMapKeyMappings.getOrDefault(key,key ),value );// "1" != "1L"        }returnmap;    }

Ifthere is no custom mapping method from Long to String,String.valueOf() is being used to convert the key (see generated code).
When you define"1L" like inMapping#constant this will not hit the Key"1" in the mapping key map.
Workaround for this: just use "1" instead. This is bad because the constant expression then has a different behaviour in@Mapping and@MapKeyMapping.

Scenario 2: Mapping Long to String with custom mapping

Let's say we have a custom mapping method (which is not qualified, sincequalifiedByName is not implemented for this yet...) from Long to String, which maps like:

Stringmap(Longvalue) {return"Company" +value;}

How should map key mappings be defined?

1. define the source typed as the source key type and define the target typed as the target key type
2. define both typed as the the target key type
3. define both typed as the the source key type

// Option 1@MapKeyMapping(target ="Company1",source ="2L" )@MapKeyMapping(target ="Company2",source ="1L" )Map<String,Long>option1(Map<Long,Long>source);// Option 2@MapKeyMapping(target ="Company1",source ="Company2" )@MapKeyMapping(target ="Company2",source ="Company1" )Map<String,Long>option2(Map<Long,Long>source);// Option 3@MapKeyMapping(target ="1L",source ="2L" )@MapKeyMapping(target ="2L",source ="1L" )Map<String,Long>option3(Map<Long,Long>source);// GENERATED CODEprivatefinalMap<Long,String>option1KeyMappings;privatefinalMap<String,String>option2KeyMappings;privatefinalMap<Long,Long>option3KeyMappings;publicMapKeyMappingCommonConversionMapperImpl() {this.option1KeyMappings =newLinkedHashMap<Long,String>();this.option1KeyMappings.put( (long)1L,"Company2" );this.option1KeyMappings.put( (long)2L,"Company1" );this.option2KeyMappings =newLinkedHashMap<Long,String>();this.option2KeyMappings.put("Company1","Company2" );this.option2KeyMappings.put("Company2","Company1" );this.option3KeyMappings =newLinkedHashMap<Long,Long>();this.option3KeyMappings.put( (long)1L, (long)2L );this.option3KeyMappings.put( (long)2L, (long)1L );    }

Option 1

Using a diffrent type between source and target needs a different access inside the mapping method:

// GENERATED CODE@OverridepublicMap<String,Long>option1(Map<Long,Long>source) {if (source ==null ) {returnnull;        }Map<String,Long>map =newLinkedHashMap<String,Long>(Math.max( (int) (source.size() /.75f ) +1,16 ) );for (java.util.Map.Entry<Long,Long>entry :source.entrySet() ) {Stringkey =String.valueOf(entry.getKey() );Longvalue =entry.getValue();LongsourceMappingKey =entry.getKey();map.put(this.option1KeyMappings.getOrDefault(sourceMappingKey,key ),value );        }returnmap;    }

Implementation-wise I am certain that we should go with option 1.
It supports a good type-safety during compile time. When an invalid mapping is defined, the mapper cannot be generated, e.g.:

@MapKeyMapping(target ="Company1",source ="NOT_A_LONG")// cannot generate mapperMap<String,Long>mapLongKeyMapToStringKeyMap(Map<Long,Long>source);

Cons: seems unintuitive to define different types

Remember the problem from Scenario 1 with"1" != "1L"? This would be solved with option 1.

Option 2

I think the second option is most understandable from a users perspective. Or is it? I am not sure. However this can lead to wrong (and missed) mappings.
What if I define a mappig that will never work, because it will never be mapped like that, e.g.:

@MapKeyMapping(target ="Company1",source ="CompanyThatNeverHits")Map<String,Long>mapLongKeyMapToStringKeyMap(Map<Long,Long>source);

This is just not mapped and the user doesn't get informed.

Option 3

I don't think that option 3 is really an option. Just using the source key type is hard to understand, right?

Scenario 3 Map to (or even from) an enum

I reuse the code to get the expression of source/target like it is handled with theconstant field. So besides primitive types, also enums are allowed.

Let's say we have a map with month and employees count, and all employees fromFEBRUARY must be switched withMARCH (only reasonable example I can find atm)

@MapKeyMapping(target ="FEBRUARY",source ="MARCH" )@MapKeyMapping(target ="MARCH",source ="FEBRUARY" )Map<String,Long>map(Map<java.util.Month,Long>source);

This works. It does not matter if the key mapping map is backed with a String or Month map.

Let's add a custom enum conversion method:

StringmapMonth(Monthvalue) {returnvalue.name().substring(0,3);}

How about the mapping? Must it be changed to the following? This example should help to chose for an option from above

// option 1@MapKeyMapping(target ="FEB",source ="MARCH" )@MapKeyMapping(target ="MAR",source ="FEBRUARY" )// option 2@MapKeyMapping(target ="FEB",source ="MAR" )@MapKeyMapping(target ="MAR",source ="FEB" )// option 3@MapKeyMapping(target ="FEBRUARY",source ="MARCH" )@MapKeyMapping(target ="MARCH",source ="FEBRUARY" )Map<String,Long>map(Map<java.util.Month,Long>source);

Scenario 4 (bonus) Mapping from Long to Long (or: same source key type as target key type)

I can see no reason why this should not work. And this will work when the key mapping map supports different types than String. This is currently implemented and working as expected.

@MapKeyMapping(taraget ="1L",source ="2L")@MapKeyMapping(taraget ="2L",source ="1L")Map<Long,Long>mapLongsToLongs(Map<Long,Long>source);

Conclusion

During writing this wall of text (took me like an hour...) I could think about it more and analyze it. I want to know your opinion about the possible options, and what would you choose?
Is there another option, like disabling the support of one of these things (like really just support@MapKeyMapping when source and target have the same type)?

Thanks in advance. Take your time to answer it. I would love to push my current code, but it is really mixed up with those three options.