You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by GitBox <gi...@apache.org> on 2020/12/11 20:42:42 UTC
[GitHub] [tinkerpop] spmallette opened a new pull request #1371: TINKERPOP-2438 Introduce GremlinScriptChecker
spmallette opened a new pull request #1371:
URL: https://github.com/apache/tinkerpop/pull/1371
https://issues.apache.org/jira/browse/TINKERPOP-2438
Even though TINKERPOP-2438 is already done this is a bit of a follow on to that change. The GremlinScriptChecker performs the same exact task as the GremlinAstChecker but it does so with regex parsing which should be faster than processing a full AST. I've left the GremlinASTChecker where it is because I wonder if there isn't yet some usage for more rigorous sort of checks we might come up with in the future or perhaps have checks that only work well with AST processing. It can be removed in the future if we like.
VOTE +1
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a change in pull request #1371: TINKERPOP-2438 Introduce GremlinScriptChecker
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1371:
URL: https://github.com/apache/tinkerpop/pull/1371#discussion_r543199952
##########
File path: gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinScriptChecker.java
##########
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.groovy.jsr223;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Optional;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
Review comment:
It is documented here: https://github.com/apache/tinkerpop/pull/1371/files#diff-0c5246d79c924132b94fc6af68af66b1f81faebac2410ca9e2af663388c31bb2R73-R79
Will add something to the reference documentation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] divijvaidya commented on a change in pull request #1371: TINKERPOP-2438 Introduce GremlinScriptChecker
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on a change in pull request #1371:
URL: https://github.com/apache/tinkerpop/pull/1371#discussion_r542756884
##########
File path: gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinScriptChecker.java
##########
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.groovy.jsr223;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Optional;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * Processes Gremlin strings using regex so as to try to detect certain properties from the script without actual
+ * having to execute a {@code eval()} on it.
+ */
+public class GremlinScriptChecker {
+
+ public static final Result EMPTY_RESULT = new Result(0);
+ private static final List<String> tokens = Arrays.asList("evaluationTimeout", "scriptEvaluationTimeout",
+ "ARGS_EVAL_TIMEOUT", "ARGS_SCRIPT_EVAL_TIMEOUT");
+
+ private static final Pattern patternClean = Pattern.compile("//.*$|/\\*(.|[\\r\\n])*?\\*/|\\s", Pattern.MULTILINE);
+ private static final String timeoutTokens = "[\"']evaluationTimeout[\"']|[\"']scriptEvaluationTimeout[\"']|(?:Tokens\\.)?ARGS_EVAL_TIMEOUT|(?:Tokens\\.)?ARGS_SCRIPT_EVAL_TIMEOUT";
+ private static final Pattern patternTimeout = Pattern.compile("\\.with\\((?:" + timeoutTokens + "),(\\d*)(:?L|l)?\\)");
Review comment:
Please add documentation about this regex so that reader understands what it is doing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] divijvaidya commented on a change in pull request #1371: TINKERPOP-2438 Introduce GremlinScriptChecker
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on a change in pull request #1371:
URL: https://github.com/apache/tinkerpop/pull/1371#discussion_r542759175
##########
File path: gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinScriptChecker.java
##########
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.groovy.jsr223;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Optional;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
Review comment:
Please document the behaviour for multiple queries (that we add their timeouts). This should probably go in reference docs as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a change in pull request #1371: TINKERPOP-2438 Introduce GremlinScriptChecker
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1371:
URL: https://github.com/apache/tinkerpop/pull/1371#discussion_r543198511
##########
File path: gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinScriptChecker.java
##########
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.groovy.jsr223;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Optional;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * Processes Gremlin strings using regex so as to try to detect certain properties from the script without actual
+ * having to execute a {@code eval()} on it.
+ */
+public class GremlinScriptChecker {
+
+ public static final Result EMPTY_RESULT = new Result(0);
+ private static final List<String> tokens = Arrays.asList("evaluationTimeout", "scriptEvaluationTimeout",
+ "ARGS_EVAL_TIMEOUT", "ARGS_SCRIPT_EVAL_TIMEOUT");
+
+ private static final Pattern patternClean = Pattern.compile("//.*$|/\\*(.|[\\r\\n])*?\\*/|\\s", Pattern.MULTILINE);
Review comment:
Interesting - the tool I've always used didn't produce that nice explain stuff. I didn't realize that such a thing existed. 👍
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] divijvaidya commented on a change in pull request #1371: TINKERPOP-2438 Introduce GremlinScriptChecker
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on a change in pull request #1371:
URL: https://github.com/apache/tinkerpop/pull/1371#discussion_r542752536
##########
File path: gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinScriptCheckerTest.java
##########
@@ -0,0 +1,300 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.groovy.jsr223;
+
+import org.junit.Test;
+
+import java.util.Optional;
+
+import static org.apache.tinkerpop.gremlin.groovy.jsr223.GremlinScriptChecker.EMPTY_RESULT;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertSame;
+
+public class GremlinScriptCheckerTest {
+
+ @Test
+ public void shouldNotFindTimeout() {
+ assertEquals(Optional.empty(), GremlinScriptChecker.parse("g.with(true).V().out('knows')").getTimeout());
+ }
+
+ @Test
+ public void shouldReturnEmpty() {
+ assertSame(EMPTY_RESULT, GremlinScriptChecker.parse(""));
+ }
+
+ @Test
+ public void shouldNotFindTimeoutCozWeCommentedItOut() {
+ assertEquals(Optional.empty(), GremlinScriptChecker.parse("g.\n" +
+ " // with('evaluationTimeout', 1000L).\n" +
+ " with(true).V().out('knows')").getTimeout());
+ }
+
+ @Test
+ public void shouldIdentifyTimeoutAsStringKeySingleQuoted() {
+ assertEquals(1000, GremlinScriptChecker.parse("g.with('evaluationTimeout', 1000L).with(true).V().out('knows')").
+ getTimeout().get().longValue());
+ assertEquals(1000, GremlinScriptChecker.parse("g.with('scriptEvaluationTimeout', 1000L).with(true).V().out('knows')").
+ getTimeout().get().longValue());
+ }
+
+ @Test
+ public void shouldIdentifyTimeoutAsStringKeyDoubleQuoted() {
+ assertEquals(1000, GremlinScriptChecker.parse("g.with(\"evaluationTimeout\", 1000L).with(true).V().out('knows')").
+ getTimeout().get().longValue());
+ assertEquals(1000, GremlinScriptChecker.parse("g.with(\"scriptEvaluationTimeout\", 1000L).with(true).V().out('knows')").
+ getTimeout().get().longValue());
+ }
+
+ @Test
+ public void shouldIdentifyTimeoutAsTokenKey() {
+ assertEquals(1000, GremlinScriptChecker.parse("g.with(Tokens.ARGS_EVAL_TIMEOUT, 1000L).with(true).V().out('knows')").
+ getTimeout().get().longValue());
+ assertEquals(1000, GremlinScriptChecker.parse("g.with(Tokens.ARGS_SCRIPT_EVAL_TIMEOUT, 1000L).with(true).V().out('knows')").
+ getTimeout().get().longValue());
+ }
+
+ @Test
+ public void shouldIdentifyTimeoutAsTokenKeyWithoutClassName() {
+ assertEquals(1000, GremlinScriptChecker.parse("g.with(ARGS_EVAL_TIMEOUT, 1000L).with(true).V().out('knows')").
+ getTimeout().get().longValue());
+ assertEquals(1000, GremlinScriptChecker.parse("g.with(ARGS_SCRIPT_EVAL_TIMEOUT, 1000L).with(true).V().out('knows')").
+ getTimeout().get().longValue());
+ }
+
+ @Test
+ public void shouldIdentifyMultipleTimeouts() {
+ assertEquals(6000, GremlinScriptChecker.parse("g.with('evaluationTimeout', 1000L).with(true).V().out('knows');" +
+ "g.with('evaluationTimeout', 1000L).with(true).V().out('knows')\n" +
+ " //g.with('evaluationTimeout', 1000L).with(true).V().out('knows')\n" +
+ " /* g.with('evaluationTimeout', 1000L).with(true).V().out('knows')*/\n" +
+ " /* \n" +
+ "g.with('evaluationTimeout', 1000L).with(true).V().out('knows') \n" +
+ "*/ \n" +
+ " g.with('evaluationTimeout', 1000L).with(true).V().out('knows')\n" +
Review comment:
This should probably have a `;` at the end?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] divijvaidya commented on a change in pull request #1371: TINKERPOP-2438 Introduce GremlinScriptChecker
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on a change in pull request #1371:
URL: https://github.com/apache/tinkerpop/pull/1371#discussion_r542756360
##########
File path: gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinScriptChecker.java
##########
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.groovy.jsr223;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Optional;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * Processes Gremlin strings using regex so as to try to detect certain properties from the script without actual
+ * having to execute a {@code eval()} on it.
+ */
+public class GremlinScriptChecker {
+
+ public static final Result EMPTY_RESULT = new Result(0);
+ private static final List<String> tokens = Arrays.asList("evaluationTimeout", "scriptEvaluationTimeout",
+ "ARGS_EVAL_TIMEOUT", "ARGS_SCRIPT_EVAL_TIMEOUT");
+
+ private static final Pattern patternClean = Pattern.compile("//.*$|/\\*(.|[\\r\\n])*?\\*/|\\s", Pattern.MULTILINE);
Review comment:
Documentation on this regEx would be nice. Something like:
//.*$ --> match strings starting with // followed by zero or more chars till the end of line
| --> OR
\\*(. --> match string ...
...
You can choose to copy these comments from an online regex tester. They usually print out a detailed description of the regex.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] divijvaidya commented on a change in pull request #1371: TINKERPOP-2438 Introduce GremlinScriptChecker
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on a change in pull request #1371:
URL: https://github.com/apache/tinkerpop/pull/1371#discussion_r542752905
##########
File path: gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinScriptCheckerTest.java
##########
@@ -0,0 +1,300 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.groovy.jsr223;
+
+import org.junit.Test;
+
+import java.util.Optional;
+
+import static org.apache.tinkerpop.gremlin.groovy.jsr223.GremlinScriptChecker.EMPTY_RESULT;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertSame;
+
+public class GremlinScriptCheckerTest {
+
+ @Test
+ public void shouldNotFindTimeout() {
+ assertEquals(Optional.empty(), GremlinScriptChecker.parse("g.with(true).V().out('knows')").getTimeout());
+ }
+
+ @Test
+ public void shouldReturnEmpty() {
+ assertSame(EMPTY_RESULT, GremlinScriptChecker.parse(""));
+ }
+
+ @Test
+ public void shouldNotFindTimeoutCozWeCommentedItOut() {
+ assertEquals(Optional.empty(), GremlinScriptChecker.parse("g.\n" +
+ " // with('evaluationTimeout', 1000L).\n" +
+ " with(true).V().out('knows')").getTimeout());
+ }
+
+ @Test
+ public void shouldIdentifyTimeoutAsStringKeySingleQuoted() {
+ assertEquals(1000, GremlinScriptChecker.parse("g.with('evaluationTimeout', 1000L).with(true).V().out('knows')").
+ getTimeout().get().longValue());
+ assertEquals(1000, GremlinScriptChecker.parse("g.with('scriptEvaluationTimeout', 1000L).with(true).V().out('knows')").
+ getTimeout().get().longValue());
+ }
+
+ @Test
+ public void shouldIdentifyTimeoutAsStringKeyDoubleQuoted() {
+ assertEquals(1000, GremlinScriptChecker.parse("g.with(\"evaluationTimeout\", 1000L).with(true).V().out('knows')").
+ getTimeout().get().longValue());
+ assertEquals(1000, GremlinScriptChecker.parse("g.with(\"scriptEvaluationTimeout\", 1000L).with(true).V().out('knows')").
+ getTimeout().get().longValue());
+ }
+
+ @Test
+ public void shouldIdentifyTimeoutAsTokenKey() {
+ assertEquals(1000, GremlinScriptChecker.parse("g.with(Tokens.ARGS_EVAL_TIMEOUT, 1000L).with(true).V().out('knows')").
+ getTimeout().get().longValue());
+ assertEquals(1000, GremlinScriptChecker.parse("g.with(Tokens.ARGS_SCRIPT_EVAL_TIMEOUT, 1000L).with(true).V().out('knows')").
+ getTimeout().get().longValue());
+ }
+
+ @Test
+ public void shouldIdentifyTimeoutAsTokenKeyWithoutClassName() {
+ assertEquals(1000, GremlinScriptChecker.parse("g.with(ARGS_EVAL_TIMEOUT, 1000L).with(true).V().out('knows')").
+ getTimeout().get().longValue());
+ assertEquals(1000, GremlinScriptChecker.parse("g.with(ARGS_SCRIPT_EVAL_TIMEOUT, 1000L).with(true).V().out('knows')").
+ getTimeout().get().longValue());
+ }
+
+ @Test
+ public void shouldIdentifyMultipleTimeouts() {
+ assertEquals(6000, GremlinScriptChecker.parse("g.with('evaluationTimeout', 1000L).with(true).V().out('knows');" +
+ "g.with('evaluationTimeout', 1000L).with(true).V().out('knows')\n" +
+ " //g.with('evaluationTimeout', 1000L).with(true).V().out('knows')\n" +
+ " /* g.with('evaluationTimeout', 1000L).with(true).V().out('knows')*/\n" +
+ " /* \n" +
+ "g.with('evaluationTimeout', 1000L).with(true).V().out('knows') \n" +
+ "*/ \n" +
+ " g.with('evaluationTimeout', 1000L).with(true).V().out('knows')\n" +
Review comment:
This should probably have a` ;` at the end?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette merged pull request #1371: TINKERPOP-2438 Introduce GremlinScriptChecker
Posted by GitBox <gi...@apache.org>.
spmallette merged pull request #1371:
URL: https://github.com/apache/tinkerpop/pull/1371
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] divijvaidya commented on pull request #1371: TINKERPOP-2438 Introduce GremlinScriptChecker
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on pull request #1371:
URL: https://github.com/apache/tinkerpop/pull/1371#issuecomment-745468829
LGTM
VOTE +1
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org