This is an automated email from the ASF dual-hosted git repository. Cole-Greer pushed a commit to branch GValueFollowupTP4 in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit 6c7c780ec574cae5b27d19ed8322f1bb94a4d435 Author: Cole Greer <[email protected]> AuthorDate: Thu Jun 4 15:17:48 2026 -0700 Align GValue support and validation across Python, .NET, and Go GLVs 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. --- .../src/Gremlin.Net/Process/Traversal/GValue.cs | 9 ++- .../Gremlin.Net/Process/Traversal/GremlinLang.cs | 50 ++++++++++-- .../Process/Traversal/GremlinLangTests.cs | 93 ++++++++++++++++++++++ gremlin-go/driver/gValue.go | 29 ++++++- gremlin-go/driver/gValue_test.go | 50 ++++++++++-- gremlin-go/driver/gremlinlang.go | 11 +-- .../python/gremlin_python/process/traversal.py | 15 ++-- .../driver/test_driver_remote_connection.py | 4 +- .../python/tests/unit/process/test_gremlin_lang.py | 68 ++++++++++++---- 9 files changed, 275 insertions(+), 54 deletions(-) diff --git a/gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GValue.cs b/gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GValue.cs index 84c4eb0402..97039fb229 100644 --- a/gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GValue.cs +++ b/gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GValue.cs @@ -72,7 +72,7 @@ namespace Gremlin.Net.Process.Traversal for (int i = 1; i < name.Length; i++) { - if (!char.IsLetterOrDigit(name[i])) + if (!char.IsLetterOrDigit(name[i]) && name[i] != '_') throw new ArgumentException($"Invalid parameter name [{name}]."); } @@ -119,10 +119,15 @@ namespace Gremlin.Net.Process.Traversal } } + /// <summary> + /// Gets a value indicating whether the parameter value is null. + /// </summary> + public bool IsNull => Value == null; + /// <inheritdoc /> public override string ToString() { - return $"GValue({Name}, {Value})"; + return $"{Name}={Value}"; } } } diff --git a/gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GremlinLang.cs b/gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GremlinLang.cs index 21115a8f8f..4767f90cfd 100644 --- a/gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GremlinLang.cs +++ b/gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GremlinLang.cs @@ -302,11 +302,6 @@ namespace Gremlin.Net.Process.Traversal { var key = gValue.Name; - if (key == null) - { - return ArgAsString(gValue.ObjectValue); - } - if (!IsValidIdentifier(key)) { throw new ArgumentException($"Invalid parameter name [{key}]."); @@ -314,7 +309,7 @@ namespace Gremlin.Net.Process.Traversal if (_parameters.ContainsKey(key)) { - if (!Equals(_parameters[key], gValue.ObjectValue)) + if (!ValuesEqual(_parameters[key], gValue.ObjectValue)) { throw new ArgumentException($"Parameter with name [{key}] already defined."); } @@ -722,12 +717,53 @@ namespace Gremlin.Net.Process.Traversal return false; for (int i = 1; i < name.Length; i++) { - if (!char.IsLetterOrDigit(name[i])) + if (!char.IsLetterOrDigit(name[i]) && name[i] != '_') return false; } return true; } + private static bool ValuesEqual(object? a, object? b) + { + if (Equals(a, b)) return true; + if (a == null || b == null) return false; + + if (a is IDictionary dictA && b is IDictionary dictB) + { + if (dictA.Count != dictB.Count) return false; + foreach (DictionaryEntry entry in dictA) + { + if (!dictB.Contains(entry.Key)) return false; + if (!ValuesEqual(entry.Value, dictB[entry.Key])) return false; + } + return true; + } + + if (a is string || b is string) return false; + + if (a is IEnumerable enumA && b is IEnumerable enumB) + { + var enumeratorA = enumA.GetEnumerator(); + var enumeratorB = enumB.GetEnumerator(); + try + { + while (enumeratorA.MoveNext()) + { + if (!enumeratorB.MoveNext()) return false; + if (!ValuesEqual(enumeratorA.Current, enumeratorB.Current)) return false; + } + return !enumeratorB.MoveNext(); + } + finally + { + if (enumeratorA is IDisposable dA) dA.Dispose(); + if (enumeratorB is IDisposable dB) dB.Dispose(); + } + } + + return false; + } + /// <summary> /// Creates a deep copy of this <see cref="GremlinLang" /> instance. /// </summary> diff --git a/gremlin-dotnet/test/Gremlin.Net.UnitTest/Process/Traversal/GremlinLangTests.cs b/gremlin-dotnet/test/Gremlin.Net.UnitTest/Process/Traversal/GremlinLangTests.cs index b4eeade168..f7d4c5ee61 100644 --- a/gremlin-dotnet/test/Gremlin.Net.UnitTest/Process/Traversal/GremlinLangTests.cs +++ b/gremlin-dotnet/test/Gremlin.Net.UnitTest/Process/Traversal/GremlinLangTests.cs @@ -993,6 +993,81 @@ namespace Gremlin.Net.UnitTest.Process.Traversal Assert.True(result.Parameters.ContainsKey("ids")); } + [Fact] + public void GValue_mid_underscore_name_accepted() + { + var gval = new GValue<int>("a_b", 42); + var result = _g.Inject((object)gval).GremlinLang; + Assert.Equal("g.inject(a_b)", result.GetGremlin()); + } + + [Fact] + public void GValue_empty_name_throws_ArgumentException() + { + Assert.Throws<ArgumentException>(() => new GValue<int>("", 1)); + } + + [Fact] + public void GValue_mid_dollar_name_throws_ArgumentException() + { + Assert.Throws<ArgumentException>(() => new GValue<int>("a$b", 1)); + } + + [Fact] + public void GValue_unicode_letter_name_accepted() + { + var gval = new GValue<int>("caf\u00e9", 7); + var result = _g.Inject((object)gval).GremlinLang; + Assert.Equal("g.inject(caf\u00e9)", result.GetGremlin()); + } + + [Fact] + public void GValue_construction_and_accessors() + { + var gval = new GValue<string>("myName", "hello"); + Assert.Equal("myName", gval.Name); + Assert.Equal("hello", gval.Value); + Assert.Equal("hello", gval.ObjectValue); + } + + [Fact] + public void GValue_IsNull_true_when_null_value() + { + var gval = new GValue<string?>("x", null); + Assert.True(gval.IsNull); + } + + [Fact] + public void GValue_IsNull_false_when_non_null_value() + { + var gval = new GValue<int>("x", 5); + Assert.False(gval.IsNull); + } + + [Fact] + public void GValue_ToString_format() + { + var gval = new GValue<int>("myVar", 42); + Assert.Equal("myVar=42", gval.ToString()); + } + + [Fact] + public void GValue_duplicate_name_equal_list_values_allowed() + { + var gval1 = new GValue<List<int>>("ids", new List<int> { 1, 2, 3 }); + var gval2 = new GValue<List<int>>("ids", new List<int> { 1, 2, 3 }); + var result = _g.Inject((object)gval1).V(gval2).GremlinLang; + Assert.Equal("g.inject(ids).V(ids)", result.GetGremlin()); + } + + [Fact] + public void GValue_duplicate_name_different_list_values_throws() + { + var gval1 = new GValue<List<int>>("ids", new List<int> { 1, 2, 3 }); + var gval2 = new GValue<List<int>>("ids", new List<int> { 4, 5, 6 }); + Assert.Throws<ArgumentException>(() => _g.Inject((object)gval1).V(gval2)); + } + // --- Cardinality with Map Tests --- [Fact] @@ -1088,5 +1163,23 @@ namespace Gremlin.Net.UnitTest.Process.Traversal var result = GremlinLang.ConvertParametersToString(parameters); Assert.Contains("\"name\":", result); } + + [Fact] + public void GValue_duplicate_name_equal_dictionary_values_allowed() + { + var gval1 = new GValue<Dictionary<string, int>>("m", new Dictionary<string, int> { { "a", 1 }, { "b", 2 } }); + var gval2 = new GValue<Dictionary<string, int>>("m", new Dictionary<string, int> { { "a", 1 }, { "b", 2 } }); + var result = _g.Inject((object)gval1).V(gval2).GremlinLang; + Assert.Equal("g.inject(m).V(m)", result.GetGremlin()); + } + + [Fact] + public void GValue_duplicate_name_equal_nested_collection_values_allowed() + { + var gval1 = new GValue<List<List<int>>>("n", new List<List<int>> { new List<int> { 1, 2 }, new List<int> { 3, 4 } }); + var gval2 = new GValue<List<List<int>>>("n", new List<List<int>> { new List<int> { 1, 2 }, new List<int> { 3, 4 } }); + var result = _g.Inject((object)gval1).V(gval2).GremlinLang; + Assert.Equal("g.inject(n).V(n)", result.GetGremlin()); + } } } diff --git a/gremlin-go/driver/gValue.go b/gremlin-go/driver/gValue.go index f69ec2599c..32b260da98 100644 --- a/gremlin-go/driver/gValue.go +++ b/gremlin-go/driver/gValue.go @@ -21,7 +21,7 @@ package gremlingo import ( "fmt" - "strings" + "unicode" ) // GValue is a variable or literal value that is used in a Traversal. It is composed of a key-value pair where the key @@ -31,14 +31,32 @@ type GValue struct { value interface{} } -// NewGValue creates a new GValue to be used in traversals. The GValue name cannot begin with "_". +// NewGValue creates a new GValue to be used in traversals. The name must be non-empty, start with a +// Unicode letter, and contain only Unicode letters, digits, or '_'. It cannot begin with "_". func NewGValue(name string, value interface{}) GValue { - if strings.HasPrefix(name, "_") { + runes := []rune(name) + if len(runes) > 0 && runes[0] == '_' { panic(fmt.Sprintf("invalid GValue name '%v'. Should not start with _.", name)) } + if !isValidGValueName(name) { + panic(fmt.Sprintf("invalid GValue name '%v'.", name)) + } return GValue{name, value} } +func isValidGValueName(name string) bool { + runes := []rune(name) + if len(runes) == 0 || !unicode.IsLetter(runes[0]) { + return false + } + for _, r := range runes[1:] { + if !unicode.IsLetter(r) && !unicode.IsDigit(r) && r != '_' { + return false + } + } + return true +} + // Name returns the name of the GValue. func (gv GValue) Name() string { return gv.name @@ -53,3 +71,8 @@ func (gv GValue) IsNil() bool { func (gv GValue) Value() interface{} { return gv.value } + +// String returns the string representation of the GValue in the format "name=value". +func (gv GValue) String() string { + return fmt.Sprintf("%v=%v", gv.name, gv.value) +} diff --git a/gremlin-go/driver/gValue_test.go b/gremlin-go/driver/gValue_test.go index 45b365262f..4f93f981b2 100644 --- a/gremlin-go/driver/gValue_test.go +++ b/gremlin-go/driver/gValue_test.go @@ -68,20 +68,54 @@ func TestGValue(t *testing.T) { }) t.Run("test invalid name that starts with _", func(t *testing.T) { - g := NewGraphTraversalSource(nil, nil) - assert.Panics(t, func() { g.Inject(NewGValue("_ids", [2]int{1, 2})) }, - "invalid GValue name _1. Should not start with _.") + assert.Panics(t, func() { NewGValue("_ids", [2]int{1, 2}) }, + "invalid GValue name _ids. Should not start with _.") }) t.Run("test name is valid identifier", func(t *testing.T) { - g := NewGraphTraversalSource(nil, nil) - assert.Panics(t, func() { g.Inject(NewGValue("1a", [2]int{1, 2})) }, - "invalid parameter name '1a'") + assert.Panics(t, func() { NewGValue("1a", [2]int{1, 2}) }, + "invalid GValue name '1a'.") }) t.Run("test name is not a number", func(t *testing.T) { + assert.Panics(t, func() { NewGValue("1", [2]int{1, 2}) }, + "invalid GValue name '1'.") + }) + + t.Run("test mid-string underscore name accepted", func(t *testing.T) { + assert.NotPanics(t, func() { NewGValue("a_b", 1) }) + }) + + t.Run("test empty-string name rejected", func(t *testing.T) { + assert.Panics(t, func() { NewGValue("", 1) }) + }) + + t.Run("test mid-string dollar sign rejected", func(t *testing.T) { + assert.Panics(t, func() { NewGValue("a$b", 1) }) + }) + + t.Run("test unicode letter name accepted", func(t *testing.T) { + assert.NotPanics(t, func() { NewGValue("café", 1) }) + }) + + t.Run("test IsNil returns true for nil value", func(t *testing.T) { + gv := NewGValue("x", nil) + assert.True(t, gv.IsNil()) + }) + + t.Run("test String representation", func(t *testing.T) { + gv := NewGValue("x", 1) + assert.Equal(t, "x=1", gv.String()) + }) + + t.Run("test Go keyword name accepted", func(t *testing.T) { + assert.NotPanics(t, func() { NewGValue("for", 1) }) + }) + + t.Run("test distinct but equal slices allowed under same name", func(t *testing.T) { g := NewGraphTraversalSource(nil, nil) - assert.Panics(t, func() { g.Inject(NewGValue("1", [2]int{1, 2})) }, - "invalid parameter name '1'") + param1 := NewGValue("ids", []int{1, 2, 3}) + param2 := NewGValue("ids", []int{1, 2, 3}) + assert.NotPanics(t, func() { g.Inject(param1).V(param2) }) }) } diff --git a/gremlin-go/driver/gremlinlang.go b/gremlin-go/driver/gremlinlang.go index 2625cb10e6..8eb46509ab 100644 --- a/gremlin-go/driver/gremlinlang.go +++ b/gremlin-go/driver/gremlinlang.go @@ -22,7 +22,6 @@ package gremlingo import ( "encoding/base64" "fmt" - "go/token" "math" "math/big" "reflect" @@ -246,17 +245,9 @@ func (gl *GremlinLang) argAsString(arg interface{}) (string, error) { return v.GetGremlin("__"), nil case GValue: key := v.Name() - if !token.IsIdentifier(key) { - panic(fmt.Sprintf("invalid parameter name '%v'.", key)) - } value := v.Value() if val, ok := gl.parameters[key]; ok { - if reflect.TypeOf(val).Kind() == reflect.Slice || reflect.TypeOf(value).Kind() == reflect.Slice || - reflect.TypeOf(val).Kind() == reflect.Map || reflect.TypeOf(value).Kind() == reflect.Map { - if !reflect.DeepEqual(val, value) { - panic(fmt.Sprintf("parameter with name '%v' already exists.", key)) - } - } else if val != value { + if !reflect.DeepEqual(val, value) { panic(fmt.Sprintf("parameter with name '%v' already exists.", key)) } } else { diff --git a/gremlin-python/src/main/python/gremlin_python/process/traversal.py b/gremlin-python/src/main/python/gremlin_python/process/traversal.py index 76c91822e7..e575f1e572 100644 --- a/gremlin-python/src/main/python/gremlin_python/process/traversal.py +++ b/gremlin-python/src/main/python/gremlin_python/process/traversal.py @@ -918,9 +918,6 @@ class GremlinLang(object): if isinstance(arg, GValue): key = arg.get_name() - if not key.isidentifier(): - raise Exception(f'invalid parameter name {key}.') - if key in self.parameters: if self.parameters[key] != arg.value: raise Exception(f'parameter with name {key} already exists.') @@ -1161,10 +1158,8 @@ class GremlinLang(object): class GValue: def __init__(self, name, value): - if name is None: - raise Exception("The parameter name cannot be None.") - if name.startswith('_'): - raise Exception(f'invalid GValue name {name}. Should not start with _.') + if not name or not name[0].isalpha() or not all(c.isalnum() or c == '_' for c in name[1:]): + raise Exception(f'invalid GValue name {name}.') self.name = name self.value = value @@ -1177,6 +1172,12 @@ class GValue: def get(self): return self.value + def __repr__(self): + return f'{self.name}={self.value}' + + def __str__(self): + return f'{self.name}={self.value}' + class CardinalityValue(GremlinLang): def __init__(self, cardinality, val): diff --git a/gremlin-python/src/main/python/tests/integration/driver/test_driver_remote_connection.py b/gremlin-python/src/main/python/tests/integration/driver/test_driver_remote_connection.py index 604190b0c6..2891851148 100644 --- a/gremlin-python/src/main/python/tests/integration/driver/test_driver_remote_connection.py +++ b/gremlin-python/src/main/python/tests/integration/driver/test_driver_remote_connection.py @@ -52,7 +52,7 @@ class TestDriverRemoteConnection(object): 'bulkResults': True} assert 6 == t.to_list()[0] - @pytest.mark.skip(reason="investigate why 'ids' parameter name fails to parse in gremlin-lang") + @pytest.mark.skip(reason="TINKERPOP-3126: g.V() with a variable/parameter argument fails to parse in gremlin-lang") def test_extract_request_options_with_params(self, remote_connection): g = traversal().with_(remote_connection) t = g.with_("evaluationTimeout", @@ -62,7 +62,7 @@ class TestDriverRemoteConnection(object): 'evaluationTimeout': 1000, 'userAgent': 'test', 'bulkResults': True, - 'params': {'ids': [1, 2, 3]}} + 'bindings': "['ids':[1,2,3]]"} assert 3 == t.to_list()[0] def test_traversals(self, remote_connection): diff --git a/gremlin-python/src/main/python/tests/unit/process/test_gremlin_lang.py b/gremlin-python/src/main/python/tests/unit/process/test_gremlin_lang.py index edf0515317..b3e7f4f09c 100644 --- a/gremlin-python/src/main/python/tests/unit/process/test_gremlin_lang.py +++ b/gremlin-python/src/main/python/tests/unit/process/test_gremlin_lang.py @@ -483,39 +483,53 @@ class TestGremlinLang(object): assert gremlin_lang == tests[t][1] def test_gvalue_name_cannot_be_null(self): - g = traversal().with_(None) try: - g.V(GValue(None, [1, 2, 3])) + GValue(None, [1, 2, 3]) except Exception as ex: - assert str(ex) == 'The parameter name cannot be None.' + assert str(ex) == 'invalid GValue name None.' - def test_gvalue_name_dont_need_escaping(self): + def test_gvalue_name_empty_string_rejected(self): + try: + GValue('', [1, 2, 3]) + except Exception as ex: + assert str(ex) == 'invalid GValue name .' + + def test_gvalue_name_mid_string_dollar_rejected(self): + try: + GValue('a$b', [1, 2, 3]) + except Exception as ex: + assert str(ex) == 'invalid GValue name a$b.' + + def test_gvalue_name_unicode_letter_accepted(self): g = traversal().with_(None) + p = GValue('café', 42) + gremlin = g.V(p).gremlin_lang + assert 'g.V(café)' == gremlin.get_gremlin() + assert 42 == gremlin.get_parameters().get('café') + + def test_gvalue_name_dont_need_escaping(self): try: - g.V(GValue('\"', [1, 2, 3])) + GValue('"', [1, 2, 3]) except Exception as ex: - assert str(ex) == 'invalid parameter name ".' + assert str(ex) == 'invalid GValue name ".' def test_gvalue_is_not_number(self): - g = traversal().with_(None) try: - g.V(GValue('1', [1, 2, 3])) + GValue('1', [1, 2, 3]) except Exception as ex: - assert str(ex) == 'invalid parameter name 1.' + assert str(ex) == 'invalid GValue name 1.' def test_gvalue_is_valid_identifier(self): - g = traversal().with_(None) try: - g.V(GValue('1a', [1, 2, 3])) + GValue('1a', [1, 2, 3]) except Exception as ex: - assert str(ex) == 'invalid parameter name 1a.' + assert str(ex) == 'invalid GValue name 1a.' def test_gvalue_is_not_reserved(self): - g = traversal().with_(None) try: - g.V(GValue('_1', [1, 2, 3])) + GValue('_1', [1, 2, 3]) except Exception as ex: - assert str(ex) == 'invalid GValue name _1. Should not start with _.' + assert str(ex) == 'invalid GValue name _1.' def test_gvalue_is_not_duplicate(self): g = traversal().with_(None) @@ -532,6 +546,30 @@ class TestGremlinLang(object): assert 'g.inject(ids).V(ids)' == gremlin.get_gremlin() assert val == gremlin.get_parameters().get('ids') + def test_gvalue_mid_string_underscore_accepted(self): + g = traversal().with_(None) + p = GValue('a_b', 42) + gremlin = g.V(p).gremlin_lang + assert 'g.V(a_b)' == gremlin.get_gremlin() + assert 42 == gremlin.get_parameters().get('a_b') + + def test_gvalue_construction_and_accessors(self): + p = GValue('x', 1) + assert 'x' == p.get_name() + assert 1 == p.get() + assert p.is_null() is False + + def test_gvalue_is_null(self): + p = GValue('n', None) + assert p.is_null() is True + q = GValue('m', 0) + assert q.is_null() is False + + def test_gvalue_string_representation(self): + p = GValue('x', 1) + assert repr(p) == 'x=1' + assert str(p) == 'x=1' + def test_unsupported_type_throws(self): g = traversal().with_(None) import pytest
