#780 FBRowUpdater incorrectly considers result set with only partial …

…PK updatable

+ Improvement: try to fall back to RDB$DB_KEY if best row identifier not matched
+ various refactoring/performance improvements/code cleanup

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 <<compatibility-changes>> 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 <<compatibility-changes>> for details.
 
 [#removal-of-deprecated-classes-and-packages]
 === Removal of deprecated classes and packages

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

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,13 +110,16 @@ public void reset() {
      * </p>
      *
      * @return {@code true} if this a deleted row marker, {@code false} otherwise
+     * @since 5
      */
     public boolean isDeletedRowMarker() {
         return false;
     }
 
     /**
      * 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;
     }

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();
+    }
+
 }