You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by justinchuch <gi...@git.apache.org> on 2018/09/21 19:30:23 UTC
[GitHub] tinkerpop pull request #934: Apply StringEscapeUtils in GroovyTranslator#con...
GitHub user justinchuch opened a pull request:
https://github.com/apache/tinkerpop/pull/934
Apply StringEscapeUtils in GroovyTranslator#convertToString(String)
Preface:
There's character escaping issue when translating.
What's changed:
- `org.apache.tinkerpop.gremlin.groovy.jsr223.GroovyTranslator#convertToString`
Apply `StringEscapeUtils.escapeJava(String)` for String object.
- `org.apache.tinkerpop.gremlin.groovy.jsr223.GroovyTranslatorTest`
Add a few test cases to verify the changes.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/justinchuch/tinkerpop groovy-translator-tp32
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tinkerpop/pull/934.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 #934
----
commit 3416e52db8c2f81239a774e2dd17b09a9e471b14
Author: Justin Chu <15...@...>
Date: 2018-09-21T19:12:26Z
Apply StringEscapeUtils in GroovyTranslator#convertToString(String)
----
---
[GitHub] tinkerpop pull request #934: Apply StringEscapeUtils in GroovyTranslator#con...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/tinkerpop/pull/934
---
[GitHub] tinkerpop issue #934: Apply StringEscapeUtils in GroovyTranslator#convertToS...
Posted by robertdale <gi...@git.apache.org>.
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/934
VOTE +1
---
[GitHub] tinkerpop issue #934: Apply StringEscapeUtils in GroovyTranslator#convertToS...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/934
Changes and tests look good to me.
VOTE +1
---
[GitHub] tinkerpop pull request #934: Apply StringEscapeUtils in GroovyTranslator#con...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/934#discussion_r219606770
--- Diff: gremlin-groovy-test/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GroovyTranslatorTest.java ---
@@ -141,4 +144,52 @@ public void shouldHandleEmptyMaps() {
public void shouldHaveValidToString() {
assertEquals("translator[h:gremlin-groovy]", GroovyTranslator.of("h").toString());
}
+
+ @Test
+ @LoadGraphWith(LoadGraphWith.GraphData.MODERN)
--- End diff --
It doesn't look like you need the "modern" graph data here - you can remove this annotation.
---
[GitHub] tinkerpop pull request #934: Apply StringEscapeUtils in GroovyTranslator#con...
Posted by justinchuch <gi...@git.apache.org>.
Github user justinchuch commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/934#discussion_r219678394
--- Diff: gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GroovyTranslator.java ---
@@ -115,7 +116,8 @@ else if (object instanceof Bytecode)
else if (object instanceof Traversal)
return convertToString(((Traversal) object).asAdmin().getBytecode());
else if (object instanceof String) {
- return (((String) object).contains("\"") ? "\"\"\"" + object + "\"\"\"" : "\"" + object + "\"").replace("$", "\\$");
+ return (((String) object).contains("\"") ? "\"\"\"" + StringEscapeUtils.escapeJava((String) object) + "\"\"\"" : "\"" + StringEscapeUtils.escapeJava((String) object) + "\"")
+ .replace("$", "\\$");
--- End diff --
The final `replace("$", "\\$")` has to be preserved or else some other original tests will be failed.
---
[GitHub] tinkerpop issue #934: Apply StringEscapeUtils in GroovyTranslator#convertToS...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/934
VOTE +1
---
[GitHub] tinkerpop pull request #934: Apply StringEscapeUtils in GroovyTranslator#con...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/934#discussion_r219606369
--- Diff: gremlin-groovy-test/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GroovyTranslatorTest.java ---
@@ -141,4 +144,52 @@ public void shouldHandleEmptyMaps() {
public void shouldHaveValidToString() {
assertEquals("translator[h:gremlin-groovy]", GroovyTranslator.of("h").toString());
}
+
+ @Test
+ @LoadGraphWith(LoadGraphWith.GraphData.MODERN)
+ public void shouldHandleTraversalAddV() {
+ final GraphTraversalSource g = graph.traversal();
+ final String script = GroovyTranslator.of("g").translate(g.addV("customer")
+ .property("customer_id", 501L)
+ .property("name", "Foo\u0020Bar")
+ .property("salary", "\u0024\u00201000")
+ .property("age", 25)
+ .asAdmin().getBytecode());
+
+ assertEquals("g.addV(\"customer\")" +
+ ".property(\"customer_id\",501L)" +
+ ".property(\"name\",\"Foo\u0020Bar\")" +
+ ".property(\"salary\",\"\u005c\u005c\u0024\u00201000\")" +
+ ".property(\"age\",(int) 25)",
+ script);
+ }
+
+ @Test
+ @LoadGraphWith(LoadGraphWith.GraphData.MODERN)
+ public void shouldHandleVertex() {
+ final GraphTraversalSource g = graph.traversal();
+
+ final String label1 = "customer";
+ final Object id1 = "name:20:foo\u0020bar#50";
+
+ final Vertex vertex1 = DetachedVertex.build().setLabel(label1).setId(id1).create();
+ final String script1 = GroovyTranslator.of("g").translate(g.V().inject(vertex1).asAdmin().getBytecode());
+ assertEquals("g.V().inject(new org.apache.tinkerpop.gremlin.structure.util.detached.DetachedVertex(\"name:20:foo bar#50\",\"customer\", Collections.emptyMap()))", script1);
+
+ final String label2 = "user";
+ final Object id2 = "name:20:foo\\u0020bar#50";
+
+ final Vertex vertex2 = DetachedVertex.build().setLabel(label2).setId(id2).create();
+ final String script2 = GroovyTranslator.of("g").translate(g.V().inject(vertex2).asAdmin().getBytecode());
+ assertEquals("g.V().inject(new org.apache.tinkerpop.gremlin.structure.util.detached.DetachedVertex(\"name:20:foo\\\\u0020bar#50\",\"user\", Collections.emptyMap()))", script2);
+ }
+
+ @Test
+ @LoadGraphWith(LoadGraphWith.GraphData.MODERN)
+ public void shouldHandleLambda() {
--- End diff --
is this doing anything discernibly different than `shouldSupportStringSupplierLambdas()` test above? if there is a specific you are trying to assert here, perhaps it could be just appended into that test?
---
[GitHub] tinkerpop pull request #934: Apply StringEscapeUtils in GroovyTranslator#con...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/934#discussion_r219608379
--- Diff: gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GroovyTranslator.java ---
@@ -115,7 +116,8 @@ else if (object instanceof Bytecode)
else if (object instanceof Traversal)
return convertToString(((Traversal) object).asAdmin().getBytecode());
else if (object instanceof String) {
- return (((String) object).contains("\"") ? "\"\"\"" + object + "\"\"\"" : "\"" + object + "\"").replace("$", "\\$");
+ return (((String) object).contains("\"") ? "\"\"\"" + StringEscapeUtils.escapeJava((String) object) + "\"\"\"" : "\"" + StringEscapeUtils.escapeJava((String) object) + "\"")
+ .replace("$", "\\$");
--- End diff --
since you used the "groovy" package for this, i was wondering if it still handles the "$" properly or if we still need the final `replace("$", "\\$")`? either way, could you please modify your tests to include a "$" character so that we can be sure that gets asserted?
---
[GitHub] tinkerpop pull request #934: Apply StringEscapeUtils in GroovyTranslator#con...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/934#discussion_r219607573
--- Diff: gremlin-groovy-test/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GroovyTranslatorTest.java ---
@@ -141,4 +144,52 @@ public void shouldHandleEmptyMaps() {
public void shouldHaveValidToString() {
assertEquals("translator[h:gremlin-groovy]", GroovyTranslator.of("h").toString());
}
+
+ @Test
+ @LoadGraphWith(LoadGraphWith.GraphData.MODERN)
+ public void shouldHandleTraversalAddV() {
--- End diff --
the names of our tests are meant to convey information about what is being tested (when possible). to that end, you're not really testing "addV()" here but you're testing "string escaping", so something more like `shouldEscapeStrings()` would probably be a better name. I would probably combine this test with `shouldHandleVertex()` as they are both validating the "escaping" but if you have a better name for that test as it test something more specific than that then feel free to just rename the other test and keep them separate.
---
[GitHub] tinkerpop pull request #934: Apply StringEscapeUtils in GroovyTranslator#con...
Posted by justinchuch <gi...@git.apache.org>.
Github user justinchuch commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/934#discussion_r219678287
--- Diff: gremlin-groovy-test/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GroovyTranslatorTest.java ---
@@ -141,4 +144,52 @@ public void shouldHandleEmptyMaps() {
public void shouldHaveValidToString() {
assertEquals("translator[h:gremlin-groovy]", GroovyTranslator.of("h").toString());
}
+
+ @Test
+ @LoadGraphWith(LoadGraphWith.GraphData.MODERN)
+ public void shouldHandleTraversalAddV() {
--- End diff --
I have renamed the test method and separate into two:
1. `shouldEscapeStrings`
2. `shouldHandleVertexAndEdge`
---
[GitHub] tinkerpop pull request #934: Apply StringEscapeUtils in GroovyTranslator#con...
Posted by justinchuch <gi...@git.apache.org>.
Github user justinchuch commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/934#discussion_r219676264
--- Diff: gremlin-groovy-test/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GroovyTranslatorTest.java ---
@@ -141,4 +144,52 @@ public void shouldHandleEmptyMaps() {
public void shouldHaveValidToString() {
assertEquals("translator[h:gremlin-groovy]", GroovyTranslator.of("h").toString());
}
+
+ @Test
+ @LoadGraphWith(LoadGraphWith.GraphData.MODERN)
--- End diff --
Thanks for pointing that out. I have combined the tests.
---
[GitHub] tinkerpop pull request #934: Apply StringEscapeUtils in GroovyTranslator#con...
Posted by justinchuch <gi...@git.apache.org>.
Github user justinchuch commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/934#discussion_r219676198
--- Diff: gremlin-groovy-test/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GroovyTranslatorTest.java ---
@@ -141,4 +144,52 @@ public void shouldHandleEmptyMaps() {
public void shouldHaveValidToString() {
assertEquals("translator[h:gremlin-groovy]", GroovyTranslator.of("h").toString());
}
+
+ @Test
+ @LoadGraphWith(LoadGraphWith.GraphData.MODERN)
+ public void shouldHandleTraversalAddV() {
+ final GraphTraversalSource g = graph.traversal();
+ final String script = GroovyTranslator.of("g").translate(g.addV("customer")
+ .property("customer_id", 501L)
+ .property("name", "Foo\u0020Bar")
+ .property("salary", "\u0024\u00201000")
+ .property("age", 25)
+ .asAdmin().getBytecode());
+
+ assertEquals("g.addV(\"customer\")" +
+ ".property(\"customer_id\",501L)" +
+ ".property(\"name\",\"Foo\u0020Bar\")" +
+ ".property(\"salary\",\"\u005c\u005c\u0024\u00201000\")" +
+ ".property(\"age\",(int) 25)",
+ script);
+ }
+
+ @Test
+ @LoadGraphWith(LoadGraphWith.GraphData.MODERN)
+ public void shouldHandleVertex() {
+ final GraphTraversalSource g = graph.traversal();
+
+ final String label1 = "customer";
+ final Object id1 = "name:20:foo\u0020bar#50";
+
+ final Vertex vertex1 = DetachedVertex.build().setLabel(label1).setId(id1).create();
+ final String script1 = GroovyTranslator.of("g").translate(g.V().inject(vertex1).asAdmin().getBytecode());
+ assertEquals("g.V().inject(new org.apache.tinkerpop.gremlin.structure.util.detached.DetachedVertex(\"name:20:foo bar#50\",\"customer\", Collections.emptyMap()))", script1);
+
+ final String label2 = "user";
+ final Object id2 = "name:20:foo\\u0020bar#50";
+
+ final Vertex vertex2 = DetachedVertex.build().setLabel(label2).setId(id2).create();
+ final String script2 = GroovyTranslator.of("g").translate(g.V().inject(vertex2).asAdmin().getBytecode());
+ assertEquals("g.V().inject(new org.apache.tinkerpop.gremlin.structure.util.detached.DetachedVertex(\"name:20:foo\\\\u0020bar#50\",\"user\", Collections.emptyMap()))", script2);
+ }
+
+ @Test
+ @LoadGraphWith(LoadGraphWith.GraphData.MODERN)
+ public void shouldHandleLambda() {
--- End diff --
I have appended the test `shouldSupportStringSupplierLambdas` to include assert the result of `translate`.
---