From 82247f257d0bab95b0760e0f4888c01c46511945 Mon Sep 17 00:00:00 2001 From: Mattias Andersson Date: Tue, 10 Mar 2020 01:33:13 +0100 Subject: [PATCH 1/2] Issue #4647 Hazelcast remote.xml configuration file do not configure hazelcast remote addresses (#4649) * Fix for #4647 Signed-off-by: Mattias Andersson Co-authored-by: Mattias Andersson --- .../config/etc/sessions/hazelcast/remote.xml | 3 ++- .../session-store-hazelcast-embedded.mod | 1 - .../session-store-hazelcast-remote.mod | 2 +- .../HazelcastSessionDataStoreFactory.java | 20 +++++++++++++++++++ 4 files changed, 23 insertions(+), 3 deletions(-) diff --git a/jetty-hazelcast/src/main/config/etc/sessions/hazelcast/remote.xml b/jetty-hazelcast/src/main/config/etc/sessions/hazelcast/remote.xml index 96588eab19f..c67f8dff1d9 100644 --- a/jetty-hazelcast/src/main/config/etc/sessions/hazelcast/remote.xml +++ b/jetty-hazelcast/src/main/config/etc/sessions/hazelcast/remote.xml @@ -4,7 +4,7 @@ - + @@ -15,6 +15,7 @@ + diff --git a/jetty-hazelcast/src/main/config/modules/session-store-hazelcast-embedded.mod b/jetty-hazelcast/src/main/config/modules/session-store-hazelcast-embedded.mod index 3d8ba5a1174..3b5970948a5 100644 --- a/jetty-hazelcast/src/main/config/modules/session-store-hazelcast-embedded.mod +++ b/jetty-hazelcast/src/main/config/modules/session-store-hazelcast-embedded.mod @@ -31,7 +31,6 @@ http://www.apache.org/licenses/LICENSE-2.0.html [ini-template] jetty.session.hazelcast.mapName=jetty-distributed-session-map jetty.session.hazelcast.hazelcastInstanceName=JETTY_DISTRIBUTED_SESSION_INSTANCE -#jetty.session.hazelcast.configurationLocation= jetty.session.hazelcast.scavengeZombies=false jetty.session.gracePeriod.seconds=3600 jetty.session.savePeriod.seconds=0 diff --git a/jetty-hazelcast/src/main/config/modules/session-store-hazelcast-remote.mod b/jetty-hazelcast/src/main/config/modules/session-store-hazelcast-remote.mod index e3d67a3d387..9bf4db8f795 100644 --- a/jetty-hazelcast/src/main/config/modules/session-store-hazelcast-remote.mod +++ b/jetty-hazelcast/src/main/config/modules/session-store-hazelcast-remote.mod @@ -34,6 +34,6 @@ jetty.session.hazelcast.mapName=jetty-distributed-session-map jetty.session.hazelcast.hazelcastInstanceName=JETTY_DISTRIBUTED_SESSION_INSTANCE jetty.session.hazelcast.onlyClient=true jetty.session.hazelcast.scavengeZombies=false -#jetty.session.hazelcast.configurationLocation= jetty.session.gracePeriod.seconds=3600 jetty.session.savePeriod.seconds=0 +#jetty.session.hazelcast.addresses= diff --git a/jetty-hazelcast/src/main/java/org/eclipse/jetty/hazelcast/session/HazelcastSessionDataStoreFactory.java b/jetty-hazelcast/src/main/java/org/eclipse/jetty/hazelcast/session/HazelcastSessionDataStoreFactory.java index bc2eea0698e..d637711488f 100644 --- a/jetty-hazelcast/src/main/java/org/eclipse/jetty/hazelcast/session/HazelcastSessionDataStoreFactory.java +++ b/jetty-hazelcast/src/main/java/org/eclipse/jetty/hazelcast/session/HazelcastSessionDataStoreFactory.java @@ -19,6 +19,7 @@ package org.eclipse.jetty.hazelcast.session; import java.io.IOException; +import java.util.Arrays; import com.hazelcast.client.HazelcastClient; import com.hazelcast.client.config.ClientConfig; @@ -29,6 +30,7 @@ import com.hazelcast.config.SerializerConfig; import com.hazelcast.config.XmlConfigBuilder; import com.hazelcast.core.Hazelcast; import com.hazelcast.core.HazelcastInstance; + import org.eclipse.jetty.server.session.AbstractSessionDataStoreFactory; import org.eclipse.jetty.server.session.SessionData; import org.eclipse.jetty.server.session.SessionDataStore; @@ -57,6 +59,8 @@ public class HazelcastSessionDataStoreFactory private boolean scavengeZombies = false; + private String addresses; + public boolean isScavengeZombies() { return scavengeZombies; @@ -81,6 +85,12 @@ public class HazelcastSessionDataStoreFactory if (configurationLocation == null) { ClientConfig config = new ClientConfig(); + + if (addresses != null && !addresses.isEmpty()) + { + config.getNetworkConfig().setAddresses(Arrays.asList(addresses.split(","))); + } + SerializerConfig sc = new SerializerConfig() .setImplementation(new SessionDataSerializer()) .setTypeClass(SessionData.class); @@ -201,4 +211,14 @@ public class HazelcastSessionDataStoreFactory { this.hazelcastInstanceName = hazelcastInstanceName; } + + public String getAddresses() + { + return addresses; + } + + public void setAddresses(String addresses) + { + this.addresses = addresses; + } } From 965483e4d9b4eac40478b28871f6d1e26d333034 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 10 Mar 2020 09:11:22 -0500 Subject: [PATCH 2/2] Issue #4631 - Warning about skipping of nodes is in wrong place for (#4632) * Issue #4631 - Fixing XML comment that was accidentally reformatted Signed-off-by: Joakim Erdfelt * Issue #4631 - Warning about skipping of nodes is in wrong place for Signed-off-by: Joakim Erdfelt * Issue #4631 - Improving testcase Signed-off-by: Joakim Erdfelt * Issue #4631 - Removing test classes Signed-off-by: Joakim Erdfelt * Issue #4631 - Cleaning up configure with index per PR review Signed-off-by: Joakim Erdfelt * Issue #4631 - More named arg test cases Signed-off-by: Joakim Erdfelt * Issue #4631 - Add testConfiguredWithNamedArgNotFirst + new testcase where is needed, but is not the first node Signed-off-by: Joakim Erdfelt * Cleanup configuration index usage Signed-off-by: Greg Wilkins Co-authored-by: Greg Wilkins --- jetty-server/src/main/config/etc/jetty.xml | 23 ++- .../eclipse/jetty/xml/XmlConfiguration.java | 45 +++-- .../jetty/xml/XmlConfigurationTest.java | 190 +++++++++++++++++- 3 files changed, 235 insertions(+), 23 deletions(-) diff --git a/jetty-server/src/main/config/etc/jetty.xml b/jetty-server/src/main/config/etc/jetty.xml index 03e916b9d27..f5a59870251 100644 --- a/jetty-server/src/main/config/etc/jetty.xml +++ b/jetty-server/src/main/config/etc/jetty.xml @@ -1,8 +1,27 @@ - + + + + + + + + + + + + - + + + + + + + + + diff --git a/jetty-xml/src/main/java/org/eclipse/jetty/xml/XmlConfiguration.java b/jetty-xml/src/main/java/org/eclipse/jetty/xml/XmlConfiguration.java index 286e28b8c39..b70b8c3152d 100644 --- a/jetty-xml/src/main/java/org/eclipse/jetty/xml/XmlConfiguration.java +++ b/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 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 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) diff --git a/jetty-xml/src/test/java/org/eclipse/jetty/xml/XmlConfigurationTest.java b/jetty-xml/src/test/java/org/eclipse/jetty/xml/XmlConfigurationTest.java index ff780086d86..280f64467b9 100644 --- a/jetty-xml/src/test/java/org/eclipse/jetty/xml/XmlConfigurationTest.java +++ b/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 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 getZeds() + { + return zeds; + } + + public String getFoo() + { + return foo; + } + } + + @Test + public void testConfiguredWithNamedArg() throws Exception + { + XmlConfiguration xmlFoo = asXmlConfiguration("foo.xml", + "\n" + + " \n" + + " foozball\n" + + " \n" + + ""); + XmlConfiguration xmlBar = asXmlConfiguration("bar.xml", + "\n" + + " \n" + + ""); + + try (StdErrCapture logCapture = new StdErrCapture(XmlConfiguration.class)) + { + Map 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 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", + "\n" + + " \n" + + " foozball\n" + + " \n" + + ""); + XmlConfiguration xmlBar = asXmlConfiguration("bar.xml", + "\n" + + " \n" + // no name specified + ""); + + try (StdErrCapture logCapture = new StdErrCapture(XmlConfiguration.class)) + { + Map 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 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", + "\n" + + " foozball\n" + // wrong name specified + ""); + + 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", + "\n" + + " foozball\n" + + " soccer\n" + // neither should win + ""); + + 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", + "\n" + + " \n" + + " foozball\n" + + " \n" + + ""); + XmlConfiguration xmlBar = asXmlConfiguration("bar.xml", + "\n" + + " \n" + + ""); + XmlConfiguration xmlAddZed = asXmlConfiguration("zed.xml", + "\n" + + " baz\n" + // the invalid line + " \n" + + " plain-zero\n" + + " \n" + + ""); + + try (StdErrCapture logCapture = new StdErrCapture(XmlConfiguration.class)) + { + Map 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 zeds = bar.getZeds(); + assertThat("BarNamed has zeds", zeds, not(empty())); + assertThat("Zeds[0]", zeds.get(0), is("plain-zero")); + + List 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 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 {