From 3e2da33c8f062db567e7ea436650af0f2e5a42bc Mon Sep 17 00:00:00 2001 From: Alexander Vasiljev Date: Mon, 4 Jul 2016 16:44:35 +0600 Subject: [PATCH 1/5] resolving of conflicting setter is refactored no change in functionality yet, see next commits --- .../apache/ibatis/reflection/Reflector.java | 52 ++++++++++--------- 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/src/main/java/org/apache/ibatis/reflection/Reflector.java b/src/main/java/org/apache/ibatis/reflection/Reflector.java index 8975b12508f..18701878567 100644 --- a/src/main/java/org/apache/ibatis/reflection/Reflector.java +++ b/src/main/java/org/apache/ibatis/reflection/Reflector.java @@ -181,35 +181,39 @@ private void addMethodConflict(Map> conflictingMethods, Str private void resolveSetterConflicts(Map> conflictingSetters) { for (String propName : conflictingSetters.keySet()) { List setters = conflictingSetters.get(propName); - Method firstMethod = setters.get(0); if (setters.size() == 1) { + // no conflict + Method firstMethod = setters.get(0); addSetMethod(propName, firstMethod); - } else { - Class expectedType = getTypes.get(propName); - if (expectedType == null) { - throw new ReflectionException("Illegal overloaded setter method with ambiguous type for property " - + propName + " in class " + firstMethod.getDeclaringClass() + ". This breaks the JavaBeans " + - "specification and can cause unpredicatble results."); - } else { - Iterator methods = setters.iterator(); - Method setter = null; - while (methods.hasNext()) { - Method method = methods.next(); - if (method.getParameterTypes().length == 1 - && expectedType.equals(method.getParameterTypes()[0])) { - setter = method; - break; - } - } - if (setter == null) { - throw new ReflectionException("Illegal overloaded setter method with ambiguous type for property " - + propName + " in class " + firstMethod.getDeclaringClass() + ". This breaks the JavaBeans " + - "specification and can cause unpredicatble results."); - } - addSetMethod(propName, setter); + continue; // next property + } + + Class getterType = getTypes.get(propName); + if (getterType != null) { + // try to resolve conflict using property getter first + Method matchingSetter = findSetterMatchingGetter(getterType, setters); + if (matchingSetter != null) { + addSetMethod(propName, matchingSetter); + continue; // next property } } + throw new ReflectionException("Illegal overloaded setter method with ambiguous type for property " + + propName + " in class " + setters.get(0).getDeclaringClass() + ". This breaks the JavaBeans " + + "specification and can cause unpredicatble results."); + } + } + + private Method findSetterMatchingGetter(Class getterType, List setters) { + for (Method method : setters) { + if (method.getParameterTypes().length != 1) { + throw new IllegalStateException("setters are asserted to have only one argument: " + method); + } + Class argType = method.getParameterTypes()[0]; + if (getterType.equals(argType)) { + return method; + } } + return null; } private void addSetMethod(String name, Method method) { From 8feb6cf4c99b3890d5df418d96b40066ab7d1cad Mon Sep 17 00:00:00 2001 From: Alexander Vasiljev Date: Mon, 4 Jul 2016 15:03:44 +0600 Subject: [PATCH 2/5] fix reflection of write-only property in case of generic inheritance --- .../apache/ibatis/reflection/Reflector.java | 39 +++++++++++++++++-- .../ibatis/reflection/ReflectorTest.java | 31 +++++++++++++++ 2 files changed, 67 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/apache/ibatis/reflection/Reflector.java b/src/main/java/org/apache/ibatis/reflection/Reflector.java index 18701878567..39054ed7f92 100644 --- a/src/main/java/org/apache/ibatis/reflection/Reflector.java +++ b/src/main/java/org/apache/ibatis/reflection/Reflector.java @@ -197,9 +197,9 @@ private void resolveSetterConflicts(Map> conflictingSetters continue; // next property } } - throw new ReflectionException("Illegal overloaded setter method with ambiguous type for property " - + propName + " in class " + setters.get(0).getDeclaringClass() + ". This breaks the JavaBeans " + - "specification and can cause unpredicatble results."); + // assume that setter argument is elaborated from parent to child + Method narrowestSetter = findNarrowestSetter(propName, setters); + addSetMethod(propName, narrowestSetter); } } @@ -216,6 +216,39 @@ private Method findSetterMatchingGetter(Class getterType, List setter return null; } + private Method findNarrowestSetter(String propName, List setters) { + Method narrowest = null; + Class narrowestType = null; + for (Method method : setters) { + if (method.getParameterTypes().length != 1) { + throw new IllegalStateException("setters are asserted to have only one argument: " + method); + } + if (narrowest == null) { + narrowest = method; + narrowestType = method.getParameterTypes()[0]; + continue; // next conflicting setter + } + + Class argType = method.getParameterTypes()[0]; + + if (argType.equals(narrowestType)) { + throw new ReflectionException("The property " + propName + " in class " + narrowest.getDeclaringClass() + + " is referenced twice with " + narrowestType + ". This case is not supported"); + } + + if (argType.isAssignableFrom(narrowestType)) { + // OK narrowest type is descendant + } else if (narrowestType.isAssignableFrom(argType)) { + narrowest = method; + narrowestType = argType; + } else { + throw new ReflectionException("The property " + propName + " in class " + narrowest.getDeclaringClass() + + " is referenced at least twice with types " + argType + " and " + narrowestType + ". This case is not supported"); + } + } + return narrowest; + } + private void addSetMethod(String name, Method method) { if (isValidPropertyName(name)) { setMethods.put(name, new MethodInvoker(method)); diff --git a/src/test/java/org/apache/ibatis/reflection/ReflectorTest.java b/src/test/java/org/apache/ibatis/reflection/ReflectorTest.java index 9367eef6f90..fd471f20084 100644 --- a/src/test/java/org/apache/ibatis/reflection/ReflectorTest.java +++ b/src/test/java/org/apache/ibatis/reflection/ReflectorTest.java @@ -160,4 +160,35 @@ public T getFld() { static class Child extends Parent { } + + + @Test + public void shouldResolveWriteOnlyProperty() throws Exception { + ReflectorFactory reflectorFactory = new DefaultReflectorFactory(); + Reflector reflector = reflectorFactory.findForClass(Child2.class); + Assert.assertEquals(Long.class, reflector.getSetterType("id")); + } + + static class Parent2 { + T id; + + /** + * A setter for write-only property according to + * Java Beans Specification 1.01 + * http://download.oracle.com/otn-pub/jcp/7224-javabeans-1.01-fr-spec-oth-JSpec/beans.101.pdf + * Section 8.3.1 + */ + public void setId(T id) { + this.id = id; + } + } + + static class Child2 extends Parent2 { + int counter; + + public void setId(Long id) { + ++counter; + super.setId(id); + } + } } From 2be2c5bc9a6e03ebeaf735350cb6a1cb23aa2575 Mon Sep 17 00:00:00 2001 From: Alexander Vasiljev Date: Mon, 4 Jul 2016 17:14:29 +0600 Subject: [PATCH 3/5] fix static methods considered as getters or setters --- .../apache/ibatis/reflection/Reflector.java | 8 +++++ .../ibatis/reflection/ReflectorTest.java | 35 +++++++++++++++++-- 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/apache/ibatis/reflection/Reflector.java b/src/main/java/org/apache/ibatis/reflection/Reflector.java index 39054ed7f92..f414c24c2e4 100644 --- a/src/main/java/org/apache/ibatis/reflection/Reflector.java +++ b/src/main/java/org/apache/ibatis/reflection/Reflector.java @@ -97,6 +97,10 @@ private void addGetMethods(Class cls) { Map> conflictingGetters = new HashMap>(); Method[] methods = getClassMethods(cls); for (Method method : methods) { + if (Modifier.isStatic(method.getModifiers())) { + // static method is not a getter + continue; + } String name = method.getName(); if (name.startsWith("get") && name.length() > 3) { if (method.getParameterTypes().length == 0) { @@ -158,6 +162,10 @@ private void addSetMethods(Class cls) { Map> conflictingSetters = new HashMap>(); Method[] methods = getClassMethods(cls); for (Method method : methods) { + if (Modifier.isStatic(method.getModifiers())) { + // static method is not a setter + continue; + } String name = method.getName(); if (name.startsWith("set") && name.length() > 3) { if (method.getParameterTypes().length == 1) { diff --git a/src/test/java/org/apache/ibatis/reflection/ReflectorTest.java b/src/test/java/org/apache/ibatis/reflection/ReflectorTest.java index fd471f20084..0930b480fcb 100644 --- a/src/test/java/org/apache/ibatis/reflection/ReflectorTest.java +++ b/src/test/java/org/apache/ibatis/reflection/ReflectorTest.java @@ -15,14 +15,15 @@ */ package org.apache.ibatis.reflection; -import static org.junit.Assert.*; - import java.io.Serializable; import java.util.List; import org.junit.Assert; import org.junit.Test; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + public class ReflectorTest { @Test @@ -191,4 +192,34 @@ public void setId(Long id) { super.setId(id); } } + + @Test + public void shouldHaveNoStaticGetters() throws Exception { + ReflectorFactory reflectorFactory = new DefaultReflectorFactory(); + Reflector reflector = reflectorFactory.findForClass(Anxious.class); + Assert.assertFalse(reflector.hasGetter("thereAnybodyOutThere")); + } + + static abstract class Anxious { + private static boolean any; + + static boolean isThereAnybodyOutThere() { + return any; + } + } + + @Test + public void shouldHaveNoStaticSetters() throws Exception { + ReflectorFactory reflectorFactory = new DefaultReflectorFactory(); + Reflector reflector = reflectorFactory.findForClass(Happy.class); + Assert.assertFalse(reflector.hasSetter("upset")); + } + + static abstract class Happy { + private static String focus; + + static void setUpset(String reason) { + focus = reason; + } + } } From d7d13affa37780a2fd99a45e439704998c19ee32 Mon Sep 17 00:00:00 2001 From: Alexander Vasiljev Date: Mon, 4 Jul 2016 10:29:02 +0600 Subject: [PATCH 4/5] Make Reflector final Though the class was exposed in public API as a return value of ReflectorFactory.findForClass(...), it is not intended to be public. This class is used in the most frequently changed part of MyBatis, so it should not be inheritable in application code. --- src/main/java/org/apache/ibatis/reflection/Reflector.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/apache/ibatis/reflection/Reflector.java b/src/main/java/org/apache/ibatis/reflection/Reflector.java index f414c24c2e4..299de0b0c2a 100644 --- a/src/main/java/org/apache/ibatis/reflection/Reflector.java +++ b/src/main/java/org/apache/ibatis/reflection/Reflector.java @@ -42,9 +42,13 @@ * This class represents a cached set of class definition information that * allows for easy mapping between property names and getter/setter methods. * + * Though the class was exposed in public API as a return value of {@link ReflectorFactory#findForClass(Class)}, + * it is not intended to be public. This class is used in the most frequently changed part of MyBatis, + * so it should not be inheritable in application code. + * * @author Clinton Begin */ -public class Reflector { +public final class Reflector { private static final String[] EMPTY_STRING_ARRAY = new String[0]; From cfb888e6ab9482153b07552da9f427d8bc5737cb Mon Sep 17 00:00:00 2001 From: Alexander Vasiljev Date: Mon, 4 Jul 2016 16:43:42 +0600 Subject: [PATCH 5/5] Allow to use property getters and setters without get- or set- prefix to serialize/deserialize DB data --- .../apache/ibatis/reflection/Reflector.java | 60 ++++++++++++++----- 1 file changed, 44 insertions(+), 16 deletions(-) diff --git a/src/main/java/org/apache/ibatis/reflection/Reflector.java b/src/main/java/org/apache/ibatis/reflection/Reflector.java index 299de0b0c2a..f54788d5106 100644 --- a/src/main/java/org/apache/ibatis/reflection/Reflector.java +++ b/src/main/java/org/apache/ibatis/reflection/Reflector.java @@ -27,10 +27,12 @@ import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; +import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.Set; import org.apache.ibatis.reflection.invoker.GetFieldInvoker; import org.apache.ibatis.reflection.invoker.Invoker; @@ -38,6 +40,8 @@ import org.apache.ibatis.reflection.invoker.SetFieldInvoker; import org.apache.ibatis.reflection.property.PropertyNamer; +import static java.util.Arrays.asList; + /** * This class represents a cached set of class definition information that * allows for easy mapping between property names and getter/setter methods. @@ -51,6 +55,9 @@ public final class Reflector { private static final String[] EMPTY_STRING_ARRAY = new String[0]; + private static final Set NON_ACCESSORS = new HashSet(asList( + "toString", "hashCode", "clone", "notify", "notifyAll", "wait", "finalize", "serialVersionUID", "getClass", "equals", "wait" + )); private Class type; private String[] readablePropertyNames = EMPTY_STRING_ARRAY; @@ -101,22 +108,29 @@ private void addGetMethods(Class cls) { Map> conflictingGetters = new HashMap>(); Method[] methods = getClassMethods(cls); for (Method method : methods) { + if (method.getParameterTypes().length > 0) { + // method with any arguments is not a getter + continue; + } + if (Modifier.isStatic(method.getModifiers())) { // static method is not a getter continue; } + String name = method.getName(); + if (NON_ACCESSORS.contains(name)) { + continue; + } + + // remove prefix from property name if exists if (name.startsWith("get") && name.length() > 3) { - if (method.getParameterTypes().length == 0) { - name = PropertyNamer.methodToProperty(name); - addMethodConflict(conflictingGetters, name, method); - } + name = PropertyNamer.methodToProperty(name); } else if (name.startsWith("is") && name.length() > 2) { - if (method.getParameterTypes().length == 0) { - name = PropertyNamer.methodToProperty(name); - addMethodConflict(conflictingGetters, name, method); - } + name = PropertyNamer.methodToProperty(name); } + + addMethodConflict(conflictingGetters, name, method); } resolveGetterConflicts(conflictingGetters); } @@ -166,17 +180,29 @@ private void addSetMethods(Class cls) { Map> conflictingSetters = new HashMap>(); Method[] methods = getClassMethods(cls); for (Method method : methods) { + if (method.getParameterTypes().length != 1) { + // Setter got single argument. The method is not a setter + continue; + } + if (Modifier.isStatic(method.getModifiers())) { // static method is not a setter continue; } + String name = method.getName(); + + if (NON_ACCESSORS.contains(name)) { + // method defined in java.lang.Object is not a setter + continue; + } + + // remove prefix from property name if exists if (name.startsWith("set") && name.length() > 3) { - if (method.getParameterTypes().length == 1) { - name = PropertyNamer.methodToProperty(name); - addMethodConflict(conflictingSetters, name, method); - } + name = PropertyNamer.methodToProperty(name); } + + addMethodConflict(conflictingSetters, name, method); } resolveSetterConflicts(conflictingSetters); } @@ -211,7 +237,10 @@ private void resolveSetterConflicts(Map> conflictingSetters } // assume that setter argument is elaborated from parent to child Method narrowestSetter = findNarrowestSetter(propName, setters); - addSetMethod(propName, narrowestSetter); + if (narrowestSetter != null) { + addSetMethod(propName, narrowestSetter); + } + // Unable to resolve conflict. Skip this property } } @@ -254,8 +283,7 @@ private Method findNarrowestSetter(String propName, List setters) { narrowest = method; narrowestType = argType; } else { - throw new ReflectionException("The property " + propName + " in class " + narrowest.getDeclaringClass() - + " is referenced at least twice with types " + argType + " and " + narrowestType + ". This case is not supported"); + return null; } } return narrowest; @@ -337,7 +365,7 @@ private void addGetField(Field field) { } private boolean isValidPropertyName(String name) { - return !(name.startsWith("$") || "serialVersionUID".equals(name) || "class".equals(name)); + return !(name.startsWith("$") || "serialVersionUID".equals(name)); } /*