Add GremlinLang to Go driver #2965
Conversation
…tes will be needed when connection is set up.
| // submitGremlinLang submits GremlinLang to the server to execute and returns a ResultSet. | ||
| // TODO test and update when connection is set up | ||
| func (client *Client) submitGremlinLang(gremlinLang *GremlinLang) (ResultSet, error) { | ||
| client.logHandler.logf(Debug, submitStartedBytecode, *gremlinLang) |
There was a problem hiding this comment.
The log message should be changed to gremlin lang
| // TODO test and update when connection is set up | ||
| func (driver *DriverRemoteConnection) submitGremlinLang(gremlinLang *GremlinLang) (ResultSet, error) { | ||
| if driver.isClosed { | ||
| return nil, newError(err0203SubmitBytecodeToClosedConnectionError) |
There was a problem hiding this comment.
Error message should be updated to reference gremlin lang.
| @@ -922,7 +925,6 @@ func (t *Transaction) Begin() (*GraphTraversalSource, error) { | |||
|
|
|||
| gts := &GraphTraversalSource{ | |||
| graph: t.g.graph, | |||
| bytecode: t.g.bytecode, | |||
There was a problem hiding this comment.
Should this be replaced by gremlinLang: t.g.gremlinLang,?
| @@ -42,95 +42,88 @@ func NewGraphTraversalSource(graph *Graph, remoteConnection *DriverRemoteConnect | |||
| convertedArgs := convertStrategyVarargs(traversalStrategies) | |||
| bc := NewBytecode(nil) | |||
| } | ||
| } | ||
| //TODO verify | ||
| var optionsStrategy TraversalStrategy = gts.gremlinLang.optionsStrategies[0] |
There was a problem hiding this comment.
Is it possible for optionStrategies to be empty and produce out of bounds error?
| ) | ||
|
|
||
| type GremlinLang struct { | ||
| emptyArray []interface{} |
There was a problem hiding this comment.
Should this be [0]interface{} so that it cannot be mutated to be non-empty?
|
|
||
| func (gl *GremlinLang) addToGremlin(name string, args ...interface{}) { | ||
| flattenedArgs := gl.flattenArguments(args...) | ||
| //flattenedArgs := reflect.ValueOf(gl.flattenArguments(args...)) |
There was a problem hiding this comment.
Should remove commented out code
| gl.gremlin.WriteString("Cardinality.") | ||
| //str0, _ := gl.argAsString(flattenedArgs.Index(0)) | ||
| //str1, _ := gl.argAsString(flattenedArgs.Index(1)) | ||
| str0, _ := gl.argAsString(flattenedArgs[0]) |
There was a problem hiding this comment.
Should there be error handling if the flattenedArgs are not an array of exactly 2 items as expected?
| } | ||
| } | ||
|
|
||
| //var withOptionsMap map[any]string = map[any]string{ |
There was a problem hiding this comment.
Should delete commented out code?
| switch v := arg.(type) { | ||
| case string: | ||
| // TODO check escaping & formatting | ||
| return fmt.Sprintf("\"%s\"", html.EscapeString(v)), nil |
There was a problem hiding this comment.
How about %q instead of %s + html.EscapeString(v) (since I don't think we want HTML syntax)?
https://pkg.go.dev/fmt#hdr-Printing
%q a double-quoted string safely escaped with Go syntax
There was a problem hiding this comment.
I think a deeper dive is warranted here. The requirement is for it to be properly escaped according to gremlin-lang syntax (cannot close out the string literal and start writing subsequent steps). It's not immediately clear that Go or HTML escaping meets this requirement.
| return "null", nil | ||
| } | ||
|
|
||
| switch v := arg.(type) { |
There was a problem hiding this comment.
Do we need to worry about a few types not included in this switch statement such as complex64, complex128, uintptr?
There was a problem hiding this comment.
Should the line t.Bytecode.AddStep("discard") be converted to gremlin lang?
| return fmt.Sprintf("%s.%s", strings.ToUpper(name[:1])+name[1:], v), nil | ||
| case *Vertex: | ||
| id, _ := gl.argAsString(v.Id) | ||
| return fmt.Sprintf("new ReferenceVertex(%s,\"%s\")", id, v.Label), nil |
There was a problem hiding this comment.
There may be a need to escape the id and label here.
| return paramName | ||
| } | ||
|
|
||
| func (gl *GremlinLang) GetGremlin(arg ...string) string { |
There was a problem hiding this comment.
Nit: I wonder if we should limit using this vargargs pattern for psuedo-overloaded methods in favour of something more idiomatic in go. Feel free to ignore this request if you'd like as this pattern is already heavily used throughout gremlin-go.
Base set of changes to add GremlinLang to Go driver, adapted from the originally translator. Datetime and strategies will be updated separately.
Additional clean-up and updates will be done once remote connection is fully set-up, tests will also remain disabled until then.