[GitHub] geode pull request #668: GEODE-3306: Remove whitespace StringBuffers/nodes c...
GitHub user darrenfoong opened a pull request: https://github.com/apache/geode/pull/668 GEODE-3306: Remove whitespace StringBuffers/nodes created by Apache X⦠â¦erces 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. --- Tested with https://github.com/darrenfoong/geode-parser-poc/blob/master/src/test/java/server/ServerTest.java --- Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [x] Is your initial contribution a single, squashed commit? - [x] Does `gradlew build` run cleanly? - [ ] Have you written or updated unit tests to verify your changes? - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? ### Note: Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to dev@geode.apache.org. You can merge this pull request into a Git repository by running: $ git pull https://github.com/darrenfoong/geode df-GEODE-3306 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/668.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #668 commit d742c9bb2dc672be4ec01a98423989795748f0d8 Author: Darren Foong Date: 2017-07-29T17:52:37Z 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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #668: GEODE-3306: Remove whitespace StringBuffers/nodes created ...
Github user darrenfoong commented on the issue: https://github.com/apache/geode/pull/668 I just realised I'll need to unset the property to prevent any side effects in the other tests; working on it now. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #668: GEODE-3306: Remove whitespace StringBuffers/nodes created ...
Github user darrenfoong commented on the issue: https://github.com/apache/geode/pull/668 I've added code to unset/clear the temporarily-set system properties for testing. Thank you Kenneth for your feedback. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #668: GEODE-3306: Remove whitespace StringBuffers/nodes created ...
Github user darrenfoong commented on the issue: https://github.com/apache/geode/pull/668 Hi Jared, thank you for your feedback. The initial use case that I was thinking of involved a user wanting to: - use Geode with another JDK (which doesn't have the `com.sun.org.apache...` Xerces implementation that the Oracle JDK uses), or - use Geode in an application where he/she wants to use the Apache Xerces implementation (which will be a dependency of the application) and sets the system property `javax.xml.parsers.SAXParserFactory` to `org.apache.xerces.jaxp.SAXParserFactoryImpl`. In these cases `xercesImpl` is part of the environment (JDK, app) so I chose to use `xercesImpl` at only test runtime and "load" it via `System.setProperty()` in my unit tests. I don't see why it's needed at test compile time and I don't really understand your point about people building with (and I presume, for) a JDK that doesn't include Xerces: in that case, shouldn't `xercesImpl` be a dependency for `main` too? I do symphatise with Xerces hell though! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #668: GEODE-3306: Remove whitespace StringBuffers/nodes c...
Github user darrenfoong commented on a diff in the pull request:
https://github.com/apache/geode/pull/668#discussion_r132833299
--- Diff:
geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java
---
@@ -111,10 +113,31 @@ public void testCacheXmlParserWithSimplePool() {
InternalDistributedSystem system =
InternalDistributedSystem.newInstanceForTesting(dm, nonDefault);
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() {
+String prevParserFactory =
System.setProperty("javax.xml.parsers.SAXParserFactory",
+"org.apache.xerces.jaxp.SAXParserFactoryImpl");
+
+testCacheXmlParserWithSimplePool();
+
+if (prevParserFactory != null) {
--- End diff --
Thanks Jared! Will find time to make the changes and get feedback on the
mailing list.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #668: GEODE-3306: Remove whitespace StringBuffers/nodes created ...
Github user darrenfoong commented on the issue: https://github.com/apache/geode/pull/668 Thanks Jared! Will find time to make the changes and get feedback on the mailing list. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
