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 2022/02/17 18:15:03 UTC
[GitHub] [tinkerpop] bechbd opened a new pull request #1570: Fixed issue with property(Map) being passed a null
bechbd opened a new pull request #1570:
URL: https://github.com/apache/tinkerpop/pull/1570
Fixed issue where property(Map) was throwing an NPE when passed a null. This now behaves as a no-op and returns the input values
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] mikepersonick commented on a change in pull request #1570: Fixed issue with property(Map) being passed a null
Posted by GitBox <gi...@apache.org>.
mikepersonick commented on a change in pull request #1570:
URL: https://github.com/apache/tinkerpop/pull/1570#discussion_r810493490
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java
##########
@@ -2473,6 +2473,8 @@ else if (value instanceof Traversal)
property(key, entry.getKey(), entry.getValue());
}
return this;
+ } else if (value == null) { // Just return the input if you pass a null
+ return this;
Review comment:
What's the alternative?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] bechbd commented on a change in pull request #1570: Fixed issue with property(Map) being passed a null
Posted by GitBox <gi...@apache.org>.
bechbd commented on a change in pull request #1570:
URL: https://github.com/apache/tinkerpop/pull/1570#discussion_r813124519
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java
##########
@@ -2473,6 +2473,8 @@ else if (value instanceof Traversal)
property(key, entry.getKey(), entry.getValue());
}
return this;
+ } else if (value == null) { // Just return the input if you pass a null
+ return this;
Review comment:
Since it performs a no-op here, I am not sure, it really matters too much that it doesn't get translated. However, I think the bigger question is if we expect the Translators to provide an exact translation of the query or if it can translate to an "optimized" version of the query.
I suspect that even without the `null` case, a general Map would result in multiple `property()` steps being chained together instead of a single step.
Thoughts on the need to provide a verbatim translation or not?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a change in pull request #1570: Fixed issue with property(Map) being passed a null
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1570:
URL: https://github.com/apache/tinkerpop/pull/1570#discussion_r811168019
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java
##########
@@ -2473,6 +2473,8 @@ else if (value instanceof Traversal)
property(key, entry.getKey(), entry.getValue());
}
return this;
+ } else if (value == null) { // Just return the input if you pass a null
+ return this;
Review comment:
I guess you would avoid folding to an existing `AddPropertyStep` and include a "no-op" form for a new instance of it. So `AddPropertyStep` would need some changes to do this.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on pull request #1570: Fixed issue with property(Map) being passed a null
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1570:
URL: https://github.com/apache/tinkerpop/pull/1570#issuecomment-1055335837
@bechbd looks like you need to rebase this - but other than that 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.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a change in pull request #1570: Fixed issue with property(Map) being passed a null
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1570:
URL: https://github.com/apache/tinkerpop/pull/1570#discussion_r816690109
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java
##########
@@ -2473,6 +2473,8 @@ else if (value instanceof Traversal)
property(key, entry.getKey(), entry.getValue());
}
return this;
+ } else if (value == null) { // Just return the input if you pass a null
+ return this;
Review comment:
I think it's ok for now to overlook this concern, but in the long run it should probably be addressed somehow. Created: https://issues.apache.org/jira/browse/TINKERPOP-2714 to track.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette merged pull request #1570: Fixed issue with property(Map) being passed a null
Posted by GitBox <gi...@apache.org>.
spmallette merged pull request #1570:
URL: https://github.com/apache/tinkerpop/pull/1570
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a change in pull request #1570: Fixed issue with property(Map) being passed a null
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1570:
URL: https://github.com/apache/tinkerpop/pull/1570#discussion_r809583093
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java
##########
@@ -2473,6 +2473,8 @@ else if (value instanceof Traversal)
property(key, entry.getKey(), entry.getValue());
}
return this;
+ } else if (value == null) { // Just return the input if you pass a null
+ return this;
Review comment:
just a side effect to consider here...by returning `this` we don't modify the traversal/bytecode which means that if you were to do a `profile()`, `toString()`, use a `Translator`, etc. you wouldn't get and exact representation of the Gremlin that you executed. i've generally tried to avoid this problem in other cases with `null` arguments, but have questioned how important it is at times. thoughts?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org