Merge pull request #521 from lutovich/1.7-bolt-protocol-commit-cleanup

Simplify `BoltProtocol#commitTransaction()`

Author: zhenlineo

driver/src/main/java/org/neo4j/driver/internal/ExplicitTransaction.java

@@ -248,7 +248,12 @@ private CompletionStage<Void> doCommitAsync()
             return failedFuture( new ClientException( "Transaction can't be committed. " +
                                                       "It has been rolled back either because of an error or explicit termination" ) );
         }
-        return protocol.commitTransaction( connection, this );
+        return protocol.commitTransaction( connection )
+                .thenApply( newBookmarks ->
+                {
+                    setBookmarks( newBookmarks );
+                    return null;
+                } );
     }
 
     private CompletionStage<Void> doRollbackAsync()
@@ -260,7 +265,7 @@ private CompletionStage<Void> doRollbackAsync()
         return protocol.rollbackTransaction( connection );
     }
 
-    private BiFunction<Void,Throwable,Void> handleCommitOrRollback( Throwable cursorFailure )
+    private static BiFunction<Void,Throwable,Void> handleCommitOrRollback( Throwable cursorFailure )
     {
         return ( ignore, commitOrRollbackError ) ->
         {

driver/src/main/java/org/neo4j/driver/internal/handlers/CommitTxResponseHandler.java

@@ -23,36 +23,32 @@
 import java.util.concurrent.CompletableFuture;
 
 import org.neo4j.driver.internal.Bookmarks;
-import org.neo4j.driver.internal.ExplicitTransaction;
 import org.neo4j.driver.internal.spi.ResponseHandler;
 import org.neo4j.driver.v1.Value;
 
 import static java.util.Objects.requireNonNull;
 
 public class CommitTxResponseHandler implements ResponseHandler
 {
-    private final CompletableFuture<Void> commitFuture;
-    private final ExplicitTransaction tx;
+    private final CompletableFuture<Bookmarks> commitFuture;
 
-    public CommitTxResponseHandler( CompletableFuture<Void> commitFuture, ExplicitTransaction tx )
+    public CommitTxResponseHandler( CompletableFuture<Bookmarks> commitFuture )
     {
         this.commitFuture = requireNonNull( commitFuture );
-        this.tx = requireNonNull( tx );
     }
 
     @Override
     public void onSuccess( Map<String,Value> metadata )
     {
         Value bookmarkValue = metadata.get( "bookmark" );
-        if ( bookmarkValue != null )
+        if ( bookmarkValue == null )
         {
-            if ( tx != null )
-            {
-                tx.setBookmarks( Bookmarks.from( bookmarkValue.asString() ) );
-            }
+            commitFuture.complete( null );
+        }
+        else
+        {
+            commitFuture.complete( Bookmarks.from( bookmarkValue.asString() ) );
         }
-
-        commitFuture.complete( null );
     }
 
     @Override

driver/src/main/java/org/neo4j/driver/internal/messaging/BoltProtocol.java

@@ -70,10 +70,9 @@ public interface BoltProtocol
      * Commit the explicit transaction.
      *
      * @param connection the connection to use.
-     * @param tx the explicit transaction being committed. Parameter is needed to update bookmark.
-     * @return a completion stage completed when transaction is committed or completed exceptionally when there was a failure.
+     * @return a completion stage completed with a bookmark when transaction is committed or completed exceptionally when there was a failure.
      */
-    CompletionStage<Void> commitTransaction( Connection connection, ExplicitTransaction tx );
+    CompletionStage<Bookmarks> commitTransaction( Connection connection );
 
     /**
      * Rollback the explicit transaction.

driver/src/main/java/org/neo4j/driver/internal/messaging/v1/BoltProtocolV1.java

@@ -108,11 +108,11 @@ public CompletionStage<Void> beginTransaction( Connection connection, Bookmarks
     }
 
     @Override
-    public CompletionStage<Void> commitTransaction( Connection connection, ExplicitTransaction tx )
+    public CompletionStage<Bookmarks> commitTransaction( Connection connection )
     {
-        CompletableFuture<Void> commitFuture = new CompletableFuture<>();
+        CompletableFuture<Bookmarks> commitFuture = new CompletableFuture<>();
 
-        ResponseHandler pullAllHandler = new CommitTxResponseHandler( commitFuture, tx );
+        ResponseHandler pullAllHandler = new CommitTxResponseHandler( commitFuture );
         connection.writeAndFlush(
                 COMMIT_MESSAGE, NoOpResponseHandler.INSTANCE,
                 PullAllMessage.PULL_ALL, pullAllHandler );

driver/src/main/java/org/neo4j/driver/internal/messaging/v3/BoltProtocolV3.java

@@ -101,10 +101,10 @@ public CompletionStage<Void> beginTransaction( Connection connection, Bookmarks
     }
 
     @Override
-    public CompletionStage<Void> commitTransaction( Connection connection, ExplicitTransaction tx )
+    public CompletionStage<Bookmarks> commitTransaction( Connection connection )
     {
-        CompletableFuture<Void> commitFuture = new CompletableFuture<>();
-        connection.writeAndFlush( COMMIT, new CommitTxResponseHandler( commitFuture, tx ) );
+        CompletableFuture<Bookmarks> commitFuture = new CompletableFuture<>();
+        connection.writeAndFlush( COMMIT, new CommitTxResponseHandler( commitFuture ) );
         return commitFuture;
     }
 

driver/src/test/java/org/neo4j/driver/internal/handlers/CommitTxResponseHandlerTest.java

@@ -0,0 +1,75 @@
+/*
+ * Copyright (c) 2002-2018 "Neo4j,"
+ * Neo4j Sweden AB [http://neo4j.com]
+ *
+ * This file is part of Neo4j.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.neo4j.driver.internal.handlers;
+
+import org.junit.jupiter.api.Test;
+
+import java.util.concurrent.CompletableFuture;
+
+import org.neo4j.driver.internal.Bookmarks;
+import org.neo4j.driver.v1.Value;
+
+import static java.util.Collections.emptyMap;
+import static java.util.Collections.singletonMap;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.neo4j.driver.v1.Values.value;
+import static org.neo4j.driver.v1.util.TestUtil.await;
+
+class CommitTxResponseHandlerTest
+{
+    private final CompletableFuture<Bookmarks> future = new CompletableFuture<>();
+    private final CommitTxResponseHandler handler = new CommitTxResponseHandler( future );
+
+    @Test
+    void shouldHandleSuccessWithoutBookmark()
+    {
+        handler.onSuccess( emptyMap() );
+
+        assertNull( await( future ) );
+    }
+
+    @Test
+    void shouldHandleSuccessWithBookmark()
+    {
+        String bookmarkString = "neo4j:bookmark:v1:tx12345";
+
+        handler.onSuccess( singletonMap( "bookmark", value( bookmarkString ) ) );
+
+        assertEquals( Bookmarks.from( bookmarkString ), await( future ) );
+    }
+
+    @Test
+    void shouldHandleFailure()
+    {
+        RuntimeException error = new RuntimeException( "Hello" );
+
+        handler.onFailure( error );
+
+        RuntimeException receivedError = assertThrows( RuntimeException.class, () -> await( future ) );
+        assertEquals( error, receivedError );
+    }
+
+    @Test
+    void shouldFailToHandleRecord()
+    {
+        assertThrows( UnsupportedOperationException.class, () -> handler.onRecord( new Value[]{value( 42 )} ) );
+    }
+}

driver/src/test/java/org/neo4j/driver/internal/messaging/v1/BoltProtocolV1Test.java

@@ -66,10 +66,14 @@
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 import static org.neo4j.driver.internal.util.Futures.blockingGet;
 import static org.neo4j.driver.v1.Values.value;
+import static org.neo4j.driver.v1.util.TestUtil.DEFAULT_TEST_PROTOCOL;
+import static org.neo4j.driver.v1.util.TestUtil.await;
 import static org.neo4j.driver.v1.util.TestUtil.connectionMock;
 
 public class BoltProtocolV1Test
@@ -168,15 +172,24 @@ void shouldBeginTransactionWithBookmark()
     @Test
     void shouldCommitTransaction()
     {
-        Connection connection = connectionMock();
+        String bookmarkString = "neo4j:bookmark:v1:tx1909";
 
-        CompletionStage<Void> stage = protocol.commitTransaction( connection, mock( ExplicitTransaction.class ) );
+        Connection connection = mock( Connection.class );
+        when( connection.protocol() ).thenReturn( DEFAULT_TEST_PROTOCOL );
+        doAnswer( invocation ->
+        {
+            ResponseHandler commitHandler = invocation.getArgument( 3 );
+            commitHandler.onSuccess( singletonMap( "bookmark", value( bookmarkString ) ) );
+            return null;
+        } ).when( connection ).writeAndFlush( eq( new RunMessage( "COMMIT" ) ), any(), any(), any() );
+
+        CompletionStage<Bookmarks> stage = protocol.commitTransaction( connection );
 
         verify( connection ).writeAndFlush(
                 eq( new RunMessage( "COMMIT" ) ), eq( NoOpResponseHandler.INSTANCE ),
                 eq( PullAllMessage.PULL_ALL ), any( CommitTxResponseHandler.class ) );
 
-        assertNull( Futures.blockingGet( stage ) );
+        assertEquals( Bookmarks.from( bookmarkString ), await( stage ) );
     }
 
     @Test