Stop catching Jparepository methods with aspectj

Question

I am trying to generate a warning if the programmer returns an Arraylist instead of a list. I use Spring Boot , Spring Data JPA.

A sample Pojo

@Entity
public class Box {

    @Id
    @GeneratedValue
    private long id;

    private long prio;


    public long getPrio() {
        return prio;
    }

    public void setPrio(long prio) {
        this.prio = prio;
    }

    public long getId() {
        return id;
    }   

    public void setId(long id) {
        this.id = id;
    }

}

my repository:

@Repository
public interface BoxRepository extends JpaRepository<Box, Long>{

    public List findByPrio(long prio);  
}

now my Aspect:

@Aspect
@Component
public class ReturnList {

    @AfterReturning(value = "within(de.fhb.*) && !within(org.springframework.*) && call(* de.fhb..*(..))", returning = "returnValue")
    public void logServiceAccess(JoinPoint joinPoint, Object returnValue) {

        if (returnValue != null) {
            if (returnValue.getClass() != null) {

                Class<?> clazz = returnValue.getClass();

                if (java.util.List.class.isAssignableFrom(clazz)) {
                    System.out
                            .println("Please use List instead of a concrete implementation ( "+ returnValue.getClass() + " ) for method: "
                                + joinPoint.getSignature().getName() + ".");
                }

            }
        }

    }

}

My problem

looks like spring data (jpa repository) is returning an Arraylist. I dont want to catch methods from the jpa repository, i excluded the org.springframework but the aspect is still triggered if i run something like this line:

System.out.println(boxRepository.findByPrio(1));

Any hints that will stop trigger the aspect from calling spring jparepository methods ?

complete code : https://github.com/svenhornberg/MDSD

Answer 1

First of all, List.class.isAssignableFrom(clazz) is true for List as well as for ArrayList, i.e. you cannot differentiate between the two like this. And by the way, returnValue.getClass() will never evaluate to List because that is an interface type. It will always evaluate to the actual implementation class such as ArrayList. So your contrived way of trying to find out what you want to know via reflection is doomed anyway.

The good news is: AspectJ enables you to do what you want. You seem to use real AspectJ via load or compile time weaving, otherwise you could not use the call() pointcut because it is unavailable in Spring AOP. Edit: Yes, your Gradle build shows you are using compile time weaving. But hey, why are you using the compiler in the current version 1.8.4 and the AspectJ runtime in a massively outdated 1.5.4? You should harmonise the two and also use aspectjrt in version 1.8.4.

Now let us deconstruct your pointcut:

within(de.fhb.*) &&
!within(org.springframework.*) &&
call(* de.fhb..*(..))

It means: Intercept calls to any methods defined in de.fhb or its subpackages (..* notation), but only if the calls are also made from classes defined in de.fhb but not in subpackages (.* notation). The !within(org.springframework.*) part is redundant because code defined in de.fhb.* can never be also come from org.springframework.* at the same time. It makes no sense.

What you probably really want is find out if in your own packages there are methods returning a concrete type like ArrayList instead of the interface type List. Correct? I think the pointcut should rather look like this:

within(de.fhb..*) &&
execution(java.util.List+ *(..)) &&
!execution(java.util.List *(..))

It means: For all classes within de.fhb or its subpackages, intercept all methods returning a List+ (the + means: a List or its subtypes) according to its method signature. Because this would also match methods returning the parent type List and you only want the subtypes, you must exclude that type in the third part of the pointcut via !execution(java.util.List *(..)).

I am matching method executions rather than calls because that is more efficient. If a method is called from 100 places in your package, call() would weave aspect code into 100 calling joinpoints while execution() really just weaves place where the method is actually defined.

Here is some sample code:

Driver application:

You see that there is one method correctly declaring (according to your wish) a List return type while others declare "forbidden" return types such as ArrayList, LinkedList and Vector.

package de.fhb.app;

import java.util.ArrayList;
import java.util.LinkedList;
import java.util.List;
import java.util.Vector;

public class Application {
    public static List<String> methodReturningList() {
        return new ArrayList<String>();
    }

    public static ArrayList<String> methodReturningArrayList() {
        return new ArrayList<String>();
    }

    public static LinkedList<String> methodReturningLinkedList() {
        return new LinkedList<String>();
    }

    public static Vector<String> methodReturningVector() {
        return new Vector<String>();
    }

    public static void main(String[] args) {
        methodReturningList();
        methodReturningArrayList();
        methodReturningLinkedList();
        methodReturningVector();
    }
}

Type checking aspect, variant A (runtime checking):

package de.fhb.aspect;

import org.aspectj.lang.JoinPoint;
import org.aspectj.lang.annotation.AfterReturning;
import org.aspectj.lang.annotation.Aspect;

@Aspect
public class ReturnTypeChecker {
    @AfterReturning("within(de.fhb..*) && execution(java.util.List+ *(..)) && !execution(java.util.List *(..))")
    public void logServiceAccess(JoinPoint thisJoinPoint) {
        System.out.println(thisJoinPoint);
    }
}

Console output:

execution(ArrayList de.fhb.app.Application.methodReturningArrayList())
execution(LinkedList de.fhb.app.Application.methodReturningLinkedList())
execution(Vector de.fhb.app.Application.methodReturningVector())

As you can see, you get exactly those methods intercepted which are problematic according to your definition.

Type checking aspect, variant B (compile time checking):

But AspectJ can do more. Why not throw a warning or even an error during compilation, i.e. before the software is even packaged and deployed? Your development team can fix the bugs before they get into the production code. In order to do that, use @DeclareWarning or @DeclareError:

package de.fhb.aspect;

import org.aspectj.lang.annotation.Aspect;
import org.aspectj.lang.annotation.DeclareError;

@Aspect
public class ReturnTypeChecker {
    @DeclareError("within(de.fhb..*) && execution(java.util.List+ *(..)) && !execution(java.util.List *(..))")
    private static final String typeWarning = "Please do not declare methods returning concrete subclasses of List";
}

Now you will get compilation errors on the console. In Eclipse it looks like this: