[java] Fixing InstanceCoercer to test properly for having a constructor in the target class (and code cleanup)

Author: barancev

java/client/src/org/openqa/selenium/json/InstanceCoercer.java

@@ -25,7 +25,6 @@
 import java.lang.reflect.Modifier;
 import java.lang.reflect.ParameterizedType;
 import java.lang.reflect.Type;
-import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.LinkedList;
 import java.util.List;
@@ -45,8 +44,12 @@ class InstanceCoercer extends TypeCoercer<Object> {
 
   @Override
   public boolean test(Class aClass) {
-    // If the class doesn't have a no-arg constructor, abandon hope
-    return getConstructor(aClass) != null;
+    try {
+      getConstructor(aClass);
+      return true;
+    } catch (JsonException ex) {
+      return false;
+    }
   }
 
   @Override
@@ -97,7 +100,7 @@ public BiFunction<JsonInput, PropertySetting, Object> apply(Type type) {
 
   private Map<String, TypeAndWriter> getFieldWriters(Constructor<?> constructor) {
     List<Field> fields = new LinkedList<>();
-    for (Class current = constructor.getDeclaringClass(); current != Object.class; current = current.getSuperclass()) {
+    for (Class<?> current = constructor.getDeclaringClass(); current != Object.class; current = current.getSuperclass()) {
       fields.addAll(Arrays.asList(current.getDeclaredFields()));
     }
 
@@ -109,17 +112,16 @@ private Map<String, TypeAndWriter> getFieldWriters(Constructor<?> constructor) {
             Collectors.toMap(
                 Field::getName,
                 field -> {
-                  TypeAndWriter tw = new TypeAndWriter();
-                  tw.type = field.getGenericType();
-                  tw.writer =
+                  Type type = field.getGenericType();
+                  BiConsumer<Object, Object> writer =
                       (instance, value) -> {
                         try {
                           field.set(instance, value);
                         } catch (IllegalAccessException e) {
                           throw new JsonException(e);
                         }
                       };
-                  return tw;
+                  return new TypeAndWriter(type, writer);
                 }));
   }
 
@@ -131,9 +133,8 @@ private Map<String, TypeAndWriter> getBeanWriters(Constructor<?> constructor) {
             Collectors.toMap(
                 SimplePropertyDescriptor::getName,
                 desc -> {
-                  TypeAndWriter tw = new TypeAndWriter();
-                  tw.type = desc.getWriteMethod().getGenericParameterTypes()[0];
-                  tw.writer = (instance, value) -> {
+                  Type type = desc.getWriteMethod().getGenericParameterTypes()[0];
+                  BiConsumer<Object, Object>writer = (instance, value) -> {
                       Method method = desc.getWriteMethod();
                       method.setAccessible(true);
                       try {
@@ -142,31 +143,31 @@ private Map<String, TypeAndWriter> getBeanWriters(Constructor<?> constructor) {
                         throw new JsonException(e);
                       }
                     };
-                  return tw;
+                  return new TypeAndWriter(type, writer);
                 }));
   }
 
   private Constructor<?> getConstructor(Type type) {
-    Class target = getClss(type);
+    Class<?> target = getClss(type);
 
     try {
-      @SuppressWarnings("unchecked") Constructor<?> constructor = target.getDeclaredConstructor();
+      Constructor<?> constructor = target.getDeclaredConstructor();
       constructor.setAccessible(true);
       return constructor;
     } catch (ReflectiveOperationException e) {
       throw new JsonException(e);
     }
   }
 
-  private static Class getClss(Type type) {
-    Class target = null;
+  private static Class<?> getClss(Type type) {
+    Class<?> target = null;
 
     if (type instanceof Class) {
-      target = (Class) type;
+      target = (Class<?>) type;
     } else if (type instanceof ParameterizedType) {
       Type rawType = ((ParameterizedType) type).getRawType();
       if (rawType instanceof Class) {
-        target = (Class) rawType;
+        target = (Class<?>) rawType;
       }
     }
 
@@ -176,9 +177,14 @@ private static Class getClss(Type type) {
     return target;
   }
 
-  private class TypeAndWriter {
-    public Type type;
-    public BiConsumer<Object, Object> writer;
+  private static class TypeAndWriter {
+    private final Type type;
+    private final BiConsumer<Object, Object> writer;
+
+    public TypeAndWriter(Type type, BiConsumer<Object, Object> writer) {
+      this.type = type;
+      this.writer = writer;
+    }
   }
 
 }

java/client/test/org/openqa/selenium/json/JsonTest.java

@@ -49,7 +49,9 @@
 import static java.util.logging.Level.OFF;
 import static java.util.logging.Level.WARNING;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 import static org.assertj.core.api.Assertions.byLessThan;
+import static org.assertj.core.api.InstanceOfAssertFactories.MAP;
 import static org.openqa.selenium.Proxy.ProxyType.PAC;
 import static org.openqa.selenium.json.Json.MAP_TYPE;
 import static org.openqa.selenium.logging.LogType.BROWSER;
@@ -198,6 +200,16 @@ public void canPopulateASimpleBean() {
     assertThat(bean.getValue()).isEqualTo("time");
   }
 
+  @Test
+  public void canNotPopulateAnObjectOfAClassWithNoDefaultConstructor() {
+    String raw = "{\"value\": \"time\"}";
+
+    assertThatExceptionOfType(JsonException.class)
+      .isThrownBy(() -> new Json().toType(raw, NoDefaultConstructor.class))
+      .withMessageStartingWith(
+        "Unable to find type coercer for class %s", NoDefaultConstructor.class.getTypeName());
+  }
+
   @Test
   public void willSilentlyDiscardUnusedFieldsWhenPopulatingABean() {
     String raw = "{\"value\": \"time\", \"frob\": \"telephone\"}";
@@ -263,8 +275,8 @@ public void shouldConvertAResponseWithAnElementInIt() {
         "{\"value\":{\"value\":\"\",\"text\":\"\",\"selected\":false,\"enabled\":true,\"id\":\"three\"},\"context\":\"con\",\"sessionId\":\"sess\"}";
     Response converted = new Json().toType(json, Response.class);
 
-    Map<String, Object> value = (Map<String, Object>) converted.getValue();
-    assertThat(value).containsEntry("id", "three");
+    assertThat(converted).extracting("value").asInstanceOf(MAP)
+      .containsEntry("id", "three");
   }
 
   @Test
@@ -310,8 +322,7 @@ public void shouldConvertObjectsInArraysToMaps() {
     Object first = list.get(0);
     assertThat(first instanceof Map).isTrue();
 
-    Map<String, Object> map = (Map<String, Object>) first;
-    assertThat(map)
+    assertThat(first).asInstanceOf(MAP)
         .containsEntry("name", "foo")
         .containsEntry("value", "bar")
         .containsEntry("domain", "localhost")
@@ -540,6 +551,23 @@ public void setValue(String value) {
     }
   }
 
+  public static class NoDefaultConstructor {
+
+    private String value;
+
+    public NoDefaultConstructor(String value) {
+      this.value = value;
+    }
+
+    public String getValue() {
+      return value;
+    }
+
+    public void setValue(String value) {
+      this.value = value;
+    }
+  }
+
   public static class ContainingBean {
 
     private String name;