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 d2be888b6706e4b489553952adb4be01e86fd8e4 Author: stephen <[email protected]> AuthorDate: Tue Nov 26 13:00:06 2019 -0500 TINKERPOP-2235 Consistent behavior for multi/meta properties and null --- docs/src/upgrade/release-3.5.x.asciidoc | 109 ++++++++++++++++++--- .../traversal/dsl/graph/GraphTraversal.java | 7 +- .../gremlin/structure/VertexPropertyTest.java | 92 ++++++++++++++++- .../tinkergraph/structure/TinkerVertex.java | 6 +- .../structure/TinkerVertexProperty.java | 3 +- 5 files changed, 198 insertions(+), 19 deletions(-) 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 @@ link:https://issues.apache.org/jira/browse/TINKERPOP-2311[TINKERPOP-2311] 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 @@ to `null` TinkerGraph will remove the property key all together: [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 @@ TinkerGraph is being used here): [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 @@ public interface GraphTraversal<S, E> extends Traversal<S, E> { * {@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 interface GraphTraversal<S, E> extends Traversal<S, E> { */ 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 @@ public class VertexPropertyTest extends AbstractGremlinTest { @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 @@ public class VertexPropertyTest extends AbstractGremlinTest { 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 @@ public final class TinkerVertex extends TinkerElement implements Vertex { 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 @@ public class TinkerVertexProperty<V> extends TinkerElement implements VertexProp } 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);
