TINKERPOP-2235 Consistent behavior for multi/meta properties and null
diff --git a/docs/src/upgrade/release-3.5.x.asciidoc b/docs/src/upgrade/release-3.5.x.asciidoc
index 841176d..ebe5456 100644
--- a/docs/src/upgrade/release-3.5.x.asciidoc
+++ b/docs/src/upgrade/release-3.5.x.asciidoc
@@ -363,15 +363,24 @@
Graph providers should take note of the changes to `null` semantics described in the "users" section of these upgrade
notes. As `null` is now acceptable as a `Traverser` object, this change may affect custom steps. Further note that
`null` now works more consistently with mutation steps and graph providers may need to include additional logic to
-deal with those possible conditions. Please see the console session below which uses TinkerGraph to demonstrate the
-current behavioral expectations:
+deal with those possible conditions. Please see the console sessions below which uses TinkerGraph to demonstrate the
+current behavioral expectations.
[source,text]
----
+gremlin> g.getGraph().features().vertex().supportsNullPropertyValues()
+==>false
gremlin> g.addV(null).property(id, null).property('name',null)
==>v[0]
gremlin> g.V().elementMap()
==>[id:0,label:vertex]
+...
+gremlin> g.getGraph().features().vertex().supportsNullPropertyValues()
+==>true
+gremlin> g.addV(null).property(id, null).property('name',null)
+==>v[0]
+gremlin> g.V().elementMap()
+==>[id:0,label:vertex,name:null]
----
In the above example, `addV()` defaults to `Vertex.DEFAULT_LABEL`, the `id` is generated and setting the "name"
@@ -380,14 +389,25 @@
[source,text]
----
-gremlin> g.V().property('name','stephen')
+gremlin> g.getGraph().features().vertex().supportsNullPropertyValues()
+==>false
+gremlin> g.addV().property('name','stephen')
==>v[0]
gremlin> g.V().elementMap()
==>[id:0,label:vertex,name:stephen]
-gremlin> g.V().property('name',null)
+gremlin> g.V().has('vertex','name','stephen').property('name',null)
==>v[0]
gremlin> g.V().elementMap()
==>[id:0,label:vertex]
+...
+gremlin> g.getGraph().features().vertex().supportsNullPropertyValues()
+==>true
+gremlin> g.addV().property('name','stephen')
+==>v[2]
+gremlin> g.V().has('vertex','name','stephen').property('name',null)
+==>v[2]
+gremlin> g.V().elementMap()
+==>[id:2,label:vertex,name:null]
----
The above examples point out the default operations of TinkerGraph, but it can be configured to actually accept the
@@ -413,20 +433,85 @@
[source,text]
----
-gremlin> g.V(0L).as('a').addE('knows').to('a').property(id,null).property('weight',null)
-==>e[1][0-knows->0]
+gremlin> g.getGraph().features().vertex().supportsNullPropertyValues()
+==>false
+gremlin> g.addV().property('name','stephen')
+==>v[0]
+gremlin> g.V().has('vertex','name','stephen').as('a').addE('knows').to('a').property(id,null).property('weight',null)
+==>e[2][0-knows->0]
gremlin> g.E().elementMap()
-==>[id:1,label:knows,IN:[id:0,label:vertex],OUT:[id:0,label:vertex]]
-gremlin> g.V(0L).as('a').addE('knows').to('a').property('weight',0.5)
-==>e[3][0-knows->0]
+==>[id:2,label:knows,IN:[id:0,label:vertex],OUT:[id:0,label:vertex]]
+gremlin> g.E().property('weight',0.5)
+==>e[2][0-knows->0]
gremlin> g.E().elementMap()
-==>[id:3,label:knows,IN:[id:0,label:vertex],OUT:[id:0,label:vertex],weight:0.5]
+==>[id:2,label:knows,IN:[id:0,label:vertex],OUT:[id:0,label:vertex],weight:0.5]
gremlin> g.E().property('weight',null)
-==>e[3][0-knows->0]
+==>e[2][0-knows->0]
gremlin> g.E().elementMap()
-==>[id:3,label:knows,IN:[id:0,label:vertex],OUT:[id:0,label:vertex]]
+==>[id:2,label:knows,IN:[id:0,label:vertex],OUT:[id:0,label:vertex]]
+...
+gremlin> g.getGraph().features().vertex().supportsNullPropertyValues()
+==>true
+gremlin> g.addV().property('name','stephen')
+==>v[8]
+gremlin> g.V().has('vertex','name','stephen').as('a').addE('knows').to('a').property(id,null).property('weight',null)
+==>e[10][8-knows->8]
+gremlin> g.E().elementMap()
+==>[id:10,label:knows,IN:[id:8,label:vertex],OUT:[id:8,label:vertex],weight:null]
+gremlin> g.E().property('weight',0.5)
+==>e[10][8-knows->8]
+gremlin> g.E().elementMap()
+==>[id:10,label:knows,IN:[id:8,label:vertex],OUT:[id:8,label:vertex],weight:0.5]
+gremlin> g.E().property('weight',null)
+==>e[10][8-knows->8]
+gremlin> g.E().elementMap()
+==>[id:10,label:knows,IN:[id:8,label:vertex],OUT:[id:8,label:vertex],weight:null]
----
+Graphs that support multi/meta-properties have some issues to consider as well as demonstrated with TinkerGraph:
+
+[source,text]
+----
+gremlin> g.getGraph().features().vertex().supportsNullPropertyValues()
+==>false
+gremlin> g.addV().property(list,'foo',"x").property(list,"foo", null).property(list,'foo','bar')
+==>v[0]
+gremlin> g.V().elementMap()
+==>[id:0,label:vertex,foo:bar]
+gremlin> g.V().valueMap()
+==>[foo:[x,bar]]
+gremlin> g.V().property('foo',null)
+==>v[0]
+gremlin> g.V().valueMap(true)
+==>[id:0,label:vertex]
+...
+gremlin> g.addV().property(list,'foo','bar','x',1,'y',null)
+==>v[0]
+gremlin> g.V().properties('foo').valueMap(true)
+==>[id:1,key:foo,value:bar,x:1]
+gremlin> g.V().properties('foo').property('x',null)
+==>vp[foo->bar]
+gremlin> g.V().properties('foo').valueMap(true)
+==>[id:1,key:foo,value:bar]
+...
+gremlin> g.getGraph().features().vertex().supportsNullPropertyValues()
+==>false
+gremlin> g.addV().property(list,'foo',"x").property(list,"foo", null).property(list,'foo','bar')
+==>v[11]
+gremlin> g.V().elementMap()
+==>[id:11,label:vertex,foo:bar]
+gremlin> g.V().valueMap()
+==>[foo:[x,null,bar]]
+...
+gremlin> g.addV().property(list,'foo','bar','x',1,'y',null)
+==>v[0]
+gremlin> g.V().properties('foo').valueMap(true)
+==>[id:1,key:foo,value:bar,x:1,y:null]
+gremlin> g.V().properties('foo').property('x',null)
+==>vp[foo->bar]
+gremlin> g.V().properties('foo').valueMap(true)
+==>[id:1,key:foo,value:bar,x:null,y:null]
+----
See: link:https://issues.apache.org/jira/browse/TINKERPOP-2235[TINKERPOP-2235],
link:https://issues.apache.org/jira/browse/TINKERPOP-2099[TINKERPOP-2099]
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java
index a8f352a..ef20e52 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java
@@ -2191,8 +2191,7 @@
* {@link VertexProperty.Cardinality} is defaulted to {@code null} which means that the default cardinality for
* the {@link Graph} will be used.
* <p/>
- * This method is effectively calls
- * {@link #property(org.apache.tinkerpop.gremlin.structure.VertexProperty.Cardinality, Object, Object, Object...)}
+ * This method is effectively calls {@link #property(VertexProperty.Cardinality, Object, Object, Object...)}
* as {@code property(null, key, value, keyValues}.
*
* @param key the key for the property
@@ -2204,8 +2203,8 @@
*/
public default GraphTraversal<S, E> property(final Object key, final Object value, final Object... keyValues) {
return key instanceof VertexProperty.Cardinality ?
- this.property((VertexProperty.Cardinality) key, value, keyValues[0],
- keyValues.length > 1 ?
+ this.property((VertexProperty.Cardinality) key, value, null == keyValues ? null : keyValues[0],
+ keyValues != null && keyValues.length > 1 ?
Arrays.copyOfRange(keyValues, 1, keyValues.length) :
new Object[]{}) :
this.property(null, key, value, keyValues);
diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/VertexPropertyTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/VertexPropertyTest.java
index 04b431d..c8b3c66 100644
--- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/VertexPropertyTest.java
+++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/VertexPropertyTest.java
@@ -111,7 +111,8 @@
@FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_META_PROPERTIES)
@FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_MULTI_PROPERTIES)
@FeatureRequirement(featureClass = Graph.Features.VertexPropertyFeatures.class, feature = Graph.Features.VertexPropertyFeatures.FEATURE_INTEGER_VALUES)
- public void shouldHandleListVertexProperties() {
+ @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_NULL_PROPERTY_VALUES, supported = false)
+ public void shouldHandleListVertexPropertiesWithoutNullPropertyValues() {
final Vertex v = graph.addVertex("name", "marko", "age", 34);
tryCommit(graph, g -> {
assertEquals("marko", v.property("name").value());
@@ -169,6 +170,95 @@
assertVertexEdgeCounts(graph, 1, 0);
});
+
+ // null property values are not supported so the value should not be added as set or list cardinality,
+ // but single will remove it
+ assertEquals(VertexProperty.empty(), v.property(VertexProperty.Cardinality.list, "name", null));
+ tryCommit(graph, g -> {
+ assertEquals(3, IteratorUtils.count(v.properties("name")));
+ assertEquals(4, IteratorUtils.count(v.properties()));
+ assertVertexEdgeCounts(graph, 1, 0);
+ });
+ assertEquals(VertexProperty.empty(), v.property(VertexProperty.Cardinality.single, "name", null));
+ tryCommit(graph, g -> {
+ assertEquals(0, IteratorUtils.count(v.properties("name")));
+ assertEquals(1, IteratorUtils.count(v.properties()));
+ assertVertexEdgeCounts(graph, 1, 0);
+ });
+ }
+
+ @Test
+ @FeatureRequirementSet(FeatureRequirementSet.Package.VERTICES_ONLY)
+ @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_META_PROPERTIES)
+ @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_MULTI_PROPERTIES)
+ @FeatureRequirement(featureClass = Graph.Features.VertexPropertyFeatures.class, feature = Graph.Features.VertexPropertyFeatures.FEATURE_INTEGER_VALUES)
+ @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_NULL_PROPERTY_VALUES)
+ public void shouldHandleListVertexPropertiesWithNullPropertyValues() {
+ final Vertex v = graph.addVertex("name", "marko", "age", 34);
+ tryCommit(graph, g -> {
+ assertEquals("marko", v.property("name").value());
+ assertEquals("marko", v.value("name"));
+ assertEquals(34, v.property("age").value());
+ assertEquals(34, v.<Integer>value("age").intValue());
+ assertEquals(1, IteratorUtils.count(v.properties("name")));
+ assertEquals(2, IteratorUtils.count(v.properties()));
+ assertVertexEdgeCounts(graph, 1, 0);
+ });
+
+ final VertexProperty<String> property = v.property(VertexProperty.Cardinality.list, "name", "marko a. rodriguez");
+ tryCommit(graph, g -> assertEquals(v, property.element()));
+
+ try {
+ v.property("name");
+ fail("This should throw a: " + Vertex.Exceptions.multiplePropertiesExistForProvidedKey("name"));
+ } catch (final Exception e) {
+ validateException(Vertex.Exceptions.multiplePropertiesExistForProvidedKey("name"), e);
+ }
+
+ assertTrue(IteratorUtils.list(v.values("name")).contains("marko"));
+ assertTrue(IteratorUtils.list(v.values("name")).contains("marko a. rodriguez"));
+ assertEquals(3, IteratorUtils.count(v.properties()));
+ assertEquals(2, IteratorUtils.count(v.properties("name")));
+ assertVertexEdgeCounts(graph, 1, 0);
+
+ assertEquals(v, v.property(VertexProperty.Cardinality.list, "name", "mrodriguez").element());
+ tryCommit(graph, g -> {
+ assertEquals(3, IteratorUtils.count(v.properties("name")));
+ assertEquals(4, IteratorUtils.count(v.properties()));
+ assertVertexEdgeCounts(graph, 1, 0);
+ });
+
+ v.<String>properties("name").forEachRemaining(meta -> {
+ meta.property("counter", meta.value().length());
+ });
+
+ tryCommit(graph, g -> {
+ v.properties().forEachRemaining(meta -> {
+ assertEquals(meta.key(), meta.label());
+ assertTrue(meta.isPresent());
+ assertEquals(v, meta.element());
+ if (meta.key().equals("age")) {
+ assertEquals(meta.value(), 34);
+ assertEquals(0, IteratorUtils.count(meta.properties()));
+ }
+ if (meta.key().equals("name")) {
+ assertEquals(((String) meta.value()).length(), meta.<Integer>value("counter").intValue());
+ assertEquals(1, IteratorUtils.count(meta.properties()));
+ assertEquals(1, meta.keys().size());
+ assertTrue(meta.keys().contains("counter"));
+ }
+ });
+
+ assertVertexEdgeCounts(graph, 1, 0);
+ });
+
+ // null property values are supported so the value should be added
+ assertEquals(v, v.property(VertexProperty.Cardinality.list, "name", null).element());
+ tryCommit(graph, g -> {
+ assertEquals(4, IteratorUtils.count(v.properties("name")));
+ assertEquals(5, IteratorUtils.count(v.properties()));
+ assertVertexEdgeCounts(graph, 1, 0);
+ });
}
@Test
diff --git a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerVertex.java b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerVertex.java
index 52518d6..1f055fb 100644
--- a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerVertex.java
+++ b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerVertex.java
@@ -89,8 +89,12 @@
ElementHelper.validateProperty(key, value);
final Optional<Object> optionalId = ElementHelper.getIdValue(keyValues);
+ // if we don't allow null property values and the value is null then the key can be removed but only if the
+ // cardinality is single. if it is list/set then we can just ignore the null.
if (!allowNullPropertyValues && null == value) {
- properties(key).forEachRemaining(VertexProperty::remove);
+ final VertexProperty.Cardinality card = null == cardinality ? graph.features().vertex().getCardinality(key) : cardinality;
+ if (VertexProperty.Cardinality.single == card)
+ properties(key).forEachRemaining(VertexProperty::remove);
return VertexProperty.empty();
}
diff --git a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerVertexProperty.java b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerVertexProperty.java
index ed8acec..f2635df 100644
--- a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerVertexProperty.java
+++ b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerVertexProperty.java
@@ -143,7 +143,8 @@
}
final AtomicBoolean delete = new AtomicBoolean(true);
this.vertex.properties(this.key).forEachRemaining(property -> {
- if (property.value().equals(this.value))
+ final Object currentPropertyValue = property.value();
+ if ((currentPropertyValue != null && currentPropertyValue.equals(this.value) || null == currentPropertyValue && null == this.value))
delete.set(false);
});
if (delete.get()) TinkerHelper.removeIndex(this.vertex, this.key, this.value);
