From b0c72dfe3eff8e1ad42706393f5ab0a6de2a1884 Mon Sep 17 00:00:00 2001 From: Martin Vysny Date: Mon, 21 Jan 2019 21:01:18 +0200 Subject: [PATCH 1/4] Issue #314: add test --- .../sql2o/reflection/PojoMetadataTest.java | 96 +++++++++++++++++++ 1 file changed, 96 insertions(+) create mode 100644 core/src/test/java/org/sql2o/reflection/PojoMetadataTest.java diff --git a/core/src/test/java/org/sql2o/reflection/PojoMetadataTest.java b/core/src/test/java/org/sql2o/reflection/PojoMetadataTest.java new file mode 100644 index 00000000..f3cdcc42 --- /dev/null +++ b/core/src/test/java/org/sql2o/reflection/PojoMetadataTest.java @@ -0,0 +1,96 @@ +package org.sql2o.reflection; + +import javassist.*; +import org.junit.AssumptionViolatedException; +import org.junit.Test; + +import java.util.HashMap; +import java.util.UUID; + +import static org.junit.Assert.*; + +/** + * Tests the {@link PojoMetadata} class. + * @author mavi + */ +public class PojoMetadataTest { + /** + * Tests that when there are two getters with different return types, the one with more concrete type is preferred. + * See https://github.com/aaberg/sql2o/issues/314 for more details. + */ + @Test + public void testPrefersGetterWithMoreConcreteType() throws Exception { + Class clazz = createClassWithGetters(false); + if (clazz.getDeclaredMethods()[0].getReturnType() == Object.class) { + // the method order is undefined :( We need to have two methods, first one returning String, second one + // returning Object. The old PojoMetadata would incorrectly use the latter; the fixed version would use the + // function returning more concrete result. + // + // try to create the class with reversed method ordering, maybe it helps? + clazz = createClassWithGetters(true); + if (clazz.getDeclaredMethods()[0].getReturnType() == Object.class) { + // nah, didn't help. bail out. + throw new AssumptionViolatedException("Can't enforce method order"); + } + } + + final PojoMetadata metadata = new PojoMetadata(clazz, false, false, new HashMap(), true); + assertEquals(String.class, metadata.getPropertyGetter("id").getType()); + } + + /** + * Tests that when there are two setters with different return types, the one with more concrete type is preferred. + * See https://github.com/aaberg/sql2o/issues/314 for more details. + */ + @Test + public void testPrefersSetterWithMoreConcreteType() throws Exception { + Class clazz = createClassWithSetters(false); + if (clazz.getDeclaredMethods()[0].getParameterTypes()[0] == Object.class) { + // the method order is undefined :( We need to have two methods, first one returning String, second one + // returning Object. The old PojoMetadata would incorrectly use the latter; the fixed version would use the + // function returning more concrete result. + // + // try to create the class with reversed method ordering, maybe it helps? + clazz = createClassWithSetters(true); + if (clazz.getDeclaredMethods()[0].getParameterTypes()[0] == Object.class) { + // nah, didn't help. bail out. + throw new AssumptionViolatedException("Can't enforce method order"); + } + } + + final PojoMetadata metadata = new PojoMetadata(clazz, false, false, new HashMap(), true); + assertEquals(String.class, metadata.getPropertySetter("id").getType()); + } + + private Class createClassWithGetters(boolean objectThenString) throws CannotCompileException, NotFoundException { + final ClassPool pool = ClassPool.getDefault(); + final CtClass ctClass = pool.makeClass("my.test.Klass" + UUID.randomUUID().toString().replace("-", "")); + ctClass.addField(new CtField(pool.get("java.lang.String"), "id", ctClass)); + if (objectThenString) { + ctClass.addMethod(CtNewMethod.make("public Object getId() { return id; }", ctClass)); + ctClass.addMethod(CtNewMethod.make("public String getId() { return id; }", ctClass)); + } else { + ctClass.addMethod(CtNewMethod.make("public String getId() { return id; }", ctClass)); + ctClass.addMethod(CtNewMethod.make("public Object getId() { return id; }", ctClass)); + } + final Class clazz = ctClass.toClass(); + assertEquals(2, clazz.getDeclaredMethods().length); + return clazz; + } + + private Class createClassWithSetters(boolean objectThenString) throws CannotCompileException, NotFoundException { + final ClassPool pool = ClassPool.getDefault(); + final CtClass ctClass = pool.makeClass("my.test.Klass" + UUID.randomUUID().toString().replace("-", "")); + ctClass.addField(new CtField(pool.get("java.lang.Object"), "id", ctClass)); + if (objectThenString) { + ctClass.addMethod(CtNewMethod.make("public void setId(Object id) { this.id = id; }", ctClass)); + ctClass.addMethod(CtNewMethod.make("public void setId(String id) { this.id = id; }", ctClass)); + } else { + ctClass.addMethod(CtNewMethod.make("public void setId(String id) { this.id = id; }", ctClass)); + ctClass.addMethod(CtNewMethod.make("public void setId(Object id) { this.id = id; }", ctClass)); + } + final Class clazz = ctClass.toClass(); + assertEquals(2, clazz.getDeclaredMethods().length); + return clazz; + } +} From 379e7c1f87bfa59bc184e886de077f778283432d Mon Sep 17 00:00:00 2001 From: Martin Vysny Date: Mon, 21 Jan 2019 21:06:22 +0200 Subject: [PATCH 2/4] Issue #314: fix PojoMetadata to prefer more concrete getter/setter --- .../java/org/sql2o/reflection/PojoMetadata.java | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/sql2o/reflection/PojoMetadata.java b/core/src/main/java/org/sql2o/reflection/PojoMetadata.java index f67c6b88..16755f60 100644 --- a/core/src/main/java/org/sql2o/reflection/PojoMetadata.java +++ b/core/src/main/java/org/sql2o/reflection/PojoMetadata.java @@ -124,7 +124,13 @@ private PropertyAndFieldInfo initializePropertyInfo() { propertyName = propertyName.toLowerCase(); } - propertyGetters.put(propertyName, factoryFacade.newGetter(m)); + final Getter existingGetter = propertyGetters.get(propertyName); + if (existingGetter != null && existingGetter.getType() != Object.class && m.getReturnType() == Object.class) { + // don't overwrite existing getter if it has more concrete type. See https://github.com/aaberg/sql2o/issues/314 + // for more details. + } else { + propertyGetters.put(propertyName, factoryFacade.newGetter(m)); + } } if (m.getName().startsWith("set") && m.getParameterTypes().length == 1) { @@ -138,7 +144,13 @@ private PropertyAndFieldInfo initializePropertyInfo() { propertyName = propertyName.toLowerCase(); } - propertySetters.put(propertyName, factoryFacade.newSetter(m)); + final Setter existingSetter = propertySetters.get(propertyName); + if (existingSetter != null && existingSetter.getType() != Object.class && m.getParameterTypes()[0] == Object.class) { + // don't overwrite existing getter if it has more concrete type. See https://github.com/aaberg/sql2o/issues/314 + // for more details. + } else { + propertySetters.put(propertyName, factoryFacade.newSetter(m)); + } } } theClass = theClass.getSuperclass(); From 130cb1fdb636ceccef99f3234f42579f1aa1a0c1 Mon Sep 17 00:00:00 2001 From: Martin Vysny Date: Mon, 21 Jan 2019 21:10:14 +0200 Subject: [PATCH 3/4] minor --- core/src/main/java/org/sql2o/reflection/PojoMetadata.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/org/sql2o/reflection/PojoMetadata.java b/core/src/main/java/org/sql2o/reflection/PojoMetadata.java index 16755f60..4996a643 100644 --- a/core/src/main/java/org/sql2o/reflection/PojoMetadata.java +++ b/core/src/main/java/org/sql2o/reflection/PojoMetadata.java @@ -146,7 +146,7 @@ private PropertyAndFieldInfo initializePropertyInfo() { final Setter existingSetter = propertySetters.get(propertyName); if (existingSetter != null && existingSetter.getType() != Object.class && m.getParameterTypes()[0] == Object.class) { - // don't overwrite existing getter if it has more concrete type. See https://github.com/aaberg/sql2o/issues/314 + // don't overwrite existing setter if it has more concrete type. See https://github.com/aaberg/sql2o/issues/314 // for more details. } else { propertySetters.put(propertyName, factoryFacade.newSetter(m)); From 95a4c927e26301aa37feb2a7f695ec47773c19c9 Mon Sep 17 00:00:00 2001 From: Martin Vysny Date: Mon, 28 Jan 2019 08:48:27 +0200 Subject: [PATCH 4/4] #314: Simplified the code to simply skip over bridge methods --- .../org/sql2o/reflection/PojoMetadata.java | 22 ++----- .../sql2o/reflection/PojoMetadataTest.java | 58 +++++-------------- 2 files changed, 19 insertions(+), 61 deletions(-) diff --git a/core/src/main/java/org/sql2o/reflection/PojoMetadata.java b/core/src/main/java/org/sql2o/reflection/PojoMetadata.java index 4996a643..364f9df7 100644 --- a/core/src/main/java/org/sql2o/reflection/PojoMetadata.java +++ b/core/src/main/java/org/sql2o/reflection/PojoMetadata.java @@ -115,6 +115,10 @@ private PropertyAndFieldInfo initializePropertyInfo() { // prepare methods. Methods will override fields, if both exists. for (Method m : theClass.getDeclaredMethods()) { + if (m.isBridge()) { + // skip bridge methods: https://github.com/aaberg/sql2o/issues/314 + continue; + } if (m.getName().startsWith("get")) { String propertyName = m.getName().substring(3); @@ -123,14 +127,7 @@ private PropertyAndFieldInfo initializePropertyInfo() { } else { propertyName = propertyName.toLowerCase(); } - - final Getter existingGetter = propertyGetters.get(propertyName); - if (existingGetter != null && existingGetter.getType() != Object.class && m.getReturnType() == Object.class) { - // don't overwrite existing getter if it has more concrete type. See https://github.com/aaberg/sql2o/issues/314 - // for more details. - } else { - propertyGetters.put(propertyName, factoryFacade.newGetter(m)); - } + propertyGetters.put(propertyName, factoryFacade.newGetter(m)); } if (m.getName().startsWith("set") && m.getParameterTypes().length == 1) { @@ -143,14 +140,7 @@ private PropertyAndFieldInfo initializePropertyInfo() { } else { propertyName = propertyName.toLowerCase(); } - - final Setter existingSetter = propertySetters.get(propertyName); - if (existingSetter != null && existingSetter.getType() != Object.class && m.getParameterTypes()[0] == Object.class) { - // don't overwrite existing setter if it has more concrete type. See https://github.com/aaberg/sql2o/issues/314 - // for more details. - } else { - propertySetters.put(propertyName, factoryFacade.newSetter(m)); - } + propertySetters.put(propertyName, factoryFacade.newSetter(m)); } } theClass = theClass.getSuperclass(); diff --git a/core/src/test/java/org/sql2o/reflection/PojoMetadataTest.java b/core/src/test/java/org/sql2o/reflection/PojoMetadataTest.java index f3cdcc42..50bda532 100644 --- a/core/src/test/java/org/sql2o/reflection/PojoMetadataTest.java +++ b/core/src/test/java/org/sql2o/reflection/PojoMetadataTest.java @@ -1,7 +1,6 @@ package org.sql2o.reflection; import javassist.*; -import org.junit.AssumptionViolatedException; import org.junit.Test; import java.util.HashMap; @@ -20,20 +19,7 @@ public class PojoMetadataTest { */ @Test public void testPrefersGetterWithMoreConcreteType() throws Exception { - Class clazz = createClassWithGetters(false); - if (clazz.getDeclaredMethods()[0].getReturnType() == Object.class) { - // the method order is undefined :( We need to have two methods, first one returning String, second one - // returning Object. The old PojoMetadata would incorrectly use the latter; the fixed version would use the - // function returning more concrete result. - // - // try to create the class with reversed method ordering, maybe it helps? - clazz = createClassWithGetters(true); - if (clazz.getDeclaredMethods()[0].getReturnType() == Object.class) { - // nah, didn't help. bail out. - throw new AssumptionViolatedException("Can't enforce method order"); - } - } - + Class clazz = createClassWithGetters(); final PojoMetadata metadata = new PojoMetadata(clazz, false, false, new HashMap(), true); assertEquals(String.class, metadata.getPropertyGetter("id").getType()); } @@ -44,53 +30,35 @@ public void testPrefersGetterWithMoreConcreteType() throws Exception { */ @Test public void testPrefersSetterWithMoreConcreteType() throws Exception { - Class clazz = createClassWithSetters(false); - if (clazz.getDeclaredMethods()[0].getParameterTypes()[0] == Object.class) { - // the method order is undefined :( We need to have two methods, first one returning String, second one - // returning Object. The old PojoMetadata would incorrectly use the latter; the fixed version would use the - // function returning more concrete result. - // - // try to create the class with reversed method ordering, maybe it helps? - clazz = createClassWithSetters(true); - if (clazz.getDeclaredMethods()[0].getParameterTypes()[0] == Object.class) { - // nah, didn't help. bail out. - throw new AssumptionViolatedException("Can't enforce method order"); - } - } - + Class clazz = createClassWithSetters(); final PojoMetadata metadata = new PojoMetadata(clazz, false, false, new HashMap(), true); assertEquals(String.class, metadata.getPropertySetter("id").getType()); } - private Class createClassWithGetters(boolean objectThenString) throws CannotCompileException, NotFoundException { + private Class createClassWithGetters() throws CannotCompileException, NotFoundException { final ClassPool pool = ClassPool.getDefault(); final CtClass ctClass = pool.makeClass("my.test.Klass" + UUID.randomUUID().toString().replace("-", "")); ctClass.addField(new CtField(pool.get("java.lang.String"), "id", ctClass)); - if (objectThenString) { - ctClass.addMethod(CtNewMethod.make("public Object getId() { return id; }", ctClass)); - ctClass.addMethod(CtNewMethod.make("public String getId() { return id; }", ctClass)); - } else { - ctClass.addMethod(CtNewMethod.make("public String getId() { return id; }", ctClass)); - ctClass.addMethod(CtNewMethod.make("public Object getId() { return id; }", ctClass)); - } + ctClass.addMethod(withBridge(CtNewMethod.make("public Object getId() { return id; }", ctClass))); + ctClass.addMethod(CtNewMethod.make("public String getId() { return id; }", ctClass)); final Class clazz = ctClass.toClass(); assertEquals(2, clazz.getDeclaredMethods().length); return clazz; } - private Class createClassWithSetters(boolean objectThenString) throws CannotCompileException, NotFoundException { + private Class createClassWithSetters() throws CannotCompileException, NotFoundException { final ClassPool pool = ClassPool.getDefault(); final CtClass ctClass = pool.makeClass("my.test.Klass" + UUID.randomUUID().toString().replace("-", "")); ctClass.addField(new CtField(pool.get("java.lang.Object"), "id", ctClass)); - if (objectThenString) { - ctClass.addMethod(CtNewMethod.make("public void setId(Object id) { this.id = id; }", ctClass)); - ctClass.addMethod(CtNewMethod.make("public void setId(String id) { this.id = id; }", ctClass)); - } else { - ctClass.addMethod(CtNewMethod.make("public void setId(String id) { this.id = id; }", ctClass)); - ctClass.addMethod(CtNewMethod.make("public void setId(Object id) { this.id = id; }", ctClass)); - } + ctClass.addMethod(withBridge(CtNewMethod.make("public void setId(Object id) { this.id = id; }", ctClass))); + ctClass.addMethod(CtNewMethod.make("public void setId(String id) { this.id = id; }", ctClass)); final Class clazz = ctClass.toClass(); assertEquals(2, clazz.getDeclaredMethods().length); return clazz; } + + private static CtMethod withBridge(CtMethod method) { + method.setModifiers(method.getModifiers() | 0x00000040); // Modifier.BRIDGE field not public :( + return method; + } }