From 078d44e2323e2a71a0fd7ef1743fbdd7d4ef6d3d Mon Sep 17 00:00:00 2001 From: Trever Shick Date: Sat, 22 Oct 2016 22:40:49 -0400 Subject: [PATCH] Fix method resolution with overloaded methods (#147) Fixes DiUS/java-faker#143 There was an intermittent test failure where the error message would say : Expected: is "Unable to coerce x to Long via Long(String) constructor." but: was "Unable to coerce x to Integer via Integer(String) constructor." The issue is getMethod() returning the methods in a non-deterministic order. Fixing this could have been easy by simply changing the check to look for Long or Integer but this would hide a real issue where by methodName(int) called via methodName(Long.MAX_VALUE) would die even if methodName(long) existed because it would attempt to coerce the arguments to int ONLY and ignore the long variation. I altered the logic to attempt to coerce the arguments at the same time the method is located by name. This means that methodName(Long.MAX_VALUE) will skip over methodName(int) because it would fail and will continue to find methodName(int). --- .../javafaker/service/FakeValuesService.java | 80 ++++++++++++------- .../service/FakeValuesServiceTest.java | 6 +- 2 files changed, 55 insertions(+), 31 deletions(-) diff --git a/src/main/java/com/github/javafaker/service/FakeValuesService.java b/src/main/java/com/github/javafaker/service/FakeValuesService.java index 7e840caa..e73ff1e5 100644 --- a/src/main/java/com/github/javafaker/service/FakeValuesService.java +++ b/src/main/java/com/github/javafaker/service/FakeValuesService.java @@ -9,6 +9,7 @@ import org.yaml.snakeyaml.Yaml; import java.io.InputStream; import java.lang.reflect.Constructor; +import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.util.*; import java.util.logging.Level; @@ -430,20 +431,10 @@ public class FakeValuesService { return null; } try { - Method accessor = accessor(obj, directive, args); - if (accessor == null) { - return null; - } - // coerce the string arguments into the correct argument types - List coerced = null; - try { - coerced = coerceArguments(accessor, args); - } catch (RuntimeException re) { - log.log(Level.FINE, "Unable to coerce arguments : " + re.getMessage()); - } - return (accessor == null || coerced == null) + final MethodAndCoercedArgs accessor = accessor(obj, directive, args); + return (accessor == null) ? null - : string(accessor.invoke(obj, coerced.toArray())); + : string(accessor.invoke(obj)); } catch (Exception e) { log.log(Level.FINE, "Can't call " + directive + " on " + obj, e); return null; @@ -460,23 +451,20 @@ public class FakeValuesService { try { String fakerMethodName = classAndMethod[0].replaceAll("_", ""); - Method fakerAccessor = accessor(faker, fakerMethodName, Collections.emptyList()); + MethodAndCoercedArgs fakerAccessor = accessor(faker, fakerMethodName, Collections.emptyList()); if (fakerAccessor == null) { throw new RuntimeException("Can't find top level faker object named " + fakerMethodName + "."); } Object objectWithMethodToInvoke = fakerAccessor.invoke(faker); String nestedMethodName = classAndMethod[1].replaceAll("_", ""); - final Method accessor = accessor(objectWithMethodToInvoke, classAndMethod[1].replaceAll("_", ""), args); + final MethodAndCoercedArgs accessor = accessor(objectWithMethodToInvoke, classAndMethod[1].replaceAll("_", ""), args); if (accessor == null) { throw new RuntimeException("Can't find method on " + objectWithMethodToInvoke.getClass().getSimpleName() + " called " + nestedMethodName + "."); } - final List coerced = coerceArguments(accessor, args); - - Object ret = accessor.invoke(objectWithMethodToInvoke, coerced.toArray()); - return ret == null ? null : ret.toString(); + return string(accessor.invoke(objectWithMethodToInvoke)); } catch (Exception e) { if (e instanceof RuntimeException) { throw (RuntimeException) e; @@ -489,19 +477,23 @@ public class FakeValuesService { /** * Find an accessor by name ignoring case. */ - private Method accessor(Object onObject, String name, List args) { + private MethodAndCoercedArgs accessor(Object onObject, String name, List args) { log.log(Level.FINE, "Find accessor named " + name + " on " + onObject.getClass().getSimpleName() + " with args " + args); - Method fakerAccessor = null; + for (Method m : onObject.getClass().getMethods()) { - if (m.getName().equalsIgnoreCase(name) && m.getParameterTypes().length == args.size()) { - fakerAccessor = m; - break; + if (m.getName().equalsIgnoreCase(name) + && m.getParameterTypes().length == args.size()) { + final List coercedArguments = coerceArguments(m, args); + if (coercedArguments != null) { + return new MethodAndCoercedArgs(m, coercedArguments); + } } } - if (fakerAccessor == null && name.contains("_")) { - fakerAccessor = accessor(onObject, name.replaceAll("_", ""), args); + + if (name.contains("_")) { + return accessor(onObject, name.replaceAll("_", ""), args); } - return fakerAccessor; + return null; } /** @@ -521,7 +513,8 @@ public class FakeValuesService { coerced.add(coercedArgument); } catch (Exception e) { - throw new RuntimeException("Unable to coerce " + args.get(i) + " to " + toType.getSimpleName() + " via " + toType.getSimpleName() + "(String) constructor."); + log.fine("Unable to coerce " + args.get(i) + " to " + toType.getSimpleName() + " via " + toType.getSimpleName() + "(String) constructor."); + return null; } } return coerced; @@ -530,4 +523,35 @@ public class FakeValuesService { private String string(Object obj) { return (obj == null) ? null : obj.toString(); } + + /** + * simple wrapper class around an accessor and a list of coerced arguments. + * this is useful as we get to find the method and coerce the arguments in one + * shot, returning both when successful. This saves us from doing it more than once (coercing args). + */ + private class MethodAndCoercedArgs { + + private final Method method; + + private final List coerced; + + private MethodAndCoercedArgs(Method m, List coerced) { + this.method = requireNonNull(m, "method cannot be null"); + this.coerced = requireNonNull(coerced, "coerced arguments cannot be null"); + } + + private Object invoke(Object on) throws InvocationTargetException, IllegalAccessException { + return method.invoke(on, coerced.toArray()); + } + + /** + * source level precludes me from using Objects.requireNonNull + */ + private T requireNonNull(T instance, String messageIfNull) { + if (instance == null) { + throw new NullPointerException(messageIfNull); + } + return instance; + } + } } diff --git a/src/test/java/com/github/javafaker/service/FakeValuesServiceTest.java b/src/test/java/com/github/javafaker/service/FakeValuesServiceTest.java index f75743ee..6d500831 100644 --- a/src/test/java/com/github/javafaker/service/FakeValuesServiceTest.java +++ b/src/test/java/com/github/javafaker/service/FakeValuesServiceTest.java @@ -226,12 +226,12 @@ public class FakeValuesServiceTest extends AbstractFakerTest { * if the message changes, it's ok to update the test provided * the two conditions above are still true. */ - @Test/*(expected = RuntimeException.class)*/ + @Test public void expressionWithValidFakerObjectValidMethodInvalidArgs() { expressionShouldFailWith("#{Number.number_between 'x','y'}", - "Unable to coerce x to Long via Long(String) constructor."); + "Can't find method on Number called numberbetween."); } - + /** * Two things are important here: * 1) the message in the exception should be USEFUL