Skip to content

Commit

Permalink
Issue #4631 - Warning about skipping of <Arg> nodes is in wrong place…
Browse files Browse the repository at this point in the history 
… for <Configure> (#4632)

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

Signed-off-by: Joakim Erdfelt <[email protected]>

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

Signed-off-by: Joakim Erdfelt <[email protected]>

* Issue #4631 - Improving testcase

Signed-off-by: Joakim Erdfelt <[email protected]>

* Issue #4631 - Removing test classes

Signed-off-by: Joakim Erdfelt <[email protected]>

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

Signed-off-by: Joakim Erdfelt <[email protected]>

* Issue #4631 - More named arg test cases

Signed-off-by: Joakim Erdfelt <[email protected]>

* Issue #4631 - Add testConfiguredWithNamedArgNotFirst

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

Signed-off-by: Joakim Erdfelt <[email protected]>

* Cleanup configuration index usage

Signed-off-by: Greg Wilkins <[email protected]>

Co-authored-by: Greg Wilkins <[email protected]>
  • Loading branch information
@joakime @gregw
joakime and gregw authored Mar 10, 2020
1 parent 82247f2 commit 965483e
Show file tree
Hide file tree
Showing 3 changed files with 235 additions and 23 deletions.
23 changes: 21 additions & 2 deletions jetty-server/src/main/config/etc/jetty.xml
View file
Original file line number Diff line number Diff line change
@@ -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>

Expand Down
45 changes: 25 additions & 20 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 @@ -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();
}

Expand Down Expand Up @@ -442,7 +444,12 @@ public Object configure(Object obj) throws Exception
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;
}

Expand All @@ -454,23 +461,32 @@ public Object configure() throws Exception
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;
}

Expand All @@ -493,21 +509,6 @@ private static Class<?> nodeClass(XmlParser.Node node) throws ClassNotFoundExcep
*/
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++)
{
Expand All @@ -521,6 +522,10 @@ public void configure(Object obj, XmlParser.Node cfg, int i) throws Exception
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;
Expand Down Expand Up @@ -1894,7 +1899,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)
Expand Down
190 changes: 189 additions & 1 deletion 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 @@ -41,6 +41,7 @@

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;
Expand All @@ -60,7 +61,10 @@

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;
Expand Down Expand Up @@ -265,7 +269,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);
Expand Down Expand Up @@ -1309,6 +1318,185 @@ public void testJettyStandardIdsAndPropertiesAndJettyHomeUriAndJettyBaseUri() th
}
}

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
{
Expand Down

0 comments on commit 965483e

Please sign in to comment.