#807 ResultSet.isBeforeFirst()/isAfterLast() should always report fal…

…se for an empty result set
FirebirdSQL · Jun 8, 2024 · ceff1cb · ceff1cb
1 parent a49b3af
commit ceff1cb
Show file tree

Hide file tree

Showing 5 changed files with 77 additions and 21 deletions.
diff --git a/src/docs/asciidoc/release_notes.adoc b/src/docs/asciidoc/release_notes.adoc
@@ -52,6 +52,7 @@ This feature was backported from Jaybird 6.
 * Improvement: The `FILTER_CONDITION` of `DatabaseMetaData.getIndexInfo` is populated for Firebird 5.0 partial indices (https://github.com/FirebirdSQL/jaybird/issues/797[#797])
 +
 This improvement was backported from Jaybird 6.
+* Fixed: `ResultSet.isBeforeFirst()` and `ResultSet.isAfterLast()` should always report `false` for an empty result set (https://github.com/FirebirdSQL/jaybird/issues/807[#807])
 * ...
 
 [#jaybird-5-0-4-changelog]

diff --git a/src/main/org/firebirdsql/jdbc/FBCachedFetcher.java b/src/main/org/firebirdsql/jdbc/FBCachedFetcher.java
@@ -294,12 +294,12 @@ public int getRowNum() {
 
     @Override
     public boolean isEmpty() {
-        return rows == null || rows.size() == 0;
+        return rows == null || rows.isEmpty();
     }
 
     @Override
     public boolean isBeforeFirst() {
-        return !isEmpty() && rowNum < 1;
+        return rowNum < 1;
     }
 
     @Override
@@ -320,28 +320,22 @@ public boolean isAfterLast() {
     @Override
     public void deleteRow() throws SQLException {
         rows.remove(rowNum - 1);
-
-        if (isAfterLast() || isBeforeFirst())
-            fetcherListener.rowChanged(this, null);
-        else
-            fetcherListener.rowChanged(this, rows.get(rowNum - 1));
+        fetcherListener.rowChanged(this, isAfterLast() || isBeforeFirst() ? null : rows.get(rowNum - 1));
     }
 
     @Override
     public void insertRow(RowValue data) throws SQLException {
-        if (rowNum == 0)
-            rowNum++;
+        if (rowNum == 0) {
+            rowNum = 1;
+        }
 
         if (rowNum > rows.size()) {
             rows.add(data);
         } else {
             rows.add(rowNum - 1, data);
         }
 
-        if (isAfterLast() || isBeforeFirst())
-            fetcherListener.rowChanged(this, null);
-        else
-            fetcherListener.rowChanged(this, rows.get(rowNum - 1));
+        fetcherListener.rowChanged(this, isAfterLast() || isBeforeFirst() ? null : rows.get(rowNum - 1));
     }
 
     @Override

diff --git a/src/main/org/firebirdsql/jdbc/FBFetcher.java b/src/main/org/firebirdsql/jdbc/FBFetcher.java
@@ -20,10 +20,11 @@
 
 import org.firebirdsql.gds.ng.fields.RowValue;
 
+import java.sql.ResultSet;
 import java.sql.SQLException;
 
 /**
- * Instances of this class are able to fetch records from the server.
+ * Instances of this class fetch records from the server.
  */
 interface FBFetcher {
 
@@ -109,10 +110,33 @@ interface FBFetcher {
      */
     int getRowNum() throws SQLException;
 
+    /**
+     * @return {@code true} if the result set is empty; otherwise {@code false}
+     */
     boolean isEmpty() throws SQLException;
+
+    /**
+     * @return {@code true} if positioned before the first row, contrary to {@link ResultSet#isBeforeFirst()} it also
+     * reports {@code true} if empty and next was not invoked
+     * @see ResultSet#isBeforeFirst()
+     */
     boolean isBeforeFirst() throws SQLException;
+
+    /**
+     * @see ResultSet#isFirst()
+     */
     boolean isFirst() throws SQLException;
+
+    /**
+     * @see ResultSet#isLast()
+     */
     boolean isLast() throws SQLException;
+
+    /**
+     * @return {@code true} if positioned after the last row, contrary to {@link ResultSet#isBeforeFirst()} it also
+     * reports {@code true} if empty and next was invoked
+     * @see ResultSet#isAfterLast() 
+     */
     boolean isAfterLast() throws SQLException;
 
     /**

diff --git a/src/main/org/firebirdsql/jdbc/FBResultSet.java b/src/main/org/firebirdsql/jdbc/FBResultSet.java
@@ -894,13 +894,13 @@ public BigDecimal getBigDecimal(String columnName) throws SQLException {
     @Override
     public boolean isBeforeFirst() throws SQLException {
         checkOpen();
-        return fbFetcher.isBeforeFirst();
+        return !fbFetcher.isEmpty() && fbFetcher.isBeforeFirst();
     }
 
     @Override
     public boolean isAfterLast() throws SQLException {
         checkOpen();
-        return fbFetcher.isAfterLast();
+        return !fbFetcher.isEmpty() && fbFetcher.isAfterLast();
     }
 
     @Override

diff --git a/src/test/org/firebirdsql/jdbc/FBResultSetTest.java b/src/test/org/firebirdsql/jdbc/FBResultSetTest.java
@@ -22,12 +22,12 @@
 import org.firebirdsql.common.extension.UsesDatabaseExtension;
 import org.firebirdsql.gds.ISCConstants;
 import org.firebirdsql.gds.JaybirdErrorCodes;
-import org.firebirdsql.jaybird.props.PropertyConstants;
 import org.firebirdsql.jaybird.props.PropertyNames;
 import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
 import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
 
 import java.io.ByteArrayInputStream;
@@ -45,6 +45,8 @@
 import static org.firebirdsql.common.FBTestProperties.*;
 import static org.firebirdsql.common.matchers.GdsTypeMatchers.isPureJavaType;
 import static org.firebirdsql.common.matchers.SQLExceptionMatchers.*;
+import static org.firebirdsql.jaybird.props.PropertyConstants.SCROLLABLE_CURSOR_EMULATED;
+import static org.firebirdsql.jaybird.props.PropertyConstants.SCROLLABLE_CURSOR_SERVER;
 import static org.hamcrest.CoreMatchers.*;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.jupiter.api.Assertions.*;
@@ -616,9 +618,9 @@ void testUpdatableResultSet(String scrollableCursorPropertyValue) throws Excepti
                         if (counter == recordCount + 1) rs.deleteRow();
                     }
 
-                    if (PropertyConstants.SCROLLABLE_CURSOR_SERVER.equals(scrollableCursorPropertyValue)
-                            && isPureJavaType().matches(GDS_TYPE)
-                            && getDefaultSupportInfo().supportsScrollableCursors()) {
+                    if (SCROLLABLE_CURSOR_SERVER.equals(scrollableCursorPropertyValue)
+                        && isPureJavaType().matches(GDS_TYPE)
+                        && getDefaultSupportInfo().supportsScrollableCursors()) {
                         assertEquals(counter - 1, recordCount + 1);
                     } else {
                         assertEquals(counter - 1, recordCount);
@@ -1434,10 +1436,45 @@ void testIsAfterLast_bug689() throws Exception {
         }
     }
 
+    /**
+     * Rationale: see <a href="https://github.com/FirebirdSQL/jaybird/issues/807">jaybird#807</a>
+     */
+    @ParameterizedTest
+    @MethodSource
+    void testIsBeforeFirst_isAfterLast_emptyResultSet_bug807(int resultSetType, int resultSetConcurrency,
+            String scrollableCursorPropertyValue) throws SQLException {
+        try (Connection connection = createConnection(scrollableCursorPropertyValue)) {
+            executeCreateTable(connection, CREATE_TABLE_STATEMENT);
+
+            try (Statement stmt = connection.createStatement(resultSetType, resultSetConcurrency);
+                 ResultSet rs = stmt.executeQuery(SELECT_TEST_TABLE)) {
+                assertFalse(rs.isBeforeFirst(), "isBeforeFirst for empty result set should be false before next");
+                assertFalse(rs.isAfterLast(), "isAfterLast for empty result set should be false before next");
+                assertFalse(rs.next(), "Expected no row in empty result set");
+                assertFalse(rs.isBeforeFirst(), "isBeforeFirst for empty result set should be false after next");
+                assertFalse(rs.isAfterLast(), "isAfterLast for empty result set should be false after next");
+            }
+        }
+    }
+
+    static Stream<Arguments> testIsBeforeFirst_isAfterLast_emptyResultSet_bug807() {
+        Stream<Arguments> defaultArguments = Stream.of(
+                Arguments.of(TYPE_FORWARD_ONLY, CONCUR_READ_ONLY, SCROLLABLE_CURSOR_EMULATED),
+                Arguments.of(TYPE_FORWARD_ONLY, CONCUR_UPDATABLE, SCROLLABLE_CURSOR_EMULATED),
+                Arguments.of(TYPE_SCROLL_INSENSITIVE, CONCUR_READ_ONLY, SCROLLABLE_CURSOR_EMULATED),
+                Arguments.of(TYPE_SCROLL_INSENSITIVE, CONCUR_UPDATABLE, SCROLLABLE_CURSOR_EMULATED));
+        if (getDefaultSupportInfo().supportsScrollableCursors()) {
+            return Stream.concat(defaultArguments, Stream.of(
+                    Arguments.of(TYPE_SCROLL_INSENSITIVE, CONCUR_READ_ONLY, SCROLLABLE_CURSOR_SERVER),
+                    Arguments.of(TYPE_SCROLL_INSENSITIVE, CONCUR_UPDATABLE, SCROLLABLE_CURSOR_SERVER)));
+        }
+        return defaultArguments;
+    }
+
     static Stream<String> scrollableCursorPropertyValues() {
         // We are unconditionally emitting SERVER, to check if the value behaves appropriately on versions that do
         // not support server-side scrollable cursors
-        return Stream.of(PropertyConstants.SCROLLABLE_CURSOR_EMULATED, PropertyConstants.SCROLLABLE_CURSOR_SERVER);
+        return Stream.of(SCROLLABLE_CURSOR_EMULATED, SCROLLABLE_CURSOR_SERVER);
     }
 
     private static Connection createConnection(String scrollableCursorPropertyValue) throws SQLException {