[GitHub] [tinkerpop] ihoro commented on pull request #1547: Add GraphBinary serialization support to gremlin-javascript

[GitBox <gi...@apache.org>]

ihoro commented on PR #1547:
URL: https://github.com/apache/tinkerpop/pull/1547#issuecomment-1133940276

   Hi,
   
   I stuck with `should allow ReservedKeysVerificationStrategy` gremlin-javascript integration test for GraphBinary. Gremlin server complained that `ReservedKeysVerificationStrategy` Java class is not found.
   
   Later I found that GraphSON sends Bytecode's `withStrategies` source without fully qualified class name, it simply sends `g:ReservedKeysVerificationStrategy` type based on JavaScript class name and, I guess (I did not research respective Java code), server side’s GraphSON implementation knows what Java class should be used. Thus, I got it that GraphBinary should send FQCN instead, and every `TraversalStrategy` of gremlin-javascript already provides respective `fqcn` field.
   
   I got the next issue after that, Gremlin server was not erroring for reserved keys as the test expected. And I found that `ReservedKeysVerificationStrategy` class in gremlin-javascript has `fqcn=org.apache.tinkerpop.gremlin.process.traversal.strategy.verification.EdgeLabelVerificationStrategy`. GraphSON does not use this field, that’s why the integration test is okay for it. I changed it to `org.apache.tinkerpop.gremlin.process.traversal.strategy.verification.ReservedKeysVerificationStrategy` and the integration test passed.
   
   As long as this typo affects GraphBinary only, i.e. the new feature, I could integrate this typo fix within this PR. But, if you want to get it fixed as a separate PR then let me know, I’m going to keep it fixed localy meanwhile.


-- 
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