From f0ebc31559352c2ea23bd38b7c5eae8865c49de4 Mon Sep 17 00:00:00 2001 From: Guillaume Darmont Date: Tue, 4 Aug 2015 23:33:20 +0200 Subject: [PATCH 1/2] Added more tests to enhance reliability of Cursor implementation --- .../java/org/apache/ibatis/cursor/Cursor.java | 2 +- .../apache/ibatis/cursor/CursorException.java | 21 +++ .../ibatis/cursor/defaults/DefaultCursor.java | 91 ++++++++--- .../cursor_simple/CursorSimpleTest.java | 146 ++++++++++++++++++ 4 files changed, 234 insertions(+), 26 deletions(-) create mode 100644 src/main/java/org/apache/ibatis/cursor/CursorException.java diff --git a/src/main/java/org/apache/ibatis/cursor/Cursor.java b/src/main/java/org/apache/ibatis/cursor/Cursor.java index 49f8814ca14..555bd5735d9 100644 --- a/src/main/java/org/apache/ibatis/cursor/Cursor.java +++ b/src/main/java/org/apache/ibatis/cursor/Cursor.java @@ -39,7 +39,7 @@ public interface Cursor extends Closeable, Iterable { /** * Get the current item index. The first item has the index 0. - * @return -1 if the cursor is not open and has not been consumed. The index of the current item retrieved. + * @return -1 if the first cursor item has not been retrieved. The index of the current item retrieved. */ int getCurrentIndex(); } diff --git a/src/main/java/org/apache/ibatis/cursor/CursorException.java b/src/main/java/org/apache/ibatis/cursor/CursorException.java new file mode 100644 index 00000000000..a05e907fb56 --- /dev/null +++ b/src/main/java/org/apache/ibatis/cursor/CursorException.java @@ -0,0 +1,21 @@ +package org.apache.ibatis.cursor; + +import org.apache.ibatis.exceptions.PersistenceException; + +/** + * @author Guillaume Darmont / guillaume@dropinocean.com + */ +public class CursorException extends PersistenceException { + + public CursorException(String message) { + super(message); + } + + public CursorException(String message, Throwable cause) { + super(message, cause); + } + + public CursorException(Throwable cause) { + super(cause); + } +} diff --git a/src/main/java/org/apache/ibatis/cursor/defaults/DefaultCursor.java b/src/main/java/org/apache/ibatis/cursor/defaults/DefaultCursor.java index 30c86afd7bb..2c90d75a7cf 100644 --- a/src/main/java/org/apache/ibatis/cursor/defaults/DefaultCursor.java +++ b/src/main/java/org/apache/ibatis/cursor/defaults/DefaultCursor.java @@ -16,6 +16,7 @@ package org.apache.ibatis.cursor.defaults; import org.apache.ibatis.cursor.Cursor; +import org.apache.ibatis.cursor.CursorException; import org.apache.ibatis.executor.resultset.DefaultResultSetHandler; import org.apache.ibatis.executor.resultset.ResultSetWrapper; import org.apache.ibatis.mapping.ResultMap; @@ -30,6 +31,9 @@ import java.util.NoSuchElementException; /** + * This is the default implementation of a MyBatis Cursor. + * This implementation is not thread safe. + * * @author Guillaume Darmont / guillaume@dropinocean.com */ public class DefaultCursor implements Cursor { @@ -41,13 +45,31 @@ public class DefaultCursor implements Cursor { private final RowBounds rowBounds; private final ObjectWrapperResultHandler objectWrapperResultHandler = new ObjectWrapperResultHandler(); - private int currentIndex = -1; + private CursorIterator cursorIterator = new CursorIterator(); + private boolean iteratorRetrieved = false; - private boolean iteratorAlreadyOpened = false; + private CursorStatus status = CursorStatus.CREATED; + private int indexWithRowBound = -1; - private boolean opened = false; + private enum CursorStatus { - private boolean resultSetConsumed = false; + /** + * A freshly created cursor, database ResultSet consuming has not started + */ + CREATED, + /** + * A cursor currently in use, database ResultSet consuming has started + */ + OPEN, + /** + * A closed cursor, not fully consumed + */ + CLOSED, + /** + * A fully consumed cursor, a consumed cursor is always closed + */ + CONSUMED + } public DefaultCursor(DefaultResultSetHandler resultSetHandler, ResultMap resultMap, ResultSetWrapper rsw, RowBounds rowBounds) { this.resultSetHandler = resultSetHandler; @@ -58,26 +80,34 @@ public DefaultCursor(DefaultResultSetHandler resultSetHandler, ResultMap resultM @Override public boolean isOpen() { - return opened; + return status == CursorStatus.OPEN; } @Override public boolean isConsumed() { - return resultSetConsumed; + return status == CursorStatus.CONSUMED; } @Override public int getCurrentIndex() { - return currentIndex; + return rowBounds.getOffset() + cursorIterator.iteratorIndex; } @Override public Iterator iterator() { - return new CursorIterator(); + if (iteratorRetrieved) { + throw new IllegalStateException("Cannot open more than one iterator on a Cursor"); + } + iteratorRetrieved = true; + return cursorIterator; } @Override public void close() { + if (isClosed()) { + return; + } + ResultSet rs = rsw.getResultSet(); try { if (rs != null) { @@ -88,7 +118,7 @@ public void close() { statement.close(); } } - opened = false; + status = CursorStatus.CLOSED; } catch (SQLException e) { // ignore } @@ -96,19 +126,19 @@ public void close() { protected T fetchNextUsingRowBound() { T result = fetchNextObjectFromDatabase(); - while (currentIndex < rowBounds.getOffset()) { + while (result != null && indexWithRowBound < rowBounds.getOffset()) { result = fetchNextObjectFromDatabase(); } return result; } protected T fetchNextObjectFromDatabase() { - if (resultSetConsumed) { + if (isClosed()) { return null; } try { - opened = true; + status = CursorStatus.OPEN; resultSetHandler.handleRowValues(rsw, resultMap, objectWrapperResultHandler, RowBounds.DEFAULT, null); } catch (SQLException e) { throw new RuntimeException(e); @@ -116,28 +146,32 @@ protected T fetchNextObjectFromDatabase() { T next = objectWrapperResultHandler.result; if (next != null) { - currentIndex++; + indexWithRowBound++; } // No more object or limit reached if (next == null || (getReadItemsCount() == rowBounds.getOffset() + rowBounds.getLimit())) { close(); - resultSetConsumed = true; + status = CursorStatus.CONSUMED; } objectWrapperResultHandler.result = null; return next; } + private boolean isClosed() { + return status == CursorStatus.CLOSED || status == CursorStatus.CONSUMED; + } + private int getReadItemsCount() { - return currentIndex + 1; + return indexWithRowBound + 1; } - private static class ObjectWrapperResultHandler implements ResultHandler { + private static class ObjectWrapperResultHandler implements ResultHandler { private E result; @Override - public void handleResult(ResultContext context) { + public void handleResult(ResultContext context) { this.result = (E) context.getResultObject(); context.stop(); } @@ -146,19 +180,21 @@ public void handleResult(ResultContext context) { private class CursorIterator implements Iterator { /** - * Holder for the next objet to be returned + * Holder for the next object to be returned */ T object; - public CursorIterator() { - if (iteratorAlreadyOpened) { - throw new IllegalStateException("Cannot open more than one iterator on a Cursor"); - } - iteratorAlreadyOpened = true; - } + /** + * Index of objects returned using next(), and as such, visible to users. + */ + int iteratorIndex = -1; @Override public boolean hasNext() { + if (isClosed()) { + return false; + } + if (object == null) { object = fetchNextUsingRowBound(); } @@ -167,6 +203,10 @@ public boolean hasNext() { @Override public T next() { + if (isClosed()) { + throw new CursorException("Cursor is closed"); + } + // Fill next with object fetched from hasNext() T next = object; @@ -176,6 +216,7 @@ public T next() { if (next != null) { object = null; + iteratorIndex++; return next; } throw new NoSuchElementException(); @@ -183,7 +224,7 @@ public T next() { @Override public void remove() { - throw new UnsupportedOperationException("Cannot currently remove element from Cursor"); + throw new UnsupportedOperationException("Cannot remove element from Cursor"); } } } diff --git a/src/test/java/org/apache/ibatis/submitted/cursor_simple/CursorSimpleTest.java b/src/test/java/org/apache/ibatis/submitted/cursor_simple/CursorSimpleTest.java index 12d666c839b..451a1c0990a 100644 --- a/src/test/java/org/apache/ibatis/submitted/cursor_simple/CursorSimpleTest.java +++ b/src/test/java/org/apache/ibatis/submitted/cursor_simple/CursorSimpleTest.java @@ -16,6 +16,7 @@ package org.apache.ibatis.submitted.cursor_simple; import org.apache.ibatis.cursor.Cursor; +import org.apache.ibatis.cursor.CursorException; import org.apache.ibatis.io.Resources; import org.apache.ibatis.jdbc.ScriptRunner; import org.apache.ibatis.session.RowBounds; @@ -26,6 +27,7 @@ import org.junit.BeforeClass; import org.junit.Test; +import java.io.IOException; import java.io.Reader; import java.sql.Connection; import java.util.Iterator; @@ -60,6 +62,9 @@ public void shouldGetAllUser() { try { Assert.assertFalse(usersCursor.isOpen()); + // Cursor is just created, current index is -1 + Assert.assertEquals(-1, usersCursor.getCurrentIndex()); + Iterator iterator = usersCursor.iterator(); // Check if hasNext, fetching is started @@ -67,6 +72,9 @@ public void shouldGetAllUser() { Assert.assertTrue(usersCursor.isOpen()); Assert.assertFalse(usersCursor.isConsumed()); + // next() has not been called, index is still -1 + Assert.assertEquals(-1, usersCursor.getCurrentIndex()); + User user = iterator.next(); Assert.assertEquals("User1", user.getName()); Assert.assertEquals(0, usersCursor.getCurrentIndex()); @@ -129,6 +137,7 @@ public void testCursorWithRowBound() { SqlSession sqlSession = sqlSessionFactory.openSession(); try { + // RowBound starting at offset 1 and limiting to 2 items Cursor usersCursor = sqlSession.selectCursor("getAllUsers", null, new RowBounds(1, 2)); Iterator iterator = usersCursor.iterator(); @@ -148,4 +157,141 @@ public void testCursorWithRowBound() { sqlSession.close(); } } + + @Test + public void testCursorWithBadRowBound() { + SqlSession sqlSession = sqlSessionFactory.openSession(); + + try { + // Trying to start at offset 10 (which does not exist, since there is only 4 items) + Cursor usersCursor = sqlSession.selectCursor("getAllUsers", null, new RowBounds(10, 2)); + Iterator iterator = usersCursor.iterator(); + + Assert.assertFalse(iterator.hasNext()); + Assert.assertFalse(usersCursor.isOpen()); + Assert.assertTrue(usersCursor.isConsumed()); + } finally { + sqlSession.close(); + } + } + + @Test + public void testCursorMultipleHasNextCall() { + SqlSession sqlSession = sqlSessionFactory.openSession(); + Mapper mapper = sqlSession.getMapper(Mapper.class); + Cursor usersCursor = mapper.getAllUsers(); + + try { + Iterator iterator = usersCursor.iterator(); + + Assert.assertEquals(-1, usersCursor.getCurrentIndex()); + + User user = iterator.next(); + Assert.assertEquals("User1", user.getName()); + Assert.assertEquals(0, usersCursor.getCurrentIndex()); + + Assert.assertTrue(iterator.hasNext()); + Assert.assertTrue(iterator.hasNext()); + Assert.assertTrue(iterator.hasNext()); + // assert that index has not changed after hasNext() call + Assert.assertEquals(0, usersCursor.getCurrentIndex()); + } finally { + sqlSession.close(); + } + } + + @Test + public void testCursorMultipleIteratorCall() { + SqlSession sqlSession = sqlSessionFactory.openSession(); + Mapper mapper = sqlSession.getMapper(Mapper.class); + Cursor usersCursor = mapper.getAllUsers(); + + Iterator iterator2 = null; + try { + Iterator iterator = usersCursor.iterator(); + User user = iterator.next(); + Assert.assertEquals("User1", user.getName()); + Assert.assertEquals(0, usersCursor.getCurrentIndex()); + + iterator2 = usersCursor.iterator(); + iterator2.hasNext(); + Assert.fail("We should have failed since calling iterator several times is not allowed"); + } catch (IllegalStateException e) { + Assert.assertNull("iterator2 should be null", iterator2); + return; + } finally { + sqlSession.close(); + } + Assert.fail("Should have returned earlier"); + } + + @Test + public void testCursorMultipleCloseCall() throws IOException { + SqlSession sqlSession = sqlSessionFactory.openSession(); + Mapper mapper = sqlSession.getMapper(Mapper.class); + Cursor usersCursor = mapper.getAllUsers(); + try { + Assert.assertFalse(usersCursor.isOpen()); + + Iterator iterator = usersCursor.iterator(); + + // Check if hasNext, fetching is started + Assert.assertTrue(iterator.hasNext()); + Assert.assertTrue(usersCursor.isOpen()); + Assert.assertFalse(usersCursor.isConsumed()); + + // Consume only the first result + User user = iterator.next(); + Assert.assertEquals("User1", user.getName()); + + usersCursor.close(); + // Check multiple close are no-op + usersCursor.close(); + + // hasNext now return false, since the cursor is closed + Assert.assertFalse(iterator.hasNext()); + Assert.assertFalse(usersCursor.isOpen()); + Assert.assertFalse(usersCursor.isConsumed()); + } finally { + sqlSession.close(); + } + } + + @Test + public void testCursorUsageAfterClose() throws IOException { + SqlSession sqlSession = sqlSessionFactory.openSession(); + Mapper mapper = sqlSession.getMapper(Mapper.class); + Cursor usersCursor = mapper.getAllUsers(); + + try { + Iterator iterator = usersCursor.iterator(); + User user = iterator.next(); + Assert.assertEquals("User1", user.getName()); + Assert.assertEquals(0, usersCursor.getCurrentIndex()); + + user = iterator.next(); + Assert.assertEquals("User2", user.getName()); + Assert.assertEquals(1, usersCursor.getCurrentIndex()); + + usersCursor.close(); + + // hasNext now return false, since the cursor is closed + Assert.assertFalse(iterator.hasNext()); + Assert.assertFalse(usersCursor.isOpen()); + Assert.assertFalse(usersCursor.isConsumed()); + + // trying next() will fail + iterator.next(); + + Assert.fail("We should have failed since Cursor is closed"); + } catch (CursorException e) { + // We had an exception and current index has not changed + Assert.assertEquals(1, usersCursor.getCurrentIndex()); + return; + } finally { + sqlSession.close(); + } + + Assert.fail("Should have returned earlier"); + } } From 3a12959c56aea511696f55a89f6fbc84830a36ee Mon Sep 17 00:00:00 2001 From: Guillaume Darmont Date: Tue, 4 Aug 2015 23:53:16 +0200 Subject: [PATCH 2/2] Added missing copyright header on CursorException --- .../org/apache/ibatis/cursor/CursorException.java | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/main/java/org/apache/ibatis/cursor/CursorException.java b/src/main/java/org/apache/ibatis/cursor/CursorException.java index a05e907fb56..1a6db7b34ae 100644 --- a/src/main/java/org/apache/ibatis/cursor/CursorException.java +++ b/src/main/java/org/apache/ibatis/cursor/CursorException.java @@ -1,3 +1,18 @@ +/** + * Copyright 2009-2015 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. + * 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.apache.ibatis.cursor; import org.apache.ibatis.exceptions.PersistenceException;