#819 Fix: Updatable server-side scrollable cursor reports wrong size …

…when inserting a row through the result set before fetching

src/docs/asciidoc/release_notes.adoc

@@ -47,6 +47,10 @@ The partial index metadata support introduced in Jaybird 5.0.5 did not take into
 As part of this fix, we no longer check if `wasNull()` is called on a row, or after getting a column value, as the JDBC API does not require such check.
 Instead, `false` is reported if not currently on a row, or if no column value was retrieved, and an exception is thrown if the result set is closed.
 The value reported by `wasNull()` is reset to `false` on each cursor move (e.g. `next()`), and modified by each `get__XXX__` call.
+* Fixed: Inserting a row into a result set backed by a server-side scrollable cursor could include the inserted row twice (https://github.com/FirebirdSQL/jaybird/issues/819[#819])
++
+This could happen if the insert was performed when the server-side cursor was not fully materialized (e.g. by inserting the row immediately after execute).
+As the server-side cursor is only fully materialized on certain operations like requesting the cursor size, the inserted row could be included in the server-side cursor, as well as the local inserted rows collection.
 
 [#jaybird-5-0-5-changelog]
 === Jaybird 5.0.5

src/main/org/firebirdsql/jdbc/FBFetcher.java

@@ -139,6 +139,26 @@ interface FBFetcher {
      */
     boolean isAfterLast() throws SQLException;
 
+    /**
+     * Signals to the fetcher that an insert is about to be executed.
+     * <p>
+     * This method is primarily intended for a workaround with {@link FBServerScrollFetcher} if the insert is performed
+     * when the server-side cursor is not fully materialized by the server, as that could result in the server also
+     * including the inserted row, leading to duplicate reporting of the inserted row. In response to this method,
+     * the fetcher can trigger full materialization of the server-side cursor.
+     * </p>
+     * <p>
+     * The default implementation of this method does nothing.
+     * </p>
+     *
+     * @throws SQLException
+     *         for database access exceptions
+     * @since 5.0.6
+     */
+    default void beforeExecuteInsert() throws SQLException {
+        // default do nothing
+    }
+
     /**
      * Insert row at current position. This method adds a row at the current
      * position in case of updatable result sets after successful execution of

src/main/org/firebirdsql/jdbc/FBResultSet.java

@@ -1485,7 +1485,9 @@ public void updateObject(String columnLabel, Object x, SQLType targetSqlType) th
     @Override
     public void insertRow() throws SQLException {
         checkUpdatable();
-
+        // prevent issue with server-side scrollable cursor including the inserted row, by forcing materialization of
+        // the entire cursor on the server
+        fbFetcher.beforeExecuteInsert();
         rowUpdater.insertRow();
         fbFetcher.insertRow(rowUpdater.getInsertRow());
         notifyRowUpdater();

src/main/org/firebirdsql/jdbc/FBServerScrollFetcher.java

@@ -56,6 +56,10 @@ final class FBServerScrollFetcher implements FBFetcher {
     private int fetchSize;
     private boolean closed;
 
+    // We delay knowing the local and server-side sizes. Requesting the server-side cursor size triggers a full
+    // materialization of the cursor on the server, so delaying this avoids overhead at the cost of some more complexity
+    // in this fetcher.
+
     // The cursor size taking account maxRows
     private int cursorSize = CURSOR_SIZE_UNKNOWN;
     // The cursor size on the server (ignoring maxRows)
@@ -156,7 +160,7 @@ private void synchronizeServerPosition(int expectedPosition) throws SQLException
 
     @Override
     public boolean first() throws SQLException {
-        try (LockCloseable ignored = stmt.withLock()) {
+        try (LockCloseable ignored = withLock()) {
             checkOpen();
             int newLocalPosition = 1;
             if (!inWindow(newLocalPosition) && cursorSize != 0) {
@@ -172,7 +176,7 @@ public boolean first() throws SQLException {
 
     @Override
     public boolean last() throws SQLException {
-        try (LockCloseable ignored = stmt.withLock()) {
+        try (LockCloseable ignored = withLock()) {
             checkOpen();
             int cursorSize = this.cursorSize;
             int newLocalPosition;
@@ -200,7 +204,7 @@ private ServerPositionCalculation lastServerPosition() {
 
     @Override
     public boolean previous() throws SQLException {
-        try (LockCloseable ignored = stmt.withLock()) {
+        try (LockCloseable ignored = withLock()) {
             checkOpen();
             int oldLocalPosition = localPosition;
             int newLocalPosition = Math.max(1, oldLocalPosition) - 1;
@@ -222,7 +226,7 @@ private ServerPositionCalculation previousServerPosition(int initialServerPositi
 
     @Override
     public boolean next() throws SQLException {
-        try (LockCloseable ignored = stmt.withLock()) {
+        try (LockCloseable ignored = withLock()) {
             checkOpen();
             int oldLocalPosition = localPosition;
             boolean hasMaxRows = maxRows > 0;
@@ -257,7 +261,7 @@ private ServerPositionCalculation nextServerPosition(int initialServerPosition)
 
     @Override
     public boolean absolute(int row) throws SQLException {
-        try (LockCloseable ignored = stmt.withLock()) {
+        try (LockCloseable ignored = withLock()) {
             checkOpen();
             // Overflow beyond cursor size is handled by inWindow returning false
             int newLocalPosition = row >= 0 ? row : Math.max(0, requireCursorSize() + 1 + row);
@@ -294,7 +298,7 @@ private ServerPositionCalculation absoluteServerPosition(int absoluteRow) {
 
     @Override
     public boolean relative(int row) throws SQLException {
-        try (LockCloseable ignored = stmt.withLock()) {
+        try (LockCloseable ignored = withLock()) {
             checkOpen();
             int oldLocalPosition = localPosition;
             // Overflow beyond cursor size is handled by inWindow returning false
@@ -331,7 +335,7 @@ private ServerPositionCalculation relativeServerPosition(int relativeRow) {
 
     @Override
     public void beforeFirst() throws SQLException {
-        try (LockCloseable ignored = stmt.withLock()) {
+        try (LockCloseable ignored = withLock()) {
             checkOpen();
             if (localPosition != 0) {
                 stmt.fetchScroll(FetchType.ABSOLUTE, -1, 0);
@@ -343,7 +347,7 @@ public void beforeFirst() throws SQLException {
 
     @Override
     public void afterLast() throws SQLException {
-        try (LockCloseable ignored = stmt.withLock()) {
+        try (LockCloseable ignored = withLock()) {
             checkOpen();
             int afterLastPosition = requireCursorSize() + 1;
             if (localPosition != afterLastPosition) {
@@ -378,15 +382,15 @@ private void checkOpen() throws SQLException {
 
     @Override
     public int getRowNum() throws SQLException {
-        try (LockCloseable ignored = stmt.withLock()) {
+        try (LockCloseable ignored = withLock()) {
             // NOTE Relying on isAfterLast to (indirectly) call checkOpen()
             return isAfterLast() ? 0 : localPosition;
         }
     }
 
     @Override
     public boolean isEmpty() throws SQLException {
-        try (LockCloseable ignored = stmt.withLock()) {
+        try (LockCloseable ignored = withLock()) {
             // NOTE Relying on requireCursorSize to call checkOpen()
             int cursorSize = requireCursorSize();
             return cursorSize == 0;
@@ -395,23 +399,23 @@ public boolean isEmpty() throws SQLException {
 
     @Override
     public boolean isBeforeFirst() throws SQLException {
-        try (LockCloseable ignored = stmt.withLock()) {
+        try (LockCloseable ignored = withLock()) {
             checkOpen();
             return localPosition == 0;
         }
     }
 
     @Override
     public boolean isFirst() throws SQLException {
-        try (LockCloseable ignored = stmt.withLock()) {
+        try (LockCloseable ignored = withLock()) {
             checkOpen();
             return localPosition == 1 && requireCursorSize() > 0;
         }
     }
 
     @Override
     public boolean isLast() throws SQLException {
-        try (LockCloseable ignored = stmt.withLock()) {
+        try (LockCloseable ignored = withLock()) {
             // NOTE Relying on requireCursorSize to call checkOpen()
             int cursorSize = requireCursorSize();
             return localPosition == cursorSize && cursorSize > 0;
@@ -420,14 +424,19 @@ public boolean isLast() throws SQLException {
 
     @Override
     public boolean isAfterLast() throws SQLException {
-        try (LockCloseable ignored = stmt.withLock()) {
+        try (LockCloseable ignored = withLock()) {
             if (localPosition == 0) return false;
             // NOTE Relying on requireCursorSize to call checkOpen()
             int cursorSize = requireCursorSize();
             return localPosition > cursorSize;
         }
     }
 
+    @Override
+    public void beforeExecuteInsert() throws SQLException {
+        requireCursorSize();
+    }
+
     @Override
     public void insertRow(RowValue data) throws SQLException {
         throw new UnsupportedOperationException("Implementation error: FBServerScrollFetcher should be decorated with FBUpdatableFetcher");
@@ -449,15 +458,15 @@ private int actualFetchSize() {
 
     @Override
     public int getFetchSize() throws SQLException {
-        try (LockCloseable ignored = stmt.withLock()) {
+        try (LockCloseable ignored = withLock()) {
             checkOpen();
             return fetchSize;
         }
     }
 
     @Override
     public void setFetchSize(int fetchSize) {
-        try (LockCloseable ignored = stmt.withLock()) {
+        try (LockCloseable ignored = withLock()) {
             this.fetchSize = fetchSize;
         }
     }
@@ -501,14 +510,16 @@ private int requireCursorSize() throws SQLException {
 
     private int requireServerCursorSize() throws SQLException{
         int serverCursorSize = this.serverCursorSize;
-        if (serverCursorSize == CURSOR_SIZE_UNKNOWN) {
-            if (!stmt.hasFetched()) {
-                // A fetch is required before we can retrieve the cursor size, fetch without moving current position
-                stmt.fetchScroll(FetchType.RELATIVE, -1, 0);
-            }
-            serverCursorSize = this.serverCursorSize = retrieveServerCursorSize();
+        if (serverCursorSize != CURSOR_SIZE_UNKNOWN) return serverCursorSize;
+        if (!stmt.hasFetched()) {
+            // A fetch is required before we can retrieve the cursor size, fetch without moving current position
+            stmt.fetchScroll(FetchType.RELATIVE, -1, 0);
         }
-        return serverCursorSize;
+        return this.serverCursorSize = retrieveServerCursorSize();
+    }
+
+    private LockCloseable withLock() {
+        return stmt.withLock();
     }
 
     private static final class RowListener implements StatementListener {

src/main/org/firebirdsql/jdbc/FBUpdatableFetcher.java

@@ -254,6 +254,11 @@ public boolean isAfterLast() throws SQLException {
         return position > size();
     }
 
+    @Override
+    public void beforeExecuteInsert() throws SQLException {
+        fetcher.beforeExecuteInsert();
+    }
+
     @Override
     public void insertRow(RowValue data) throws SQLException {
         insertedRows.add(data);