Skip to content

Commit

Permalink
Jetty 12.0.x xmlconfig prefers interfaces (#11901)
Browse files Browse the repository at this point in the history 
* Change XmlConfiguration to prefer classes over interfaces
  • Loading branch information
@janbartel
janbartel authored Oct 31, 2024
1 parent dffb065 commit 0d3a4f2
Show file tree
Hide file tree
Showing 4 changed files with 163 additions and 19 deletions.
30 changes: 28 additions & 2 deletions jetty-core/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 @@ -110,6 +110,32 @@ public class XmlConfiguration
.toList();
private static final Pool<ConfigurationParser> __parsers =
new ConcurrentPool<>(ConcurrentPool.StrategyType.THREAD_ID, Math.min(8, Runtime.getRuntime().availableProcessors()));

/**
* Order two classes to prefer a non Object type as the highest ranking, followed by
* an interface type. Where both are the Object type, or both are an interface they
* are considered equal.
*
* @param t1 the first class to compare
* @param t2 the second class to compare
* @return 1 if t1 is a non Object class or an interface type, return 0 if both are
* interfaces or the Object class, return -1 if only t2 is the Object class or an
* interface.
*/
private static int compare(Class<?> t1, Class<?> t2)
{
if (t1 == Object.class)
{
if (t2 == Object.class)
return 0;
return 1;
}
if (t2 == Object.class)
return -1;

return Boolean.compare(t1.isInterface(), t2.isInterface());
}

public static final Comparator<Executable> EXECUTABLE_COMPARATOR = (e1, e2) ->
{
// Favour methods with less parameters
Expand All @@ -135,8 +161,8 @@ public class XmlConfiguration
compare = Boolean.compare(t2.isPrimitive(), t1.isPrimitive());
if (compare == 0)
{
// prefer interfaces
compare = Boolean.compare(t2.isInterface(), t1.isInterface());
// prefer (non Object.class) classes over interfaces
compare = compare(t1, t2);

if (compare == 0)
{
Expand Down
24 changes: 24 additions & 0 deletions jetty-core/jetty-xml/src/test/java/org/eclipse/jetty/xml/ExampleConfiguration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ public class ExampleConfiguration extends HashMap<String, Object>
public int testInt;
public URL url;
public static boolean called = false;

public static boolean calledWithClass = false;

public Object[] oa;
public int[] ia;
public int testField1;
Expand All @@ -49,6 +52,16 @@ public class ExampleConfiguration extends HashMap<String, Object>
public Map map;
public Double number;

public interface TestInterface
{

}

public static class TestImpl implements TestInterface
{

}

public ExampleConfiguration()
{
this("");
Expand Down Expand Up @@ -131,6 +144,17 @@ public static void callStatic()
called = true;
}

public static void callStaticWithVarArgs(TestInterface i, String... strings)
{
called = true;
}

public static void callStaticWithVarArgs(TestImpl i, String... strings)
{
called = true;
calledWithClass = true;
}

public void call(Object[] oa)
{
this.oa = oa;
Expand Down
115 changes: 98 additions & 17 deletions jetty-core/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 @@ -218,6 +218,11 @@ public void testPassedObject(String configure) throws Exception
Map<String, String> concurrentMap = (Map<String, String>)configuration.getIdMap().get("concurrentMap");
assertThat(concurrentMap, instanceOf(ConcurrentMap.class));
assertEquals(concurrentMap.get("KEY"), "ITEM");

if ("org/eclipse/jetty/xml/configureWithElements.xml".equals(configure))
{
System.err.println("Static call with TestImpl: " + ExampleConfiguration.calledWithClass);
}
}

@ParameterizedTest
Expand Down Expand Up @@ -781,6 +786,10 @@ public void call(Ccc aaa)
{
}

public void call(Ddd ddd)
{
}

public void call(Abc abc)
{
}
Expand Down Expand Up @@ -808,14 +817,16 @@ 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);
methods.sort(EXECUTABLE_COMPARATOR);

assertThat(methods, Matchers.contains(
TestOrder.class.getMethod("call"),
TestOrder.class.getMethod("call", int.class),
TestOrder.class.getMethod("call", Abc.class),
TestOrder.class.getMethod("call", Aaa.class),
TestOrder.class.getMethod("call", Ddd.class), //more derived than String, Bbb or Ccc
TestOrder.class.getMethod("call", String.class),
TestOrder.class.getMethod("call", Bbb.class),
TestOrder.class.getMethod("call", Ccc.class),
TestOrder.class.getMethod("call", Abc.class),
TestOrder.class.getMethod("call", Aaa.class),
TestOrder.class.getMethod("call", Object.class),
TestOrder.class.getMethod("call", String[].class),
TestOrder.class.getMethod("call", String.class, String[].class)
Expand Down Expand Up @@ -2112,21 +2123,29 @@ public void testExecutableComparator()
aaa(null);
bbb(null);
ccc(null);

Stream.of(XmlConfigurationTest.class.getMethods())
.filter(m -> m.getName().length() == 3)
.filter(m -> !m.getName().equals("foo"))
.sorted(EXECUTABLE_COMPARATOR)
.map(Executable::toGenericString)
.forEach(System.out::println);
Logger slf4jLogger = LoggerFactory.getLogger(XmlConfiguration.class);
if (slf4jLogger.isDebugEnabled())
{
Stream.of(XmlConfigurationTest.class.getMethods())
.filter(m -> m.getName().length() == 3)
.filter(m -> !m.getName().equals("foo"))
.sorted(EXECUTABLE_COMPARATOR)
.map(Executable::toGenericString)
.forEach(System.out::println);
}

List<Method> methods = Arrays.asList(Arrays.stream(XmlConfigurationTest.class.getMethods())
.filter(m -> m.getName().length() == 3)
.sorted(EXECUTABLE_COMPARATOR)
.toArray(Method[]::new));

// The implementor must also ensure that the relation is transitive: ((compare(x, y)>0) && (compare(y, z)>0)) implies compare(x, z)>0
assertThat(EXECUTABLE_COMPARATOR.compare(methods.get(0), methods.get(1)), is(EXECUTABLE_COMPARATOR.compare(methods.get(1), methods.get(2))));
assertThat(EXECUTABLE_COMPARATOR.compare(methods.get(0), methods.get(1)), is(EXECUTABLE_COMPARATOR.compare(methods.get(0), methods.get(2))));
// Test that it is als reflexive
assertThat(EXECUTABLE_COMPARATOR.compare(methods.get(0), methods.get(1)), is(-EXECUTABLE_COMPARATOR.compare(methods.get(1), methods.get(0))));
assertThat(EXECUTABLE_COMPARATOR.compare(methods.get(0), methods.get(2)), is(-EXECUTABLE_COMPARATOR.compare(methods.get(2), methods.get(0))));
assertThat(EXECUTABLE_COMPARATOR.compare(methods.get(1), methods.get(2)), is(-EXECUTABLE_COMPARATOR.compare(methods.get(2), methods.get(1))));
}

public void aaa(Aaa ignored)
Expand Down Expand Up @@ -2157,6 +2176,31 @@ public static class Ccc implements Aaa
{
}

public abstract static class Ddd extends Ccc
{
}

@Test
public void testClassBeforeInterface()
{
List<String> orderedMethodIds = Stream.of(Example.class.getMethods())
.filter(m -> m.getName().equals("foo"))
.sorted(EXECUTABLE_COMPARATOR)
.map(Executable::toGenericString)
.collect(Collectors.toList());
orderedMethodIds.stream().forEach(System.err::println);

String[] expectedOrder = {
"public void org.eclipse.jetty.xml.XmlConfigurationTest$Example.foo()",
"public void org.eclipse.jetty.xml.XmlConfigurationTest$Example.foo(int)",
"public void org.eclipse.jetty.xml.XmlConfigurationTest$Example.foo(java.lang.Integer)",
"public int org.eclipse.jetty.xml.XmlConfigurationTest$Example.foo(java.lang.String)",
"public int org.eclipse.jetty.xml.XmlConfigurationTest$Example.foo(java.lang.CharSequence)"
};

assertThat(orderedMethodIds, contains(expectedOrder));
}

@Test
public void testFooExecutableComparator()
{
Expand All @@ -2165,15 +2209,22 @@ public void testFooExecutableComparator()
.sorted(EXECUTABLE_COMPARATOR)
.map(Executable::toGenericString)
.collect(Collectors.toList());
orderedMethodIds.forEach(System.out::println);

Logger slf4jLogger = LoggerFactory.getLogger(XmlConfiguration.class);
if (slf4jLogger.isDebugEnabled())
{
orderedMethodIds.forEach(System.out::println);
}

String[] expectedOrder = {
"public void org.eclipse.jetty.xml.XmlConfigurationTest$FooObj.foo()", // favor fewer args
"public void org.eclipse.jetty.xml.XmlConfigurationTest$FooObj.foo(int)", // favor primitives over non-primitives
"public void org.eclipse.jetty.xml.XmlConfigurationTest$FooObj.foo(java.time.temporal.Temporal)", // favor over Instant
"public int org.eclipse.jetty.xml.XmlConfigurationTest$FooObj.foo(java.lang.String)",
"public java.util.Locale org.eclipse.jetty.xml.XmlConfigurationTest$FooObj.foo(java.nio.charset.Charset)",
"public void org.eclipse.jetty.xml.XmlConfigurationTest$FooObj.foo(java.time.Instant)",
"public void org.eclipse.jetty.xml.XmlConfigurationTest$FooObj.foo(java.util.Locale)",
"public void org.eclipse.jetty.xml.XmlConfigurationTest$FooObj.foo()", // favour fewer args
"public void org.eclipse.jetty.xml.XmlConfigurationTest$FooObj.foo(int)", // favour primitives over non-primitives
"public int org.eclipse.jetty.xml.XmlConfigurationTest$FooObj.foo(java.lang.String)", //favour classes over interfaces
"public java.util.Locale org.eclipse.jetty.xml.XmlConfigurationTest$FooObj.foo(java.nio.charset.Charset)", //derived abstract class
"public void org.eclipse.jetty.xml.XmlConfigurationTest$FooObj.foo(java.time.Instant)", //class
"public void org.eclipse.jetty.xml.XmlConfigurationTest$FooObj.foo(java.util.Locale)", ///class
"public void org.eclipse.jetty.xml.XmlConfigurationTest$FooObj.foo(java.time.temporal.Temporal)", //derived interface
"public int org.eclipse.jetty.xml.XmlConfigurationTest$FooObj.foo(java.lang.CharSequence)", //interface
"public void org.eclipse.jetty.xml.XmlConfigurationTest$FooObj.foo(int,java.lang.String)",
"public void org.eclipse.jetty.xml.XmlConfigurationTest$FooObj.foo(java.lang.String,int)",
"public void org.eclipse.jetty.xml.XmlConfigurationTest$FooObj.foo(int,java.lang.String,java.lang.String)",
Expand All @@ -2188,6 +2239,11 @@ public void foo()
{
}

public int foo(CharSequence sequence)
{
return -99;
}

public int foo(String name)
{
return -1;
Expand Down Expand Up @@ -2230,4 +2286,29 @@ public void foo(int id, String name, String description, Object value)
{
}
}

public static class Example
{
public void foo()
{
}

public void foo(Integer i)
{
}

public void foo(int id)
{
}

public int foo(CharSequence sequence)
{
return 0;
}

public int foo(String string)
{
return -1;
}
}
}
13 changes: 13 additions & 0 deletions jetty-core/jetty-xml/src/test/resources/org/eclipse/jetty/xml/configureWithElements.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,19 @@
<Class>org.eclipse.jetty.xml.ExampleConfiguration</Class>
</Call>

<Call class="org.eclipse.jetty.xml.ExampleConfiguration" name="callStaticWithVarArgs">
<Arg>
<New class="org.eclipse.jetty.xml.ExampleConfiguration$TestImpl"/>
</Arg>
<Arg>
<Array type="java.lang.String">
<Item>A</Item>
<Item>B</Item>
<Item>C</Item>
</Array>
</Arg>
</Call>

<Call>
<Name>call</Name>
<Arg><Array type="java.lang.Object">
Expand Down

0 comments on commit 0d3a4f2

Please sign in to comment.