Skip to content

Commit

Permalink
Issue #4550 XmlConfiguration argument matching
Browse files Browse the repository at this point in the history 
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 <gregw@webtide.com>
  • Loading branch information
@gregw
gregw committed Feb 22, 2020
1 parent 8c7861d commit 2073003
Show file tree
Hide file tree
Showing 4 changed files with 188 additions and 42 deletions.
27 changes: 27 additions & 0 deletions jetty-util/src/main/java/org/eclipse/jetty/util/TypeUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,22 @@ public class TypeUtil

private static final HashMap<String, Class<?>> name2Class = new HashMap<>();

private static final Map<Class<?>, Class<?>> __unbox;

static
{
Map<Class<?>, Class<?>> unbox = new HashMap<>();
unbox.put(int.class, Integer.class);
unbox.put(long.class, Long.class);
unbox.put(byte.class, Byte.class);
unbox.put(char.class, Character.class);
unbox.put(float.class, Float.class);
unbox.put(double.class, Double.class);
unbox.put(short.class, Long.class);
unbox.put(boolean.class, Boolean.class);
__unbox = Collections.unmodifiableMap(unbox);
}

static
{
name2Class.put("boolean", java.lang.Boolean.TYPE);
Expand Down Expand Up @@ -727,4 +743,15 @@ public static URI getModuleLocation(Class<?> clazz)
}
return null;
}

public static boolean isUnboxable(Class<?> type, Object arg)
{
if (!type.isPrimitive())
return false;
if (arg == null)
return true;

Class<?> c = __unbox.get(type);
return arg.getClass() == __unbox.get(type);
}
}
123 changes: 81 additions & 42 deletions jetty-xml/src/main/java/org/eclipse/jetty/xml/XmlConfiguration.java
View file
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,41 @@ 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)
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)
Parameter[] p1 = e1.getParameters();
Parameter[] p2 = e2.getParameters();
boolean va1 = p1[count - 1].isVarArgs();
boolean va2 = p2[count - 1].isVarArgs();
if (va1 && !va2)
compare = 1;
else if (!a1 && a2)
else if (!va1 && va2)
compare = -1;
else
{
// Rank by assignability of the arguments
// This is only a rough guide that will favour call(String) vs call(Object),
// but it will not resolve call(String, Object) vs call (Object, String)
for (int i = 0; i < count; i++)
{
Class<?> t1 = p1[i].getType();
Class<?> t2 = p2[i].getType();
if (t1 != t2)
{
if (t1.isAssignableFrom(t2))
compare++;
else if (t2.isAssignableFrom(t1))
compare--;
}
}
}
compare = Math.min(1, Math.max(compare, -1));
}

return compare;
};

Expand Down Expand Up @@ -1703,7 +1725,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 +1756,64 @@ 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]);

// Check assignable
Parameter[] params = executable.getParameters();
for (int i = 0; i < args.length; i++)
{
if (args[i] != null &&
!params[i].getType().isAssignableFrom(args[i].getClass()) &&
!TypeUtil.isUnboxable(params[i].getType(), args[i]))
return null;
}

return args;
}
}
}
Expand Down
15 changes: 15 additions & 0 deletions jetty-xml/src/test/java/org/eclipse/jetty/xml/AnnotatedTestConfiguration.java
View file
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
65 changes: 65 additions & 0 deletions jetty-xml/src/test/java/org/eclipse/jetty/xml/XmlConfigurationTest.java
View file
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,7 +45,9 @@
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;
Expand Down Expand Up @@ -604,6 +608,67 @@ 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(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", 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

0 comments on commit 2073003

Please sign in to comment.