Skip to content

Conversation

@xiazcy
Copy link
Contributor

@xiazcy xiazcy commented Jan 5, 2026

A follow up of #3059 to make the go GLV working and buildable in master. Most implementations were done previously inside a feature branch, updates were needed due to accumulated tech debts in the past year.

Major changes included in this PR:

  • GremlinLang replace Bytecode
  • GraphBinary 4

Additional changes includes test set-up fixes, HTTP connection updates, basic auth, and various update in traversal to enable successful build.

Major changes pending:

  • Response chunk streaming (entire response is processed at once)
  • Interceptors (ie. authentication)

xiazcy and others added 2 commits January 2, 2026 13:14
…dates will be needed when connection is set up.

* update graph binary serializer to 4.0.0 spec
* Updated datetime, added GValue, updated Options strategy, removed unneeded strategy package name, added NewTraversalStrategy to allow custom strategies. (#3056)
* Replace websockets with http. Changed connection pooling to be delegated to aio http library. Added POC changes to request serialization just to make happy path work. Full connection configuration and response chunking not yet implemented. (#3057)
* update gremlinlang to use slices to store instructions instead
* update strategies test after gremlinlang change
* update serializer to use marker
* fix strategies translation and add missing strategies
* Remove result set map container as it is no longer needed.
* Removed concept of session as it is no longer needed.
* Read response status code from graph binary response.
* Removed response status attributes as they no longer exist.
* Read response status message and exception strings.
* Remove gremlin-socket-server from docker compose.
* Removed socket server tests.
* Changed transporter to delegate most of the request creation to client library by using http.NewRequest. Removed requestId as it's no longer used. Made some small logging changes.
* Removed unused request op attribute.
* Removed unused request processor.
* Added some comments, refactored error handling and resource closing.
* Removed reference to partial content status code. Added some comments and did some minor cleanup.
* Removed reference to response metadata.
* Removed unused synchronized mpa.
* Removed tests for custom type serialization. Removed response id as it is no longer used. Adjusted expected serialized request bytes according to TP4 format. Adjusted bytes for testing deserialization according to TP4 format. Removed serializer unwrapping of single item results as it was causing serialization test to fail. (#3066)

fix various gremlinlang and serializer bugs in go for http
github.com/gorilla/websocket v1.5.3/go.mod h1:YR8l580nyteQvAITg2hZ9XVh4b55+EU/adAjf1fMHhE=
github.com/nicksnyder/go-i18n/v2 v2.5.0 h1:3wH1gpaekcgGuwzWdSu7JwJhH9Tk87k1ezt0i1p2/Is=
github.com/nicksnyder/go-i18n/v2 v2.5.0/go.mod h1:DrhgsSDZxoAfvVrBVLXoxZn/pN5TXqaDbq7ju94viiQ=
github.com/nicksnyder/go-i18n/v2 v2.6.0/go.mod h1:88sRqr0C6OPyJn0/KRNaEz1uWorjxIKP7rUUcvycecE=
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is both go-i18n/v2 v2.5.0 and go-i18n/v2 v2.6.0 required here?

request := makeBytecodeRequest(bytecode, client.traversalSource)
return client.gremlinClient.send(&request)
// submitGremlinLang submits GremlinLang to the server to execute and returns a ResultSet.
// TODO test and update when connection is set up
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there jira for this todo?

Comment on lines +197 to +203
// submitBytecode submits Bytecode to the server to execute and returns a ResultSet.
// TODO remove
//func (client *Client) submitBytecode(bytecode *Bytecode) (ResultSet, error) {
// client.logHandler.logf(Debug, submitStartedBytecode, *bytecode)
// request := makeBytecodeRequest(bytecode, client.traversalSource)
// return client.httpProtocol.send(&request)
//}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be removed now?

testBasicAuthAuthInfo := getBasicAuthInfo()
testBasicAuthTlsConfig := &tls.Config{InsecureSkipVerify: true}

// this test is used to test the ws->http POC changes via manual execution with a local TP 4.0 gremlin server running on 8182
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the plan for this test? It looks like it's mostly setup for manual debugging. Is the intention to keep it or will it be cleaned up or removed at a later date?

return driver.submitBytecode(bc)
}
// TODO remove
// submitBytecode sends a Bytecode traversal to the server.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be removed now?


// GValue is a variable or literal value that is used in a Traversal. It is composed of a key-value pair where the key
// is the name given to the variable and the value is the object that the variable resolved to.
type GValue interface {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it's necessary to have an interface for GValue with accessor methods. I think we could get away with capitalizing the gValue struct type and allowing users to directly interact with it. I wouldn't expect there is much need for multiple implementations of the interface.

return gts.bytecode
}
// TODO remove
//func (gts *GraphTraversalSource) GetBytecode() *Bytecode {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this going to be replaced with a new GetGremlinLang() method?

Comment on lines +142 to +150
if math.IsNaN(float64(v)) {
return "NaN", nil
}
if math.IsInf(float64(v), 1) {
return "Infinity", nil
}
if math.IsInf(float64(v), -1) {
return "-Infinity", nil
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely out of scope for this PR, but its occurring to me that we are losing the float 32 type for infinities and NaNs. Seems like a minor gap in the grammar which we may want to address.

case time.Time:
return fmt.Sprintf("datetime(\"%v\")", v.Format(time.RFC3339)), nil
case cardinality, column, direction, operator, order, pick, pop, barrier, scope, t, merge, gType:
name := reflect.ValueOf(v).Type().Name()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unfortunate that we need to use reflection here. Definitely out of scope for this PR but we might want to revisit the design of our "pseudo-enums" and see if there's anything better.

return fmt.Sprintf("%s.%s", strings.ToUpper(name), v), nil
case *Vertex:
id, _ := gl.argAsString(v.Id)
return fmt.Sprintf("new ReferenceVertex(%s,\"%s\")", escapeQuotes.Replace(id), escapeQuotes.Replace(v.Label)), nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there tests for this? ReferenceVertex should not be present in the grammar.

return g.V("44").ValueMap().With(WithOptions.Tokens, WithOptions.Labels)
},
equals: "g.V('44').valueMap().with(WithOptions.tokens,WithOptions.labels)",
equals: "g.V(\"44\").valueMap().with(\"~tinkerpop.valueMap.tokens\",2)",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The WithOptions in this and following cases don't look right, the grammar should parse the enums directly.

Suggested change
equals: "g.V(\"44\").valueMap().with(\"~tinkerpop.valueMap.tokens\",2)",
equals: "g.V(\"44\").valueMap().with(WithOptions.tokens,WithOptions.labels)",

UnscaledValue *big.Int
}

func (bd *BigDecimal) Value() *big.Float {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if we could get rid of our BigDecimal type and allow users to directly use the native big.Float type

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants