[TINKERPOP-3105] Running 3.6.x python-driver with 3.7.x server leads …

…to deserialization errors (apache#2742)

Added check and returned `null` for properties in reference elements instead of empty list in GraphBinary serializer for compatibility, and changed deserialization of null properties from reference elements into empty list for compatibility.

CHANGELOG.asciidoc

@@ -36,6 +36,7 @@ This release also includes changes from <<release-3-6-8, 3.6.8>>.
 * Fix cases where Map keys of incomparable types could panic in `gremlin-go`.
 * Fixed an issue where missing necessary parameters for logging, resulting in '%!x(MISSING)' output in `gremlin-go`.
 * Added getter method to `ConcatStep`, `ConjoinStep`, `SplitGlobalStep` and `SplitLocalStep` for their private fields.
+* When using `ReferenceElementStrategy` with `GraphBinaryV1`, properties on elements will be returned as `null` instead of empty list, to stay compatible with older drivers.
 * Gremlin Server docker containers shutdown gracefully when receiving a SIGTERM.
 * Added DefaultIdManager.STRING for proper string id creation/handling.
 * Allowed specification of an `Operator` as a reducer in `withSideEffect` when parsing with the grammar.

docs/src/reference/gremlin-applications.asciidoc

@@ -2336,6 +2336,45 @@ List<Vertex> results = g.V().hasLabel("person").elementMap('name').toList();
 
 Both of the above requests return a list of `Map` instances that contain the `id`, `label` and the "name" property.
 
+*Compatibility*
+
+*It is not recommended to use 3.6.x or below driver versions with 3.7.x or above Gremlin Server*, as some older drivers do not construct
+graph elements with properties and thus are not designed to handle the returned properties by default; however, compatibility
+can be achieved by configuring `ReferenceElementStrategy` in the server such that properties are not returned.
+Per-request configuration option `materializeProperties` is not supported older driver versions.
+
+Also note that older drivers of different language variants will handle incoming properties differently with different
+serializers used. Drivers using `GraphSON` serializers will remain compatible, but may encounter deserialization errors
+with `GraphBinary`. Below is a table documenting GLV behaviors using `GraphBinary` when properties are returned by the
+default 3.7.x server, as well as if `ReferenceElementStrategy` is configured (i.e. mimic the behavior
+of a 3.6.x server). This can be observed with the results of `g.V().next()`. Note that only `gremlin-driver`
+and `gremlin-javacript` have the `properties` attribute in the Element objects, all other GLVs only have `id` and `label`.
+
+[cols="1,1,1"]
+|===
+|3.6.x drivers with `GraphBinary` |Behavior with default 3.7.x Server | Behavior with `ReferenceElementStrategy`
+
+|`gremlin-driver`
+|Properties returned as empty iterator
+|Properties returned as empty iterator
+
+|`gremlin-dotnet`
+|Skips properties in Elements
+|Skips properties in Elements
+
+|`gremlin-javascript`
+|Deserialization error
+|Properties returned as empty list
+
+|`gremlin-python`
+|Deserialization error
+|Skips properties in Elements
+
+|`gremlin-go`
+|Deserialization error
+|Skips properties in Elements
+|===
+
 TIP: Consider utilizing `ReferenceElementStrategy` whenever creating a `GraphTraversalSource` in Java to ensure 
 the most portable Gremlin.
 

docs/src/upgrade/release-3.7.x.asciidoc

@@ -21,7 +21,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
 
 *Gremfir Master of the Pan Flute*
 
-=== TinkerPop 3.7.3
+== TinkerPop 3.7.3
 
 *Release Date: NOT OFFICIALLY RELEASED YET*
 
@@ -30,6 +30,25 @@ complete list of all the modifications that are part of this release.
 
 === Upgrading for Users
 
+==== GraphBinary Deserialization of Reference Element Properties with ReferenceElementStrategy
+When properties on element were introduced and returned as default in 3.7.0, setting ReferenceElementStrategy on the
+server meant as a way to send reference element without properties, for lightweight wire transfer and compatibility
+reasons. However, an issue was discovered where using GraphBinary, the 3.7.x server were not serializing properties as
+`null` as per IO spec, but as empty lists instead. This caused deserialization failures in Python, JavaScript and Go
+driver versions 3.6.x or below.
+
+A fix was introduced to correct such error, where Gremlin Server versions 3.7.3 and above will return element properties
+as `null` when ReferenceElementStrategy is applied, or when `token` is used with `materializedProperties` option in 3.7.x
+drivers. However, this also led to a change in 3.7.x driver behavior, where all non-Java drivers returns `null` instead
+of empty list. As such, an additional change was introduced in these GLVs, where `null` properties from reference elements
+will now deserialized into an empty list, to maintain such behavior with older 3.7.x drivers. This should minimize impact for users.
+
+One caveat is that when using 3.7.0 to 3.7.2 drivers to connect to 3.7.3 and above server, these drivers will not contain
+the deserialization change and return `null` as properties. In these cases, it is recommended to upgrade to 3.7.3 drivers.
+
+See: link:https://tinkerpop.apache.org/docs/x.y.z/reference/#_properties_of_elements[Properties of Elements],
+link:https://issues.apache.org/jira/browse/TINKERPOP-3105[TINKERPOP-3105]
+
 == TinkerPop 3.7.2
 
 *Release Date: April 8, 2024*
@@ -705,7 +724,7 @@ For 3.7:
 ===== Enabling the previous behavior
 
 Note that drivers from earlier versions like 3.5 and 3.6 will not be able to retrieve properties on elements. Older
-drivers connecting to 3.7.x servers should disable this functionality server-side in one of two ways:
+drivers connecting to 3.7.x servers should disable this functionality server-side:
 
 *Configure Gremlin Server to not return properties* - update Gremlin Server initialization script with
 `ReferenceElementStrategy`. This configuration is essentially the one used in older versions of the server by default.
@@ -715,7 +734,7 @@ drivers connecting to 3.7.x servers should disable this functionality server-sid
 globals << [g : traversal().withEmbedded(graph).withStrategies(ReferenceElementStrategy)]
 ----
 
-*Disable property inclusion per request* - the `materializeProperties` has a `tokens` option for this purpose.
+For 3.7 drivers, properties on elements can also be disabled per request using the `tokens` option with `materializeProperties`.
 
 [source,csharp]
 ----

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/binary/types/EdgeSerializer.java

@@ -27,6 +27,7 @@
 import org.apache.tinkerpop.gremlin.structure.io.binary.GraphBinaryWriter;
 import org.apache.tinkerpop.gremlin.structure.util.detached.DetachedEdge;
 import org.apache.tinkerpop.gremlin.structure.util.detached.DetachedVertex;
+import org.apache.tinkerpop.gremlin.structure.util.reference.ReferenceEdge;
 
 import java.io.IOException;
 import java.util.List;
@@ -81,7 +82,7 @@ protected void writeValue(final Edge value, final Buffer buffer, final GraphBina
 
         // we don't serialize the parent Vertex for edges.
         context.write(null, buffer);
-        if (value.properties() == null) {
+        if (value instanceof ReferenceEdge) {
             context.write(null, buffer);
         }
         else {

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/binary/types/VertexPropertySerializer.java

@@ -26,6 +26,7 @@
 import org.apache.tinkerpop.gremlin.structure.VertexProperty;
 import org.apache.tinkerpop.gremlin.structure.io.Buffer;
 import org.apache.tinkerpop.gremlin.structure.util.detached.DetachedVertexProperty;
+import org.apache.tinkerpop.gremlin.structure.util.reference.ReferenceVertexProperty;
 
 import java.io.IOException;
 import java.util.Collections;
@@ -67,9 +68,14 @@ protected void writeValue(final VertexProperty value, final Buffer buffer, final
         // we don't serialize the parent vertex, let's hold a place for it
         context.write(null, buffer);
 
-        final List<?> asList = value.graph().features().vertex().supportsMetaProperties() ?
-                IteratorUtils.toList(value.properties()) :
-                Collections.emptyList();
-        context.write(asList, buffer);
+        if (value instanceof ReferenceVertexProperty) {
+            context.write(null, buffer);
+        }
+        else {
+            final List<?> asList = value.graph().features().vertex().supportsMetaProperties() ?
+                    IteratorUtils.toList(value.properties()) :
+                    Collections.emptyList();
+            context.write(asList, buffer);
+        }
     }
 }

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/binary/types/VertexSerializer.java

@@ -27,6 +27,7 @@
 import org.apache.tinkerpop.gremlin.structure.io.binary.GraphBinaryWriter;
 import org.apache.tinkerpop.gremlin.structure.util.detached.DetachedVertex;
 import org.apache.tinkerpop.gremlin.structure.util.detached.DetachedVertexProperty;
+import org.apache.tinkerpop.gremlin.structure.util.reference.ReferenceVertex;
 
 import java.io.IOException;
 import java.util.List;
@@ -60,7 +61,7 @@ protected Vertex readValue(final Buffer buffer, final GraphBinaryReader context)
     protected void writeValue(final Vertex value, final Buffer buffer, final GraphBinaryWriter context) throws IOException {
         context.write(value.id(), buffer);
         context.writeValue(value.label(), buffer, false);
-        if (value.properties() == null) {
+        if (value instanceof ReferenceVertex) {
             context.write(null, buffer);
         }
         else {

gremlin-dotnet/src/Gremlin.Net/Structure/IO/GraphBinary/Types/EdgeSerializer.cs

@@ -21,6 +21,7 @@
 
 #endregion
 
+using System;
 using System.Collections.Generic;
 using System.IO;
 using System.Threading;
@@ -76,7 +77,7 @@ protected override async Task<Edge> ReadValueAsync(Stream stream, GraphBinaryRea
             await reader.ReadAsync(stream, cancellationToken).ConfigureAwait(false);
 
             var properties = await reader.ReadAsync(stream, cancellationToken).ConfigureAwait(false);
-            var propertiesAsArray = (properties as List<object>)?.ToArray();
+            var propertiesAsArray = null == properties ? Array.Empty<object>() : (properties as List<object>)?.ToArray();
 
             return new Edge(id, outV, label, inV, propertiesAsArray);
         }

gremlin-dotnet/src/Gremlin.Net/Structure/IO/GraphBinary/Types/VertexPropertySerializer.cs

@@ -21,6 +21,7 @@
 
 #endregion
 
+using System;
 using System.Collections.Generic;
 using System.IO;
 using System.Threading;
@@ -68,7 +69,7 @@ protected override async Task<VertexProperty> ReadValueAsync(Stream stream, Grap
             await reader.ReadAsync(stream, cancellationToken).ConfigureAwait(false);
 
             var properties = await reader.ReadAsync(stream, cancellationToken).ConfigureAwait(false);
-            var propertiesAsArray = (properties as List<object>)?.ToArray();
+            var propertiesAsArray = null == properties ? Array.Empty<object>() : (properties as List<object>)?.ToArray();
 
             return new VertexProperty(id, label, value, null, propertiesAsArray);
         }

gremlin-dotnet/src/Gremlin.Net/Structure/IO/GraphBinary/Types/VertexSerializer.cs

@@ -21,6 +21,7 @@
 
 #endregion
 
+using System;
 using System.Collections.Generic;
 using System.IO;
 using System.Threading;
@@ -57,7 +58,7 @@ protected override async Task<Vertex> ReadValueAsync(Stream stream, GraphBinaryR
             var label = (string)await reader.ReadNonNullableValueAsync<string>(stream, cancellationToken).ConfigureAwait(false);
 
             var properties = await reader.ReadAsync(stream, cancellationToken).ConfigureAwait(false);
-            var propertiesAsArray = (properties as List<object>)?.ToArray();
+            var propertiesAsArray = null == properties ? Array.Empty<object>() : (properties as List<object>)?.ToArray();
 
             return new Vertex(id, label, propertiesAsArray);
         }

gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Traversal/DriverRemoteConnection/GraphTraversalTests.cs

@@ -291,5 +291,47 @@ public async Task ShouldThrowExceptionOnRollbackWhenGraphNotSupportTx()
             var exception = await Assert.ThrowsAsync<ResponseException>(async () => await tx.RollbackAsync());
             Assert.Equal("ServerError: Graph does not support transactions", exception.Message);
         }
+
+        [Fact]
+        public void shouldUseMaterializedPropertiesTokenInV()
+        {
+            var connection = _connectionFactory.CreateRemoteConnection();
+            var g = AnonymousTraversalSource.Traversal().WithRemote(connection);
+            var vertices = g.With("materializeProperties", "tokens").V().ToList();
+            foreach (var v in vertices)
+            {
+                Assert.NotNull(v);
+                // GraphSON will deserialize into null and GraphBinary to []
+                Assert.True(v.Properties == null || v.Properties.Length == 0);
+            }
+        }
+
+        [Fact]
+        public void shouldUseMaterializedPropertiesTokenInE()
+        {
+            var connection = _connectionFactory.CreateRemoteConnection();
+            var g = AnonymousTraversalSource.Traversal().WithRemote(connection);
+            var edges = g.With("materializeProperties", "tokens").E().ToList();
+            foreach (var e in edges)
+            {
+                Assert.NotNull(e);
+                // GraphSON will deserialize into null and GraphBinary to []
+                Assert.True(e.Properties == null || e.Properties.Length == 0);
+            }
+        }
+
+        [Fact]
+        public void shouldUseMaterializedPropertiesTokenInVP()
+        {
+            var connection = _connectionFactory.CreateRemoteConnection();
+            var g = AnonymousTraversalSource.Traversal().WithRemote(connection);
+            var vps = g.With("materializeProperties", "tokens").V().Properties<VertexProperty>().ToList();
+            foreach (var vp in vps)
+            {
+                Assert.NotNull(vp);
+                // GraphSON will deserialize into null and GraphBinary to []
+                Assert.True(vp.Properties == null || vp.Properties.Length == 0);
+            }
+        }
     }
 }

gremlin-go/driver/connection_test.go

@@ -1230,4 +1230,41 @@ func TestConnection(t *testing.T) {
 
 		AssertMarkoVertexWithProperties(t, r)
 	})
+
+	t.Run("Test DriverRemoteConnection Traversal With materializeProperties in Modern Graph", func(t *testing.T) {
+		skipTestsIfNotEnabled(t, integrationTestSuiteName, testNoAuthEnable)
+
+		g := getModernGraph(t, testNoAuthUrl, &AuthInfo{}, &tls.Config{})
+		defer g.remoteConnection.Close()
+
+		vertices, err := g.With("materializeProperties", MaterializeProperties.Tokens).V().ToList()
+		assert.Nil(t, err)
+		for _, res := range vertices {
+			v, _ := res.GetVertex()
+			assert.Nil(t, err)
+			properties, ok := v.Properties.([]interface{})
+			assert.True(t, ok)
+			assert.Equal(t, 0, len(properties))
+		}
+
+		edges, err := g.With("materializeProperties", MaterializeProperties.Tokens).E().ToList()
+		assert.Nil(t, err)
+		for _, res := range edges {
+			e, _ := res.GetEdge()
+			assert.Nil(t, err)
+			properties, ok := e.Properties.([]interface{})
+			assert.True(t, ok)
+			assert.Equal(t, 0, len(properties))
+		}
+
+		vps, err := g.With("materializeProperties", MaterializeProperties.Tokens).V().Properties().ToList()
+		assert.Nil(t, err)
+		for _, res := range vps {
+			vp, _ := res.GetVertexProperty()
+			assert.Nil(t, err)
+			properties, ok := vp.Properties.([]interface{})
+			assert.True(t, ok)
+			assert.Equal(t, 0, len(properties))
+		}
+	})
 }

gremlin-go/driver/graphBinary.go

@@ -1069,10 +1069,15 @@ func vertexReaderReadingProperties(data *[]byte, i *int, readProperties bool) (i
 	}
 	v.Label = label.(string)
 	if readProperties {
-		v.Properties, err = readFullyQualifiedNullable(data, i, true)
+		props, err := readFullyQualifiedNullable(data, i, true)
 		if err != nil {
 			return nil, err
 		}
+		// null properties are returned as empty slices
+		v.Properties = make([]interface{}, 0)
+		if props != nil {
+			v.Properties = props
+		}
 	}
 	return v, nil
 }
@@ -1101,10 +1106,15 @@ func edgeReader(data *[]byte, i *int) (interface{}, error) {
 	}
 	e.OutV = *v.(*Vertex)
 	*i += 2
-	e.Properties, err = readFullyQualifiedNullable(data, i, true)
+	props, err := readFullyQualifiedNullable(data, i, true)
 	if err != nil {
 		return nil, err
 	}
+	// null properties are returned as empty slices
+	e.Properties = make([]interface{}, 0)
+	if props != nil {
+		e.Properties = props
+	}
 	return e, nil
 }
 
@@ -1149,7 +1159,11 @@ func vertexPropertyReader(data *[]byte, i *int) (interface{}, error) {
 		return nil, err
 	}
 
-	vp.Properties = props
+	// null properties are returned as empty slices
+	vp.Properties = make([]interface{}, 0)
+	if props != nil {
+		vp.Properties = props
+	}
 
 	return vp, nil
 }

gremlin-javascript/src/main/javascript/gremlin-javascript/lib/structure/io/binary/internals/VertexPropertySerializer.js

@@ -164,7 +164,10 @@ module.exports = class VertexPropertySerializer {
       // TODO: should we verify that properties is null?
       cursor = cursor.slice(properties_len);
 
-      const v = new g.VertexProperty(id, label, value, properties);
+      // null properties are deserialized into empty lists
+      const vp_props = properties ? properties : [];
+
+      const v = new g.VertexProperty(id, label, value, vp_props);
       return { v, len };
     } catch (err) {
       throw this.ioc.utils.des_error({ serializer: this, args: arguments, cursor, err });

gremlin-javascript/src/main/javascript/gremlin-javascript/lib/structure/io/binary/internals/VertexSerializer.js

@@ -127,7 +127,10 @@ module.exports = class VertexSerializer {
       }
       cursor = cursor.slice(properties_len);
 
-      const v = new g.Vertex(id, label, properties);
+      // null properties are deserialized into empty lists
+      const vertex_props = properties ? properties : [];
+
+      const v = new g.Vertex(id, label, vertex_props);
       return { v, len };
     } catch (err) {
       throw this.ioc.utils.des_error({ serializer: this, args: arguments, cursor, err });