[
https://issues.apache.org/jira/browse/TINKERPOP-2727?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17514271#comment-17514271
]
ASF GitHub Bot commented on TINKERPOP-2727:
-------------------------------------------
divijvaidya commented on a change in pull request #1596:
URL: https://github.com/apache/tinkerpop/pull/1596#discussion_r837790540
##########
File path:
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/JavaTranslator.java
##########
@@ -266,6 +267,23 @@ private Object invokeMethod(final Object delegate, final
Class<?> returnType, fi
}
}
}
+
+ // special case has() where the first arg is null -
sometimes this can end up with the T being
+ // null and in 3.5.x that generates an exception which
raises badly in the translator. it is
+ // safer to force this to the String form by letting
this "found" version pass. In java this
+ // form of GraphTraversal can't be produced because of
validations for has(T, ...) but in
+ // other language it might be allowed which means that
has((T) null, ...) from something like
+ // python will end up as has((String) null, ...) which
filters rather than generates an
+ // exception. calling the T version even in a non-JVM
language seems odd and more likely the
+ // caller was shooting for the String one, but ugh who
knows. this issue showcases again how
+ // badly bytecode should either change to use
gremlin-language and go away or bytecode should
+ // get a safer way to be translated to a traversal
with more explicit calls that don't rely
+ // on reflection.
+ if (methodName.equals(GraphTraversal.Symbols.has) &&
newArguments.length > 0 && null == newArguments[0] &&
+
method.getParameterTypes()[0].isAssignableFrom(org.apache.tinkerpop.gremlin.structure.T.class))
{
Review comment:
Please correct me if I am wrong here but the tests in this PR do not
test for this condition that is mentioned in this comment `has((T) null, ...)`
. Could we please add a test for it?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
> HasContainer should allow a null key
> ------------------------------------
>
> Key: TINKERPOP-2727
> URL: https://issues.apache.org/jira/browse/TINKERPOP-2727
> Project: TinkerPop
> Issue Type: Improvement
> Components: process
> Affects Versions: 3.5.2
> Reporter: Florian Hockmann
> Priority: Minor
>
> TINKERPOP-2605 changed {{null}} semantics to allow {{null}} as an argument in
> more places so that {{has(null)}} should not throw a NPE any more and instead
> filter all traversers out.
> {{HasContainer}} however still throws a NPE if the {{key}} is {{{}null{}}}.
> I discovered this when trying to update JanusGraph [as that folds in a
> {{HasContainer}} like
> this|https://github.com/JanusGraph/janusgraph/blob/4246d49cee46d549d752515f3956cd2d59f1fd0a/janusgraph-core/src/main/java/org/janusgraph/graphdb/tinkerpop/optimize/step/HasStepFolder.java#L266]
> which leads to the NPE and thus failing tests.
> The same behavior can also be produced with just TinkerGraph and for example
> the following traversals:
> {code:java}
> g.V().has(null, 1)
> g.V().has(null, P.neq(null)){code}
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
