Add unit tests and fixes to class instrumentation

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

@@ -0,0 +1,36 @@
+package com.flowingcode.vaadin.jsonmigration;
+
+import elemental.json.JsonArray;
+import elemental.json.JsonBoolean;
+import elemental.json.JsonNumber;
+import elemental.json.JsonObject;
+import elemental.json.JsonString;
+import elemental.json.JsonValue;
+import org.objectweb.asm.Type;
+import tools.jackson.databind.JsonNode;
+import tools.jackson.databind.node.ArrayNode;
+import tools.jackson.databind.node.BooleanNode;
+import tools.jackson.databind.node.DoubleNode;
+import tools.jackson.databind.node.ObjectNode;
+import tools.jackson.databind.node.StringNode;
+
+class ClassInstrumentationJacksonHelper {
+
+  public static String getConvertedTypeDescriptor(Class<?> type) {
+    if (type == JsonObject.class) {
+      return Type.getDescriptor(ObjectNode.class);
+    } else if (type == JsonArray.class) {
+      return Type.getDescriptor(ArrayNode.class);
+    } else if (type == JsonBoolean.class) {
+      return Type.getDescriptor(BooleanNode.class);
+    } else if (type == JsonNumber.class) {
+      return Type.getDescriptor(DoubleNode.class);
+    } else if (type == JsonString.class) {
+      return Type.getDescriptor(StringNode.class);
+    } else if (JsonValue.class.isAssignableFrom(type)) {
+      return Type.getDescriptor(JsonNode.class);
+    }
+    return Type.getDescriptor(type);
+  }
+
+}

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

@@ -21,31 +21,24 @@
 
 import com.vaadin.flow.component.ClientCallable;
 import com.vaadin.flow.component.Component;
-import elemental.json.JsonArray;
-import elemental.json.JsonBoolean;
-import elemental.json.JsonNumber;
-import elemental.json.JsonObject;
-import elemental.json.JsonString;
 import elemental.json.JsonValue;
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.MethodHandles;
+import java.lang.invoke.MethodType;
 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 lombok.SneakyThrows;
 import org.objectweb.asm.ClassWriter;
 import org.objectweb.asm.MethodVisitor;
 import org.objectweb.asm.Opcodes;
 import org.objectweb.asm.Type;
-import tools.jackson.databind.JsonNode;
-import tools.jackson.databind.node.ArrayNode;
-import tools.jackson.databind.node.BooleanNode;
-import tools.jackson.databind.node.DoubleNode;
-import tools.jackson.databind.node.ObjectNode;
-import tools.jackson.databind.node.StringNode;
 
 /**
  * Utility class for instrumenting classes at runtime.
@@ -59,11 +52,15 @@
  *
  * @author Javier Godoy / Flowing Code
  */
-@UtilityClass
 final class ClassInstrumentationUtil {
 
-  private static final Map<ClassLoader, InstrumentedClassLoader> classLoaderCache =
-      new WeakHashMap<>();
+  private final int version;
+
+  private 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 +141,56 @@ 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);
+      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(),
+            LegacyClientCallable.class.getSimpleName()));
+      }
+
+      if (hasLegacyVaadin()) {
+        return isLegacyCallable;
+      }
+
+      if (isCallable || isLegacyCallable) {
+
+        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 +204,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 +214,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 +262,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,9 +318,19 @@ 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 if (method.getReturnType().isPrimitive()) {
+        if (method.getReturnType() == Long.TYPE) {
+          mv.visitInsn(Opcodes.LRETURN);
+        } else if (method.getReturnType() == Float.TYPE) {
+          mv.visitInsn(Opcodes.FRETURN);
+        } else if (method.getReturnType() == Double.TYPE) {
+          mv.visitInsn(Opcodes.DRETURN);
+        } else {
+          mv.visitInsn(Opcodes.IRETURN);
+        }
       } else {
         mv.visitInsn(Opcodes.ARETURN);
       }
@@ -344,21 +372,17 @@ private String getMethodDescriptor(Method method, boolean convertJsonValueParams
       return sb.toString();
     }
 
+    private MethodHandle getConvertedTypeDescriptor;
+
+    @SneakyThrows
     private String getConvertedTypeDescriptor(Class<?> type) {
-      if (type == JsonObject.class) {
-        return Type.getDescriptor(ObjectNode.class);
-      } else if (type == JsonArray.class) {
-        return Type.getDescriptor(ArrayNode.class);
-      } else if (type == JsonBoolean.class) {
-        return Type.getDescriptor(BooleanNode.class);
-      } else if (type == JsonNumber.class) {
-        return Type.getDescriptor(DoubleNode.class);
-      } else if (type == JsonString.class) {
-        return Type.getDescriptor(StringNode.class);
-      } else if (JsonValue.class.isAssignableFrom(type)) {
-        return Type.getDescriptor(JsonNode.class);
+      if (getConvertedTypeDescriptor == null) {
+        Class<?> helper = Class.forName("com.flowingcode.vaadin.jsonmigration.ClassInstrumentationJacksonHelper");
+        MethodHandles.Lookup lookup = MethodHandles.lookup();
+        MethodType methodType = MethodType.methodType(String.class, Class.class);
+        getConvertedTypeDescriptor = lookup.findStatic(helper, "getConvertedTypeDescriptor", methodType);
       }
-      return Type.getDescriptor(type);
+      return (String) getConvertedTypeDescriptor.invokeExact(type);
     }
 
     private String[] getExceptionInternalNames(Class<?>[] exceptionTypes) {

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

@@ -28,5 +28,10 @@ public ElementalNullNode() {
     super();
   }
 
+  @Override
+  public String toJson() {
+    return null;
+  }
+
 }
 

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

@@ -28,5 +28,14 @@ public ElementalNumberNode(double value) {
     super(value);
   }
 
+  @Override
+  public String toJson() {
+    double value = doubleValue();
+    if (value == (long) value) {
+      return String.valueOf((long) value);
+    } else {
+      return UnsupportedJsonValueImpl.super.toJson();
+    }
+  }
 }
 

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

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

@@ -21,6 +21,7 @@
 
 import elemental.json.JsonType;
 import elemental.json.JsonValue;
+import tools.jackson.databind.JsonNode;
 
 interface UnsupportedJsonValueImpl extends JsonValue {
 
@@ -46,7 +47,7 @@ default JsonType getType() {
 
   @Override
   default String toJson() {
-    throw new UnsupportedOperationException();
+    return ((JsonNode) this).toString();
   }
 
   @Override

src/test/java/com/flowingcode/vaadin/jsonmigration/BaseClientCallable.java

@@ -0,0 +1,17 @@
+package com.flowingcode.vaadin.jsonmigration;
+
+import com.vaadin.flow.component.html.Div;
+
+public abstract class BaseClientCallable extends Div {
+
+  private boolean traced;
+
+  protected final void trace() {
+    traced = true;
+  }
+
+  public boolean hasBeenTraced() {
+    return traced;
+  }
+
+}

src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_D__V.java

@@ -0,0 +1,14 @@
+package com.flowingcode.vaadin.jsonmigration;
+
+
+import com.vaadin.flow.component.ClientCallable;
+
+public class ClientCallable_D__V extends BaseClientCallable {
+
+    @ClientCallable
+    public void test(double arg) {
+        trace();
+    }
+}
+
+

src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_I__V.java

@@ -0,0 +1,14 @@
+package com.flowingcode.vaadin.jsonmigration;
+
+
+import com.vaadin.flow.component.ClientCallable;
+
+public class ClientCallable_I__V extends BaseClientCallable {
+
+    @ClientCallable
+    public void test(int arg) {
+        trace();
+    }
+}
+
+

src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonArray__V.java

@@ -0,0 +1,16 @@
+package com.flowingcode.vaadin.jsonmigration;
+
+
+import com.vaadin.flow.component.ClientCallable;
+
+import elemental.json.JsonArray;
+
+public class ClientCallable_JsonArray__V extends BaseClientCallable {
+
+    @ClientCallable
+    public void test(JsonArray arg) {
+        trace();
+    }
+}
+
+