You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by darrenfoong <gi...@git.apache.org> on 2017/07/29 18:04:18 UTC
[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 <da...@gmail.com>
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #668: GEODE-3306: Remove whitespace StringBuffers/nodes created ...
Posted by darrenfoong <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #668: GEODE-3306: Remove whitespace StringBuffers/nodes created ...
Posted by darrenfoong <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #668: GEODE-3306: Remove whitespace StringBuffers/nodes c...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/geode/pull/668
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #668: GEODE-3306: Remove whitespace StringBuffers/nodes created ...
Posted by jaredjstewart <gi...@git.apache.org>.
Github user jaredjstewart commented on the issue:
https://github.com/apache/geode/pull/668
Hi Darren,
It looks like `xercesImpl` may need to be declared as a `testCompile` dependency rather than `testRuntime` in case people are building with a JDK which does not include Xerxes. I also think it would be prudent to solicit feedback from the community via dev@geode.apache.org before we add this library, since I know that Xerces can sometimes be troublesome. (See e.g. https://stackoverflow.com/questions/11677572/dealing-with-xerces-hell-in-java-maven)
Thank you,
Jared Stewart
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #668: GEODE-3306: Remove whitespace StringBuffers/nodes c...
Posted by darrenfoong <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #668: GEODE-3306: Remove whitespace StringBuffers/nodes c...
Posted by jaredjstewart <gi...@git.apache.org>.
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/668#discussion_r131968121
--- 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 --
This cleanup of system properties can be made simpler by using the `RestoreSystemProperties` JUnit rule. All you need to do is add this member variable to your test class:
``` @Rule
public final RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties();
```
(For an example, see [LocatorLauncherTest](https://github.com/apache/geode/blob/d1db2f02d2ce45a437b34488934e5b1d53c7b5ca/geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherTest.java).)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #668: GEODE-3306: Remove whitespace StringBuffers/nodes created ...
Posted by darrenfoong <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #668: GEODE-3306: Remove whitespace StringBuffers/nodes c...
Posted by jaredjstewart <gi...@git.apache.org>.
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/668#discussion_r131966460
--- Diff: 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",
--- End diff --
You can make this cleanup of the system properties more robust by using the `RestoreSystemProperties` JUnit rule. All you need to do is add this as a member variable of the test class:
```
@Rule
public final RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties();
```
(For an example see [LocatorLauncherTest](https://github.com/apache/geode/blob/d1db2f02d2ce45a437b34488934e5b1d53c7b5ca/geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherTest.java).)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #668: GEODE-3306: Remove whitespace StringBuffers/nodes created ...
Posted by darrenfoong <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #668: GEODE-3306: Remove whitespace StringBuffers/nodes c...
Posted by jaredjstewart <gi...@git.apache.org>.
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/668#discussion_r131967266
--- Diff: 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("")) {
--- End diff --
I think `StringUtils.isBlank( (StringBuffer o).toString())` might be simpler here as well as in `endElement`.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---