From 1b8c4bbb191be87985644caa10f45f04f16c85b4 Mon Sep 17 00:00:00 2001 From: lutovich Date: Sun, 3 Jun 2018 13:01:06 +0200 Subject: [PATCH 1/3] Allow chunks of size 65535 Driver used to only accept chunks of size 32767. This was a limitation on the total chunk size, which includes a 2 byte length header and body. However, Bolt protocol specification allows chunks with body of size 65535 (0xFFFF). This commit fixes the problem by making `ChunkDecoder` accept chunks of size 65535 + 2. --- .../internal/async/inbound/ChunkDecoder.java | 3 ++- .../internal/async/inbound/ChunkDecoderTest.java | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/driver/src/main/java/org/neo4j/driver/internal/async/inbound/ChunkDecoder.java b/driver/src/main/java/org/neo4j/driver/internal/async/inbound/ChunkDecoder.java index 736b989879..f7bc8e96db 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/async/inbound/ChunkDecoder.java +++ b/driver/src/main/java/org/neo4j/driver/internal/async/inbound/ChunkDecoder.java @@ -29,11 +29,12 @@ public class ChunkDecoder extends LengthFieldBasedFrameDecoder { - private static final int MAX_FRAME_LENGTH = Short.MAX_VALUE; + private static final int MAX_FRAME_BODY_LENGTH = 0xFFFF; private static final int LENGTH_FIELD_OFFSET = 0; private static final int LENGTH_FIELD_LENGTH = 2; private static final int LENGTH_ADJUSTMENT = 0; private static final int INITIAL_BYTES_TO_STRIP = LENGTH_FIELD_LENGTH; + private static final int MAX_FRAME_LENGTH = LENGTH_FIELD_LENGTH + MAX_FRAME_BODY_LENGTH; private final Logging logging; private Logger log; diff --git a/driver/src/test/java/org/neo4j/driver/internal/async/inbound/ChunkDecoderTest.java b/driver/src/test/java/org/neo4j/driver/internal/async/inbound/ChunkDecoderTest.java index 3281002b38..e82e80ead0 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/async/inbound/ChunkDecoderTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/async/inbound/ChunkDecoderTest.java @@ -182,6 +182,22 @@ public void shouldLogNonEmptyChunkOnTraceLevel() assertByteBufEquals( wrappedBuffer( bytes ), channel.readInbound() ); } + @Test + public void shouldDecodeMaxSizeChunk() + { + byte[] message = new byte[0xFFFF]; + + ByteBuf input = buffer(); + input.writeShort( message.length ); // chunk header + input.writeBytes( message ); // chunk body + + assertTrue( channel.writeInbound( input ) ); + assertTrue( channel.finish() ); + + assertEquals( 1, channel.inboundMessages().size() ); + assertByteBufEquals( wrappedBuffer( message ), channel.readInbound() ); + } + private static ChunkDecoder newChunkDecoder() { return new ChunkDecoder( DEV_NULL_LOGGING ); From e691bef9db9fd2fec3a554fde8301a0ec28c8afd Mon Sep 17 00:00:00 2001 From: lutovich Date: Sun, 3 Jun 2018 13:10:57 +0200 Subject: [PATCH 2/3] Fix handling of codec exceptions without cause Driver tries to extract cause of every `CodecException` received in `#exceptionCaught(Throwable)` netty callbacks. It did not handle cases when the cause is null, which resulted in an attempt to exceptionally complete a `CompletableFuture` with null. This commit fixes the problem by making handlers extract the exception cause only if it is defined and propagate the original error otherwise. --- .../internal/async/HandshakeHandler.java | 17 ++++++++----- .../async/inbound/ChannelErrorHandler.java | 4 +-- .../async/ChannelErrorHandlerTest.java | 12 +++++++++ .../internal/async/HandshakeHandlerTest.java | 25 +++++++++++++++++++ 4 files changed, 50 insertions(+), 8 deletions(-) diff --git a/driver/src/main/java/org/neo4j/driver/internal/async/HandshakeHandler.java b/driver/src/main/java/org/neo4j/driver/internal/async/HandshakeHandler.java index 9255368444..239d941eb0 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/async/HandshakeHandler.java +++ b/driver/src/main/java/org/neo4j/driver/internal/async/HandshakeHandler.java @@ -165,18 +165,23 @@ private static Throwable protocolNoSupportedByDriverError( int suggestedProtocol private static Throwable transformError( Throwable error ) { - Throwable cause = error instanceof DecoderException ? error.getCause() : error; - if ( cause instanceof ServiceUnavailableException ) + if ( error instanceof DecoderException && error.getCause() != null ) { - return cause; + // unwrap the DecoderException if it has a cause + error = error.getCause(); } - else if ( cause instanceof SSLHandshakeException ) + + if ( error instanceof ServiceUnavailableException ) + { + return error; + } + else if ( error instanceof SSLHandshakeException ) { - return new SecurityException( "Failed to establish secured connection with the server", cause ); + return new SecurityException( "Failed to establish secured connection with the server", error ); } else { - return new ServiceUnavailableException( "Failed to establish connection with the server", cause ); + return new ServiceUnavailableException( "Failed to establish connection with the server", error ); } } } diff --git a/driver/src/main/java/org/neo4j/driver/internal/async/inbound/ChannelErrorHandler.java b/driver/src/main/java/org/neo4j/driver/internal/async/inbound/ChannelErrorHandler.java index bccda6a6a6..662cba33fb 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/async/inbound/ChannelErrorHandler.java +++ b/driver/src/main/java/org/neo4j/driver/internal/async/inbound/ChannelErrorHandler.java @@ -103,9 +103,9 @@ private void fail( ChannelHandlerContext ctx, Throwable error ) private static Throwable transformError( Throwable error ) { - if ( error instanceof CodecException ) + if ( error instanceof CodecException && error.getCause() != null ) { - // unwrap exception from message encoder/decoder + // unwrap the CodecException if it has a cause error = error.getCause(); } diff --git a/driver/src/test/java/org/neo4j/driver/internal/async/ChannelErrorHandlerTest.java b/driver/src/test/java/org/neo4j/driver/internal/async/ChannelErrorHandlerTest.java index ebe471b60d..d9e7d1424f 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/async/ChannelErrorHandlerTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/async/ChannelErrorHandlerTest.java @@ -117,6 +117,18 @@ public void shouldHandleCodecException() assertFalse( channel.isOpen() ); } + @Test + public void shouldHandleCodecExceptionWithoutCause() + { + CodecException codecException = new CodecException( "Unable to encode or decode message" ); + channel.pipeline().fireExceptionCaught( codecException ); + + Throwable error = messageDispatcher.currentError(); + + assertEquals( codecException, error ); + assertFalse( channel.isOpen() ); + } + @Test public void shouldHandleIOException() { diff --git a/driver/src/test/java/org/neo4j/driver/internal/async/HandshakeHandlerTest.java b/driver/src/test/java/org/neo4j/driver/internal/async/HandshakeHandlerTest.java index ccd796350f..42b32d147b 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/async/HandshakeHandlerTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/async/HandshakeHandlerTest.java @@ -187,6 +187,31 @@ public void shouldUnwrapDecoderException() assertNull( await( channel.closeFuture() ) ); } + @Test + public void shouldHandleDecoderExceptionWithoutCause() + { + ChannelPromise handshakeCompletedPromise = channel.newPromise(); + HandshakeHandler handler = newHandler( handshakeCompletedPromise ); + channel.pipeline().addLast( handler ); + + DecoderException decoderException = new DecoderException( "Unable to decode a message" ); + channel.pipeline().fireExceptionCaught( decoderException ); + + try + { + // promise should fail + await( handshakeCompletedPromise ); + fail( "Exception expected" ); + } + catch ( ServiceUnavailableException e ) + { + assertEquals( decoderException, e.getCause() ); + } + + // channel should be closed + assertNull( await( channel.closeFuture() ) ); + } + @Test public void shouldTranslateSSLHandshakeException() { From d7225861e00712c8be7963ec7e95adc5ed029b83 Mon Sep 17 00:00:00 2001 From: lutovich Date: Sun, 3 Jun 2018 13:40:25 +0200 Subject: [PATCH 3/3] Add ITs for long lists, byte arrays and strings --- .../driver/v1/integration/ParametersIT.java | 43 +++++++++++++++++++ .../driver/v1/tck/DriverComplianceSteps.java | 20 ++------- .../org/neo4j/driver/v1/util/TestUtil.java | 13 ++++++ 3 files changed, 60 insertions(+), 16 deletions(-) diff --git a/driver/src/test/java/org/neo4j/driver/v1/integration/ParametersIT.java b/driver/src/test/java/org/neo4j/driver/v1/integration/ParametersIT.java index b980478d92..97ce0ea252 100644 --- a/driver/src/test/java/org/neo4j/driver/v1/integration/ParametersIT.java +++ b/driver/src/test/java/org/neo4j/driver/v1/integration/ParametersIT.java @@ -24,6 +24,7 @@ import java.io.IOException; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.concurrent.ThreadLocalRandom; @@ -35,11 +36,15 @@ import org.neo4j.driver.v1.exceptions.ClientException; import org.neo4j.driver.v1.exceptions.ServiceUnavailableException; import org.neo4j.driver.v1.util.TestNeo4jSession; +import org.neo4j.driver.v1.util.TestUtil; +import static java.util.Collections.singletonMap; +import static java.util.stream.Collectors.toList; import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.fail; import static org.junit.Assume.assumeFalse; import static org.junit.Assume.assumeTrue; @@ -52,6 +57,8 @@ public class ParametersIT { + private static final int LONG_VALUE_SIZE = 1_000_000; + @Rule public TestNeo4jSession session = new TestNeo4jSession(); @@ -451,6 +458,35 @@ public void shouldNotBePossibleToUsePathAsParameterViaMapValue() expectIOExceptionWithMessage( mapValue, "Unknown type: PATH" ); } + @Test + public void shouldSendAndReceiveLongString() + { + String string = TestUtil.randomString( LONG_VALUE_SIZE ); + testSendAndReceiveValue( string ); + } + + @Test + public void shouldSendAndReceiveLongListOfLongs() + { + List longs = ThreadLocalRandom.current() + .longs( LONG_VALUE_SIZE ) + .boxed() + .collect( toList() ); + + testSendAndReceiveValue( longs ); + } + + @Test + public void shouldSendAndReceiveLongArrayOfBytes() + { + assumeTrue( supportsBytes() ); + + byte[] bytes = new byte[LONG_VALUE_SIZE]; + ThreadLocalRandom.current().nextBytes( bytes ); + + testSendAndReceiveValue( bytes ); + } + private void testBytesProperty( byte[] array ) { assumeTrue( supportsBytes() ); @@ -509,4 +545,11 @@ private void expectIOExceptionWithMessage( Value value, String message ) fail( "Expecting a ServiceUnavailableException but got " + e ); } } + + private void testSendAndReceiveValue( Object value ) + { + StatementResult result = session.run( "RETURN $value", singletonMap( "value", value ) ); + Object receivedValue = result.single().get( 0 ).asObject(); + assertArrayEquals( new Object[]{value}, new Object[]{receivedValue} ); + } } diff --git a/driver/src/test/java/org/neo4j/driver/v1/tck/DriverComplianceSteps.java b/driver/src/test/java/org/neo4j/driver/v1/tck/DriverComplianceSteps.java index f1f571abd5..bab5cd62ce 100644 --- a/driver/src/test/java/org/neo4j/driver/v1/tck/DriverComplianceSteps.java +++ b/driver/src/test/java/org/neo4j/driver/v1/tck/DriverComplianceSteps.java @@ -29,7 +29,6 @@ import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.Random; import org.neo4j.driver.v1.Record; import org.neo4j.driver.v1.Session; @@ -39,11 +38,12 @@ import org.neo4j.driver.v1.tck.tck.util.runners.CypherStatementRunner; import org.neo4j.driver.v1.tck.tck.util.runners.StatementRunner; import org.neo4j.driver.v1.tck.tck.util.runners.StringRunner; +import org.neo4j.driver.v1.util.TestUtil; import static org.hamcrest.core.IsEqual.equalTo; import static org.junit.Assert.assertThat; -import static org.neo4j.driver.v1.tck.Environment.driver; import static org.neo4j.driver.v1.Values.parameters; +import static org.neo4j.driver.v1.tck.Environment.driver; import static org.neo4j.driver.v1.tck.Environment.expectedBoltValue; import static org.neo4j.driver.v1.tck.Environment.expectedJavaValue; import static org.neo4j.driver.v1.tck.Environment.listOfObjects; @@ -77,9 +77,9 @@ public void a_value( String value ) throws Throwable } @Given( "^a String of size (\\d+)$" ) - public void a_String_of_size( long size ) throws Throwable + public void a_String_of_size( int size ) throws Throwable { - expectedJavaValue = getRandomString( size ); + expectedJavaValue = TestUtil.randomString( size ); expectedBoltValue = Values.value( expectedJavaValue ); } @@ -191,18 +191,6 @@ public void result_should_be_equal_to_a_single_Type_of_Input() throws Throwable } } - public String getRandomString( long size ) - { - StringBuilder stringBuilder = new StringBuilder(); - String alphabet = "abcdefghijklmnopqrstuvwxyz"; - Random random = new Random(); - while ( size-- > 0 ) - { - stringBuilder.append( alphabet.charAt( random.nextInt( alphabet.length() ) ) ); - } - return stringBuilder.toString(); - } - public List getListOfRandomsOfTypes( Type type, long size ) { List list = new ArrayList<>(); diff --git a/driver/src/test/java/org/neo4j/driver/v1/util/TestUtil.java b/driver/src/test/java/org/neo4j/driver/v1/util/TestUtil.java index e8c6a8762c..f8cc2de089 100644 --- a/driver/src/test/java/org/neo4j/driver/v1/util/TestUtil.java +++ b/driver/src/test/java/org/neo4j/driver/v1/util/TestUtil.java @@ -29,6 +29,7 @@ import java.util.concurrent.CompletionStage; import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; +import java.util.concurrent.ThreadLocalRandom; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import java.util.function.BooleanSupplier; @@ -57,6 +58,7 @@ public final class TestUtil { private static final long DEFAULT_WAIT_TIME_MS = MINUTES.toMillis( 1 ); + private static final String ALPHANUMERICS = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz123456789"; private TestUtil() { @@ -257,6 +259,17 @@ public static void awaitCondition( BooleanSupplier condition, long value, TimeUn } } + public static String randomString( int size ) + { + StringBuilder sb = new StringBuilder( size ); + ThreadLocalRandom random = ThreadLocalRandom.current(); + for ( int i = 0; i < size; i++ ) + { + sb.append( ALPHANUMERICS.charAt( random.nextInt( ALPHANUMERICS.length() ) ) ); + } + return sb.toString(); + } + private static void setupSuccessfulPullAll( Connection connection, String statement ) { doAnswer( invocation ->