Pass annotations from source property to custom mapping method

[cnaj]

As described in Issue #3140 and discussion #3132.

This is a proof-of-concept that works for my use case. I'd be happy if you could comment on this PR where I might have missed some best practices or where I simply might have done something stupid :-)

I'll add improvements and documentation if you'd consider this PR for inclusion.

[filiphr]

I had a brief look at this and I like the idea. We'll need to discuss with the team to see if everyone agrees that we should have something like this.

However, the generated code will need to be changed. We are not fans of using reflection during runtime. I think that perhaps we would need to generate an implementation of the annotation and then create an instance and pass it to the methods.

Something else, how are we going to handle repeatable annotations?

[cnaj]

I'm glad that you find it interesting.

Concerning the runtime reflection: I've thought about it, but I didn't know if you even can create instances of an annotation, or if they are singletons managed by the VM. Anyway, when working with annotations, reflection is the natural way of retrieving them, even in hand-optimized code. So, IMHO, the use of reflection is justified in this case.

About repeatable annotations: I think this is handled with "container annotations", see e.g. JLS 8, chapter 9.7.5. So it's up to the mapper implementation to deal with them.

[cnaj]

Another thought: We could move the reflection part to the mapper class constructor. Would that be OK from your perspective?

[cnaj]

Edit: Changed annotation constants to array.

The mapper could look like this, for example:

public class EmployeeMapperImplPoc implements EmployeeMapper {
    public static final EmployeeMapperImplPoc INSTANCE = new EmployeeMapperImplPoc();

    private static final Confidential[] SOURCE_ANNOTATIONS_CONFIDENTIAL;

    static {
        try {
            SOURCE_ANNOTATIONS_CONFIDENTIAL = new Confidential[] {
                Employee.class.getMethod( "getAddress" ).getAnnotation( Confidential.class ),
                Employee.class.getMethod( "getSalary" ).getAnnotation( Confidential.class ),
            };
        }
        catch ( NoSuchMethodException e ) {
            throw new RuntimeException( e );
        }
    }

    @Override
    public EmployeeDTO toDto(Employee value, String accessLevel) {
        if ( value == null ) {
            return null;
        }

        String name = null;
        String address = null;
        String salary = null;

        name = mapProperty( value.getName(), accessLevel, null );
        address = mapProperty( value.getAddress(), accessLevel, SOURCE_ANNOTATIONS_CONFIDENTIAL[0] );
        salary = mapProperty( value.getSalary(), accessLevel, SOURCE_ANNOTATIONS_CONFIDENTIAL[1] );

        EmployeeDTO employeeDTO = new EmployeeDTO( name, address, salary );

        return employeeDTO;
    }
}

[filiphr]

Concerning the runtime reflection: I've thought about it, but I didn't know if you even can create instances of an annotation, or if they are singletons managed by the VM. Anyway, when working with annotations, reflection is the natural way of retrieving them, even in hand-optimized code. So, IMHO, the use of reflection is justified in this case.

The compiler does not stop you of implementing an annotation and creating an interface out of it. I think that I would prefer the approach of creating a class and implementing it rather than using reflection. We have all the information during compile time, so why not get it and use it.

I don't like the use of an array.

This:

address = mapProperty( value.getAddress(), accessLevel, ADDRESS_CONFIDENTIAL_ANNOTATION );

is way more readable and understandable than:

address = mapProperty( value.getAddress(), accessLevel, SOURCE_ANNOTATIONS_CONFIDENTIAL[0] );

---

The reason why I am saying these things is the fact that the goals of MapStruct are to avoid reflection at runtime and to generate a human readable code.

[cnaj]

I've done a rework to my pull request so that the needed annotations will be stored into fields in the constructor. Here's what my branch produces for the example above:

public class EmployeeMapperImpl implements EmployeeMapper {

    private final Confidential employeeAddressConfidentialAnnotation;
    private final Confidential employeeNameConfidentialAnnotation;
    private final Confidential employeeSalaryConfidentialAnnotation;

    public EmployeeMapperImpl() {
        employeeAddressConfidentialAnnotation = getMethodAnnotation( Employee.class, "getAddress", Confidential.class );
        employeeNameConfidentialAnnotation = getMethodAnnotation( Employee.class, "getName", Confidential.class );
        employeeSalaryConfidentialAnnotation = getMethodAnnotation( Employee.class, "getSalary", Confidential.class );
    }

    @Override
    public EmployeeDTO toDto(Employee value, String accessLevel) {
        if ( value == null ) {
            return null;
        }

        String name = null;
        String address = null;
        String salary = null;

        name = mapProperty( value.getName(), accessLevel, employeeNameConfidentialAnnotation );
        address = mapProperty( value.getAddress(), accessLevel, employeeAddressConfidentialAnnotation );
        salary = mapProperty( value.getSalary(), accessLevel, employeeSalaryConfidentialAnnotation );

        EmployeeDTO employeeDTO = new EmployeeDTO( name, address, salary );

        return employeeDTO;
    }

    private static <T extends Annotation> T getMethodAnnotation(Class<?> sourceType, String methodName, Class<T> annotationClass) {
        try {
            return sourceType.getMethod( methodName ).getAnnotation( annotationClass );
        }
        catch (NoSuchMethodException e) {
            throw new RuntimeException(e);
        }
    }
}

I take your point about readable (field) names. However, in contrast to my previous suggestion, I decided against static fields / constants because (1) there is already Field support in MapStruct and (2) I think you have more control about when the initialization runs as opposed to having a static initializer. Anyway, static or not should not be a major problem; if you like I can look for a solution to produce static constants for the annotations.

Concerning reflection (again): I think there is no benefit in instantiating the annotation classes by ourselves:

1. There is no runtime penalty except when constructing the mapper instance. During mapping invocation, there is no runtime reflection.
2. Using reflection like above is natural for hand-written code, even when optimizing for runtime performance.
3. Regarding this discussion on Stack Overflow, creating an instance of an annotation is not that easy for it to behave exactly as a system-provided instance. The cleanest solution as far as I can see from the other answers is to generate a dynamic proxy, which is normally helped by third-party libraries. I think it's out of the question to introduce third-party dependencies in generated code. On the other hand, creating dynamic proxies in generated code would look awkward and unusual for readable code.

What do you think?

[cnaj]

Yet another change; this one removes the helper methods so that the example mapper looks like this:

public class EmployeeMapperImpl implements EmployeeMapper {

    private final Confidential employeeAddressConfidentialAnnotation;
    private final Confidential employeeNameConfidentialAnnotation;
    private final Confidential employeeSalaryConfidentialAnnotation;

    public EmployeeMapperImpl() {
        try {
            employeeAddressConfidentialAnnotation = Employee.class.getMethod( "getAddress" ).getAnnotation( Confidential.class );
            employeeNameConfidentialAnnotation = Employee.class.getMethod( "getName" ).getAnnotation( Confidential.class );
            employeeSalaryConfidentialAnnotation = Employee.class.getMethod( "getSalary" ).getAnnotation( Confidential.class );
        }
        catch (ReflectiveOperationException e) {
            throw new RuntimeException(e);
        }
    }

    @Override
    public EmployeeDTO toDto(Employee value, String accessLevel) {
        if ( value == null ) {
            return null;
        }

        String name = null;
        String address = null;
        String salary = null;

        name = mapProperty( value.getName(), accessLevel, employeeNameConfidentialAnnotation );
        address = mapProperty( value.getAddress(), accessLevel, employeeAddressConfidentialAnnotation );
        salary = mapProperty( value.getSalary(), accessLevel, employeeSalaryConfidentialAnnotation );

        EmployeeDTO employeeDTO = new EmployeeDTO( name, address, salary );

        return employeeDTO;
    }
}

[Ksenia989]

Such a nice fix, there's been a few months of discussion, isn't it ready to be merged?