Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #4550 XmlConfiguration argument matching #4599

Merged
merged 7 commits into from
Feb 25, 2020
Merged

Issue #4550 XmlConfiguration argument matching #4599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2073003
Issue #4550 XmlConfiguration argument matching
gregw Feb 22, 2020
f35522c
Issue #4550 XmlConfiguration argument matching
gregw Feb 22, 2020
d6843a9
Issue #4550
gregw Feb 24, 2020
3728314
Issue #4550
gregw Feb 24, 2020
626fe71
Issue #4550
gregw Feb 24, 2020
39dd245
Issue #4550
gregw Feb 25, 2020
a95e40e
Issue #4550
gregw Feb 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 80 additions & 46 deletions jetty-xml/src/main/java/org/eclipse/jetty/xml/XmlConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
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;
Expand Down Expand Up @@ -99,20 +100,47 @@ public class XmlConfiguration
};
private static final Iterable<ConfigurationProcessorFactory> __factoryLoader = ServiceLoader.load(ConfigurationProcessorFactory.class);
private static final XmlParser __parser = initParser();
public static final Comparator<Executable> EXECUTABLE_COMPARATOR = (o1, o2) ->
public static final Comparator<Executable> 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;
};
gregw marked this conversation as resolved.
Show resolved Hide resolved

Expand Down Expand Up @@ -1703,7 +1731,7 @@ Object[] applyTo(Executable executable)
{
// 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);
Expand Down Expand Up @@ -1734,47 +1762,54 @@ Object[] matchArgsToParameters(Executable executable)
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<String, Integer> 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<Object> arguments = new ArrayList<>(_arguments);
List<String> 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<String, Integer> 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<Object> arguments = new ArrayList<>(_arguments);
List<String> 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;
}
}
}
Expand All @@ -1796,9 +1831,8 @@ private static List<XmlParser.Node> getNodes(XmlParser.Node node, String element
}
}

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;
Expand Down
15 changes: 15 additions & 0 deletions jetty-xml/src/test/java/org/eclipse/jetty/xml/AnnotatedTestConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,21 @@ public void setVarArgs(String first, String... theRest)
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 <E> void call(E value)
{
this.third = String.valueOf(value);
}

public String getFirst()
{
return first;
Expand Down
11 changes: 11 additions & 0 deletions jetty-xml/src/test/java/org/eclipse/jetty/xml/TestConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ public class TestConfiguration extends HashMap<String, Object>
private Set set;
private ConstructorArgTestClass constructorArgTestClass;
public Map map;
public Double number;

public TestConfiguration()
{
Expand All @@ -65,6 +66,16 @@ public TestConfiguration(@Name("name") String n)
name = n;
}

public void setNumber(Object value)
{
testObject = value;
}

public void setNumber(double value)
{
number = value;
}

public void setTest(Object value)
{
testObject = value;
Expand Down
103 changes: 103 additions & 0 deletions jetty-xml/src/test/java/org/eclipse/jetty/xml/XmlConfigurationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,13 @@
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;
Expand All @@ -43,14 +45,17 @@
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;
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;
Expand Down Expand Up @@ -604,6 +609,72 @@ public void testNestedConstructorNamedInjectionOrdered() throws Exception
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<Method> 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(
"<Configure class=\"org.eclipse.jetty.xml.AnnotatedTestConfiguration\">" +
" <Call name=\"call\">" +
" <Arg type=\"int\">1</Arg>" +
" </Call>" +
" <Call name=\"call\">" +
" <Arg>2</Arg>" +
" </Call>" +
" <Call name=\"call\">" +
" <Arg type=\"Long\">3</Arg>" +
" </Call>" +
"</Configure>");

AnnotatedTestConfiguration atc = (AnnotatedTestConfiguration)xmlConfiguration.configure();

assertEquals("1", atc.getFirst());
assertEquals("2", atc.getSecond());
assertEquals("3", atc.getThird());
}

@Test
public void testNestedConstructorNamedInjectionUnOrdered() throws Exception
{
Expand Down Expand Up @@ -791,6 +862,38 @@ public void testCallMissingVarArgs() throws Exception
assertNull(atc.getThird());
}

public static List<String> 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(
"<Configure class=\"org.eclipse.jetty.xml.TestConfiguration\">" +
" <Call name=\"setNumber\">" +
" <Arg type=\"" + type + "\">42</Arg>" +
" </Call>" +
"</Configure>");

TestConfiguration tc = (TestConfiguration)xmlConfiguration.configure();
assertEquals(42.0D, tc.number);
}

@Test
public void testArgumentsGetIgnoredMissingDTD() throws Exception
{
Expand Down