diff --git a/community/kernel-api/src/main/java/org/neo4j/internal/kernel/api/Write.java b/community/kernel-api/src/main/java/org/neo4j/internal/kernel/api/Write.java index 62cc408ba3f8..4d8e9420d01e 100644 --- a/community/kernel-api/src/main/java/org/neo4j/internal/kernel/api/Write.java +++ b/community/kernel-api/src/main/java/org/neo4j/internal/kernel/api/Write.java @@ -86,7 +86,7 @@ public interface Write * @param propertyKey the property key id * @return The removed value, or Values.NO_VALUE if the node did not have the property before */ - Value nodeRemoveProperty( long node, int propertyKey ); + Value nodeRemoveProperty( long node, int propertyKey ) throws KernelException; /** * Set a property on a relationship diff --git a/community/kernel-api/src/test/java/org/neo4j/internal/kernel/api/NodeWriteTestBase.java b/community/kernel-api/src/test/java/org/neo4j/internal/kernel/api/NodeWriteTestBase.java index bff42c4d1fac..5e8bc44108f5 100644 --- a/community/kernel-api/src/test/java/org/neo4j/internal/kernel/api/NodeWriteTestBase.java +++ b/community/kernel-api/src/test/java/org/neo4j/internal/kernel/api/NodeWriteTestBase.java @@ -39,6 +39,7 @@ import static org.neo4j.values.storable.Values.intValue; import static org.neo4j.values.storable.Values.stringValue; +@SuppressWarnings("Duplicates") public abstract class NodeWriteTestBase extends KernelAPIWriteTestBase { @Rule @@ -54,7 +55,7 @@ public void shouldCreateNode() throws Exception tx.success(); } - try ( org.neo4j.graphdb.Transaction ctx = graphDb.beginTx() ) + try ( org.neo4j.graphdb.Transaction ignore = graphDb.beginTx() ) { assertEquals( node, graphDb.getNodeById( node ).getId() ); } @@ -70,7 +71,7 @@ public void shouldRollbackOnFailure() throws Exception tx.failure(); } - try ( org.neo4j.graphdb.Transaction ctx = graphDb.beginTx() ) + try ( org.neo4j.graphdb.Transaction ignore = graphDb.beginTx() ) { graphDb.getNodeById( node ); fail( "There should be no node" ); @@ -96,7 +97,7 @@ public void shouldRemoveNode() throws Exception tx.dataWrite().nodeDelete( node ); tx.success(); } - try ( org.neo4j.graphdb.Transaction tx = graphDb.beginTx() ) + try ( org.neo4j.graphdb.Transaction ignore = graphDb.beginTx() ) { try { @@ -150,7 +151,7 @@ public void shouldAddLabelNode() throws Exception } // Then - try ( org.neo4j.graphdb.Transaction ctx = graphDb.beginTx() ) + try ( org.neo4j.graphdb.Transaction ignore = graphDb.beginTx() ) { assertThat( graphDb.getNodeById( node ).getLabels(), equalTo( Iterables.iterable( label( labelName ) ) ) ); } @@ -176,7 +177,7 @@ public void shouldAddLabelNodeOnce() throws Exception tx.success(); } - try ( org.neo4j.graphdb.Transaction ctx = graphDb.beginTx() ) + try ( org.neo4j.graphdb.Transaction ignore = graphDb.beginTx() ) { assertThat( graphDb.getNodeById( node ).getLabels(), equalTo( Iterables.iterable( label( labelName ) ) ) ); } @@ -202,7 +203,7 @@ public void shouldRemoveLabel() throws Exception tx.success(); } - try ( org.neo4j.graphdb.Transaction ctx = graphDb.beginTx() ) + try ( org.neo4j.graphdb.Transaction ignore = graphDb.beginTx() ) { assertThat( graphDb.getNodeById( nodeId ).getLabels(), equalTo( Iterables.empty() ) ); } @@ -249,7 +250,7 @@ public void shouldRemoveLabelOnce() throws Exception tx.success(); } - try ( org.neo4j.graphdb.Transaction ctx = graphDb.beginTx() ) + try ( org.neo4j.graphdb.Transaction ignore = graphDb.beginTx() ) { assertThat( graphDb.getNodeById( nodeId ).getLabels(), equalTo( Iterables.empty() ) ); } @@ -276,7 +277,7 @@ public void shouldAddPropertyToNode() throws Exception } // Then - try ( org.neo4j.graphdb.Transaction ctx = graphDb.beginTx() ) + try ( org.neo4j.graphdb.Transaction ignore = graphDb.beginTx() ) { assertThat( graphDb.getNodeById( node ).getProperty( "prop" ), equalTo( "hello" ) ); } @@ -306,12 +307,102 @@ public void shouldUpdatePropertyToNode() throws Exception } // Then - try ( org.neo4j.graphdb.Transaction ctx = graphDb.beginTx() ) + try ( org.neo4j.graphdb.Transaction ignore = graphDb.beginTx() ) { assertThat( graphDb.getNodeById( node ).getProperty( "prop" ), equalTo( "hello" ) ); } } + @Test + public void shouldRemovePropertyFromNode() throws Exception + { + // Given + long node; + String propertyKey = "prop"; + try ( org.neo4j.graphdb.Transaction ctx = graphDb.beginTx() ) + { + Node proxy = graphDb.createNode(); + proxy.setProperty( propertyKey, 42 ); + node = proxy.getId(); + ctx.success(); + } + + // When + try ( Transaction tx = session.beginTransaction() ) + { + int token = session.token().propertyKeyGetOrCreateForName( propertyKey ); + assertThat( tx.dataWrite().nodeRemoveProperty( node, token ), + equalTo( intValue( 42 ) ) ); + tx.success(); + } + + // Then + try ( org.neo4j.graphdb.Transaction ignore = graphDb.beginTx() ) + { + assertFalse( graphDb.getNodeById( node ).hasProperty( "prop" ) ); + } + } + + @Test + public void shouldRemoveNonExistingPropertyFromNode() throws Exception + { + // Given + long node; + String propertyKey = "prop"; + try ( org.neo4j.graphdb.Transaction ctx = graphDb.beginTx() ) + { + node = graphDb.createNode().getId(); + ctx.success(); + } + + // When + try ( Transaction tx = session.beginTransaction() ) + { + int token = session.token().propertyKeyGetOrCreateForName( propertyKey ); + assertThat( tx.dataWrite().nodeRemoveProperty( node, token ), + equalTo( NO_VALUE ) ); + tx.success(); + } + + // Then + try ( org.neo4j.graphdb.Transaction ignore = graphDb.beginTx() ) + { + assertFalse( graphDb.getNodeById( node ).hasProperty( "prop" ) ); + } + } + + @Test + public void shouldRemovePropertyFromNodeTwice() throws Exception + { + // Given + long node; + String propertyKey = "prop"; + try ( org.neo4j.graphdb.Transaction ctx = graphDb.beginTx() ) + { + Node proxy = graphDb.createNode(); + proxy.setProperty( propertyKey, 42 ); + node = proxy.getId(); + ctx.success(); + } + + // When + try ( Transaction tx = session.beginTransaction() ) + { + int token = session.token().propertyKeyGetOrCreateForName( propertyKey ); + assertThat( tx.dataWrite().nodeRemoveProperty( node, token ), + equalTo( intValue( 42 ) ) ); + assertThat( tx.dataWrite().nodeRemoveProperty( node, token ), + equalTo( NO_VALUE ) ); + tx.success(); + } + + // Then + try ( org.neo4j.graphdb.Transaction ignore = graphDb.beginTx() ) + { + assertFalse( graphDb.getNodeById( node ).hasProperty( "prop" ) ); + } + } + @Test public void shouldUpdatePropertyToNodeInTransaction() throws Exception { @@ -335,7 +426,7 @@ public void shouldUpdatePropertyToNodeInTransaction() throws Exception } // Then - try ( org.neo4j.graphdb.Transaction ctx = graphDb.beginTx() ) + try ( org.neo4j.graphdb.Transaction ignore = graphDb.beginTx() ) { assertThat( graphDb.getNodeById( node ).getProperty( "prop" ), equalTo( 1337 ) ); } diff --git a/community/kernel-api/src/test/java/org/neo4j/internal/kernel/api/TransactionStateTestBase.java b/community/kernel-api/src/test/java/org/neo4j/internal/kernel/api/TransactionStateTestBase.java index c01636e6f2d8..dac76956db2c 100644 --- a/community/kernel-api/src/test/java/org/neo4j/internal/kernel/api/TransactionStateTestBase.java +++ b/community/kernel-api/src/test/java/org/neo4j/internal/kernel/api/TransactionStateTestBase.java @@ -35,6 +35,7 @@ import static org.neo4j.values.storable.Values.NO_VALUE; import static org.neo4j.values.storable.Values.stringValue; +@SuppressWarnings( "Duplicates" ) public abstract class TransactionStateTestBase extends KernelAPIWriteTestBase { @Test @@ -197,8 +198,8 @@ public void shouldSeeNewNodePropertyInTransaction() throws Exception nodeId = tx.dataWrite().nodeCreate(); int prop1 = session.token().propertyKeyGetOrCreateForName( propKey1 ); int prop2 = session.token().propertyKeyGetOrCreateForName( propKey2 ); - assertEquals( tx.dataWrite().nodeSetProperty( nodeId, prop1, stringValue( "hello" ) ), NO_VALUE ); - assertEquals( tx.dataWrite().nodeSetProperty( nodeId, prop2, stringValue( "world" ) ), NO_VALUE ); + assertEquals( tx.dataWrite().nodeSetProperty( nodeId, prop1, stringValue( "hello" ) ), NO_VALUE ); + assertEquals( tx.dataWrite().nodeSetProperty( nodeId, prop2, stringValue( "world" ) ), NO_VALUE ); try ( NodeCursor node = cursors.allocateNodeCursor(); PropertyCursor property = cursors.allocatePropertyCursor() ) @@ -240,7 +241,7 @@ public void shouldSeeAddedPropertyFromExistingNodeWithoutPropertiesInTransaction { int propToken = session.token().propertyKeyGetOrCreateForName( propKey ); - assertEquals( tx.dataWrite().nodeSetProperty( nodeId, propToken, stringValue( "hello" ) ), NO_VALUE ); + assertEquals( tx.dataWrite().nodeSetProperty( nodeId, propToken, stringValue( "hello" ) ), NO_VALUE ); try ( NodeCursor node = cursors.allocateNodeCursor(); PropertyCursor property = cursors.allocatePropertyCursor() ) @@ -281,7 +282,7 @@ public void shouldSeeAddedPropertyFromExistingNodeWithPropertiesInTransaction() { nodeId = tx.dataWrite().nodeCreate(); propToken1 = session.token().propertyKeyGetOrCreateForName( propKey1 ); - assertEquals( tx.dataWrite().nodeSetProperty( nodeId, propToken1, stringValue( "hello" ) ), NO_VALUE ); + assertEquals( tx.dataWrite().nodeSetProperty( nodeId, propToken1, stringValue( "hello" ) ), NO_VALUE ); tx.success(); } @@ -290,7 +291,7 @@ public void shouldSeeAddedPropertyFromExistingNodeWithPropertiesInTransaction() { propToken2 = session.token().propertyKeyGetOrCreateForName( propKey2 ); - assertEquals( tx.dataWrite().nodeSetProperty( nodeId, propToken2, stringValue( "world" ) ), NO_VALUE ); + assertEquals( tx.dataWrite().nodeSetProperty( nodeId, propToken2, stringValue( "world" ) ), NO_VALUE ); try ( NodeCursor node = cursors.allocateNodeCursor(); PropertyCursor property = cursors.allocatePropertyCursor() ) @@ -336,14 +337,15 @@ public void shouldSeeUpdatedPropertyFromExistingNodeWithPropertiesInTransaction( { nodeId = tx.dataWrite().nodeCreate(); propToken = session.token().propertyKeyGetOrCreateForName( propKey ); - assertEquals( tx.dataWrite().nodeSetProperty( nodeId, propToken, stringValue( "hello" ) ), NO_VALUE ); + assertEquals( tx.dataWrite().nodeSetProperty( nodeId, propToken, stringValue( "hello" ) ), NO_VALUE ); tx.success(); } // When/Then try ( Transaction tx = session.beginTransaction() ) { - assertEquals( tx.dataWrite().nodeSetProperty( nodeId, propToken, stringValue( "world" ) ), stringValue( "hello" ) ); + assertEquals( tx.dataWrite().nodeSetProperty( nodeId, propToken, stringValue( "world" ) ), + stringValue( "hello" ) ); try ( NodeCursor node = cursors.allocateNodeCursor(); PropertyCursor property = cursors.allocatePropertyCursor() ) { @@ -370,6 +372,92 @@ public void shouldSeeUpdatedPropertyFromExistingNodeWithPropertiesInTransaction( } } + + @Test + public void shouldSeeRemovedPropertyInTransaction() throws Exception + { + // Given + long nodeId; + String propKey = "prop1"; + int propToken; + try ( Transaction tx = session.beginTransaction() ) + { + nodeId = tx.dataWrite().nodeCreate(); + propToken = session.token().propertyKeyGetOrCreateForName( propKey ); + assertEquals( tx.dataWrite().nodeSetProperty( nodeId, propToken, stringValue( "hello" ) ), NO_VALUE ); + tx.success(); + } + + // When/Then + try ( Transaction tx = session.beginTransaction() ) + { + assertEquals( tx.dataWrite().nodeRemoveProperty( nodeId, propToken ), stringValue( "hello" ) ); + try ( NodeCursor node = cursors.allocateNodeCursor(); + PropertyCursor property = cursors.allocatePropertyCursor() ) + { + tx.dataRead().singleNode( nodeId, node ); + assertTrue( "should access node", node.next() ); + + node.properties( property ); + assertFalse( "should not find any properties", property.next() ); + assertFalse( "should only find one node", node.next() ); + } + + tx.success(); + } + + try ( org.neo4j.graphdb.Transaction ignored = graphDb.beginTx() ) + { + assertFalse( + graphDb.getNodeById( nodeId ).hasProperty( propKey ) ); + } + } + + @Test + public void shouldSeeRemovedThenAddedPropertyInTransaction() throws Exception + { + // Given + long nodeId; + String propKey = "prop1"; + int propToken; + try ( Transaction tx = session.beginTransaction() ) + { + nodeId = tx.dataWrite().nodeCreate(); + propToken = session.token().propertyKeyGetOrCreateForName( propKey ); + assertEquals( tx.dataWrite().nodeSetProperty( nodeId, propToken, stringValue( "hello" ) ), NO_VALUE ); + tx.success(); + } + + // When/Then + try ( Transaction tx = session.beginTransaction() ) + { + assertEquals( tx.dataWrite().nodeRemoveProperty( nodeId, propToken ), stringValue( "hello" ) ); + assertEquals( tx.dataWrite().nodeSetProperty( nodeId, propToken, stringValue( "world" ) ), NO_VALUE ); + try ( NodeCursor node = cursors.allocateNodeCursor(); + PropertyCursor property = cursors.allocatePropertyCursor() ) + { + tx.dataRead().singleNode( nodeId, node ); + assertTrue( "should access node", node.next() ); + + node.properties( property ); + assertTrue( property.next() ); + assertEquals( propToken, property.propertyKey() ); + assertEquals( property.propertyValue(), stringValue( "world" ) ); + + assertFalse( "should not find any properties", property.next() ); + assertFalse( "should only find one node", node.next() ); + } + + tx.success(); + } + + try ( org.neo4j.graphdb.Transaction ignored = graphDb.beginTx() ) + { + assertThat( + graphDb.getNodeById( nodeId ).getProperty( propKey ), equalTo( "world" ) ); + } + } + private void assertLabels( LabelSet labels, int... expected ) { assertEquals( expected.length, labels.numberOfLabels() ); diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/Operations.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/Operations.java index 7000c3563138..d5b006857d96 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/Operations.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/Operations.java @@ -430,30 +430,14 @@ public boolean nodeRemoveLabel( long node, int nodeLabel ) throws KernelExceptio public Value nodeSetProperty( long node, int propertyKey, Value value ) throws EntityNotFoundException { assertOpen(); - allStoreHolder.singleNode( node, nodeCursor ); - if ( !nodeCursor.next() ) - { - throw new EntityNotFoundException( EntityType.NODE, node ); - } - - nodeCursor.properties( propertyCursor ); - //Find out if the property had a value - Value existingValue = NO_VALUE; - while( propertyCursor.next() ) - { - if (propertyCursor.propertyKey() == propertyKey) - { - existingValue = propertyCursor.propertyValue(); - break; - } - } + Value existingValue = readProperty( node, propertyKey ); if ( existingValue == NO_VALUE ) { //no existing value, we just add it - ktx.txState().nodeDoAddProperty( node, propertyKey, value ); //TODO autoIndexing.nodes().propertyAdded( ops, nodeId, propertyKeyId, value ); + ktx.txState().nodeDoAddProperty( node, propertyKey, value ); //TODO updater.onPropertyAdd( state, node, propertyKeyId, value ); return NO_VALUE; } @@ -462,8 +446,8 @@ public Value nodeSetProperty( long node, int propertyKey, Value value ) throws E if ( !value.equals( existingValue ) ) { //the value has changed to a new value - ktx.txState().nodeDoChangeProperty( node, propertyKey, existingValue, value ); //TODO autoIndexing.nodes().propertyChanged( ops, nodeId, propertyKeyId, existingValue, value ); + ktx.txState().nodeDoChangeProperty( node, propertyKey, existingValue, value ); //TODO updater.onPropertyChange( state, node, propertyKeyId, existingValue, value ); } return existingValue; @@ -471,10 +455,20 @@ public Value nodeSetProperty( long node, int propertyKey, Value value ) throws E } @Override - public Value nodeRemoveProperty( long node, int propertyKey ) + public Value nodeRemoveProperty( long node, int propertyKey ) throws EntityNotFoundException { assertOpen(); - throw new UnsupportedOperationException(); + Value existingValue = readProperty( node, propertyKey ); + + if ( existingValue != NO_VALUE ) + { + //no existing value,® we just add it + //TODO autoIndexing.nodes().propertyRemoved( ops, nodeId, propertyKeyId ); + ktx.txState().nodeDoRemoveProperty( node, propertyKey, existingValue); + //TODO indexTxStateUpdater.onPropertyRemove( state, node, propertyKeyId, existingValue ); + } + + return existingValue; } @Override @@ -518,4 +512,27 @@ public void nodeExplicitIndexCreateLazily( String indexName, Map assertOpen(); allStoreHolder.getOrCreateNodeIndexConfig( indexName, customConfig ); } + + private Value readProperty(long node, int propertyKey) throws EntityNotFoundException + { + allStoreHolder.singleNode( node, nodeCursor ); + if ( !nodeCursor.next() ) + { + throw new EntityNotFoundException( EntityType.NODE, node ); + } + + nodeCursor.properties( propertyCursor ); + + //Find out if the property had a value + Value existingValue = NO_VALUE; + while( propertyCursor.next() ) + { + if (propertyCursor.propertyKey() == propertyKey) + { + existingValue = propertyCursor.propertyValue(); + break; + } + } + return existingValue; + } }