Skip to content

Commit

Permalink
Fixes from code review
Browse files Browse the repository at this point in the history
  • Loading branch information
pontusmelke committed Dec 8, 2017
1 parent 46f4a4c commit 5d4c9a7
Show file tree
Hide file tree
Showing 10 changed files with 86 additions and 139 deletions.
Expand Up @@ -31,16 +31,18 @@ import org.neo4j.cypher.internal.v3_4.expressions.SemanticDirection
import org.neo4j.graphdb._ import org.neo4j.graphdb._
import org.neo4j.graphdb.config.Setting import org.neo4j.graphdb.config.Setting
import org.neo4j.graphdb.factory.GraphDatabaseSettings import org.neo4j.graphdb.factory.GraphDatabaseSettings
import org.neo4j.internal.kernel.api.NodeCursor
import org.neo4j.internal.kernel.api.Transaction.Type import org.neo4j.internal.kernel.api.Transaction.Type
import org.neo4j.internal.kernel.api.security.SecurityContext import org.neo4j.internal.kernel.api.security.SecurityContext
import org.neo4j.internal.kernel.api.security.SecurityContext.AUTH_DISABLED import org.neo4j.internal.kernel.api.security.SecurityContext.AUTH_DISABLED
import org.neo4j.io.pagecache.tracing.cursor.PageCursorTracerSupplier import org.neo4j.io.pagecache.tracing.cursor.PageCursorTracerSupplier
import org.neo4j.kernel.api.KernelTransaction
import org.neo4j.kernel.api.security.AnonymousContext import org.neo4j.kernel.api.security.AnonymousContext
import org.neo4j.kernel.impl.api.{KernelStatement, KernelTransactionImplementation, StatementOperationParts} import org.neo4j.kernel.impl.api.{KernelStatement, KernelTransactionImplementation, StatementOperationParts}
import org.neo4j.kernel.impl.core.ThreadToStatementContextBridge
import org.neo4j.kernel.impl.coreapi.{InternalTransaction, PropertyContainerLocker} import org.neo4j.kernel.impl.coreapi.{InternalTransaction, PropertyContainerLocker}
import org.neo4j.kernel.impl.factory.CanWrite import org.neo4j.kernel.impl.factory.CanWrite
import org.neo4j.kernel.impl.locking.LockTracer import org.neo4j.kernel.impl.locking.LockTracer
import org.neo4j.kernel.impl.newapi.Cursors
import org.neo4j.kernel.impl.proc.Procedures import org.neo4j.kernel.impl.proc.Procedures
import org.neo4j.kernel.impl.query.clientconnection.ClientConnectionInfo import org.neo4j.kernel.impl.query.clientconnection.ClientConnectionInfo
import org.neo4j.kernel.impl.query.{Neo4jTransactionalContext, Neo4jTransactionalContextFactory} import org.neo4j.kernel.impl.query.{Neo4jTransactionalContext, Neo4jTransactionalContextFactory}
Expand Down Expand Up @@ -81,7 +83,12 @@ class TransactionBoundQueryContextTest extends CypherFunSuite {
when(outerTx.failure()).thenThrow(new AssertionError("Shouldn't be called")) when(outerTx.failure()).thenThrow(new AssertionError("Shouldn't be called"))
when(outerTx.transactionType()).thenReturn(Type.`implicit`) when(outerTx.transactionType()).thenReturn(Type.`implicit`)
when(outerTx.securityContext()).thenReturn(AUTH_DISABLED) when(outerTx.securityContext()).thenReturn(AUTH_DISABLED)
val tc = new Neo4jTransactionalContext(graph, null, null, null, locker, outerTx, statement, null)
val bridge = mock[ThreadToStatementContextBridge]
val transaction = mock[KernelTransaction]
when(transaction.cursors()).thenReturn(new Cursors())
when(bridge.getKernelTransactionBoundToThisThread(true)).thenReturn(transaction)
val tc = new Neo4jTransactionalContext(graph, null, null, bridge, locker, outerTx, statement, null)
val transactionalContext = TransactionalContextWrapper(tc) val transactionalContext = TransactionalContextWrapper(tc)
val context = new TransactionBoundQueryContext(transactionalContext)(indexSearchMonitor) val context = new TransactionBoundQueryContext(transactionalContext)(indexSearchMonitor)
// WHEN // WHEN
Expand All @@ -100,7 +107,11 @@ class TransactionBoundQueryContextTest extends CypherFunSuite {
when(outerTx.success()).thenThrow(new AssertionError("Shouldn't be called")) when(outerTx.success()).thenThrow(new AssertionError("Shouldn't be called"))
when(outerTx.transactionType()).thenReturn(Type.`implicit`) when(outerTx.transactionType()).thenReturn(Type.`implicit`)
when(outerTx.securityContext()).thenReturn(AUTH_DISABLED) when(outerTx.securityContext()).thenReturn(AUTH_DISABLED)
val tc = new Neo4jTransactionalContext(graph, null, null, null, locker, outerTx, statement, null) val bridge = mock[ThreadToStatementContextBridge]
val transaction = mock[KernelTransaction]
when(transaction.cursors()).thenReturn(new Cursors())
when(bridge.getKernelTransactionBoundToThisThread(true)).thenReturn(transaction)
val tc = new Neo4jTransactionalContext(graph, null, null, bridge, locker, outerTx, statement, null)
val transactionalContext = TransactionalContextWrapper(tc) val transactionalContext = TransactionalContextWrapper(tc)
val context = new TransactionBoundQueryContext(transactionalContext)(indexSearchMonitor) val context = new TransactionBoundQueryContext(transactionalContext)(indexSearchMonitor)
// WHEN // WHEN
Expand Down Expand Up @@ -199,7 +210,11 @@ class TransactionBoundQueryContextTest extends CypherFunSuite {


test("should close all resources when closing resources") { test("should close all resources when closing resources") {
// GIVEN // GIVEN
val tc = new Neo4jTransactionalContext(graph, null, null, null, locker, outerTx, statement, null) val bridge = mock[ThreadToStatementContextBridge]
val transaction = mock[KernelTransaction]
when(transaction.cursors()).thenReturn(new Cursors())
when(bridge.getKernelTransactionBoundToThisThread(true)).thenReturn(transaction)
val tc = new Neo4jTransactionalContext(graph, null, null, bridge, locker, outerTx, statement, null)
val transactionalContext = TransactionalContextWrapper(tc) val transactionalContext = TransactionalContextWrapper(tc)
val context = new TransactionBoundQueryContext(transactionalContext)(indexSearchMonitor) val context = new TransactionBoundQueryContext(transactionalContext)(indexSearchMonitor)
val resource1 = mock[AutoCloseable] val resource1 = mock[AutoCloseable]
Expand All @@ -225,14 +240,13 @@ class TransactionBoundQueryContextTest extends CypherFunSuite {
val tx = graph.beginTransaction(Type.explicit, AnonymousContext.read()) val tx = graph.beginTransaction(Type.explicit, AnonymousContext.read())
val transactionalContext = TransactionalContextWrapper(createTransactionContext(graph, tx)) val transactionalContext = TransactionalContextWrapper(createTransactionContext(graph, tx))
val context = new TransactionBoundQueryContext(transactionalContext)(indexSearchMonitor) val context = new TransactionBoundQueryContext(transactionalContext)(indexSearchMonitor)
val initSize = context.resources.allResources.size


// WHEN // WHEN
context.resources.allResources shouldBe empty
context.nodeOps.all context.nodeOps.all


// THEN // THEN
context.resources.allResources should have size 1 context.resources.allResources should have size initSize + 1
context.resources.allResources.head shouldBe a [NodeCursor]
tx.close() tx.close()
} }


Expand All @@ -243,14 +257,13 @@ class TransactionBoundQueryContextTest extends CypherFunSuite {
val tx = graph.beginTransaction(Type.explicit, AnonymousContext.read()) val tx = graph.beginTransaction(Type.explicit, AnonymousContext.read())
val transactionalContext = TransactionalContextWrapper(createTransactionContext(graph, tx)) val transactionalContext = TransactionalContextWrapper(createTransactionContext(graph, tx))
val context = new TransactionBoundQueryContext(transactionalContext)(indexSearchMonitor) val context = new TransactionBoundQueryContext(transactionalContext)(indexSearchMonitor)
val initSize = context.resources.allResources.size


// WHEN // WHEN
context.resources.allResources shouldBe empty
context.nodeOps.allPrimitive context.nodeOps.allPrimitive


// THEN // THEN
context.resources.allResources should have size 1 context.resources.allResources should have size initSize + 1
context.resources.allResources.head shouldBe a [NodeCursor]
tx.close() tx.close()
} }


Expand All @@ -261,11 +274,11 @@ class TransactionBoundQueryContextTest extends CypherFunSuite {
val tx = graph.beginTransaction(Type.explicit, AnonymousContext.read()) val tx = graph.beginTransaction(Type.explicit, AnonymousContext.read())
val transactionalContext = TransactionalContextWrapper(createTransactionContext(graph, tx)) val transactionalContext = TransactionalContextWrapper(createTransactionContext(graph, tx))
val context = new TransactionBoundQueryContext(transactionalContext)(indexSearchMonitor) val context = new TransactionBoundQueryContext(transactionalContext)(indexSearchMonitor)
val initSize = context.resources.allResources.size


// WHEN // WHEN
context.resources.allResources shouldBe empty
context.nodeOps.all context.nodeOps.all
context.resources.allResources should have size 1 context.resources.allResources should have size initSize + 1
context.resources.close(success = true) context.resources.close(success = true)


// THEN // THEN
Expand Down
Expand Up @@ -21,12 +21,10 @@


import java.util.Optional; import java.util.Optional;


import org.neo4j.internal.kernel.api.NodeCursor;
import org.neo4j.internal.kernel.api.PropertyCursor;
import org.neo4j.internal.kernel.api.Transaction; import org.neo4j.internal.kernel.api.Transaction;
import org.neo4j.internal.kernel.api.security.SecurityContext;
import org.neo4j.kernel.api.exceptions.Status; import org.neo4j.kernel.api.exceptions.Status;
import org.neo4j.kernel.api.exceptions.TransactionFailureException; import org.neo4j.kernel.api.exceptions.TransactionFailureException;
import org.neo4j.internal.kernel.api.security.SecurityContext;
import org.neo4j.kernel.impl.api.Kernel; import org.neo4j.kernel.impl.api.Kernel;


/** /**
Expand Down Expand Up @@ -98,10 +96,6 @@ interface CloseListener
*/ */
Statement acquireStatement(); Statement acquireStatement();


NodeCursor nodeCursor();

PropertyCursor propertyCursor();

/** /**
* Closes this transaction, committing its changes if {@link #success()} has been called and neither * Closes this transaction, committing its changes if {@link #success()} has been called and neither
* {@link #failure()} nor {@link #markForTermination(Status)} has been called. * {@link #failure()} nor {@link #markForTermination(Status)} has been called.
Expand Down
Expand Up @@ -37,12 +37,9 @@
import org.neo4j.internal.kernel.api.CursorFactory; import org.neo4j.internal.kernel.api.CursorFactory;
import org.neo4j.internal.kernel.api.ExplicitIndexRead; import org.neo4j.internal.kernel.api.ExplicitIndexRead;
import org.neo4j.internal.kernel.api.ExplicitIndexWrite; import org.neo4j.internal.kernel.api.ExplicitIndexWrite;
import org.neo4j.internal.kernel.api.NodeCursor;
import org.neo4j.internal.kernel.api.PropertyCursor;
import org.neo4j.internal.kernel.api.Read; import org.neo4j.internal.kernel.api.Read;
import org.neo4j.internal.kernel.api.SchemaRead; import org.neo4j.internal.kernel.api.SchemaRead;
import org.neo4j.internal.kernel.api.SchemaWrite; import org.neo4j.internal.kernel.api.SchemaWrite;
import org.neo4j.internal.kernel.api.Token;
import org.neo4j.internal.kernel.api.TokenRead; import org.neo4j.internal.kernel.api.TokenRead;
import org.neo4j.internal.kernel.api.TokenWrite; import org.neo4j.internal.kernel.api.TokenWrite;
import org.neo4j.internal.kernel.api.Write; import org.neo4j.internal.kernel.api.Write;
Expand Down Expand Up @@ -369,18 +366,6 @@ public KernelStatement acquireStatement()
return currentStatement; return currentStatement;
} }


@Override
public NodeCursor nodeCursor()
{
return operations.nodeCursor();
}

@Override
public PropertyCursor propertyCursor()
{
return operations.propertyCursor();
}

ExecutingQueryList executingQueries() ExecutingQueryList executingQueries()
{ {
return currentStatement.executingQueryList(); return currentStatement.executingQueryList();
Expand Down
Expand Up @@ -26,7 +26,6 @@
import java.util.List; import java.util.List;
import java.util.Map; import java.util.Map;
import java.util.Objects; import java.util.Objects;
import java.util.Optional;


import org.neo4j.collection.primitive.PrimitiveIntIterator; import org.neo4j.collection.primitive.PrimitiveIntIterator;
import org.neo4j.graphdb.ConstraintViolationException; import org.neo4j.graphdb.ConstraintViolationException;
Expand All @@ -40,9 +39,10 @@
import org.neo4j.graphdb.ResourceIterable; import org.neo4j.graphdb.ResourceIterable;
import org.neo4j.graphdb.ResourceIterator; import org.neo4j.graphdb.ResourceIterator;
import org.neo4j.graphdb.TransactionTerminatedException; import org.neo4j.graphdb.TransactionTerminatedException;
import org.neo4j.internal.kernel.api.CursorFactory;
import org.neo4j.internal.kernel.api.LabelSet;
import org.neo4j.internal.kernel.api.NodeCursor; import org.neo4j.internal.kernel.api.NodeCursor;
import org.neo4j.internal.kernel.api.PropertyCursor; import org.neo4j.internal.kernel.api.PropertyCursor;
import org.neo4j.internal.kernel.api.Token;
import org.neo4j.internal.kernel.api.TokenRead; import org.neo4j.internal.kernel.api.TokenRead;
import org.neo4j.internal.kernel.api.exceptions.InvalidTransactionTypeKernelException; import org.neo4j.internal.kernel.api.exceptions.InvalidTransactionTypeKernelException;
import org.neo4j.internal.kernel.api.exceptions.LabelNotFoundKernelException; import org.neo4j.internal.kernel.api.exceptions.LabelNotFoundKernelException;
Expand Down Expand Up @@ -350,15 +350,16 @@ public Object getProperty( String key, Object defaultValue )
throw new IllegalArgumentException( "(null) property key is not allowed" ); throw new IllegalArgumentException( "(null) property key is not allowed" );
} }
KernelTransaction transaction = safeAcquireTransaction(); KernelTransaction transaction = safeAcquireTransaction();
try ( Statement ignore = transaction.acquireStatement() ) CursorFactory cursors = transaction.cursors();
try ( Statement ignore = transaction.acquireStatement();
NodeCursor nodes = cursors.allocateNodeCursor();
PropertyCursor properties = cursors.allocatePropertyCursor() )
{ {
int propertyKey = transaction.tokenRead().propertyKey( key ); int propertyKey = transaction.tokenRead().propertyKey( key );
if ( propertyKey == KeyReadOperations.NO_SUCH_PROPERTY_KEY ) if ( propertyKey == KeyReadOperations.NO_SUCH_PROPERTY_KEY )
{ {
return defaultValue; return defaultValue;
} }
NodeCursor nodes = transaction.nodeCursor();
PropertyCursor properties = transaction.propertyCursor();
transaction.dataRead().singleNode( nodeId, nodes ); transaction.dataRead().singleNode( nodeId, nodes );
if ( !nodes.next() ) if ( !nodes.next() )
{ {
Expand All @@ -382,10 +383,11 @@ public Iterable<String> getPropertyKeys()
{ {
KernelTransaction transaction = safeAcquireTransaction(); KernelTransaction transaction = safeAcquireTransaction();
List<String> keys = new ArrayList<>(); List<String> keys = new ArrayList<>();
try ( Statement ignore = transaction.acquireStatement() ) CursorFactory cursors = transaction.cursors();
try ( Statement ignore = transaction.acquireStatement();
NodeCursor nodes = cursors.allocateNodeCursor();
PropertyCursor properties = cursors.allocatePropertyCursor() )
{ {
NodeCursor nodes = transaction.nodeCursor();
PropertyCursor properties = transaction.propertyCursor();
transaction.dataRead().singleNode( nodeId, nodes ); transaction.dataRead().singleNode( nodeId, nodes );
TokenRead token = transaction.tokenRead(); TokenRead token = transaction.tokenRead();
if ( !nodes.next() ) if ( !nodes.next() )
Expand Down Expand Up @@ -435,28 +437,31 @@ public Map<String,Object> getProperties( String... keys )
propertyIds[i] = token.propertyKey( key ); propertyIds[i] = token.propertyKey( key );
} }


NodeCursor nodes = transaction.nodeCursor(); CursorFactory cursors = transaction.cursors();
PropertyCursor propertyCursor = transaction.propertyCursor(); try ( NodeCursor nodes = cursors.allocateNodeCursor();
transaction.dataRead().singleNode( nodeId, nodes ); PropertyCursor propertyCursor = cursors.allocatePropertyCursor() )
if ( !nodes.next() )
{ {
throw new NotFoundException( new EntityNotFoundException( EntityType.NODE, nodeId ) ); transaction.dataRead().singleNode( nodeId, nodes );
} if ( !nodes.next() )
nodes.properties( propertyCursor ); {
int propertiesToFind = itemsToReturn; throw new NotFoundException( new EntityNotFoundException( EntityType.NODE, nodeId ) );
while ( propertiesToFind > 0 && propertyCursor.next() ) }
{ nodes.properties( propertyCursor );
//Do a linear check if this is a property we are interested in. int propertiesToFind = itemsToReturn;
for ( int i = 0; i < itemsToReturn; i++ ) while ( propertiesToFind > 0 && propertyCursor.next() )
{ {
int propertyId = propertyIds[i]; //Do a linear check if this is a property we are interested in.
int currentKey = propertyCursor.propertyKey(); for ( int i = 0; i < itemsToReturn; i++ )
if ( propertyId == currentKey )
{ {
properties.put( keys[i], int propertyId = propertyIds[i];
propertyCursor.propertyValue().asObjectCopy() ); int currentKey = propertyCursor.propertyKey();
propertiesToFind--; if ( propertyId == currentKey )
break; {
properties.put( keys[i],
propertyCursor.propertyValue().asObjectCopy() );
propertiesToFind--;
break;
}
} }
} }
} }
Expand All @@ -470,11 +475,12 @@ public Map<String,Object> getAllProperties()
KernelTransaction transaction = safeAcquireTransaction(); KernelTransaction transaction = safeAcquireTransaction();
Map<String,Object> properties = new HashMap<>(); Map<String,Object> properties = new HashMap<>();


try ( Statement ignore = transaction.acquireStatement() ) CursorFactory cursors = transaction.cursors();
try ( Statement ignore = transaction.acquireStatement();
NodeCursor nodes = cursors.allocateNodeCursor();
PropertyCursor propertyCursor = cursors.allocatePropertyCursor() )
{ {
TokenRead token = transaction.tokenRead(); TokenRead token = transaction.tokenRead();
PropertyCursor propertyCursor = transaction.propertyCursor();
NodeCursor nodes = transaction.nodeCursor();
transaction.dataRead().singleNode( nodeId, nodes ); transaction.dataRead().singleNode( nodeId, nodes );
if ( !nodes.next() ) if ( !nodes.next() )
{ {
Expand Down Expand Up @@ -507,9 +513,11 @@ public Object getProperty( String key ) throws NotFoundException
{ {
throw new NotFoundException( format( "No such property, '%s'.", key ) ); throw new NotFoundException( format( "No such property, '%s'.", key ) );
} }
NodeCursor nodes = transaction.nodeCursor();
PropertyCursor properties = transaction.propertyCursor(); CursorFactory cursors = transaction.cursors();
try ( Statement ignore = transaction.acquireStatement() ) try ( Statement ignore = transaction.acquireStatement();
NodeCursor nodes = cursors.allocateNodeCursor();
PropertyCursor properties = cursors.allocatePropertyCursor() )
{ {
transaction.dataRead().singleNode( nodeId, nodes ); transaction.dataRead().singleNode( nodeId, nodes );
if ( !nodes.next() ) if ( !nodes.next() )
Expand All @@ -519,7 +527,7 @@ public Object getProperty( String key ) throws NotFoundException
nodes.properties( properties ); nodes.properties( properties );
while ( properties.next() ) while ( properties.next() )
{ {
if ( propertyKey == properties.propertyKey() ) if ( propertyKey == properties.propertyKey() )
{ {
Value value = properties.propertyValue(); Value value = properties.propertyValue();
if ( value == Values.NO_VALUE ) if ( value == Values.NO_VALUE )
Expand All @@ -542,16 +550,16 @@ public boolean hasProperty( String key )
} }


KernelTransaction transaction = safeAcquireTransaction(); KernelTransaction transaction = safeAcquireTransaction();

int propertyKey = transaction.tokenRead().propertyKey( key );
try ( Statement ignore = transaction.acquireStatement() ) if ( propertyKey == KeyReadOperations.NO_SUCH_PROPERTY_KEY )
{
return false;
}
CursorFactory cursors = transaction.cursors();
try ( Statement ignore = transaction.acquireStatement();
NodeCursor nodes = cursors.allocateNodeCursor();
PropertyCursor properties = cursors.allocatePropertyCursor() )
{ {
int propertyKey = transaction.tokenRead().propertyKey( key );
if ( propertyKey == KeyReadOperations.NO_SUCH_PROPERTY_KEY )
{
return false;
}
NodeCursor nodes = transaction.nodeCursor();
PropertyCursor properties = transaction.propertyCursor();
transaction.dataRead().singleNode( nodeId, nodes ); transaction.dataRead().singleNode( nodeId, nodes );
if ( !nodes.next() ) if ( !nodes.next() )
{ {
Expand All @@ -572,10 +580,10 @@ public boolean hasProperty( String key )
private KernelTransaction safeAcquireTransaction() private KernelTransaction safeAcquireTransaction()
{ {
KernelTransaction transaction = actions.kernelTransaction(); KernelTransaction transaction = actions.kernelTransaction();
Optional<Status> terminationReason = transaction.getReasonIfTerminated(); if ( transaction.isTerminated() )
if ( terminationReason.isPresent() )
{ {
throw new TransactionTerminatedException( terminationReason.get() ); Status terminationReason = transaction.getReasonIfTerminated().orElse( Status.Transaction.Terminated );
throw new TransactionTerminatedException( terminationReason );
} }
return transaction; return transaction;
} }
Expand Down Expand Up @@ -740,6 +748,7 @@ private Label convertToLabel( Statement statement, int labelId )
try try
{ {
return label( statement.readOperations().labelGetName( labelId ) ); return label( statement.readOperations().labelGetName( labelId ) );

} }
catch ( LabelNotFoundKernelException e ) catch ( LabelNotFoundKernelException e )
{ {
Expand Down
Expand Up @@ -23,7 +23,6 @@
import java.net.URL; import java.net.URL;
import java.util.Collections; import java.util.Collections;
import java.util.Map; import java.util.Map;
import java.util.Optional;
import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeUnit;
import java.util.function.Supplier; import java.util.function.Supplier;


Expand Down Expand Up @@ -819,10 +818,10 @@ private void assertTransactionOpen()


private void assertTransactionOpen( KernelTransaction transaction ) private void assertTransactionOpen( KernelTransaction transaction )
{ {
Optional<Status> terminationReason = transaction.getReasonIfTerminated(); if ( transaction.isTerminated() )
if ( terminationReason.isPresent() )
{ {
throw new TransactionTerminatedException( terminationReason.get() ); Status terminationReason = transaction.getReasonIfTerminated().orElse( Status.Transaction.Terminated );
throw new TransactionTerminatedException( terminationReason );
} }
} }
} }
Expand Up @@ -94,6 +94,8 @@ public AllStoreHolder( StorageEngine engine,
@Override @Override
public boolean nodeExists( long id ) public boolean nodeExists( long id )
{ {
assertOpen.assertOpen();

if ( hasTxStateWithChanges() ) if ( hasTxStateWithChanges() )
{ {
TransactionState txState = txState(); TransactionState txState = txState();
Expand Down

0 comments on commit 5d4c9a7

Please sign in to comment.