fix: instrument legacy callables in Vaadin 14-24

Author: javier-godoy

src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java

@@ -30,12 +30,12 @@
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
-import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
 import java.util.WeakHashMap;
 import java.util.concurrent.ConcurrentHashMap;
-import lombok.experimental.UtilityClass;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 import org.objectweb.asm.ClassWriter;
 import org.objectweb.asm.MethodVisitor;
 import org.objectweb.asm.Opcodes;
@@ -59,11 +59,15 @@
  *
  * @author Javier Godoy / Flowing Code
  */
-@UtilityClass
 final class ClassInstrumentationUtil {
 
-  private static final Map<ClassLoader, InstrumentedClassLoader> classLoaderCache =
-      new WeakHashMap<>();
+  private final int version;
+
+  private static final Map<ClassLoader, InstrumentedClassLoader> classLoaderCache = new WeakHashMap<>();
+
+  ClassInstrumentationUtil(int version) {
+    this.version = version;
+  }
 
   /**
    * Creates and returns an instance of a dynamically instrumented class that extends the specified
@@ -144,34 +148,54 @@ public <T extends Component> Class<? extends T> instrumentClass(Class<T> parent)
     }
   }
 
-  private static boolean needsInstrumentation(Class<?> parent) {
+  private boolean needsInstrumentation(Class<?> parent) {
     return !getInstrumentableMethods(parent).isEmpty();
   }
 
-  private static List<Method> getInstrumentableMethods(Class<?> parent) {
-    List<Method> methods = new ArrayList<>();
-    for (Method method : parent.getDeclaredMethods()) {
-      if (!Modifier.isStatic(method.getModifiers()) && !Modifier.isPrivate(method.getModifiers())) {
+  private boolean hasLegacyVaadin() {
+    return version <= 24;
+  }
+
+  private static Stream<Method> getDeclaredCallables(Class<?> parent) {
+    return Stream.of(parent.getDeclaredMethods()).filter(method -> {
+      int modifiers = method.getModifiers();
+      if (!Modifier.isStatic(modifiers) && !Modifier.isPrivate(modifiers)) {
         boolean isCallable = method.isAnnotationPresent(ClientCallable.class);
         boolean isLegacyCallable = method.isAnnotationPresent(LegacyClientCallable.class);
-        if (isCallable || isLegacyCallable) {
-          boolean hasJsonValueReturn = JsonValue.class.isAssignableFrom(method.getReturnType());
-          boolean hasJsonValueParams = hasJsonValueParameters(method);
-
-          if (isCallable && hasJsonValueParams) {
-            throw new IllegalArgumentException(String.format(
-                "Instrumented method '%s' in class '%s' has JsonValue arguments and must be annotated with @%s instead of @ClientCallable",
-                method.getName(), method.getDeclaringClass().getName(),
-                LegacyClientCallable.class.getName()));
-          } else if (isCallable && hasJsonValueReturn) {
-            methods.add(method);
-          } else if (isLegacyCallable) {
-            methods.add(method);
-          }
+        return isCallable || isLegacyCallable;
+      }
+      return false;
+    });
+  }
+
+  private List<Method> getInstrumentableMethods(Class<?> parent) {
+    return getDeclaredCallables(parent).filter(method -> {
+      boolean isCallable = method.isAnnotationPresent(ClientCallable.class);
+      boolean isLegacyCallable = method.isAnnotationPresent(LegacyClientCallable.class);
+
+      if (hasLegacyVaadin()) {
+        return isLegacyCallable;
+      }
+
+      if (isCallable || isLegacyCallable) {
+        boolean hasJsonValueReturn = JsonValue.class.isAssignableFrom(method.getReturnType());
+        boolean hasJsonValueParams = hasJsonValueParameters(method);
+
+        if (isCallable && hasJsonValueParams) {
+          throw new IllegalArgumentException(String.format(
+              "Instrumented method '%s' in class '%s' has JsonValue arguments and must be annotated with @%s instead of @ClientCallable",
+              method.getName(), method.getDeclaringClass().getName(),
+              LegacyClientCallable.class.getName()));
+        } else if (isCallable && hasJsonValueReturn) {
+          return true;
+        } else if (isLegacyCallable) {
+          return true;
         }
       }
-    }
-    return methods;
+
+      return false;
+
+    }).collect(Collectors.toList());
   }
 
   private static boolean hasJsonValueParameters(Method method) {
@@ -185,8 +209,7 @@ private static boolean hasJsonValueParameters(Method method) {
 
   private <T extends Component> Class<? extends T> createInstrumentedClass(Class<T> parent,
       String className) throws Exception {
-    InstrumentedClassLoader classLoader =
-        getOrCreateInstrumentedClassLoader(parent.getClassLoader());
+    InstrumentedClassLoader classLoader = getOrCreateInstrumentedClassLoader(parent.getClassLoader());
     return classLoader.defineInstrumentedClass(className, parent).asSubclass(parent);
   }
 
@@ -196,7 +219,7 @@ private InstrumentedClassLoader getOrCreateInstrumentedClassLoader(ClassLoader p
     }
   }
 
-  private static final class InstrumentedClassLoader extends ClassLoader {
+  private final class InstrumentedClassLoader extends ClassLoader {
 
     private final Map<Class<?>, Class<?>> instrumentedClassCache = new ConcurrentHashMap<>();
 
@@ -244,8 +267,8 @@ private void generateClientCallableOverrides(ClassWriter cw, Class<?> parent,
     }
 
     private void generateMethodOverride(ClassWriter cw, Method method, String internalParentName) {
-      boolean hasJsonValueReturn = JsonValue.class.isAssignableFrom(method.getReturnType());
-      boolean hasJsonValueParams = hasJsonValueParameters(method);
+      boolean hasJsonValueReturn = !hasLegacyVaadin() && JsonValue.class.isAssignableFrom(method.getReturnType());
+      boolean hasJsonValueParams = !hasLegacyVaadin() && hasJsonValueParameters(method);
 
       String overrideDescriptor = getMethodDescriptor(method, hasJsonValueParams);
       String superDescriptor = getMethodDescriptor(method, false);
@@ -300,7 +323,7 @@ private void generateMethodOverride(ClassWriter cw, Method method, String intern
             false);
       }
 
-      // Return converted result or void
+      // Return result or void
       if (method.getReturnType() == Void.TYPE) {
         mv.visitInsn(Opcodes.RETURN);
       } else {

src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java

@@ -42,9 +42,11 @@
 @NoArgsConstructor
 class JsonMigrationHelper25 implements JsonMigrationHelper {
 
+  private static final ClassInstrumentationUtil instrumentation = new ClassInstrumentationUtil(25);
+
   @Override
   public <T extends Component> Class<? extends T> instrumentClass(Class<T> clazz) {
-    return ClassInstrumentationUtil.instrumentClass(clazz);
+    return instrumentation.instrumentClass(clazz);
   }
 
   @Override

src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyJsonMigrationHelper.java

@@ -31,9 +31,11 @@
 @NoArgsConstructor
 class LegacyJsonMigrationHelper implements JsonMigrationHelper {
 
+  private static final ClassInstrumentationUtil instrumentation = new ClassInstrumentationUtil(24);
+
   @Override
-  public <T extends Component> Class<T> instrumentClass(Class<T> clazz) {
-    return clazz;
+  public <T extends Component> Class<? extends T> instrumentClass(Class<T> clazz) {
+    return instrumentation.instrumentClass(clazz);
   }
 
   @Override