Mapping collection with specific implementation

[djarnis73]

Hi

I had the need for a set with order retained, so I created beans that used the specific LinkedHashSet implementation instead of the Set interface.

This resulted in a runtime mapping error, since remap internally creates a HashSet and calls the setter, this leads to an IllegalArgumentException: argument type mismatch (it took me a while in the debugger to actually figure it out).

Example that fails (I use lombok for brevity):

public class RemapCollectionTest {
    @Data
    @NoArgsConstructor
    @AllArgsConstructor
    public static class X {
        LinkedHashSet<String> x = new LinkedHashSet<>();
    }

    @Test
    public void testRemapperWithSpecificCollectionImpl() {
        Mapper<X, X> mapper = Mapping
                .from(X.class)
                .to(X.class)
                .mapper();
        mapper.map(new X());
    }
}

with

com.remondis.remap.MappingException: Invoking access method for property java.beans.PropertyDescriptor[name=x; propertyType=class java.util.LinkedHashSet; readMethod=public java.util.LinkedHashSet dk.danskespil.safe.aws.sqs.RemapCollectionTest$X.getX(); writeMethod=public void dk.danskespil.safe.aws.sqs.RemapCollectionTest$X.setX(java.util.LinkedHashSet)] failed.
	at com.remondis.remap.MappingException.invocationFailed(MappingException.java:104)
	at com.remondis.remap.Transformation.writeOrFail(Transformation.java:58)
	at com.remondis.remap.ReassignTransformation.performTransformation(ReassignTransformation.java:52)
	at com.remondis.remap.Transformation.performTransformation(Transformation.java:70)
	at com.remondis.remap.ReassignTransformation.performTransformation(ReassignTransformation.java:23)
	at com.remondis.remap.MappingConfiguration.map(MappingConfiguration.java:711)
	at com.remondis.remap.MappingConfiguration.map(MappingConfiguration.java:691)
	at com.remondis.remap.Mapper.map(Mapper.java:40)
	at RemapCollectionTest.testRemapperWithSpecificCollectionImpl(RemapCollectionTest.java:28)
	(junit part of stack trace snipped)
Caused by: java.lang.IllegalArgumentException: argument type mismatch
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at com.remondis.remap.Transformation.writeOrFail(Transformation.java:54)

I worked around it by adding the following to the mapper:
.useMapper(TypeMapping.from(LinkedHashSet.class).to(LinkedHashSet.class).applying(x -> x))
But this means that I don't get a copy of the collection, which I can live with in my use case.

But, question is if there should be some extra checks to ensure that the used collection types are assignable or perhaps smart code that will use the destination type and instantiate the "correct" type instead of a HashSet.

Best regards Jens

[schuettec]

Hm, I will have a look. Are you sure about the type ReMap tries to set using the set-method mentioned in the stacktrace?

You said

[...] remap internally creates a LinkedHashSet and calls the setter

Could it be that ReMap creates something else than a LinkedHashSet? Otherwise the error makes no sense... confused :D

[djarnis73]

Yeah, I'm sorry, it creates a HashSet, will update the orignal description. This issue is that it creates a HashSet and calls a method that requires a LinkedHashSet as argument.

It's in ReassignTransformation.convertCollection(...) where Collectors.toSet is used with generates a HashSet.

[djarnis73]

I think perhaps a solution could be to do something in the line of (little cleanup needed, perhaps create a getCollectionSupplier() method) in ReflectionUtil:

  static Collector getCollector(Class<? extends Collection> collectionType) {
    final Supplier<Collection> collectionSupplier = () -> {
      try {
        return collectionType.newInstance();
      } catch (Exception e) {
        throw new RuntimeException("No default constructor for " + collectionType, e);
      }
    };

    if (Set.class.isAssignableFrom(collectionType)) {
      return Collectors.toCollection(collectionSupplier);
    } else if (List.class.isAssignableFrom(collectionType)) {
      return Collectors.toCollection(collectionSupplier);
    } else {
      throw MappingException.unsupportedCollection(collectionType);
    }
  }

[schuettec]

Yeah I thought about something like this, too. Since the JavaDoc for Collection class says something like "every collection should provide a default constructor" we're not that hacky as I thought initially :D And you're handling the error case adequately. Would you like to do a PR? 🤓 Jens Teglhus Møller <notifications@github.com> schrieb am Mi., 27. Jan. 2021, 16:59:

…

I think perhaps a solution could be to do something in the line of (little cleanup needed, perhaps create a getCollectionSupplier() medthod) in ReflectionUtil: static Collector getCollector(Class<? extends Collection> collectionType) { final Supplier<Collection> collectionSupplier = () -> { try { return collectionType.newInstance(); } catch (Exception e) { throw new RuntimeException("No default constructor for " + collectionType, e); } }; if (Set.class.isAssignableFrom(collectionType)) { return Collectors.toCollection(collectionSupplier); } else if (List.class.isAssignableFrom(collectionType)) { return Collectors.toCollection(collectionSupplier); } else { throw MappingException.unsupportedCollection(collectionType); } } — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#140 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADNFVQQLSPKFIZ37MVYDY43S4AZ6NANCNFSM4WVEXZOQ> .

[djarnis73]

I can have a go at it. I'm a little unsure about error handling (just rethrowing instantiation exceptions as a RuntimeException) and if you are ok with using newInstance(), it comes up as deprecated since java 9.

[schuettec]

Cool!
Okay you're right, the exception handling might be a little too unspecific. But the solution might come up with changing the way of instantiation. As far as i know newInstance() is deprecated because you should use java.lang.Class.getConstructor(Class<?>...) to get the constructor first. The reflective constructor provides another newInstance() method, which is the replacement for the deprecated one.
As a result if you use the getConstructors() method, you get a bunch of other exception types to handle. There is something like NoSuchMethodException if you request a constructor (the default no-param one) and it's not available. This would be the right situation to rethrow it with a nice message.

Does this help?