GEODE-3306: Remove whitespace StringBuffers/nodes created by Apache X… #668
Conversation
…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.
darrenfoong
force-pushed
the
df-GEODE-3306
branch
from
d742c9b
to
6849c21
Compare
There was a problem hiding this comment.
Darren, thank you for your contribution.
The product code changes look OK, but per the project guidelines, the tests for your changes should be incorporated into the project as unit tests - same package declaration as the changed file but put the test class under the 'geode-core/src/testjava' directory. (See the Geode wiki: https://cwiki.apache.org/confluence/display/GEODE/Criteria+for+Code+Submissions and https://cwiki.apache.org/confluence/display/GEODE/Writing+tests.) In particular, note the distinction between unit tests, integration tests (one JVM, but touches filesystem or other external resource), and distributed unit tests (multiple JVMs). If you mock your test XMLs, add the tests as unit tests (preferred), otherwise if you create test files to read in then they would be Integration tests.
darrenfoong
force-pushed
the
df-GEODE-3306
branch
from
fabbe2b
to
6764003
Compare
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".
darrenfoong
force-pushed
the
df-GEODE-3306
branch
from
6764003
to
6dd702c
Compare
|
I just realised I'll need to unset the property to prevent any side effects in the other tests; working on it now. |
darrenfoong
force-pushed
the
df-GEODE-3306
branch
from
08dc6d7
to
64d46b0
Compare
|
I've added code to unset/clear the temporarily-set system properties for testing. Thank you Kenneth for your feedback. |
|
Hi Darren, It looks like Thank you, |
|
Hi Jared, thank you for your feedback. The initial use case that I was thinking of involved a user wanting to:
In these cases 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 I do symphatise with Xerces hell though! |
There was a problem hiding this comment.
I think you may be right about using testRuntime for xercesImpl instead of testCompile. The first time I tried to run the test with your PR I think I forgot to refresh Gradle first, which led me to believe testRuntime was not sufficient.
I still think it would be a good idea to email dev@geode.apache.org to ensure that the community has no objections to adding this dependency.
| while (!stack.empty()) { | ||
| Object o = stack.peek(); | ||
| if (o instanceof StringBuffer | ||
| && ((StringBuffer) o).toString().replaceAll("\\s", "").equals("")) { |
There was a problem hiding this comment.
I think StringUtils.isBlank( (StringBuffer o).toString()) might be simpler here as well as in endElement.
|
|
||
| testCacheXmlParserWithSimplePool(); | ||
|
|
||
| if (prevParserFactory != null) { |
There was a problem hiding this comment.
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:
public final RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties();
(For an example, see LocatorLauncherTest.)
|
Thanks Jared! Will find time to make the changes and get feedback on the mailing list. |
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.
…erces
This commit makes Geode compatible with the official Apache Xerces
implementation, which calls
characters()when it reads ignorablewhitespace 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:
Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
Has your PR been rebased against the latest commit within the target branch (typically
develop)?Is your initial contribution a single, squashed commit?
Does
gradlew buildrun 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?
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.