andreachild commented on code in PR #3274:
URL: https://github.com/apache/tinkerpop/pull/3274#discussion_r2505063672
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/JavaTranslator.java:
##########
@@ -301,20 +298,30 @@ 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
+ // special cases of has() where the first arg is null
or third arg is null - sometimes this can end up with the T being
+ // null or in the second case calling has that accepts
predicate instead of string as parameter in the last argument.
+ // 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
+ // form of GraphTraversal generated in the first case
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))
{
- found = false;
+ var parametersTypes = method.getParameterTypes();
+ if (methodName.equals(GraphTraversal.Symbols.has) &&
newArguments.length > 0) {
+ //first case has((T)null, ...) instead of
has((String)null, ...)
+ if (null == newArguments[0] &&
+
parametersTypes[0].isAssignableFrom(org.apache.tinkerpop.gremlin.structure.T.class))
{
+ found = false;
+ } else if (newArguments.length == 3 &&
newArguments[2] == null && parametersTypes[0] == String.class &&
+ parametersTypes[1] == String.class &&
+ parametersTypes[2] == P.class) {
+ //the second case has(String, String,
(P)(null)) instead of has(String,String, (Object)null)
+ found = false;
+ }
Review Comment:
I'm surprised the has overloads with 2 method args have not also surfaced
also a problem (it could just be by luck depending on the order of the methods
loaded in the global cache). You could change the logic to address both the 2
and 3 arg method overloads:
```suggestion
} else if (newArguments[newArguments.length - 1]
== null && parametersTypes[newArguments.length - 1] == P.class) {
//the second case has(String, String,
(P)(null)) instead of has(String,String, (Object)null)
//or has(String, (P)(null)) instead of
has(String, (Object)null)
found = false;
}
```
--
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]
