diff --git a/src/docs/asciidoc/release_notes.adoc b/src/docs/asciidoc/release_notes.adoc index 58ee64159..aa10033fb 100644 --- a/src/docs/asciidoc/release_notes.adoc +++ b/src/docs/asciidoc/release_notes.adoc @@ -851,13 +851,6 @@ This contradiction has been removed, and the implementations will now send array // TODO add major changes -[#potentially-breaking-changes] -=== Potentially breaking changes - -Jaybird 6 contains a number of changes that might break existing applications. - -See also <> for details. - [#other-fixes-and-changes] === Other fixes and changes @@ -917,6 +910,24 @@ The modified implementation returns an empty array, but given this is unspecifie This change was also backported to Jaybird 4.0.10 and Jaybird 5.0.3. * `FBResultSetNotUpdatableException` now extends `SQLNonTransientException` instead of `FBSQLException`. * Jaybird no longer throws any instances of `FBSQLException`. +* Fixed: FBRowUpdater incorrectly considers result set with only partial PK updatable (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. ++ +This change was backported to Jaybird 5.0.4. +* Improved detection of (non-)updatable result sets. ++ +If the best row identifier was not matched, but the result set contains `RDB$DB_KEY`, we will now consider the result set updatable. +However, if the table in question has a primary key, and the columns missing from the result set are not generated, this may still fail when calling `ResultSet.insertRow()`. + +[#potentially-breaking-changes] +=== Potentially breaking changes + +Jaybird 6 contains a number of changes that might break existing applications. + +See also <> for details. [#removal-of-deprecated-classes-and-packages] === Removal of deprecated classes and packages diff --git a/src/main/org/firebirdsql/gds/ng/fields/FieldDescriptor.java b/src/main/org/firebirdsql/gds/ng/fields/FieldDescriptor.java index b8b1260dc..246ae9d54 100644 --- a/src/main/org/firebirdsql/gds/ng/fields/FieldDescriptor.java +++ b/src/main/org/firebirdsql/gds/ng/fields/FieldDescriptor.java @@ -32,6 +32,7 @@ import java.util.Objects; import static org.firebirdsql.gds.ISCConstants.*; +import static org.firebirdsql.jaybird.util.StringUtils.trimToNull; /** * The class {@code FieldDescriptor} contains the column metadata of the XSQLVAR server @@ -56,7 +57,10 @@ public final class FieldDescriptor { private final String originalName; private final String originalTableName; private final String ownerName; + + // cached data derived from immutable state private int hash; + private byte dbKey; /** * Constructor for metadata FieldDescriptor. @@ -85,11 +89,11 @@ public final class FieldDescriptor { * @param ownerName * Owner of the column/table */ + @SuppressWarnings("java:S107") public FieldDescriptor(int position, DatatypeCoder datatypeCoder, int type, int subType, int scale, int length, String fieldName, String tableAlias, String originalName, String originalTableName, String ownerName) { - assert datatypeCoder != null : "dataTypeCoder should not be null"; this.position = position; this.datatypeCoder = datatypeCoderForType(datatypeCoder, type, subType, scale); this.type = type; @@ -99,14 +103,20 @@ public FieldDescriptor(int position, DatatypeCoder datatypeCoder, this.fieldName = fieldName; // Assign null if table alias is empty string // TODO May want to do the reverse, or handle this better; see FirebirdResultSetMetaData contract - this.tableAlias = tableAlias == null || !tableAlias.isEmpty() ? tableAlias : null; + this.tableAlias = trimToNull(tableAlias); this.originalName = originalName; this.originalTableName = originalTableName; this.ownerName = ownerName; } /** - * @return The 0-based position of this field (or {@code -1}) + * The position of the field in the row or parameter set. + *

+ * In general this should be equal to the position of this descriptor in {@link RowDescriptor}, but in some cases + * (usually test code), it might be {@code -1} instead. + *

+ * + * @return The 0-based position of this field in the row or parameter set (or {@code -1} if unknown) */ public int getPosition() { return position; @@ -215,7 +225,7 @@ public boolean isFbType(int fbType) { * @return {@code true} if this field is nullable. */ public boolean isNullable() { - return (type & 1) == 1; + return (type & 1) != 0; } /** @@ -228,9 +238,13 @@ public boolean isNullable() { * @since 4.0 */ public boolean isDbKey() { - return "DB_KEY".equals(originalName) - && isFbType(SQL_TEXT) - && (subType & 0xFF) == CS_BINARY; + return dbKey > 0 || dbKey == 0 && isDbKey0(); + } + + private boolean isDbKey0() { + dbKey = ("DB_KEY".equals(originalName) && isFbType(SQL_TEXT) && (subType & 0xFF) == CS_BINARY) + ? (byte) 1 : -1; + return dbKey > 0; } /** @@ -242,17 +256,15 @@ && isFbType(SQL_TEXT) * @return Character length, or {@code -1} for non-character types (including blobs) */ public int getCharacterLength() { - switch (type & ~1) { - case SQL_TEXT: - case SQL_VARYING: { - int maxBytesPerChar = getDatatypeCoder().getEncodingDefinition().getMaxBytesPerChar(); - // In Firebird 1.5 and earlier, the CHAR(31) metadata columns are reported with a byte length of 31, - // while UNICODE_FSS has maxBytesPerChar 3 - return maxBytesPerChar > 1 && length % maxBytesPerChar == 0 ? length / maxBytesPerChar : length; - } - default: - return -1; - } + return switch (type & ~1) { + case SQL_TEXT, SQL_VARYING -> { + int maxBytesPerChar = getDatatypeCoder().getEncodingDefinition().getMaxBytesPerChar(); + // In Firebird 1.5 and earlier, the CHAR(31) metadata columns are reported with a byte length of 31, + // while UNICODE_FSS has maxBytesPerChar 3 + yield maxBytesPerChar > 1 && length % maxBytesPerChar == 0 ? length / maxBytesPerChar : length; + } + default -> -1; + }; } /** @@ -291,20 +303,18 @@ private static DatatypeCoder datatypeCoderForType(DatatypeCoder datatypeCoder, i * is returned */ private static int getCharacterSetId(int type, int subType, int scale) { - switch (type & ~1) { - case SQL_TEXT: - case SQL_VARYING: - return subType & 0xFF; - case SQL_BLOB: - if (subType == BLOB_SUB_TYPE_TEXT) { - return scale & 0xFF; + return switch (type & ~1) { + case SQL_TEXT, SQL_VARYING -> subType & 0xFF; + case SQL_BLOB -> { + if (subType == BLOB_SUB_TYPE_TEXT) { + yield scale & 0xFF; + } + // Assume binary/octets (instead of NONE) + yield CS_BINARY; } - // Assume binary/octets (instead of NONE) - return CS_BINARY; - default: // Technically not a character type, but assume connection character set - return CS_dynamic; - } + default -> CS_dynamic; + }; } /** @@ -351,21 +361,21 @@ public boolean typeEquals(final FieldDescriptor other) { * @since 5 */ public byte getPaddingByte() { - switch (type & ~1) { - case SQL_TEXT: - case SQL_VARYING: - if (getCharacterSetId() == CS_BINARY) { - return 0x00; + return switch (type & ~1) { + case SQL_TEXT, SQL_VARYING -> { + if (getCharacterSetId() == CS_BINARY) { + yield 0x00; + } + yield 0x20; } - return 0x20; - default: - return 0x00; - } + default -> 0x00; + }; } @Override public String toString() { - StringBuilder sb = new StringBuilder(); + // 180 - 124 literals in appendFieldDescriptor + 56 for values (estimated size) + var sb = new StringBuilder(180); appendFieldDescriptor(sb); return sb.toString(); } @@ -388,8 +398,7 @@ void appendFieldDescriptor(final StringBuilder sb) { @Override public boolean equals(final Object obj) { if (this == obj) return true; - if (!(obj instanceof FieldDescriptor)) return false; - FieldDescriptor other = (FieldDescriptor) obj; + if (!(obj instanceof FieldDescriptor other)) return false; return this.position == other.position && this.type == other.type && this.subType == other.subType @@ -406,11 +415,10 @@ public boolean equals(final Object obj) { @Override public int hashCode() { // Depend on immutability to cache hashCode - if (hash == 0) { - hash = Objects.hash(position, type, subType, scale, length, fieldName, tableAlias, originalName, - originalTableName, ownerName); - } - return hash; + if (hash != 0) return hash; + int newHash = Objects.hash(position, type, subType, scale, length, fieldName, tableAlias, originalName, + originalTableName, ownerName); + return hash = newHash != 0 ? newHash : 1; } } diff --git a/src/main/org/firebirdsql/gds/ng/fields/RowValue.java b/src/main/org/firebirdsql/gds/ng/fields/RowValue.java index c46f1dae3..467d5c7ed 100644 --- a/src/main/org/firebirdsql/gds/ng/fields/RowValue.java +++ b/src/main/org/firebirdsql/gds/ng/fields/RowValue.java @@ -21,7 +21,7 @@ import java.util.Arrays; /** - * Collection of values of fields. Usually a row or set of parameters. + * Collection of values of fields. Usually row or parameter values. * * @author Mark Rotteveel * @since 3.0 @@ -96,6 +96,8 @@ public final byte[] getFieldData(int index) { /** * Resets the state of this row value to uninitialized. + * + * @since 4 */ public void reset() { Arrays.fill(fieldData, NOT_INITIALIZED); @@ -108,6 +110,7 @@ public void reset() { *

* * @return {@code true} if this a deleted row marker, {@code false} otherwise + * @since 5 */ public boolean isDeletedRowMarker() { return false; @@ -115,6 +118,8 @@ public boolean isDeletedRowMarker() { /** * Initializes uninitialized fields with {@code null}. + * + * @since 5 */ public final void initializeFields() { for (int idx = 0; idx < fieldData.length; idx++) { @@ -132,24 +137,37 @@ public final void initializeFields() { * @return {@code true} if the field is initialized * @throws IndexOutOfBoundsException * if index is not {@code 0 <= index > getCount()} + * @since 4 */ public final boolean isInitialized(int index) { return fieldData[index] != NOT_INITIALIZED; } + /** + * @return number of initialized fields in this row value + * @since 6 + */ + public final int initializedCount() { + int count = 0; + for (byte[] fieldDatum : fieldData) { + if (fieldDatum != NOT_INITIALIZED) { + count++; + } + } + return count; + } + /** * Convenience method for creating a default, uninitialized, row value for a {@link RowDescriptor}. * * @param rowDescriptor * The row descriptor * @return {@code RowValue} object + * @since 4 */ public static RowValue defaultFor(RowDescriptor rowDescriptor) { int count = rowDescriptor.getCount(); - if (count == 0) { - return EMPTY_ROW_VALUE; - } - return new RowValue(count, true); + return count == 0 ? EMPTY_ROW_VALUE : new RowValue(count, true); } /** @@ -193,6 +211,7 @@ public static RowValue of(RowDescriptor rowDescriptor, byte[]... rowData) { * * @param count The number of columns * @return {@code RowValue} object + * @since 5 */ public static RowValue deletedRowMarker(int count) { return new DeletedRowMarker(count); @@ -242,10 +261,7 @@ public final RowValue deepCopy() { RowValue newRowValue = new RowValue(size, false); for (int idx = 0; idx < size; idx++) { byte[] value = fieldData[idx]; - if (value != null && value != NOT_INITIALIZED) { - value = value.clone(); - } - newRowValue.fieldData[idx] = value; + newRowValue.fieldData[idx] = value == null || value == NOT_INITIALIZED ? value : value.clone(); } return newRowValue; } diff --git a/src/main/org/firebirdsql/jaybird/util/StringUtils.java b/src/main/org/firebirdsql/jaybird/util/StringUtils.java index d564f0225..cd5f30843 100644 --- a/src/main/org/firebirdsql/jaybird/util/StringUtils.java +++ b/src/main/org/firebirdsql/jaybird/util/StringUtils.java @@ -22,6 +22,7 @@ * Helper class for string operations * * @author Mark Rotteveel + * @since 4 */ public final class StringUtils { @@ -34,7 +35,7 @@ private StringUtils() { * or empty after trimming. * * @param value - * Value to trim + * value to trim * @return Trimmed string {@code value}, or {@code null} when null, or empty after trim. * @see String#trim() */ @@ -47,4 +48,17 @@ public static String trimToNull(String value) { } return null; } + + /** + * Checks if {@code value} is {@code null} or empty. + * + * @param value + * value to test + * @return {@code true} if {@code value} is {@code null} or emoty, {@code false} for non-empty strings + * @since 6 + */ + public static boolean isNullOrEmpty(String value) { + return value == null || value.isEmpty(); + } + } diff --git a/src/main/org/firebirdsql/jdbc/FBRowUpdater.java b/src/main/org/firebirdsql/jdbc/FBRowUpdater.java index dcbc21693..c5a23faac 100644 --- a/src/main/org/firebirdsql/jdbc/FBRowUpdater.java +++ b/src/main/org/firebirdsql/jdbc/FBRowUpdater.java @@ -26,6 +26,7 @@ import org.firebirdsql.gds.ng.fields.RowValue; import org.firebirdsql.gds.ng.listeners.StatementListener; import org.firebirdsql.jaybird.util.SQLExceptionChainBuilder; +import org.firebirdsql.jaybird.util.UncheckedSQLException; import org.firebirdsql.jdbc.field.FBField; import org.firebirdsql.jdbc.field.FBFlushableField; import org.firebirdsql.jdbc.field.FieldDataProvider; @@ -35,7 +36,10 @@ import java.sql.SQLException; import java.util.ArrayList; import java.util.List; +import java.util.Objects; +import java.util.stream.StreamSupport; +import static org.firebirdsql.jaybird.util.StringUtils.isNullOrEmpty; import static org.firebirdsql.jdbc.SQLStateConstants.SQL_STATE_INVALID_CURSOR_STATE; /** @@ -66,106 +70,109 @@ */ final class FBRowUpdater implements FirebirdRowUpdater { - private static final int PARAMETER_UNUSED = 0; - private static final int PARAMETER_USED = 1; - private static final int PARAMETER_DBKEY = 2; + // Estimated average column length of 10 + 2 quote characters + comma (for pre-sizing string builders). + // We could be precise by summing over the field descriptors, but we don't want to waste cycles on it. + private static final int EST_COLUMN_SIZE = 13; + // Estimated size for update/delete/select statement (for pre-sizing string builders). + // This is probably still too small for some cases, but will prevent a number of resizes from the default size + private static final int EST_STATEMENT_SIZE = 64; + + private static final String ROW_INSERT = "insert"; + private static final String ROW_CURRENT = "current"; + private static final String ROW_UPDATE = "update"; + private static final String ROW_OLD = "old"; + private static final byte[][] EMPTY_2D_BYTES = new byte[0][]; - private final FBConnection connection; + private final String tableName; + private final FBObjectListener.ResultSetListener rsListener; private final GDSHelper gdsHelper; private final RowDescriptor rowDescriptor; - private final FBField[] fields; + private final List fields; private final QuoteStrategy quoteStrategy; + private final FbStatement[] statements = new FbStatement[4]; - private boolean inInsertRow; - - private RowValue newRow; + private final List keyColumns; + private final RowValue newRow; private RowValue oldRow; - private RowValue insertRow; - private boolean[] updatedFlags; - - private String tableName; - - private final FbStatement[] statements = new FbStatement[4]; - private final FBObjectListener.ResultSetListener rsListener; + private boolean inInsertRow; private boolean closed; private boolean processing; FBRowUpdater(FBConnection connection, RowDescriptor rowDescriptor, boolean cached, FBObjectListener.ResultSetListener rsListener) throws SQLException { - this.rsListener = rsListener; + tableName = requireSingleTableName(rowDescriptor); + keyColumns = deriveKeyColumns(tableName, rowDescriptor, connection.getMetaData()); - this.connection = connection; + this.rsListener = rsListener; gdsHelper = connection.getGDSHelper(); - - this.rowDescriptor = rowDescriptor; - fields = new FBField[rowDescriptor.getCount()]; - quoteStrategy = connection.getQuoteStrategy(); + fields = createFields(rowDescriptor, cached); newRow = rowDescriptor.createDefaultFieldValues(); - updatedFlags = new boolean[rowDescriptor.getCount()]; - - for (int i = 0; i < rowDescriptor.getCount(); i++) { - final int fieldPos = i; - - // implementation of the FieldDataProvider interface - final FieldDataProvider dataProvider = new FieldDataProvider() { - @Override - public byte[] getFieldData() { - if (!updatedFlags[fieldPos]) { - return oldRow.getFieldData(fieldPos); - } else if (inInsertRow) { - return insertRow.getFieldData(fieldPos); - } else { - return newRow.getFieldData(fieldPos); - } - } + this.rowDescriptor = rowDescriptor; + } - @Override - public void setFieldData(byte[] data) { - if (inInsertRow) { - insertRow.setFieldData(fieldPos, data); - } else { - newRow.setFieldData(fieldPos, data); - } - updatedFlags[fieldPos] = true; - } - }; + private List createFields(RowDescriptor rowDescriptor, boolean cached) throws SQLException { + try { + return StreamSupport.stream(rowDescriptor.spliterator(), false) + .map(fieldDescriptor -> createFieldUnchecked(fieldDescriptor, cached)) + .toList(); + } catch (UncheckedSQLException e) { + throw e.getCause(); + } + } - fields[i] = FBField.createField(rowDescriptor.getFieldDescriptor(i), dataProvider, gdsHelper, cached); + private FBField createFieldUnchecked(FieldDescriptor fieldDescriptor, boolean cached) { + try { + return FBField.createField( + fieldDescriptor, new FieldDataProviderImpl(fieldDescriptor.getPosition()), gdsHelper, cached); + } catch (SQLException e) { + throw new UncheckedSQLException(e); } + } - // find the table name (there can be only one table per result set) + /** + * Returns the single table name referenced by {@code rowDescriptor}, or throws an exception if there are no or + * multiple table names. + * + * @param rowDescriptor + * row descriptor + * @return non-null table name + * @throws SQLException + * if {@code rowDescriptor} references multiple table names or no table names at all + */ + private static String requireSingleTableName(RowDescriptor rowDescriptor) throws SQLException { + // find the table name (there can be only one table per updatable result set) + String tableName = null; for (FieldDescriptor fieldDescriptor : rowDescriptor) { + // TODO This will not detect derived columns in the prefix of the select list if (tableName == null) { tableName = fieldDescriptor.getOriginalTableName(); - } else if (!tableName.equals(fieldDescriptor.getOriginalTableName())) { + } else if (!Objects.equals(tableName, fieldDescriptor.getOriginalTableName())) { throw new FBResultSetNotUpdatableException( "Underlying result set references at least two relations: %s and %s." .formatted(tableName, fieldDescriptor.getOriginalTableName())); } } + if (isNullOrEmpty(tableName)) { + throw new FBResultSetNotUpdatableException("Underlying result set references no relations"); + } + return tableName; } private void notifyExecutionStarted() throws SQLException { - if (closed) { - throw new SQLException("Corresponding result set is closed.", SQL_STATE_INVALID_CURSOR_STATE); - } - + if (closed) throw new SQLException("Corresponding result set is closed.", SQL_STATE_INVALID_CURSOR_STATE); if (processing) return; - rsListener.executionStarted(this); - this.processing = true; + processing = true; } private void notifyExecutionCompleted(boolean success) throws SQLException { - if (!processing) - return; - + if (!processing) return; rsListener.executionCompleted(this, success); - this.processing = false; + processing = false; } private void deallocateStatement(FbStatement handle, SQLExceptionChainBuilder chain) { @@ -177,215 +184,245 @@ private void deallocateStatement(FbStatement handle, SQLExceptionChainBuilder chain = new SQLExceptionChainBuilder<>(); + closed = true; + var chain = new SQLExceptionChainBuilder<>(); for (FbStatement statement : statements) { deallocateStatement(statement, chain); } - - // TODO: Close not completed by throw at this point? - if (chain.hasException()) - throw chain.getException(); - - this.closed = true; - if (processing) + try { notifyExecutionCompleted(true); + } catch (SQLException e) { + chain.append(e); + } + if (chain.hasException()) { + throw chain.getException(); + } } @Override public void setRow(RowValue row) { - this.oldRow = row; - this.updatedFlags = new boolean[rowDescriptor.getCount()]; - this.inInsertRow = false; + oldRow = row; + newRow.reset(); + inInsertRow = false; } @Override public void cancelRowUpdates() { - this.newRow = rowDescriptor.createDefaultFieldValues(); - this.updatedFlags = new boolean[rowDescriptor.getCount()]; - this.inInsertRow = false; + newRow.reset(); + inInsertRow = false; } @Override public FBField getField(int fieldPosition) { - return fields[fieldPosition]; + return fields.get(fieldPosition); } /** - * This method gets the parameter mask for the UPDATE or DELETE statement. - * Parameter mask is an array of booleans, where array item is set to true, - * if the appropriate field should be included in WHERE clause of the - * UPDATE or DELETE statement. + * This method derives the key columns that uniquely identify the row in the result set. *

- * This method obtains the parameter mask from the best row identifiers, in - * other words set of columns that form "best row identifiers" must be a - * subset of the selected columns (no distinction is made whether columns - * are real or are pseudo-columns). If no + * The key column(s) are the best row identifier, or {@code RDB$DB_KEY} if available. The columns of that 'best row + * identifier' (or the DB key) must be a subset of the selected columns. If no suitable columns are found, an + * exception is thrown. + *

* - * @return array of booleans that represent parameter mask. + * @return immutable list of columns that uniquely identify the row, for use in the WHERE clause of the UPDATE, + * DELETE, or SELECT statements created in this class. + * @throws FBResultSetNotUpdatableException + * if there are no suitable columns to identify a row uniquely + * @throws SQLException + * for errors looking up the best row identifier */ - private int[] getParameterMask() throws SQLException { - // loop through the "best row identifiers" and set appropriate flags. - FBDatabaseMetaData metaData = (FBDatabaseMetaData) connection.getMetaData(); - - try (ResultSet bestRowIdentifier = metaData.getBestRowIdentifier("", "", tableName, - DatabaseMetaData.bestRowTransaction, true)) { - int[] result = new int[rowDescriptor.getCount()]; - boolean hasParams = false; + private static List deriveKeyColumns(String tableName, RowDescriptor rowDescriptor, + DatabaseMetaData dbmd) throws SQLException { + // first try best row identifier + List keyColumns = keyColumnsOfBestRowIdentifier( tableName, rowDescriptor, dbmd); + if (keyColumns.isEmpty()) { + // best row identifier not available or not fully matched, fallback to RDB$DB_KEY + // NOTE: fallback is updatable, but may not be insertable (e.g. if missing PK column(s) are not generated)! + keyColumns = keyColumnsOfDbKey(rowDescriptor); + + if (keyColumns.isEmpty()) { + // we did not find the columns of the best row identifier or RDB$DB_KEY in our result set, + // throw an exception, since we cannot reliably identify the row. + throw new FBResultSetNotUpdatableException("Underlying result set does not contain all columns that " + + "form 'best row identifier' and no RDB$DB_KEY was available as fallback"); + } + } + + return List.copyOf(keyColumns); + } + + /** + * Derives the key columns based on {@code DatabaseMetaData.getBestRowIdentifier}. + *

+ * The 'best row identifier' are the primary key columns or {@code RDB$DB_KEY} if there is no primary key. + *

+ * + * @return a list of columns in the best row identifier, or empty if there is no best row identifier, or not all + * columns of the best row identifier exist in {@code rowDescriptor} + * @throws SQLException + * for errors looking up the best row identifier + */ + private static List keyColumnsOfBestRowIdentifier(String tableName, RowDescriptor rowDescriptor, + DatabaseMetaData dbmd) throws SQLException { + try (ResultSet bestRowIdentifier = dbmd + .getBestRowIdentifier("", "", tableName, DatabaseMetaData.bestRowTransaction, true)) { + int bestRowIdentifierColumnCount = 0; + List keyColumns = new ArrayList<>(); while (bestRowIdentifier.next()) { + bestRowIdentifierColumnCount++; String columnName = bestRowIdentifier.getString(2); - - if (columnName == null) - continue; - - 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; - } else if (columnName.equals(rowDescriptor.getFieldDescriptor(i).getOriginalName())) { - result[i] = PARAMETER_USED; - hasParams = true; + if (columnName == null) continue; + + for (FieldDescriptor fieldDescriptor : rowDescriptor) { + // NOTE: We only use the first occurrence of a column + // TODO repeated columns in an updatable result set might be problematic in and of itself, maybe we + // need to explicitly disallow it. + if ("RDB$DB_KEY".equals(columnName) && fieldDescriptor.isDbKey()) { + // special handling for the RDB$DB_KEY columns that must be referenced as RDB$DB_KEY in select + // and where, but in metadata are represented as DB_KEY + return List.of(fieldDescriptor); + } else if (columnName.equals(fieldDescriptor.getOriginalName())) { + keyColumns.add(fieldDescriptor); } } - // 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) { - throw new FBResultSetNotUpdatableException( - "Underlying result set does not contain all columns that form 'best row identifier'."); + // column of best row identifier not found, stop matching process + if (keyColumns.size() != bestRowIdentifierColumnCount) { + return List.of(); } } + return keyColumns; + } + } - if (!hasParams) { - throw new FBResultSetNotUpdatableException( - "No columns that can be used in WHERE clause could be found."); + /** + * Derives the key column based on {@code RDB$DB_KEY}, if present in the result set. + *

+ * This is intended as a fallback when {@code keyColumnsOfBestRowIdentifier} returns an empty list. + *

+ * + * @return list with the (single) {@link FieldDescriptor} of the {@code RDB$DB_KEY} column, or an empty list if + * {@code rowDescriptor} contains no {@code RDB$DB_KEY} column + */ + private static List keyColumnsOfDbKey(RowDescriptor rowDescriptor) { + for (FieldDescriptor fieldDescriptor : rowDescriptor) { + if (fieldDescriptor.isDbKey()) { + return List.of(fieldDescriptor); } - - return result; } + return List.of(); } - private void appendWhereClause(StringBuilder sb, int[] parameterMask) { - sb.append("WHERE "); + private void appendWhereClause(StringBuilder sb) { + sb.append("where "); // handle the RDB$DB_KEY case first - boolean hasDbKey = false; - for (int aParameterMask : parameterMask) { - if (aParameterMask == PARAMETER_DBKEY) { - hasDbKey = true; - break; - } - } - - if (hasDbKey) { - sb.append("RDB$DB_KEY = ?"); + if (keyColumns.get(0).isDbKey()) { + sb.append("RDB$DB_KEY=?"); return; } - // if we are here, then no RDB$DB_KEY update was used - // therefore loop through the parameters and build the - // WHERE clause + // no RDB$DB_KEY update was used, so loop through the key columns and build the WHERE clause boolean first = true; - for (int i = 0; i < rowDescriptor.getCount(); i++) { - if (parameterMask[i] == PARAMETER_UNUSED) continue; - - if (!first) sb.append(" AND"); - - sb.append("\n\t"); - quoteStrategy.appendQuoted(rowDescriptor.getFieldDescriptor(i).getOriginalName(), sb).append(" = ?"); + for (FieldDescriptor fieldDescriptor : keyColumns) { + if (first) { + first = false; + } else { + sb.append("\nand "); + } - first = false; + quoteStrategy.appendQuoted(fieldDescriptor.getOriginalName(), sb).append("=?"); } } - private String buildUpdateStatement(int[] parameterMask) { - StringBuilder sb = new StringBuilder("UPDATE "); - quoteStrategy.appendQuoted(tableName, sb) - .append("\nSET\n"); + private String buildUpdateStatement() { + // TODO raise exception if there are no updated columns, or do nothing? + var sb = new StringBuilder(EST_STATEMENT_SIZE + newRow.initializedCount() * EST_COLUMN_SIZE) + .append("update "); + quoteStrategy.appendQuoted(tableName, sb).append(" set "); boolean first = true; - for (int i = 0; i < rowDescriptor.getCount(); i++) { - if (!updatedFlags[i]) - continue; - - if (!first) - sb.append(','); - - sb.append("\n\t"); - quoteStrategy.appendQuoted(rowDescriptor.getFieldDescriptor(i).getOriginalName(), sb).append(" = ?"); + for (FieldDescriptor fieldDescriptor : rowDescriptor) { + if (!newRow.isInitialized(fieldDescriptor.getPosition()) || fieldDescriptor.isDbKey()) continue; - first = false; + if (first) { + first = false; + } else { + sb.append(",\n\t"); + } + + quoteStrategy.appendQuoted(fieldDescriptor.getOriginalName(), sb).append("=?"); } sb.append('\n'); - appendWhereClause(sb, parameterMask); + appendWhereClause(sb); return sb.toString(); } - private String buildDeleteStatement(int[] parameterMask) { - StringBuilder sb = new StringBuilder("DELETE FROM "); + private String buildDeleteStatement() { + var sb = new StringBuilder(EST_STATEMENT_SIZE).append("delete from "); quoteStrategy.appendQuoted(tableName, sb).append('\n'); - appendWhereClause(sb, parameterMask); + appendWhereClause(sb); return sb.toString(); } private String buildInsertStatement() { - StringBuilder columns = new StringBuilder(); - StringBuilder params = new StringBuilder(); + // TODO raise exception if there are no initialized columns, or use INSERT .. DEFAULT VALUES? + final int initializedColumnCount = newRow.initializedCount(); + var columns = new StringBuilder(initializedColumnCount * EST_COLUMN_SIZE); + var params = new StringBuilder(initializedColumnCount * 2); boolean first = true; - for (int i = 0; i < rowDescriptor.getCount(); i++) { - - if (!updatedFlags[i]) - continue; + for (FieldDescriptor fieldDescriptor : rowDescriptor) { + if (!newRow.isInitialized(fieldDescriptor.getPosition()) || fieldDescriptor.isDbKey()) continue; - if (!first) { + if (first) { + first = false; + } else { columns.append(','); params.append(','); } - quoteStrategy.appendQuoted(rowDescriptor.getFieldDescriptor(i).getOriginalName(), columns); + quoteStrategy.appendQuoted(fieldDescriptor.getOriginalName(), columns); params.append('?'); - - first = false; } - StringBuilder sb = new StringBuilder("INSERT INTO "); + // 27 = length of appended literals + 2 quote characters + var sb = new StringBuilder(27 + tableName.length() + columns.length() + params.length()).append("insert into "); quoteStrategy.appendQuoted(tableName, sb) - .append(" (").append(columns).append(") VALUES (").append(params).append(')'); + .append(" (").append(columns).append(") values (").append(params).append(')'); return sb.toString(); } - private String buildSelectStatement(int[] parameterMask) { - StringBuilder columns = new StringBuilder(); + private String buildSelectStatement() { + var columns = new StringBuilder(rowDescriptor.getCount() * EST_COLUMN_SIZE); boolean first = true; for (FieldDescriptor fieldDescriptor : rowDescriptor) { - if (!first) + if (first) { + first = false; + } else { columns.append(','); + } - // do special handling of RDB$DB_KEY, since Firebird returns - // DB_KEY column name instead of the correct one + // special handling of RDB$DB_KEY, since Firebird returns DB_KEY column name instead of the correct one if (fieldDescriptor.isDbKey()) { columns.append("RDB$DB_KEY"); } else { quoteStrategy.appendQuoted(fieldDescriptor.getOriginalName(), columns); } - first = false; } - StringBuilder sb = new StringBuilder("SELECT "); - sb.append(columns).append('\n') - .append("FROM "); + var sb = new StringBuilder(EST_STATEMENT_SIZE + columns.length()) + .append("select ").append(columns).append("\nfrom "); quoteStrategy.appendQuoted(tableName, sb).append('\n'); - appendWhereClause(sb, parameterMask); + appendWhereClause(sb); return sb.toString(); } @@ -476,95 +513,115 @@ public void refreshRow() throws SQLException { } private void executeStatement(int statementType, FbStatement stmt) throws SQLException { - if (inInsertRow && statementType != INSERT_STATEMENT_TYPE) { - throw new SQLException("Only insertRow() is allowed when result set is positioned on insert row."); - } - - if (statementType != INSERT_STATEMENT_TYPE && oldRow == null) { - throw new SQLException("Result set is not positioned on a row."); - } - - // we have to flush before constructing the parameters - // since flushable field can update the value, which - // in turn can change the parameter distribution - for (int i = 0; i < rowDescriptor.getCount(); i++) { - if (fields[i] instanceof FBFlushableField flushableField) { - flushableField.flushCachedData(); + if (statementType != INSERT_STATEMENT_TYPE) { + if (inInsertRow) { + throw new SQLException("Only insertRow() is allowed when result set is positioned on insert row."); + } else if (oldRow == null) { + throw new SQLException("Result set is not positioned on a row."); } } - int[] parameterMask = getParameterMask(); - - String sql = switch (statementType) { - case UPDATE_STATEMENT_TYPE -> buildUpdateStatement(parameterMask); - case DELETE_STATEMENT_TYPE -> buildDeleteStatement(parameterMask); - case INSERT_STATEMENT_TYPE -> buildInsertStatement(); - case SELECT_STATEMENT_TYPE -> buildSelectStatement(parameterMask); - default -> throw new IllegalArgumentException("Incorrect statement type specified."); - }; - - stmt.prepare(sql); + // Flushable field can update the value, which in turn can change the parameter distribution + flushFields(); - List params = new ArrayList<>(); + stmt.prepare(generateStatementText(statementType)); - if (statementType == UPDATE_STATEMENT_TYPE) { - for (int i = 0; i < rowDescriptor.getCount(); i++) { - if (!updatedFlags[i]) continue; + List params = new ArrayList<>(newRow.initializedCount() + keyColumns.size()); - params.add(newRow.getFieldData(i)); + // Set parameters of new values + if (statementType == UPDATE_STATEMENT_TYPE || statementType == INSERT_STATEMENT_TYPE) { + for (FieldDescriptor fieldDescriptor : rowDescriptor) { + if (newRow.isInitialized(fieldDescriptor.getPosition()) && !fieldDescriptor.isDbKey()) { + params.add(newRow.getFieldData(fieldDescriptor.getPosition())); + } } } - RowValue source = statementType == INSERT_STATEMENT_TYPE ? insertRow : oldRow; - for (int i = 0; i < rowDescriptor.getCount(); i++) { - if (parameterMask[i] == PARAMETER_UNUSED && statementType != INSERT_STATEMENT_TYPE) { - continue; - } else if (!updatedFlags[i] && statementType == INSERT_STATEMENT_TYPE) { - continue; + // Set parameters of where clause + if (statementType != INSERT_STATEMENT_TYPE) { + for (FieldDescriptor keyColumn : keyColumns) { + params.add(oldRow.getFieldData(keyColumn.getPosition())); } - params.add(source.getFieldData(i)); } stmt.execute(RowValue.of(params.toArray(EMPTY_2D_BYTES))); } + private void flushFields() throws SQLException { + for (FBField field : fields) { + if (field instanceof FBFlushableField flushableField) { + flushableField.flushCachedData(); + } + } + } + + private String generateStatementText(int statementType) { + return switch (statementType) { + case UPDATE_STATEMENT_TYPE -> buildUpdateStatement(); + case DELETE_STATEMENT_TYPE -> buildDeleteStatement(); + case INSERT_STATEMENT_TYPE -> buildInsertStatement(); + case SELECT_STATEMENT_TYPE -> buildSelectStatement(); + default -> throw new IllegalArgumentException("Incorrect statement type specified."); + }; + } + @Override - public RowValue getNewRow() { + public RowValue getNewRow() throws SQLException { + if (inInsertRow) { + throw wrongRow(ROW_UPDATE, ROW_INSERT); + } + RowValue newRowCopy = rowDescriptor.createDefaultFieldValues(); for (int i = 0; i < rowDescriptor.getCount(); i++) { - RowValue source = updatedFlags[i] ? newRow : oldRow; - byte[] fieldData = source.getFieldData(i); + byte[] fieldData = getFieldData(i); newRowCopy.setFieldData(i, fieldData != null ? fieldData.clone() : null); } return newRowCopy; } + private byte[] getFieldData(int field) { + RowValue source = newRow.isInitialized(field) || inInsertRow ? newRow : oldRow; + return source.getFieldData(field); + } + @Override - public RowValue getInsertRow() { - return insertRow; + public RowValue getInsertRow() throws SQLException { + if (inInsertRow) { + RowValue newRowCopy = newRow.deepCopy(); + newRowCopy.initializeFields(); + return newRowCopy; + } + throw wrongRow(ROW_INSERT, ROW_CURRENT); } @Override - public RowValue getOldRow() { + public RowValue getOldRow() throws SQLException { + if (inInsertRow) { + throw wrongRow(ROW_OLD, ROW_INSERT); + } return oldRow; } + private static SQLException wrongRow(String expectedRow, String actualRow) { + return new SQLException( + "Cannot return %s row, currently positioned on %s row".formatted(expectedRow, actualRow)); + } + @Override public void moveToInsertRow() { inInsertRow = true; - insertRow = rowDescriptor.createDefaultFieldValues(); - this.updatedFlags = new boolean[rowDescriptor.getCount()]; + newRow.reset(); } @Override public void moveToCurrentRow() { inInsertRow = false; - insertRow = rowDescriptor.createDefaultFieldValues(); - this.updatedFlags = new boolean[rowDescriptor.getCount()]; + newRow.reset(); } private static final class RowListener implements StatementListener { - private final List rows = new ArrayList<>(); + // expect 0 or 1 rows (2 or more would mean the key columns didn't identify a row uniquely) + private final List rows = new ArrayList<>(1); @Override public void receivedRow(FbStatement sender, RowValue rowValue) { @@ -575,4 +632,24 @@ public List getRows() { return rows; } } + + private final class FieldDataProviderImpl implements FieldDataProvider { + + private final int field; + + FieldDataProviderImpl(int field) { + this.field = field; + } + + @Override + public byte[] getFieldData() { + return FBRowUpdater.this.getFieldData(field); + } + + @Override + public void setFieldData(byte[] data) { + newRow.setFieldData(field, data); + } + + } } \ No newline at end of file diff --git a/src/test/org/firebirdsql/jaybird/util/StringUtilsTest.java b/src/test/org/firebirdsql/jaybird/util/StringUtilsTest.java index 8d70156ed..7490a4e6d 100644 --- a/src/test/org/firebirdsql/jaybird/util/StringUtilsTest.java +++ b/src/test/org/firebirdsql/jaybird/util/StringUtilsTest.java @@ -19,9 +19,15 @@ package org.firebirdsql.jaybird.util; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EmptySource; +import org.junit.jupiter.params.provider.NullSource; +import org.junit.jupiter.params.provider.ValueSource; 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; class StringUtilsTest { @@ -60,4 +66,17 @@ void trimToNull_endsWithSpace_yields_valueWithoutSpace() { assertEquals(expectedValue, StringUtils.trimToNull(value)); } + @ParameterizedTest + @NullSource + @EmptySource + void testIsNullOrEmpty_nullOrEmptyYieldsTrue(String value) { + assertTrue(StringUtils.isNullOrEmpty(value)); + } + + @ParameterizedTest + @ValueSource(strings = { " ", "a", "\0", "abc" }) + void testIsNullOrEmpty_nonEmptyYieldsFalse(String value) { + assertFalse(StringUtils.isNullOrEmpty(value)); + } + } \ No newline at end of file diff --git a/src/test/org/firebirdsql/jdbc/FBResultSetTest.java b/src/test/org/firebirdsql/jdbc/FBResultSetTest.java index c159dd5d7..0787f71f0 100644 --- a/src/test/org/firebirdsql/jdbc/FBResultSetTest.java +++ b/src/test/org/firebirdsql/jdbc/FBResultSetTest.java @@ -24,7 +24,6 @@ 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; @@ -85,6 +84,14 @@ very_long_str VARCHAR(20000), "CamelStr" VARCHAR(255) )"""; + private static final String CREATE_WITH_COMPOSITE_PK = """ + create table WITH_COMPOSITE_PK ( + ID1 integer not null, + ID2 integer not null, + VAL varchar(50), + constraint PK_WITH_COMPOSITE_PK primary key (ID1, ID2) + )"""; + private static final String CREATE_VIEW_STATEMENT = """ CREATE VIEW test_empty_string_view(marker, id, empty_char) AS @@ -431,57 +438,6 @@ void testBugReport3() throws Exception { } } - @Disabled - @Test - void testMemoryGrowth() throws Exception { - Properties props = getDefaultPropertiesForConnection(); - props.put("no_result_set_tracking", ""); - try (Connection connection = DriverManager.getConnection(getUrl(), props)) { - executeCreateTable(connection, CREATE_TABLE_STATEMENT); - connection.setAutoCommit(false); - - System.out.println("Inserting..."); - int recordCount = 1; - - IntFunction rowData = id -> new String(DataGenerator.createRandomAsciiBytes(19000)); - createTestData(recordCount, rowData, connection, "very_long_str"); - - connection.commit(); - - System.gc(); - - long memoryBeforeSelects = Runtime.getRuntime().totalMemory() - Runtime.getRuntime().freeMemory(); - - System.out.println("Selecting..."); - int selectRuns = 10000; - for (int i = 0; i < selectRuns; i++) { - if ((i % 1000) == 0) System.out.println("Select no. " + i); - - try (Statement stmt = connection.createStatement(); - ResultSet rs = stmt.executeQuery("SELECT * FROM test_table")) { - //noinspection StatementWithEmptyBody - while (rs.next()) { - // just loop through result set - } - } - } - System.gc(); - - long memoryAfterSelects = Runtime.getRuntime().totalMemory() - Runtime.getRuntime().freeMemory(); - - connection.commit(); - - System.gc(); - - long memoryAfterCommit = Runtime.getRuntime().totalMemory() - Runtime.getRuntime().freeMemory(); - - System.out.println("Memory before selects " + memoryBeforeSelects); - System.out.println("Memory after selects " + memoryAfterSelects); - System.out.println("Memory after commit " + memoryAfterCommit); - System.out.println("Commit freed " + (memoryAfterSelects - memoryAfterCommit)); - } - } - @Test void testResultSetNotClosed() throws Exception { try (Connection connection = getConnectionViaDriverManager()) { @@ -682,11 +638,53 @@ void testUpdatableStatementResultSetDowngradeToReadOnlyWhenQueryNotUpdatable( executeCreateTable(connection, CREATE_TABLE_STATEMENT); executeCreateTable(connection, CREATE_TABLE_STATEMENT2); - try (Statement stmt = connection.createStatement(TYPE_SCROLL_INSENSITIVE, CONCUR_UPDATABLE); - ResultSet rs = stmt.executeQuery( - "select * from test_table t1 left join test_table2 t2 on t1.id = t2.id")) { - SQLWarning warning = stmt.getWarnings(); - assertThat(warning, allOf( + try (var stmt = connection.createStatement(TYPE_SCROLL_INSENSITIVE, CONCUR_UPDATABLE); + var rs = stmt.executeQuery("select * from test_table t1 left join test_table2 t2 on t1.id = t2.id")) { + 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 prefix) will be downgraded to read-only. + *

+ * Rationale: a previous implementation was satisfied if at least the first PK column was found + *

+ */ + @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. + *

+ * Rationale: a previous implementation was satisfied if at least the first PK column was found + *

+ */ + @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))); @@ -695,6 +693,49 @@ void testUpdatableStatementResultSetDowngradeToReadOnlyWhenQueryNotUpdatable( } } + /** + * Tests if a result set that doesn't contain all PK columns, but does contain RDB$DB_KEY, is updatable. + *

+ * Rationale: see also {@link #testUpdatableStatementPrefixPK_downgradeToReadOnly(String)}, and previous + * implementation considered either the primary key or RDB$DB_KEY, current implementation falls back to RDB$DB_KEY. + *

+ */ + @ParameterizedTest + @MethodSource("scrollableCursorPropertyValues") + void testUpdatableStatementPartialPKAndDBKey(String scrollableCursorPropertyValue) throws Exception { + try (Connection connection = createConnection(scrollableCursorPropertyValue)) { + executeCreateTable(connection, CREATE_WITH_COMPOSITE_PK); + try (var pstmt = connection.prepareStatement( + "insert into WITH_COMPOSITE_PK (ID1, ID2, VAL) values (?, ?, ?)")) { + pstmt.setInt(1, 1); + pstmt.setInt(2, 1); + pstmt.setString(3, "ID_1_1"); + pstmt.addBatch(); + pstmt.setInt(2, 2); + pstmt.setString(3, "ID_1_2"); + pstmt.addBatch(); + pstmt.executeBatch(); + } + + try (var stmt = connection.createStatement(TYPE_SCROLL_INSENSITIVE, CONCUR_UPDATABLE)) { + try (var rs = stmt.executeQuery("select ID1, RDB$DB_KEY, VAL from WITH_COMPOSITE_PK where ID2 = 1")) { + assertEquals(CONCUR_UPDATABLE, rs.getConcurrency(), "Expected CONCUR_UPDATABLE result set"); + assertTrue(rs.next(), "expected a row"); + assertEquals("ID_1_1", rs.getString(3), "unexpected value for VAL"); + + rs.updateString(3, "modified"); + rs.updateRow(); + } + try (var rs = stmt.executeQuery("select ID1, ID2, VAL from WITH_COMPOSITE_PK order by ID1, ID2")) { + assertTrue(rs.next(), "expected a row"); + assertEquals("modified", rs.getString(3), "expected modified value for VAL of first row"); + assertTrue(rs.next(), "expected a row"); + assertEquals("ID_1_2", rs.getString(3), "expected original value (ID_1_2) for VAL of second row"); + } + } + } + } + @Test void testGetExecutionPlan() throws Exception { try (Connection connection = getConnectionViaDriverManager()) {