Cast certain traversal step inputs as int32 for serialization in Gremlin-Go

[simonz-bq]

Certain traversal steps in Gremlin Server only take in int as arguments, and by default numbers passed through in Go, and therefore by extension Gremlin-Go, are passed in as int, which in Go are a larger data type and is different from int32, which is what matches Java's int. The default serialization for such numbers ends up being a long, which causes the server to return a failure due to a type mismatch.

This change makes it so that when a user uses Gremlin-Go, they no longer have to enter numbers in certain steps as Times(int32(10)) and instead can now just simply do Times(10). It will now cast the input as int32 which will then be serialized to a Java int, if the number itself is valid to fit in an int32. Otherwise, it will be rejected by the server, as expected.

[xiazcy]

Now, there are other steps which also need int32 casting if the user enters int (examples can be found in gremlin.go by searching int32(), given we are updating this one, should we look into the others and address the same way?

[jroimartin]

@simonz-bq @xiazcy would it be an option to deal with this kind of type conversions inside the function makeBytecodeRequest?

makeBytecodeRequest could be modified to return (request, error) and (*Client).submitBytecode could return with error if it fails. That would allow to check that the conversion is valid before sending it to the server.

I'm asking because it seems that the root cause of the problem is dealing with this inside the step itself which cannot return an error.

[xiazcy]

makeBytecodeRequest could be modified to return (request, error) and (*Client).submitBytecode could return with error if it fails. That would allow to check that the conversion is valid before sending it to the server.

@jroimartin That may work, and I suppose it would involve checking within a range of bytecode steps that needs casting and perform the cast? Although in that case would we want to add this extra check for every bytecode request we make?

[jroimartin]

I suppose it would involve checking within a range of bytecode steps that needs casting and perform the cast?

That's what I had in mind.

Although in that case would we want to add this extra check for every bytecode request we make?

I guess it's a trade-off:

• Updating the GLV docs only: users might be confused at first, but it would be documented and it does not add tricky nuances to the API or extra overhead.
• Implementing it inside the Times step: it would cover some of the most common use cases, but bugs due to truncated integers can be very tricky and even more confusing.
• Implementing it inside makeBytecodeRequest: adds an overhead to every request. Although bytecode is already traversed to extract per-request arguments, so this could be maybe "merged" somehow to traverse the bytecode only once.

Besides that, it is also true that Limit(1) being valid versus Times(int32(1)) requiring a type conversion does not look very natural.

[xiazcy]

• Implementing it inside the Times step: it would cover some of the most common use cases, but bugs due to truncated integers can be very tricky and even more confusing.

Yeah, while it is unlikely for users to enter very large numbers, we probably should handle the case, and I don't see how we can do that here when we can't return errors in a traversal step.

• Implementing it inside makeBytecodeRequest: adds an overhead to every request. Although bytecode is already traversed to extract per-request arguments, so this could be maybe "merged" somehow to traverse the bytecode only once.

I would agree this may be the most ideal if we can minimize the overhead, to make it more natural for users without the need to cast.

[jroimartin]

@simonz-bq so the idea with a0a6db8 is to convert to int32 only if possible and allow the server to fail otherwise, right?

I think it is a good approach. It fulfills all the mentioned requirements: it covers the most common use cases, it does not truncate the user data and it does not add unnecessary overhead on every request.

The only thing I see is that we could fail faster because at the time of the conversion check we already know that the server will fail, but I guess it is not a big deal?

[codecov-commenter]

Codecov Report

Merging #1743 (05dedd5) into 3.5-dev (71f3ee0) will decrease coverage by 0.29%.
The diff coverage is 23.07%.

@@             Coverage Diff             @@
##           3.5-dev    #1743      +/-   ##
===========================================
- Coverage    63.51%   63.21%   -0.30%     
===========================================
  Files           23       23              
  Lines         3601     3626      +25     
===========================================
+ Hits          2287     2292       +5     
- Misses        1136     1156      +20     
  Partials       178      178              

Impacted Files	Coverage Δ
gremlin-go/driver/graphTraversal.go	85.64% <23.07%> (-4.26%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40508a4...05dedd5. Read the comment docs.

[xiazcy]

LGTM. Thanks Simon!

[jroimartin]

LGTM. Although, given my little knowledge of the project, mine are mostly suggestions and a way of learning more about the internals 🙂