[
https://issues.apache.org/jira/browse/TINKERPOP-2824?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17690453#comment-17690453
]
ASF GitHub Bot commented on TINKERPOP-2824:
-------------------------------------------
FlorianHockmann commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1108148751
##########
gremlin-dotnet/src/Gremlin.Net/Structure/IO/GraphBinary/Types/EdgeSerializer.cs:
##########
@@ -45,15 +46,17 @@ public EdgeSerializer() : base(DataType.Edge)
{
await writer.WriteAsync(value.Id, stream,
cancellationToken).ConfigureAwait(false);
await writer.WriteNonNullableValueAsync(value.Label, stream,
cancellationToken).ConfigureAwait(false);
-
+
+ // !!! await writer.WriteValueAsync(value.Label, stream, false,
cancellationToken).ConfigureAwait(false);
Review Comment:
?
##########
gremlin-dotnet/src/Gremlin.Net/Structure/Edge.cs:
##########
@@ -52,6 +55,15 @@ public Edge(object? id, Vertex outV, string label, Vertex
inV)
/// </summary>
public Vertex OutV { get; }
+ /// <summary>
+ /// Get property by key
+ /// </summary>
+ /// <returns>property or null when not found</returns>
+ public Property? Property(string key)
Review Comment:
Any reason against implementing this in `Element` instead of here and in
`Vertex`?
##########
gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Traversal/DriverRemoteConnection/RemoteConnectionFactory.cs:
##########
@@ -49,19 +49,32 @@ public IRemoteConnection CreateRemoteConnection(int
connectionPoolSize = 2)
return CreateRemoteConnection("gmodern", connectionPoolSize);
}
- public IRemoteConnection CreateRemoteConnection(string
traversalSource, int connectionPoolSize = 2)
+ public IRemoteConnection CreateRemoteConnection(string
traversalSource,
+ int connectionPoolSize = 2,
+ IMessageSerializer? messageSerializer = null)
{
var c = new DriverRemoteConnectionImpl(
- new GremlinClient(new GremlinServer(TestHost, TestPort),
_messageSerializer,
+ new GremlinClient(new GremlinServer(TestHost, TestPort),
Review Comment:
(nitpick) Why not call the newly added method `CreateClient` here?
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/test/integration/client-tests.js:
##########
@@ -75,6 +75,68 @@ describe('Client', function () {
});
});
+ it('should handle properties for bytecode request', function () {
+ return client.submit(new Bytecode().addStep('V', [1]))
+ .then(function (result) {
+ assert.ok(result);
+ assert.strictEqual(result.length, 1);
+ const vertex = result.first().object;
+ assert.ok(vertex instanceof graphModule.Vertex);
+ let age, name
+ if (vertex.properties instanceof Array) {
+ age = vertex.properties[1]
+ name = vertex.properties[0]
+ } else {
+ age = vertex.properties.age[0]
+ name = vertex.properties.name[0]
+ }
+ assert.strictEqual(age.value, 29);
+ assert.strictEqual(name.value, 'marko');
+ });
+ });
+
+ it('should skip properties for bytecode request', function () {
Review Comment:
(nitpick)
```suggestion
it('should skip properties for bytecode request with tokens', function
() {
```
##########
gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Traversal/DriverRemoteConnection/RemoteConnectionFactory.cs:
##########
@@ -49,19 +49,32 @@ public IRemoteConnection CreateRemoteConnection(int
connectionPoolSize = 2)
return CreateRemoteConnection("gmodern", connectionPoolSize);
}
- public IRemoteConnection CreateRemoteConnection(string
traversalSource, int connectionPoolSize = 2)
+ public IRemoteConnection CreateRemoteConnection(string
traversalSource,
+ int connectionPoolSize = 2,
+ IMessageSerializer? messageSerializer = null)
{
var c = new DriverRemoteConnectionImpl(
- new GremlinClient(new GremlinServer(TestHost, TestPort),
_messageSerializer,
+ new GremlinClient(new GremlinServer(TestHost, TestPort),
+ messageSerializer ?? _messageSerializer,
connectionPoolSettings: new ConnectionPoolSettings {
PoolSize = connectionPoolSize }),
traversalSource);
- _connections.Add(c);
+ _cleanUp.Add(c);
+ return c;
+ }
+
+ public IGremlinClient CreateClient(IMessageSerializer
messageSerializer = null)
Review Comment:
Please add a nullable annotation:
```suggestion
public IGremlinClient CreateClient(IMessageSerializer?
messageSerializer = null)
```
##########
gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Driver/PropertyDeserializationTests.cs:
##########
@@ -0,0 +1,247 @@
+#region License
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#endregion
+
+using Gremlin.Net.Driver;
+using Gremlin.Net.IntegrationTest.Process.Traversal.DriverRemoteConnection;
+using Gremlin.Net.Process.Traversal;
+using Gremlin.Net.Structure;
+using Gremlin.Net.Structure.IO.GraphBinary;
+using Gremlin.Net.Structure.IO.GraphSON;
+using System.Collections.Generic;
+using System.Linq;
+using System.Threading.Tasks;
+using Xunit;
+
+namespace Gremlin.Net.IntegrationTest.Driver
+{
+ public class PropertyDeserializationTests
+ {
+ private readonly RemoteConnectionFactory _connectionFactory = new();
+
+ [Theory]
+ [MemberData(nameof(Serializers))]
+ public void
ShouldDeserializeVertexPropertiesForBytecode(IMessageSerializer serializer)
+ {
+ var connection =
_connectionFactory.CreateRemoteConnection("gmodern", 2, serializer);
+ var g =
AnonymousTraversalSource.Traversal().WithRemote(connection);
+
+ var vertex = g.V(1).Next();
+
+ VerifyVertexProperties(vertex);
+ }
+
+ [Theory]
+ [MemberData(nameof(Serializers))]
+ public void
ShouldRespectMaterializePropertiesTokensForBytecode(IMessageSerializer
serializer)
+ {
+ var connection =
_connectionFactory.CreateRemoteConnection("gmodern", 2, serializer);
+ var g =
AnonymousTraversalSource.Traversal().WithRemote(connection);
+
+ var vertex = g.With(Tokens.ArgMaterializeProperties,
"tokens").V(1).Next();
+
+ VerifyEmptyProperties(vertex);
+ }
+
+ [Theory]
+ [MemberData(nameof(Serializers))]
+ public void
ShouldRespectMaterializePropertiesAllForBytecode(IMessageSerializer serializer)
+ {
+ var connection =
_connectionFactory.CreateRemoteConnection("gmodern", 2, serializer);
+ var g =
AnonymousTraversalSource.Traversal().WithRemote(connection);
+
+ var vertex = g.With(Tokens.ArgMaterializeProperties,
"all").V(1).Next();
+
+ VerifyVertexProperties(vertex);
+ }
+
+ [Theory]
+ [MemberData(nameof(Serializers))]
+ public void
ShouldHandleEmptyVertexPropertiesForBytecode(IMessageSerializer serializer)
+ {
+ var connection =
_connectionFactory.CreateRemoteConnection("gimmutable", 2, serializer);
+ var g =
AnonymousTraversalSource.Traversal().WithRemote(connection);
+
+ var vertex = g.AddV("test").Next();
+
+ VerifyEmptyProperties(vertex);
+ }
+
+ [Theory]
+ [MemberData(nameof(Serializers))]
+ public async Task
ShouldDeserializeVertexPropertiesForGremlin(IMessageSerializer serializer)
+ {
+ var client = _connectionFactory.CreateClient(serializer);
+
+ var vertex = await
client.SubmitWithSingleResultAsync<Vertex>("gmodern.V(1)");
+
+ VerifyVertexProperties(vertex);
+ }
+
+ [Theory]
+ [MemberData(nameof(Serializers))]
+ public async Task
ShouldHandleEmptyVertexPropertiesForGremlin(IMessageSerializer serializer)
+ {
+ var client = _connectionFactory.CreateClient(serializer);
+
+ var vertex = await
client.SubmitWithSingleResultAsync<Vertex>("gimmutable.addV('test')");
+
+ VerifyEmptyProperties(vertex);
+ }
+
+ [Theory]
+ [MemberData(nameof(Serializers))]
+ public async Task
ShouldRespectMaterializePropertiesAllForGremlin(IMessageSerializer serializer)
+ {
+ var client = _connectionFactory.CreateClient(serializer);
+
+ var vertex = await
client.SubmitWithSingleResultAsync<Vertex>("gmodern.with('materializeProperties',
'all').V(1)");
+
+ VerifyVertexProperties(vertex);
+ }
+
+ [Theory]
+ [MemberData(nameof(Serializers))]
+ public async Task
ShouldRespectMaterializePropertiesTokensForGremlin(IMessageSerializer
serializer)
+ {
+ var client = _connectionFactory.CreateClient(serializer);
+
+ var vertex = await
client.SubmitWithSingleResultAsync<Vertex>("gmodern.with('materializeProperties',
'tokens').V(1)");
+
+ VerifyEmptyProperties(vertex);
+ }
+
+ [Theory]
+ [MemberData(nameof(Serializers))]
+ public void
ShouldDeserializeEdgePropertiesForBytecode(IMessageSerializer serializer)
+ {
+ var connection =
_connectionFactory.CreateRemoteConnection("gmodern", 2, serializer);
+ var g =
AnonymousTraversalSource.Traversal().WithRemote(connection);
+
+ var edge = g.E(7).Next();
+
+ VerifyEdgeProperties(edge);
+ }
+
+ [Theory]
+ [MemberData(nameof(Serializers))]
+ public void
ShouldHandleEmptyEdgePropertiesForBytecode(IMessageSerializer serializer)
+ {
+ var connection =
_connectionFactory.CreateRemoteConnection("gimmutable", 2, serializer);
+ var g =
AnonymousTraversalSource.Traversal().WithRemote(connection);
+
+ var v1 = g.AddV("v1").Next();
+ var v2 = g.AddV("v2").Next();
+ var edge = g.AddE("test").From(v1).To(v2).Next();
+
+ VerifyEmptyProperties(edge);
+ }
+
+ [Theory]
+ [MemberData(nameof(Serializers))]
+ public async Task
ShouldDeserializeEdgePropertiesForGremlin(IMessageSerializer serializer)
+ {
+ var client = _connectionFactory.CreateClient(serializer);
+
+ var edge = await
client.SubmitWithSingleResultAsync<Edge>("gmodern.E(7)");
+
+ VerifyEdgeProperties(edge);
+ }
+
+ [Theory]
+ [MemberData(nameof(Serializers))]
+ public async Task
ShouldHandleEmptyEdgePropertiesForGremlin(IMessageSerializer serializer)
+ {
+ var client = _connectionFactory.CreateClient(serializer);
+
+ var edge = await client.SubmitWithSingleResultAsync<Edge>(
+
"gimmutable.addV().as('v1').addV().as('v2').addE('test').from('v1').to('v2')");
+
+ VerifyEmptyProperties(edge);
+ }
+
+ [Theory]
+ [MemberData(nameof(Serializers))]
+ public void
ShouldHandleMultiplePropertiesWithSameNameForVertex(IMessageSerializer
serializer)
+ {
+ var connection =
_connectionFactory.CreateRemoteConnection("gimmutable", 2, serializer);
+ var g =
AnonymousTraversalSource.Traversal().WithRemote(connection);
+
+ var vertex = g.AddV()
+ .Property(Cardinality.List, "test", "value1")
+ .Property(Cardinality.List, "test", "value2")
+ .Property(Cardinality.List, "test", "value3")
+ .Next()!;
+
+ vertex = g.V(vertex.Id).Next();
+
+ Assert.NotNull(vertex);
+
+ var properties = vertex.Properties!;
+ Assert.Equal(3, properties.Length);
+ var propertyValues = properties.Cast<VertexProperty>().Where(p =>
p.Key == "test").Select(p => p.Value).ToArray();
+ Assert.Equal(3, propertyValues.Length);
+ Assert.Equal(new[] { "value1", "value2", "value3" },
propertyValues);
+ }
+
+ private static void VerifyVertexProperties(Vertex? vertex)
+ {
+ Assert.NotNull(vertex);
+ Assert.Equal(1, vertex.Id);
+
+ Assert.Equal(2, vertex.Properties!.Length);
+ var age = vertex.Property("age");
+ Assert.NotNull(age);
+ Assert.Equal(29, age.Value);
+ }
+
+ private static void VerifyEdgeProperties(Edge? edge)
+ {
+ Assert.NotNull(edge);
+ Assert.Equal(7, edge.Id);
+
+ Assert.Single(edge.Properties!);
+ var weight = edge.Property("weight");
+ Assert.NotNull(weight);
+ Assert.Equal(0.5, weight.Value);
+ }
+
+ private static void VerifyEmptyProperties(Vertex? vertex)
Review Comment:
Not that important but you could easily let this take an `Element` instead
and then remove the method below.
##########
gremlin-go/driver/graphBinary.go:
##########
@@ -1052,8 +1052,8 @@ func vertexReader(data *[]byte, i *int) (interface{},
error) {
return vertexReaderNullByte(data, i, true)
}
-// {fully qualified id}{unqualified label}{[unused null byte]}
-func vertexReaderNullByte(data *[]byte, i *int, unusedByte bool) (interface{},
error) {
+// {fully qualified id}{unqualified label}{fully qualified properties}
+func vertexReaderNullByte(data *[]byte, i *int, readProperties bool)
(interface{}, error) {
Review Comment:
Shouldn't this now be named `vertexReaderReadingProperties` or something
like that?
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/test/integration/client-tests.js:
##########
@@ -75,6 +75,68 @@ describe('Client', function () {
});
});
+ it('should handle properties for bytecode request', function () {
+ return client.submit(new Bytecode().addStep('V', [1]))
+ .then(function (result) {
+ assert.ok(result);
+ assert.strictEqual(result.length, 1);
+ const vertex = result.first().object;
+ assert.ok(vertex instanceof graphModule.Vertex);
+ let age, name
+ if (vertex.properties instanceof Array) {
+ age = vertex.properties[1]
+ name = vertex.properties[0]
+ } else {
+ age = vertex.properties.age[0]
+ name = vertex.properties.name[0]
+ }
+ assert.strictEqual(age.value, 29);
+ assert.strictEqual(name.value, 'marko');
+ });
+ });
+
+ it('should skip properties for bytecode request', function () {
+ return client.submit(new Bytecode().addStep('V', [1]), null,
{'materializeProperties': 'tokens'})
+ .then(function (result) {
+ assert.ok(result);
+ assert.strictEqual(result.length, 1);
+ const vertex = result.first().object;
+ assert.ok(vertex instanceof graphModule.Vertex);
+ assert.ok(vertex.properties === undefined ||
vertex.properties.length === 0);
+ });
+ });
+
+ it('should handle properties for gremlin request', function () {
+ return client.submit('g.V(1)')
+ .then(function (result) {
+ assert.ok(result);
+ assert.strictEqual(result.length, 1);
+ const vertex = result.first();
+ assert.ok(vertex instanceof graphModule.Vertex);
+ let age, name
+ if (vertex.properties instanceof Array) {
+ age = vertex.properties[1]
+ name = vertex.properties[0]
+ } else {
+ age = vertex.properties.age[0]
+ name = vertex.properties.name[0]
+ }
+ assert.strictEqual(age.value, 29);
+ assert.strictEqual(name.value, 'marko');
+ });
+ });
+
+ it('should skip properties for gremlin request', function () {
Review Comment:
```suggestion
it('should skip properties for gremlin request with tokens', function ()
{
```
##########
gremlin-python/src/main/python/tests/structure/io/test_functionalityio.py:
##########
@@ -25,6 +25,34 @@
from gremlin_python.statics import *
+def test_vertex(remote_connection):
+ g = Graph().traversal().withRemote(remote_connection)
+ vertex = g.V(1).next()
+ assert vertex.id == 1
+ assert len(vertex.properties) == 2
+ assert vertex.properties[0].key == 'name'
+ assert vertex.properties[0].value == 'marko'
+ assert vertex.properties[1].key == 'age'
+ assert vertex.properties[1].value == 29
+
+
+def test_vertex_without_properties(remote_connection):
+ g = Graph().traversal().withRemote(remote_connection)
+ vertex = g.with_('materializeProperties', 'tokens').V(1).next()
+ assert vertex.id == 1
+ # empty array for GraphBinary and missing field for GraphSON
+ assert vertex.properties is None or len(vertex.properties) == 0
+
+
+def test_edge(remote_connection):
Review Comment:
(nitpick) You could also add a test for an edge with `tokens`.
##########
gremlin-server/src/test/scripts/generate-all.groovy:
##########
@@ -61,7 +61,7 @@ globals << [hook : [
// add default TraversalSource instances for each graph instance
globals << [gclassic :
traversal().withEmbedded(classic).withStrategies(ReferenceElementStrategy)]
-globals << [gmodern :
traversal().withEmbedded(modern).withStrategies(ReferenceElementStrategy)]
+globals << [gmodern : traversal().withEmbedded(modern)]
Review Comment:
Just for my own understanding: You're removing this here so `gmodern` can be
used by tests where we want to get properties back, right?
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/test/cucumber/feature-steps.js:
##########
@@ -131,17 +131,30 @@ Given(/^using the parameter (.+) defined as "(.+)"$/,
function (paramName, strin
});
});
+var removeProperties = function(p) {
+ if (p === undefined) {
+ } else if (p instanceof graphModule.Vertex || p instanceof graphModule.Edge)
{
+ p.properties = undefined;
+ } else if (p instanceof Array) {
+ p.forEach(removeProperties)
+ } else if (p instanceof Map) {
+ removeProperties(Array.from(p.keys()))
+ removeProperties(Array.from(p.values()))
+ } else if (p instanceof graphModule.Path) {
+ removeProperties(p.objects)
+ }
+
+ return p
+}
+
When('iterated to list', function () {
- return this.traversal.toList().then(list => this.result = list).catch(err =>
this.result = err);
+ return this.traversal.toList().then(list => this.result =
removeProperties(list)).catch(err => this.result = err);
Review Comment:
Shouldn't we make sure that things like element equality is handled the same
way across GLVs? I think JS should also treat elements as equal if their IDs
are equal to stay consistent with Java & .NET.
But that's probably out of scope for this PR. We can also create an issue
for this if we agree that all GLVs should behave the same in this regard.
> Properties on Elements
> ----------------------
>
> Key: TINKERPOP-2824
> URL: https://issues.apache.org/jira/browse/TINKERPOP-2824
> Project: TinkerPop
> Issue Type: Improvement
> Components: dotnet, driver, go, javascript, process, python
> Affects Versions: 3.5.4
> Reporter: Valentyn Kahamlyk
> Assignee: Valentyn Kahamlyk
> Priority: Major
>
> Problem: When a user writes `g.V()` they get back a Vertex object. The
> problem is that depending on the execution context of the traversal, the
> result could be quite different, with or without properties.
> Solution: Implement new finalization strategy DetachStrategy(detachMode,
> properties) where mode is one of ALL, NONE or CUSTOM. `properties` is list of
> properties name, are taken into account only for CUSTOM mode.
> Discussion thread in dev list: [Proposal to handle properties on response
> Elements-Apache Mail
> Archives|https://lists.apache.org/thread/l8rw7ydj7kym8vhtwk50nhbp45ng9986]
> Stephen's thread in dev list: [The Issue of Detachment-Apache Mail
> Archives|https://lists.apache.org/thread/xltcon4zxnwq4fyw2r2126syyrqm8spy]
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
