diff --git a/janusgraph-backend-testutils/src/main/java/org/janusgraph/graphdb/JanusGraphTest.java b/janusgraph-backend-testutils/src/main/java/org/janusgraph/graphdb/JanusGraphTest.java index 366a438e32..bcc3414ae5 100644 --- a/janusgraph-backend-testutils/src/main/java/org/janusgraph/graphdb/JanusGraphTest.java +++ b/janusgraph-backend-testutils/src/main/java/org/janusgraph/graphdb/JanusGraphTest.java @@ -151,6 +151,7 @@ import org.janusgraph.graphdb.database.management.ManagementSystem; import org.janusgraph.graphdb.database.serialize.Serializer; import org.janusgraph.graphdb.internal.ElementCategory; +import org.janusgraph.graphdb.internal.ElementLifeCycle; import org.janusgraph.graphdb.internal.InternalRelationType; import org.janusgraph.graphdb.internal.Order; import org.janusgraph.graphdb.internal.OrderList; @@ -162,7 +163,7 @@ import org.janusgraph.graphdb.query.profile.QueryProfiler; import org.janusgraph.graphdb.query.profile.SimpleQueryProfiler; import org.janusgraph.graphdb.query.vertex.BasicVertexCentricQueryBuilder; -import org.janusgraph.graphdb.relations.CacheVertexProperty; +import org.janusgraph.graphdb.relations.AbstractEdge; import org.janusgraph.graphdb.relations.RelationIdentifier; import org.janusgraph.graphdb.schema.EdgeLabelDefinition; import org.janusgraph.graphdb.schema.PropertyKeyDefinition; @@ -206,6 +207,207 @@ final boolean isLockingOptimistic() { return features.hasOptimisticLocking(); } + + /* ================================================================================== + UPDATE THEN REMOVE IN THE SAME TRANSACTION + ==================================================================================*/ + + private void initializeGraphWithVerticesAndEdges() { + if (mgmt.getEdgeLabel("fork-connect") == null) { + EdgeLabel forkConnect = mgmt.makeEdgeLabel("fork-connect").make(); + mgmt.setConsistency(forkConnect, ConsistencyModifier.FORK); + finishSchema(); + } + + Vertex v = graph.traversal().addV().property("_v", 1).next(); + v.property("_v").property("flag", false); + Vertex v2 = graph.traversal().addV().property("_v", 2).property("prop", "old").next(); + Edge edge = v.addEdge("connect", v2, "_e", 1); + Edge forkEdge = v.addEdge("fork-connect", v2, "_e", 2); + graph.tx().commit(); + } + + @Test + public void testUpdateVertexPropThenRemoveProp() { + initializeGraphWithVerticesAndEdges(); + Vertex v2 = graph.traversal().V().has("_v", 2).next(); + assertEquals("old", v2.property("prop").value()); + v2.property("prop", "new"); + assertEquals("new", v2.property("prop").value()); + v2.property("prop", "new2"); + assertEquals("new2", v2.property("prop").value()); + v2 = graph.traversal().V().has("_v", 2).next(); + v2.property("prop", "new3"); + assertEquals("new3", v2.property("prop").value()); + v2.property("prop").remove(); + graph.tx().commit(); + assertFalse(graph.traversal().V(v2).values("prop").hasNext()); + } + + @Test + public void testNestedAddVertexPropThenRemoveProp() { + // prepare graph with a single vertex + mgmt.makePropertyKey("prop").dataType(String.class).make(); + mgmt.commit(); + graph.addVertex(); + graph.tx().commit(); + + // open two transactions at the same time + final JanusGraphTransaction tx1 = graph.newTransaction(); + final JanusGraphTransaction tx2 = graph.newTransaction(); + + // tx1: add property + tx1.traversal().V().next().property("prop", "tx1"); + + // tx2: add property + Vertex v = tx2.traversal().V().next(); + v.property("prop", "tx2"); + + // tx1: commit + tx1.commit(); + + // tx2: remove property + v.property("prop").remove(); + + // tx2: commit + tx2.commit(); + + assertTrue(graph.traversal().V().hasNext()); + assertFalse(graph.traversal().V().values("prop").hasNext()); + } + + @Test + public void testUpdateVertexPropThenRemoveVertex() { + initializeGraphWithVerticesAndEdges(); + Vertex v2 = graph.traversal().V().has("_v", 2).next(); + assertEquals("old", v2.property("prop").value()); + v2.property("prop", "new"); + assertEquals("new", v2.property("prop").value()); + v2 = graph.traversal().V().has("_v", 2).next(); + v2.property("prop", "new2"); + assertEquals("new2", v2.property("prop").value()); + v2.remove(); + graph.tx().commit(); + assertFalse(graph.traversal().V(v2).hasNext()); + assertFalse(graph.traversal().V().has("prop", "old").hasNext()); + assertFalse(graph.traversal().V().has("prop", "new").hasNext()); + } + + /** + * update property of a vertex property and remove the vertex property + */ + @Test + public void testUpdatePropertyPropThenRemoveProperty() { + for (boolean reload : Arrays.asList(true, false)) { + graph.traversal().V().drop().iterate(); + initializeGraphWithVerticesAndEdges(); + assertTrue(graph.traversal().V().has("_v", 1).values("_v").hasNext()); + + VertexProperty p = (VertexProperty) graph.traversal().V().has("_v", 1).properties("_v").next(); + assertEquals(false, p.property("flag").value()); + p.property("flag", true); + assertEquals(true, p.property("flag").value()); + if (!reload) { + p.remove(); + } else { + ((VertexProperty) graph.traversal().V().has("_v", 1).properties("_v").next()).remove(); + } + graph.tx().commit(); + + assertFalse(graph.traversal().V().has("_v", 1).values("_v").hasNext()); + } + } + + /** + * update property of a vertex property and remove the property of the vertex property + */ + @Test + public void testUpdatePropertyPropThenRemovePropertyProp() { + initializeGraphWithVerticesAndEdges(); + VertexProperty p = (VertexProperty) graph.traversal().V().has("_v", 1).properties("_v").next(); + assertTrue(graph.traversal().V().has("_v", 1).properties("_v").values("flag").hasNext()); + assertEquals(false, p.property("flag").value()); + p.property("flag", true); + assertEquals(true, p.property("flag").value()); + p.property("flag").remove(); + graph.tx().commit(); + assertTrue(graph.traversal().V().has("_v", 1).values("_v").hasNext()); + assertFalse(graph.traversal().V().has("_v", 1).properties("_v").values("flag").hasNext()); + } + + @Test + public void testUpdatePropertyPropThenRemoveVertex() { + initializeGraphWithVerticesAndEdges(); + Vertex v = graph.traversal().V().has("_v", 1).next(); + VertexProperty p = (VertexProperty) v.properties("_v").next(); + assertEquals(false, p.property("flag").value()); + p.property("flag", true); + assertEquals(true, p.property("flag").value()); + p.property("flag").remove(); + v.remove(); + graph.tx().commit(); + assertFalse(graph.traversal().V().has("_v", 1).hasNext()); + } + + @Test + public void testUpdateEdgePropertyThenRemoveEdge() { + initializeGraphWithVerticesAndEdges(); + // normal edge + AbstractEdge edge = (AbstractEdge) graph.traversal().E().has("_e", 1).next(); + assertTrue(ElementLifeCycle.isLoaded(edge.getLifeCycle())); + Object id = edge.id(); + + for (int val : Arrays.asList(-1, -11)) { + edge.property("_e", val); + // the edge object represents the old edge to be deleted + assertEquals(id, edge.id()); + assertTrue(ElementLifeCycle.isRemoved(edge.getLifeCycle())); + // the edge object has a corresponding new edge with same id + assertEquals(id, edge.it().id()); + assertTrue(ElementLifeCycle.isNew(edge.it().getLifeCycle())); + assertTrue(edge.isNew()); + } + + edge.remove(); + graph.tx().commit(); + assertFalse(graph.traversal().E().has("_e", 1).hasNext()); + assertFalse(graph.traversal().E().has("_e", -1).hasNext()); + assertFalse(graph.traversal().E().has("_e", -11).hasNext()); + assertTrue(graph.traversal().E().has("_e", 2).hasNext()); + } + + @Test + public void testUpdateForkEdgePropertyThenRemoveEdge() { + initializeGraphWithVerticesAndEdges(); + // fork edge + AbstractEdge edge = (AbstractEdge) graph.traversal().E().has("_e", 2).next(); + assertTrue(ElementLifeCycle.isLoaded(edge.getLifeCycle())); + Object id = edge.id(); + + edge.property("_e", -2); + // the edge object represents the old edge to be deleted + assertEquals(id, edge.id()); + assertTrue(ElementLifeCycle.isRemoved(edge.getLifeCycle())); + // the edge object has a corresponding new (forked) edge with different id + Object forkedId = edge.it().id(); + assertNotEquals(id, forkedId); + assertTrue(ElementLifeCycle.isNew(edge.it().getLifeCycle())); + assertTrue(edge.isNew()); + + edge.property("_e", -3); + assertEquals(id, edge.id()); + assertTrue(ElementLifeCycle.isRemoved(edge.getLifeCycle())); + assertEquals(forkedId, edge.it().id()); + assertTrue(ElementLifeCycle.isNew(edge.it().getLifeCycle())); + assertTrue(edge.isNew()); + + edge.remove(); + graph.tx().commit(); + assertFalse(graph.traversal().E().has("_e", 2).hasNext()); + assertFalse(graph.traversal().E().has("_e", -2).hasNext()); + assertFalse(graph.traversal().E().has("_e", -3).hasNext()); + } + /* ================================================================================== INDEXING ==================================================================================*/ @@ -299,38 +501,6 @@ public void testVertexRemoval() { assertCount(0, graph.query().has(nameUniqueVertexPropertyName, "v2").vertices()); } - /** - * Update properties of vertex property, vertex, and edge, and then remove the vertex and edge in the same transaction - */ - @Test - public void testUpdateThenRemove() { - EdgeLabel forkConnect = mgmt.makeEdgeLabel("fork-connect").make(); - mgmt.setConsistency(forkConnect, ConsistencyModifier.FORK); - mgmt.commit(); - - Vertex v = graph.traversal().addV().property("_v", 1).next(); - v.property("_v").property("flag", false); - Vertex v2 = graph.traversal().addV().property("_v", 2).next(); - Edge edge = v.addEdge("connect", v2, "_p", 1); - Edge forkEdge = v.addEdge("fork-connect", v2, "_p", 1); - graph.tx().commit(); - - // update properties and then remove vertices & edges - JanusGraphVertexProperty p = (JanusGraphVertexProperty) graph.traversal().V(v).properties("_v").next(); - p.property("flag", true); - edge = graph.traversal().E(edge).next(); - edge.property("_p", 2); - forkEdge = graph.traversal().E(forkEdge).next(); - forkEdge.property("_p", 2); - // FIXME: https://github.com/JanusGraph/janusgraph/issues/1981 - // v2 = graph.traversal().V().has("_v", 2).next(); - // v2.property("_v", 3); - graph.traversal().V().drop().iterate(); - graph.tx().commit(); - assertFalse(graph.traversal().V().hasNext()); - assertFalse(graph.traversal().E().hasNext()); - } - /** * Iterating over all vertices and edges in a graph */ diff --git a/janusgraph-core/src/main/java/org/janusgraph/core/JanusGraphVertexProperty.java b/janusgraph-core/src/main/java/org/janusgraph/core/JanusGraphVertexProperty.java index 123fe48849..58bc67c23e 100644 --- a/janusgraph-core/src/main/java/org/janusgraph/core/JanusGraphVertexProperty.java +++ b/janusgraph-core/src/main/java/org/janusgraph/core/JanusGraphVertexProperty.java @@ -17,6 +17,8 @@ import org.apache.tinkerpop.gremlin.structure.VertexProperty; +import java.util.function.Consumer; + /** * JanusGraphProperty is a {@link JanusGraphRelation} connecting a vertex to a value. * JanusGraphProperty extends {@link JanusGraphRelation}, with methods for retrieving the property's value and key. @@ -45,4 +47,16 @@ default PropertyKey propertyKey() { return (PropertyKey)getType(); } + static Consumer getRemover(VertexProperty.Cardinality cardinality, Object value) { + if (cardinality == VertexProperty.Cardinality.single) { + return JanusGraphElement::remove; + } else { + return p -> { + if (p.value().equals(value)) { + p.remove(); + } + }; + } + } + } diff --git a/janusgraph-core/src/main/java/org/janusgraph/graphdb/relations/CacheEdge.java b/janusgraph-core/src/main/java/org/janusgraph/graphdb/relations/CacheEdge.java index 2754d5581e..9df0842ce5 100644 --- a/janusgraph-core/src/main/java/org/janusgraph/graphdb/relations/CacheEdge.java +++ b/janusgraph-core/src/main/java/org/janusgraph/graphdb/relations/CacheEdge.java @@ -62,10 +62,10 @@ public InternalRelation it() { if (startVertex.hasAddedRelations() && startVertex.hasRemovedRelations()) { //Test whether this relation has been replaced final long id = super.longId(); - final Iterable previous = startVertex.getAddedRelations( + final Iterable added = startVertex.getAddedRelations( internalRelation -> (internalRelation instanceof StandardEdge) && ((StandardEdge) internalRelation).getPreviousID() == id); - assert Iterables.size(previous) <= 1 || (isLoop() && Iterables.size(previous) == 2); - it = Iterables.getFirst(previous, null); + assert Iterables.size(added) <= 1 || (isLoop() && Iterables.size(added) == 2); + it = Iterables.getFirst(added, null); } if (it != null) @@ -91,7 +91,6 @@ private synchronized InternalRelation update() { StandardEdge u = (StandardEdge) tx().addEdge(id, getVertex(0), getVertex(1), edgeLabel()); u.setPreviousID(longId()); copyProperties(u); - setId(u.longId()); return u; } @@ -139,7 +138,7 @@ public byte getLifeCycle() { @Override public void remove() { - if (!tx().isRemovedRelation(super.longId())) { + if (!isRemoved()) { tx().removeRelation(this); }// else throw InvalidElementException.removedException(this); } diff --git a/janusgraph-core/src/main/java/org/janusgraph/graphdb/relations/CacheVertexProperty.java b/janusgraph-core/src/main/java/org/janusgraph/graphdb/relations/CacheVertexProperty.java index f2c44bc07a..3cecd1d88f 100644 --- a/janusgraph-core/src/main/java/org/janusgraph/graphdb/relations/CacheVertexProperty.java +++ b/janusgraph-core/src/main/java/org/janusgraph/graphdb/relations/CacheVertexProperty.java @@ -121,7 +121,7 @@ public byte getLifeCycle() { @Override public void remove() { - if (!tx().isRemovedRelation(longId())) { + if (!isRemoved()) { tx().removeRelation(this); }// else throw InvalidElementException.removedException(this); } diff --git a/janusgraph-core/src/main/java/org/janusgraph/graphdb/relations/StandardEdge.java b/janusgraph-core/src/main/java/org/janusgraph/graphdb/relations/StandardEdge.java index 7bcd365313..77d669ed92 100644 --- a/janusgraph-core/src/main/java/org/janusgraph/graphdb/relations/StandardEdge.java +++ b/janusgraph-core/src/main/java/org/janusgraph/graphdb/relations/StandardEdge.java @@ -38,8 +38,6 @@ public StandardEdge(long id, EdgeLabel label, InternalVertex start, InternalVert this.lifecycle = lifecycle; } - //############## SAME CODE AS StandardProperty ############################# - private static final Map EMPTY_PROPERTIES = ImmutableMap.of(); private byte lifecycle; diff --git a/janusgraph-core/src/main/java/org/janusgraph/graphdb/relations/StandardVertexProperty.java b/janusgraph-core/src/main/java/org/janusgraph/graphdb/relations/StandardVertexProperty.java index 70086f8a9d..f3cf65475a 100644 --- a/janusgraph-core/src/main/java/org/janusgraph/graphdb/relations/StandardVertexProperty.java +++ b/janusgraph-core/src/main/java/org/janusgraph/graphdb/relations/StandardVertexProperty.java @@ -17,6 +17,10 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; +import org.apache.tinkerpop.gremlin.structure.VertexProperty; +import org.janusgraph.core.JanusGraphElement; +import org.janusgraph.core.JanusGraphRelation; +import org.janusgraph.core.JanusGraphVertexProperty; import org.janusgraph.core.PropertyKey; import org.janusgraph.graphdb.internal.ElementLifeCycle; import org.janusgraph.graphdb.internal.InternalVertex; @@ -25,6 +29,7 @@ import java.util.Collections; import java.util.HashMap; import java.util.Map; +import java.util.function.Consumer; /** * @author Matthias Broecheler (me@matthiasb.com) @@ -37,14 +42,22 @@ public StandardVertexProperty(long id, PropertyKey type, InternalVertex vertex, this.lifecycle = lifecycle; } - //############## SAME CODE AS StandardEdge ############################# - private static final Map EMPTY_PROPERTIES = ImmutableMap.of(); + private boolean isUpsert; private byte lifecycle; private long previousID = 0; private volatile Map properties = EMPTY_PROPERTIES; + /** + * Mark this property as 'upsert', i.e. the old property value is not read from DB and marked as + * deleted in the transaction + * @param upsert + */ + public void setUpsert(final boolean upsert) { + isUpsert = upsert; + } + @Override public long getPreviousID() { return previousID; @@ -101,6 +114,11 @@ public synchronized void remove() { if (!ElementLifeCycle.isRemoved(lifecycle)) { tx().removeRelation(this); lifecycle = ElementLifeCycle.update(lifecycle, ElementLifeCycle.Event.REMOVED); + if (isUpsert) { + VertexProperty.Cardinality cardinality = ((PropertyKey) type).cardinality().convert(); + Consumer propertyRemover = JanusGraphVertexProperty.getRemover(cardinality, value()); + element().query().types(type.name()).properties().forEach(propertyRemover); + } } //else throw InvalidElementException.removedException(this); } diff --git a/janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/StandardJanusGraphTx.java b/janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/StandardJanusGraphTx.java index 65b9921f2b..bebff1b7a1 100644 --- a/janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/StandardJanusGraphTx.java +++ b/janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/StandardJanusGraphTx.java @@ -798,14 +798,13 @@ public JanusGraphVertexProperty addProperty(VertexProperty.Cardinality cardinali // } // } + long propId = id == null ? IDManager.getTemporaryRelationID(temporaryIds.nextID()) : id; + StandardVertexProperty prop = new StandardVertexProperty(propId, key, (InternalVertex) vertex, normalizedValue, ElementLifeCycle.New); + if (config.hasAssignIDsImmediately() && id == null) graph.assignID(prop); + //Delete properties if the cardinality is restricted if (cardinality==VertexProperty.Cardinality.single || cardinality== VertexProperty.Cardinality.set) { - Consumer propertyRemover; - if (cardinality == VertexProperty.Cardinality.single) - propertyRemover = JanusGraphElement::remove; - else - propertyRemover = p -> { if (((JanusGraphVertexProperty)p).value().equals(normalizedValue)) p.remove(); }; - + Consumer propertyRemover = JanusGraphVertexProperty.getRemover(cardinality, normalizedValue); /* If we are simply overwriting a vertex property, then we don't have to explicitly remove it thereby saving a read operation However, this only applies if 1) we don't lock on the property key or consistency checks are disabled and @@ -816,7 +815,10 @@ public JanusGraphVertexProperty addProperty(VertexProperty.Cardinality cardinali if ( (!config.hasVerifyUniqueness() || ((InternalRelationType)key).getConsistencyModifier()!=ConsistencyModifier.LOCK) && !TypeUtil.hasAnyIndex(key) && cardinality==keyCardinality.convert()) { //Only delete in-memory so as to not trigger a read from the database which isn't necessary because we will overwrite blindly - ((InternalVertex) vertex).getAddedRelations(p -> p.getType().equals(key)).forEach(propertyRemover); + //We need to label the new property as "upsert", so that in case property deletion happens, we not only delete this new + //in-memory property, but also read from database to delete the old value (if exists) + ((InternalVertex) vertex).getAddedRelations(p -> p.getType().equals(key)).forEach(p -> propertyRemover.accept((JanusGraphVertexProperty) p)); + prop.setUpsert(true); } else { ((InternalVertex) vertex).query().types(key).properties().forEach(propertyRemover); } @@ -830,9 +832,6 @@ public JanusGraphVertexProperty addProperty(VertexProperty.Cardinality cardinali throw new SchemaViolationException("Adding this property for key [%s] and value [%s] violates a uniqueness constraint [%s]", key.name(), normalizedValue, lockTuple.getIndex()); } } - long propId = id == null ? IDManager.getTemporaryRelationID(temporaryIds.nextID()) : id; - StandardVertexProperty prop = new StandardVertexProperty(propId, key, (InternalVertex) vertex, normalizedValue, ElementLifeCycle.New); - if (config.hasAssignIDsImmediately() && id == null) graph.assignID(prop); connectRelation(prop); return prop; } finally {