You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by aledsage <gi...@git.apache.org> on 2016/06/22 15:48:58 UTC
[GitHub] brooklyn-server pull request #214: BROOKLYN-305: handle invalid xml chars in...
GitHub user aledsage opened a pull request:
https://github.com/apache/brooklyn-server/pull/214
BROOKLYN-305: handle invalid xml chars in attribute vals
I'm not convinced by this approach (calling a special method `XmlUtil.xpathHandlingIllegalChars()`, which retries the xpath having escaped the illegal characters). It works well enough for us because we are just making very basic use of xpath - it is primarily to just extact the entity id from the xml file, before we try to fully deserialize it.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/brooklyn-server fix/xml-deserialize-illegal-chars
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/214.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 #214
----
commit e15697f42eb103fb393ab391f53bcd1d7ebd4d38
Author: Aled Sage <al...@gmail.com>
Date: 2016-06-22T12:24:01Z
Improve/test performance of XmlUtil.xpath
commit 7981326612d20a0d6bd9b7d972620f5e2e0b4fc3
Author: Aled Sage <al...@gmail.com>
Date: 2016-06-22T12:24:30Z
Adds XmlSerializerTest
commit 17d496a9d73f2079349ba01cc8f296fa303efc40
Author: Aled Sage <al...@gmail.com>
Date: 2016-06-22T14:09:02Z
BROOKLYN-305: Handle invalid xml chars in attribute vals
----
---
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] brooklyn-server issue #214: BROOKLYN-305: handle invalid xml chars in attrib...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/214
Thanks @duncangrant - merging now.
We can later look at including in our persisted state:
<?xml version=\"1.1\" encoding=\"UTF-8\"?>
But a very quick test gave the error below, which I still need to look into:
com.thoughtworks.xstream.io.StreamException: : only 1.0 is supported as <?xml version not '1.1' (position: START_DOCUMENT seen <?xml version="1.1"... @1:19)
---
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] brooklyn-server pull request #214: BROOKLYN-305: handle invalid xml chars in...
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/214#discussion_r68080375
--- Diff: core/src/main/java/org/apache/brooklyn/core/mgmt/persist/BrooklynMementoPersisterToObjectStore.java ---
@@ -294,7 +294,7 @@ public void visit(BrooklynObjectType type, String id, String contentsSubpath) th
exceptionHandler.onLoadMementoFailed(type, "memento "+id+" read error", e);
}
- String xmlId = (String) XmlUtil.xpath(contents, "/"+type.toCamelCase()+"/id");
+ String xmlId = (String) XmlUtil.xpathHandlingIllegalChars(contents, "/"+type.toCamelCase()+"/id");
--- End diff --
Would `xpathHandlingIllegalChars` escape mechanism will be appropariate for filenames?
Ideally if a function like `File.escapeSpecialChars` exists then this should look like `File.escapeSpecialChars(XmlUtil.xpathHandlingIllegalChars(contents, "/"+type.toCamelCase()+"/id"))`
---
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] brooklyn-server pull request #214: BROOKLYN-305: handle invalid xml chars in...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/214
---
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] brooklyn-server pull request #214: BROOKLYN-305: handle invalid xml chars in...
Posted by duncangrant <gi...@git.apache.org>.
Github user duncangrant commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/214#discussion_r68134027
--- Diff: core/src/test/java/org/apache/brooklyn/util/core/xstream/XmlUtilTest.java ---
@@ -20,15 +20,65 @@
import static org.testng.Assert.assertEquals;
-import org.apache.brooklyn.util.core.xstream.XmlUtil;
+import org.apache.brooklyn.util.core.xstream.XmlUtil.Escaper;
import org.testng.annotations.Test;
+import com.google.common.base.Optional;
+
public class XmlUtilTest {
@Test
public void testXpath() throws Exception {
String xml = "<a><b>myb</b></a>";
- assertEquals(XmlUtil.xpath(xml, "/a/b[text()]"), "myb");
+ for (int i = 0; i < 2; i++) {
--- End diff --
Why twice?
---
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] brooklyn-server issue #214: BROOKLYN-305: handle invalid xml chars in attrib...
Posted by duncangrant <gi...@git.apache.org>.
Github user duncangrant commented on the issue:
https://github.com/apache/brooklyn-server/pull/214
LGTM
---
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] brooklyn-server pull request #214: BROOKLYN-305: handle invalid xml chars in...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/214#discussion_r68202316
--- Diff: core/src/test/java/org/apache/brooklyn/util/core/xstream/XmlUtilTest.java ---
@@ -20,15 +20,65 @@
import static org.testng.Assert.assertEquals;
-import org.apache.brooklyn.util.core.xstream.XmlUtil;
+import org.apache.brooklyn.util.core.xstream.XmlUtil.Escaper;
import org.testng.annotations.Test;
+import com.google.common.base.Optional;
+
public class XmlUtilTest {
@Test
public void testXpath() throws Exception {
String xml = "<a><b>myb</b></a>";
- assertEquals(XmlUtil.xpath(xml, "/a/b[text()]"), "myb");
+ for (int i = 0; i < 2; i++) {
--- End diff --
One of the commits changes `XmlUtil` to use thread-local storage (rather than re-creating the `DocumentBuilder` on every invocation). By calling it twice, we confirm that thread-local storage isn't messing things up (e.g. if you removed the `documentBuilder.reset()` then it would probably break).
---
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] brooklyn-server pull request #214: BROOKLYN-305: handle invalid xml chars in...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/214#discussion_r68202480
--- Diff: core/src/main/java/org/apache/brooklyn/core/mgmt/persist/BrooklynMementoPersisterToObjectStore.java ---
@@ -294,7 +294,7 @@ public void visit(BrooklynObjectType type, String id, String contentsSubpath) th
exceptionHandler.onLoadMementoFailed(type, "memento "+id+" read error", e);
}
- String xmlId = (String) XmlUtil.xpath(contents, "/"+type.toCamelCase()+"/id");
+ String xmlId = (String) XmlUtil.xpathHandlingIllegalChars(contents, "/"+type.toCamelCase()+"/id");
--- End diff --
We're making the assumption that the `id` field etc won't have crazy characters in it (because the Brooklyn framework generates those ids). The place we can get crazy characters is in attributes and config values that are set by the user.
---
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.
---