#805 Preserve connection read-only state across changes to transactio…

…n isolation

src/docs/asciidoc/release_notes.adoc

@@ -1034,6 +1034,7 @@ Given this is a non-standard extension, it is advisable to retrieve these column
 +
 To be clear, Jaybird does not provide statement pooling.
 This is change is only about returning and recording the poolable information for JDBC compliance, so it can be used by -- for example -- connection pool implementations that provide statement pooling.
+* The state of `Connection.setReadOnly(boolean)` was not retained after calling `Connection.setTransactionIsolation(int)` or other method calls that changed the current transaction parameter buffer (https://github.com/FirebirdSQL/jaybird/issues/805[#805])
 
 [#potentially-breaking-changes]
 === Potentially breaking changes
@@ -1194,6 +1195,21 @@ Switch to using one of the normal execute methods.
 
 See also <<allow-tx-stmts>>.
 
+[#compat-read-only]
+=== Read-only behaviour of connections
+
+In previous versions of Jaybird the read-only state of a connection was not retained if the transaction parameter buffer was replaced, for example by calls to `setTransactionIsolation(int)`.
+
+Now this has been corrected, it is possible that your code unexpectedly throws an exception with message "`__attempted update during read-only transaction [SQLState:42000, ISC error code:335544361]__`" (error `isc_read_only_trans`).
+
+You need to make sure to call `setReadOnly(false)` if the connection was previously marked read-only.
+If you're using a connection pool, you need to ensure it properly resets the read-only state of the connection when checking in or checking out the connection.
+For example, both Apache DBCP and Apache Tomcat connection pools requires the `defaultReadOnly` property to be set (i.e. to `false`), otherwise it will not reset the read-only state.
+
+If overridden transaction mappings are used, and the default isolation level has `isc_tpb_read`, the connection will be marked as read-only.
+As a result, switching isolation levels will now also result in read-only transactions, even if the mapping of the other isolation level is defined with `isc_tpb_write`.
+You will need to explicitly call `setReadOnly(false)`, or -- better yet -- do not override transaction mappings with a `isc_tpb_read`, but always use `isc_tpb_write`, and control read-only state only through `setReadOnly`.
+
 // TODO Document compatibility issues
 
 [#removal-of-classes-packages-and-methods-without-deprecation]
@@ -1576,7 +1592,11 @@ this package is internal API only, and not exported from the module (see also ea
 ** Constructor `SQLExceptionChainBuilder(SQLException)` was removed, as in practice this was never used;
 replacement is `new SQLExceptionChainBuilder().append(exception)`
 * `FBTpb` was removed, and its usages were replaced with `TransactionParameterBuffer`
+* `FBTpbMapper` no longer implements `Cloneable`, use `FBTpbMapper.copyOf(FBTpbMapper)` instead
 * `ParameterBuffer` now extends `Serializable`, as all implementations are serializable, and some usages expect serializable behaviour even when the interfaces were used (though in practice, these objects are hardly ever serialized)
+* `FBManagedConnection`
+** `setReadOnly(boolean)` was renamed to `setTpbReadOnly(boolean)` to reflect what it actually does
+** `isReadOnly()` was renamed to `isTpbReadOnly()` to reflect what it actually does
 
 [#breaking-changes-unlikely]
 === Unlikely breaking changes

src/main/org/firebirdsql/gds/ParameterBuffer.java

@@ -120,7 +120,7 @@ public interface ParameterBuffer extends Iterable<Parameter>, Serializable {
     void addArgument(int argumentType, byte[] content);
 
     /**
-     * Remove specified argument.
+     * Remove the first occurrence of the specified argument.
      *
      * @param argumentType
      *         type of argument to remove.

src/main/org/firebirdsql/gds/TransactionParameterBuffer.java

@@ -60,9 +60,18 @@ default void copyTo(TransactionParameterBuffer destination) {
      * @since 6
      */
     default void setReadOnly(boolean readOnly) {
-        removeArgument(isc_tpb_read);
-        removeArgument(isc_tpb_write);
-        addArgument(readOnly ? isc_tpb_read : isc_tpb_write);
+        if (readOnly) {
+            ensurePresentAbsent(isc_tpb_read, isc_tpb_write);
+        } else {
+            ensurePresentAbsent(isc_tpb_write, isc_tpb_read);
+        }
+    }
+
+    private void ensurePresentAbsent(int present, int absent) {
+        if (!hasArgument(present)) {
+            addArgument(present);
+        }
+        removeArgument(absent);
     }
 
     /**
@@ -88,9 +97,12 @@ default boolean isReadOnly() {
      * @since 6
      */
     default void setAutoCommit(boolean autoCommit) {
-        removeArgument(isc_tpb_autocommit);
         if (autoCommit) {
-            addArgument(isc_tpb_autocommit);
+            if (!hasArgument(isc_tpb_autocommit)) {
+                addArgument(isc_tpb_autocommit);
+            }
+        } else {
+            removeArgument(isc_tpb_autocommit);
         }
     }
 

src/main/org/firebirdsql/gds/impl/ParameterBufferBase.java

@@ -35,6 +35,7 @@
 import java.util.ArrayList;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Optional;
 
 /**
  * Base class for parameter buffers
@@ -45,7 +46,7 @@ public abstract class ParameterBufferBase implements ParameterBuffer, Serializab
 
     @Serial
     private static final long serialVersionUID = 8812835147477954476L;
-    
+
     private final List<Argument> arguments = new ArrayList<>();
 
     private final String defaultEncodingName;
@@ -98,72 +99,68 @@ public final void addArgument(int argumentType, String value) {
 
     @Override
     public final void addArgument(int argumentType, String value, Encoding encoding) {
-        getArgumentsList().add(new StringArgument(argumentType, parameterBufferMetaData.getStringArgumentType(argumentType), value, encoding));
+        addArgument(new StringArgument(
+                argumentType, parameterBufferMetaData.getStringArgumentType(argumentType), value, encoding));
     }
 
     @Override
-    public void addArgument(int argumentType, byte value) {
-        getArgumentsList().add(new ByteArgument(argumentType, parameterBufferMetaData.getByteArgumentType(argumentType), value));
+    public final void addArgument(int argumentType, byte value) {
+        addArgument(new ByteArgument(argumentType, parameterBufferMetaData.getByteArgumentType(argumentType), value));
     }
 
     @Override
     public final void addArgument(int argumentType, int value) {
-        getArgumentsList().add(new NumericArgument(argumentType, parameterBufferMetaData.getIntegerArgumentType(argumentType), value));
+        addArgument(new NumericArgument(
+                argumentType, parameterBufferMetaData.getIntegerArgumentType(argumentType), value));
     }
 
     @Override
     public final void addArgument(int argumentType, long value) {
-        getArgumentsList().add(new BigIntArgument(argumentType, parameterBufferMetaData.getIntegerArgumentType(argumentType), value));
+        addArgument(new BigIntArgument(
+                argumentType, parameterBufferMetaData.getIntegerArgumentType(argumentType), value));
     }
 
     @Override
     public final void addArgument(int argumentType) {
-        getArgumentsList().add(new SingleItem(argumentType, parameterBufferMetaData.getSingleArgumentType(argumentType)));
+        addArgument(new SingleItem(argumentType, parameterBufferMetaData.getSingleArgumentType(argumentType)));
     }
 
     @Override
     public final void addArgument(int type, byte[] content) {
-        getArgumentsList().add(new ByteArrayArgument(type, parameterBufferMetaData.getByteArrayArgumentType(type), content));
+        addArgument(new ByteArrayArgument(type, parameterBufferMetaData.getByteArrayArgumentType(type), content));
+    }
+
+    protected final void addArgument(Argument argument) {
+        arguments.add(argument);
     }
 
     @Override
     public final String getArgumentAsString(int type) {
-        final List<Argument> argumentsList = getArgumentsList();
-        for (final Argument argument : argumentsList) {
-            if (argument.getType() == type) {
-                return argument.getValueAsString();
-            }
-        }
-        return null;
+        return findFirst(type).map(Argument::getValueAsString).orElse(null);
     }
 
+    @SuppressWarnings("OptionalIsPresent")
     @Override
     public final int getArgumentAsInt(int type) {
-        final List<Argument> argumentsList = getArgumentsList();
-        for (final Argument argument : argumentsList) {
-            if (argument.getType() == type) {
-                return argument.getValueAsInt();
-            }
-        }
-        return 0;
+        Optional<Argument> argumentOpt = findFirst(type);
+        return argumentOpt.isPresent() ? argumentOpt.get().getValueAsInt() : 0;
     }
 
     @Override
     public final boolean hasArgument(int type) {
-        final List<Argument> argumentsList = getArgumentsList();
-        for (final Argument argument : argumentsList) {
-            if (argument.getType() == type) return true;
-        }
-        return false;
+        return findFirst(type).isPresent();
+    }
+
+    protected Optional<Argument> findFirst(int type) {
+        return arguments.stream().filter(argument -> argument.getType() == type).findFirst();
     }
 
     @Override
     public final void removeArgument(int type) {
-        final List<Argument> argumentsList = getArgumentsList();
-        for (int i = 0, n = argumentsList.size(); i < n; i++) {
-            final Argument argument = argumentsList.get(i);
-            if (argument.getType() == type) {
-                argumentsList.remove(i);
+        Iterator<Argument> argumentIterator = arguments.iterator();
+        while (argumentIterator.hasNext()) {
+            if (argumentIterator.next().getType() == type) {
+                argumentIterator.remove();
                 return;
             }
         }
@@ -175,7 +172,7 @@ public final Iterator<Parameter> iterator() {
     }
 
     public final void writeArgumentsTo(OutputStream outputStream) throws IOException {
-        for (final Argument currentArgument : arguments) {
+        for (Argument currentArgument : arguments) {
             currentArgument.writeTo(outputStream);
         }
     }
@@ -186,9 +183,8 @@ public final Xdrable toXdrable() {
     }
 
     protected final int getLength() {
-        final List<Argument> argumentsList = getArgumentsList();
         int length = 0;
-        for (final Argument currentArgument : argumentsList) {
+        for (Argument currentArgument : arguments) {
             length += currentArgument.getLength();
         }
         return length;
@@ -200,7 +196,7 @@ protected final List<Argument> getArgumentsList() {
 
     @Override
     public final byte[] toBytes() {
-        final ByteArrayOutputStream bout = new ByteArrayOutputStream();
+        var bout = new ByteArrayOutputStream();
         try {
             writeArgumentsTo(bout);
         } catch (IOException e) {
@@ -211,7 +207,7 @@ public final byte[] toBytes() {
 
     @Override
     public final byte[] toBytesWithType() {
-        final ByteArrayOutputStream bout = new ByteArrayOutputStream();
+        final var bout = new ByteArrayOutputStream();
         try {
             bout.write(getType());
             writeArgumentsTo(bout);
@@ -229,29 +225,30 @@ public final int size() {
     @Override
     @SuppressWarnings("java:S2097")
     public final boolean equals(Object other) {
-        if (other == null || !(this.getClass().isAssignableFrom(other.getClass())))
+        if (other == null || !(this.getClass().isAssignableFrom(other.getClass()))) {
             return false;
+        }
 
         final ParameterBufferBase otherServiceBufferBase = (ParameterBufferBase) other;
-        return otherServiceBufferBase.getArgumentsList().equals(this.getArgumentsList());
+        return otherServiceBufferBase.arguments.equals(this.arguments);
     }
 
     @Override
     public final int hashCode() {
-        return getArgumentsList().hashCode();
+        return arguments.hashCode();
     }
 
     /**
      * Default implementation for serializing the parameter buffer to the XDR output stream
      */
-    private class ParameterBufferXdrable implements Xdrable {
+    private final class ParameterBufferXdrable implements Xdrable {
         @Override
         public int getLength() {
             return ParameterBufferBase.this.getLength();
         }
 
         @Override
-        public void read(XdrInputStream inputStream, int length) throws IOException {
+        public void read(XdrInputStream inputStream, int length) {
             throw new UnsupportedOperationException();
         }
 

src/main/org/firebirdsql/jaybird/xca/FBManagedConnection.java

@@ -1211,21 +1211,23 @@ public FBManagedConnectionFactory getManagedConnectionFactory() {
     }
 
     /**
-     * Set whether this connection is to be readonly
+     * Set the current TPB to read-only.
      *
      * @param readOnly
-     *         If {@code true}, the connection will be set read-only, otherwise it will be writable
+     *         if {@code true}, the connection will be set read-only, otherwise it will be writable
+     * @since 6
      */
-    public void setReadOnly(boolean readOnly) {
+    public void setTpbReadOnly(boolean readOnly) {
         tpb.setReadOnly(readOnly);
     }
 
     /**
-     * Retrieve whether this connection is readonly.
+     * Retrieve whether the current TPB is read-only.
      *
      * @return {@code true} if this connection is readonly, {@code false} otherwise
+     * @since 6
      */
-    public boolean isReadOnly() {
+    public boolean isTpbReadOnly() {
         return tpb.isReadOnly();
     }
 

src/main/org/firebirdsql/jaybird/xca/FBManagedConnectionFactory.java

@@ -323,7 +323,7 @@ public TransactionParameterBuffer getTpb(int isolation) throws SQLException {
      *         For errors on obtaining or creating the transaction mapping
      */
     FBTpbMapper getTransactionMappingCopy() throws SQLException {
-        return (FBTpbMapper) connectionProperties.getMapper().clone();
+        return FBTpbMapper.copyOf(connectionProperties.getMapper());
     }
 
     /**

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

@@ -91,6 +91,7 @@ public class FBConnection implements FirebirdConnection {
     private StoredProcedureMetaData storedProcedureMetaData;
     private GeneratedKeysSupport generatedKeysSupport;
     private ClientInfoProvider clientInfoProvider;
+    private boolean readOnly;
 
     /**
      * Create a new FBConnection instance based on a {@link FBManagedConnection}.
@@ -107,6 +108,9 @@ public FBConnection(FBManagedConnection mc) {
         resultSetHoldability = props.isDefaultResultSetHoldable()
                 ? ResultSet.HOLD_CURSORS_OVER_COMMIT
                 : ResultSet.CLOSE_CURSORS_AT_COMMIT;
+
+        // Inherit read-only state from the initial TPB
+        readOnly = mc.isTpbReadOnly();
     }
 
     @Override
@@ -603,15 +607,16 @@ public void setReadOnly(boolean readOnly) throws SQLException {
                         "Calling setReadOnly(boolean) method is not allowed when transaction is already started.",
                         SQL_STATE_TX_ACTIVE);
             }
-            mc.setReadOnly(readOnly);
+            this.readOnly = readOnly;
+            mc.setTpbReadOnly(readOnly);
         }
     }
 
     @Override
     public boolean isReadOnly() throws SQLException {
         try (LockCloseable ignored = withLock()) {
             checkValidity();
-            return mc.isReadOnly();
+            return readOnly;
         }
     }
 

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

@@ -108,7 +108,7 @@ public Object clone() {
             clone.properties = (FbConnectionProperties) properties.asNewMutable();
             clone.properties.registerPropertyUpdateListener(clone.createPropertyUpdateListener());
             clone.customMapping = new HashMap<>(customMapping);
-            clone.mapper = mapper != null ? (FBTpbMapper) mapper.clone() : null;
+            clone.mapper = mapper != null ? FBTpbMapper.copyOf(mapper) : null;
 
             return clone;
         } catch (CloneNotSupportedException ex) {