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

* Issue #4631 - Fixing XML comment that was accidentally reformatted

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>

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

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>

* Issue #4631 - Improving testcase

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>

* Issue #4631 - Removing test classes

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>

* Issue #4631 - Cleaning up configure with index per PR review

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>

* Issue #4631 - More named arg test cases

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>

* Issue #4631 - Add testConfiguredWithNamedArgNotFirst

+ new testcase where <Arg> is needed, but is not the first node

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>

* Cleanup configuration index usage

Signed-off-by: Greg Wilkins <gregw@webtide.com>

Co-authored-by: Greg Wilkins <gregw@webtide.com>

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 class XmlConfiguration
      */
     public Object configure() throws Exception
     {
+        if (LOG.isDebugEnabled())
+            LOG.debug("Configure {}", _location);
         return _processor.configure();
     }
 
@@ -442,7 +444,12 @@ public class XmlConfiguration
             String id = _root.getAttribute("id");
             if (id != null)
                 _configuration.getIdMap().put(id, obj);
-            configure(obj, _root, 0);
+
+            AttrOrElementNode aoeNode = new AttrOrElementNode(obj, _root, "Arg");
+            // The Object already existed, if it has <Arg> nodes, warn about them not being used.
+            aoeNode.getNodes("Arg")
+                .forEach((node) -> LOG.warn("Ignored arg {} in {}", node, this._configuration._location));
+            configure(obj, _root, aoeNode.getNext());
             return obj;
         }
 
@@ -454,23 +461,32 @@ public class XmlConfiguration
             String id = _root.getAttribute("id");
             Object obj = id == null ? null : _configuration.getIdMap().get(id);
 
-            int index = 0;
+            AttrOrElementNode aoeNode;
+
             if (obj == null && oClass != null)
             {
+                aoeNode = new AttrOrElementNode(_root, "Arg");
                 try
                 {
-                    obj = construct(oClass, new Args(null, oClass, XmlConfiguration.getNodes(_root, "Arg")));
+                    obj = construct(oClass, new Args(null, oClass, aoeNode.getNodes("Arg")));
                 }
                 catch (NoSuchMethodException x)
                 {
                     throw new IllegalStateException(String.format("No matching constructor %s in %s", oClass, _configuration));
                 }
             }
+            else
+            {
+                aoeNode = new AttrOrElementNode(obj, _root, "Arg");
+                // The Object already existed, if it has <Arg> nodes, warn about them not being used.
+                aoeNode.getNodes("Arg")
+                    .forEach((node) -> LOG.warn("Ignored arg {} in {}", node, this._configuration._location));
+            }
             if (id != null)
                 _configuration.getIdMap().put(id, obj);
 
             _configuration.initializeDefaults(obj);
-            configure(obj, _root, index);
+            configure(obj, _root, aoeNode.getNext());
             return obj;
         }
 
@@ -493,21 +509,6 @@ public class XmlConfiguration
          */
         public void configure(Object obj, XmlParser.Node cfg, int i) throws Exception
         {
-            // Object already constructed so skip any arguments
-            for (; i < cfg.size(); i++)
-            {
-                Object o = cfg.get(i);
-                if (o instanceof String)
-                    continue;
-                XmlParser.Node node = (XmlParser.Node)o;
-                if ("Arg".equals(node.getTag()))
-                {
-                    LOG.warn("Ignored arg: " + node);
-                    continue;
-                }
-                break;
-            }
-
             // Process real arguments
             for (; i < cfg.size(); i++)
             {
@@ -521,6 +522,10 @@ public class XmlConfiguration
                     String tag = node.getTag();
                     switch (tag)
                     {
+                        case "Arg":
+                        case "Class":
+                        case "Id":
+                            throw new IllegalStateException("Element '" + tag + "' not skipped");
                         case "Set":
                             set(obj, node);
                             break;
@@ -1894,7 +1899,7 @@ public class XmlConfiguration
                 {
                     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

@@ -41,6 +41,7 @@ import java.util.stream.Stream;
 
 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.log.Log;
 import org.eclipse.jetty.util.log.Logger;
 import org.eclipse.jetty.util.log.StdErrLog;
@@ -60,7 +61,10 @@ import org.xml.sax.SAXException;
 
 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.instanceOf;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.notNullValue;
@@ -265,7 +269,12 @@ public class XmlConfigurationTest
 
     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 +1318,185 @@ public class XmlConfigurationTest
         }
     }
 
+    public static class BarNamed
+    {
+        private String foo;
+        private List<String> zeds;
+
+        public BarNamed(@Name("foo") String foo)
+        {
+            this.foo = foo;
+        }
+
+        public void addZed(String zed)
+        {
+            if (zeds == null)
+                zeds = new ArrayList<>();
+            zeds.add(zed);
+        }
+
+        public List<String> getZeds()
+        {
+            return zeds;
+        }
+
+        public String getFoo()
+        {
+            return foo;
+        }
+    }
+
+    @Test
+    public void testConfiguredWithNamedArg() throws Exception
+    {
+        XmlConfiguration xmlFoo = asXmlConfiguration("foo.xml",
+            "<Configure>\n" +
+                "  <New id=\"foo\" class=\"java.lang.String\">\n" +
+                "    <Arg>foozball</Arg>\n" +
+                "  </New>\n" +
+                "</Configure>");
+        XmlConfiguration xmlBar = asXmlConfiguration("bar.xml",
+            "<Configure id=\"bar\" class=\"" + BarNamed.class.getName() + "\">\n" +
+                "  <Arg name=\"foo\"><Ref refid=\"foo\"/></Arg>\n" +
+                "</Configure>");
+
+        try (StdErrCapture logCapture = new StdErrCapture(XmlConfiguration.class))
+        {
+            Map<String, Object> idMap = mimicXmlConfigurationMain(xmlFoo, xmlBar);
+            Object obj = idMap.get("bar");
+            assertThat("BarNamed instance created", obj, instanceOf(BarNamed.class));
+            BarNamed bar = (BarNamed)obj;
+            assertThat("BarNamed has foo", bar.getFoo(), is("foozball"));
+
+            List<String> warnLogs = logCapture.getLines()
+                .stream().filter(line -> line.contains(":WARN:"))
+                .collect(Collectors.toList());
+
+            assertThat("WARN logs size", warnLogs.size(), is(0));
+        }
+    }
+
+    @Test
+    public void testConfiguredWithArgNotUsingName() throws Exception
+    {
+        XmlConfiguration xmlFoo = asXmlConfiguration("foo.xml",
+            "<Configure>\n" +
+                "  <New id=\"foo\" class=\"java.lang.String\">\n" +
+                "    <Arg>foozball</Arg>\n" +
+                "  </New>\n" +
+                "</Configure>");
+        XmlConfiguration xmlBar = asXmlConfiguration("bar.xml",
+            "<Configure id=\"bar\" class=\"" + BarNamed.class.getName() + "\">\n" +
+                "  <Arg><Ref refid=\"foo\"/></Arg>\n" + // no name specified
+                "</Configure>");
+
+        try (StdErrCapture logCapture = new StdErrCapture(XmlConfiguration.class))
+        {
+            Map<String, Object> idMap = mimicXmlConfigurationMain(xmlFoo, xmlBar);
+            Object obj = idMap.get("bar");
+            assertThat("BarNamed instance created", obj, instanceOf(BarNamed.class));
+            BarNamed bar = (BarNamed)obj;
+            assertThat("BarNamed has foo", bar.getFoo(), is("foozball"));
+
+            List<String> warnLogs = logCapture.getLines()
+                .stream().filter(line -> line.contains(":WARN:"))
+                .collect(Collectors.toList());
+
+            assertThat("WARN logs size", warnLogs.size(), is(0));
+        }
+    }
+
+    @Test
+    public void testConfiguredWithBadNamedArg() throws Exception
+    {
+        XmlConfiguration xmlBar = asXmlConfiguration("bar.xml",
+            "<Configure id=\"bar\" class=\"" + BarNamed.class.getName() + "\">\n" +
+                "  <Arg name=\"foozball\">foozball</Arg>\n" + // wrong name specified
+                "</Configure>");
+
+        IllegalStateException cause = assertThrows(IllegalStateException.class, () ->
+            mimicXmlConfigurationMain(xmlBar));
+
+        assertThat("Cause message", cause.getMessage(), containsString("No matching constructor"));
+    }
+
+    @Test
+    public void testConfiguredWithTooManyNamedArgs() throws Exception
+    {
+        XmlConfiguration xmlBar = asXmlConfiguration("bar.xml",
+            "<Configure id=\"bar\" class=\"" + BarNamed.class.getName() + "\">\n" +
+                "  <Arg name=\"foo\">foozball</Arg>\n" +
+                "  <Arg name=\"foo\">soccer</Arg>\n" + // neither should win
+                "</Configure>");
+
+        IllegalStateException cause = assertThrows(IllegalStateException.class, () ->
+            mimicXmlConfigurationMain(xmlBar));
+
+        assertThat("Cause message", cause.getMessage(), containsString("No matching constructor"));
+    }
+
+    @Test
+    public void testConfiguredSameWithNamedArgTwice() throws Exception
+    {
+        XmlConfiguration xmlFoo = asXmlConfiguration("foo.xml",
+            "<Configure>\n" +
+                "  <New id=\"foo\" class=\"java.lang.String\">\n" +
+                "    <Arg>foozball</Arg>\n" +
+                "  </New>\n" +
+                "</Configure>");
+        XmlConfiguration xmlBar = asXmlConfiguration("bar.xml",
+            "<Configure id=\"bar\" class=\"" + BarNamed.class.getName() + "\">\n" +
+                "  <Arg name=\"foo\"><Ref refid=\"foo\"/></Arg>\n" +
+                "</Configure>");
+        XmlConfiguration xmlAddZed = asXmlConfiguration("zed.xml",
+            "<Configure id=\"bar\" class=\"" + BarNamed.class.getName() + "\">\n" +
+                "  <Arg name=\"foo\">baz</Arg>\n" + // the invalid line
+                "  <Call name=\"addZed\">\n" +
+                "    <Arg>plain-zero</Arg>\n" +
+                "  </Call>\n" +
+                "</Configure>");
+
+        try (StdErrCapture logCapture = new StdErrCapture(XmlConfiguration.class))
+        {
+            Map<String, Object> idMap = mimicXmlConfigurationMain(xmlFoo, xmlBar, xmlAddZed);
+            Object obj = idMap.get("bar");
+            assertThat("BarNamed instance created", obj, instanceOf(BarNamed.class));
+            BarNamed bar = (BarNamed)obj;
+            assertThat("BarNamed has foo", bar.getFoo(), is("foozball"));
+            List<String> zeds = bar.getZeds();
+            assertThat("BarNamed has zeds", zeds, not(empty()));
+            assertThat("Zeds[0]", zeds.get(0), is("plain-zero"));
+
+            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("zed.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
     {