Skip to content

Commit

Permalink
Bug fixes: relations cannot be removed after update
Browse files Browse the repository at this point in the history 
This commit includes fixes for three bugs:

1) Caused by the optimization in StandardJanusGraphTx::addProperty, which saves a database
read (and delete) operation for vertex property upsert. If a vertex property (or the vertex
as a whole) is removed after the property is modified in the same transaction, then the old
value in database will not be deleted after the transaction commits. Reported by JanusGraph#1981.

2) Caused by CacheVertexProperty::remove method which does nothing if it finds the given
vertex property is already in the deletedRelations map. If the user deletes a vertex property
after they update a nested property of that vertex property in the same transaction, then
the old value will be removed from database but the new value will be saved into database
after the transaction commits.

3) Caused by CacheEdge::remove method which does nothing if it finds the given edge is
already in the deletedRelations map. If the user deletes an edge after they update a property
of that edge in the same transaction, then the old value will be removed from database but the
new value will be saved into database after the transaction commits.

Signed-off-by: Boxuan Li <[email protected]>
  • Loading branch information
@li-boxuan
li-boxuan committed Nov 13, 2020
1 parent b6575f2 commit a22a62e
Show file tree
Hide file tree
Showing 7 changed files with 251 additions and 53 deletions.
236 changes: 203 additions & 33 deletions janusgraph-backend-testutils/src/main/java/org/janusgraph/graphdb/JanusGraphTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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
==================================================================================*/
Expand Down Expand Up @@ -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
*/
Expand Down
14 changes: 14 additions & 0 deletions janusgraph-core/src/main/java/org/janusgraph/core/JanusGraphVertexProperty.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -45,4 +47,16 @@ default PropertyKey propertyKey() {
return (PropertyKey)getType();
}

static Consumer<JanusGraphVertexProperty> getRemover(VertexProperty.Cardinality cardinality, Object value) {
if (cardinality == VertexProperty.Cardinality.single) {
return JanusGraphElement::remove;
} else {
return p -> {
if (p.value().equals(value)) {
p.remove();
}
};
}
}

}
9 changes: 4 additions & 5 deletions janusgraph-core/src/main/java/org/janusgraph/graphdb/relations/CacheEdge.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<InternalRelation> previous = startVertex.getAddedRelations(
final Iterable<InternalRelation> 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)
Expand All @@ -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;
}

Expand Down Expand Up @@ -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);
}
Expand Down
2 changes: 1 addition & 1 deletion janusgraph-core/src/main/java/org/janusgraph/graphdb/relations/CacheVertexProperty.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
2 changes: 0 additions & 2 deletions janusgraph-core/src/main/java/org/janusgraph/graphdb/relations/StandardEdge.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,6 @@ public StandardEdge(long id, EdgeLabel label, InternalVertex start, InternalVert
this.lifecycle = lifecycle;
}

//############## SAME CODE AS StandardProperty #############################

private static final Map<PropertyKey, Object> EMPTY_PROPERTIES = ImmutableMap.of();

private byte lifecycle;
Expand Down
22 changes: 20 additions & 2 deletions janusgraph-core/src/main/java/org/janusgraph/graphdb/relations/StandardVertexProperty.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -25,6 +29,7 @@
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.function.Consumer;

/**
* @author Matthias Broecheler ([email protected])
Expand All @@ -37,14 +42,22 @@ public StandardVertexProperty(long id, PropertyKey type, InternalVertex vertex,
this.lifecycle = lifecycle;
}

//############## SAME CODE AS StandardEdge #############################

private static final Map<PropertyKey, Object> EMPTY_PROPERTIES = ImmutableMap.of();

private boolean isUpsert;
private byte lifecycle;
private long previousID = 0;
private volatile Map<PropertyKey, Object> 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;
Expand Down Expand Up @@ -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<JanusGraphVertexProperty> propertyRemover = JanusGraphVertexProperty.getRemover(cardinality, value());
element().query().types(type.name()).properties().forEach(propertyRemover);
}
} //else throw InvalidElementException.removedException(this);
}

Expand Down
Loading

0 comments on commit a22a62e

Please sign in to comment.