Merge pull request #1657 from harawata/gh/1653

Prevent null element from stopping cursor iteration

Author: harawata

src/main/java/org/apache/ibatis/cursor/defaults/DefaultCursor.java

@@ -41,7 +41,7 @@ public class DefaultCursor<T> implements Cursor<T> {
   private final ResultMap resultMap;
   private final ResultSetWrapper rsw;
   private final RowBounds rowBounds;
-  private final ObjectWrapperResultHandler<T> objectWrapperResultHandler = new ObjectWrapperResultHandler<>();
+  protected final ObjectWrapperResultHandler<T> objectWrapperResultHandler = new ObjectWrapperResultHandler<>();
 
   private final CursorIterator cursorIterator = new CursorIterator();
   private boolean iteratorRetrieved;
@@ -123,7 +123,7 @@ public void close() {
 
   protected T fetchNextUsingRowBound() {
     T result = fetchNextObjectFromDatabase();
-    while (result != null && indexWithRowBound < rowBounds.getOffset()) {
+    while (objectWrapperResultHandler.fetched && indexWithRowBound < rowBounds.getOffset()) {
       result = fetchNextObjectFromDatabase();
     }
     return result;
@@ -135,6 +135,7 @@ protected T fetchNextObjectFromDatabase() {
     }
 
     try {
+      objectWrapperResultHandler.fetched = false;
       status = CursorStatus.OPEN;
       if (!rsw.getResultSet().isClosed()) {
         resultSetHandler.handleRowValues(rsw, resultMap, objectWrapperResultHandler, RowBounds.DEFAULT, null);
@@ -144,11 +145,11 @@ protected T fetchNextObjectFromDatabase() {
     }
 
     T next = objectWrapperResultHandler.result;
-    if (next != null) {
+    if (objectWrapperResultHandler.fetched) {
       indexWithRowBound++;
     }
     // No more object or limit reached
-    if (next == null || getReadItemsCount() == rowBounds.getOffset() + rowBounds.getLimit()) {
+    if (!objectWrapperResultHandler.fetched || getReadItemsCount() == rowBounds.getOffset() + rowBounds.getLimit()) {
       close();
       status = CursorStatus.CONSUMED;
     }
@@ -165,18 +166,20 @@ private int getReadItemsCount() {
     return indexWithRowBound + 1;
   }
 
-  private static class ObjectWrapperResultHandler<T> implements ResultHandler<T> {
+  protected static class ObjectWrapperResultHandler<T> implements ResultHandler<T> {
 
-    private T result;
+    protected T result;
+    protected boolean fetched;
 
     @Override
     public void handleResult(ResultContext<? extends T> context) {
       this.result = context.getResultObject();
       context.stop();
+      fetched = true;
     }
   }
 
-  private class CursorIterator implements Iterator<T> {
+  protected class CursorIterator implements Iterator<T> {
 
     /**
      * Holder for the next object to be returned.
@@ -190,22 +193,23 @@ private class CursorIterator implements Iterator<T> {
 
     @Override
     public boolean hasNext() {
-      if (object == null) {
+      if (!objectWrapperResultHandler.fetched) {
         object = fetchNextUsingRowBound();
       }
-      return object != null;
+      return objectWrapperResultHandler.fetched;
     }
 
     @Override
     public T next() {
       // Fill next with object fetched from hasNext()
       T next = object;
 
-      if (next == null) {
+      if (!objectWrapperResultHandler.fetched) {
         next = fetchNextUsingRowBound();
       }
 
-      if (next != null) {
+      if (objectWrapperResultHandler.fetched) {
+        objectWrapperResultHandler.fetched = false;
         object = null;
         iteratorIndex++;
         return next;

src/test/java/org/apache/ibatis/submitted/cursor_simple/CursorSimpleTest.java

@@ -15,6 +15,11 @@
  */
 package org.apache.ibatis.submitted.cursor_simple;
 
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
 import org.apache.ibatis.BaseDataTest;
 import org.apache.ibatis.cursor.Cursor;
 import org.apache.ibatis.io.Resources;
@@ -387,4 +392,85 @@ void shouldThrowIllegalStateExceptionUsingIteratorOnSessionClosed() {
 
   }
 
+  @Test
+  void shouldNullItemNotStopIteration() {
+    try (SqlSession sqlSession = sqlSessionFactory.openSession()) {
+      Mapper mapper = sqlSession.getMapper(Mapper.class);
+      Cursor<User> cursor = mapper.getNullUsers(new RowBounds());
+      Iterator<User> iterator = cursor.iterator();
+
+      assertFalse(cursor.isOpen());
+
+      // Cursor is just created, current index is -1
+      assertEquals(-1, cursor.getCurrentIndex());
+
+      // Check if hasNext, fetching is started
+      assertTrue(iterator.hasNext());
+      // Re-invoking hasNext() should not fetch the next row
+      assertTrue(iterator.hasNext());
+      assertTrue(cursor.isOpen());
+      assertFalse(cursor.isConsumed());
+
+      // next() has not been called, index is still -1
+      assertEquals(-1, cursor.getCurrentIndex());
+
+      User user;
+      user = iterator.next();
+      assertNull(user);
+      assertEquals(0, cursor.getCurrentIndex());
+
+      assertTrue(iterator.hasNext());
+      user = iterator.next();
+      assertEquals("Kate", user.getName());
+      assertEquals(1, cursor.getCurrentIndex());
+
+      assertTrue(iterator.hasNext());
+      user = iterator.next();
+      assertNull(user);
+      assertEquals(2, cursor.getCurrentIndex());
+
+      assertTrue(iterator.hasNext());
+      user = iterator.next();
+      assertNull(user);
+      assertEquals(3, cursor.getCurrentIndex());
+
+      // Check no more elements
+      assertFalse(iterator.hasNext());
+      assertFalse(cursor.isOpen());
+      assertTrue(cursor.isConsumed());
+    }
+  }
+
+  @Test
+  void shouldRowBoundsCountNullItem() {
+    try (SqlSession sqlSession = sqlSessionFactory.openSession()) {
+      Mapper mapper = sqlSession.getMapper(Mapper.class);
+      Cursor<User> cursor = mapper.getNullUsers(new RowBounds(1, 2));
+      Iterator<User> iterator = cursor.iterator();
+
+      assertFalse(cursor.isOpen());
+
+      // Check if hasNext, fetching is started
+      assertTrue(iterator.hasNext());
+      // Re-invoking hasNext() should not fetch the next row
+      assertTrue(iterator.hasNext());
+      assertTrue(cursor.isOpen());
+      assertFalse(cursor.isConsumed());
+
+      User user;
+      user = iterator.next();
+      assertEquals("Kate", user.getName());
+      assertEquals(1, cursor.getCurrentIndex());
+
+      assertTrue(iterator.hasNext());
+      user = iterator.next();
+      assertNull(user);
+      assertEquals(2, cursor.getCurrentIndex());
+
+      // Check no more elements
+      assertFalse(iterator.hasNext());
+      assertFalse(cursor.isOpen());
+      assertTrue(cursor.isConsumed());
+    }
+  }
 }

src/test/java/org/apache/ibatis/submitted/cursor_simple/Mapper.java

@@ -1,5 +1,5 @@
 /**
- *    Copyright 2009-2015 the original author or authors.
+ *    Copyright 2009-2019 the original author or authors.
  *
  *    Licensed under the Apache License, Version 2.0 (the "License");
  *    you may not use this file except in compliance with the License.
@@ -15,10 +15,22 @@
  */
 package org.apache.ibatis.submitted.cursor_simple;
 
+import org.apache.ibatis.annotations.Select;
 import org.apache.ibatis.cursor.Cursor;
+import org.apache.ibatis.session.RowBounds;
 
 public interface Mapper {
 
   Cursor<User> getAllUsers();
 
+  @Select({
+    "select null id, null name from (values (0))",
+    "union all",
+    "select 99 id, 'Kate' name from (values (0))",
+    "union all",
+    "select null id, null name from (values (0))",
+    "union all",
+    "select null id, null name from (values (0))"
+  })
+  Cursor<User> getNullUsers(RowBounds rowBounds);
 }