diff --git a/src/docs/asciidoc/release_notes.adoc b/src/docs/asciidoc/release_notes.adoc index 1de7105cc..8d1d2096c 100644 --- a/src/docs/asciidoc/release_notes.adoc +++ b/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 diff --git a/src/main/org/firebirdsql/jdbc/FBFetcher.java b/src/main/org/firebirdsql/jdbc/FBFetcher.java index 4ad39256d..afe564766 100644 --- a/src/main/org/firebirdsql/jdbc/FBFetcher.java +++ b/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. + *
+ * 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. + *
+ *+ * The default implementation of this method does nothing. + *
+ * + * @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 diff --git a/src/main/org/firebirdsql/jdbc/FBResultSet.java b/src/main/org/firebirdsql/jdbc/FBResultSet.java index 0e673243c..1379fd166 100644 --- a/src/main/org/firebirdsql/jdbc/FBResultSet.java +++ b/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(); diff --git a/src/main/org/firebirdsql/jdbc/FBServerScrollFetcher.java b/src/main/org/firebirdsql/jdbc/FBServerScrollFetcher.java index 3d25fb31e..e03c08100 100644 --- a/src/main/org/firebirdsql/jdbc/FBServerScrollFetcher.java +++ b/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,7 +382,7 @@ 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; } @@ -386,7 +390,7 @@ public int getRowNum() throws SQLException { @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,7 +399,7 @@ public boolean isEmpty() throws SQLException { @Override public boolean isBeforeFirst() throws SQLException { - try (LockCloseable ignored = stmt.withLock()) { + try (LockCloseable ignored = withLock()) { checkOpen(); return localPosition == 0; } @@ -403,7 +407,7 @@ public boolean isBeforeFirst() throws SQLException { @Override public boolean isFirst() throws SQLException { - try (LockCloseable ignored = stmt.withLock()) { + try (LockCloseable ignored = withLock()) { checkOpen(); return localPosition == 1 && requireCursorSize() > 0; } @@ -411,7 +415,7 @@ public boolean isFirst() throws SQLException { @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,7 +424,7 @@ 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(); @@ -428,6 +432,11 @@ public boolean isAfterLast() throws SQLException { } } + @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,7 +458,7 @@ private int actualFetchSize() { @Override public int getFetchSize() throws SQLException { - try (LockCloseable ignored = stmt.withLock()) { + try (LockCloseable ignored = withLock()) { checkOpen(); return fetchSize; } @@ -457,7 +466,7 @@ public int getFetchSize() throws SQLException { @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 { diff --git a/src/main/org/firebirdsql/jdbc/FBUpdatableFetcher.java b/src/main/org/firebirdsql/jdbc/FBUpdatableFetcher.java index 9b137377e..5fff1bee4 100644 --- a/src/main/org/firebirdsql/jdbc/FBUpdatableFetcher.java +++ b/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); diff --git a/src/test/org/firebirdsql/common/assertions/ResultSetAssertions.java b/src/test/org/firebirdsql/common/assertions/ResultSetAssertions.java new file mode 100644 index 000000000..7ebce6d73 --- /dev/null +++ b/src/test/org/firebirdsql/common/assertions/ResultSetAssertions.java @@ -0,0 +1,336 @@ +/* + * Firebird Open Source JDBC Driver + * + * Distributable under LGPL license. + * You may obtain a copy of the License at http://www.gnu.org/copyleft/lgpl.html + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * LGPL License for more details. + * + * This file was created by members of the firebird development team. + * All individual contributions remain the Copyright (C) of those + * individuals. Contributors to this file are either listed here or + * can be obtained from a source control history command. + * + * All rights reserved. + */ +package org.firebirdsql.common.assertions; + +import org.junit.jupiter.api.function.ThrowingSupplier; + +import java.sql.ResultSet; +import java.sql.ResultSetMetaData; +import java.util.Arrays; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Various assertions for result set. + * + * @author Mark Rotteveel + * @since 6 + */ +public final class ResultSetAssertions { + + private ResultSetAssertions() { + // no instances + } + + /** + * Asserts that {@code rs} has a next row by checking if {@link ResultSet#next()} returned {@code true}. + *+ * Equivalent to using {@link #assertNextRow(ResultSet, String)} with message {@code "Expected a row"}. + *
+ * + * @param rs + * result set + * @see #assertNextRow(ResultSet, String) + */ + public static void assertNextRow(ResultSet rs) { + assertNextRow(rs, "Expected a row"); + } + + /** + * Asserts that {@code rs} has a next row by checking if {@link ResultSet#next()} returned {@code true}. + * + * @param rs + * result set + * @param message + * message to use for the assertion if {@link ResultSet#next()} returned {@code false} + */ + public static void assertNextRow(ResultSet rs, String message) { + assertTrue(assertDoesNotThrow(rs::next, "No exception expected for ResultSet.next()"), message); + } + + /** + * Asserts that {@code rs} has no next row ({@link ResultSet#next()} returned {@code false}). + * + * @param rs + * result set + * @see #assertNoNextRow(ResultSet, String) + */ + public static void assertNoNextRow(ResultSet rs) { + assertNoNextRow(rs, "Expected no more rows"); + } + + /** + * Asserts that {@code rs} has no next row ({@link ResultSet#next()} returned {@code false}). + * + * @param rs + * result set + * @param message + * message to use for the assertion if {@link ResultSet#next()} returned {@code true} + */ + public static void assertNoNextRow(ResultSet rs, String message) { + assertFalse(assertNoException(rs::next, "ResultSet.next()"), message); + } + + /** + * Asserts that {@code rs} is open ({@link ResultSet#isClosed()} returned {@code false}) + * + * @param rs + * result set + * @see #assertResultSetOpen(ResultSet, String) + */ + public static void assertResultSetOpen(ResultSet rs) { + assertResultSetOpen(rs, "Expected open result set, was closed"); + } + + /** + * Asserts that {@code rs} is open ({@link ResultSet#isClosed()} returned {@code false}) + * + * @param rs + * result set + * @param message + * message to use for the assertion if {@link ResultSet#isClosed()} returned {@code true} + */ + public static void assertResultSetOpen(ResultSet rs, String message) { + assertFalse(assertNoException(rs::isClosed, "ResultSet.isClosed()"), message); + } + + /** + * Asserts that {@code rs} is closed ({@link ResultSet#isClosed()} returned {@code true}) + * + * @param rs + * result set + * @see #assertResultSetClosed(ResultSet, String) + */ + public static void assertResultSetClosed(ResultSet rs) { + assertResultSetClosed(rs, "Expected closed result set, was open"); + } + + /** + * Asserts that {@code rs} is closed ({@link ResultSet#isClosed()} returned {@code true}) + * + * @param rs + * result set + * @param message + * message to use for the assertion if {@link ResultSet#isClosed()} returned {@code false} + */ + public static void assertResultSetClosed(ResultSet rs, String message) { + assertTrue(assertNoException(rs::isClosed, "ResultSet.isClosed()"), message); + } + + /** + * Asserts that {@code rs} is before first ({@link ResultSet#isBeforeFirst()} returns {@code true}). + * + * @param rs + * result set + * @see #assertBeforeFirst(ResultSet, String) + */ + public static void assertBeforeFirst(ResultSet rs) { + assertBeforeFirst(rs, "Expected ResultSet.isBeforeFirst == true"); + } + + /** + * Asserts that {@code rs} is before first ({@link ResultSet#isBeforeFirst()} returns {@code true}). + * + * @param rs + * result set + * @param message + * message to use for the assertion if {@link ResultSet#isBeforeFirst()} returned {@code false} + */ + public static void assertBeforeFirst(ResultSet rs, String message) { + assertTrue(assertNoException(rs::isBeforeFirst, "ResultSet.isBeforeFirst()"), message); + } + + /** + * Asserts that {@code rs} is not before first ({@link ResultSet#isBeforeFirst()} returns {@code false}). + * + * @param rs + * result set + * @see #assertNotBeforeFirst(ResultSet, String) + */ + public static void assertNotBeforeFirst(ResultSet rs) { + assertNotBeforeFirst(rs, "Expected ResultSet.isBeforeFirst == false"); + } + + /** + * Asserts that {@code rs} is not before first ({@link ResultSet#isBeforeFirst()} returns {@code false}). + * + * @param rs + * result set + * @param message + * message to use for the assertion if {@link ResultSet#isBeforeFirst()} returned {@code true} + */ + public static void assertNotBeforeFirst(ResultSet rs, String message) { + assertFalse(assertNoException(rs::isBeforeFirst, "ResultSet.isBeforeFirst()"), message); + } + + /** + * Asserts that {@code rs} is after last ({@link ResultSet#isAfterLast()} returns {@code true}). + * + * @param rs + * result set + * @see #assertAfterLast(ResultSet, String) + */ + public static void assertAfterLast(ResultSet rs) { + assertAfterLast(rs, "Expected ResultSet.isAfterLast == true"); + } + + /** + * Asserts that {@code rs} is after last ({@link ResultSet#isAfterLast()} returns {@code true}). + * + * @param rs + * result set + * @param message + * message to use for the assertion if {@link ResultSet#isAfterLast()} returned {@code false} + */ + public static void assertAfterLast(ResultSet rs, String message) { + assertTrue(assertNoException(rs::isAfterLast, "ResultSet.isAfterLast()"), message); + } + + /** + * Asserts that {@code rs} is not after last ({@link ResultSet#isAfterLast()} returns {@code false}). + * + * @param rs + * result set + * @see #assertNotAfterLast(ResultSet, String) + */ + public static void assertNotAfterLast(ResultSet rs) { + assertNotAfterLast(rs, "Expected ResultSet.isAfterLast == false"); + } + + /** + * Asserts that {@code rs} is not after last ({@link ResultSet#isAfterLast()} returns {@code false}). + * + * @param rs + * result set + * @param message + * message to use for the assertion if {@link ResultSet#isAfterLast()} returned {@code true} + */ + public static void assertNotAfterLast(ResultSet rs, String message) { + assertFalse(assertNoException(rs::isAfterLast, "ResultSet.isAfterLast()"), message); + } + + /** + * Asserts that the current row of the result set matches in length and values. + * + * @param rs + * result set + * @param expectedValues + * expected values + * @see #assertRowEquals(String, ResultSet, List) + */ + public static void assertRowEquals(ResultSet rs, Object... expectedValues) { + assertRowEquals(rs, Arrays.asList(expectedValues)); + } + + /** + * Asserts that the current row of the result set matches in length and values. + * + * @param message + * message to use for assertion failures + * @param rs + * result set + * @param expectedValues + * expected values + * @see #assertRowEquals(String, ResultSet, List) + */ + public static void assertRowEquals(String message, ResultSet rs, Object... expectedValues) { + assertRowEquals(message, rs, Arrays.asList(expectedValues)); + } + + /** + * Asserts that the current row of the result set matches in length and values. + * + * @param rs + * result set + * @param expectedValues + * expected values + * @see #assertRowEquals(String, ResultSet, List) + */ + public static void assertRowEquals(ResultSet rs, List