TINKERPOP-2235 More consistent null handling for vertex mutations
Made addV(null) and property(id, null) work more consistently.
diff --git a/docs/src/upgrade/release-3.5.x.asciidoc b/docs/src/upgrade/release-3.5.x.asciidoc
index cdd920b..ce7a00e 100644
--- a/docs/src/upgrade/release-3.5.x.asciidoc
+++ b/docs/src/upgrade/release-3.5.x.asciidoc
@@ -358,6 +358,45 @@
link:https://issues.apache.org/jira/browse/TINKERPOP-2310[TINKERPOP-2310],
link:https://issues.apache.org/jira/browse/TINKERPOP-2311[TINKERPOP-2311]
+===== Null Semantics
+
+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:
+
+[source,text]
+----
+gremlin> g.addV(null).property(id, null).property('name',null)
+==>v[0]
+gremlin> g.V().elementMap()
+==>[id:0,label:vertex]
+----
+
+In the above example, `addV()` defaults to `Vertex.DEFAULT_LABEL`, the `id` is generated and setting the "name"
+property to `null` results in the value not being set. If the property value is set to an actual value and then set
+to `null` TinkerGraph will remove the property key all together:
+
+[source,text]
+----
+gremlin> g.V().property('name','stephen')
+==>v[0]
+gremlin> g.V().elementMap()
+==>[id:0,label:vertex,name:stephen]
+gremlin> g.V().property('name',null)
+==>v[0]
+gremlin> g.V().elementMap()
+==>[id:0,label:vertex]
+----
+
+The above examples point out the default operations of TinkerGraph, but it can be configured to actually accept the
+`null` as a property value and it is up to graph providers to decided how they wish to treat a `null` property value.
+Providers should use the new `supportsNullPropertyValues()` feature to indicate to users how `null` is handled.
+
+See: link:https://issues.apache.org/jira/browse/TINKERPOP-2235[TINKERPOP-2235],
+link:https://issues.apache.org/jira/browse/TINKERPOP-2099[TINKERPOP-2099]
+
===== AbstractOpProcessor API Change
The `generateMetaData()` method was removed as it was deprecated in a previous version. There already was a preferred
@@ -373,6 +412,8 @@
See: link:https://issues.apache.org/jira/browse/TINKERPOP-2254[TINKERPOP-2254]
+==== Graph Driver Providers
+
===== TraversalOpProcessor Side-effects
`TraversalOpProcessor` no longer holds a cache of side-effects and more generally the entire side-effect protocol has
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/JavaTranslator.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/JavaTranslator.java
index bb9273c..6034793 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/JavaTranslator.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/JavaTranslator.java
@@ -235,8 +235,18 @@
// is valid hits like four different possible overloads, but we can rely on the most
// generic one which takes Object as the second parameter. that seems to work in this
// case, but it's a shame this isn't nicer. seems like nicer would mean a heavy
- // overhaul to Gremlin or to GLVs/bytecode and/or to serialization mechanisms
- if (i < argumentsCopy.length && ((null == argumentsCopy[i] && parameters[i].getType() == Object.class) ||
+ // overhaul to Gremlin or to GLVs/bytecode and/or to serialization mechanisms.
+ //
+ // the check where argumentsCopy[i] is null could be accompanied by a type check for
+ // allowable signatures like:
+ // null == argumentsCopy[i] && parameters[i].getType() == Object.class
+ // but that doesn't seem helpful. perhaps this approach is fine as long as we ensure
+ // consistency of null calls to all overloads. in other words addV(String) must behave
+ // the same as addV(Traversal) if null is used as the argument. so far, that seems to
+ // be the case. if we find that is not happening we either fix that specific
+ // inconsistency, start special casing those method finds here, or as mentioned above
+ // do something far more drastic that doesn't involve reflection.
+ if (i < argumentsCopy.length && (null == argumentsCopy[i] ||
(argumentsCopy[i] != null && (
parameters[i].getType().isAssignableFrom(argumentsCopy[i].getClass()) ||
(parameters[i].getType().isPrimitive() &&
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/ElementHelper.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/ElementHelper.java
index a121abd..0d6900b 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/ElementHelper.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/ElementHelper.java
@@ -122,12 +122,11 @@
*
* @param keyValues a list of key/value pairs
* @return the value associated with {@link T#id}
- * @throws NullPointerException if the value for the {@link T#id} key is {@code null}
*/
public static Optional<Object> getIdValue(final Object... keyValues) {
for (int i = 0; i < keyValues.length; i = i + 2) {
if (keyValues[i].equals(T.id))
- return Optional.of(keyValues[i + 1]);
+ return Optional.ofNullable(keyValues[i + 1]);
}
return Optional.empty();
}
diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/structure/util/ElementHelperTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/structure/util/ElementHelperTest.java
index d4f1a77..3d7be9c 100644
--- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/structure/util/ElementHelperTest.java
+++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/structure/util/ElementHelperTest.java
@@ -134,9 +134,9 @@
assertFalse(ElementHelper.getIdValue("test", 321, "xyz", 123l, "testagain", "that").isPresent());
}
- @Test(expected = NullPointerException.class)
+ @Test
public void shouldNotFindAnIdValueBecauseItIsNull() {
- ElementHelper.getIdValue("test", 321, T.id, null, "testagain", "that");
+ assertEquals("default", ElementHelper.getIdValue("test", 321, T.id, null, "testagain", "that").orElse("default"));
}
@Test
diff --git a/gremlin-test/features/map/AddVertex.feature b/gremlin-test/features/map/AddVertex.feature
index 0fc338f..e8e1248 100644
--- a/gremlin-test/features/map/AddVertex.feature
+++ b/gremlin-test/features/map/AddVertex.feature
@@ -412,3 +412,12 @@
| result |
| name |
+ Scenario: g_addVXnullX_propertyXid_nullX
+ Given the empty graph
+ And the traversal of
+ """
+ g.addV(null).property(T.id, null)
+ """
+ When iterated to list
+ Then the result should have a count of 1
+ And the graph should return 1 for count of "g.V().hasLabel(\"vertex\")"
diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddVertexTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddVertexTest.java
index 8937a44..fec146d 100644
--- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddVertexTest.java
+++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddVertexTest.java
@@ -44,6 +44,7 @@
import static org.hamcrest.core.Is.is;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
/**
@@ -60,6 +61,8 @@
public abstract Traversal<Vertex, Vertex> get_g_V_hasLabelXpersonX_propertyXname_nullX();
+ public abstract Traversal<Vertex, Vertex> get_g_addVXnullX_propertyXid_nullX();
+
public abstract Traversal<Vertex, Vertex> get_g_addVXpersonX_propertyXsingle_name_stephenX_propertyXsingle_name_stephenmX();
public abstract Traversal<Vertex, Vertex> get_g_addVXpersonX_propertyXsingle_name_stephenX_propertyXsingle_name_stephenm_since_2010X();
@@ -95,7 +98,6 @@
assertEquals(7, IteratorUtils.count(g.V()));
}
-
@Test
@LoadGraphWith(MODERN)
@FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_ADD_VERTICES)
@@ -116,6 +118,20 @@
}
@Test
+ @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_ADD_VERTICES)
+ @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_ADD_PROPERTY)
+ public void g_addVXnullX_propertyXid_nullX() {
+ final Traversal<Vertex, Vertex> traversal = get_g_addVXnullX_propertyXid_nullX();
+ printTraversalForm(traversal);
+
+ final Vertex vertex = traversal.next();
+ assertEquals(Vertex.DEFAULT_LABEL, vertex.label());
+
+ // should generate an id for the null value
+ assertNotNull(vertex.id());
+ }
+
+ @Test
@LoadGraphWith(MODERN)
@FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_ADD_VERTICES)
@FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_ADD_PROPERTY)
@@ -381,5 +397,11 @@
public Traversal<Vertex, Map<Object, Object>> get_g_V_asXaX_hasXname_markoX_outXcreatedX_asXbX_addVXselectXaX_labelX_propertyXtest_selectXbX_labelX_valueMap_withXtokensX() {
return g.V().as("a").has("name", "marko").out("created").as("b").addV(select("a").label()).property("test", select("b").label()).valueMap().with(WithOptions.tokens);
}
+
+ @Override
+ public Traversal<Vertex, Vertex> get_g_addVXnullX_propertyXid_nullX() {
+ // testing Traversal but should work the same for String
+ return g.addV((Traversal.Admin<?, String>) null).property(T.id, null);
+ }
}
}
\ No newline at end of file
