Skip to content

Commit

Permalink
#781 Backport: FBRowUpdater incorrectly considers result set with onl…
Browse files Browse the repository at this point in the history 
…y partial PK updatable
  • Loading branch information
@mrotteveel
mrotteveel committed Jan 13, 2024
1 parent c632af7 commit 22e8d25
Show file tree
Hide file tree
Showing 3 changed files with 93 additions and 42 deletions.
5 changes: 5 additions & 0 deletions src/docs/asciidoc/release_notes.adoc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ The following has been changed or fixed since Jaybird 5.0.3:
+
As part of this change, the constructor `AbstractWireOperations` now explicitly requires the `connection` and `defaultWarningMessageCallback` to be non-``null`` and throws a `NullPointerException` otherwise.
This may affect custom protocol implementations extending `AbstractWireOperations` and/or calling `ProtocolDescriptor#createWireOperations(WireConnection, WarningMessageCallback)`: make sure you don't pass `null`.
* Fixed: FBRowUpdater incorrectly considers result set with only partial PK updatable -- backported from Jaybird 6 (https://github.com/FirebirdSQL/jaybird/issues/780[#780])
+
This change also improves performance of `updateRow()`, `insertRow()`, `deleteRow()` and `refreshRow()`.
The best row identifier or `RDB$DB_KEY` were detected _each time_ when calling `updateRow()`, `insertRow()`, `deleteRow()`, or `refreshRow()`.
This has been improved so this detection is done once, and in a way that non-updatable result sets can now be downgraded to `CONCUR_READ_ONLY` instead of throwing an exception when performing the modification.
* ...
[#jaybird-5-0-3-changelog]
Expand Down
78 changes: 36 additions & 42 deletions src/main/org/firebirdsql/jdbc/FBRowUpdater.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ final class FBRowUpdater implements FirebirdRowUpdater {
private static final int PARAMETER_DBKEY = 2;
private static final byte[][] EMPTY_2D_BYTES = new byte[0][];

private final FBConnection connection;
private final GDSHelper gdsHelper;
private final RowDescriptor rowDescriptor;
private final FBField[] fields;
Expand All @@ -81,6 +80,7 @@ final class FBRowUpdater implements FirebirdRowUpdater {
private RowValue oldRow;
private RowValue insertRow;
private boolean[] updatedFlags;
private final int[] parameterMask;

private String tableName;

Expand All @@ -92,19 +92,27 @@ final class FBRowUpdater implements FirebirdRowUpdater {

FBRowUpdater(FBConnection connection, RowDescriptor rowDescriptor, boolean cached,
FBObjectListener.ResultSetListener rsListener) throws SQLException {
this.rsListener = rsListener;
// find the table name (there can be only one table per result set)
for (FieldDescriptor fieldDescriptor : rowDescriptor) {
if (tableName == null) {
tableName = fieldDescriptor.getOriginalTableName();
} else if (!tableName.equals(fieldDescriptor.getOriginalTableName())) {
throw new FBResultSetNotUpdatableException(
"Underlying result set references at least two relations: " +
tableName + " and " + fieldDescriptor.getOriginalTableName() + ".");
}
}
parameterMask = getParameterMask(tableName, rowDescriptor, connection.getMetaData());

this.connection = connection;
this.rsListener = rsListener;
gdsHelper = connection.getGDSHelper();

this.rowDescriptor = rowDescriptor;
fields = new FBField[rowDescriptor.getCount()];

quoteStrategy = QuoteStrategy.forDialect(gdsHelper.getDialect());

this.rowDescriptor = rowDescriptor;
newRow = rowDescriptor.createDefaultFieldValues();
updatedFlags = new boolean[rowDescriptor.getCount()];

fields = new FBField[rowDescriptor.getCount()];
for (int i = 0; i < rowDescriptor.getCount(); i++) {
final int fieldPos = i;

Expand Down Expand Up @@ -134,17 +142,6 @@ public void setFieldData(byte[] data) {

fields[i] = FBField.createField(rowDescriptor.getFieldDescriptor(i), dataProvider, gdsHelper, cached);
}

// find the table name (there can be only one table per result set)
for (FieldDescriptor fieldDescriptor : rowDescriptor) {
if (tableName == null) {
tableName = fieldDescriptor.getOriginalTableName();
} else if (!tableName.equals(fieldDescriptor.getOriginalTableName())) {
throw new FBResultSetNotUpdatableException(
"Underlying result set references at least two relations: " +
tableName + " and " + fieldDescriptor.getOriginalTableName() + ".");
}
}
}

private void notifyExecutionStarted() throws SQLException {
Expand Down Expand Up @@ -222,40 +219,39 @@ public FBField getField(int fieldPosition) {
*
* @return array of booleans that represent parameter mask.
*/
private int[] getParameterMask() throws SQLException {
private static int[] getParameterMask(String tableName, RowDescriptor rowDescriptor, DatabaseMetaData dbmd)
throws SQLException {
// loop through the "best row identifiers" and set appropriate flags.
FBDatabaseMetaData metaData = (FBDatabaseMetaData) connection.getMetaData();

try (ResultSet bestRowIdentifier = metaData.getBestRowIdentifier("", "", tableName,
try (ResultSet bestRowIdentifier = dbmd.getBestRowIdentifier("", "", tableName,
DatabaseMetaData.bestRowTransaction, true)) {
int[] result = new int[rowDescriptor.getCount()];
boolean hasParams = false;
while (bestRowIdentifier.next()) {
String columnName = bestRowIdentifier.getString(2);
if (columnName == null) continue;

if (columnName == null)
continue;

boolean found = false;
for (int i = 0; i < rowDescriptor.getCount(); i++) {
// special handling for the RDB$DB_KEY columns that must be
// selected as RDB$DB_KEY, but in XSQLVAR are represented
// as DB_KEY
if ("RDB$DB_KEY".equals(columnName) && rowDescriptor.getFieldDescriptor(i).isDbKey()) {
result[i] = PARAMETER_DBKEY;
hasParams = true;
found = true;
} else if (columnName.equals(rowDescriptor.getFieldDescriptor(i).getOriginalName())) {
result[i] = PARAMETER_USED;
hasParams = true;
found = true;
}
}

// if we did not find a column from the best row identifier
// in our result set, throw an exception, since we cannot
// reliably identify the row.
if (!hasParams)
if (!found) {
throw new FBResultSetNotUpdatableException(
"Underlying result set does not contain all columns " +
"that form 'best row identifier'.");
"Underlying result set does not contain all columns that form 'best row identifier'.");
}
hasParams = true;
}

if (!hasParams)
Expand All @@ -266,7 +262,7 @@ private int[] getParameterMask() throws SQLException {
}
}

private void appendWhereClause(StringBuilder sb, int[] parameterMask) {
private void appendWhereClause(StringBuilder sb) {
sb.append("WHERE ");

// handle the RDB$DB_KEY case first
Expand Down Expand Up @@ -299,7 +295,7 @@ private void appendWhereClause(StringBuilder sb, int[] parameterMask) {
}
}

private String buildUpdateStatement(int[] parameterMask) {
private String buildUpdateStatement() {
StringBuilder sb = new StringBuilder("UPDATE ");
quoteStrategy.appendQuoted(tableName, sb)
.append("\nSET\n");
Expand All @@ -319,15 +315,15 @@ private String buildUpdateStatement(int[] parameterMask) {
}

sb.append('\n');
appendWhereClause(sb, parameterMask);
appendWhereClause(sb);

return sb.toString();
}

private String buildDeleteStatement(int[] parameterMask) {
private String buildDeleteStatement() {
StringBuilder sb = new StringBuilder("DELETE FROM ");
quoteStrategy.appendQuoted(tableName, sb).append('\n');
appendWhereClause(sb, parameterMask);
appendWhereClause(sb);

return sb.toString();
}
Expand Down Expand Up @@ -360,7 +356,7 @@ private String buildInsertStatement() {
return sb.toString();
}

private String buildSelectStatement(int[] parameterMask) {
private String buildSelectStatement() {
StringBuilder columns = new StringBuilder();

boolean first = true;
Expand All @@ -382,7 +378,7 @@ private String buildSelectStatement(int[] parameterMask) {
sb.append(columns).append('\n')
.append("FROM ");
quoteStrategy.appendQuoted(tableName, sb).append('\n');
appendWhereClause(sb, parameterMask);
appendWhereClause(sb);
return sb.toString();
}

Expand Down Expand Up @@ -487,24 +483,22 @@ private void executeStatement(int statementType, FbStatement stmt) throws SQLExc
((FBFlushableField) fields[i]).flushCachedData();
}

int[] parameterMask = getParameterMask();

String sql;
switch (statementType) {
case UPDATE_STATEMENT_TYPE:
sql = buildUpdateStatement(parameterMask);
sql = buildUpdateStatement();
break;

case DELETE_STATEMENT_TYPE:
sql = buildDeleteStatement(parameterMask);
sql = buildDeleteStatement();
break;

case INSERT_STATEMENT_TYPE:
sql = buildInsertStatement();
break;

case SELECT_STATEMENT_TYPE:
sql = buildSelectStatement(parameterMask);
sql = buildSelectStatement();
break;

default:
Expand Down
52 changes: 52 additions & 0 deletions src/test/org/firebirdsql/jdbc/FBResultSetTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,14 @@ class FBResultSetTest {
" \"CamelStr\" VARCHAR(255)" +
")";

private static final String CREATE_WITH_COMPOSITE_PK =
"create table WITH_COMPOSITE_PK (\n" +
" ID1 integer not null,\n" +
" ID2 integer not null,\n" +
" VAL varchar(50),\n" +
" constraint PK_WITH_COMPOSITE_PK primary key (ID1, ID2)\n" +
")";

private static final String CREATE_VIEW_STATEMENT =
"CREATE VIEW test_empty_string_view(marker, id, empty_char) " +
" AS " +
Expand Down Expand Up @@ -705,6 +713,50 @@ void testUpdatableStatementResultSetDowngradeToReadOnlyWhenQueryNotUpdatable(
}
}

/**
* Tests if a result set that doesn't contain all PK columns (but only a prefix) will be downgraded to read-only.
* <p>
* Rationale: a previous implementation was satisfied if at least the first PK column was found
* </p>
*/
@ParameterizedTest
@MethodSource("scrollableCursorPropertyValues")
void testUpdatableStatementPrefixPK_downgradeToReadOnly(String scrollableCursorPropertyValue) throws Exception {
try (Connection connection = createConnection(scrollableCursorPropertyValue)) {
executeCreateTable(connection, CREATE_WITH_COMPOSITE_PK);
try (var stmt = connection.createStatement(TYPE_SCROLL_INSENSITIVE, CONCUR_UPDATABLE);
var rs = stmt.executeQuery("select id1, val from WITH_COMPOSITE_PK")) {
assertThat(stmt.getWarnings(), allOf(
notNullValue(),
fbMessageStartsWith(JaybirdErrorCodes.jb_concurrencyResetReadOnlyReasonNotUpdatable)));

assertEquals(CONCUR_READ_ONLY, rs.getConcurrency(), "Expected downgrade to CONCUR_READ_ONLY");
}
}
}

/**
* Tests if a result set that doesn't contain all PK columns (but only a suffix) will be downgraded to read-only.
* <p>
* Rationale: a previous implementation was satisfied if at least the first PK column was found
* </p>
*/
@ParameterizedTest
@MethodSource("scrollableCursorPropertyValues")
void testUpdatableStatementSuffixPK_downgradeToReadOnly(String scrollableCursorPropertyValue) throws Exception {
try (Connection connection = createConnection(scrollableCursorPropertyValue)) {
executeCreateTable(connection, CREATE_WITH_COMPOSITE_PK);
try (var stmt = connection.createStatement(TYPE_SCROLL_INSENSITIVE, CONCUR_UPDATABLE);
var rs = stmt.executeQuery("select id2, val from WITH_COMPOSITE_PK")) {
assertThat(stmt.getWarnings(), allOf(
notNullValue(),
fbMessageStartsWith(JaybirdErrorCodes.jb_concurrencyResetReadOnlyReasonNotUpdatable)));

assertEquals(CONCUR_READ_ONLY, rs.getConcurrency(), "Expected downgrade to CONCUR_READ_ONLY");
}
}
}

@Test
void testGetExecutionPlan() throws Exception {
try (Connection connection = getConnectionViaDriverManager()) {
Expand Down

0 comments on commit 22e8d25

Please sign in to comment.