#821 Retrofit updatable emulated scrollable cursors to have same beha…

…viour as updatable server-side scrollable cursors

publishes jdp-2024-05

devdoc/jdp/jdp-2024-05-behavior-of-updatable-result-sets.adoc

@@ -2,8 +2,8 @@
 
 == Status
 
-* Draft
-* Proposed for: Jaybird 6
+* Published: 2024-09-24
+* Implemented in: Jaybird 6
 
 == Type
 
@@ -14,11 +14,11 @@
 In https://github.com/FirebirdSQL/jaybird/blob/master/devdoc/jdp/jdp-2021-04-real-scrollable-cursor-support.md[jdp-2021-04], real server-side scrollable cursors where defined.
 In that JDP, for `CONCUR_UPDATABLE`, the behaviour of inserted, deleted, and -- in some respects -- updated rows was changed compared to the original "`emulated`" scrollable cursors:
 
-* new rows are inserted at the end of the cursor;
+* New rows are inserted at the end of the cursor;
 in emulated they are inserted immediately before the current row.
-* deleted rows have an all-``null`` marker row;
+* Deleted rows have an all-``null`` marker row;
 in emulated the row is removed from the cursor.
-* the result set reports `true` for `rowDeleted()`, `rowInserted()` or `rowUpdated()` for -- respectively -- deleted, inserted or updated rows;
+* The result set reports `true` for `rowDeleted()`, `rowInserted()` or `rowUpdated()` for -- respectively -- deleted, inserted or updated rows;
 in emulated these always report `false`.
 
 The first two differences were necessary to keep the accounting of row positions both server-side and locally simple and correct.

src/docs/asciidoc/release_notes.adoc

@@ -960,6 +960,29 @@ If you relied on this implicit close for correctness of your application, you ma
 
 For more information, see also https://github.com/FirebirdSQL/jaybird/blob/master/devdoc/jdp/jdp-2024-03-do-not-close-result-set-after-last-row-in-auto-commit.adoc[jdp-2024-03: Do not close result set after last row in auto-commit^].
 
+[#scroll-rs-update-behavior]
+=== Changes to behaviour of updatable scrollable result sets
+
+Jaybird 5 introduced support for server-side scrollable cursors on Firebird 5.0 and higher in the pure Java protocol.
+This can be enabled using the connection property `scrollableCursor=SERVER`.
+
+For implementation reasons, updatable server-side scrollable cursors had a different behaviour than the emulated client-side scrollable cursors.
+These differences are:
+
+* New rows are inserted at the end of the cursor;
+in emulated they were inserted immediately before the current row.
+* Deleted rows have an all-``null`` marker row;
+in emulated the row was removed from the cursor.
+* The result set reports `true` for `rowDeleted()`, `rowInserted()` or `rowUpdated()` for -- respectively -- deleted, inserted or updated rows;
+in emulated these always reported `false`.
+
+In Jaybird 6, this new behaviour is now also used for the updatable emulated scrollable cursors.
+The reason is that having two different sets of behaviours can be confusing, as it makes it impossible to switch between the two without having to account for the behavioural differences (either intentionally, or because you're connecting with the native or embedded protocol, or to an older version of Firebird).
+
+We're considering to make server-side scrollable cursors the default in a future Jaybird version (Jaybird 7 or later).
+
+See also https://github.com/FirebirdSQL/jaybird/blob/master/devdoc/jdp/jdp-2024-05-behavior-of-updatable-result-sets.adoc[jdp-2024-05: Behaviour of Updatable Result Sets^].
+
 // TODO add major changes
 
 [#other-fixes-and-changes]
@@ -1097,6 +1120,9 @@ If a type is not annotated, consider it nullable unless stated otherwise in the
 +
 In practice, this is an optional dependency, but Maven will pull it in by default.
 If the JSpecify JAR is not included on the classpath or modulepath, Jaybird will still work.
+* `DatabaseMetaData` now reports `ResultSet.TYPE_SCROLL_SENSITIVE` as not supported, as it is always downgraded to `TYPE_SCROLL_INSENSITIVE`, and thus effectively not supported.
++
+This affects the return value of the methods `supportsResultSetType(int)`, `supportsResultSetConcurrency(int, int)`, `ownUpdatesAreVisible(int)`, `ownDeletesAreVisible(int)`, `ownInsertsAreVisible(int)`.
 
 [#potentially-breaking-changes]
 === Potentially breaking changes
@@ -1278,6 +1304,12 @@ You will need to explicitly call `setReadOnly(false)`, or -- better yet -- do no
 
 For more information, see <<no-close-after-last>>.
 
+[#compat-scroll-rs-update-behavior]
+
+=== Behavioural changes for updatable scrollable `ResultSet`
+
+For more information, see <<scroll-rs-update-behavior>>.
+
 // TODO Document compatibility issues
 
 [#removal-of-classes-packages-and-methods-without-deprecation]

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

@@ -43,6 +43,7 @@ abstract sealed class AbstractFetcher implements FBFetcher
 
     private FetchConfig fetchConfig;
     private FetcherListener fetcherListener;
+    private @Nullable RowValue currentRow;
     private volatile boolean closed;
 
     AbstractFetcher(FetchConfig fetchConfig, FetcherListener fetcherListener) {
@@ -64,9 +65,15 @@ public final void setFetcherListener(FetcherListener fetcherListener) {
      *         for exceptions thrown by {@link FetcherListener#rowChanged(FBFetcher, RowValue)}
      */
     protected final void notifyRowChanged(@Nullable RowValue newRow) throws SQLException {
+        currentRow = newRow;
         fetcherListener.rowChanged(this, newRow);
     }
 
+    @Override
+    public final void renotifyCurrentRow() throws SQLException {
+        fetcherListener.rowChanged(this, currentRow);
+    }
+
     @Override
     public final void close() throws SQLException {
         close(CompletionReason.OTHER);

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

@@ -296,33 +296,22 @@ public boolean isAfterLast() {
 
     @Override
     public void deleteRow() throws SQLException {
-        if (!isAfterLast() && !isBeforeFirst()) {
-            rows.remove(rowNum - 1);
-            notifyRowChanged(adjustIfPositionAfterLast() ? null : rows.get(rowNum - 1));
-        }
+        throw calledUndecorated();
     }
 
     @Override
     public void insertRow(RowValue data) throws SQLException {
-        if (rowNum == 0) {
-            rowNum = 1;
-        }
-
-        if (rowNum > rows.size()) {
-            rows.add(data);
-        } else {
-            rows.add(rowNum - 1, data);
-        }
-
-        notifyRowChanged(isAfterLast() || isBeforeFirst() ? null : rows.get(rowNum - 1));
+        throw calledUndecorated();
     }
 
     @Override
     public void updateRow(RowValue data) throws SQLException {
-        if (!isAfterLast() && !isBeforeFirst()) {
-            rows.set(rowNum - 1, data);
-            notifyRowChanged(data);
-        }
+        throw calledUndecorated();
+    }
+
+    private static UnsupportedOperationException calledUndecorated() {
+        return new UnsupportedOperationException(
+                "Implementation error: FBServerScrollFetcher should be decorated with FBUpdatableFetcher");
     }
 
     @Override

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

@@ -1487,43 +1487,35 @@ public ResultSet getIndexInfo(String catalog, String schema, String table, boole
 
     @Override
     public boolean supportsResultSetType(int type) throws SQLException {
-        // TODO Return false for TYPE_SCROLL_SENSITVE as we only support it by downgrading to INSENSITIVE?
+        // TYPE_SCROLL_SENSITIVE is always downgraded to TYPE_SCROLL_INSENSITIVE, so we report false for it
         return switch (type) {
-            case ResultSet.TYPE_FORWARD_ONLY, ResultSet.TYPE_SCROLL_INSENSITIVE, ResultSet.TYPE_SCROLL_SENSITIVE ->
-                    true;
+            case ResultSet.TYPE_FORWARD_ONLY, ResultSet.TYPE_SCROLL_INSENSITIVE -> true;
             default -> false;
         };
     }
 
     @Override
     public boolean supportsResultSetConcurrency(int type, int concurrency) throws SQLException {
-        // TODO Return false for TYPE_SCROLL_SENSITVE as we only support it by downgrading to INSENSITIVE?
         return switch (type) {
-            case ResultSet.TYPE_FORWARD_ONLY, ResultSet.TYPE_SCROLL_INSENSITIVE, ResultSet.TYPE_SCROLL_SENSITIVE ->
+            case ResultSet.TYPE_FORWARD_ONLY, ResultSet.TYPE_SCROLL_INSENSITIVE ->
                     concurrency == ResultSet.CONCUR_READ_ONLY || concurrency == ResultSet.CONCUR_UPDATABLE;
             default -> false;
         };
     }
 
     @Override
     public boolean ownUpdatesAreVisible(int type) throws SQLException {
-        // TODO Return false for TYPE_SCROLL_SENSITVE as we only support it by downgrading to INSENSITIVE?
-        return ResultSet.TYPE_SCROLL_INSENSITIVE == type ||
-                ResultSet.TYPE_SCROLL_SENSITIVE == type;
+        return ResultSet.TYPE_SCROLL_INSENSITIVE == type;
     }
 
     @Override
     public boolean ownDeletesAreVisible(int type) throws SQLException {
-        // TODO Return false for TYPE_SCROLL_SENSITVE as we only support it by downgrading to INSENSITIVE?
-        return ResultSet.TYPE_SCROLL_INSENSITIVE == type ||
-                ResultSet.TYPE_SCROLL_SENSITIVE == type;
+        return ResultSet.TYPE_SCROLL_INSENSITIVE == type;
     }
 
     @Override
     public boolean ownInsertsAreVisible(int type) throws SQLException {
-        // TODO Return false for TYPE_SCROLL_SENSITVE as we only support it by downgrading to INSENSITIVE?
-        return ResultSet.TYPE_SCROLL_INSENSITIVE == type ||
-                ResultSet.TYPE_SCROLL_SENSITIVE == type;
+        return ResultSet.TYPE_SCROLL_INSENSITIVE == type;
     }
 
     @Override
@@ -1543,23 +1535,20 @@ public boolean othersInsertsAreVisible(int type) throws SQLException {
 
     @Override
     public boolean updatesAreDetected(int type) throws SQLException {
-        // TODO Currently not correct when scrollableCursor=SERVER (and not a holdable cursor),
-        //  change to return true when behaviour of EMULATED is the same
-        return false;
+        // Only updates through the result set
+        return ResultSet.TYPE_SCROLL_INSENSITIVE == type;
     }
 
     @Override
     public boolean deletesAreDetected(int type) throws SQLException {
-        // TODO Currently not correct when scrollableCursor=SERVER (and not a holdable cursor),
-        //  change to return true when behaviour of EMULATED is the same
-        return false;
+        // Only deletes through the result set
+        return ResultSet.TYPE_SCROLL_INSENSITIVE == type;
     }
 
     @Override
     public boolean insertsAreDetected(int type) throws SQLException {
-        // TODO Currently not correct when scrollableCursor=SERVER (and not a holdable cursor),
-        //  change to return true when behaviour of EMULATED is the same
-        return false;
+        // Only inserts through the result set
+        return ResultSet.TYPE_SCROLL_INSENSITIVE == type;
     }
 
     @Override

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

@@ -201,6 +201,11 @@ default void beforeExecuteInsert() throws SQLException {
      */
     void updateRow(RowValue data) throws SQLException;
 
+    /**
+     * Notifies the fetcher listener with the row data of the current row (or {{@code null} if not currently in a row).
+     */
+    void renotifyCurrentRow() throws SQLException;
+
     /**
      * @return current fetch config of this fetcher
      * @since 6

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

@@ -140,7 +140,7 @@ public FBResultSet(AbstractStatement statement, FBObjectListener.@Nullable Resul
             if (behavior.isUpdatable()) {
                 try {
                     rowUpdater = new FBRowUpdater(connection, rowDescriptor, cached, listener);
-                    if (fbFetcher instanceof FBServerScrollFetcher) {
+                    if (behavior.isScrollable()) {
                         fbFetcher = new FBUpdatableFetcher(fbFetcher, this, rowDescriptor.createDeletedRowMarker());
                     }
                 } catch (FBResultSetNotUpdatableException ex) {
@@ -1482,6 +1482,9 @@ public void moveToInsertRow() throws SQLException {
     @Override
     public void moveToCurrentRow() throws SQLException {
         requireRowUpdater().moveToCurrentRow();
+        // Make sure we have the correct data of the row
+        fbFetcher.renotifyCurrentRow();
+        notifyRowUpdater();
     }
 
     @Override

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

@@ -265,7 +265,7 @@ public boolean absolute(int row) throws SQLException {
             checkOpen();
             // Overflow beyond cursor size is handled by inWindow returning false
             int newLocalPosition = row >= 0 ? row : Math.max(0, requireCursorSize() + 1 + row);
-            if (!inWindow(row)) {
+            if (!inWindow(newLocalPosition)) {
                 if (getMaxRows() != 0 && newLocalPosition > requireCursorSize()) {
                     afterLast();
                     return false;

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

@@ -36,8 +36,9 @@
 /**
  * Decorator that handles tracking updates, deletes and inserts of an updatable result set.
  * <p>
- * This fetcher handles the updatable result set behaviour defined in jdp-2021-04 for server-side scrollable cursors
- * it is not yet "supported" for emulated scrollable result sets.
+ * This fetcher handles the updatable result set behaviour defined in <a href="https://github.com/FirebirdSQL/jaybird/blob/master/devdoc/jdp/jdp-2021-04-real-scrollable-cursor-support.md">jdp-2021-04</a>
+ * for server-side scrollable cursors and <a href="https://github.com/FirebirdSQL/jaybird/blob/master/devdoc/jdp/jdp-2024-05-behavior-of-updatable-result-sets.adoc">jdp-2024-05</a>
+ * for emulated scrollable cursors.
  * </p>
  * <p>
  * This behaviour can be summarized as: updates are visible, deletes are visible (with a deletion marker row),
@@ -299,6 +300,17 @@ public void updateRow(RowValue data) throws SQLException {
         fetcherListener.rowChanged(this, data);
     }
 
+    @Override
+    public void renotifyCurrentRow() throws SQLException {
+        int position = this.position;
+        // We can reuse the lastReceivedRow of the fetcher listener if the current row is not stored in this fetcher
+        if (position <= fetcherSize()) {
+            notifyFetcherRow(position);
+        } else {
+            notifyInsertedRow(position);
+        }
+    }
+
     @Override
     public int getFetchSize() throws SQLException {
         return fetcher.getFetchSize();

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

@@ -154,6 +154,11 @@ public void updateRow(RowValue data) throws SQLException {
         fetcher.updateRow(data);
     }
 
+    @Override
+    public void renotifyCurrentRow() throws SQLException {
+        fetcher.renotifyCurrentRow();
+    }
+
     @Override
     public int getFetchSize() throws SQLException {
         return fetcher.getFetchSize();