From 353dc9bf08864c8cfd4fbf9623b0ddc88de3aaef Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Tue, 25 Feb 2020 16:19:37 +0100 Subject: [PATCH] Issue #4550 XmlConfiguration argument matching (#4599) * Issue #4550 XmlConfiguration argument matching Improve argument matching by: + rejecting obviously non matches (with allowance for unboxing) + sorting methods so that derived arguments are tried before more generic (eg String before Object) Signed-off-by: Greg Wilkins * Issue #4550 XmlConfiguration argument matching Improve argument matching by: + can unbox from any Number to any Number Signed-off-by: Greg Wilkins * Issue #4550 Do not check the assignability of the arguments. Instead rely on the order of the methods. Signed-off-by: Greg Wilkins * Issue #4550 unbox test no longer required Signed-off-by: Greg Wilkins * Issue #4550 Simplified test Signed-off-by: Greg Wilkins * Issue #4550 Cleanup comparator Signed-off-by: Greg Wilkins * Issue #4550 Cleanup comparator Signed-off-by: Greg Wilkins --- .../eclipse/jetty/xml/XmlConfiguration.java | 126 +++++++++++------- .../jetty/xml/AnnotatedTestConfiguration.java | 15 +++ .../eclipse/jetty/xml/TestConfiguration.java | 11 ++ .../jetty/xml/XmlConfigurationTest.java | 103 ++++++++++++++ 4 files changed, 209 insertions(+), 46 deletions(-) diff --git a/jetty-xml/src/main/java/org/eclipse/jetty/xml/XmlConfiguration.java b/jetty-xml/src/main/java/org/eclipse/jetty/xml/XmlConfiguration.java index f8132f7ceb6..286e28b8c39 100644 --- a/jetty-xml/src/main/java/org/eclipse/jetty/xml/XmlConfiguration.java +++ b/jetty-xml/src/main/java/org/eclipse/jetty/xml/XmlConfiguration.java @@ -29,6 +29,7 @@ import java.lang.reflect.Field; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.lang.reflect.Modifier; +import java.lang.reflect.Parameter; import java.net.InetAddress; import java.net.MalformedURLException; import java.net.URL; @@ -99,20 +100,47 @@ public class XmlConfiguration }; private static final Iterable __factoryLoader = ServiceLoader.load(ConfigurationProcessorFactory.class); private static final XmlParser __parser = initParser(); - public static final Comparator EXECUTABLE_COMPARATOR = (o1, o2) -> + public static final Comparator EXECUTABLE_COMPARATOR = (e1, e2) -> { - int p1 = o1.getParameterCount(); - int p2 = o2.getParameterCount(); - int compare = Integer.compare(p1, p2); - if (compare == 0 && p1 > 0) + // Favour methods with less parameters + int count = e1.getParameterCount(); + int compare = Integer.compare(count, e2.getParameterCount()); + if (compare == 0 && count > 0) { - boolean a1 = o1.getParameterTypes()[p1 - 1].isArray(); - boolean a2 = o2.getParameterTypes()[p2 - 1].isArray(); - if (a1 && !a2) - compare = 1; - else if (!a1 && a2) - compare = -1; + Parameter[] p1 = e1.getParameters(); + Parameter[] p2 = e2.getParameters(); + + // Favour methods without varargs + compare = Boolean.compare(p1[count - 1].isVarArgs(), p2[count - 1].isVarArgs()); + if (compare == 0) + { + // Rank by differences in the parameters + for (int i = 0; i < count; i++) + { + Class t1 = p1[i].getType(); + Class t2 = p2[i].getType(); + if (t1 != t2) + { + // Favour derived type over base type + compare = Boolean.compare(t1.isAssignableFrom(t2), t2.isAssignableFrom(t1)); + if (compare == 0) + { + // favour primitive type over reference + compare = Boolean.compare(!t1.isPrimitive(), !t2.isPrimitive()); + if (compare == 0) + // Use name to avoid non determinant sorting + compare = t1.getName().compareTo(t2.getName()); + } + + // break on the first different parameter (should always be true) + if (compare != 0) + break; + } + } + } + compare = Math.min(1, Math.max(compare, -1)); } + return compare; }; @@ -1703,7 +1731,7 @@ public class XmlConfiguration { // Could this be an empty varargs match? int count = executable.getParameterCount(); - if (count > 0 && executable.getParameterTypes()[count - 1].isArray()) + if (count > 0 && executable.getParameters()[count - 1].isVarArgs()) { // There is not a no varArgs alternative so let's try a an empty varArgs match args = asEmptyVarArgs(executable.getParameterTypes()[count - 1]).matchArgsToParameters(executable); @@ -1734,47 +1762,54 @@ public class XmlConfiguration return new Object[0]; // If no arg names are specified, keep the arg order + Object[] args; if (_names.stream().noneMatch(Objects::nonNull)) - return _arguments.toArray(new Object[0]); - - // If we don't have any parameters with names, then no match - Annotation[][] parameterAnnotations = executable.getParameterAnnotations(); - if (parameterAnnotations == null || parameterAnnotations.length == 0) - return null; - - // Find the position of all named parameters from the executable - Map position = new HashMap<>(); - int p = 0; - for (Annotation[] paramAnnotation : parameterAnnotations) { - Integer pos = p++; - Arrays.stream(paramAnnotation) - .filter(Name.class::isInstance) - .map(Name.class::cast) - .findFirst().ifPresent(n -> position.put(n.value(), pos)); + args = _arguments.toArray(new Object[0]); } - - List arguments = new ArrayList<>(_arguments); - List names = new ArrayList<>(_names); - // Map the actual arguments to the names - for (p = 0; p < count; p++) + else { - String name = names.get(p); - if (name != null) + // If we don't have any parameters with names, then no match + Annotation[][] parameterAnnotations = executable.getParameterAnnotations(); + if (parameterAnnotations == null || parameterAnnotations.length == 0) + return null; + + // Find the position of all named parameters from the executable + Map position = new HashMap<>(); + int p = 0; + for (Annotation[] paramAnnotation : parameterAnnotations) { - Integer pos = position.get(name); - if (pos == null) - return null; - if (pos != p) + Integer pos = p++; + Arrays.stream(paramAnnotation) + .filter(Name.class::isInstance) + .map(Name.class::cast) + .findFirst().ifPresent(n -> position.put(n.value(), pos)); + } + + List arguments = new ArrayList<>(_arguments); + List names = new ArrayList<>(_names); + // Map the actual arguments to the names + for (p = 0; p < count; p++) + { + String name = names.get(p); + if (name != null) { - // adjust position of parameter - arguments.add(pos, arguments.remove(p)); - names.add(pos, names.remove(p)); - p = Math.min(p, pos); + Integer pos = position.get(name); + if (pos == null) + return null; + if (pos != p) + { + // adjust position of parameter + arguments.add(pos, arguments.remove(p)); + names.add(pos, names.remove(p)); + p = Math.min(p, pos); + } } } + args = arguments.toArray(new Object[0]); } - return arguments.toArray(new Object[0]); + + return args; } } } @@ -1796,9 +1831,8 @@ public class XmlConfiguration } } - for (int i = 0; i < node.size(); i++) + for (Object o : node) { - Object o = node.get(i); if (!(o instanceof XmlParser.Node)) continue; XmlParser.Node n = (XmlParser.Node)o; diff --git a/jetty-xml/src/test/java/org/eclipse/jetty/xml/AnnotatedTestConfiguration.java b/jetty-xml/src/test/java/org/eclipse/jetty/xml/AnnotatedTestConfiguration.java index d114fc3b78a..a184d54bc15 100644 --- a/jetty-xml/src/test/java/org/eclipse/jetty/xml/AnnotatedTestConfiguration.java +++ b/jetty-xml/src/test/java/org/eclipse/jetty/xml/AnnotatedTestConfiguration.java @@ -92,6 +92,21 @@ public class AnnotatedTestConfiguration this.third = theRest.length > 1 ? theRest[1] : null; } + public void call(Integer value) + { + this.first = String.valueOf(value); + } + + public void call(String value) + { + this.second = value; + } + + public void call(E value) + { + this.third = String.valueOf(value); + } + public String getFirst() { return first; diff --git a/jetty-xml/src/test/java/org/eclipse/jetty/xml/TestConfiguration.java b/jetty-xml/src/test/java/org/eclipse/jetty/xml/TestConfiguration.java index 71e989cb5f2..bc4af813e5d 100644 --- a/jetty-xml/src/test/java/org/eclipse/jetty/xml/TestConfiguration.java +++ b/jetty-xml/src/test/java/org/eclipse/jetty/xml/TestConfiguration.java @@ -54,6 +54,7 @@ public class TestConfiguration extends HashMap private Set set; private ConstructorArgTestClass constructorArgTestClass; public Map map; + public Double number; public TestConfiguration() { @@ -65,6 +66,16 @@ public class TestConfiguration extends HashMap name = n; } + public void setNumber(Object value) + { + testObject = value; + } + + public void setNumber(double value) + { + number = value; + } + public void setTest(Object value) { testObject = value; diff --git a/jetty-xml/src/test/java/org/eclipse/jetty/xml/XmlConfigurationTest.java b/jetty-xml/src/test/java/org/eclipse/jetty/xml/XmlConfigurationTest.java index e730d35af1d..ff780086d86 100644 --- a/jetty-xml/src/test/java/org/eclipse/jetty/xml/XmlConfigurationTest.java +++ b/jetty-xml/src/test/java/org/eclipse/jetty/xml/XmlConfigurationTest.java @@ -24,11 +24,13 @@ import java.io.IOException; import java.io.PrintStream; import java.io.UnsupportedEncodingException; import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; import java.net.URL; import java.nio.file.Files; import java.nio.file.Path; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -43,7 +45,9 @@ import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; import org.eclipse.jetty.util.log.StdErrLog; import org.eclipse.jetty.util.resource.PathResource; +import org.hamcrest.Matchers; import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.RepeatedTest; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.api.extension.ExtensionContext; @@ -51,6 +55,7 @@ import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.ArgumentsProvider; import org.junit.jupiter.params.provider.ArgumentsSource; +import org.junit.jupiter.params.provider.MethodSource; import org.xml.sax.SAXException; import static java.nio.charset.StandardCharsets.UTF_8; @@ -604,6 +609,72 @@ public class XmlConfigurationTest assertEquals("arg3", atc.getNested().getThird(), "nested third parameter not wired correctly"); } + private static class TestOrder + { + public void call() + { + } + + public void call(int o) + { + } + + public void call(Object o) + { + } + + public void call(String s) + { + } + + public void call(String... ss) + { + } + + public void call(String s, String... ss) + { + } + } + + @RepeatedTest(10) + public void testMethodOrdering() throws Exception + { + List methods = Arrays.stream(TestOrder.class.getMethods()).filter(m -> "call".equals(m.getName())).collect(Collectors.toList()); + Collections.shuffle(methods); + Collections.sort(methods, XmlConfiguration.EXECUTABLE_COMPARATOR); + assertThat(methods, Matchers.contains( + TestOrder.class.getMethod("call"), + TestOrder.class.getMethod("call", int.class), + TestOrder.class.getMethod("call", String.class), + TestOrder.class.getMethod("call", Object.class), + TestOrder.class.getMethod("call", String[].class), + TestOrder.class.getMethod("call", String.class, String[].class) + )); + } + + @Test + public void testOverloadedCall() throws Exception + { + XmlConfiguration xmlConfiguration = asXmlConfiguration( + "" + + " " + + " 1" + + " " + + " " + + " 2" + + " " + + " " + + " 3" + + " " + + ""); + + AnnotatedTestConfiguration atc = (AnnotatedTestConfiguration)xmlConfiguration.configure(); + + assertEquals("1", atc.getFirst()); + assertEquals("2", atc.getSecond()); + assertEquals("3", atc.getThird()); + } + @Test public void testNestedConstructorNamedInjectionUnOrdered() throws Exception { @@ -791,6 +862,38 @@ public class XmlConfigurationTest assertNull(atc.getThird()); } + public static List typeTestData() + { + return Arrays.asList( + "byte", + "int", + "short", + "long", + "float", + "double", + "Byte", + "Integer", + "Short", + "Long", + "Float", + "Double"); + } + + @ParameterizedTest + @MethodSource("typeTestData") + public void testCallNumberConversion(String type) throws Exception + { + XmlConfiguration xmlConfiguration = asXmlConfiguration( + "" + + " " + + " 42" + + " " + + ""); + + TestConfiguration tc = (TestConfiguration)xmlConfiguration.configure(); + assertEquals(42.0D, tc.number); + } + @Test public void testArgumentsGetIgnoredMissingDTD() throws Exception {