#3303 Mapping from Map to Map and specify source and target keys of type String #3367

thunderhook · 2023-08-28T14:11:29Z

Tackling #3303 map mapping with source and target keys.

Things to note:

The variable containing key mappings is not built safe like in org.mapstruct.ap.internal.model.SupportingField#getSafeField. This would need a bigger change in the MapperCreationProcessor. However, a collision with a field named like method.getName() + "KeyMappings" is very unlikely.
Only @Mapping annotations with set source and target are considered, the rest is ignored, like expression or ignore (I felt that it is not worth to throw an error in that case)
This PR tackles only mapping of maps with keys of type string (Map<String, ?>). Any other type does not feel like a valid use-case (well, CharSequence eventually?)
- Due to this restriction it was easier to implement. Because the type of the keyMappings field are now hard-coded as Map<String, String> in the FreeMarker template.
- I found no easy solution to get a type of Map<String, String> with the TypeFactory. If you have a solution for this, please let me know. I could then remove the MapKeyMappingField.ftl and use a typed Field.ftl

…keys (for Map<String, String>)

filiphr

Really nice PR @thunderhook. Thanks for your work on this.

I've left some comments in the PR review, still need to play with it to see the actual generated code.

Here are some comments for your notes:

The variable containing key mappings is not built safe like in ...

I've left a review comment for this. If we do it like I suggest there, I think that we can use the SupportingField#getSafeField

Only @Mapping annotations with set source and target are considered, the rest is ignored, like expression or ignore (I felt that it is not worth to throw an error in that case)

If we are going to start something that we didn't support before then in my opinion we should be as strict as possible. This means that it is fine to only support source and target. However, if someone sets something else then we should have a compile error.

Alternatively we can also go with a new annotation for this since for this source is mandatory, but not target. I do see people asking for things like we have in BeanMapping#ignoreByDefault and maybe expanding more on how value for a particular key should be mapped using the qualifedByName and stuff like this. Having said all of this I think it would be better for us if we go with a new annotation e.g. @MapKeyMapping (open to other name suggestions as well)

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. We do have #3293 where something else around map mapping is being requested.

This PR tackles only mapping of maps with keys of type string (Map<String, ?>). Any other type does not feel like a valid use-case (well, CharSequence eventually?)

That is completely OK. However, see my comment for the validations. We also need to add tests for mappings from Map<X, Y> to Map<String, ?>. We do support converting X to String through some kind of a mapping or if it is some of the out of the box mappings from X to String. Check org.mapstruct.ap.test.collection.map.SourceTargetMapper as an example.

Map<String, Long> map(Map<Long, Long> source);

I found no easy solution to get a type of Map<String, String> with the TypeFactory. If you have a solution for this, please let me know. I could then remove the MapKeyMappingField.ftl and use a typed Field.ftl

You can get the right map by doing something like:

In the TypeFactory add:

    public Type getMapType(String keyCanonicalName, String valueCanonicalName) {
        TypeElement mapTypeElement = getTypeElement( Map.class.getCanonicalName() );
        TypeElement keyTypeElement = getTypeElement( keyCanonicalName );
        TypeElement valueTypeElement = getTypeElement( valueCanonicalName );
        DeclaredType mapType = typeUtils.getDeclaredType(
            mapTypeElement,
            keyTypeElement.asType(),
            valueTypeElement.asType()
        );
        return getType( mapType );
    }

The getTypeElement looks like:

    private TypeElement getTypeElement(String canonicalName) {
        TypeElement typeElement = elementUtils.getTypeElement( canonicalName );

        if ( typeElement == null ) {
            throw new AnnotationProcessingException(
                "Couldn't find type " + canonicalName + ". Are you missing a dependency on your classpath?"
            );
        }

        return typeElement;
    }

This is extracted from private Type getType(String canonicalName, boolean isLiteral)

filiphr · 2023-09-10T20:06:16Z

processor/src/main/java/org/mapstruct/ap/internal/model/MapMappingMethod.java

@@ -39,6 +41,8 @@ public class MapMappingMethod extends NormalTypeMappingMethod {
    private final Assignment valueAssignment;
    private final Parameter sourceParameter;
    private final PresenceCheck sourceParameterPresenceCheck;
+    private final MapKeyMappingConstructorFragment mapKeyMappingConstructorFragment;


We only need this here because it is used in the MapperCreationProcessor. However, if you look at what we do there we do

Set<Field> supportingFieldSet = new LinkedHashSet<>(mappingContext.getUsedSupportedFields());

which means that we can easily add the supported fields in the mapping context in the builder for the MapMappingMethod. doing it there would also making it possible to create the fields using SupportingField#getSafeField

For the constructor fragments. We can add a Set<ConstructorFragment> and do it in a similar way as we are doing for the supported fields.

filiphr · 2023-09-10T20:58:58Z

processor/src/main/java/org/mapstruct/ap/internal/model/MapKeyMappingConstructor.java

+ *
+ * @author Oliver Erhart
+ */
+public class MapKeyMappingConstructor extends ModelElement implements Constructor {


Where is this used?

filiphr · 2023-09-10T21:05:37Z

...ssor/src/main/resources/org/mapstruct/ap/internal/model/MapKeyMappingConstructorFragment.ftl

+
+-->
+<#-- @ftlvariable name="" type="org.mapstruct.ap.internal.model.MapKeyMappingConstructorFragment" -->
+this.${field.variableName} = new java.util.HashMap<>();


For this you can then use an IterableCreation in the MapKeyMappingConstructorFragment. Most likely we'll need a new method in IterableCreation

e.g.

public static IterableCreation create(Type resultType) { return new IterableCreation( resultType, null, null ); }

I think that the new line will then be

this.${field.variableName} = <@includeModel object=iterableCreation useSizeIfPossible=false/>;

filiphr · 2023-09-10T21:10:08Z

processor/src/test/java/org/mapstruct/ap/test/collection/map/AnnotatedMapMappingTest.java

+        assertThat( target ).isNotNull()
+            .hasSize( 2 )


isNotNull and hasSize are obsolete since we are doing isEqualTo already

filiphr · 2023-09-10T21:11:14Z

processor/src/test/java/org/mapstruct/ap/test/collection/map/AnnotatedMapMappingTest.java

+        assertThat( target ).isNotNull()
+            .hasSize( 2 )
+            .contains(
+                entry( "targetKey", "value" ),
+                entry( "unmappedKey", "otherValue" )
+            );


we can remove isNotNull and hasSize if we use containsOnly instead of contains

filiphr · 2023-09-10T21:11:49Z

processor/src/test/java/org/mapstruct/ap/test/collection/map/AnnotatedMapMappingTest.java

+    final GeneratedSource generatedSource = new GeneratedSource();
+
+    @ProcessorTest
+    public void shouldContainTwoKeyMappingMapsInConstructor() {


I would prefer if we have a fixture for this and we test the contents. Makes it a bit easier to see what gets generated as well

filiphr · 2023-09-10T21:13:10Z

processor/src/test/java/org/mapstruct/ap/test/collection/map/AnnotatedMapMappingTest.java

+    CustomNumberMapper.class,
+    Source.class,
+    Target.class,
+    ImportedType.class


Why are these needed? I guess it is a copy paste thing. Let's remove it if that is the case

thunderhook · 2023-09-12T20:47:41Z

Thank you for your detailed and thorough review.

Alternatively we can also go with a new annotation for this since for this source is mandatory, but not target.

Agreed with a new annotation (didn't think of the target 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 via source and target. 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.

filiphr · 2023-09-16T09:17:27Z

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.

thunderhook · 2023-10-04T21:03:43Z

@filiphr
Sorry for the wall of text 🙈

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

Company ID	Employees
1	100
2	200
3	300

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 CODE
    private final Map<String, String> mapLongKeyMapToStringKeyMapKeyMappings;

    public MapKeyMappingCommonConversionMapperImpl() {
        this.mapLongKeyMapToStringKeyMapKeyMappings = new LinkedHashMap<String, String>();
        this.mapLongKeyMapToStringKeyMapKeyMappings.put( "1L", "2L" );
        this.mapLongKeyMapToStringKeyMapKeyMappings.put( "2L", "1L" );
    }

    public Map<String, Long> mapLongKeyMapToStringKeyMap(Map<Long, Long> source) {

        Map<String, Long> map = new LinkedHashMap<String, Long>();

        for ( java.util.Map.Entry<Long, Long> entry : source.entrySet() ) {
            String key = String.valueOf( entry.getKey() ); // generates "1" instead of "1L" 
            Long value = entry.getValue();
            map.put( this.mapLongKeyMapToStringKeyMapKeyMappings.getOrDefault( key, key ), value ); // "1" != "1L"
        }

        return map;
    }

If there 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 in Mapping#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, since qualifiedByName is not implemented for this yet...) from Long to String, which maps like:

String map(Long value) {
    return "Company" + value;
}

How should map key mappings be defined?

define the source typed as the source key type and define the target typed as the target key type
define both typed as the the target key type
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 CODE
    private final Map<Long, String> option1KeyMappings;
    private final Map<String, String> option2KeyMappings;
    private final Map<Long, Long> option3KeyMappings;

    public MapKeyMappingCommonConversionMapperImpl() {

        this.option1KeyMappings = new LinkedHashMap<Long, String>();
        this.option1KeyMappings.put( (long) 1L, "Company2" );
        this.option1KeyMappings.put( (long) 2L, "Company1" );

        this.option2KeyMappings = new LinkedHashMap<Long, String>();
        this.option2KeyMappings.put( "Company1", "Company2" );
        this.option2KeyMappings.put( "Company2", "Company1" );

        this.option3KeyMappings = new LinkedHashMap<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
    @Override
    public Map<String, Long> option1(Map<Long, Long> source) {
        if ( source == null ) {
            return null;
        }

        Map<String, Long> map = new LinkedHashMap<String, Long>( Math.max( (int) ( source.size() / .75f ) + 1, 16 ) );

        for ( java.util.Map.Entry<Long, Long> entry : source.entrySet() ) {
            String key = String.valueOf( entry.getKey() );
            Long value = entry.getValue();
            Long sourceMappingKey = entry.getKey();
            map.put( this.option1KeyMappings.getOrDefault( sourceMappingKey, key ), value );
        }

        return map;
    }

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 mapper
    Map<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 the constant field. So besides primitive types, also enums are allowed.

Let's say we have a map with month and employees count, and all employees from FEBRUARY must be switched with MARCH (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:

String mapMonth(Month value) {
    return value.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.

thunderhook added 3 commits August 28, 2023 15:40

mapstruct#3303 Mapping from Map to Map and specify source and target …

23f464e

…keys (for Map<String, String>)

mapstruct#3303 support any map with keys of type String (Map<String, ?>)

ae32b47

mapstruct#3303 documentation of reassigning keys

332e214

filiphr requested changes Sep 10, 2023

View reviewed changes

thunderhook marked this pull request as draft September 12, 2023 20:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#3303 Mapping from Map to Map and specify source and target keys of type String #3367

#3303 Mapping from Map to Map and specify source and target keys of type String #3367

thunderhook commented Aug 28, 2023 •

edited

filiphr left a comment

filiphr Sep 10, 2023

filiphr Sep 10, 2023

filiphr Sep 10, 2023

filiphr Sep 10, 2023

filiphr Sep 10, 2023

filiphr Sep 10, 2023

filiphr Sep 10, 2023

thunderhook commented Sep 12, 2023

filiphr commented Sep 16, 2023

thunderhook commented Oct 4, 2023

#3303 Mapping from Map to Map and specify source and target keys of type String #3367

Are you sure you want to change the base?

#3303 Mapping from Map to Map and specify source and target keys of type String #3367

Conversation

thunderhook commented Aug 28, 2023 • edited

filiphr left a comment

Choose a reason for hiding this comment

filiphr Sep 10, 2023

Choose a reason for hiding this comment

filiphr Sep 10, 2023

Choose a reason for hiding this comment

filiphr Sep 10, 2023

Choose a reason for hiding this comment

filiphr Sep 10, 2023

Choose a reason for hiding this comment

filiphr Sep 10, 2023

Choose a reason for hiding this comment

filiphr Sep 10, 2023

Choose a reason for hiding this comment

filiphr Sep 10, 2023

Choose a reason for hiding this comment

thunderhook commented Sep 12, 2023

filiphr commented Sep 16, 2023

thunderhook commented Oct 4, 2023

Scenario 1 Mapping Long to String without custom mapping method

Scenario 2: Mapping Long to String with custom mapping

Option 1

Option 2

Option 3

Scenario 3 Map to (or even from) an enum

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

Conclusion

thunderhook commented Aug 28, 2023 •

edited