Clarify semantics of list and consume

o Also simplifies implementation to avoid unnecessary recursion

Author: boggle

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

@@ -42,7 +42,6 @@
 import org.neo4j.driver.v1.util.Function;
 import org.neo4j.driver.v1.util.Functions;
 
-import static java.lang.String.format;
 import static java.util.Collections.emptyList;
 
 public class InternalStatementResult implements StatementResult
@@ -55,7 +54,6 @@ public class InternalStatementResult implements StatementResult
     private List<String> keys = null;
     private ResultSummary summary = null;
 
-    private boolean open = true;
     private long position = -1;
     private boolean done = false;
 
@@ -170,26 +168,17 @@ StreamCollector pullAllResponseCollector()
     @Override
     public List<String> keys()
     {
-        tryFetching();
+        if ( keys == null )
+        {
+            tryFetchNext();
+        }
         return keys;
     }
 
     @Override
     public boolean hasNext()
     {
-        if (!recordBuffer.isEmpty())
-        {
-            return true;
-        }
-        else if (done)
-        {
-            return false;
-        }
-        else
-        {
-            tryFetching();
-            return hasNext();
-        }
+        return tryFetchNext();
     }
 
     @Override
@@ -201,67 +190,52 @@ public Record next()
         // in a way that makes the two equivalent in performance.
         // To get the intended benefit, we need to allocate Record in this method,
         // and have it copy out its fields from some lower level data structure.
-        assertOpen();
-        Record nextRecord = recordBuffer.poll();
-        if ( nextRecord != null )
+        if ( tryFetchNext() )
         {
             position += 1;
-            return nextRecord;
-        }
-        else if ( done )
-        {
-            return null;
+            return recordBuffer.poll();
         }
         else
         {
-            tryFetching();
-            return next();
+            throw new NoSuchRecordException( "No more records" );
         }
     }
 
     @Override
     public Record single()
     {
-        if( !hasNext() )
+        if ( hasNext() )
         {
-            throw new NoSuchRecordException( "Cannot retrieve a single record, because this result is empty." );
-        }
+            Record single = next();
+            boolean hasMoreThanOne = hasNext();
 
-        Record single = next();
-        boolean hasMany = hasNext();
+            consume();
 
-        consume();
+            if ( hasMoreThanOne )
+            {
+                throw new NoSuchRecordException( "Expected a result with a single record, but this result contains at least one more. " +
+                        "Ensure your query returns only one record, or use `first` instead of `single` if " +
+                        "you do not care about the number of records in the result." );
+            }
 
-        if( hasMany )
+            return single;
+        }
+        else
         {
-            throw new NoSuchRecordException( "Expected a result with a single record, but this result contains at least one more. " +
-                    "Ensure your query returns only one record, or use `first` instead of `single` if " +
-                    "you do not care about the number of records in the result." );
+            throw new NoSuchRecordException( "Cannot retrieve a single record, because this result is empty." );
         }
-
-        return single;
     }
 
     @Override
     public Record peek()
     {
-        assertOpen();
-
-        while ( true )
+        if ( tryFetchNext() )
         {
-            tryFetching();
-            Record nextRecord = recordBuffer.peek();
-            if ( nextRecord == null )
-            {
-                if ( done )
-                {
-                    throw new NoSuchRecordException( "Cannot peek past the last record" );
-                }
-            }
-            else
-            {
-                return nextRecord;
-            }
+            return recordBuffer.peek();
+        }
+        else
+        {
+            throw new NoSuchRecordException( "Cannot peek past the last record" );
         }
     }
 
@@ -274,46 +248,34 @@ public List<Record> list()
     @Override
     public <T> List<T> list( Function<Record, T> mapFunction )
     {
-        if ( isEmpty() )
-        {
-            assertOpen();
-            return emptyList();
-        }
-        else if ( position == -1 && hasNext() )
+        if ( hasNext() )
         {
             List<T> result = new ArrayList<>();
+
             do
             {
                 result.add( mapFunction.apply( next() ) );
             }
             while ( hasNext() );
 
-            consume();
             return result;
         }
         else
         {
-            throw new ClientException(
-                    format( "Can't retain records when cursor is not pointing at the first record (currently at position %d)", position )
-            );
+            return emptyList();
         }
     }
 
-    @SuppressWarnings("StatementWithEmptyBody")
     @Override
     public ResultSummary consume()
     {
-        if(!open)
-        {
-            return summary;
-        }
-
         while ( !done )
         {
             connection.receiveOne();
+            recordBuffer.clear();
         }
         recordBuffer.clear();
-        open = false;
+
         return summary;
     }
 
@@ -323,26 +285,17 @@ public void remove()
         throw new ClientException( "Removing records from a result is not supported." );
     }
 
-    private void assertOpen()
+    private boolean tryFetchNext()
     {
-        if ( !open )
-        {
-            throw new ClientException( "Result has been closed" );
-        }
-    }
-
-    private boolean isEmpty()
-    {
-        tryFetching();
-        return position == -1 && recordBuffer.isEmpty() && done;
-    }
-
-    private void tryFetching()
-    {
-        while ( recordBuffer.isEmpty() && !done )
+        while ( recordBuffer.isEmpty() )
         {
+            if ( done )
+            {
+                return false;
+            }
             connection.receiveOne();
         }
-    }
 
+        return true;
+    }
 }

driver/src/test/java/org/neo4j/driver/internal/InternalStatementResultTest.java

@@ -20,7 +20,6 @@
 
 
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.LinkedList;
 import java.util.List;
 
@@ -33,14 +32,14 @@
 import org.neo4j.driver.internal.spi.Connection;
 import org.neo4j.driver.internal.value.NullValue;
 import org.neo4j.driver.v1.Record;
-import org.neo4j.driver.v1.Records;
 import org.neo4j.driver.v1.Statement;
 import org.neo4j.driver.v1.StatementResult;
 import org.neo4j.driver.v1.Value;
-import org.neo4j.driver.v1.exceptions.ClientException;
 import org.neo4j.driver.v1.exceptions.NoSuchRecordException;
 import org.neo4j.driver.v1.util.Pair;
 
+import static java.util.Arrays.asList;
+
 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.collection.IsCollectionWithSize.hasSize;
 import static org.junit.Assert.assertFalse;
@@ -52,6 +51,8 @@
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.mock;
 
+import static org.neo4j.driver.v1.Records.column;
+import static org.neo4j.driver.v1.Values.ofString;
 import static org.neo4j.driver.v1.Values.value;
 
 public class InternalStatementResultTest
@@ -67,16 +68,20 @@ public void iterationShouldWorksAsExpected()
 
         // WHEN
         assertTrue( result.hasNext() );
-        assertThat( values( result.next() ), equalTo(Arrays.asList(value("v1-1"), value( "v2-1" ))));
+        assertThat( values( result.next() ), equalTo( asList(value("v1-1"), value( "v2-1" ))));
 
         assertTrue( result.hasNext() );
-        assertThat( values( result.next() ), equalTo(Arrays.asList(value("v1-2"), value( "v2-2" ))));
+        assertThat( values( result.next() ), equalTo( asList(value("v1-2"), value( "v2-2" ))));
 
         assertTrue( result.hasNext() ); //1 -> 2
 
         // THEN
-        assertThat( values( result.next() ), equalTo(Arrays.asList(value("v1-3"), value( "v2-3" ))));
+        assertThat( values( result.next() ), equalTo( asList(value("v1-3"), value( "v2-3" ))));
         assertFalse( result.hasNext() );
+
+        expectedException.expect( NoSuchRecordException.class );
+
+        // WHEN
         assertNull( result.next() );
     }
 
@@ -206,6 +211,44 @@ public void singleShouldThrowOnConsumedResult()
         result.single();
     }
 
+    @Test
+    public void shouldConsumeTwice()
+    {
+        // GIVEN
+        StatementResult result = createResult( 2 );
+        result.consume();
+
+        // WHEN
+        result.consume();
+
+        // THEN
+        assertFalse( result.hasNext() );
+    }
+
+    @Test
+    public void shouldList()
+    {
+        // GIVEN
+        StatementResult result = createResult( 2 );
+        List<String> records = result.list( column( "k1", ofString() ) );
+
+        // THEN
+        assertThat( records, equalTo( asList( "v1-1", "v1-2" ) ) );
+    }
+
+    @Test
+    public void shouldListTwice()
+    {
+        // GIVEN
+        StatementResult result = createResult( 2 );
+        List<Record> firstList = result.list();
+        assertThat( firstList.size(), equalTo( 2 ) );
+
+        // THEN
+        List<Record> secondList = result.list();
+        assertThat( secondList.size(), equalTo( 0 ) );
+    }
+
     @Test
     public void singleShouldNotThrowOnPartiallyConsumedResult()
     {
@@ -255,7 +298,7 @@ public void retainAndMapByKeyShouldWorkAsExpected()
         StatementResult result = createResult( 3 );
 
         // WHEN
-        List<Value> records = result.list( Records.column( "k1" ) );
+        List<Value> records = result.list( column( "k1" ) );
 
         // THEN
         assertFalse(result.hasNext());
@@ -269,23 +312,13 @@ public void retainAndMapByIndexShouldWorkAsExpected()
         StatementResult result = createResult( 3 );
 
         // WHEN
-        List<Value> records = result.list( Records.column( 0 ) );
+        List<Value> records = result.list( column( 0 ) );
 
         // THEN
         assertFalse(result.hasNext());
         assertThat(records, hasSize( 3 ) );
     }
 
-    @Test
-    public void retainFailsIfItCannotRetainEntireResult()
-    {
-        StatementResult result = createResult( 17 );
-        result.next();
-
-        expectedException.expect( ClientException.class );
-        result.list();
-    }
-
     @Test
     public void accessingOutOfBoundsShouldBeNull()
     {
@@ -312,7 +345,7 @@ public void accessingKeysWithoutCallingNextShouldNotFail()
         // not calling next or single
 
         // THEN
-        assertThat( result.keys(), equalTo( Arrays.asList( "k1", "k2" ) ) );
+        assertThat( result.keys(), equalTo( asList( "k1", "k2" ) ) );
     }
 
     @Test