From 6849c2137d66b8ff273eb1c77d20dc2937e12c23 Mon Sep 17 00:00:00 2001 From: Darren Foong Date: Sun, 30 Jul 2017 01:52:37 +0800 Subject: [PATCH 1/8] GEODE-3306: Remove whitespace StringBuffers/nodes created by Apache Xerces This commit makes Geode compatible with the official Apache Xerces implementation, which calls `characters()` when it reads ignorable whitespace in `cache.xml`. The while loop is required to handle comments in `cache.xml`, i.e. a comment with whitespace before and after will generate two empty StringBuffers (one for each set of whitespace before and after) on the parse stack. The while loop removes all "consecutive" whitespace StringBuffers from the top of the stack. --- .../cache/xmlcache/CacheXmlParser.java | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParser.java b/geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParser.java index b1d292933cab..130efb897c94 100644 --- a/geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParser.java +++ b/geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParser.java @@ -2596,6 +2596,18 @@ private void endDeclarable() { public void startElement(String namespaceURI, String localName, String qName, Attributes atts) throws SAXException { + // This while loop pops all StringBuffers at the top of the stack + // that contain only whitespace; see GEODE-3306 + while (!stack.empty()) { + Object o = stack.peek(); + if (o instanceof StringBuffer + && ((StringBuffer) o).toString().replaceAll("\\s", "").equals("")) { + stack.pop(); + } else { + break; + } + } + if (qName.equals(CACHE)) { startCache(atts); } else if (qName.equals(CLIENT_CACHE)) { @@ -2872,6 +2884,18 @@ private void startDiskWriteAttributes(Attributes atts) { } public void endElement(String namespaceURI, String localName, String qName) throws SAXException { + // This while loop pops all StringBuffers at the top of the stack + // that contain only whitespace; see GEODE-3306 + while (!stack.empty()) { + Object o = stack.peek(); + if (o instanceof StringBuffer + && ((StringBuffer) o).toString().replaceAll("\\s", "").equals("")) { + stack.pop(); + } else { + break; + } + } + try { // logger.debug("endElement namespaceURI=" + namespaceURI // + "; localName = " + localName + "; qName = " + qName); From 6dd702cc407eb460c92b71102443f2d622d0e058 Mon Sep 17 00:00:00 2001 From: Darren Foong Date: Thu, 3 Aug 2017 21:16:08 +0800 Subject: [PATCH 2/8] GEODE-3306: Add unit tests Modified existing `testCacheXmlParserWithSimplePool()` to be able to close the created cache, otherwise there will be an IllegalStateException thrown about an "incomplete shutdown of a previous cache". --- geode-core/build.gradle | 2 ++ .../xmlcache/CacheXmlParserJUnitTest.java | 34 +++++++++++++++++-- gradle/dependency-versions.properties | 1 + 3 files changed, 35 insertions(+), 2 deletions(-) diff --git a/geode-core/build.gradle b/geode-core/build.gradle index 9ecb0f9cca43..1017e3d81018 100755 --- a/geode-core/build.gradle +++ b/geode-core/build.gradle @@ -142,6 +142,8 @@ dependencies { testCompile 'com.pholser:junit-quickcheck-core:' + project.'junit-quickcheck.version' testCompile 'com.pholser:junit-quickcheck-generators:' + project.'junit-quickcheck.version' testCompile 'com.pholser:junit-quickcheck-guava:' + project.'junit-quickcheck.version' + + testRuntime 'xerces:xercesImpl:' + project.'xercesImpl.version' } def generatedResources = "$buildDir/generated-resources/main" diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java b/geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java index 4043888561d4..da67a9870ae8 100644 --- a/geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java +++ b/geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java @@ -23,6 +23,8 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import org.apache.geode.cache.Cache; +import org.apache.geode.cache.CacheFactory; import org.apache.geode.distributed.internal.DM; import org.apache.geode.distributed.internal.InternalDistributedSystem; import org.apache.geode.test.junit.categories.UnitTest; @@ -113,8 +115,23 @@ public void testCacheXmlParserWithSimplePool() { when(dm.getSystem()).thenReturn(system); InternalDistributedSystem.connect(nonDefault); - CacheXmlParser.parse(getClass() - .getResourceAsStream("CacheXmlParserJUnitTest.testSimpleClientCacheXml.cache.xml")); + Cache cache = new CacheFactory() + .set("cache-xml-file", "CacheXmlParserJUnitTest.testSimpleClientCacheXml.cache.xml") + .create(InternalDistributedSystem.connect(nonDefault)); + cache.close(); + } + + /** + * Test that {@link CacheXmlParser} can parse the test cache.xml file, using the Apache Xerces XML + * parser. + * + * @since Geode 1.3 + */ + @Test + public void testCacheXmlParserWithSimplePoolXerces() { + System.setProperty("javax.xml.parsers.SAXParserFactory", + "org.apache.xerces.jaxp.SAXParserFactoryImpl"); + testCacheXmlParserWithSimplePool(); } /** @@ -137,6 +154,19 @@ public void testDTDFallbackWithNonEnglishLocal() { } } + /** + * Test that {@link CacheXmlParser} falls back to DTD parsing when locale language is not English, + * using the Apache Xerces XML parser. + * + * @since Geode 1.3 + */ + @Test + public void testDTDFallbackWithNonEnglishLocalXerces() { + System.setProperty("javax.xml.parsers.SAXParserFactory", + "org.apache.xerces.jaxp.SAXParserFactoryImpl"); + testDTDFallbackWithNonEnglishLocal(); + } + /** * Get access to {@link CacheXmlParser} protected methods and fields. * diff --git a/gradle/dependency-versions.properties b/gradle/dependency-versions.properties index 4dd3f3427ad7..650d1b4cda2e 100644 --- a/gradle/dependency-versions.properties +++ b/gradle/dependency-versions.properties @@ -99,3 +99,4 @@ tomcat6.version = 6.0.37 tomcat7.version = 7.0.73 tomcat8.version = 8.5.9 commons-validator.version = 1.6 +xercesImpl.version = 2.11.0 From 0a9c3181e78d8767198a0531ad0e9189cc43423a Mon Sep 17 00:00:00 2001 From: Darren Foong Date: Thu, 3 Aug 2017 23:38:23 +0800 Subject: [PATCH 3/8] GEODE-3306: Remove duplicate call to connect() --- .../geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java b/geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java index da67a9870ae8..ccc7432709ec 100644 --- a/geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java +++ b/geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java @@ -113,7 +113,6 @@ public void testCacheXmlParserWithSimplePool() { InternalDistributedSystem system = InternalDistributedSystem.newInstanceForTesting(dm, nonDefault); when(dm.getSystem()).thenReturn(system); - InternalDistributedSystem.connect(nonDefault); Cache cache = new CacheFactory() .set("cache-xml-file", "CacheXmlParserJUnitTest.testSimpleClientCacheXml.cache.xml") From 64d46b0a6ca5073fb7bdfabf57a13eb1d71b2b50 Mon Sep 17 00:00:00 2001 From: Darren Foong Date: Fri, 4 Aug 2017 00:12:53 +0800 Subject: [PATCH 4/8] GEODE 3306: Unset system properties after parsing --- .../xmlcache/CacheXmlParserJUnitTest.java | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java b/geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java index ccc7432709ec..97f15a516168 100644 --- a/geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java +++ b/geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java @@ -128,9 +128,16 @@ public void testCacheXmlParserWithSimplePool() { */ @Test public void testCacheXmlParserWithSimplePoolXerces() { - System.setProperty("javax.xml.parsers.SAXParserFactory", + String prevParserFactory = System.setProperty("javax.xml.parsers.SAXParserFactory", "org.apache.xerces.jaxp.SAXParserFactoryImpl"); + testCacheXmlParserWithSimplePool(); + + if (prevParserFactory != null) { + System.setProperty("javax.xml.parsers.SAXParserFactory", prevParserFactory); + } else { + System.clearProperty("javax.xml.parsers.SAXParserFactory"); + } } /** @@ -161,9 +168,16 @@ public void testDTDFallbackWithNonEnglishLocal() { */ @Test public void testDTDFallbackWithNonEnglishLocalXerces() { - System.setProperty("javax.xml.parsers.SAXParserFactory", + String prevParserFactory = System.setProperty("javax.xml.parsers.SAXParserFactory", "org.apache.xerces.jaxp.SAXParserFactoryImpl"); + testDTDFallbackWithNonEnglishLocal(); + + if (prevParserFactory != null) { + System.setProperty("javax.xml.parsers.SAXParserFactory", prevParserFactory); + } else { + System.clearProperty("javax.xml.parsers.SAXParserFactory"); + } } /** From 8d924ba82563989a7cbb09a04f23a42a62a1953b Mon Sep 17 00:00:00 2001 From: Darren Foong Date: Thu, 17 Aug 2017 19:43:12 +0800 Subject: [PATCH 5/8] GEODE-3306: Use StringUtils.isBlank() --- .../geode/internal/cache/xmlcache/CacheXmlParser.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParser.java b/geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParser.java index 130efb897c94..0d9397f0eaaf 100644 --- a/geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParser.java +++ b/geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParser.java @@ -37,6 +37,7 @@ import org.apache.geode.cache.util.GatewayConflictResolver; import org.apache.logging.log4j.Logger; +import org.apache.commons.lang.StringUtils; import org.xml.sax.Attributes; import org.xml.sax.ContentHandler; import org.xml.sax.InputSource; @@ -2600,8 +2601,7 @@ public void startElement(String namespaceURI, String localName, String qName, At // that contain only whitespace; see GEODE-3306 while (!stack.empty()) { Object o = stack.peek(); - if (o instanceof StringBuffer - && ((StringBuffer) o).toString().replaceAll("\\s", "").equals("")) { + if (o instanceof StringBuffer && StringUtils.isBlank(((StringBuffer) o).toString())) { stack.pop(); } else { break; @@ -2888,8 +2888,7 @@ public void endElement(String namespaceURI, String localName, String qName) thro // that contain only whitespace; see GEODE-3306 while (!stack.empty()) { Object o = stack.peek(); - if (o instanceof StringBuffer - && ((StringBuffer) o).toString().replaceAll("\\s", "").equals("")) { + if (o instanceof StringBuffer && StringUtils.isBlank(((StringBuffer) o).toString())) { stack.pop(); } else { break; From 0fb6ea266da4283782bb4b4d0cb89fb08611d34a Mon Sep 17 00:00:00 2001 From: Darren Foong Date: Thu, 17 Aug 2017 20:38:48 +0800 Subject: [PATCH 6/8] GEODE-3306: Use RestoreSystemProperties() in unit tests --- .../cache/xmlcache/CacheXmlParserJUnitTest.java | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java b/geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java index 97f15a516168..2d79a9b5ac8c 100644 --- a/geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java +++ b/geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java @@ -29,7 +29,9 @@ import org.apache.geode.distributed.internal.InternalDistributedSystem; import org.apache.geode.test.junit.categories.UnitTest; import org.junit.After; +import org.junit.Rule; import org.junit.Test; +import org.junit.contrib.java.lang.system.RestoreSystemProperties; import org.junit.experimental.categories.Category; import org.xml.sax.Attributes; import org.xml.sax.SAXException; @@ -49,6 +51,9 @@ @Category(UnitTest.class) public class CacheXmlParserJUnitTest { + @Rule + public final RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties(); + private static final String NAMESPACE_URI = "urn:java:org.apache.geode.internal.cache.xmlcache.MockXmlParser"; @@ -132,12 +137,6 @@ public void testCacheXmlParserWithSimplePoolXerces() { "org.apache.xerces.jaxp.SAXParserFactoryImpl"); testCacheXmlParserWithSimplePool(); - - if (prevParserFactory != null) { - System.setProperty("javax.xml.parsers.SAXParserFactory", prevParserFactory); - } else { - System.clearProperty("javax.xml.parsers.SAXParserFactory"); - } } /** @@ -172,12 +171,6 @@ public void testDTDFallbackWithNonEnglishLocalXerces() { "org.apache.xerces.jaxp.SAXParserFactoryImpl"); testDTDFallbackWithNonEnglishLocal(); - - if (prevParserFactory != null) { - System.setProperty("javax.xml.parsers.SAXParserFactory", prevParserFactory); - } else { - System.clearProperty("javax.xml.parsers.SAXParserFactory"); - } } /** From 0c2a5b8d924e574d722820c0045f41a75aa7f171 Mon Sep 17 00:00:00 2001 From: Darren Foong Date: Thu, 17 Aug 2017 21:32:23 +0800 Subject: [PATCH 7/8] GEODE-3306: Remove unnecessary variable assignment --- .../internal/cache/xmlcache/CacheXmlParserJUnitTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java b/geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java index 2d79a9b5ac8c..19a3546616df 100644 --- a/geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java +++ b/geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java @@ -133,7 +133,7 @@ public void testCacheXmlParserWithSimplePool() { */ @Test public void testCacheXmlParserWithSimplePoolXerces() { - String prevParserFactory = System.setProperty("javax.xml.parsers.SAXParserFactory", + System.setProperty("javax.xml.parsers.SAXParserFactory", "org.apache.xerces.jaxp.SAXParserFactoryImpl"); testCacheXmlParserWithSimplePool(); @@ -167,7 +167,7 @@ public void testDTDFallbackWithNonEnglishLocal() { */ @Test public void testDTDFallbackWithNonEnglishLocalXerces() { - String prevParserFactory = System.setProperty("javax.xml.parsers.SAXParserFactory", + System.setProperty("javax.xml.parsers.SAXParserFactory", "org.apache.xerces.jaxp.SAXParserFactoryImpl"); testDTDFallbackWithNonEnglishLocal(); From 102c89f0c66967257496307018454548ab87285d Mon Sep 17 00:00:00 2001 From: Darren Foong Date: Fri, 18 Aug 2017 02:54:02 +0800 Subject: [PATCH 8/8] GEODE-3306: Fix unit test Remove previous setup code for `parse()`, which is already called in `CacheFactory.create()`. The end result is a simpler `CacheFactory.create()` call. The path of `cache.xml` is different because the `GemFireCacheImpl` class is the one that retrieves `cache.xml`, but its package is `org.apache.geode.internal.cache` and not `org.apache.geode.internal.cache.xmlcache` (where this class resides). Hence the additional `xmlcache` is needed for `GemFireCacheImpl` to find the resource for the unit test. --- .../cache/xmlcache/CacheXmlParserJUnitTest.java | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java b/geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java index 19a3546616df..72320d4c704b 100644 --- a/geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java +++ b/geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java @@ -23,8 +23,8 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; -import org.apache.geode.cache.Cache; -import org.apache.geode.cache.CacheFactory; +import org.apache.geode.cache.client.ClientCache; +import org.apache.geode.cache.client.ClientCacheFactory; import org.apache.geode.distributed.internal.DM; import org.apache.geode.distributed.internal.InternalDistributedSystem; import org.apache.geode.test.junit.categories.UnitTest; @@ -110,18 +110,11 @@ public void testCacheXmlParserWithSimplePool() { assertNotNull("Did not find simple config.xml file", getClass() .getResourceAsStream("CacheXmlParserJUnitTest.testSimpleClientCacheXml.cache.xml")); - DM dm = mock(DM.class); - Properties nonDefault = new Properties(); nonDefault.setProperty(MCAST_PORT, "0"); // loner - InternalDistributedSystem system = - InternalDistributedSystem.newInstanceForTesting(dm, nonDefault); - when(dm.getSystem()).thenReturn(system); - - Cache cache = new CacheFactory() - .set("cache-xml-file", "CacheXmlParserJUnitTest.testSimpleClientCacheXml.cache.xml") - .create(InternalDistributedSystem.connect(nonDefault)); + ClientCache cache = new ClientCacheFactory(nonDefault).set("cache-xml-file", + "xmlcache/CacheXmlParserJUnitTest.testSimpleClientCacheXml.cache.xml").create(); cache.close(); }