Merge pull request #1631 from harawata/gh/1201-2

harawata · web-flow · commit 876038379b13 · 2019-08-15T22:52:37.000+09:00
Ambiguous setters/getters should throw exception only when they are invoked
diff --git a/src/main/java/org/apache/ibatis/reflection/Reflector.java b/src/main/java/org/apache/ibatis/reflection/Reflector.java
@@ -24,6 +24,7 @@
 import java.lang.reflect.ParameterizedType;
 import java.lang.reflect.ReflectPermission;
 import java.lang.reflect.Type;
+import java.text.MessageFormat;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
@@ -33,6 +34,7 @@
 import java.util.Map;
 import java.util.Map.Entry;
 
+import org.apache.ibatis.reflection.invoker.AmbiguousMethodInvoker;
 import org.apache.ibatis.reflection.invoker.GetFieldInvoker;
 import org.apache.ibatis.reflection.invoker.Invoker;
 import org.apache.ibatis.reflection.invoker.MethodInvoker;
@@ -92,6 +94,7 @@ private void resolveGetterConflicts(Map<String, List<Method>> conflictingGetters
     for (Entry<String, List<Method>> entry : conflictingGetters.entrySet()) {
       Method winner = null;
       String propName = entry.getKey();
+      boolean isAmbiguous = false;
       for (Method candidate : entry.getValue()) {
         if (winner == null) {
           winner = candidate;
@@ -101,10 +104,8 @@ private void resolveGetterConflicts(Map<String, List<Method>> conflictingGetters
         Class<?> candidateType = candidate.getReturnType();
         if (candidateType.equals(winnerType)) {
           if (!boolean.class.equals(candidateType)) {
-            throw new ReflectionException(
-                "Illegal overloaded getter method with ambiguous type for property "
-                    + propName + " in class " + winner.getDeclaringClass()
-                    + ". This breaks the JavaBeans specification and can cause unpredictable results.");
+            isAmbiguous = true;
+            break;
           } else if (candidate.getName().startsWith("is")) {
             winner = candidate;
           }
@@ -113,22 +114,23 @@ private void resolveGetterConflicts(Map<String, List<Method>> conflictingGetters
         } else if (winnerType.isAssignableFrom(candidateType)) {
           winner = candidate;
         } else {
-          throw new ReflectionException(
-              "Illegal overloaded getter method with ambiguous type for property "
-                  + propName + " in class " + winner.getDeclaringClass()
-                  + ". This breaks the JavaBeans specification and can cause unpredictable results.");
+          isAmbiguous = true;
+          break;
         }
       }
-      addGetMethod(propName, winner);
+      addGetMethod(propName, winner, isAmbiguous);
     }
   }
 
-  private void addGetMethod(String name, Method method) {
-    if (isValidPropertyName(name)) {
-      getMethods.put(name, new MethodInvoker(method));
-      Type returnType = TypeParameterResolver.resolveReturnType(method, type);
-      getTypes.put(name, typeToClass(returnType));
-    }
+  private void addGetMethod(String name, Method method, boolean isAmbiguous) {
+    MethodInvoker invoker = isAmbiguous
+        ? new AmbiguousMethodInvoker(method, MessageFormat.format(
+            "Illegal overloaded getter method with ambiguous type for property ''{0}'' in class ''{1}''. This breaks the JavaBeans specification and can cause unpredictable results.",
+            name, method.getDeclaringClass().getName()))
+        : new MethodInvoker(method);
+    getMethods.put(name, invoker);
+    Type returnType = TypeParameterResolver.resolveReturnType(method, type);
+    getTypes.put(name, typeToClass(returnType));
   }
 
   private void addSetMethods(Class<?> clazz) {
@@ -140,35 +142,31 @@ private void addSetMethods(Class<?> clazz) {
   }
 
   private void addMethodConflict(Map<String, List<Method>> conflictingMethods, String name, Method method) {
-    List<Method> list = conflictingMethods.computeIfAbsent(name, k -> new ArrayList<>());
-    list.add(method);
+    if (isValidPropertyName(name)) {
+      List<Method> list = conflictingMethods.computeIfAbsent(name, k -> new ArrayList<>());
+      list.add(method);
+    }
   }
 
   private void resolveSetterConflicts(Map<String, List<Method>> conflictingSetters) {
     for (String propName : conflictingSetters.keySet()) {
       List<Method> setters = conflictingSetters.get(propName);
       Class<?> getterType = getTypes.get(propName);
+      boolean isGetterAmbiguous = getMethods.get(propName) instanceof AmbiguousMethodInvoker;
+      boolean isSetterAmbiguous = false;
       Method match = null;
-      ReflectionException exception = null;
       for (Method setter : setters) {
-        if (setter.getParameterTypes()[0].equals(getterType)) {
+        if (!isGetterAmbiguous && setter.getParameterTypes()[0].equals(getterType)) {
           // should be the best match
           match = setter;
           break;
         }
-        if (exception == null) {
-          try {
-            match = pickBetterSetter(match, setter, propName);
-          } catch (ReflectionException e) {
-            // there could still be the 'best match'
-            match = null;
-            exception = e;
-          }
+        if (!isSetterAmbiguous) {
+          match = pickBetterSetter(match, setter, propName);
+          isSetterAmbiguous = match == null;
         }
       }
-      if (match == null) {
-        throw exception;
-      } else {
+      if (match != null) {
         addSetMethod(propName, match);
       }
     }
@@ -185,17 +183,21 @@ private Method pickBetterSetter(Method setter1, Method setter2, String property)
     } else if (paramType2.isAssignableFrom(paramType1)) {
       return setter1;
     }
-    throw new ReflectionException("Ambiguous setters defined for property '" + property + "' in class '"
-        + setter2.getDeclaringClass() + "' with types '" + paramType1.getName() + "' and '"
-        + paramType2.getName() + "'.");
+    MethodInvoker invoker = new AmbiguousMethodInvoker(setter1,
+        MessageFormat.format(
+            "Ambiguous setters defined for property ''{0}'' in class ''{1}'' with types ''{2}'' and ''{3}''.",
+            property, setter2.getDeclaringClass().getName(), paramType1.getName(), paramType2.getName()));
+    setMethods.put(property, invoker);
+    Type[] paramTypes = TypeParameterResolver.resolveParamTypes(setter1, type);
+    setTypes.put(property, typeToClass(paramTypes[0]));
+    return null;
   }
 
   private void addSetMethod(String name, Method method) {
-    if (isValidPropertyName(name)) {
-      setMethods.put(name, new MethodInvoker(method));
-      Type[] paramTypes = TypeParameterResolver.resolveParamTypes(method, type);
-      setTypes.put(name, typeToClass(paramTypes[0]));
-    }
+    MethodInvoker invoker = new MethodInvoker(method);
+    setMethods.put(name, invoker);
+    Type[] paramTypes = TypeParameterResolver.resolveParamTypes(method, type);
+    setTypes.put(name, typeToClass(paramTypes[0]));
   }
 
   private Class<?> typeToClass(Type src) {
diff --git a/src/main/java/org/apache/ibatis/reflection/invoker/AmbiguousMethodInvoker.java b/src/main/java/org/apache/ibatis/reflection/invoker/AmbiguousMethodInvoker.java
@@ -0,0 +1,36 @@
+/**
+ *    Copyright 2009-2019 the original author or authors.
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *       http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+package org.apache.ibatis.reflection.invoker;
+
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+
+import org.apache.ibatis.reflection.ReflectionException;
+
+public class AmbiguousMethodInvoker extends MethodInvoker {
+  private final String exceptionMessage;
+
+  public AmbiguousMethodInvoker(Method method, String exceptionMessage) {
+    super(method);
+    this.exceptionMessage = exceptionMessage;
+  }
+
+  @Override
+  public Object invoke(Object target, Object[] args) throws IllegalAccessException, InvocationTargetException {
+    throw new ReflectionException(exceptionMessage);
+  }
+}
diff --git a/src/test/java/org/apache/ibatis/reflection/ReflectorTest.java b/src/test/java/org/apache/ibatis/reflection/ReflectorTest.java
@@ -18,10 +18,13 @@
 import static org.junit.jupiter.api.Assertions.*;
 
 import java.io.Serializable;
+import java.util.Arrays;
 import java.util.List;
 
+import org.apache.ibatis.reflection.invoker.Invoker;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
+
 import static com.googlecode.catchexception.apis.BDDCatchException.*;
 import static org.assertj.core.api.BDDAssertions.then;
 
@@ -189,20 +192,100 @@ interface BeanInterface<T> {
   }
 
   @Test
-  void shouldSettersWithUnrelatedArgTypesThrowException() {
+  void shouldSettersWithUnrelatedArgTypesThrowException() throws Exception {
     @SuppressWarnings("unused")
     class BeanClass {
-      public void setTheProp(String arg) {}
-      public void setTheProp(Integer arg) {}
+      public void setProp1(String arg) {}
+      public void setProp2(String arg) {}
+      public void setProp2(Integer arg) {}
+      public void setProp2(boolean arg) {}
     }
+    ReflectorFactory reflectorFactory = new DefaultReflectorFactory();
+    Reflector reflector = reflectorFactory.findForClass(BeanClass.class);
+
+    List<String> setableProps = Arrays.asList(reflector.getSetablePropertyNames());
+    assertTrue(setableProps.contains("prop1"));
+    assertTrue(setableProps.contains("prop2"));
+    assertEquals("prop1", reflector.findPropertyName("PROP1"));
+    assertEquals("prop2", reflector.findPropertyName("PROP2"));
+
+    assertEquals(String.class, reflector.getSetterType("prop1"));
+    assertNotNull(reflector.getSetInvoker("prop1"));
+
+    Class<?> paramType = reflector.getSetterType("prop2");
+    assertTrue(String.class.equals(paramType) || Integer.class.equals(paramType));
 
+    Invoker ambiguousInvoker = reflector.getSetInvoker("prop2");
+    Object[] param = String.class.equals(paramType)? new String[]{"x"} : new Integer[]{1};
+    when(ambiguousInvoker).invoke(new BeanClass(), param);
+    then(caughtException()).isInstanceOf(ReflectionException.class)
+        .hasMessageMatching(
+            "Ambiguous setters defined for property 'prop2' in class '" + BeanClass.class.getName().replace("$", "\\$")
+                + "' with types '(java.lang.String|java.lang.Integer|boolean)' and '(java.lang.String|java.lang.Integer|boolean)'\\.");
+  }
+
+  @Test
+  public void shouldTwoGettersForNonBooleanPropertyThrowException() throws Exception {
+    @SuppressWarnings("unused")
+    class BeanClass {
+      public Integer getProp1() {return 1;}
+      public int getProp2() {return 0;}
+      public int isProp2() {return 0;}
+    }
     ReflectorFactory reflectorFactory = new DefaultReflectorFactory();
-    when(reflectorFactory).findForClass(BeanClass.class);
+    Reflector reflector = reflectorFactory.findForClass(BeanClass.class);
+
+    List<String> getableProps = Arrays.asList(reflector.getGetablePropertyNames());
+    assertTrue(getableProps.contains("prop1"));
+    assertTrue(getableProps.contains("prop2"));
+    assertEquals("prop1", reflector.findPropertyName("PROP1"));
+    assertEquals("prop2", reflector.findPropertyName("PROP2"));
+
+    assertEquals(Integer.class, reflector.getGetterType("prop1"));
+    Invoker getInvoker = reflector.getGetInvoker("prop1");
+    assertEquals(Integer.valueOf(1), getInvoker.invoke(new BeanClass(), null));
+
+    Class<?> paramType = reflector.getGetterType("prop2");
+    assertEquals(int.class, paramType);
+
+    Invoker ambiguousInvoker = reflector.getGetInvoker("prop2");
+    when(ambiguousInvoker).invoke(new BeanClass(), new Integer[] {1});
     then(caughtException()).isInstanceOf(ReflectionException.class)
-      .hasMessageContaining("theProp")
-      .hasMessageContaining("BeanClass")
-      .hasMessageContaining("java.lang.String")
-      .hasMessageContaining("java.lang.Integer");
+        .hasMessageContaining("Illegal overloaded getter method with ambiguous type for property 'prop2' in class '"
+            + BeanClass.class.getName()
+            + "'. This breaks the JavaBeans specification and can cause unpredictable results.");
+  }
+
+  @Test
+  public void shouldTwoGettersWithDifferentTypesThrowException() throws Exception {
+    @SuppressWarnings("unused")
+    class BeanClass {
+      public Integer getProp1() {return 1;}
+      public Integer getProp2() {return 1;}
+      public boolean isProp2() {return false;}
+    }
+    ReflectorFactory reflectorFactory = new DefaultReflectorFactory();
+    Reflector reflector = reflectorFactory.findForClass(BeanClass.class);
+
+    List<String> getableProps = Arrays.asList(reflector.getGetablePropertyNames());
+    assertTrue(getableProps.contains("prop1"));
+    assertTrue(getableProps.contains("prop2"));
+    assertEquals("prop1", reflector.findPropertyName("PROP1"));
+    assertEquals("prop2", reflector.findPropertyName("PROP2"));
+
+    assertEquals(Integer.class, reflector.getGetterType("prop1"));
+    Invoker getInvoker = reflector.getGetInvoker("prop1");
+    assertEquals(Integer.valueOf(1), getInvoker.invoke(new BeanClass(), null));
+
+    Class<?> returnType = reflector.getGetterType("prop2");
+    assertTrue(Integer.class.equals(returnType) || boolean.class.equals(returnType));
+
+    Invoker ambiguousInvoker = reflector.getGetInvoker("prop2");
+    when(ambiguousInvoker).invoke(new BeanClass(), null);
+    then(caughtException()).isInstanceOf(ReflectionException.class)
+        .hasMessageContaining("Illegal overloaded getter method with ambiguous type for property 'prop2' in class '"
+            + BeanClass.class.getName()
+            + "'. This breaks the JavaBeans specification and can cause unpredictable results.");
   }
 
   @Test
@@ -218,4 +301,25 @@ public void setBool(boolean bool) {}
     Reflector reflector = reflectorFactory.findForClass(Bean.class);
     assertTrue((Boolean)reflector.getGetInvoker("bool").invoke(new Bean(), new Byte[0]));
   }
+
+  @Test
+  void shouldIgnoreBestMatchSetterIfGetterIsAmbiguous() throws Exception {
+    @SuppressWarnings("unused")
+    class Bean {
+      public Integer isBool() {return Integer.valueOf(1);}
+      public Integer getBool() {return Integer.valueOf(2);}
+      public void setBool(boolean bool) {}
+      public void setBool(Integer bool) {}
+    }
+    ReflectorFactory reflectorFactory = new DefaultReflectorFactory();
+    Reflector reflector = reflectorFactory.findForClass(Bean.class);
+    Class<?> paramType = reflector.getSetterType("bool");
+    Object[] param = boolean.class.equals(paramType) ? new Boolean[] { true } : new Integer[] { 1 };
+    Invoker ambiguousInvoker = reflector.getSetInvoker("bool");
+    when(ambiguousInvoker).invoke(new Bean(), param);
+    then(caughtException()).isInstanceOf(ReflectionException.class)
+        .hasMessageMatching(
+            "Ambiguous setters defined for property 'bool' in class '" + Bean.class.getName().replace("$", "\\$")
+                + "' with types '(java.lang.Integer|boolean)' and '(java.lang.Integer|boolean)'\\.");
+  }
 }
