Issue #4631 - Warning about skipping of <Arg> nodes is in wrong place for <Configure>

jetty-server/src/main/config/etc/jetty.xml

@@ -1,8 +1,27 @@
 <?xml version="1.0"?><!DOCTYPE Configure PUBLIC "-//Jetty//Configure//EN" "http://www.eclipse.org/jetty/configure_9_3.dtd">
 
-<!-- =============================================================== --><!-- Documentation of this file format can be found at:              --><!-- https://www.eclipse.org/jetty/documentation/current/       --><!--                                                                 --><!-- Additional configuration files are available in $JETTY_HOME/etc --><!-- and can be mixed in. See start.ini file for the default         --><!-- configuration files.                                            --><!--                                                                 --><!-- For a description of the configuration mechanism, see the       --><!-- output of:                                                      --><!--   java -jar start.jar -?                                        --><!-- =============================================================== -->
+<!-- =============================================================== -->
+<!-- Documentation of this file format can be found at:              -->
+<!-- https://www.eclipse.org/jetty/documentation/current/            -->
+<!--                                                                 -->
+<!-- Additional configuration files are available in $JETTY_HOME/etc -->
+<!-- and can be mixed in. See start.ini file for the default         -->
+<!-- configuration files.                                            -->
+<!--                                                                 -->
+<!-- For a description of the configuration mechanism, see the       -->
+<!-- output of:                                                      -->
+<!--   java -jar start.jar -?                                        -->
+<!-- =============================================================== -->
 
-<!-- =============================================================== --><!-- Configure a Jetty Server instance with an ID "Server"           --><!-- Other configuration files may also configure the "Server"       --><!-- ID, in which case they are adding configuration to the same     --><!-- instance.  If other configuration have a different ID, they     --><!-- will create and configure another instance of Jetty.            --><!-- Consult the javadoc of o.e.j.server.Server for all              --><!-- configuration that may be set here.                             --><!-- =============================================================== -->
+<!-- =============================================================== -->
+<!-- Configure a Jetty Server instance with an ID "Server"           -->
+<!-- Other configuration files may also configure the "Server"       -->
+<!-- ID, in which case they are adding configuration to the same     -->
+<!-- instance.  If other configuration have a different ID, they     -->
+<!-- will create and configure another instance of Jetty.            -->
+<!-- Consult the javadoc of o.e.j.server.Server for all              -->
+<!-- configuration that may be set here.                             -->
+<!-- =============================================================== -->
 <Configure id="Server" class="org.eclipse.jetty.server.Server">
     <Arg name="threadpool"><Ref refid="threadPool"/></Arg>
 

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

@@ -396,6 +396,8 @@ public Object configure(Object obj) throws Exception
      */
     public Object configure() throws Exception
     {
+        if (LOG.isDebugEnabled())
+            LOG.debug("Configure {}", _location);
         return _processor.configure();
     }
 
@@ -466,6 +468,12 @@ public Object configure() throws Exception
                     throw new IllegalStateException(String.format("No matching constructor %s in %s", oClass, _configuration));
                 }
             }
+            else
+            {
+                // The Object already existed, if it has <Arg> nodes, warn about them not being used.
+                XmlConfiguration.getNodes(_root, "Arg")
+                    .forEach((node) -> LOG.warn("Ignored arg {} in {}", node, this._configuration._location));
+            }
             if (id != null)
                 _configuration.getIdMap().put(id, obj);
 
@@ -502,7 +510,7 @@ public void configure(Object obj, XmlParser.Node cfg, int i) throws Exception
                 XmlParser.Node node = (XmlParser.Node)o;
                 if ("Arg".equals(node.getTag()))
                 {
-                    LOG.warn("Ignored arg: " + node);
+                    // Skip <Arg> nodes (not handled here, see: newObj(), call(), and construct() methods)
                     continue;
                 }
                 break;
@@ -1894,7 +1902,7 @@ else if (arg.toLowerCase(Locale.ENGLISH).endsWith(".properties"))
                 {
                     if (!arg.toLowerCase(Locale.ENGLISH).endsWith(".properties") && (arg.indexOf('=') < 0))
                     {
-                        XmlConfiguration configuration = new XmlConfiguration(Resource.newResource(arg).getURI().toURL());
+                        XmlConfiguration configuration = new XmlConfiguration(Resource.newResource(arg));
                         if (last != null)
                             configuration.getIdMap().putAll(last.getIdMap());
                         if (properties.size() > 0)

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

@@ -45,6 +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.eclipse.jetty.xml.objects.BogusConnector;
+import org.eclipse.jetty.xml.objects.BogusServer;
+import org.eclipse.jetty.xml.objects.BogusThreadPool;
 import org.hamcrest.Matchers;
 import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.RepeatedTest;
@@ -60,7 +63,11 @@
 
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.allOf;
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.empty;
+import static org.hamcrest.Matchers.emptyString;
+import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.notNullValue;
@@ -265,7 +272,12 @@ public void initializeDefaults(Object object)
 
     public XmlConfiguration asXmlConfiguration(String rawXml) throws IOException, SAXException
     {
-        Path testFile = workDir.getEmptyPathDir().resolve("raw.xml");
+        return asXmlConfiguration("raw.xml", rawXml);
+    }
+
+    public XmlConfiguration asXmlConfiguration(String filename, String rawXml) throws IOException, SAXException
+    {
+        Path testFile = workDir.getEmptyPathDir().resolve(filename);
         try (BufferedWriter writer = Files.newBufferedWriter(testFile, UTF_8))
         {
             writer.write(rawXml);
@@ -1309,6 +1321,100 @@ public void testJettyStandardIdsAndPropertiesAndJettyHomeUriAndJettyBaseUri() th
         }
     }
 
+    @Test
+    public void testConfiguredWithNamedArg() throws Exception
+    {
+        XmlConfiguration xmlThreadPool = asXmlConfiguration("threadpool.xml",
+            "<Configure>\n" +
+                "  <New id=\"threadPool\" class=\"" + BogusThreadPool.class.getName() + "\">\n" +
+                "  </New>\n" +
+                "</Configure>");
+        XmlConfiguration xmlServer = asXmlConfiguration("server.xml",
+            "<Configure id=\"Server\" class=\"" + BogusServer.class.getName() + "\">\n" +
+                "  <Arg name=\"threadPool\"><Ref refid=\"threadPool\"/></Arg>\n" +
+                "</Configure>");
+
+        try (StdErrCapture logCapture = new StdErrCapture(XmlConfiguration.class))
+        {
+            Map<String, Object> idMap = mimicXmlConfigurationMain(xmlThreadPool, xmlServer);
+            Object server = idMap.get("Server");
+            assertThat("Server instance created", server, instanceOf(BogusServer.class));
+            BogusServer bogusServer = (BogusServer)server;
+            assertThat("Server has thread pool", bogusServer.getThreadPool(), instanceOf(BogusThreadPool.class));
+            List<String> logLines = logCapture.getLines();
+
+            String warnings = logLines.stream()
+                .filter(line -> line.contains(":WARN:"))
+                .collect(Collectors.joining("\n"));
+
+            assertThat("WARN logs", warnings, is(emptyString()));
+        }
+    }
+
+    @Test
+    public void testConfiguredSameWithNamedArgTwice() throws Exception
+    {
+        XmlConfiguration xmlThreadPool = asXmlConfiguration("threadpool.xml",
+            "<Configure>\n" +
+                "  <New id=\"threadPool\" class=\"" + BogusThreadPool.class.getName() + "\">\n" +
+                "  </New>\n" +
+                "</Configure>");
+        XmlConfiguration xmlServer = asXmlConfiguration("server.xml",
+            "<Configure id=\"Server\" class=\"" + BogusServer.class.getName() + "\">\n" +
+                "  <Arg name=\"threadPool\"><Ref refid=\"threadPool\"/></Arg>\n" +
+                "</Configure>");
+        XmlConfiguration xmlConnector = asXmlConfiguration("connector.xml",
+            "<Configure id=\"Server\" class=\"" + BogusServer.class.getName() + "\">\n" +
+                "  <Arg name=\"threadPool\"><Ref refid=\"threadPool\"/></Arg>\n" + // invalid line
+                "  <Call name=\"addConnector\">\n" +
+                "    <Arg>\n" +
+                "      <New class=\"" + BogusConnector.class.getName() + "\">\n" +
+                "        <Arg>plain</Arg>\n" +
+                "      </New>\n" +
+                "    </Arg>\n" +
+                "  </Call>\n" +
+                "</Configure>");
+
+        try (StdErrCapture logCapture = new StdErrCapture(XmlConfiguration.class))
+        {
+            Map<String, Object> idMap = mimicXmlConfigurationMain(xmlThreadPool, xmlServer, xmlConnector);
+            Object server = idMap.get("Server");
+            assertThat("Server instance created", server, instanceOf(BogusServer.class));
+            BogusServer bogusServer = (BogusServer)server;
+            assertThat("Server has thread pool", bogusServer.getThreadPool(), instanceOf(BogusThreadPool.class));
+            List<BogusConnector> connectors = bogusServer.getConnectors();
+            assertThat("Server has connectors", connectors, not(empty()));
+            assertThat("Connector[0]", connectors.get(0).getName(), is("plain"));
+            List<String> warnLogs = logCapture.getLines()
+                .stream().filter(line -> line.contains(":WARN:"))
+                .collect(Collectors.toList());
+
+            assertThat("WARN logs count", warnLogs.size(), is(1));
+
+            String actualWarn = warnLogs.get(0);
+            assertThat("WARN logs", actualWarn,
+                allOf(containsString("Ignored arg <Arg name="),
+                    containsString("connector.xml")
+                ));
+        }
+    }
+
+    /**
+     * This mimics the XML load behavior in XmlConfiguration.main(String ... args)
+     */
+    private Map<String, Object> mimicXmlConfigurationMain(XmlConfiguration... configurations) throws Exception
+    {
+        XmlConfiguration last = null;
+        for (XmlConfiguration configuration : configurations)
+        {
+            if (last != null)
+                configuration.getIdMap().putAll(last.getIdMap());
+            configuration.configure();
+            last = configuration;
+        }
+        return last.getIdMap();
+    }
+
     @Test
     public void testDeprecatedMany() throws Exception
     {

jetty-xml/src/test/java/org/eclipse/jetty/xml/objects/BogusConnector.java

@@ -0,0 +1,40 @@
+//
+//  ========================================================================
+//  Copyright (c) 1995-2020 Mort Bay Consulting Pty Ltd and others.
+//  ------------------------------------------------------------------------
+//  All rights reserved. This program and the accompanying materials
+//  are made available under the terms of the Eclipse Public License v1.0
+//  and Apache License v2.0 which accompanies this distribution.
+//
+//      The Eclipse Public License is available at
+//      http://www.eclipse.org/legal/epl-v10.html
+//
+//      The Apache License v2.0 is available at
+//      http://www.opensource.org/licenses/apache2.0.php
+//
+//  You may elect to redistribute this code under either of these licenses.
+//  ========================================================================
+//
+
+package org.eclipse.jetty.xml.objects;
+
+public class BogusConnector
+{
+    private final String name;
+
+    public BogusConnector(String name)
+    {
+        this.name = name;
+    }
+
+    public String getName()
+    {
+        return name;
+    }
+
+    @Override
+    public String toString()
+    {
+        return String.format("%s@%x[name=%s]", this.getClass().getSimpleName(), hashCode(), name);
+    }
+}

jetty-xml/src/test/java/org/eclipse/jetty/xml/objects/BogusServer.java

@@ -0,0 +1,56 @@
+//
+//  ========================================================================
+//  Copyright (c) 1995-2020 Mort Bay Consulting Pty Ltd and others.
+//  ------------------------------------------------------------------------
+//  All rights reserved. This program and the accompanying materials
+//  are made available under the terms of the Eclipse Public License v1.0
+//  and Apache License v2.0 which accompanies this distribution.
+//
+//      The Eclipse Public License is available at
+//      http://www.eclipse.org/legal/epl-v10.html
+//
+//      The Apache License v2.0 is available at
+//      http://www.opensource.org/licenses/apache2.0.php
+//
+//  You may elect to redistribute this code under either of these licenses.
+//  ========================================================================
+//
+
+package org.eclipse.jetty.xml.objects;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.eclipse.jetty.util.annotation.Name;
+
+public class BogusServer
+{
+    private BogusThreadPool threadPool;
+    private List<BogusConnector> connectors;
+
+    public BogusServer()
+    {
+    }
+
+    public BogusServer(@Name("threadPool") BogusThreadPool threadPool)
+    {
+        this.threadPool = threadPool;
+    }
+
+    public BogusThreadPool getThreadPool()
+    {
+        return threadPool;
+    }
+
+    public List<BogusConnector> getConnectors()
+    {
+        return connectors;
+    }
+
+    public void addConnector(BogusConnector connector)
+    {
+        if (connectors == null)
+            connectors = new ArrayList<>();
+        connectors.add(connector);
+    }
+}

jetty-xml/src/test/java/org/eclipse/jetty/xml/objects/BogusThreadPool.java

@@ -0,0 +1,28 @@
+//
+//  ========================================================================
+//  Copyright (c) 1995-2020 Mort Bay Consulting Pty Ltd and others.
+//  ------------------------------------------------------------------------
+//  All rights reserved. This program and the accompanying materials
+//  are made available under the terms of the Eclipse Public License v1.0
+//  and Apache License v2.0 which accompanies this distribution.
+//
+//      The Eclipse Public License is available at
+//      http://www.eclipse.org/legal/epl-v10.html
+//
+//      The Apache License v2.0 is available at
+//      http://www.opensource.org/licenses/apache2.0.php
+//
+//  You may elect to redistribute this code under either of these licenses.
+//  ========================================================================
+//
+
+package org.eclipse.jetty.xml.objects;
+
+public class BogusThreadPool
+{
+    @Override
+    public String toString()
+    {
+        return String.format("%s@x", this.getClass().getSimpleName(), hashCode());
+    }
+}