This is an automated email from the ASF dual-hosted git repository. spmallette pushed a commit to branch TINKERPOP-2235 in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit b5c033e25f9123e5f6d25150ead139e6baebf9e5 Author: stephen <[email protected]> AuthorDate: Tue Nov 26 09:50:41 2019 -0500 TINKERPOP-2235 More consistent null handling for vertex mutations Made addV(null) and property(id, null) work more consistently. --- docs/src/upgrade/release-3.5.x.asciidoc | 41 ++++++++++++++++++++++ .../tinkerpop/gremlin/jsr223/JavaTranslator.java | 14 ++++++-- .../gremlin/structure/util/ElementHelper.java | 3 +- .../gremlin/structure/util/ElementHelperTest.java | 4 +-- gremlin-test/features/map/AddVertex.feature | 9 +++++ .../process/traversal/step/map/AddVertexTest.java | 24 ++++++++++++- 6 files changed, 88 insertions(+), 7 deletions(-) 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 @@ See: link:https://issues.apache.org/jira/browse/TINKERPOP-1568[TINKERPOP-1568], 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 @@ that rely on the old step names. 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 @@ public final class JavaTranslator<S extends TraversalSource, T extends Traversal // 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 @@ public final class ElementHelper { * * @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 @@ public class ElementHelperTest { 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 @@ Feature: Step - addV() | 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.MatcherAssert.assertThat; 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 class AddVertexTest extends AbstractGremlinTest { 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 @@ public abstract class AddVertexTest extends AbstractGremlinTest { 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 @@ public abstract class AddVertexTest extends AbstractGremlinTest { } @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 abstract class AddVertexTest extends AbstractGremlinTest { 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
