From 31a2e290ae93804f67c9d8923eb4d1a656ca30ff Mon Sep 17 00:00:00 2001 From: Pontus Melke Date: Tue, 25 Oct 2016 13:31:28 +0200 Subject: [PATCH] Fixes from code review Instead of just keep the latest unpacking failure, keep them all and combine --- .../messaging/BoltRequestMessageReader.java | 21 ++++++----- .../neo4j/bolt/v1/messaging/Neo4jPack.java | 25 +++++++------ .../bolt/v1/runtime/BoltStateMachine.java | 6 ++- .../org/neo4j/bolt/v1/runtime/Neo4jError.java | 28 ++++++++++++++ .../neo4j/bolt/v1/runtime/Neo4jErrorTest.java | 37 +++++++++++++++++++ 5 files changed, 93 insertions(+), 24 deletions(-) diff --git a/community/bolt/src/main/java/org/neo4j/bolt/v1/messaging/BoltRequestMessageReader.java b/community/bolt/src/main/java/org/neo4j/bolt/v1/messaging/BoltRequestMessageReader.java index bcc0205db9ea2..8ad1126996c69 100644 --- a/community/bolt/src/main/java/org/neo4j/bolt/v1/messaging/BoltRequestMessageReader.java +++ b/community/bolt/src/main/java/org/neo4j/bolt/v1/messaging/BoltRequestMessageReader.java @@ -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}. */ @@ -64,7 +64,7 @@ public void read( BoltRequestMessageHandler handler ) t { case INIT: String clientName = unpacker.unpackString(); - Map credentials = unpacker.unpackMap(); + Map credentials = unpacker.unpackMap(); handler.onInit( clientName, credentials ); break; case ACK_FAILURE: @@ -75,14 +75,15 @@ public void read( BoltRequestMessageHandler handler ) t break; case RUN: String statement = unpacker.unpackString(); - Map params = unpacker.unpackMap(); + Map params = unpacker.unpackMap(); Optional error = unpacker.consumeError(); - if (error.isPresent()) + if ( error.isPresent() ) { handler.onExternalError( error.get() ); - } else { + } + else + { handler.onRun( statement, params ); - } break; case DISCARD_ALL: @@ -105,7 +106,7 @@ public void read( BoltRequestMessageHandler 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 ); } } } diff --git a/community/bolt/src/main/java/org/neo4j/bolt/v1/messaging/Neo4jPack.java b/community/bolt/src/main/java/org/neo4j/bolt/v1/messaging/Neo4jPack.java index 83fb5eb8ece8d..7592df98e09aa 100644 --- a/community/bolt/src/main/java/org/neo4j/bolt/v1/messaging/Neo4jPack.java +++ b/community/bolt/src/main/java/org/neo4j/bolt/v1/messaging/Neo4jPack.java @@ -264,7 +264,7 @@ public boolean hasErrors() public static class Unpacker extends PackStream.Unpacker { - private Optional error = Optional.empty(); + private List errors = new ArrayList<>( 2 ); public Unpacker( PackInput input ) { @@ -398,11 +398,12 @@ public Map 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(); @@ -423,7 +424,7 @@ public Map 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; @@ -438,7 +439,7 @@ public Map 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 + "`." )); } } } @@ -447,17 +448,17 @@ public Map unpackMap() throws IOException public Optional 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 diff --git a/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/BoltStateMachine.java b/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/BoltStateMachine.java index 74fe8273ef6ab..a0bec8496b9ab 100644 --- a/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/BoltStateMachine.java +++ b/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/BoltStateMachine.java @@ -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() diff --git a/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/Neo4jError.java b/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/Neo4jError.java index e79946966bb0f..8fcea8f518bab 100644 --- a/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/Neo4jError.java +++ b/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/Neo4jError.java @@ -19,6 +19,7 @@ */ package org.neo4j.bolt.v1.runtime; +import java.util.List; import java.util.UUID; import org.neo4j.graphdb.DatabaseShutdownException; @@ -189,6 +190,33 @@ public static Neo4jError from( Throwable any ) return fromThrowable( any, false ); } + public static Neo4jError combine( List 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 ); diff --git a/community/bolt/src/test/java/org/neo4j/bolt/v1/runtime/Neo4jErrorTest.java b/community/bolt/src/test/java/org/neo4j/bolt/v1/runtime/Neo4jErrorTest.java index ea94b4602b62a..27fec956cfb00 100644 --- a/community/bolt/src/test/java/org/neo4j/bolt/v1/runtime/Neo4jErrorTest.java +++ b/community/bolt/src/test/java/org/neo4j/bolt/v1/runtime/Neo4jErrorTest.java @@ -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; @@ -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) ); + } }