Skip to content

Commit

Permalink
Fixes from code review
Browse files Browse the repository at this point in the history 
Instead of just keep the latest unpacking failure, keep them all
and combine
  • Loading branch information
@pontusmelke
pontusmelke committed Oct 25, 2016
1 parent e7c1e7a commit 31a2e29
Show file tree
Hide file tree
Showing 5 changed files with 93 additions and 24 deletions.
21 changes: 11 additions & 10 deletions community/bolt/src/main/java/org/neo4j/bolt/v1/messaging/BoltRequestMessageReader.java
View file
Expand Up @@ -20,14 +20,14 @@

package org.neo4j.bolt.v1.messaging;

import org.neo4j.bolt.v1.packstream.PackStream;
import org.neo4j.bolt.v1.runtime.Neo4jError;
import org.neo4j.kernel.api.exceptions.Status;

import java.io.IOException;
import java.util.Map;
import java.util.Optional;

import org.neo4j.bolt.v1.packstream.PackStream;
import org.neo4j.bolt.v1.runtime.Neo4jError;
import org.neo4j.kernel.api.exceptions.Status;

/**
* Reader for Bolt request messages made available via a {@link Neo4jPack.Unpacker}.
*/
Expand Down Expand Up @@ -64,7 +64,7 @@ public <E extends Exception> void read( BoltRequestMessageHandler<E> handler ) t
{
case INIT:
String clientName = unpacker.unpackString();
Map<String, Object> credentials = unpacker.unpackMap();
Map<String,Object> credentials = unpacker.unpackMap();
handler.onInit( clientName, credentials );
break;
case ACK_FAILURE:
Expand All @@ -75,14 +75,15 @@ public <E extends Exception> void read( BoltRequestMessageHandler<E> handler ) t
break;
case RUN:
String statement = unpacker.unpackString();
Map<String, Object> params = unpacker.unpackMap();
Map<String,Object> params = unpacker.unpackMap();
Optional<Neo4jError> error = unpacker.consumeError();
if (error.isPresent())
if ( error.isPresent() )
{
handler.onExternalError( error.get() );
} else {
}
else
{
handler.onRun( statement, params );

}
break;
case DISCARD_ALL:
Expand All @@ -105,7 +106,7 @@ public <E extends Exception> void read( BoltRequestMessageHandler<E> handler ) t
catch ( PackStream.PackStreamException e )
{
throw new BoltIOException( Status.Request.InvalidFormat, "Unable to read message type. " +
"Error was: " + e.getMessage(), e );
"Error was: " + e.getMessage(), e );
}
}
}
25 changes: 13 additions & 12 deletions community/bolt/src/main/java/org/neo4j/bolt/v1/messaging/Neo4jPack.java
View file
Expand Up @@ -264,7 +264,7 @@ public boolean hasErrors()
public static class Unpacker extends PackStream.Unpacker
{

private Optional<Neo4jError> error = Optional.empty();
private List<Neo4jError> errors = new ArrayList<>( 2 );

public Unpacker( PackInput input )
{
Expand Down Expand Up @@ -398,11 +398,12 @@ public Map<String, Object> unpackMap() throws IOException
val = unpack();
if( map.put( key, val ) != null )
{
error = Optional.of( Neo4jError.from( Status.Request.Invalid, "Duplicate map key `" + key + "`." ));
errors.add(
Neo4jError.from( Status.Request.Invalid, "Duplicate map key `" + key + "`." ) );
}
break;
case NULL:
error = Optional.of( Neo4jError.from( Status.Request.Invalid,
errors.add( Neo4jError.from( Status.Request.Invalid,
"Value `null` is not supported as key in maps, must be a non-nullable string." ) );
unpackNull();
val = unpack();
Expand All @@ -423,7 +424,7 @@ public Map<String, Object> unpackMap() throws IOException
switch ( type )
{
case NULL:
error = Optional.of( Neo4jError.from( Status.Request.Invalid,
errors.add( Neo4jError.from( Status.Request.Invalid,
"Value `null` is not supported as key in maps, must be a non-nullable string." ) );
unpackNull();
key = null;
Expand All @@ -438,7 +439,7 @@ public Map<String, Object> unpackMap() throws IOException
Object val = unpack();
if( map.put( key, val ) != null )
{
error = Optional.of( Neo4jError.from( Status.Request.Invalid, "Duplicate map key `" + key + "`." ));
errors.add( Neo4jError.from( Status.Request.Invalid, "Duplicate map key `" + key + "`." ));
}
}
}
Expand All @@ -447,17 +448,17 @@ public Map<String, Object> unpackMap() throws IOException

public Optional<Neo4jError> consumeError()
{
if (error.isPresent())
{
Neo4jError e = error.get();
error = Optional.empty();
return Optional.of( e );
} else
if ( errors.isEmpty() )
{
return Optional.empty();
}
else
{
Neo4jError combined = Neo4jError.combine( errors );
errors.clear();
return Optional.of( combined );
}
}

}

private static class Error
Expand Down
6 changes: 4 additions & 2 deletions community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/BoltStateMachine.java
View file
Expand Up @@ -270,8 +270,10 @@ public void externalError( Neo4jError error, BoltResponseHandler handler ) throw
fail( this, error );
this.state = State.FAILED;
}
finally { after(); }

finally
{
after();
}
}

public boolean isClosed()
Expand Down
28 changes: 28 additions & 0 deletions community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/Neo4jError.java
View file
Expand Up @@ -19,6 +19,7 @@
*/
package org.neo4j.bolt.v1.runtime;

import java.util.List;
import java.util.UUID;

import org.neo4j.graphdb.DatabaseShutdownException;
Expand Down Expand Up @@ -189,6 +190,33 @@ public static Neo4jError from( Throwable any )
return fromThrowable( any, false );
}

public static Neo4jError combine( List<Neo4jError> errors )
{
assert errors.size() >= 1;

if (errors.size() == 1)
{
return errors.get( 0 );
}
else
{
Neo4jError first = errors.get( 0 );
Status combinedStatus = first.status;
StringBuilder combinedMessage = new StringBuilder( String.format("The following errors has occurred:%n%n" ));
combinedMessage.append( first.message );
for (int i = 1; i < errors.size(); i++)
{
Neo4jError error = errors.get( i );
combinedStatus = error.status == combinedStatus ? error.status : Status.General.UnknownError;
combinedMessage
.append( System.lineSeparator() )
.append( error.message );
}

return from(combinedStatus, combinedMessage.toString());
}
}

public static Neo4jError fatalFrom( Throwable any )
{
return fromThrowable( any, true );
Expand Down
37 changes: 37 additions & 0 deletions community/bolt/src/test/java/org/neo4j/bolt/v1/runtime/Neo4jErrorTest.java
View file
Expand Up @@ -26,6 +26,7 @@
import org.neo4j.kernel.DeadlockDetectedException;
import org.neo4j.kernel.api.exceptions.Status;

import static java.util.Arrays.asList;
import static junit.framework.TestCase.assertEquals;
import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.is;
Expand Down Expand Up @@ -77,4 +78,40 @@ public void shouldSetStatusToDatabaseUnavailableOnDatabaseShutdownException()
assertThat( error.status(), equalTo( Status.General.DatabaseUnavailable ) );
assertThat( error.cause(), equalTo( ex ) );
}

@Test
public void shouldCombineErrors()
{
// Given
Neo4jError error1 = Neo4jError.from( new DeadlockDetectedException( "In my life" ) );
Neo4jError error2 = Neo4jError.from( new DeadlockDetectedException( "Why do I give valuable time" ) );
Neo4jError error3 = Neo4jError.from( new DeadlockDetectedException( "To people who don't care if I live or die?" ) );

// When
Neo4jError combine = Neo4jError.combine( asList( error1, error2, error3 ) );

// Then
assertThat( combine.status(), equalTo( Status.Transaction.DeadlockDetected ) );
assertThat( combine.message(), equalTo( String.format(
"The following errors has occurred:%n%n" +
"In my life%n" +
"Why do I give valuable time%n" +
"To people who don't care if I live or die?"
)));
}

@Test
public void shouldBeUnknownIfCombiningDifferentStatus()
{
// Given
Neo4jError error1 = Neo4jError.from( Status.General.DatabaseUnavailable, "foo" );
Neo4jError error2 = Neo4jError.from( Status.Request.Invalid, "bar");
Neo4jError error3 = Neo4jError.from( Status.Schema.ConstraintAlreadyExists, "baz");

// When
Neo4jError combine = Neo4jError.combine( asList( error1, error2, error3 ) );

// Then
assertThat( combine.status(), equalTo( Status.General.UnknownError) );
}
}

0 comments on commit 31a2e29

Please sign in to comment.