Fix after review

Zhen · Zhen · commit 95be53ae53be · 2017-01-24T17:14:28.000+01:00
diff --git a/driver/src/main/java/org/neo4j/driver/internal/net/ChannelFactory.java b/driver/src/main/java/org/neo4j/driver/internal/net/ChannelFactory.java
@@ -65,7 +65,7 @@ private static void connect( SocketChannel soChannel, BoltServerAddress address,
         }
         catch ( SocketTimeoutException e )
         {
-            throw new ConnectException( "Timeout " + timeoutMillis + "ms expired" + e.getMessage() );
+            throw new ConnectException( "Timeout " + timeoutMillis + "ms expired" + e );
         }
     }
 }
diff --git a/driver/src/main/java/org/neo4j/driver/internal/security/TLSSocketChannel.java b/driver/src/main/java/org/neo4j/driver/internal/security/TLSSocketChannel.java
@@ -33,6 +33,7 @@
 import org.neo4j.driver.v1.exceptions.ClientException;
 import org.neo4j.driver.v1.exceptions.ServiceUnavailableException;
 
+import static java.lang.String.format;
 import static javax.net.ssl.SSLEngineResult.HandshakeStatus.FINISHED;
 import static javax.net.ssl.SSLEngineResult.HandshakeStatus.NOT_HANDSHAKING;
 
@@ -80,8 +81,7 @@ public static TLSSocketChannel create( ByteChannel channel, Logger logger, SSLEn
         }
         catch ( SSLHandshakeException e )
         {
-            throw new ClientException( "Failed to establish secured connection with the server: " + e.getMessage(),
-                    e.getCause() );
+            throw new ClientException( "Failed to establish secured connection with the server: " + e.getMessage(), e );
         }
         return tlsChannel;
     }
@@ -153,12 +153,10 @@ private HandshakeStatus runDelegatedTasks()
      * @param toBuffer the destination where the data read from the socket channel are saved
      * @throws IOException when failed to read from channel
      */
-    void channelRead( ByteBuffer toBuffer ) throws IOException
+    int channelRead( ByteBuffer toBuffer ) throws IOException
     {
-        /**
-         * This is the only place to read from the underlying channel
-         */
-        if ( channel.read( toBuffer ) < 0 )
+        int read = channel.read( toBuffer );
+        if ( read < 0 )
         {
             try
             {
@@ -172,16 +170,18 @@ void channelRead( ByteBuffer toBuffer ) throws IOException
                     "SSL Connection terminated while receiving data. " +
                     "This can happen due to network instabilities, or due to restarts of the database." );
         }
+        return read;
     }
 
     /**
      * Write the data saved in the buffer to the socket channel
      * @param fromBuffer the source where the data written to the socket channel are saved
      * @throws IOException when failed to write to channel
      */
-    void channelWrite( ByteBuffer fromBuffer ) throws IOException
+    int channelWrite( ByteBuffer fromBuffer ) throws IOException
     {
-        if ( channel.write( fromBuffer ) < 0 )
+        int written = channel.write( fromBuffer );
+        if ( written < 0 )
         {
             try
             {
@@ -195,6 +195,7 @@ void channelWrite( ByteBuffer fromBuffer ) throws IOException
                     "SSL Connection terminated while writing data. " +
                     "This can happen due to network instabilities, or due to restarts of the database." );
         }
+        return written;
     }
 
     /**
@@ -259,7 +260,7 @@ private HandshakeStatus unwrap( ByteBuffer buffer ) throws IOException
                 if ( newAppSize > appSize * 2 )
                 {
                     throw new ClientException(
-                            String.format( "Failed ro enlarge application input buffer from %s to %s, as the maximum " +
+                            format( "Failed ro enlarge application input buffer from %s to %s, as the maximum " +
                                            "buffer size allowed is %s. The content in the buffer is: %s\n",
                                     curAppSize, newAppSize, appSize * 2, BytePrinter.hex( plainIn ) ) );
                 }
@@ -311,7 +312,7 @@ private HandshakeStatus unwrap( ByteBuffer buffer ) throws IOException
      * @return The status of the current handshake
      * @throws IOException
      */
-    private HandshakeStatus wrap( ByteBuffer buffer ) throws IOException
+    private HandshakeStatus wrap( ByteBuffer buffer ) throws IOException, ClientException
     {
         HandshakeStatus handshakeStatus = sslEngine.getHandshakeStatus();
         Status status = sslEngine.wrap( buffer, cipherOut ).getStatus();
@@ -345,7 +346,14 @@ private HandshakeStatus wrap( ByteBuffer buffer ) throws IOException
             {
                 // flush as much data as possible
                 cipherOut.flip();
-                channelWrite( cipherOut );
+                if ( channelWrite( cipherOut ) == 0 )
+                {
+                    throw new ClientException( format(
+                                    "Failed to enlarge network buffer from %s to %s. This is either because the " +
+                                    "new size is however less than the old size, or because the application " +
+                                    "buffer size %s is so big that the application data still cannot fit into the " +
+                                    "new network buffer.", curNetSize, netSize, buffer.capacity() ) );
+                }
                 cipherOut.compact();
                 logger.debug( "Network output buffer couldn't be enlarged, flushing data to the channel instead." );
             }
diff --git a/driver/src/test/java/org/neo4j/driver/internal/security/TLSSocketChannelTest.java b/driver/src/test/java/org/neo4j/driver/internal/security/TLSSocketChannelTest.java
@@ -36,7 +36,7 @@
 import static org.mockito.Matchers.any;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 import static org.neo4j.driver.internal.logging.DevNullLogger.DEV_NULL_LOGGER;
@@ -72,7 +72,7 @@ public void shouldCloseConnectionIfFailedToRead() throws Throwable
             assertThat( e.getMessage(), startsWith( "SSL Connection terminated while receiving data. " ) );
         }
         // Then
-        verify( mockedChannel, times( 1 ) ).close();
+        verify( mockedChannel ).close();
     }
 
     @Test
@@ -103,7 +103,7 @@ public void shouldCloseConnectionIfFailedToWrite() throws Throwable
         }
 
         // Then
-        verify( mockedChannel, times( 1 ) ).close();
+        verify( mockedChannel ).close();
     }
 
     @Test
@@ -131,6 +131,6 @@ public void shouldThrowClientErrorIfFailedToHandshake() throws Throwable
             assertThat( e, instanceOf( ClientException.class ) );
             assertThat( e.getMessage(), startsWith( "Failed to establish secured connection with the server: Failed handshake!" ) );
         }
-        verify( mockedChannel, times( 0 ) ).close();
+        verify( mockedChannel, never() ).close();
     }
 }
diff --git a/driver/src/test/java/org/neo4j/driver/v1/integration/TLSSocketChannelIT.java b/driver/src/test/java/org/neo4j/driver/v1/integration/TLSSocketChannelIT.java
@@ -53,7 +53,6 @@
 import static org.hamcrest.Matchers.containsString;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
 import static org.mockito.Matchers.anyString;
 import static org.mockito.Mockito.atLeastOnce;
 import static org.mockito.Mockito.mock;
@@ -160,8 +159,7 @@ public void shouldNotPerformTLSHandshakeWithNonSystemCert() throws Throwable
             catch ( ClientException e )
             {
                 assertThat( e.getMessage(), containsString( "General SSLEngine problem" ) );
-                assertEquals( "General SSLEngine problem", e.getCause().getMessage() );
-                assertThat( e.getCause().getCause().getMessage(),
+                assertThat( getRootCause( e ).getMessage(),
                         containsString( "unable to find valid certification path to requested target" ) );
             }
         }
@@ -196,8 +194,7 @@ public void shouldFailTLSHandshakeDueToWrongCertInKnownCertsFile() throws Throwa
         catch ( SSLHandshakeException e )
         {
             assertEquals( "General SSLEngine problem", e.getMessage() );
-            assertEquals( "General SSLEngine problem", e.getCause().getMessage() );
-            assertTrue( e.getCause().getCause().getMessage().contains(
+            assertThat( getRootCause( e ).getMessage(), containsString(
                     "If you trust the certificate the server uses now, simply remove the line that starts with" ) );
         }
         finally
@@ -249,8 +246,7 @@ public void shouldFailTLSHandshakeDueToServerCertNotSignedByKnownCA() throws Thr
         catch ( ClientException e )
         {
             assertThat( e.getMessage(), containsString( "General SSLEngine problem" ) );
-            assertEquals( "General SSLEngine problem", e.getCause().getMessage() );
-            assertEquals( "No trusted certificate found", e.getCause().getCause().getMessage() );
+            assertThat( getRootCause( e ).getMessage(), containsString( "No trusted certificate found" ) );
         }
         finally
         {
@@ -261,6 +257,20 @@ public void shouldFailTLSHandshakeDueToServerCertNotSignedByKnownCA() throws Thr
         }
     }
 
+    private Throwable getRootCause( Throwable e )
+    {
+        Throwable parentError = e;
+        Throwable error = null;
+        do
+        {
+            error = parentError;
+            parentError = error.getCause();
+
+        }
+        while( parentError != null );
+        return error;
+    }
+
     @Test
     public void shouldPerformTLSHandshakeWithTheSameTrustedServerCert() throws Throwable
     {

Original file line number	Diff line number	Diff line change
`@@ -65,7 +65,7 @@ private static void connect( SocketChannel soChannel, BoltServerAddress address,`
`65`	`65`	`}`
`66`	`66`	`catch ( SocketTimeoutException e )`
`67`	`67`	`{`
`68`		`- throw new ConnectException( "Timeout " + timeoutMillis + "ms expired" + e.getMessage() );`
	`68`	`+ throw new ConnectException( "Timeout " + timeoutMillis + "ms expired" + e );`
`69`	`69`	`}`
`70`	`70`	`}`
`71`	`71`	`}`
Original file line number	Diff line number	Diff line change
`@@ -33,6 +33,7 @@`
`33`	`33`	`import org.neo4j.driver.v1.exceptions.ClientException;`
`34`	`34`	`import org.neo4j.driver.v1.exceptions.ServiceUnavailableException;`
`35`	`35`
	`36`	`+import static java.lang.String.format;`
`36`	`37`	`import static javax.net.ssl.SSLEngineResult.HandshakeStatus.FINISHED;`
`37`	`38`	`import static javax.net.ssl.SSLEngineResult.HandshakeStatus.NOT_HANDSHAKING;`
`38`	`39`
`@@ -80,8 +81,7 @@ public static TLSSocketChannel create( ByteChannel channel, Logger logger, SSLEn`
`80`	`81`	`}`
`81`	`82`	`catch ( SSLHandshakeException e )`
`82`	`83`	`{`
`83`		`- throw new ClientException( "Failed to establish secured connection with the server: " + e.getMessage(),`
`84`		`- e.getCause() );`
	`84`	`+ throw new ClientException( "Failed to establish secured connection with the server: " + e.getMessage(), e );`
`85`	`85`	`}`
`86`	`86`	`return tlsChannel;`
`87`	`87`	`}`
`@@ -153,12 +153,10 @@ private HandshakeStatus runDelegatedTasks()`
`153`	`153`	`* @param toBuffer the destination where the data read from the socket channel are saved`
`154`	`154`	`* @throws IOException when failed to read from channel`
`155`	`155`	`*/`
`156`		`- void channelRead( ByteBuffer toBuffer ) throws IOException`
	`156`	`+ int channelRead( ByteBuffer toBuffer ) throws IOException`
`157`	`157`	`{`
`158`		`- /**`
`159`		`- * This is the only place to read from the underlying channel`
`160`		`- */`
`161`		`- if ( channel.read( toBuffer ) < 0 )`
	`158`	`+ int read = channel.read( toBuffer );`
	`159`	`+ if ( read < 0 )`
`162`	`160`	`{`
`163`	`161`	`try`
`164`	`162`	`{`
`@@ -172,16 +170,18 @@ void channelRead( ByteBuffer toBuffer ) throws IOException`
`172`	`170`	`"SSL Connection terminated while receiving data. " +`
`173`	`171`	`"This can happen due to network instabilities, or due to restarts of the database." );`
`174`	`172`	`}`
	`173`	`+ return read;`
`175`	`174`	`}`
`176`	`175`
`177`	`176`	`/**`
`178`	`177`	`* Write the data saved in the buffer to the socket channel`
`179`	`178`	`* @param fromBuffer the source where the data written to the socket channel are saved`
`180`	`179`	`* @throws IOException when failed to write to channel`
`181`	`180`	`*/`
`182`		`- void channelWrite( ByteBuffer fromBuffer ) throws IOException`
	`181`	`+ int channelWrite( ByteBuffer fromBuffer ) throws IOException`
`183`	`182`	`{`
`184`		`- if ( channel.write( fromBuffer ) < 0 )`
	`183`	`+ int written = channel.write( fromBuffer );`
	`184`	`+ if ( written < 0 )`
`185`	`185`	`{`
`186`	`186`	`try`
`187`	`187`	`{`
`@@ -195,6 +195,7 @@ void channelWrite( ByteBuffer fromBuffer ) throws IOException`
`195`	`195`	`"SSL Connection terminated while writing data. " +`
`196`	`196`	`"This can happen due to network instabilities, or due to restarts of the database." );`
`197`	`197`	`}`
	`198`	`+ return written;`
`198`	`199`	`}`
`199`	`200`
`200`	`201`	`/**`
`@@ -259,7 +260,7 @@ private HandshakeStatus unwrap( ByteBuffer buffer ) throws IOException`
`259`	`260`	`if ( newAppSize > appSize * 2 )`
`260`	`261`	`{`
`261`	`262`	`throw new ClientException(`
`262`		`- String.format( "Failed ro enlarge application input buffer from %s to %s, as the maximum " +`
	`263`	`+ format( "Failed ro enlarge application input buffer from %s to %s, as the maximum " +`
`263`	`264`	`"buffer size allowed is %s. The content in the buffer is: %s\n",`
`264`	`265`	`curAppSize, newAppSize, appSize * 2, BytePrinter.hex( plainIn ) ) );`
`265`	`266`	`}`
`@@ -311,7 +312,7 @@ private HandshakeStatus unwrap( ByteBuffer buffer ) throws IOException`
`311`	`312`	`* @return The status of the current handshake`
`312`	`313`	`* @throws IOException`
`313`	`314`	`*/`
`314`		`- private HandshakeStatus wrap( ByteBuffer buffer ) throws IOException`
	`315`	`+ private HandshakeStatus wrap( ByteBuffer buffer ) throws IOException, ClientException`
`315`	`316`	`{`
`316`	`317`	`HandshakeStatus handshakeStatus = sslEngine.getHandshakeStatus();`
`317`	`318`	`Status status = sslEngine.wrap( buffer, cipherOut ).getStatus();`
`@@ -345,7 +346,14 @@ private HandshakeStatus wrap( ByteBuffer buffer ) throws IOException`
`345`	`346`	`{`
`346`	`347`	`// flush as much data as possible`
`347`	`348`	`cipherOut.flip();`
`348`		`- channelWrite( cipherOut );`
	`349`	`+ if ( channelWrite( cipherOut ) == 0 )`
	`350`	`+ {`
	`351`	`+ throw new ClientException( format(`
	`352`	`+ "Failed to enlarge network buffer from %s to %s. This is either because the " +`
	`353`	`+ "new size is however less than the old size, or because the application " +`
	`354`	`+ "buffer size %s is so big that the application data still cannot fit into the " +`
	`355`	`+ "new network buffer.", curNetSize, netSize, buffer.capacity() ) );`
	`356`	`+ }`
`349`	`357`	`cipherOut.compact();`
`350`	`358`	`logger.debug( "Network output buffer couldn't be enlarged, flushing data to the channel instead." );`
`351`	`359`	`}`
Original file line number	Diff line number	Diff line change
`@@ -36,7 +36,7 @@`
`36`	`36`	`import static org.mockito.Matchers.any;`
`37`	`37`	`import static org.mockito.Mockito.doThrow;`
`38`	`38`	`import static org.mockito.Mockito.mock;`
`39`		`-import static org.mockito.Mockito.times;`
	`39`	`+import static org.mockito.Mockito.never;`
`40`	`40`	`import static org.mockito.Mockito.verify;`
`41`	`41`	`import static org.mockito.Mockito.when;`
`42`	`42`	`import static org.neo4j.driver.internal.logging.DevNullLogger.DEV_NULL_LOGGER;`
`@@ -72,7 +72,7 @@ public void shouldCloseConnectionIfFailedToRead() throws Throwable`
`72`	`72`	`assertThat( e.getMessage(), startsWith( "SSL Connection terminated while receiving data. " ) );`
`73`	`73`	`}`
`74`	`74`	`// Then`
`75`		`- verify( mockedChannel, times( 1 ) ).close();`
	`75`	`+ verify( mockedChannel ).close();`
`76`	`76`	`}`
`77`	`77`
`78`	`78`	`@Test`
`@@ -103,7 +103,7 @@ public void shouldCloseConnectionIfFailedToWrite() throws Throwable`
`103`	`103`	`}`
`104`	`104`
`105`	`105`	`// Then`
`106`		`- verify( mockedChannel, times( 1 ) ).close();`
	`106`	`+ verify( mockedChannel ).close();`
`107`	`107`	`}`
`108`	`108`
`109`	`109`	`@Test`
`@@ -131,6 +131,6 @@ public void shouldThrowClientErrorIfFailedToHandshake() throws Throwable`
`131`	`131`	`assertThat( e, instanceOf( ClientException.class ) );`
`132`	`132`	`assertThat( e.getMessage(), startsWith( "Failed to establish secured connection with the server: Failed handshake!" ) );`
`133`	`133`	`}`
`134`		`- verify( mockedChannel, times( 0 ) ).close();`
	`134`	`+ verify( mockedChannel, never() ).close();`
`135`	`135`	`}`
`136`	`136`	`}`
Original file line number	Diff line number	Diff line change
`@@ -53,7 +53,6 @@`
`53`	`53`	`import static org.hamcrest.Matchers.containsString;`
`54`	`54`	`import static org.junit.Assert.assertEquals;`
`55`	`55`	`import static org.junit.Assert.assertFalse;`
`56`		`-import static org.junit.Assert.assertTrue;`
`57`	`56`	`import static org.mockito.Matchers.anyString;`
`58`	`57`	`import static org.mockito.Mockito.atLeastOnce;`
`59`	`58`	`import static org.mockito.Mockito.mock;`
`@@ -160,8 +159,7 @@ public void shouldNotPerformTLSHandshakeWithNonSystemCert() throws Throwable`
`160`	`159`	`catch ( ClientException e )`
`161`	`160`	`{`
`162`	`161`	`assertThat( e.getMessage(), containsString( "General SSLEngine problem" ) );`
`163`		`- assertEquals( "General SSLEngine problem", e.getCause().getMessage() );`
`164`		`- assertThat( e.getCause().getCause().getMessage(),`
	`162`	`+ assertThat( getRootCause( e ).getMessage(),`
`165`	`163`	`containsString( "unable to find valid certification path to requested target" ) );`
`166`	`164`	`}`
`167`	`165`	`}`
`@@ -196,8 +194,7 @@ public void shouldFailTLSHandshakeDueToWrongCertInKnownCertsFile() throws Throwa`
`196`	`194`	`catch ( SSLHandshakeException e )`
`197`	`195`	`{`
`198`	`196`	`assertEquals( "General SSLEngine problem", e.getMessage() );`
`199`		`- assertEquals( "General SSLEngine problem", e.getCause().getMessage() );`
`200`		`- assertTrue( e.getCause().getCause().getMessage().contains(`
	`197`	`+ assertThat( getRootCause( e ).getMessage(), containsString(`
`201`	`198`	`"If you trust the certificate the server uses now, simply remove the line that starts with" ) );`
`202`	`199`	`}`
`203`	`200`	`finally`
`@@ -249,8 +246,7 @@ public void shouldFailTLSHandshakeDueToServerCertNotSignedByKnownCA() throws Thr`
`249`	`246`	`catch ( ClientException e )`
`250`	`247`	`{`
`251`	`248`	`assertThat( e.getMessage(), containsString( "General SSLEngine problem" ) );`
`252`		`- assertEquals( "General SSLEngine problem", e.getCause().getMessage() );`
`253`		`- assertEquals( "No trusted certificate found", e.getCause().getCause().getMessage() );`
	`249`	`+ assertThat( getRootCause( e ).getMessage(), containsString( "No trusted certificate found" ) );`
`254`	`250`	`}`
`255`	`251`	`finally`
`256`	`252`	`{`
`@@ -261,6 +257,20 @@ public void shouldFailTLSHandshakeDueToServerCertNotSignedByKnownCA() throws Thr`
`261`	`257`	`}`
`262`	`258`	`}`
`263`	`259`
	`260`	`+ private Throwable getRootCause( Throwable e )`
	`261`	`+ {`
	`262`	`+ Throwable parentError = e;`
	`263`	`+ Throwable error = null;`
	`264`	`+ do`
	`265`	`+ {`
	`266`	`+ error = parentError;`
	`267`	`+ parentError = error.getCause();`
	`268`	`+`
	`269`	`+ }`
	`270`	`+ while( parentError != null );`
	`271`	`+ return error;`
	`272`	`+ }`
	`273`	`+`
`264`	`274`	`@Test`
`265`	`275`	`public void shouldPerformTLSHandshakeWithTheSameTrustedServerCert() throws Throwable`
`266`	`276`	`{`