Merge pull request #153 from boggle/1.0-consumption-fixes

pontusmelke · pontusmelke · commit 744e9ab059a0 · 2016-04-11T13:15:31.000+02:00
Clarify semantics of statement result methods [DO NOT MERGE]
diff --git a/driver/src/main/java/org/neo4j/driver/internal/InternalStatementResult.java b/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( position > 0 )
+        if ( hasNext() )
         {
-            throw new NoSuchRecordException(
-                    "Cannot retrieve the first record, because other operations have already used the first record. " +
-                    "Please ensure you are not calling `first` multiple times, or are mixing it with calls " +
-                    "to `next`, `single`, `list` or any other method that changes the position of the cursor." );
-        }
+            Record single = next();
+            boolean hasMoreThanOne = hasNext();
 
-        if( !hasNext() )
-        {
-            throw new NoSuchRecordException( "Cannot retrieve the first record, because this result is empty." );
-        }
+            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." );
+            }
 
-        Record first = next();
-        if( hasNext() )
+            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 first;
     }
 
     @Override
     public Record peek()
     {
-        assertOpen();
-        Record nextRecord = recordBuffer.peek();
-        if ( nextRecord != null )
-        {
-            return nextRecord;
-        }
-        else if ( done )
+        if ( tryFetchNext() )
         {
-            return null;
+            return recordBuffer.peek();
         }
         else
         {
-            tryFetching();
-            return peek();
+            throw new NoSuchRecordException( "Cannot peek past the last record" );
         }
     }
 
@@ -274,46 +248,41 @@ 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)
+        if ( done )
         {
-            return summary;
+            recordBuffer.clear();
         }
-
-        while ( !done )
+        else
         {
-            connection.receiveOne();
+            do
+            {
+                connection.receiveOne();
+                recordBuffer.clear();
+            }
+            while ( !done );
         }
-        recordBuffer.clear();
-        open = false;
+
         return summary;
     }
 
@@ -323,26 +292,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;
+    }
 }
diff --git a/driver/src/main/java/org/neo4j/driver/v1/StatementResult.java b/driver/src/main/java/org/neo4j/driver/v1/StatementResult.java
@@ -21,7 +21,6 @@
 import java.util.Iterator;
 import java.util.List;
 
-import org.neo4j.driver.v1.exceptions.ClientException;
 import org.neo4j.driver.v1.exceptions.NoSuchRecordException;
 import org.neo4j.driver.v1.summary.ResultSummary;
 import org.neo4j.driver.v1.util.Function;
@@ -70,23 +69,28 @@ public interface StatementResult extends Iterator<Record>
 
     /**
      * Navigate to and retrieve the next {@link Record} in this result.
+     *
+     * @throws NoSuchRecordException if there is no record left in the stream
      * @return the next record
      */
     @Override Record next();
 
     /**
      * Return the first record in the result, failing if there is not exactly
-     * one record, or if this result has already been used to move past the first record.
+     * one record left in the stream
+     *
+     * Calling this method always exhausts the result, even when {@link NoSuchRecordException} is thrown.
      *
      * @return the first and only record in the stream
-     * @throws NoSuchRecordException if there is not exactly one record in the stream, or if the cursor has been used already
+     * @throws NoSuchRecordException if there is not exactly one record left in the stream
      */
     Record single() throws NoSuchRecordException;
 
     /**
      * Investigate the next upcoming record without moving forward in the result.
      *
-     * @return the next record, or null if there is no next record
+     * @throws NoSuchRecordException if there is no record left in the stream
+     * @return the next record
      */
     Record peek();
 
@@ -102,8 +106,7 @@ public interface StatementResult extends Iterator<Record>
      *
      * Calling this method exhausts the result.
      *
-     * @throws ClientException if the result has already been used
-     * @return list of all immutable records
+     * @return list of all remaining immutable records
      */
     List<Record> list();
 
@@ -119,11 +122,10 @@ public interface StatementResult extends Iterator<Record>
      *
      * Calling this method exhausts the result.
      *
-     * @throws ClientException if the result has already been used
      * @param mapFunction a function to map from Value to T. See {@link Values} for some predefined functions, such
      * as {@link Values#ofBoolean()}, {@link Values#ofList(Function)}.
      * @param <T> the type of result list elements
-     * @return list of all mapped immutable records
+     * @return list of all mapped remaining immutable records
      */
     <T> List<T> list( Function<Record, T> mapFunction );
 
@@ -138,7 +140,7 @@ public interface StatementResult extends Iterator<Record>
      * }
      * </pre>
      *
-     * @return a summary for the whole query
+     * @return a summary for the whole query result
      */
     ResultSummary consume();
-}
+}
diff --git a/driver/src/main/java/org/neo4j/driver/v1/exceptions/Neo4jException.java b/driver/src/main/java/org/neo4j/driver/v1/exceptions/Neo4jException.java
@@ -19,7 +19,8 @@
 package org.neo4j.driver.v1.exceptions;
 
 /**
- * This is the base class for all Neo4j exceptions.
+ * This is the base class for all exceptions caused as part of communication with the remote Neo4j server.
+ *
  * @since 1.0
  */
 public abstract class Neo4jException extends RuntimeException
diff --git a/driver/src/main/java/org/neo4j/driver/v1/exceptions/NoSuchRecordException.java b/driver/src/main/java/org/neo4j/driver/v1/exceptions/NoSuchRecordException.java
@@ -18,10 +18,16 @@
  */
 package org.neo4j.driver.v1.exceptions;
 
+import java.util.NoSuchElementException;
+
 /**
+ * Thrown whenever a client expected to read a record that was not available (i.e. because it wasn't returned by the server).
+ *
+ * This usually indicates an expectation mismatch between client code and database application logic.
+ *
  * @since 1.0
  */
-public class NoSuchRecordException extends ClientException
+public class NoSuchRecordException extends NoSuchElementException
 {
     private static final long serialVersionUID = 9091962868264042491L;
 
diff --git a/driver/src/test/java/org/neo4j/driver/internal/InternalStatementResultTest.java b/driver/src/test/java/org/neo4j/driver/internal/InternalStatementResultTest.java
