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)

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 +156
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately not (no coverage in feature tests either). I've updated to make it consistent with other languages, but I think I'll revisit this later separately from this PR, and check the existing coverage for java/python as well.

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)",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The grammar actually accepts the string literal. It looks like python also sends this, so I'd think it's a separate topic for revisit.

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

xiazcy and others added 2 commits January 20, 2026 10:29
…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

update strategy config test
@xiazcy xiazcy force-pushed the go-http-converge-conn-lang branch from 60a6887 to 9ba6ac2 Compare January 21, 2026 01:32
@xiazcy
Copy link
Contributor Author

xiazcy commented Jan 21, 2026

VOTE +1

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