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 f80990b0cc99df428a16743e704199c019c716dc Author: stephen <[email protected]> AuthorDate: Thu Nov 14 09:58:33 2019 -0500 TINKERPOP-2235 Adjusted semantics of null a bit for Graph If the graph supports null as a value then it will store it as such. If it does not then setting a property to null is the same as doing a property drop(). --- CHANGELOG.asciidoc | 1 + docs/src/upgrade/release-3.5.x.asciidoc | 48 +++++++-------- .../gremlin/process/computer/VertexComputeKey.java | 2 +- .../apache/tinkerpop/gremlin/structure/Graph.java | 4 +- .../tinkerpop/gremlin/structure/Property.java | 4 -- .../gremlin/structure/util/ElementHelper.java | 60 +++++++++++------- .../gremlin/structure/util/star/StarGraph.java | 16 ++--- .../gremlin/structure/util/ElementHelperTest.java | 71 ++++++++++++++-------- gremlin-test/features/map/AddEdge.feature | 11 ++-- gremlin-test/features/map/AddVertex.feature | 8 +-- .../process/traversal/CoreTraversalTest.java | 39 ++++++++++++ .../process/traversal/step/map/AddEdgeTest.java | 30 +++------ .../process/traversal/step/map/AddVertexTest.java | 24 +++----- .../tinkerpop/gremlin/structure/PropertyTest.java | 33 +++------- .../gremlin/neo4j/structure/Neo4jEdge.java | 8 ++- .../gremlin/neo4j/structure/Neo4jGraph.java | 2 +- .../gremlin/neo4j/structure/Neo4jVertex.java | 10 ++- .../process/computer/TinkerGraphComputerView.java | 2 +- .../gremlin/tinkergraph/structure/TinkerEdge.java | 8 ++- .../gremlin/tinkergraph/structure/TinkerGraph.java | 4 +- .../tinkergraph/structure/TinkerHelper.java | 2 +- .../tinkergraph/structure/TinkerVertex.java | 10 ++- .../structure/TinkerVertexProperty.java | 20 +++--- .../tinkergraph/structure/TinkerGraphTest.java | 1 - 24 files changed, 238 insertions(+), 180 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 96d3d4f..58dc701 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -41,6 +41,7 @@ This release also includes changes from <<release-3-4-3, 3.4.3>>. * Added a parameterized `TypeTranslator` for use with `GroovyTranslator` that should produce more cache hits. * Added support for `TextP` in Neo4j using its string search functions. * Changed `TraversalStrategy` application methodology to apply each strategy in turn to each level of the traversal hierarchy starting from root down to children. +* Removed `Property.Exceptions.propertyValueCanNotBeNull` exception type as `null` now has meaning in Gremlin. * Removed the "experimental" support for multi/meta-properties in Neo4j. * Removed Gryo serialization configurations from Gremlin Server sample configurations and default configurations. * Removed previously deprecated `TraversalStrategies.applyStrategies()`. diff --git a/docs/src/upgrade/release-3.5.x.asciidoc b/docs/src/upgrade/release-3.5.x.asciidoc index fcd66de..f0739e5 100644 --- a/docs/src/upgrade/release-3.5.x.asciidoc +++ b/docs/src/upgrade/release-3.5.x.asciidoc @@ -105,12 +105,6 @@ gremlin> g.inject(1, null, null, 2, null) ==>null ==>null ==>2 -gremlin> g.addV("person").property("name", 'stephen').property("age", null) -==>v[13] -gremlin> g.V().has('person','name','stephen').elementMap() -==>[id:13,label:person,name:stephen,age:null] -gremlin> g.V().has('person','age',null) -==>v[13] gremlin> g.V().constant(null) ==>null ==>null @@ -120,13 +114,12 @@ gremlin> g.V().constant(null) ==>null ---- -Note that the above examples use TinkerGraph which now supports `null` as a property value (though it can be configured -to work in the prior fashion) and all graphs may not support this feature (for example, Neo4j does not). Please be -sure to check the new `supportsNullPropertyValues()` feature (or their documentation) to determine if the `Graph` -implementation allows `null` in this same fashion. +TinkerGraph can be configured to support `null` as a property value and all graphs may not support this feature (for +example, Neo4j does not). Please be sure to check the new `supportsNullPropertyValues()` feature (or the documentation +of the graph provider) to determine if the `Graph` implementation allows `null` as a property value. -As a final consideration, there was a bit of inconsistency in the handling of `null` in calls to `property()` -depending on the type of mutation being executed demonstrated as follows in earlier versions: +With respect to `null` in relation to properties, there was a bit of inconsistency in the handling of `null` in calls +to `property()` depending on the type of mutation being executed demonstrated as follows in earlier versions: [source,text] ---- @@ -141,29 +134,34 @@ gremlin> g.V(13).properties() ==>vp[z->2] ---- -In this release, this behavior has been altered to become consistent. First, assuming `null` is not supported as a -property value: +This behavior has been altered to become consistent. First, assuming `null` is not supported as a property value, the +setting of a property to `null` should have the behavior of removing the property in the same way in which you might +do `g.V().properties().drop()`: [source,text] ---- gremlin> g.V(1).property("x", 1).property("y", null).property("z", 2) -Property value can not be null -Type ':help' or ':h' for help. -Display stack trace? [yN]n -gremlin> g.addV().property("x", 1).property("y", null).property("z", 2) -Property value can not be null -Type ':help' or ':h' for help. -Display stack trace? [yN] +==>v[1] +gremlin> g.V(1).elementMap() +==>[id:1,label:person,name:marko,x:1,z:2,age:29] +gremlin> g.V().hasLabel('person').property('age',null).iterate() +gremlin> g.V().hasLabel('person').elementMap() +==>[id:1,label:person,name:marko] +==>[id:2,label:person,name:vadas] +==>[id:4,label:person,name:josh] +==>[id:6,label:person,name:peter] ---- Then, assuming `null` is supported as a property value: [source,text] ---- -gremlin> g.V(1).property("x", 1).property("y", null).property("z", 2) -==>v[1] -gremlin> g.addV().property("x", 1).property("y", null).property("z", 2) -==>v[15] +gremlin> g.addV("person").property("name", 'stephen').property("age", null) +==>v[13] +gremlin> g.V().has('person','name','stephen').elementMap() +==>[id:13,label:person,name:stephen,age:null] +gremlin> g.V().has('person','age',null) +==>v[13] ---- In conclusion, this change in greater support of `null` may affect the behavior of existing traversals written in past diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/VertexComputeKey.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/VertexComputeKey.java index 072ad59..284430a 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/VertexComputeKey.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/VertexComputeKey.java @@ -37,7 +37,7 @@ public final class VertexComputeKey implements Serializable { private VertexComputeKey(final String key, final boolean isTransient) { this.key = key; this.isTransient = isTransient; - ElementHelper.validateProperty(true, key, key); + ElementHelper.validateProperty(key, key); } public String getKey() { diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/Graph.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/Graph.java index 6609dab..c35ec2e 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/Graph.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/Graph.java @@ -685,7 +685,9 @@ public interface Graph extends AutoCloseable, Host { public static final String FEATURE_NULL_PROPERTY_VALUES = "NullPropertyValues"; /** - * Determines if an {@link Element} allows properties with {@code null} property values. + * Determines if an {@link Element} allows properties with {@code null} property values. In the event that + * this value is {@code false}, the underlying graph must treat {@code null} as an indication to remove + * the property. */ @FeatureDescriptor(name = FEATURE_NULL_PROPERTY_VALUES) public default boolean supportsNullPropertyValues() { diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/Property.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/Property.java index 5b67f07..eac8355 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/Property.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/Property.java @@ -139,10 +139,6 @@ public interface Property<V> { return new IllegalArgumentException("Property key can not be null"); } - public static IllegalArgumentException propertyValueCanNotBeNull() { - return new IllegalArgumentException("Property value can not be null"); - } - public static IllegalArgumentException propertyKeyCanNotBeAHiddenKey(final String key) { return new IllegalArgumentException("Property key can not be a hidden key: " + key); } 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 b117745..a121abd 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 @@ -87,15 +87,12 @@ public final class ElementHelper { * Determines whether the property key/value for the specified thing can be legally set. This is typically used as * a pre-condition check prior to setting a property. * - * @param allowNullValue true if {@code null} is allowed as a value to the property * @param key the key of the property * @param value the value of the property\ * @throws IllegalArgumentException whether the key/value pair is legal and if not, a clear reason exception * message is provided */ - public static void validateProperty(final boolean allowNullValue, final String key, final Object value) throws IllegalArgumentException { - if (!allowNullValue && null == value) - throw Property.Exceptions.propertyValueCanNotBeNull(); + public static void validateProperty(final String key, final Object value) throws IllegalArgumentException { if (null == key) throw Property.Exceptions.propertyKeyCanNotBeNull(); if (key.isEmpty()) @@ -106,22 +103,17 @@ public final class ElementHelper { /** * Determines whether a list of key/values are legal, ensuring that there are an even number of values submitted - * and that the key values in the list of arguments are {@link String} or {@link Element} objects. + * and that the keys in the list of arguments are {@link String} or {@link T} objects. * - * @param allowNullValues true if {@code null} is allowed as a value to the property * @param propertyKeyValues a list of key/value pairs * @throws IllegalArgumentException if something in the pairs is illegal */ - public static void legalPropertyKeyValueArray(final boolean allowNullValues, final Object... propertyKeyValues) throws IllegalArgumentException { + public static void legalPropertyKeyValueArray(final Object... propertyKeyValues) throws IllegalArgumentException { if (propertyKeyValues.length % 2 != 0) throw Element.Exceptions.providedKeyValuesMustBeAMultipleOfTwo(); for (int i = 0; i < propertyKeyValues.length; i = i + 2) { if (!(propertyKeyValues[i] instanceof String) && !(propertyKeyValues[i] instanceof T)) throw Element.Exceptions.providedKeyValuesMustHaveALegalKeyOnEvenIndices(); - - if (!allowNullValues && null == propertyKeyValues[i + 1]) { - throw Property.Exceptions.propertyValueCanNotBeNull(); - } } } @@ -282,16 +274,24 @@ public final class ElementHelper { if (null == element) throw Graph.Exceptions.argumentCanNotBeNull("element"); + final boolean allowNullPropertyValues = element instanceof Vertex ? + element.graph().features().vertex().supportsNullPropertyValues() : element instanceof Edge ? + element.graph().features().edge().supportsNullPropertyValues() : + element.graph().features().vertex().properties().supportsNullPropertyValues(); + for (int i = 0; i < propertyKeyValues.length; i = i + 2) { if (!propertyKeyValues[i].equals(T.id) && !propertyKeyValues[i].equals(T.label)) - element.property((String) propertyKeyValues[i], propertyKeyValues[i + 1]); + if (!allowNullPropertyValues && null == propertyKeyValues[i + 1]) + element.properties(((String) propertyKeyValues[i])).forEachRemaining(Property::remove); + else + element.property((String) propertyKeyValues[i], propertyKeyValues[i + 1]); } } /** - * Assign key/value pairs as properties to an {@link Vertex}. If the value of {@link T#id} or - * {@link T#label} is in the set of pairs, then they are ignored. - * The {@link VertexProperty.Cardinality} of the key is determined from the {@link Graph.Features.VertexFeatures}. + * Assign key/value pairs as properties to an {@link Vertex}. If the value of {@link T#id} or {@link T#label} is + * in the set of pairs, then they are ignored. The {@link VertexProperty.Cardinality} of the key is determined from + * the {@link Graph.Features.VertexFeatures}. * * @param vertex the graph vertex to assign the {@code propertyKeyValues} * @param propertyKeyValues the key/value pairs to assign to the {@code element} @@ -302,15 +302,20 @@ public final class ElementHelper { if (null == vertex) throw Graph.Exceptions.argumentCanNotBeNull("vertex"); + final boolean allowNullPropertyValues = vertex.graph().features().vertex().supportsNullPropertyValues(); + for (int i = 0; i < propertyKeyValues.length; i = i + 2) { if (!propertyKeyValues[i].equals(T.id) && !propertyKeyValues[i].equals(T.label)) - vertex.property(vertex.graph().features().vertex().getCardinality((String) propertyKeyValues[i]), (String) propertyKeyValues[i], propertyKeyValues[i + 1]); + if (!allowNullPropertyValues && null == propertyKeyValues[i + 1]) + vertex.properties(((String) propertyKeyValues[i])).forEachRemaining(VertexProperty::remove); + else + vertex.property(vertex.graph().features().vertex().getCardinality((String) propertyKeyValues[i]), (String) propertyKeyValues[i], propertyKeyValues[i + 1]); } } /** - * Assign key/value pairs as properties to a {@link Vertex}. - * If the value of {@link T#id} or {@link T#label} is in the set of pairs, then they are ignored. + * Assign key/value pairs as properties to a {@link Vertex}. If the value of {@link T#id} or {@link T#label} is + * in the set of pairs, then they are ignored. * * @param vertex the vertex to attach the properties to * @param cardinality the cardinality of the key value pair settings @@ -318,18 +323,24 @@ public final class ElementHelper { * @throws ClassCastException if the value of the key is not a {@link String} * @throws IllegalArgumentException if the value of {@code element} is null */ - public static void attachProperties(final Vertex vertex, final VertexProperty.Cardinality cardinality, final Object... propertyKeyValues) { + public static void attachProperties(final Vertex vertex, final VertexProperty.Cardinality cardinality, + final Object... propertyKeyValues) { if (null == vertex) throw Graph.Exceptions.argumentCanNotBeNull("vertex"); + final boolean allowNullPropertyValues = vertex.graph().features().vertex().supportsNullPropertyValues(); + for (int i = 0; i < propertyKeyValues.length; i = i + 2) { if (!propertyKeyValues[i].equals(T.id) && !propertyKeyValues[i].equals(T.label)) - vertex.property(cardinality, (String) propertyKeyValues[i], propertyKeyValues[i + 1]); + if (!allowNullPropertyValues && null == propertyKeyValues[i + 1]) + vertex.properties(((String) propertyKeyValues[i])).forEachRemaining(VertexProperty::remove); + else + vertex.property(cardinality, (String) propertyKeyValues[i], propertyKeyValues[i + 1]); } } /** - * This is a helper method for dealing with vertex property cardinality and typically used in {@link Vertex#property(org.apache.tinkerpop.gremlin.structure.VertexProperty.Cardinality, String, Object, Object...)}. + * This is a helper method for dealing with vertex property cardinality and typically used in {@link Vertex#property(VertexProperty.Cardinality, String, Object, Object...)}. * If the cardinality is list, simply return {@link Optional#empty}. * If the cardinality is single, delete all existing properties of the provided key and return {@link Optional#empty}. * If the cardinality is set, find one that has the same key/value and attached the properties to it and return it. Else, if no equal value is found, return {@link Optional#empty}. @@ -342,7 +353,9 @@ public final class ElementHelper { * @param <V> the type of the vertex property value * @return a vertex property if it has been found in set with equal value */ - public static <V> Optional<VertexProperty<V>> stageVertexProperty(final Vertex vertex, final VertexProperty.Cardinality cardinality, final String key, final V value, final Object... keyValues) { + public static <V> Optional<VertexProperty<V>> stageVertexProperty(final Vertex vertex, + final VertexProperty.Cardinality cardinality, + final String key, final V value, final Object... keyValues) { if (cardinality.equals(VertexProperty.Cardinality.single)) vertex.properties(key).forEachRemaining(VertexProperty::remove); else if (cardinality.equals(VertexProperty.Cardinality.set)) { @@ -389,7 +402,8 @@ public final class ElementHelper { /** * A standard method for determining if two {@link Element} objects are equal. This method should be used by any - * {@link Object#equals(Object)} implementation to ensure consistent behavior. This method is used for Vertex, Edge, and VertexProperty. + * {@link Object#equals(Object)} implementation to ensure consistent behavior. This method is used for Vertex, + * Edge, and VertexProperty. * * @param a The first {@link Element} * @param b The second {@link Element} (as an {@link Object}) diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/star/StarGraph.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/star/StarGraph.java index 935ebef..3bcdb7d 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/star/StarGraph.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/star/StarGraph.java @@ -95,7 +95,7 @@ public final class StarGraph implements Graph, Serializable { @Override public Vertex addVertex(final Object... keyValues) { if (null == this.starVertex) { - ElementHelper.legalPropertyKeyValueArray(true, keyValues); + ElementHelper.legalPropertyKeyValueArray(keyValues); this.starVertex = new StarVertex(ElementHelper.getIdValue(keyValues).orElse(this.nextId()), ElementHelper.getLabelValue(keyValues).orElse(Vertex.DEFAULT_LABEL)); ElementHelper.attachProperties(this.starVertex, VertexProperty.Cardinality.list, keyValues); // TODO: is this smart? I say no... cause vertex property ids are not preserved. return this.starVertex; @@ -413,14 +413,14 @@ public final class StarGraph implements Graph, Serializable { @Override public <V> VertexProperty<V> property(final String key, final V value, final Object... keyValues) { - ElementHelper.validateProperty(true, key, value); - ElementHelper.legalPropertyKeyValueArray(true, keyValues); + ElementHelper.validateProperty(key, value); + ElementHelper.legalPropertyKeyValueArray(keyValues); return this.property(VertexProperty.Cardinality.single, key, value, keyValues); } Edge addOutEdge(final String label, final Vertex inVertex, final Object... keyValues) { ElementHelper.validateLabel(label); - ElementHelper.legalPropertyKeyValueArray(true, keyValues); + ElementHelper.legalPropertyKeyValueArray(keyValues); if (null == this.outEdges) this.outEdges = new HashMap<>(); List<Edge> outE = this.outEdges.get(label); @@ -436,7 +436,7 @@ public final class StarGraph implements Graph, Serializable { Edge addInEdge(final String label, final Vertex outVertex, final Object... keyValues) { ElementHelper.validateLabel(label); - ElementHelper.legalPropertyKeyValueArray(true, keyValues); + ElementHelper.legalPropertyKeyValueArray(keyValues); if (null == this.inEdges) this.inEdges = new HashMap<>(); List<Edge> inE = this.inEdges.get(label); @@ -452,7 +452,7 @@ public final class StarGraph implements Graph, Serializable { @Override public <V> VertexProperty<V> property(final VertexProperty.Cardinality cardinality, final String key, V value, final Object... keyValues) { - ElementHelper.legalPropertyKeyValueArray(true, keyValues); + ElementHelper.legalPropertyKeyValueArray(keyValues); if (null == this.vertexProperties) this.vertexProperties = new HashMap<>(); final List<VertexProperty> list = cardinality.equals(VertexProperty.Cardinality.single) ? new ArrayList<>(1) : this.vertexProperties.getOrDefault(key, new ArrayList<>()); @@ -645,7 +645,7 @@ public final class StarGraph implements Graph, Serializable { @Override public <U> Property<U> property(final String key, final U value) { - ElementHelper.validateProperty(true, key, value); + ElementHelper.validateProperty(key, value); if (null == metaProperties) metaProperties = new HashMap<>(); Map<String, Object> properties = metaProperties.get(this.id); @@ -760,7 +760,7 @@ public final class StarGraph implements Graph, Serializable { @Override public <V> Property<V> property(final String key, final V value) { - ElementHelper.validateProperty(true, key, value); + ElementHelper.validateProperty(key, value); if (null == edgeProperties) edgeProperties = new HashMap<>(); Map<String, Object> properties = edgeProperties.get(this.id); 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 b72e6cd..d4f1a77 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 @@ -54,24 +54,14 @@ public class ElementHelperTest { } @Test - public void shouldValidatePropertyAndNotAllowNullValue() { - try { - ElementHelper.validateProperty(false,"test", null); - fail("Should fail as property value cannot be null"); - } catch (IllegalArgumentException iae) { - assertEquals(Property.Exceptions.propertyValueCanNotBeNull().getMessage(), iae.getMessage()); - } - } - - @Test public void shouldValidatePropertyAndAllowNullValue() { - ElementHelper.validateProperty(true,"test", null); + ElementHelper.validateProperty("test", null); } @Test public void shouldValidatePropertyAndNotAllowNullKey() { try { - ElementHelper.validateProperty(false,null, "test"); + ElementHelper.validateProperty(null, "test"); fail("Should fail as property key cannot be null"); } catch (IllegalArgumentException iae) { assertEquals(Property.Exceptions.propertyKeyCanNotBeNull().getMessage(), iae.getMessage()); @@ -81,7 +71,7 @@ public class ElementHelperTest { @Test public void shouldValidatePropertyAndNotAllowEmptyKey() { try { - ElementHelper.validateProperty(false,"", "test"); + ElementHelper.validateProperty("", "test"); fail("Should fail as property key cannot be empty"); } catch (IllegalArgumentException iae) { assertEquals(Property.Exceptions.propertyKeyCanNotBeEmpty().getMessage(), iae.getMessage()); @@ -92,7 +82,7 @@ public class ElementHelperTest { public void shouldValidatePropertyAndNotAllowHiddenKey() { final String key = Graph.Hidden.hide("key"); try { - ElementHelper.validateProperty(false, key, "test"); + ElementHelper.validateProperty(key, "test"); fail("Should fail as property key cannot be hidden"); } catch (IllegalArgumentException iae) { assertEquals(Property.Exceptions.propertyKeyCanNotBeAHiddenKey(key).getMessage(), iae.getMessage()); @@ -101,13 +91,13 @@ public class ElementHelperTest { @Test public void shouldHaveValidProperty() { - ElementHelper.validateProperty(false,"aKey", "value"); + ElementHelper.validateProperty("aKey", "value"); } @Test public void shouldAllowEvenNumberOfKeyValues() { try { - ElementHelper.legalPropertyKeyValueArray(false,"aKey", "test", "no-value-for-this-one"); + ElementHelper.legalPropertyKeyValueArray("aKey", "test", "no-value-for-this-one"); fail("Should fail as there is an odd number of key-values"); } catch (IllegalArgumentException iae) { assertEquals(Element.Exceptions.providedKeyValuesMustBeAMultipleOfTwo().getMessage(), iae.getMessage()); @@ -117,7 +107,7 @@ public class ElementHelperTest { @Test public void shouldNotAllowEvenNumberOfKeyValuesAndInvalidKeys() { try { - ElementHelper.legalPropertyKeyValueArray(false,"aKey", "test", "value-for-this-one", 1, 1, "none"); + ElementHelper.legalPropertyKeyValueArray("aKey", "test", "value-for-this-one", 1, 1, "none"); fail("Should fail as there is an even number of key-values, but a bad key"); } catch (IllegalArgumentException iae) { assertEquals(Element.Exceptions.providedKeyValuesMustHaveALegalKeyOnEvenIndices().getMessage(), iae.getMessage()); @@ -126,16 +116,7 @@ public class ElementHelperTest { @Test public void shouldAllowEvenNumberOfKeyValuesAndValidKeys() { - ElementHelper.legalPropertyKeyValueArray(false,"aKey", "test", "value-for-this-one", 1, "1", "none"); - } - - @Test - public void shouldNotAllowEvenNumberOfKeyValuesAndInvalidValues() { - try { - ElementHelper.legalPropertyKeyValueArray(false,"aKey", "test", "value-for-this-one", 1, "1", null); - } catch (IllegalArgumentException iae) { - assertEquals(Property.Exceptions.propertyValueCanNotBeNull().getMessage(), iae.getMessage()); - } + ElementHelper.legalPropertyKeyValueArray("aKey", "test", "value-for-this-one", 1, "1", "none"); } @Test @@ -196,6 +177,15 @@ public class ElementHelperTest { @Test public void shouldAttachPropertiesButNotLabelsOrId() { final Element mockElement = mock(Element.class); + final Graph mockGraph = mock(Graph.class); + final Graph.Features mockGraphFeatures = mock(Graph.Features.class); + final Graph.Features.VertexFeatures mockVertexFeatures = mock(Graph.Features.VertexFeatures.class); + final Graph.Features.VertexPropertyFeatures mockVertexPropertyFeatures = mock(Graph.Features.VertexPropertyFeatures.class); + when(mockElement.graph()).thenReturn(mockGraph); + when(mockGraph.features()).thenReturn(mockGraphFeatures); + when(mockGraphFeatures.vertex()).thenReturn(mockVertexFeatures); + when(mockVertexFeatures.properties()).thenReturn(mockVertexPropertyFeatures); + when(mockVertexPropertyFeatures.supportsNullPropertyValues()).thenReturn(true); ElementHelper.attachProperties(mockElement, "test", 123, T.id, 321, T.label, "friends"); verify(mockElement, times(1)).property("test", 123); verify(mockElement, times(0)).property(T.id.getAccessor(), 321); @@ -205,6 +195,15 @@ public class ElementHelperTest { @Test(expected = ClassCastException.class) public void shouldFailTryingToAttachPropertiesNonStringKey() { final Element mockElement = mock(Element.class); + final Graph mockGraph = mock(Graph.class); + final Graph.Features mockGraphFeatures = mock(Graph.Features.class); + final Graph.Features.VertexFeatures mockVertexFeatures = mock(Graph.Features.VertexFeatures.class); + final Graph.Features.VertexPropertyFeatures mockVertexPropertyFeatures = mock(Graph.Features.VertexPropertyFeatures.class); + when(mockElement.graph()).thenReturn(mockGraph); + when(mockGraph.features()).thenReturn(mockGraphFeatures); + when(mockGraphFeatures.vertex()).thenReturn(mockVertexFeatures); + when(mockVertexFeatures.properties()).thenReturn(mockVertexPropertyFeatures); + when(mockVertexPropertyFeatures.supportsNullPropertyValues()).thenReturn(true); ElementHelper.attachProperties(mockElement, "test", 123, 321, "test"); } @@ -221,6 +220,15 @@ public class ElementHelperTest { @Test public void shouldAttachPropertiesWithCardinalityButNotLabelsOrId() { final Vertex mockElement = mock(Vertex.class); + final Graph mockGraph = mock(Graph.class); + final Graph.Features mockGraphFeatures = mock(Graph.Features.class); + final Graph.Features.VertexFeatures mockVertexFeatures = mock(Graph.Features.VertexFeatures.class); + final Graph.Features.VertexPropertyFeatures mockVertexPropertyFeatures = mock(Graph.Features.VertexPropertyFeatures.class); + when(mockElement.graph()).thenReturn(mockGraph); + when(mockGraph.features()).thenReturn(mockGraphFeatures); + when(mockGraphFeatures.vertex()).thenReturn(mockVertexFeatures); + when(mockVertexFeatures.properties()).thenReturn(mockVertexPropertyFeatures); + when(mockVertexPropertyFeatures.supportsNullPropertyValues()).thenReturn(true); ElementHelper.attachProperties(mockElement, VertexProperty.Cardinality.single, "test", 123, T.id, 321, T.label, "friends"); verify(mockElement, times(1)).property(VertexProperty.Cardinality.single, "test", 123); verify(mockElement, times(0)).property(VertexProperty.Cardinality.single, T.id.getAccessor(), 321); @@ -230,6 +238,15 @@ public class ElementHelperTest { @Test(expected = ClassCastException.class) public void shouldFailTryingToAttachPropertiesWithCardinalityNonStringKey() { final Element mockElement = mock(Vertex.class); + final Graph mockGraph = mock(Graph.class); + final Graph.Features mockGraphFeatures = mock(Graph.Features.class); + final Graph.Features.VertexFeatures mockVertexFeatures = mock(Graph.Features.VertexFeatures.class); + final Graph.Features.VertexPropertyFeatures mockVertexPropertyFeatures = mock(Graph.Features.VertexPropertyFeatures.class); + when(mockElement.graph()).thenReturn(mockGraph); + when(mockGraph.features()).thenReturn(mockGraphFeatures); + when(mockGraphFeatures.vertex()).thenReturn(mockVertexFeatures); + when(mockVertexFeatures.properties()).thenReturn(mockVertexPropertyFeatures); + when(mockVertexPropertyFeatures.supportsNullPropertyValues()).thenReturn(true); ElementHelper.attachProperties(mockElement, VertexProperty.Cardinality.single, "test", 123, 321, "test"); } diff --git a/gremlin-test/features/map/AddEdge.feature b/gremlin-test/features/map/AddEdge.feature index 875e474..8e03ac3 100644 --- a/gremlin-test/features/map/AddEdge.feature +++ b/gremlin-test/features/map/AddEdge.feature @@ -72,7 +72,7 @@ Feature: Step - addE() And the graph should return 4 for count of "g.V(v1Id).bothE()" And the graph should return 1 for count of "g.V(v1Id).inE().has(\"weight\", 2.0)" - Scenario: g_VX1X_asXaX_outXcreatedX_addEXcreatedByX_toXaX_propertyXweight_nullX + Scenario: g_V_outE_propertyXweight_nullX Given the empty graph And the graph initializer of """ @@ -89,16 +89,13 @@ Feature: Step - addE() addE("created").from("josh").to("lop").property(T.id, 11).property("weight", 0.4). addE("created").from("peter").to("lop").property(T.id, 12).property("weight", 0.2) """ - And using the parameter v1Id defined as "v[marko].id" And the traversal of """ - g.V(v1Id).as("a").out("created").addE("createdBy").to("a").property("weight", null) + g.V().outE().property("weight", null) """ When iterated to list - Then the result should have a count of 1 - And the graph should return 7 for count of "g.E()" - And the graph should return 4 for count of "g.V(v1Id).bothE()" - And the graph should return 1 for count of "g.V(v1Id).inE().has(\"weight\", null)" + Then the result should have a count of 6 + And the graph should return 0 for count of "g.E().properties(\"weight\")" Scenario: g_V_aggregateXxX_asXaX_selectXxX_unfold_addEXexistsWithX_toXaX_propertyXtime_nowX Given the empty graph diff --git a/gremlin-test/features/map/AddVertex.feature b/gremlin-test/features/map/AddVertex.feature index efe3fc0..0fc338f 100644 --- a/gremlin-test/features/map/AddVertex.feature +++ b/gremlin-test/features/map/AddVertex.feature @@ -93,7 +93,7 @@ Feature: Step - addV() Then the result should have a count of 1 And the graph should return 1 for count of "g.V().has(\"person\",\"name\",\"stephen\")" - Scenario: g_addVXpersonX_propertyXname_nullX + Scenario: g_V_hasLabelXpersonX_propertyXname_nullX Given the empty graph And the graph initializer of """ @@ -112,11 +112,11 @@ Feature: Step - addV() """ And the traversal of """ - g.addV("person").property("name", null) + g.V().hasLabel("person").property("name", 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().has(\"person\",\"name\",null)" + Then the result should have a count of 4 + And the graph should return 2 for count of "g.V().properties(\"name\")" Scenario: g_addVXpersonX_propertyXsingle_name_stephenX_propertyXsingle_name_stephenmX Given the empty graph diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/CoreTraversalTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/CoreTraversalTest.java index 92794c5..afdc1ef 100644 --- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/CoreTraversalTest.java +++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/CoreTraversalTest.java @@ -50,6 +50,7 @@ import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.inject import static org.apache.tinkerpop.gremlin.structure.Graph.Features.GraphFeatures.FEATURE_TRANSACTIONS; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -66,6 +67,44 @@ import static org.junit.Assert.fail; public class CoreTraversalTest extends AbstractGremlinProcessTest { @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) + @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_NULL_PROPERTY_VALUES) + public void g_addVXpersonX_propertyXname_nullX() { + final Traversal<Vertex, Vertex> traversal = g.addV("person").property("name", null); + printTraversalForm(traversal); + final Vertex nulled = traversal.next(); + assertFalse(traversal.hasNext()); + assertEquals("person", nulled.label()); + assertNull(nulled.value("name")); + assertEquals(1, IteratorUtils.count(nulled.properties())); + assertEquals(7, IteratorUtils.count(g.V())); + } + + @Test + @LoadGraphWith(MODERN) + @FeatureRequirement(featureClass = Graph.Features.EdgeFeatures.class, feature = Graph.Features.EdgeFeatures.FEATURE_ADD_EDGES) + @FeatureRequirement(featureClass = Graph.Features.EdgeFeatures.class, feature = Graph.Features.EdgeFeatures.FEATURE_NULL_PROPERTY_VALUES) + public void g_VX1X_asXaX_outXcreatedX_addEXcreatedByX_toXaX_propertyXweight_nullX() { + final Traversal<Vertex, Edge> traversal = g.V(convertToVertexId("marko")).as("a").out("created").addE("createdBy").to("a").property("weight", null); + printTraversalForm(traversal); + int count = 0; + while (traversal.hasNext()) { + final Edge edge = traversal.next(); + assertEquals("createdBy", edge.label()); + assertNull(g.E(edge).<Double>values("weight").next()); + assertEquals(1, g.E(edge).properties().count().next().intValue()); + count++; + + + } + assertEquals(1, count); + assertEquals(7, IteratorUtils.count(g.E())); + assertEquals(6, IteratorUtils.count(g.V())); + } + + @Test @LoadGraphWith public void shouldNeverPropagateANoBulkTraverser() { try { diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddEdgeTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddEdgeTest.java index f534a4b..1e7f162 100644 --- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddEdgeTest.java +++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddEdgeTest.java @@ -44,9 +44,10 @@ import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.outE; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.select; import static org.apache.tinkerpop.gremlin.structure.Column.keys; import static org.apache.tinkerpop.gremlin.structure.Column.values; +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.assertNull; /** * @author Marko A. Rodriguez (http://markorodriguez.com) @@ -59,7 +60,7 @@ public abstract class AddEdgeTest extends AbstractGremlinProcessTest { public abstract Traversal<Vertex, Edge> get_g_VX1X_asXaX_outXcreatedX_addEXcreatedByX_toXaX_propertyXweight_2X(final Object v1Id); - public abstract Traversal<Vertex, Edge> get_g_VX1X_asXaX_outXcreatedX_addEXcreatedByX_toXaX_propertyXweight_nullX(final Object v1Id); + public abstract Traversal<Vertex, Edge> get_g_V_outE_propertyXweight_nullX(); public abstract Traversal<Vertex, Edge> get_g_V_aggregateXxX_asXaX_selectXxX_unfold_addEXexistsWithX_toXaX_propertyXtime_nowX(); @@ -123,24 +124,11 @@ public abstract class AddEdgeTest extends AbstractGremlinProcessTest { @Test @LoadGraphWith(MODERN) - @FeatureRequirement(featureClass = Graph.Features.EdgeFeatures.class, feature = Graph.Features.EdgeFeatures.FEATURE_ADD_EDGES) - @FeatureRequirement(featureClass = Graph.Features.EdgeFeatures.class, feature = Graph.Features.EdgeFeatures.FEATURE_NULL_PROPERTY_VALUES) - public void g_VX1X_asXaX_outXcreatedX_addEXcreatedByX_toXaX_propertyXweight_nullX() { - final Traversal<Vertex, Edge> traversal = get_g_VX1X_asXaX_outXcreatedX_addEXcreatedByX_toXaX_propertyXweight_nullX(convertToVertexId("marko")); + @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_NULL_PROPERTY_VALUES, supported = false) + public void g_V_outE_propertyXweight_nullX() { + final Traversal<Vertex, Edge> traversal = get_g_V_outE_propertyXweight_nullX(); printTraversalForm(traversal); - int count = 0; - while (traversal.hasNext()) { - final Edge edge = traversal.next(); - assertEquals("createdBy", edge.label()); - assertNull(g.E(edge).<Double>values("weight").next()); - assertEquals(1, g.E(edge).properties().count().next().intValue()); - count++; - - - } - assertEquals(1, count); - assertEquals(7, IteratorUtils.count(g.E())); - assertEquals(6, IteratorUtils.count(g.V())); + traversal.forEachRemaining(e -> assertThat(e.properties("weight").hasNext(), is(false))); } @Test @@ -331,8 +319,8 @@ public abstract class AddEdgeTest extends AbstractGremlinProcessTest { } @Override - public Traversal<Vertex, Edge> get_g_VX1X_asXaX_outXcreatedX_addEXcreatedByX_toXaX_propertyXweight_nullX(final Object v1Id) { - return g.V(v1Id).as("a").out("created").addE("createdBy").to("a").property("weight", null); + public Traversal<Vertex, Edge> get_g_V_outE_propertyXweight_nullX() { + return g.V().outE().property("weight", null); } @Override 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 ebad007..8937a44 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 @@ -40,9 +40,10 @@ import java.util.Map; import static org.apache.tinkerpop.gremlin.LoadGraphWith.GraphData.MODERN; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.V; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.select; +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.assertNull; import static org.junit.Assert.assertTrue; /** @@ -57,7 +58,7 @@ public abstract class AddVertexTest extends AbstractGremlinTest { public abstract Traversal<Vertex, Vertex> get_g_addVXpersonX_propertyXname_stephenX(); - public abstract Traversal<Vertex, Vertex> get_g_addVXpersonX_propertyXname_nullX(); + public abstract Traversal<Vertex, Vertex> get_g_V_hasLabelXpersonX_propertyXname_nullX(); public abstract Traversal<Vertex, Vertex> get_g_addVXpersonX_propertyXsingle_name_stephenX_propertyXsingle_name_stephenmX(); @@ -131,18 +132,11 @@ public abstract class AddVertexTest extends AbstractGremlinTest { @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) - @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_NULL_PROPERTY_VALUES) - public void g_addVXpersonX_propertyXname_nullX() { - final Traversal<Vertex, Vertex> traversal = get_g_addVXpersonX_propertyXname_nullX(); + @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_NULL_PROPERTY_VALUES, supported = false) + public void g_V_hasLabelXpersonX_propertyXname_nullX() { + final Traversal<Vertex, Vertex> traversal = get_g_V_hasLabelXpersonX_propertyXname_nullX(); printTraversalForm(traversal); - final Vertex nulled = traversal.next(); - assertFalse(traversal.hasNext()); - assertEquals("person", nulled.label()); - assertNull(nulled.value("name")); - assertEquals(1, IteratorUtils.count(nulled.properties())); - assertEquals(7, IteratorUtils.count(g.V())); + traversal.forEachRemaining(v -> assertThat(v.properties("name").hasNext(), is(false))); } @Test @@ -334,8 +328,8 @@ public abstract class AddVertexTest extends AbstractGremlinTest { } @Override - public Traversal<Vertex, Vertex> get_g_addVXpersonX_propertyXname_nullX() { - return g.addV("person").property("name", null); + public Traversal<Vertex, Vertex> get_g_V_hasLabelXpersonX_propertyXname_nullX() { + return g.V().hasLabel("person").property("name", null); } @Override diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/PropertyTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/PropertyTest.java index b76f137..099b3a3 100644 --- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/PropertyTest.java +++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/PropertyTest.java @@ -40,6 +40,7 @@ import java.util.Map; import static org.apache.tinkerpop.gremlin.structure.Graph.Features.PropertyFeatures.FEATURE_PROPERTIES; import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; import static org.junit.Assert.fail; @@ -57,9 +58,6 @@ public class PropertyTest { /** * Basic tests for the {@link Property} class. */ - @ExceptionCoverage(exceptionClass = Property.Exceptions.class, methods = { - "propertyValueCanNotBeNull" - }) public static class BasicPropertyTest extends AbstractGremlinTest { @Test @FeatureRequirementSet(FeatureRequirementSet.Package.VERTICES_ONLY) @@ -150,25 +148,17 @@ public class PropertyTest { @FeatureRequirementSet(FeatureRequirementSet.Package.VERTICES_ONLY) @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_NULL_PROPERTY_VALUES, supported = false) public void shouldNotAllowNullAddVertex() throws Exception { - try { - this.graph.addVertex("name", null); - fail("Call to addVertex() should have thrown an exception as null is not a supported property value"); - } catch (Exception ex) { - validateException(Property.Exceptions.propertyValueCanNotBeNull(), ex); - } + final Vertex v = this.graph.addVertex("name", null); + assertThat(v.properties("name").hasNext(), is(false)); } @Test @FeatureRequirementSet(FeatureRequirementSet.Package.SIMPLE) @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_NULL_PROPERTY_VALUES, supported = false) public void shouldNotAllowNullAddEdge() throws Exception { - try { - final Vertex v = this.graph.addVertex(); - v.addEdge("self", v, "name", null); - fail("Call to addEdge() should have thrown an exception as null is not a supported property value"); - } catch (Exception ex) { - validateException(Property.Exceptions.propertyValueCanNotBeNull(), ex); - } + final Vertex v = this.graph.addVertex(); + final Edge e = v.addEdge("self", v, "name", null); + assertThat(e.properties("name").hasNext(), is(false)); } @Test @@ -204,13 +194,10 @@ public class PropertyTest { @FeatureRequirement(featureClass = Graph.Features.VertexPropertyFeatures.class, feature = Graph.Features.VertexPropertyFeatures.FEATURE_NULL_PROPERTY_VALUES, supported = false) @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_META_PROPERTIES) public void shouldNotAllowNullAddVertexProperty() throws Exception { - try { - final Vertex v = this.graph.addVertex("person"); - final VertexProperty vp = v.property("location", "santa fe", "startTime", 1995, "endTime", null); - fail("Call to property() should have thrown an exception as null is not a supported property value"); - } catch (Exception ex) { - validateException(Property.Exceptions.propertyValueCanNotBeNull(), ex); - } + final Vertex v = this.graph.addVertex("person"); + final VertexProperty vp = v.property("location", "santa fe", "startTime", 1995, "endTime", null); + assertEquals(1995, (int) vp.value("startTime")); + assertThat(vp.properties("endTime").hasNext(), is(false)); } } diff --git a/neo4j-gremlin/src/main/java/org/apache/tinkerpop/gremlin/neo4j/structure/Neo4jEdge.java b/neo4j-gremlin/src/main/java/org/apache/tinkerpop/gremlin/neo4j/structure/Neo4jEdge.java index 2729f9f..08282fc 100644 --- a/neo4j-gremlin/src/main/java/org/apache/tinkerpop/gremlin/neo4j/structure/Neo4jEdge.java +++ b/neo4j-gremlin/src/main/java/org/apache/tinkerpop/gremlin/neo4j/structure/Neo4jEdge.java @@ -114,7 +114,13 @@ public final class Neo4jEdge extends Neo4jElement implements Edge, WrappedEdge<N @Override public <V> Property<V> property(final String key, final V value) { - ElementHelper.validateProperty(false, key, value); + ElementHelper.validateProperty(key, value); + + if (null == value) { + properties(key).forEachRemaining(Property::remove); + return Property.empty(); + } + this.graph.tx().readWrite(); try { this.baseElement.setProperty(key, value); diff --git a/neo4j-gremlin/src/main/java/org/apache/tinkerpop/gremlin/neo4j/structure/Neo4jGraph.java b/neo4j-gremlin/src/main/java/org/apache/tinkerpop/gremlin/neo4j/structure/Neo4jGraph.java index 74270d3..55be5f6 100644 --- a/neo4j-gremlin/src/main/java/org/apache/tinkerpop/gremlin/neo4j/structure/Neo4jGraph.java +++ b/neo4j-gremlin/src/main/java/org/apache/tinkerpop/gremlin/neo4j/structure/Neo4jGraph.java @@ -127,7 +127,7 @@ public final class Neo4jGraph implements Graph, WrappedGraph<Neo4jGraphAPI> { @Override public Vertex addVertex(final Object... keyValues) { - ElementHelper.legalPropertyKeyValueArray(false, keyValues); + ElementHelper.legalPropertyKeyValueArray(keyValues); if (ElementHelper.getIdValue(keyValues).isPresent()) throw Vertex.Exceptions.userSuppliedIdsNotSupported(); this.tx().readWrite(); diff --git a/neo4j-gremlin/src/main/java/org/apache/tinkerpop/gremlin/neo4j/structure/Neo4jVertex.java b/neo4j-gremlin/src/main/java/org/apache/tinkerpop/gremlin/neo4j/structure/Neo4jVertex.java index 2bbca2f..98b1893 100644 --- a/neo4j-gremlin/src/main/java/org/apache/tinkerpop/gremlin/neo4j/structure/Neo4jVertex.java +++ b/neo4j-gremlin/src/main/java/org/apache/tinkerpop/gremlin/neo4j/structure/Neo4jVertex.java @@ -56,7 +56,7 @@ public final class Neo4jVertex extends Neo4jElement implements Vertex, WrappedVe public Edge addEdge(final String label, final Vertex inVertex, final Object... keyValues) { if (null == inVertex) throw Graph.Exceptions.argumentCanNotBeNull("inVertex"); ElementHelper.validateLabel(label); - ElementHelper.legalPropertyKeyValueArray(false, keyValues); + ElementHelper.legalPropertyKeyValueArray(keyValues); if (ElementHelper.getIdValue(keyValues).isPresent()) throw Edge.Exceptions.userSuppliedIdsNotSupported(); @@ -91,13 +91,19 @@ public final class Neo4jVertex extends Neo4jElement implements Vertex, WrappedVe @Override public <V> VertexProperty<V> property(final VertexProperty.Cardinality cardinality, final String key, final V value, final Object... keyValues) { - ElementHelper.validateProperty(false, key, value); + ElementHelper.validateProperty(key, value); if (ElementHelper.getIdValue(keyValues).isPresent()) throw Vertex.Exceptions.userSuppliedIdsNotSupported(); this.graph.tx().readWrite(); if (cardinality != VertexProperty.Cardinality.single) throw VertexProperty.Exceptions.multiPropertiesNotSupported(); + + if (null == value) { + properties(key).forEachRemaining(VertexProperty::remove); + return VertexProperty.empty(); + } + if (keyValues.length > 0) throw VertexProperty.Exceptions.metaPropertiesNotSupported(); try { diff --git a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/computer/TinkerGraphComputerView.java b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/computer/TinkerGraphComputerView.java index b61fc2a..43998fb 100644 --- a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/computer/TinkerGraphComputerView.java +++ b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/computer/TinkerGraphComputerView.java @@ -79,7 +79,7 @@ public final class TinkerGraphComputerView { } public <V> Property<V> addProperty(final TinkerVertex vertex, final String key, final V value) { - ElementHelper.validateProperty(true, key, value); + ElementHelper.validateProperty(key, value); if (isComputeKey(key)) { final TinkerVertexProperty<V> property = new TinkerVertexProperty<V>((TinkerVertex) vertex, key, value) { @Override diff --git a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerEdge.java b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerEdge.java index d594fea..02f7ede 100644 --- a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerEdge.java +++ b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerEdge.java @@ -56,7 +56,13 @@ public final class TinkerEdge extends TinkerElement implements Edge { @Override public <V> Property<V> property(final String key, final V value) { if (this.removed) throw elementAlreadyRemoved(Edge.class, id); - ElementHelper.validateProperty(allowNullPropertyValues, key, value); + ElementHelper.validateProperty(key, value); + + if (!allowNullPropertyValues && null == value) { + properties(key).forEachRemaining(Property::remove); + return Property.empty(); + } + final Property oldProperty = super.property(key); final Property<V> newProperty = new TinkerProperty<>(this, key, value); if (null == this.properties) this.properties = new HashMap<>(); diff --git a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraph.java b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraph.java index 2410445..6f4f821 100644 --- a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraph.java +++ b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraph.java @@ -116,7 +116,7 @@ public final class TinkerGraph implements Graph { vertexPropertyIdManager = selectIdManager(configuration, GREMLIN_TINKERGRAPH_VERTEX_PROPERTY_ID_MANAGER, VertexProperty.class); defaultVertexPropertyCardinality = VertexProperty.Cardinality.valueOf( configuration.getString(GREMLIN_TINKERGRAPH_DEFAULT_VERTEX_PROPERTY_CARDINALITY, VertexProperty.Cardinality.single.name())); - allowNullPropertyValues = configuration.getBoolean(GREMLIN_TINKERGRAPH_ALLOW_NULL_PROPERTY_VALUES, true); + allowNullPropertyValues = configuration.getBoolean(GREMLIN_TINKERGRAPH_ALLOW_NULL_PROPERTY_VALUES, false); graphLocation = configuration.getString(GREMLIN_TINKERGRAPH_GRAPH_LOCATION, null); graphFormat = configuration.getString(GREMLIN_TINKERGRAPH_GRAPH_FORMAT, null); @@ -161,7 +161,7 @@ public final class TinkerGraph implements Graph { @Override public Vertex addVertex(final Object... keyValues) { - ElementHelper.legalPropertyKeyValueArray(allowNullPropertyValues, keyValues); + ElementHelper.legalPropertyKeyValueArray(keyValues); Object idValue = vertexIdManager.convert(ElementHelper.getIdValue(keyValues).orElse(null)); final String label = ElementHelper.getLabelValue(keyValues).orElse(Vertex.DEFAULT_LABEL); diff --git a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerHelper.java b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerHelper.java index 2b1a928..4ed3fdd 100644 --- a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerHelper.java +++ b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerHelper.java @@ -49,7 +49,7 @@ public final class TinkerHelper { protected static Edge addEdge(final TinkerGraph graph, final TinkerVertex outVertex, final TinkerVertex inVertex, final String label, final Object... keyValues) { ElementHelper.validateLabel(label); - ElementHelper.legalPropertyKeyValueArray(graph.features().edge().supportsNullPropertyValues(), keyValues); + ElementHelper.legalPropertyKeyValueArray(keyValues); Object idValue = graph.edgeIdManager.convert(ElementHelper.getIdValue(keyValues).orElse(null)); 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 7fe7ce0..52518d6 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 @@ -85,9 +85,15 @@ public final class TinkerVertex extends TinkerElement implements Vertex { @Override public <V> VertexProperty<V> property(final VertexProperty.Cardinality cardinality, final String key, final V value, final Object... keyValues) { if (this.removed) throw elementAlreadyRemoved(Vertex.class, id); - ElementHelper.legalPropertyKeyValueArray(allowNullPropertyValues, keyValues); - ElementHelper.validateProperty(allowNullPropertyValues, key, value); + ElementHelper.legalPropertyKeyValueArray(keyValues); + ElementHelper.validateProperty(key, value); final Optional<Object> optionalId = ElementHelper.getIdValue(keyValues); + + if (!allowNullPropertyValues && null == value) { + properties(key).forEachRemaining(VertexProperty::remove); + return VertexProperty.empty(); + } + final Optional<VertexProperty<V>> optionalVertexProperty = ElementHelper.stageVertexProperty(this, cardinality, key, value, keyValues); if (optionalVertexProperty.isPresent()) return optionalVertexProperty.get(); 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 f01a3f2..ed8acec 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 @@ -18,7 +18,6 @@ */ package org.apache.tinkerpop.gremlin.tinkergraph.structure; -import org.apache.tinkerpop.gremlin.structure.Element; import org.apache.tinkerpop.gremlin.structure.Graph; import org.apache.tinkerpop.gremlin.structure.Property; import org.apache.tinkerpop.gremlin.structure.Vertex; @@ -53,13 +52,7 @@ public class TinkerVertexProperty<V> extends TinkerElement implements VertexProp * with {@link TinkerGraphComputerView}. */ public TinkerVertexProperty(final TinkerVertex vertex, final String key, final V value, final Object... propertyKeyValues) { - super(((TinkerGraph) vertex.graph()).vertexPropertyIdManager.getNextId((TinkerGraph) vertex.graph()), key); - this.allowNullPropertyValues = vertex.graph().features().vertex().properties().supportsNullPropertyValues(); - this.vertex = vertex; - this.key = key; - this.value = value; - ElementHelper.legalPropertyKeyValueArray(allowNullPropertyValues, propertyKeyValues); - ElementHelper.attachProperties(this, propertyKeyValues); + this(((TinkerGraph) vertex.graph()).vertexPropertyIdManager.getNextId((TinkerGraph) vertex.graph()), vertex, key, value, propertyKeyValues); } /** @@ -69,10 +62,13 @@ public class TinkerVertexProperty<V> extends TinkerElement implements VertexProp public TinkerVertexProperty(final Object id, final TinkerVertex vertex, final String key, final V value, final Object... propertyKeyValues) { super(id, key); this.allowNullPropertyValues = vertex.graph().features().vertex().properties().supportsNullPropertyValues(); + if (!allowNullPropertyValues && null == value) + throw new IllegalArgumentException("value cannot be null as feature supportsNullPropertyValues is false"); + this.vertex = vertex; this.key = key; this.value = value; - ElementHelper.legalPropertyKeyValueArray(allowNullPropertyValues, propertyKeyValues); + ElementHelper.legalPropertyKeyValueArray(propertyKeyValues); ElementHelper.attachProperties(this, propertyKeyValues); } @@ -120,6 +116,12 @@ public class TinkerVertexProperty<V> extends TinkerElement implements VertexProp @Override public <U> Property<U> property(final String key, final U value) { if (this.removed) throw elementAlreadyRemoved(VertexProperty.class, id); + + if ((!allowNullPropertyValues && null == value)) { + properties(key).forEachRemaining(Property::remove); + return Property.empty(); + } + final Property<U> property = new TinkerProperty<>(this, key, value); if (this.properties == null) this.properties = new HashMap<>(); this.properties.put(key, property); diff --git a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphTest.java b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphTest.java index f9c661e..555c7ca 100644 --- a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphTest.java +++ b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphTest.java @@ -27,7 +27,6 @@ import org.apache.tinkerpop.gremlin.process.traversal.P; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; import org.apache.tinkerpop.gremlin.process.traversal.strategy.verification.ReservedKeysVerificationStrategy; -import org.apache.tinkerpop.gremlin.process.traversal.strategy.verification.VerificationException; import org.apache.tinkerpop.gremlin.process.traversal.util.Metrics; import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalMetrics; import org.apache.tinkerpop.gremlin.structure.Edge;
