GValue parity across all GLVs and parameterized feature tests#3456
Open
Cole-Greer wants to merge 16 commits into
Open
GValue parity across all GLVs and parameterized feature tests#3456Cole-Greer wants to merge 16 commits into
Cole-Greer wants to merge 16 commits into
Conversation
Brings the client-side GValue implementations into consistent behavior: - Name validation: single shared predicate (non-empty; first char a Unicode letter; remaining chars Unicode letter/digit/underscore), enforced in the constructor (fail-fast). Mid-string underscores are now accepted in all GLVs; no Gremlin-keyword check (reserved words fail server-side). Replaces Python's str.isidentifier() and Go's token.IsIdentifier(), and relaxes the .NET rule. - Duplicate-name detection now uses structural/value equality: new recursive ValuesEqual helper in .NET GremlinLang; Go simplified to reflect.DeepEqual (also removes a latent panic on non-comparable values); Python unchanged. - String representation standardized on the Java "name=value" format (.NET ToString, Python __repr__/__str__, Go String()). - Added IsNull to the .NET GValue; removed a dead null-name branch in .NET GremlinLang. Tests: - Added cross-GLV parity unit tests (mid-string underscore accepted, empty/$ rejected, Unicode accepted, accessors, null check, string representation, and collection-valued duplicate equality). - Corrected the stale Python integration-test assertion to expect the 'bindings' gremlin-lang string instead of a 'params' dict. - Re-skipped the parameterized g.V(variable) integration test under TINKERPOP-3126 (a separate, out-of-scope server-side parse limitation), with an accurate skip reason.
JavaScript was the only Gremlin Language Variant without a client-side GValue. Adds lib/process/gvalue.ts: a generic, always-named GValue<T> with fail-fast name validation using the shared Unicode predicate (first char a Unicode letter, remaining chars letter/digit/underscore; no keyword check), a nested-GValue guard, isNull(), and toString() -> "name=value". Integrates it into GremlinLang._argAsString (renders the variable name and stores the value in the parameters map, with duplicate-name detection via Node's util.isDeepStrictEqual), exports it from the process namespace, and adds unit tests plus the export-surface assertion. Behavior is consistent with the Python/.NET/Go GValue implementations.
Match the Java reference GValue, which forbids wrapping a GValue inside another GValue. Adds a fail-fast guard to the Python, .NET, and Go GValue constructors (JavaScript already includes it in its new implementation), each rejecting a nested GValue with the message "GValues cannot be nested", plus a unit test per GLV.
The .NET GLV's strongly-typed traversal steps (e.g. AddV(string), MergeV(IDictionary), Limit(long), Range(long,long), Coin(double), HasLabel(string,...), Out(string...)) could not accept a GValue, so parameters could only be used with object-typed steps (V, Inject, etc.). Java's GraphTraversal/GraphTraversalSource/__ provide GValue<T> overloads for exactly these strongly-typed steps; this mirrors them in .NET. Adds GValue<T> overloads across GraphTraversal.cs (30), __.cs (24), and GraphTraversalSource.cs (6): constant, out/in/both/outE/inE/bothE/to/toE (labels), addV, addE, mergeV, mergeE, from, to, call, has(label,...), hasLabel, coin, range, limit, tail, skip, and the merge option modulator. Each overload passes the GValue through to GremlinLang, which renders it as a named parameter/binding. The label varargs overloads use a (GValue<string> first, params GValue<string>[] rest) shape to avoid zero-arg overload ambiguity with the existing string varargs; a bare null literal now requires a disambiguating cast (updated one such test call).
The new Out(GValue<string>, params GValue<string>[]) overload made the bare __.Out(null!) call ambiguous; it had been disambiguated as (string)null!, which passes a single null label (a non-null array element) rather than a null array, so it no longer threw ArgumentNullException. Cast to (string?[])null! to restore the original null-array intent while staying unambiguous.
Mirrors Python's parameterize feature run for .NET, exercising the GValue step
overloads end-to-end across the Gherkin corpus. Adds a parameterize mode to
DotNetTranslateVisitor (Translator.DOTNET_PARAMETERIZE) that wraps variable
arguments as new GValue<T>("name", (T) value); generate.groovy emits a second
parameterized translation set (with a quote lookbehind so GValue name literals
aren't rewritten to p["..."]); CommonSteps/GherkinTestRunner dispatch to the
parameterized traversals when PARAMETERIZE=true and a second Gherkin-only run is
added to docker-compose. The literal baseline run is unchanged. Both runs pass.
When PARAMETERIZE=true, usingTheParameterDefined wraps feature-test parameter values in gremlingo.NewGValue(name, value); docker-compose adds a second cucumber run with PARAMETERIZE=true. Mirrors Python's parameterize feature run.
Wires a PARAMETERIZE=true cucumber variant that wraps each non-g parameter in a GValue (matching the Python reference), and fixes the features-*-docker npm scripts to target the actual mounted corpus path (/gremlin-test) instead of the nonexistent ../gremlin-test, which had made the docker feature run pass vacuously. Also fixes GremlinLang._argAsString to merge a child/anonymous traversal's parameters into the parent so GValue bindings nested inside child traversals are sent to the server, resolving 'No variable found for <name>' errors. Adds unit coverage for the nested-binding merge. Assisted-by: Kiro:claude-opus-4.8
Adds the regenerated dotnet_parameterize translation set produced by generate.groovy (the companion test resource for the .NET parameterize feature) and removes the now-unreferenced hasVariableInVarargs method left behind after the parameterize work. Assisted-by: Kiro:claude-opus-4.8
Adds unit coverage proving a GValue used inside an anonymous/child traversal has its binding merged into the parent GremlinLang's parameters, mirroring the new JavaScript tests. Assisted-by: Kiro:claude-opus-4.8
Moves the GValue<T> step overloads out of the block at the bottom of GraphTraversal.cs and __.cs so each sits immediately after the existing value-typed overload of the same step, matching the convention already used in GraphTraversalSource.cs. Pure reorganization with no behavior change (identical public method sets). Assisted-by: Kiro:claude-opus-4.8
Removes the leading-underscore name reservation from Java GValue and every GLV. That rule existed to avoid collisions with auto-generated _0/_1 parameter names, a fallback that has since been removed, so it no longer adds value (and '__' is just one of ~300 reserved grammar tokens, a grammar-level limitation rather than a client concern). Also drops the stricter ^[letter][letter|digit|_]*$ identifier-pattern enforcement the non-Java drivers had added on top of Java. The only client-side name checks retained are: GValues cannot be nested, and (for the non-Java drivers, where a null name is meaningless rather than a literal-boxing case) the name cannot be null. Tests updated to assert acceptance of the previously-rejected names. Assisted-by: Kiro:claude-opus-4.8
Drops NewGValue and the unexported fields/getters in favor of a plain struct with exported Name and Value fields (construct via GValue{Name: ..., Value: ...}), which is more idiomatic Go. The String() and IsNil() helpers are retained; the Name()/Value() getters are removed as they are redundant with the exported fields.
This removes the client-side nested-GValue guard that lived in the constructor, so Go no longer fails fast on a nested GValue (it surfaces as a downstream serialization error instead). Updates GremlinLang, the cucumber step, the unit tests, and the Go GValue doc example accordingly.
Assisted-by: Kiro:claude-opus-4.8
Adds a CHANGELOG entry for relaxing GValue name validation (removal of the reserved leading-underscore restriction) and replaces the stale 'not yet implemented' JavaScript GValue reference section with real usage now that the JavaScript GLV supports GValue. Assisted-by: Kiro:claude-opus-4.8
Adds a new GValue entry to the TinkerPop 4.0.0 section of the upgrade docs scoped to the 3.8 to 4.0 path: GValue is now a client-side API in all GLVs (with reference-doc links for each driver), and the leading-underscore name restriction present in 3.8.0 has been removed. The sealed 4.0.0-beta.1 entry is left untouched. Assisted-by: Kiro:claude-opus-4.8
Parameter-name validation belongs at the GremlinLang layer (enforced on use), where Java uses SourceVersion.isIdentifier — first char a Unicode letter, '_' or '$'; remaining letters, digits, '_' or '$'. The GLVs had diverged: .NET's GremlinLang.IsValidIdentifier required a letter start (rejecting leading '_'), and JS/Python/Go validated nothing after the earlier removal of their GValue-constructor pattern. Relaxes .NET's check to allow '_'/'$', and adds the matching identifier check to the JS, Python, and Go GremlinLang layers, so all GLVs accept exactly the names Java accepts (and reject invalid identifiers like '1a' when used). GValue constructors remain permissive. Also fixes a stale Java test that asserted leading-underscore rejection. Assisted-by: Kiro:claude-opus-4.8
Cole-Greer
force-pushed
the
GValueFollowupTP4
branch
from
8f5c608 to
6d2e1c1
Compare
Contributor
Author
|
VOTE +1 |
spmallette
reviewed
Jun 16, 2026
| } | ||
| writer.writeLine(' };\n') | ||
|
|
||
| // Generate parameterized translations |
Contributor
There was a problem hiding this comment.
The PR description points this out:
.NET parameterize design: .NET is the only driver that parameterizes at the translator level (a second generated dotnet_parameterize translation set) rather than at runtime, because it's the only variant that is both statically typed and exposes strongly-typed GValue overloads—so value-vs-GValue is a compile-
time overload choice.
I think i'd like to see comments here along with the code explaining the purpose. We will need to remind ourselves someday why this is the way it is, to say nothing of agents.
spmallette
reviewed
Jun 16, 2026
| this(graphTraversalSourceName, false); | ||
| } | ||
|
|
||
| public DotNetTranslateVisitor(final String graphTraversalSourceName, final boolean parameterize) { |
Contributor
There was a problem hiding this comment.
This is the only visitor that needs this "parameterize" - it's weird enough that i think the constructors need javadoc to explain
spmallette
reviewed
Jun 16, 2026
| /** | ||
| * Translates to gremlin-dotnet with parameterized GValue wrapping for variables. | ||
| */ | ||
| DOTNET_PARAMETERIZE("DotNetParameterize", name -> new DotNetTranslateVisitor(name, true)), |
Contributor
There was a problem hiding this comment.
as mentioned elsewhere, this is odd enough that it deserves more javadoc explaining usage/why. Probably extends to its DOTNET counterpart.
as a side note, this feels unfortunate to our Translator enum. from a discoverability perspective, the API looks asymetric. a user might wonder how they get parameters with JAVASCRIPT given this naming. Thoughts on other things we could do?
spmallette
reviewed
Jun 16, 2026
| /** | ||
| * Translates to gremlin-dotnet with parameterized GValue wrapping for variables. | ||
| */ | ||
| DOTNET_PARAMETERIZE("DotNetParameterize", name -> new DotNetTranslateVisitor(name, true)), |
Contributor
There was a problem hiding this comment.
As a separate point, assuming there aren't other options, shouldn't there be some tests for this translator. I don't see any: https://github.com/apache/tinkerpop/blob/GValueFollowupTP4/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/language/translator/GremlinTranslatorTest.java
spmallette
reviewed
Jun 16, 2026
| ' new Dictionary<string, List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>>>\n' + | ||
| ' {') | ||
|
|
||
| gremlins.each { k,v -> |
Contributor
There was a problem hiding this comment.
i know this is just a internal utility script, but maybe we should encapsulate some of this duplicate logic?? i could envision an the accident of changing one but not the other and that regex seems complicated enough for one to get out of sync with the other.
spmallette
reviewed
Jun 16, 2026
| * Modified HTTP API to expect gremlin-lang strings for parameters and update all GLVs to send requests in new format. | ||
| * Added string parameter parsing to `GremlinServer` to prevent traversal injection and excessive nesting depths. | ||
| * Modified all GLVs to detect unsupported types in `GremlinLang` and throw consistent error for that case. | ||
| * Relaxed `GValue` name validation by removing the reserved leading underscore restriction. |
Contributor
There was a problem hiding this comment.
didn't this PR do a bit more than just this?
Contributor
Author
There was a problem hiding this comment.
This PR did more, but I'm writing the changelog from the perspective of an upgrade from 3.8. This is really the only notable changes from GValue in 3.8 Java. The other GLVs didn't have GValue before as it was tied into bytecode bindings. From the perspective of the 3.8 upgrade, there isn't really changes to GValue in the GLVs, it's a fresh start which is consistent with the existing Java pattern.
I can add a new entry detailing that GValue has been extended to all GLVs, but I don't think the modifications warrant changelog entries here.
spmallette
reviewed
Jun 16, 2026
|
|
||
| IMPORTANT: 4.0.0-beta.2 Release - `GValue` parameterization is not yet implemented for the JavaScript driver. This | ||
| functionality is planned for a future release. | ||
| A `GValue` is an encapsulation of a parameter name and value. A <<traversal-parameterization,subset of gremlin steps>> |
Contributor
There was a problem hiding this comment.
JavaScript had no GValue at all; Python/.NET/Go diverged on validation, duplicate detection, and string representation.
shouldn't we have more docs then? like "GValue Parameterization" sections for all variants?
Contributor
Author
There was a problem hiding this comment.
Javascript was the only GLV which was missing the "GValue Parameterization" section altogether, other than the go GValue construction edit, there were no changes needed to the existing docs here.
One slight exception is some GLVs include 2 examples (has() and mergeV()), while others only have the has() example. I'll add the merge example everywhere for consistency.
Contributor
|
VOTE +1 pending comment resolution |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Completes the client-side GValue (named query-parameter) feature so it behaves consistently across all GLVs, adds parameterized feature-test coverage for the GLVs, and simplifies GValue name validation.
Issues, gaps, and inconsistencies resolved
Details
.NET parameterize design: .NET is the only driver that parameterizes at the translator level (a second generated dotnet_parameterize translation set) rather than at runtime, because it's the only variant that is both statically typed and exposes strongly-typed GValue overloads—so value-vs-GValue is a compile-
time overload choice. The other GLVs wrap values at runtime and reuse one translation set. The trade-off is higher regeneration cost for .NET, inherent to the typed API.
Tips for Reviewers
On the surface, this is a big PR, but the bulk of the lines added come from 2 generated files which can be largely ignored. The .NET feature test design required adding a parameterized mode to the translators, such that the generated steps will include casts for GValues when present. This change alone doubled the size of
gremlin.cs, and added thousands of lines of new cases totranslations.json. This is a lot of volume from a relatively minor change.The most consequential decisions in this work is the introduction of GValue step overloads in GraphTraversal, GraphTraversalSource, and __ in .NET, the complete introduction of the GValue concept to JS, and the change in GValue contruction and validation in Go. The most notable testing change is that all GLVs have now copied the feature test design from python, where the suite runs twice, once with all script parameters inlined as literals, and the second time with parameters preserved as GValues.
Limitations