Fixes #8913 - Review Jetty XML syntax to allow calling JDK methods (#8915)

* Fixes #8913 - Review Jetty XML syntax to allow calling JDK methods

Now `<Call>`, `<Get>` and `<Set>` elements can use the `class` attribute
to specify the exact class to perform method lookup.

Improved support for `<Property>`, `<SystemProperty>` and `<Env>` so that
attribute `name` is now optional (as specified in the DTD), and a
`deprecated` attribute may be present instead.
This is necessary to terminally deprecate properties that have
no replacement.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

documentation/jetty-documentation/src/main/asciidoc/operations-guide/xml/xml-syntax.adoc

@@ -74,6 +74,8 @@ var mystring = new String();
 
 If an object with the id `mystring` already exists, then it is not created again but rather just referenced.
 
+Within element `<Configure>`, the created object (if any) is in xref:og-xml-syntax-scope[scope] and may be the implicit target of other, nested, elements.
+
 Typically the `<Configure>` element is used to configure a `Server` instance or `ContextHandler` subclasses such as `WebAppContext` that represent web applications.
 
 [[og-xml-syntax-arg]]
@@ -187,12 +189,38 @@ This is equivalent to the following Java code:
 var myhost = InetAddress.getByName("jdk.java.net");
 ----
 
+The `class` attribute (or `<Class>` element) can also be used to specify the Java class or interface to use to lookup the non-``static`` method name.
+This is necessary when the object in scope, onto which the `<Call>` would be applied, is an instance of a class that is not visible to Jetty classes, or not accessible because it is not `public`.
+For example:
+
+[source,xml,subs=normal]
+----
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE Configure PUBLIC "-//Jetty//Configure//EN" "https://www.eclipse.org/jetty/configure_10_0.dtd">
+
+<Configure>
+  <Call class="java.util.concurrent.Executors" name="newSingleThreadScheduledExecutor">
+    #<Call class="java.util.concurrent.ExecutorService" name="shutdown" />#
+  </Call>
+</Configure>
+----
+
+In the example above, `Executors.newSingleThreadScheduledExecutor()` returns an object whose class is a private JDK implementation class.
+Without an explicit `class` attribute (or `<Class>` element), it is not possible to invoke the method `shutdown()` when it is obtained via reflection from the private JDK implementation class, because while the method is `public`, the private JDK implementation class is not, therefore this exception is thrown:
+
+[source]
+----
+java.lang.IllegalAccessException: class org.eclipse.jetty.xml.XmlConfiguration$JettyXmlConfiguration (in module org.eclipse.jetty.xml) cannot access a member of class java.util.concurrent.Executors$DelegatedExecutorService (in module java.base) with modifiers "public"
+----
+
+The solution is to explicitly use the `class` attribute (or `<Class>` element) of the `<Call>` element that is invoking the `shutdown()` method, specifying a publicly accessible class or interface that the object in scope extends or implements (in the example above `java.util.concurrent.ExecutorService`).
+
 [[og-xml-syntax-get]]
 ===== `<Get>`
 
 Element `<Get>` retrieves the value of a JavaBean property specified by the mandatory `name` attribute.
 
-If the JavaBean property is `foo` (or `Foo`), `<Get>` first attempts to invoke _method_ `getFoo()`; failing that, attempts to retrieve the value from _field_ `foo` (or `Foo`).
+If the JavaBean property is `foo` (or `Foo`), `<Get>` first attempts to invoke _method_ `getFoo()` or _method_ `isFoo()`; failing that, attempts to retrieve the value from _field_ `foo` (or `Foo`).
 
 [source,xml]
 ----
@@ -212,6 +240,8 @@ If the JavaBean property is `foo` (or `Foo`), `<Get>` first attempts to invoke _
 </Configure>
 ----
 
+The `class` attribute (or `<Class>` element) allows to perform `static` calls, or to lookup the getter method from the specified class, as described in the xref:og-xml-syntax-call[`<Call>` section].
+
 [[og-xml-syntax-set]]
 ===== `<Set>`
 
@@ -235,6 +265,8 @@ If the JavaBean property is `foo` (or `Foo`), `<Set>` first attempts to invoke _
 </Configure>
 ----
 
+The `class` attribute (or `<Class>` element) allows to perform `static` calls, or to lookup the setter method from the specified class, as described in the xref:og-xml-syntax-call[`<Call>` section].
+
 [[og-xml-syntax-map]]
 ===== `<Map>` and `<Entry>`
 

jetty-xml/src/main/java/org/eclipse/jetty/xml/XmlConfiguration.java

@@ -522,9 +522,7 @@ public class XmlConfiguration
             String propertyValue = null;
 
             Class<?> oClass = nodeClass(node);
-            if (oClass != null)
+            if (oClass == null)
-                obj = null;
-            else
                 oClass = obj.getClass();
 
             // Look for a property value
@@ -553,9 +551,9 @@ public class XmlConfiguration
                 value = propertyValue;
             Object[] arg = {value};
 
-            Class<?>[] vClass = {Object.class};
+            Class<?> vClass = Object.class;
             if (value != null)
-                vClass[0] = value.getClass();
+                vClass = value.getClass();
 
             if (LOG.isDebugEnabled())
                 LOG.debug("XML {}.{} ({})", (obj != null ? obj.toString() : oClass.getName()), setter, value);
@@ -582,8 +580,8 @@ public class XmlConfiguration
                 // Try for native match
                 try
                 {
-                    Field type = vClass[0].getField("TYPE");
+                    Field type = vClass.getField("TYPE");
-                    vClass[0] = (Class<?>)type.get(null);
+                    vClass = (Class<?>)type.get(null);
                     Method set = oClass.getMethod(setter, vClass);
                     invokeMethod(set, obj, arg);
                     return;
@@ -715,7 +713,7 @@ public class XmlConfiguration
             }
 
             // No Joy
-            String message = oClass + "." + setter + "(" + vClass[0] + ")";
+            String message = oClass + "." + setter + "(" + vClass + ")";
             if (types != null)
                 message += ". Found setters for " + types;
             NoSuchMethodException failure = new NoSuchMethodException(message);
@@ -828,19 +826,11 @@ public class XmlConfiguration
 
             Class<?> oClass;
             if (clazz != null)
-            {
-                // static call
                 oClass = Loader.loadClass(clazz);
-                obj = null;
-            }
             else if (obj != null)
-            {
                 oClass = obj.getClass();
-            }
             else
-            {
                 throw new IllegalArgumentException(node.toString());
-            }
 
             if (LOG.isDebugEnabled())
                 LOG.debug("XML get {}", name);
@@ -906,19 +896,11 @@ public class XmlConfiguration
 
             Class<?> oClass;
             if (clazz != null)
-            {
-                // static call
                 oClass = Loader.loadClass(clazz);
-                obj = null;
-            }
             else if (obj != null)
-            {
                 oClass = obj.getClass();
-            }
             else
-            {
                 throw new IllegalArgumentException(node.toString());
-            }
 
             if (LOG.isDebugEnabled())
                 LOG.debug("XML call {}", name);
@@ -955,10 +937,6 @@ public class XmlConfiguration
                 Object[] arguments = args.applyTo(method);
                 if (arguments == null)
                     continue;
-                if (Modifier.isStatic(method.getModifiers()) != (obj == null))
-                    continue;
-                if ((obj == null) && method.getDeclaringClass() != oClass)
-                    continue;
 
                 try
                 {
@@ -1177,13 +1155,16 @@ public class XmlConfiguration
         {
             AttrOrElementNode aoeNode = new AttrOrElementNode(node, "Id", "Name", "Deprecated", "Default");
             String id = aoeNode.getString("Id");
-            String name = aoeNode.getString("Name", true);
+            String name = aoeNode.getString("Name", false);
             List<Object> deprecated = aoeNode.getList("Deprecated");
             String dftValue = aoeNode.getString("Default");
 
+            if (name == null && deprecated.isEmpty())
+                throw new IllegalStateException("Invalid <Property> element");
+
             // Look for a value
             Map<String, String> properties = _configuration.getProperties();
-            String value = properties.get(name);
+            String value = name == null ? null : properties.get(name);
 
             // Look for a deprecated name value
             String alternate = null;
@@ -1191,13 +1172,22 @@ public class XmlConfiguration
             {
                 for (Object d : deprecated)
                 {
-                    String v = properties.get(StringUtil.valueOf(d));
+                    if (d == null)
+                        continue;
+                    String v = properties.get(d.toString());
                     if (v != null)
                     {
                         if (value == null)
-                            LOG.warn("Property '{}' is deprecated, use '{}' instead", d, name);
+                        {
+                            if (name == null)
+                                LOG.warn("Property '{}' is deprecated, no replacement available", d);
+                            else
+                                LOG.warn("Property '{}' is deprecated, use '{}' instead", d, name);
+                        }
                         else
-                            LOG.warn("Property '{}' is deprecated, value from '{}' used", d, name);
+                        {
+                            LOG.warn("Property '{}' is deprecated, value from '{}' used instead", d, name);
+                        }
                     }
                     if (alternate == null)
                         alternate = v;
@@ -1228,12 +1218,15 @@ public class XmlConfiguration
         {
             AttrOrElementNode aoeNode = new AttrOrElementNode(node, "Id", "Name", "Deprecated", "Default");
             String id = aoeNode.getString("Id");
-            String name = aoeNode.getString("Name", true);
+            String name = aoeNode.getString("Name", false);
             List<Object> deprecated = aoeNode.getList("Deprecated");
             String dftValue = aoeNode.getString("Default");
 
+            if (name == null && deprecated.isEmpty())
+                throw new IllegalStateException("Invalid <SystemProperty> element");
+
             // Look for a value
-            String value = System.getProperty(name);
+            String value = name == null ? null : System.getProperty(name);
 
             // Look for a deprecated name value
             String alternate = null;
@@ -1247,9 +1240,16 @@ public class XmlConfiguration
                     if (v != null)
                     {
                         if (value == null)
-                            LOG.warn("SystemProperty '{}' is deprecated, use '{}' instead", d, name);
+                        {
+                            if (name == null)
+                                LOG.warn("SystemProperty '{}' is deprecated, no replacement available", d);
+                            else
+                                LOG.warn("SystemProperty '{}' is deprecated, use '{}' instead", d, name);
+                        }
                         else
+                        {
                             LOG.warn("SystemProperty '{}' is deprecated, value from '{}' used", d, name);
+                        }
                     }
                     if (alternate == null)
                         alternate = v;
@@ -1281,22 +1281,30 @@ public class XmlConfiguration
         {
             AttrOrElementNode aoeNode = new AttrOrElementNode(node, "Id", "Name", "Deprecated", "Default");
             String id = aoeNode.getString("Id");
-            String name = aoeNode.getString("Name", true);
+            String name = aoeNode.getString("Name", false);
             List<Object> deprecated = aoeNode.getList("Deprecated");
             String dftValue = aoeNode.getString("Default");
 
+            if (name == null && deprecated.isEmpty())
+                throw new IllegalStateException("Invalid <Env> element");
+
             // Look for a value
-            String value = System.getenv(name);
+            String value = name == null ? null : System.getenv(name);
 
             // Look for a deprecated name value
             if (value == null && !deprecated.isEmpty())
             {
                 for (Object d : deprecated)
                 {
-                    value = System.getenv(StringUtil.valueOf(d));
+                    if (d == null)
+                        continue;
+                    value = System.getenv(d.toString());
                     if (value != null)
                     {
-                        LOG.warn("Property '{}' is deprecated, use '{}' instead", d, name);
+                        if (name == null)
+                            LOG.warn("Property '{}' is deprecated, no replacement available", d);
+                        else
+                            LOG.warn("Property '{}' is deprecated, use '{}' instead", d, name);
                         break;
                     }
                 }

jetty-xml/src/main/resources/org/eclipse/jetty/xml/configure_10_0.dtd

@@ -286,7 +286,7 @@ System Property Element.
 This element allows JVM System properties to be retrieved as
 part of the value of elements such as Set, Put, Arg, etc.
 The name attribute specifies the property name and the optional
-default argument provides a default value.
+default attribute provides a default value.
 
  <SystemProperty name="Test" default="value" />
 
@@ -303,7 +303,7 @@ Environment variable Element.
 This element allows OS Environment variables to be retrieved as
 part of the value of elements such as Set, Put, Arg, etc.
 The name attribute specifies the env variable name and the optional
-default argument provides a default value.
+default attribute provides a default value.
 
  <Env name="Test" default="value" />
 
@@ -311,7 +311,6 @@ This is equivalent to:
 
  String v=System.getEnv("Test");
  if (v==null) v="value";
-
 -->
 <!ELEMENT Env (Id?,Name?,Deprecated*,Default?) >
 <!ATTLIST Env %ID_ATTR; %NAME_ATTR; %DEPRECATED_ATTR; %DEFAULT_ATTR; >
@@ -321,7 +320,7 @@ This is equivalent to:
 Property Element.
 This element allows arbitrary properties to be retrieved by name.
 The name attribute specifies the property name and the optional
-default argument provides a default value.
+default attribute provides a default value.
 -->
 <!ELEMENT Property (Id?,Name?,Deprecated*,Default?) >
 <!ATTLIST Property %ID_ATTR; %NAME_ATTR; %DEPRECATED_ATTR; %DEFAULT_ATTR; >

jetty-xml/src/test/java/org/eclipse/jetty/xml/XmlConfigurationTest.java

@@ -20,6 +20,7 @@ import java.io.PrintStream;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.net.URL;
+import java.nio.channels.SocketChannel;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.ArrayList;
@@ -29,9 +30,11 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
+import javax.net.ssl.SSLSocketFactory;
 
 import org.eclipse.jetty.logging.JettyLevel;
 import org.eclipse.jetty.logging.JettyLogger;
@@ -39,7 +42,6 @@ import org.eclipse.jetty.logging.StdErrAppender;
 import org.eclipse.jetty.toolchain.test.FS;
 import org.eclipse.jetty.toolchain.test.MavenTestingUtils;
 import org.eclipse.jetty.toolchain.test.jupiter.WorkDir;
-import org.eclipse.jetty.toolchain.test.jupiter.WorkDirExtension;
 import org.eclipse.jetty.util.annotation.Name;
 import org.eclipse.jetty.util.resource.PathResource;
 import org.eclipse.jetty.util.resource.Resource;
@@ -48,7 +50,6 @@ import org.junit.jupiter.api.Assumptions;
 import org.junit.jupiter.api.RepeatedTest;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.condition.OS;
-import org.junit.jupiter.api.extension.ExtendWith;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
@@ -74,19 +75,19 @@ import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
-@ExtendWith(WorkDirExtension.class)
 public class XmlConfigurationTest
 {
-    public WorkDir workDir;
-
     private static final String STRING_ARRAY_XML = "<Array type=\"String\"><Item type=\"String\">String1</Item><Item type=\"String\">String2</Item></Array>";
     private static final String INT_ARRAY_XML = "<Array type=\"int\"><Item type=\"int\">1</Item><Item type=\"int\">2</Item></Array>";
 
+    public WorkDir workDir;
+
     @Test
     public void testMortBay() throws Exception
     {
         URL url = XmlConfigurationTest.class.getResource("mortbay.xml");
         Resource resource = Resource.newResource(url);
+        assertNotNull(resource);
         XmlConfiguration configuration = new XmlConfiguration(resource);
         configuration.configure();
     }
@@ -269,7 +270,7 @@ public class XmlConfigurationTest
 
     public XmlConfiguration asXmlConfiguration(String rawXml) throws IOException, SAXException
     {
-        if (rawXml.indexOf("!DOCTYPE") < 0)
+        if (!rawXml.contains("!DOCTYPE"))
             rawXml = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
                 "<!DOCTYPE Configure PUBLIC \"-//Jetty//Configure//EN\" \"https://www.eclipse.org/jetty/configure_10_0.dtd\">\n" +
                 rawXml;
@@ -278,7 +279,7 @@ public class XmlConfigurationTest
 
     public XmlConfiguration asXmlConfiguration(String filename, String rawXml) throws IOException, SAXException
     {
-        if (rawXml.indexOf("!DOCTYPE") < 0)
+        if (!rawXml.contains("!DOCTYPE"))
             rawXml = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
                 "<!DOCTYPE Configure PUBLIC \"-//Jetty//Configure//EN\" \"https://www.eclipse.org/jetty/configure_10_0.dtd\">\n" +
                 rawXml;
@@ -348,7 +349,7 @@ public class XmlConfigurationTest
         tc.testField1 = -1;
         configuration.configure(tc);
         assertEquals(-1, tc.testField1);
-        assertEquals(configuration.getIdMap().get("test"), null);
+        assertNull(configuration.getIdMap().get("test"));
     }
 
     @Test
@@ -745,7 +746,7 @@ public class XmlConfigurationTest
     {
         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);
+        methods.sort(XmlConfiguration.EXECUTABLE_COMPARATOR);
         assertThat(methods, Matchers.contains(
             TestOrder.class.getMethod("call"),
             TestOrder.class.getMethod("call", int.class),
@@ -1465,7 +1466,7 @@ public class XmlConfigurationTest
             assertThat("BarNamed has foo", bar.getFoo(), is("foozball"));
         });
 
-        List<String> warnings = Arrays.stream(logBytes.toString(UTF_8.name()).split(System.lineSeparator()))
+        List<String> warnings = Arrays.stream(logBytes.toString(UTF_8).split(System.lineSeparator()))
             .filter(line -> line.contains(":WARN"))
             .collect(Collectors.toList());
 
@@ -1495,7 +1496,7 @@ public class XmlConfigurationTest
             assertThat("BarNamed has foo", bar.getFoo(), is("foozball"));
         });
 
-        List<String> warnings = Arrays.stream(logBytes.toString(UTF_8.name()).split(System.lineSeparator()))
+        List<String> warnings = Arrays.stream(logBytes.toString(UTF_8).split(System.lineSeparator()))
             .filter(line -> line.contains(":WARN :"))
             .collect(Collectors.toList());
 
@@ -1564,7 +1565,7 @@ public class XmlConfigurationTest
             assertThat("Zeds[0]", zeds.get(0), is("plain-zero"));
         });
 
-        List<String> warnings = Arrays.stream(logBytes.toString(UTF_8.name()).split(System.lineSeparator()))
+        List<String> warnings = Arrays.stream(logBytes.toString(UTF_8).split(System.lineSeparator()))
             .filter(line -> line.contains(":WARN :"))
             .collect(Collectors.toList());
 
@@ -1590,6 +1591,7 @@ public class XmlConfigurationTest
             configuration.configure();
             last = configuration;
         }
+        assertNotNull(last);
         return last.getIdMap();
     }
 
@@ -1628,7 +1630,7 @@ public class XmlConfigurationTest
 
         ByteArrayOutputStream logBytes = captureLoggingBytes(xmlConfiguration::configure);
 
-        String[] lines = logBytes.toString(UTF_8.name()).split(System.lineSeparator());
+        String[] lines = logBytes.toString(UTF_8).split(System.lineSeparator());
         List<String> warnings = Arrays.stream(lines)
             .filter(line -> line.contains(":WARN :"))
             .filter(line -> line.contains(testClass.getSimpleName()))
@@ -1765,6 +1767,94 @@ public class XmlConfigurationTest
         assertEquals(baseDir.resolve("etc/keystore").toString(), resolved);
     }
 
+    @Test
+    public void testPropertyNoNameAttributeWithDeprecatedAttribute() throws Exception
+    {
+        XmlConfiguration configuration = asXmlConfiguration(
+            "<Configure class=\"org.eclipse.jetty.xml.TestConfiguration\">" +
+            "  <Set name=\"TestString\">" +
+            "    <Property deprecated=\"jetty.deprecated\" />" +
+            "  </Set>" +
+            "</Configure>"
+        );
+
+        String value = "deprecated";
+        configuration.getProperties().put("jetty.deprecated", value);
+
+        TestConfiguration tc = new TestConfiguration();
+        configuration.configure(tc);
+
+        assertThat(tc.getTestString(), is(value));
+    }
+
+    @Test
+    public void testVirtualGetWithClassAttribute() throws Exception
+    {
+        // SocketChannel.open() returns an internal class
+        // that cannot be accessed, so <Get> must specify the
+        // class to use to lookup the 'getLocalAddress' method.
+        XmlConfiguration configuration = asXmlConfiguration(
+            "<Configure class=\"org.eclipse.jetty.xml.TestConfiguration\">" +
+            "  <Set name=\"Test\">" +
+            "    <Call class=\"java.nio.channels.SocketChannel\" name=\"open\">" +
+            "      <Get class=\"java.nio.channels.SocketChannel\" name=\"localAddress\" />" +
+            "    </Call>" +
+            "  </Set>" +
+            "</Configure>"
+        );
+
+        TestConfiguration tc = new TestConfiguration();
+        configuration.configure(tc);
+
+        assertThat(tc.testObject, instanceOf(SocketChannel.class));
+    }
+
+    @Test
+    public void testVirtualSetWithClassAttribute() throws Exception
+    {
+        // SSLSocketFactory.getDefault().createSocket() returns an internal
+        // class that cannot be accessed, so <Set> must specify the
+        // class to use to lookup the 'setNeedClientAuth' method.
+        XmlConfiguration configuration = asXmlConfiguration(
+            "<Configure class=\"org.eclipse.jetty.xml.TestConfiguration\">" +
+            "  <Set name=\"Test\">" +
+            "    <Call class=\"javax.net.ssl.SSLSocketFactory\" name=\"getDefault\">" +
+            "      <Call class=\"javax.net.ssl.SSLSocketFactory\" name=\"createSocket\">" +
+            "        <Set class=\"javax.net.ssl.SSLSocket\" name=\"needClientAuth\">true</Set>" +
+            "      </Call>" +
+            "    </Call>" +
+            "  </Set>" +
+            "</Configure>"
+        );
+
+        TestConfiguration tc = new TestConfiguration();
+        configuration.configure(tc);
+
+        assertThat(tc.testObject, instanceOf(SSLSocketFactory.class));
+    }
+
+    @Test
+    public void testVirtualCallWithClassAttribute() throws Exception
+    {
+        // Executors.newSingleThreadScheduledExecutor() returns an
+        // internal class that cannot be accessed, so <Call> must
+        // specify the class to use to lookup the 'shutdown' method.
+        XmlConfiguration configuration = asXmlConfiguration(
+            "<Configure class=\"org.eclipse.jetty.xml.TestConfiguration\">" +
+            "  <Set name=\"Test\">" +
+            "    <Call class=\"java.util.concurrent.Executors\" name=\"newSingleThreadScheduledExecutor\">" +
+            "      <Call class=\"java.util.concurrent.ExecutorService\" name=\"shutdown\" />" +
+            "    </Call>" +
+            "  </Set>" +
+            "</Configure>"
+        );
+
+        TestConfiguration tc = new TestConfiguration();
+        configuration.configure(tc);
+
+        assertThat(tc.testObject, instanceOf(ScheduledExecutorService.class));
+    }
+
     private ByteArrayOutputStream captureLoggingBytes(ThrowableAction action) throws Exception
     {
         Logger slf4jLogger = LoggerFactory.getLogger(XmlConfiguration.class);