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 |
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
1 participant
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