Compiler Error with Qualified Mapping and Custom Converter - Duplicate Method Generation

[gigena-git]

Let say I create two classes Source:

public class Source {
    public byte fieldA;
    public byte fieldB;
    public byte fieldC;
    public byte fieldD;
}

and Target:

public class Target {
    public byte fieldE;
    public byte fieldF;
}

Then I create a new mapper for the two classes:

@Mapper(config = MapperTestConfig.class)
public interface SourceToTargetConverter extends Converter<Source, Target> {

  @Mapping(target = "fieldE", source = "source")
  @Mapping(
      target = "fieldF",
      source = "source",
      qualifiedByName = {"cAndD", "toF"})
  @Override
  Target convert(Source source);
}

And then I create mappers for each of the two operations:

@Mapper
public interface FieldsAPlusBToFieldEMapper extends Converter<Source, Byte> {

  @Override
  default Byte convert(Source source) {
      return (byte) (source.fieldA + source.fieldB);
  }
}

and:

@Named("cAndD")
@Mapper
public interface FieldsCPlusDToFieldFMapper extends Converter<Source, Byte> {

  @Named("toF")
  @Override
  default Byte convert(Source source) {
      return (byte) (source.fieldC + source.fieldD);
  }
}

Finally, I create the @MapperConfig:

@MapperConfig(componentModel = MappingConstants.ComponentModel.SPRING)
@SpringMapperConfig
public interface MapperTestConfig {}

Result: Two methods with the same are passed down to ConversionServiceAdapter, thus creating a compiler error:

@Component
public class ConversionServiceAdapter {
  private final ConversionService conversionService;

  public ConversionServiceAdapter(@Lazy final ConversionService conversionService) {
    this.conversionService = conversionService;
  }

  public Byte mapSourceToByte(final Source source) {
    return (Byte) conversionService.convert(source, TypeDescriptor.valueOf(Source.class), TypeDescriptor.valueOf(Byte.class));
  }

  public Byte mapSourceToByte(final Source source) {
    return (Byte) conversionService.convert(source, TypeDescriptor.valueOf(Source.class), TypeDescriptor.valueOf(Byte.class));
  }

  public Target mapSourceToTarget(final Source source) {
    return (Target) conversionService.convert(source, TypeDescriptor.valueOf(Source.class), TypeDescriptor.valueOf(Target.class));
  }
}

Stacktrace (redacted):

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.10.1:compile (default-compile) on project [name]: Compilation failure: Compilation failure:
[ERROR] [generated-annotation-path]/ConversionServiceAdapter.java:[30,15] method mapSourceToByte(Source) is already defined in class ConversionServiceAdapter
[ERROR] [source-file-path]/FieldsCPlusDToFieldFMapper.java:[9,8] No implementation was created for FieldsCPlusDToFieldFMapper due to having a problem in the erroneous element java.util.ArrayList. Hint: this often means that some other annotation processor was supposed to process the erroneous element. You can also enable MapStruct verbose mode by setting -Amapstruct.verbose=true as a compilation argument.
[ERROR] [source-file-path]/SourceToTargetConverter.java:[8,8] No implementation was created for SourceToTargetConverter due to having a problem in the erroneous element java.util.ArrayList. Hint: this often means that some other annotation processor was supposed to process the erroneous element. You can also enable MapStruct verbose mode by setting -Amapstruct.verbose=true as a compilation argument.
[ERROR] [source-file-path]/FieldsAPlusBToFieldEMapper.java:[7,8] No implementation was created for FieldsAPlusBToFieldEMapper due to having a problem in the erroneous element java.util.ArrayList. Hint: this often means that some other annotation processor was supposed to process the erroneous element. You can also enable MapStruct verbose mode by setting -Amapstruct.verbose=true as a compilation argument.

Expected:
The enqine should generate two methods with different signatures, each resolving the qualifications specified with (at)Named.

@Component
public class ConversionServiceAdapter {
  // fields and constructor. Might use more than one ConversionService to qualify different methods.
  public Byte mapSourceToByte(final Source source) {
    return (Byte) conversionService.convert(source, TypeDescriptor.valueOf(Source.class), TypeDescriptor.valueOf(Byte.class));
  }
 
  // Method resolved with (at)Named = "cAndD" and "toF".
  public Byte mapSourceToByteCandDtoF(final Source source) {
    // logic for qualified mapping, maybe using another ConversionService.
  }

  public Target mapSourceToTarget(final Source source) {
    return (Target) conversionService.convert(source, TypeDescriptor.valueOf(Source.class), TypeDescriptor.valueOf(Target.class));
  }
}

and then use them in the converter:

@Component
public class SourceToTargetConverterImpl implements SourceToTargetConverter {

    @Autowired
    private ConversionServiceAdapter conversionServiceAdapter;

    @Override
    public Target convert(Source source) {
        if ( source == null ) {
            return null;
        }

        Target target = new Target();

        target.fieldE = conversionServiceAdapter.mapSourceToByte( source );
        target.fieldF = conversionServiceAdapter.mapSourceToBytecAndDtoF( source );

        return target;
    }
}

[Chessray]

Things to note here:

• The ConversionService only allows for a single Converter per source and target type.
• We'd have to keep in mind the qualifiedBy attribute as well.
• I agree that generating method signatures should be relatively straightforward.
• Method bodies are a different beast. As you already noted yourself, we'd have to somehow cater for several ConversionService beans in the adapter bean.

To sum things up, this is definitely a non-trivial issue. I'll see if I can give it some thought over the next couple of days/weeks.

[gigena-git]

Hello @Chessray, thanks for your answer!

Yes, I overlooked the qualifiedBy attribute as I don't use it, but considering it makes sense.
If there's any additional information, use cases, insights, or help I can provide from my end, please let me know.

Thanks, Maximiliano.

[Chessray]

Solution idea:

• Include qualifying elements in generated method names for adapter class.
• Clone @Named or respective qualifier annotation from called methods onto generated methods so that MapStruct Processor can "see" them.
• Make a note of this in the core documentation on qualifiers as in this scenario the called Mapper must not be included in the uses attribute.
• Consider separate ConversionService bean for every qualified method? Or add a separate annotation that allows the developer to specify a non-default bean name on the called method?

@gigena-git It's definitely intriguing, and as you can see there are a few things to consider. I can't guarantee a quick solution. Happy for you to take a head start in this yourself if you wish. 🙂

[gigena-git]

I agree with the approach for method name generation. Even though I'm still getting acquainted with how the MapStruct processor works, I can visualize a way in which it could be implemented.
I didn't understand the third bullet point. Why the called Mapper should not be included in the uses attribute?
About the fourth point, I think that giving each method a different ConversionService, while easy to implement it is not efficient and might not be a trivial issue when dealing with code that declares multiple mappers. Let's say for instance that we declare the following methods across various Converters/Mappers.

@Named("toUppercase")
String convert(String);
////////////////////////////////
String convert(String);
////////////////////////////////
@QualifiedInt  // custom qualifier
int convert(int);
////////////////////////////////
@QualifiedByte  // custom qualifier
byte convert(byte);
////////////////////////////////
@QualifiedLong  // custom qualifier
long convert(long)
////////////////////////////////
@QualifiedBoolean // custom qualifier
boolean convert(boolean)
////////////////////////////////
@QualifiedCharToChar // custom qualifier
char convert(char)
////////////////////////////////
@QualifiedCharToString // custom qualifier
String convert(char)

If the processor gives every single qualified method it's own conversion service, the adapter would end up with 7 instances. However, with the given methods, only 2 are needed - as the only the String convert(String) methods are colliding.

The best approach - if possible - would be to determine the number of ConversionService instances. Something like this:

1. Start at the point where the processor has the list of all the converters - and it's annotations - that have been declared throughout the codebase.
2. Determine the minimum number of ConversionService instances that are needed. I don't have at the moment a particular algorithm for this, but I think it can be done in one loop through the converters.
3. Instantiate the computed number of conversion services, and keep them on an array.
4. Create a Map<String, ConversionService> object.
5. Assign the converters to each of the conversion services, making sure that no service is assigned two converters with the same source and target.
   a. Loop through all the converters. For each, extract source, target, @Named, and @Qualifier annotations.
   b. Generate a String from the annotation names. Concatenate and camelCase if multiple exist.
   c. Check if the map contains the annotations' string as key. If it does, get the conversion service from the map determine if it can convert from the converter's source to the converter's target.
   I. If it can't, add the converter to the conversion service and continue.
   II. If it can throw an exception, as adding the converter to the service would put two converters of the same source and target.
   d. Start with the zeroth conversion service of the array. If the conversion service cannot convert from the converter's source to the converter's target, add the converter. If it can move to the next conversion service and repeat this step until it can't.
   e. Add the conversion service to the map with the annotations' string as key and continue.
6. Return the Map - this is what will be injected into ConversionServiceAdapter.

The implementation could look something like this:

// Step 1 it could be a bean declared in the configuration class, or it could be a call from a Post Processor.
@Bean
Map<String, ConversionService> conversionServiceMapBean(List<Converter> converters) {
    // Step 2 get the minimum number of conversion services needed.
    // Step 3
    ConversionService[] csArr = new ConversionService[numberOfMinimumConversionServices];
    // Step 4
    Map<String, ConversionService> conversionServiceMap = new HashMap<>();
    // Step 5
    for(Converter c : converters) {
        // Step 5a
        Class<?> source = c.getSource();
        Class<?> target = c.getTarget();
        // Extract annotations
        Class<?> clazz = c.getClass();
        Annotation[] anns = clazz.getAnnotations();
        Method m = clazz.getMethod("convert", ActionItemDTO.class);
        Annotation[] mAnns = m.getAnnotations();
        // Generate String from annotations
        String key = "";
        for(Annotation ann : anns) {
            if(ann.annotationType().equals(Named.class){
              key += ann.annotationType().value();
            } if(ann.annotationType().equals(Qualifier.class)) {
              key += ann.getClass().getSimpleName(); 
            }
        }
        for(Annotation mAnn : mAnns) {
            if(mAnn.annotationType().equals(Named.class){
              key += mAnn.annotationType().value();
            } if(mAnn.annotationType().equals(Qualifier.class)) {
              key += mAnn.getClass().getSimpleName(); 
            }
        }
        key = (key != "") ? key : "primary";
        // Step 5b
        if(conversionServiceMap.containsKey(key)) {
            ConversionService conversionService = conversionServiceMap.get(key);
            // Step 5c
            if(!conversionService.canConvert(source, target) {
                // Step 5 c i
                conversionService.addConverter(c);
                continue;
            } else {
                // Step 5 c ii
                throw new Exception("Cannot add converter to conversion service. A converter with the same source and target already exists.");
            }
        }
        // Step 5d
        for(int i = 0; i < csArr.length; i++) {
            ConversionService conversionService = csArr[i];
            if(!conversionService.canConvert(c.getSource(), c.getTarget())) {
                conversionService.addConverter(c);
                // Step 5e
                conversionServiceMap.put(key, conversionService);
                break;
            }
        }
    }
    // Step 6
    return conversionServiceMap;
}

This is a brief description of what the Mapper builder should do.

1. Now the constructor will receive a Map<String, ConversionService> instead of a ConversionService.
2. For each Source/Target pair, the annotations that were extracted from the converter, will be passed to a variable that will be used to get the conversion service from the map. The default one will be assigned the "primary" key.

Now, inside ConversionServiceAdapter, the end result should look something like this:

@Component
public class ConversionServiceAdapter {
  private final Map<String, ConversionService> conversionServiceMap;

  // New Constructor
  public ConversionServiceAdapter(final @Lazy Map<String, ConversionService> conversionServiceMap) {
    this.conversionServiceMap = conversionServiceMap;
  }

  // Method with no annotations, assigned with @Primary on the initialization of the Map.
  public Byte mapSourceToByte(final Source source) {
    String key = "primary"; // This line should be generated from the processor.
    ConversionService conversionService = conversionServiceMap.get(key);
    return (Byte) conversionService.convert(source, TypeDescriptor.valueOf(Source.class), TypeDescriptor.valueOf(Byte.class));
  }
 
  // Method name resolved with @Named = "cAndD" and "toF".
  public Byte mapSourceToByteCandDtoF(final Source source) {
    String key = "cAndDtoF"; // This line should be generated from the processor.
    ConversionService conversionService = conversionServiceMap.get(key);
    return (Byte) conversionService.convert(source, TypeDescriptor.valueOf(Source.class), TypeDescriptor.valueOf(Byte.class));
  }

  public Target mapSourceToTarget(final Source source) {
    String key = "primary";
    ConversionService conversionService = conversionServiceMap.get(key);
    return (Target) conversionService.convert(source, TypeDescriptor.valueOf(Source.class), TypeDescriptor.valueOf(Target.class));
  }
}

I still have to look how would a mapper implementation call the methods with modified qualified names.

[Chessray]

I didn't understand the third bullet point. Why the called Mapper should not be included in the uses attribute?

The whole point of this module is the decoupling of Mappers. The idea is that we clone the annotations onto the generated methods so only the Adapter class shows up in the uses clause. The calling Mapper will then simply use the method in the Adapter class like it does normally. Were the called Mapper in the uses clause as well, we'd end up with ambiguity.

About the fourth point, I think that giving each method a different ConversionService, while easy to implement it is not efficient and might not be a trivial issue when dealing with code that declares multiple mappers.

I merely threw some quick ideas around. If there is a way to determine this for all cases automatically, then I'm certainly on board with that.

[Chessray]

After giving this some thought, it feels like we'd be going down a nearly bottomless rabbit hole without much gain. The idea as described so far would require several ConversionServices initialized by some additional configuration code. I definitely want to avoid generating that kind of thing. Spring provides several different service implementations, and users might want to add their own. This is different from the testing context where the default service implementation covers pretty much all scenarios.

So if we always leave the service initialization to the user, there seems to be little gain in pursuing this idea. What we could rather think about is suppressing the method generation for certain cases so at least the generated Adapter passes compilation. This seems like one of the situations where you want to just use MapStruct directly and not let the Mapper extend and/or implement the Converter interface.