Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from
Mar 10, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b839ce6
Issue #4631 - Fixing XML comment that was accidentally reformatted
joakime Mar 2, 2020
cfdffd1
Issue #4631 - Warning about skipping of <Arg> nodes is in wrong place…
joakime Mar 2, 2020
89f2cad
Issue #4631 - Improving testcase
joakime Mar 2, 2020
3ee3246
Issue #4631 - Removing test classes
joakime Mar 3, 2020
da7e8ed
Issue #4631 - Cleaning up configure with index per PR review
joakime Mar 3, 2020
69fa180
Issue #4631 - More named arg test cases
joakime Mar 3, 2020
cc80ebe
Issue #4631 - Add testConfiguredWithNamedArgNotFirst
joakime Mar 3, 2020
ad3149c
Cleanup configuration index usage
gregw Mar 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions jetty-server/src/main/config/etc/jetty.xml
View file
Open in desktop
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
Open in desktop
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));
gregw marked this conversation as resolved.
Show resolved Hide resolved
}
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
gregw marked this conversation as resolved.
Show resolved Hide resolved
{
// 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
Open in desktop
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();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see a test where there is a Arg misplaced, as below, so that we define in a test how we should behave.

<Configure ...>
  <Arg>...</Arg>
  <New ...>...</New>
  <Arg>...</Arg>
</Configure>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gregw pointed out that the Configure_9_3.dtd (and Configure_10_0.dtd) does not allow a <Arg> anywhere but the start of the element list.

https://github.com/eclipse/jetty.project/blob/678385bfda0f90a0bcccf7f86aa272073c1b4f29/jetty-xml/src/main/resources/org/eclipse/jetty/xml/configure_10_0.dtd#L45-L46

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test that is expected to fail in this scenario is dependent on the dtd used (or not used) I would think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then shouldn't we do the same for Call and New? I don't think the DTD has the same enforcement for those.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #4658 for these new test cases.

Also <Call>

https://github.com/eclipse/jetty.project/blob/678385bfda0f90a0bcccf7f86aa272073c1b4f29/jetty-xml/src/main/resources/org/eclipse/jetty/xml/configure_10_0.dtd#L168-L169

And <New>

https://github.com/eclipse/jetty.project/blob/678385bfda0f90a0bcccf7f86aa272073c1b4f29/jetty-xml/src/main/resources/org/eclipse/jetty/xml/configure_10_0.dtd#L209-L210

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also opened #4659 for the missing Id?,Name?,Class?,Arg* in the <Configure> section on Configure_10_0.dtd

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbordet we had such a test, but that is invalid XML against the DTD. I don't think we should be required to process non-valid XML

Note that I'm doing a cleanup in 10 - see #4656


@Test
public void testDeprecatedMany() throws Exception
{
Expand Down