Skip to content

Commit

Permalink
Fixes from code review
Browse files Browse the repository at this point in the history
  • Loading branch information
@pontusmelke
pontusmelke committed Feb 23, 2018
1 parent 261c65d commit 219b97f
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 34 deletions.
87 changes: 55 additions & 32 deletions ...api/src/test/java/org/neo4j/internal/kernel/api/RelationshipTransactionStateTestBase.java
View file
Expand Up @@ -33,6 +33,7 @@
import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue; import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import static org.neo4j.graphdb.Direction.BOTH; import static org.neo4j.graphdb.Direction.BOTH;
import static org.neo4j.graphdb.Direction.INCOMING; import static org.neo4j.graphdb.Direction.INCOMING;
import static org.neo4j.graphdb.Direction.OUTGOING; import static org.neo4j.graphdb.Direction.OUTGOING;
Expand All @@ -44,7 +45,8 @@
import static org.neo4j.values.storable.Values.stringValue; import static org.neo4j.values.storable.Values.stringValue;


@SuppressWarnings( "Duplicates" ) @SuppressWarnings( "Duplicates" )
public abstract class RelationshipTransactionStateTestBase<G extends KernelAPIWriteTestSupport> extends KernelAPIWriteTestBase<G> public abstract class RelationshipTransactionStateTestBase<G extends KernelAPIWriteTestSupport>
extends KernelAPIWriteTestBase<G>
{ {
@Test @Test
public void shouldSeeSingleRelationshipInTransaction() throws Exception public void shouldSeeSingleRelationshipInTransaction() throws Exception
Expand Down Expand Up @@ -253,7 +255,7 @@ public void shouldSeeRelationshipInTransactionBeforeCursorInitialization() throw
int label = tx.tokenWrite().relationshipTypeGetOrCreateForName( "R" ); int label = tx.tokenWrite().relationshipTypeGetOrCreateForName( "R" );
long r = tx.dataWrite().relationshipCreate( n1, label, n2 ); long r = tx.dataWrite().relationshipCreate( n1, label, n2 );
try ( NodeCursor node = cursors.allocateNodeCursor(); try ( NodeCursor node = cursors.allocateNodeCursor();
RelationshipTraversalCursor relationship = cursors.allocateRelationshipTraversalCursor() ) RelationshipTraversalCursor relationship = cursors.allocateRelationshipTraversalCursor() )
{ {
tx.dataRead().singleNode( n1, node ); tx.dataRead().singleNode( n1, node );
assertTrue( "should access node", node.next() ); assertTrue( "should access node", node.next() );
Expand Down Expand Up @@ -344,23 +346,29 @@ public void shouldSeeNewRelationshipPropertyInTransaction() throws Exception
assertTrue( "should access relationship", relationship.next() ); assertTrue( "should access relationship", relationship.next() );


relationship.properties( property ); relationship.properties( property );
assertTrue( property.next() );
//First property
assertEquals( prop1, property.propertyKey() );
assertEquals( property.propertyValue(), stringValue( "hello" ) );
//second property
assertTrue( property.next() );
assertEquals( prop2, property.propertyKey() );
assertEquals( property.propertyValue(), stringValue( "world" ) );


assertFalse( "should only find two properties", property.next() ); while ( property.next() )
{
if ( property.propertyKey() == prop1 )
{
assertEquals( property.propertyValue(), stringValue( "hello" ) );
}
else if ( property.propertyKey() == prop2 )
{
assertEquals( property.propertyValue(), stringValue( "world" ) );
}
else
{
fail( property.propertyKey() + " was not the property key you were looking for" );
}
}

assertFalse( "should only find one relationship", relationship.next() ); assertFalse( "should only find one relationship", relationship.next() );
} }
tx.success(); tx.success();
} }
} }


//start here
@Test @Test
public void shouldSeeAddedPropertyFromExistingRelationshipWithoutPropertiesInTransaction() throws Exception public void shouldSeeAddedPropertyFromExistingRelationshipWithoutPropertiesInTransaction() throws Exception
{ {
Expand All @@ -379,7 +387,8 @@ public void shouldSeeAddedPropertyFromExistingRelationshipWithoutPropertiesInTra
try ( Transaction tx = session.beginTransaction() ) try ( Transaction tx = session.beginTransaction() )
{ {
int propToken = session.token().propertyKeyGetOrCreateForName( propKey ); int propToken = session.token().propertyKeyGetOrCreateForName( propKey );
assertEquals( tx.dataWrite().relationshipSetProperty( relationshipId, propToken, stringValue( "hello" ) ), NO_VALUE ); assertEquals( tx.dataWrite().relationshipSetProperty( relationshipId, propToken, stringValue( "hello" ) ),
NO_VALUE );


try ( RelationshipScanCursor relationship = cursors.allocateRelationshipScanCursor(); try ( RelationshipScanCursor relationship = cursors.allocateRelationshipScanCursor();
PropertyCursor property = cursors.allocatePropertyCursor() ) PropertyCursor property = cursors.allocatePropertyCursor() )
Expand Down Expand Up @@ -421,15 +430,17 @@ public void shouldSeeAddedPropertyFromExistingRelationshipWithPropertiesInTransa
relationshipId = write.relationshipCreate( write.nodeCreate(), relationshipId = write.relationshipCreate( write.nodeCreate(),
tx.tokenWrite().relationshipTypeGetOrCreateForName( "R" ), write.nodeCreate() ); tx.tokenWrite().relationshipTypeGetOrCreateForName( "R" ), write.nodeCreate() );
propToken1 = session.token().propertyKeyGetOrCreateForName( propKey1 ); propToken1 = session.token().propertyKeyGetOrCreateForName( propKey1 );
assertEquals( write.relationshipSetProperty( relationshipId, propToken1, stringValue( "hello" ) ), NO_VALUE ); assertEquals( write.relationshipSetProperty( relationshipId, propToken1, stringValue( "hello" ) ),
NO_VALUE );
tx.success(); tx.success();
} }


// When/Then // When/Then
try ( Transaction tx = session.beginTransaction() ) try ( Transaction tx = session.beginTransaction() )
{ {
propToken2 = session.token().propertyKeyGetOrCreateForName( propKey2 ); propToken2 = session.token().propertyKeyGetOrCreateForName( propKey2 );
assertEquals( tx.dataWrite().relationshipSetProperty( relationshipId, propToken2, stringValue( "world" ) ), NO_VALUE ); assertEquals( tx.dataWrite().relationshipSetProperty( relationshipId, propToken2, stringValue( "world" ) ),
NO_VALUE );


try ( RelationshipScanCursor relationship = cursors.allocateRelationshipScanCursor(); try ( RelationshipScanCursor relationship = cursors.allocateRelationshipScanCursor();
PropertyCursor property = cursors.allocatePropertyCursor() ) PropertyCursor property = cursors.allocatePropertyCursor() )
Expand All @@ -439,17 +450,23 @@ public void shouldSeeAddedPropertyFromExistingRelationshipWithPropertiesInTransa


relationship.properties( property ); relationship.properties( property );


//property 2, start with tx state while ( property.next() )
assertTrue( property.next() ); {
assertEquals( propToken2, property.propertyKey() ); if ( property.propertyKey() == propToken1 )//from disk
assertEquals( property.propertyValue(), stringValue( "world" ) ); {
assertEquals( property.propertyValue(), stringValue( "hello" ) );


//property 1, from disk }
assertTrue( property.next() ); else if ( property.propertyKey() == propToken2 )//from tx state
assertEquals( propToken1, property.propertyKey() ); {
assertEquals( property.propertyValue(), stringValue( "hello" ) ); assertEquals( property.propertyValue(), stringValue( "world" ) );
}
else
{
fail( property.propertyKey() + " was not the property you were looking for" );
}
}


assertFalse( "should only find two properties", property.next() );
assertFalse( "should only find one relationship", relationship.next() ); assertFalse( "should only find one relationship", relationship.next() );
} }
tx.success(); tx.success();
Expand All @@ -476,7 +493,8 @@ public void shouldSeeUpdatedPropertyFromExistingRelationshipWithPropertiesInTran
relationshipId = write.relationshipCreate( write.nodeCreate(), relationshipId = write.relationshipCreate( write.nodeCreate(),
tx.tokenWrite().relationshipTypeGetOrCreateForName( "R" ), write.nodeCreate() ); tx.tokenWrite().relationshipTypeGetOrCreateForName( "R" ), write.nodeCreate() );
propToken = session.token().propertyKeyGetOrCreateForName( propKey ); propToken = session.token().propertyKeyGetOrCreateForName( propKey );
assertEquals( write.relationshipSetProperty( relationshipId, propToken, stringValue( "hello" ) ), NO_VALUE ); assertEquals( write.relationshipSetProperty( relationshipId, propToken, stringValue( "hello" ) ),
NO_VALUE );
tx.success(); tx.success();
} }


Expand Down Expand Up @@ -512,7 +530,7 @@ public void shouldSeeUpdatedPropertyFromExistingRelationshipWithPropertiesInTran
} }


@Test @Test
public void shouldSeeRemovedPropertyInTransaction() throws Exception public void shouldNotSeeRemovedPropertyInTransaction() throws Exception
{ {
// Given // Given
long relationshipId; long relationshipId;
Expand All @@ -524,14 +542,16 @@ public void shouldSeeRemovedPropertyInTransaction() throws Exception
relationshipId = write.relationshipCreate( write.nodeCreate(), relationshipId = write.relationshipCreate( write.nodeCreate(),
tx.tokenWrite().relationshipTypeGetOrCreateForName( "R" ), write.nodeCreate() ); tx.tokenWrite().relationshipTypeGetOrCreateForName( "R" ), write.nodeCreate() );
propToken = session.token().propertyKeyGetOrCreateForName( propKey ); propToken = session.token().propertyKeyGetOrCreateForName( propKey );
assertEquals( write.relationshipSetProperty( relationshipId, propToken, stringValue( "hello" ) ), NO_VALUE ); assertEquals( write.relationshipSetProperty( relationshipId, propToken, stringValue( "hello" ) ),
NO_VALUE );
tx.success(); tx.success();
} }


// When/Then // When/Then
try ( Transaction tx = session.beginTransaction() ) try ( Transaction tx = session.beginTransaction() )
{ {
assertEquals( tx.dataWrite().relationshipRemoveProperty( relationshipId, propToken ), stringValue( "hello" ) ); assertEquals( tx.dataWrite().relationshipRemoveProperty( relationshipId, propToken ),
stringValue( "hello" ) );
try ( RelationshipScanCursor relationship = cursors.allocateRelationshipScanCursor(); try ( RelationshipScanCursor relationship = cursors.allocateRelationshipScanCursor();
PropertyCursor property = cursors.allocatePropertyCursor() ) PropertyCursor property = cursors.allocatePropertyCursor() )
{ {
Expand Down Expand Up @@ -566,15 +586,18 @@ public void shouldSeeRemovedThenAddedPropertyInTransaction() throws Exception
relationshipId = write.relationshipCreate( write.nodeCreate(), relationshipId = write.relationshipCreate( write.nodeCreate(),
tx.tokenWrite().relationshipTypeGetOrCreateForName( "R" ), write.nodeCreate() ); tx.tokenWrite().relationshipTypeGetOrCreateForName( "R" ), write.nodeCreate() );
propToken = session.token().propertyKeyGetOrCreateForName( propKey ); propToken = session.token().propertyKeyGetOrCreateForName( propKey );
assertEquals( write.relationshipSetProperty( relationshipId, propToken, stringValue( "hello" ) ), NO_VALUE ); assertEquals( write.relationshipSetProperty( relationshipId, propToken, stringValue( "hello" ) ),
NO_VALUE );
tx.success(); tx.success();
} }


// When/Then // When/Then
try ( Transaction tx = session.beginTransaction() ) try ( Transaction tx = session.beginTransaction() )
{ {
assertEquals( tx.dataWrite().relationshipRemoveProperty( relationshipId, propToken ), stringValue( "hello" ) ); assertEquals( tx.dataWrite().relationshipRemoveProperty( relationshipId, propToken ),
assertEquals( tx.dataWrite().relationshipSetProperty( relationshipId, propToken, stringValue( "world" ) ), NO_VALUE ); stringValue( "hello" ) );
assertEquals( tx.dataWrite().relationshipSetProperty( relationshipId, propToken, stringValue( "world" ) ),
NO_VALUE );
try ( RelationshipScanCursor relationship = cursors.allocateRelationshipScanCursor(); try ( RelationshipScanCursor relationship = cursors.allocateRelationshipScanCursor();
PropertyCursor property = cursors.allocatePropertyCursor() ) PropertyCursor property = cursors.allocatePropertyCursor() )
{ {
Expand Down Expand Up @@ -702,7 +725,7 @@ private void traverseViaGroups( RelationshipTestSupport.StartNode start, boolean
private Map<String,Integer> modifyStartNodeRelationships( RelationshipTestSupport.StartNode start, Transaction tx ) private Map<String,Integer> modifyStartNodeRelationships( RelationshipTestSupport.StartNode start, Transaction tx )
throws KernelException throws KernelException
{ {
Map<String, Integer> expectedCounts = new HashMap<>(); Map<String,Integer> expectedCounts = new HashMap<>();
for ( Map.Entry<String,List<RelationshipTestSupport.StartRelationship>> kv : start.relationships.entrySet() ) for ( Map.Entry<String,List<RelationshipTestSupport.StartRelationship>> kv : start.relationships.entrySet() )
{ {
List<RelationshipTestSupport.StartRelationship> rs = kv.getValue(); List<RelationshipTestSupport.StartRelationship> rs = kv.getValue();
Expand Down
2 changes: 0 additions & 2 deletions community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/Operations.java
View file
Expand Up @@ -439,7 +439,6 @@ public Value nodeRemoveProperty( long node, int propertyKey )


if ( existingValue != NO_VALUE ) if ( existingValue != NO_VALUE )
{ {
//no existing value, we just add it
autoIndexing.nodes().propertyRemoved( this, node, propertyKey); autoIndexing.nodes().propertyRemoved( this, node, propertyKey);
ktx.txState().nodeDoRemoveProperty( node, propertyKey, existingValue); ktx.txState().nodeDoRemoveProperty( node, propertyKey, existingValue);
updater.onPropertyRemove( nodeCursor, propertyCursor, propertyKey, existingValue ); updater.onPropertyRemove( nodeCursor, propertyCursor, propertyKey, existingValue );
Expand Down Expand Up @@ -486,7 +485,6 @@ public Value relationshipRemoveProperty( long relationship, int propertyKey )


if ( existingValue != NO_VALUE ) if ( existingValue != NO_VALUE )
{ {
//no existing value, we just add it
autoIndexing.relationships().propertyRemoved( this, relationship, propertyKey); autoIndexing.relationships().propertyRemoved( this, relationship, propertyKey);
ktx.txState().relationshipDoRemoveProperty( relationship, propertyKey, existingValue); ktx.txState().relationshipDoRemoveProperty( relationship, propertyKey, existingValue);
} }
Expand Down

0 comments on commit 219b97f

Please sign in to comment.