Usage of null deviates from TinkerPop 3.5.0

[li-boxuan]

TinkerPop 3.5.0 makes the usage of null consistent: http://tinkerpop.apache.org/docs/3.5.0-SNAPSHOT/upgrade/#_use_of_null

This means the semantics of null has changed: http://tinkerpop.apache.org/docs/3.5.0-SNAPSHOT/upgrade/#_graph_system_providers

This blocks the upgrade to 3.5.0 because the following test is failing

https://github.com/apache/tinkerpop/blob/7025ee2a9abf10cef10749394476061e289cbf70/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/VertexPropertyTest.java#L174-L188

// 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);
});

[porunov]

Can't we just disallow null values? Tinkerpop 3.5.0 has a feature method supportsNullPropertyValues() which returns true if a graph provider supports null values and false if it doesn't. JanusGraph should probably set supportsNullPropertyValues to false because we won't be able to support null values in the near future (probably). That said, I didn't check the implementation yet.

[li-boxuan]

This test is exactly for the case when supportsNullPropertyValues is false. If the null value is supported, the null value will be inserted as a legitimate value. If the null value is not supported and null appears in mutation, providers are expected to ignore it if the property has set or list cardinality and remove the property if it has single cardinality.

[porunov]

Ah, I see now from the action logs that we are producing:

org.janusgraph.core.SchemaViolationException: Property value cannot be null
    at org.apache.tinkerpop.gremlin.structure.VertexPropertyTest$VertexPropertyAddition.shouldHandleListVertexPropertiesWithoutNullPropertyValues(VertexPropertyTest.java:176)

I think, we should change the logic to follow TinkerPop's logic. I.e., instead of throwing an exception we should remove property when the cardinality is Single and otherwise we should simply ignore this method.

[li-boxuan]

This has to be fixed within https://github.com/JanusGraph/janusgraph/pull/2619 since TinkerPop 3.4 does not allow null values in mutation.

[porunov]

Fixed in #2619