#825 Add option to disable reporting of SQLWarnings

devdoc/jdp/jdp-2024-08-optionally-disable-sql-warnings.adoc

@@ -2,8 +2,8 @@
 
 == Status
 
-* Draft
-* Proposed for: Jaybird 6
+* Published: 2024-11-06
+* Implemented in: Jaybird 6
 
 == Type
 
@@ -19,21 +19,22 @@ Our experience is that users don't like ``SQLWarning``s, especially if tools log
 == Decision
 
 Jaybird will add a connection property and a system property to disable reporting of all ``SQLWarning``s.
-The property will also be added to data sources as an explicit setter.
+The property will also be added to data sources as an explicit getter/setter pair.
 
 The connection property is called `reportSQLWarnings` with case-insensitive values:
 
 [horizontal]
 `ALL`:: Report all ``SQLWarning``s (the default)
 `NONE`:: Report no ``SQLWarning``s
 
-Invalid values will behave as `ALL`.
+Invalid values set as a connection property will be rejected.
 `ALL` is the default because it is behaviour required by the JDBC Specification.
 
 The use of names instead of Boolean values leaves the option open to add a value `SERVER` to report server-generated warnings, but not driver-generated warnings.
 However, at this time we don't think that is needed (especially as Firebird has very few warnings to report).
 
 The system property is called `org.firebirdsql.jdbc.defaultReportSQLWarnings` with the same values.
+Invalid values set through the system property will be ignored.
 It will be dynamically checked when the connection configuration is created.
 
 The ignored warnings will not be logged.

src/docs/asciidoc/release_notes.adoc

@@ -1133,6 +1133,9 @@ If the JSpecify JAR is not included on the classpath or modulepath, Jaybird will
 +
 This affects the return value of the methods `supportsResultSetType(int)`, `supportsResultSetConcurrency(int, int)`, `ownUpdatesAreVisible(int)`, `ownDeletesAreVisible(int)`, `ownInsertsAreVisible(int)`.
 * Improvement: `setObject`/`updateObject` methods on `PreparedStatement`, `CallableStatement` and `ResultSet` with the `int scaleOrLength` parameter will now redirect to variants accepting a length of `set/updateBinaryStream` for `InputStream` and `set/updateCharacterStream` for `Reader` (https://github.com/FirebirdSQL/jaybird/issues/822[#822])
+* New feature: Reporting of ``SQLWarning``s can be disabled with connection property `reportSQLWarnings` (supported case-insensitive values: `ALL` (default), `NONE`) (https://github.com/FirebirdSQL/jaybird/issues/825[#825])
++
+The default can also be configured globally using system property `org.firebirdsql.jdbc.defaultReportSQLWarnings`.
 
 [#potentially-breaking-changes]
 === Potentially breaking changes

src/main/org/firebirdsql/ds/AbstractConnectionPropertiesDataSource.java

@@ -486,6 +486,16 @@ public void setCreateDatabaseIfNotExist(boolean createDatabaseIfNotExist) {
         FirebirdConnectionProperties.super.setCreateDatabaseIfNotExist(createDatabaseIfNotExist);
     }
 
+    @Override
+    public String getReportSQLWarnings() {
+        return FirebirdConnectionProperties.super.getReportSQLWarnings();
+    }
+
+    @Override
+    public void setReportSQLWarnings(String reportSQLWarnings) {
+        FirebirdConnectionProperties.super.setReportSQLWarnings(reportSQLWarnings);
+    }
+
     @SuppressWarnings("deprecation")
     @Deprecated(since = "5")
     @Override

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

@@ -40,6 +40,7 @@ public final class JaybirdSystemProperties {
     public static final String DEFAULT_CONNECTION_ENCODING_PROPERTY = JDBC_PREFIX + "defaultConnectionEncoding";
     public static final String REQUIRE_CONNECTION_ENCODING_PROPERTY = JDBC_PREFIX + "requireConnectionEncoding";
     public static final String DEFAULT_ENABLE_PROTOCOL = JDBC_PREFIX + "defaultEnableProtocol";
+    public static final String DEFAULT_REPORT_SQL_WARNINGS = JDBC_PREFIX + "defaultReportSQLWarnings";
     public static final String DATATYPE_CODER_CACHE_SIZE = COMMON_PREFIX + "datatypeCoderCacheSize";
     public static final String NATIVE_LIBRARY_SHUTDOWN_DISABLED = COMMON_PREFIX + "nativeResourceShutdownDisabled";
     public static final String WIRE_DEFLATE_BUFFER_SIZE = WIRE_PREFIX + "deflateBufferSize";
@@ -104,6 +105,10 @@ public static String getDefaultEnableProtocol() {
         return getSystemPropertyPrivileged(DEFAULT_ENABLE_PROTOCOL);
     }
 
+    public static String getDefaultReportSQLWarnings() {
+        return getSystemPropertyPrivileged(DEFAULT_REPORT_SQL_WARNINGS);
+    }
+
     private static int getWithDefault(String propertyName, int defaultValue) {
         Integer value = getIntegerSystemPropertyPrivileged(propertyName);
         return value != null ? value : defaultValue;

src/main/org/firebirdsql/gds/ng/FbConnectionProperties.java

@@ -18,6 +18,7 @@
  */
 package org.firebirdsql.gds.ng;
 
+import org.firebirdsql.gds.JaybirdSystemProperties;
 import org.firebirdsql.jaybird.props.PropertyConstants;
 import org.firebirdsql.jaybird.props.PropertyNames;
 import org.firebirdsql.jaybird.props.def.ConnectionProperty;
@@ -64,6 +65,11 @@ public FbConnectionProperties(IConnectionProperties src) {
     public FbConnectionProperties() {
         setSessionTimeZone(defaultTimeZone());
         setSqlDialect(PropertyConstants.DEFAULT_DIALECT);
+        try {
+            setReportSQLWarnings(JaybirdSystemProperties.getDefaultReportSQLWarnings());
+        } catch (IllegalArgumentException ignored) {
+            // Incorrect value, ignore
+        }
     }
 
     // For internal use, to provide serialization support

src/main/org/firebirdsql/jaybird/props/DatabaseConnectionProperties.java

@@ -26,7 +26,10 @@
 
 import org.firebirdsql.jdbc.FirebirdCallableStatement;
 
+import java.sql.Connection;
 import java.sql.DatabaseMetaData;
+import java.sql.ResultSet;
+import java.sql.Statement;
 
 /**
  * Properties for database connections.
@@ -234,19 +237,19 @@ default void setUseStreamBlobs(boolean useStreamBlobs) {
     /**
      * Get whether ResultSets are holdable by default.
      *
-     * @return {@code true} ResultSets by default are {@link java.sql.ResultSet#HOLD_CURSORS_OVER_COMMIT},
-     * {@code false} (default), ResultSets are {@link java.sql.ResultSet#CLOSE_CURSORS_AT_COMMIT}
+     * @return {@code true} ResultSets by default are {@link ResultSet#HOLD_CURSORS_OVER_COMMIT},
+     * {@code false} (default), ResultSets are {@link ResultSet#CLOSE_CURSORS_AT_COMMIT}
      */
     default boolean isDefaultResultSetHoldable() {
         return getBooleanProperty(PropertyNames.defaultResultSetHoldable, PropertyConstants.DEFAULT_RESULT_SET_HOLDABLE);
     }
 
     /**
-     * Set if {@link java.sql.ResultSet} should be {@link java.sql.ResultSet#HOLD_CURSORS_OVER_COMMIT} by default.
+     * Set if {@link ResultSet} should be {@link ResultSet#HOLD_CURSORS_OVER_COMMIT} by default.
      *
      * @param defaultResultSetHoldable
-     *         {@code true} ResultSets are holdable, {@code false} (default) ResultSets are {@link
-     *         java.sql.ResultSet#CLOSE_CURSORS_AT_COMMIT}
+     *         {@code true} ResultSets are holdable, {@code false} (default) ResultSets are
+     *         {@link ResultSet#CLOSE_CURSORS_AT_COMMIT}
      */
     default void setDefaultResultSetHoldable(boolean defaultResultSetHoldable) {
         setBooleanProperty(PropertyNames.defaultResultSetHoldable, defaultResultSetHoldable);
@@ -409,7 +412,7 @@ default String getTpbMapping() {
      * </p>
      * <p>
      * The resource bundle should contain a mapping between the transaction isolation level (name of the constant in
-     * the {@link java.sql.Connection} interface and a comma-separated list of TPB parameters).
+     * the {@link Connection} interface and a comma-separated list of TPB parameters).
      * </p>
      * <p>
      *
@@ -623,11 +626,11 @@ default boolean isAllowTxStmts() {
      * executed.
      * <p>
      * Setting to {@code true} will enable Jaybird to execute <em>equivalent</em> operations through the JDBC API
-     * (specifically, {@link java.sql.Statement#execute(String)}, {@link java.sql.Statement#executeUpdate(String)},
-     * {@link java.sql.Statement#executeLargeUpdate(String)} and siblings, and statements prepared with
-     * {@link java.sql.Connection#prepareStatement(String)} and siblings. Using callable statements (e.g. using
-     * {@link java.sql.Connection#prepareCall(String)}), {@link java.sql.Statement#executeQuery(String)}, or batch
-     * execution is never supported.
+     * (specifically, {@link Statement#execute(String)}, {@link Statement#executeUpdate(String)},
+     * {@link Statement#executeLargeUpdate(String)} and siblings, and statements prepared with
+     * {@link Connection#prepareStatement(String)} and siblings. Using callable statements (e.g. using
+     * {@link Connection#prepareCall(String)}), {@link Statement#executeQuery(String)}, or batch execution is never
+     * supported.
      * </p>
      * <p>
      * The implementation is free to execute the provided statement text or use an equivalent operation that has
@@ -713,4 +716,39 @@ default void setCreateDatabaseIfNotExist(boolean createDatabaseIfNotExist) {
         setBooleanProperty(PropertyNames.createDatabaseIfNotExist, createDatabaseIfNotExist);
     }
 
+    /**
+     * @return {@code ALL} (default) if {@link java.sql.SQLWarning} should be reported by {@link Connection},
+     * {@link Statement} and {@link ResultSet}, {@code NONE} if {@link java.sql.SQLWarning} should not be reported
+     * @see #setReportSQLWarnings(String)
+     * @since 6
+     */
+    default String getReportSQLWarnings() {
+        return getProperty(PropertyNames.reportSQLWarnings, PropertyConstants.DEFAULT_REPORT_SQL_WARNINGS);
+    }
+
+    /**
+     * Sets if {@link java.sql.SQLWarning} should be reported by {@link Connection#getWarnings()},
+     * {@link Statement#getWarnings()}, and {@link ResultSet#getWarnings()}.
+     * <p>
+     * Allowed values (case-insensitive):
+     * <ul>
+     * <li>ALL &mdash; (default) report all {@link java.sql.SQLWarning}</li>
+     * <li>NONE &mdash; report no {@link java.sql.SQLWarning}; this behaviour is not JDBC-compliant</li>
+     * </ul>
+     * </p>
+     * <p>
+     * The default value can be overridden by setting system property
+     * {@code org.firebirdsql.jdbc.defaultReportSQLWarnings}.
+     * </p>
+     *
+     * @param reportSQLWarnings
+     *         {@code ALL} (default) if {@link java.sql.SQLWarning} should be reported by {@link Connection},
+     *         {@link Statement} and {@link ResultSet}, {@code NONE} if {@link java.sql.SQLWarning} should not be
+     *         reported; setting {@code null} will use {@code ALL}
+     * @since 6
+     */
+    default void setReportSQLWarnings(String reportSQLWarnings) {
+        setProperty(PropertyNames.reportSQLWarnings, reportSQLWarnings);
+    }
+
 }

src/main/org/firebirdsql/jaybird/props/PropertyConstants.java

@@ -68,6 +68,10 @@ public final class PropertyConstants {
     public static final String SCROLLABLE_CURSOR_SERVER = "SERVER";
     public static final String DEFAULT_SCROLLABLE_CURSOR = SCROLLABLE_CURSOR_EMULATED;
 
+    public static final String REPORT_SQL_WARNINGS_ALL = "ALL";
+    public static final String REPORT_SQL_WARNINGS_NONE = "NONE";
+    public static final String DEFAULT_REPORT_SQL_WARNINGS = REPORT_SQL_WARNINGS_ALL;
+
     public static final int TIMEOUT_NOT_SET = -1;
     public static final int BUFFER_SIZE_NOT_SET = -1;
     static final int PARALLEL_WORKERS_NOT_SET = -1;

src/main/org/firebirdsql/jaybird/props/PropertyNames.java

@@ -86,6 +86,7 @@ public final class PropertyNames {
     public static final String allowTxStmts = "allowTxStmts";
     public static final String extendedMetadata = "extendedMetadata";
     public static final String createDatabaseIfNotExist = "createDatabaseIfNotExist";
+    public static final String reportSQLWarnings = "reportSQLWarnings";
 
     // service connection
     public static final String expectedDb = "expectedDb";

src/main/org/firebirdsql/jaybird/props/internal/StandardConnectionPropertyDefiner.java

@@ -114,6 +114,7 @@ public Stream<ConnectionProperty> defineProperties() {
                 builder(allowTxStmts).type(BOOLEAN),
                 builder(extendedMetadata).type(BOOLEAN),
                 builder(createDatabaseIfNotExist).type(BOOLEAN),
+                builder(reportSQLWarnings).choices(REPORT_SQL_WARNINGS_ALL, REPORT_SQL_WARNINGS_NONE),
 
                 // TODO Consider removing this property, otherwise formally add it to PropertyNames
                 builder("filename_charset"),

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

@@ -326,6 +326,13 @@ public final void setCursorName(@Nullable String cursorName) throws SQLException
         return cursorName;
     }
 
+    /**
+     * {@inheritDoc}
+     * <p>
+     * If connection property {@code reportSQLWarnings} is set to {@code NONE} (case-insensitive), this method will
+     * not report warnings and always return {@code null}.
+     * </p>
+     */
     @Override
     public final @Nullable SQLWarning getWarnings() throws SQLException {
         checkValidity();
@@ -340,6 +347,7 @@ public final void clearWarnings() throws SQLException {
 
     protected final void addWarning(SQLWarning warning) {
         try (var ignored = withLock()) {
+            if (connection.isIgnoreSQLWarnings()) return;
             SQLWarning currentWarning = this.warning;
             if (currentWarning == null) {
                 this.warning = warning;

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

@@ -666,6 +666,13 @@ public int getTransactionIsolation() throws SQLException {
         }
     }
 
+    /**
+     * {@inheritDoc}
+     * <p>
+     * If connection property {@code reportSQLWarnings} is set to {@code NONE} (case-insensitive), this method will
+     * not report warnings and always return {@code null}.
+     * </p>
+     */
     @Override
     public SQLWarning getWarnings() throws SQLException {
         try (LockCloseable ignored = withLock()) {
@@ -1085,6 +1092,7 @@ public String getSchema() throws SQLException {
 
     public void addWarning(SQLWarning warning) {
         try (LockCloseable ignored = withLock()) {
+            if (isIgnoreSQLWarnings()) return;
             // TODO: Find way so this method can be protected (or less visible) again.
             if (firstWarning == null)
                 firstWarning = warning;
@@ -1424,4 +1432,10 @@ boolean isExtendedMetadata() {
         return props != null && props.isExtendedMetadata();
     }
 
+    boolean isIgnoreSQLWarnings() {
+        DatabaseConnectionProperties props = connectionProperties();
+        return props != null
+               && PropertyConstants.REPORT_SQL_WARNINGS_NONE.equalsIgnoreCase(props.getReportSQLWarnings());
+    }
+
 }

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

@@ -695,6 +695,10 @@ public double getDouble(String columnName) throws SQLException {
     /**
      * {@inheritDoc}
      * <p>
+     * If connection property {@code reportSQLWarnings} is set to {@code NONE} (case-insensitive), this method will
+     * not report warnings and always return {@code null}.
+     * </p>
+     * <p>
      * <b>NOTE:</b> The implementation currently always returns {@code null} as warnings are never recorded for result
      * sets.
      * </p>

src/test/org/firebirdsql/gds/ng/FbConnectionPropertiesTest.java

@@ -276,4 +276,29 @@ void enableProtocolDefaultDerivedFromSystemProperty(String defaultValue) {
         }
     }
 
+    @Test
+    void defaultReportSQLWarningsValue() {
+        assertEquals(PropertyConstants.DEFAULT_REPORT_SQL_WARNINGS, info.getReportSQLWarnings(),
+                "Unexpected reportSQLWarnings value");
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = { "ALL", "NONE" })
+    void reportSQLWarningsDefaultDerivedFromSystemProperty(String defaultValue) {
+        try (var ignored = withTemporarySystemProperty(
+                JaybirdSystemProperties.DEFAULT_REPORT_SQL_WARNINGS, defaultValue)) {
+            assertEquals(defaultValue, new FbConnectionProperties().getReportSQLWarnings(),
+                    "Unexpected reportSQLWarnings value");
+        }
+    }
+
+    @Test
+    void reportSQLWarnings_invalidSystemPropertyValue_reportsALL() {
+        try (var ignored = withTemporarySystemProperty(
+                JaybirdSystemProperties.DEFAULT_REPORT_SQL_WARNINGS, "INVALID_VALUE")) {
+            assertEquals(PropertyConstants.DEFAULT_REPORT_SQL_WARNINGS,
+                    new FbConnectionProperties().getReportSQLWarnings(), "Unexpected reportSQLWarnings value");
+        }
+    }
+
 }

src/test/org/firebirdsql/jdbc/FBConnectionTest.java

@@ -42,13 +42,15 @@
 import org.junit.jupiter.api.extension.RegisterExtension;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.EnumSource;
+import org.junit.jupiter.params.provider.NullSource;
 import org.junit.jupiter.params.provider.ValueSource;
 import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
 
 import java.nio.charset.StandardCharsets;
 import java.sql.*;
 import java.util.List;
+import java.util.Map;
 import java.util.Properties;
 import java.util.concurrent.*;
 
@@ -999,6 +1001,31 @@ void readOnlyShouldInheritFromTransactionConfigurationOfDefaultIsolation() throw
         }
     }
 
+    @ParameterizedTest
+    @NullSource
+    @ValueSource(strings = { "ALL", "all" })
+    void reportSQLWarnings_ALL_or_default_reportsWarning(String reportSQLWarning) throws Exception {
+        Map<String, String> props =
+                reportSQLWarning != null ? Map.of(PropertyNames.reportSQLWarnings, reportSQLWarning) : Map.of();
+        try (FBConnection connection = (FBConnection) getConnectionViaDriverManager(props)) {
+            var warning = new SQLWarning("test");
+            connection.addWarning(warning);
+
+            assertSame(warning, connection.getWarnings(), "Expected warning to be reported");
+        }
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = { "NONE", "none" })
+    void reportSQLWarnings_NONE_ignoresWarning(String reportSQLWarning) throws Exception {
+        try (FBConnection connection =
+                     (FBConnection) getConnectionViaDriverManager(PropertyNames.reportSQLWarnings, reportSQLWarning)) {
+            connection.addWarning(new SQLWarning("test"));
+
+            assertNull(connection.getWarnings(), "Expected warning to be ignored");
+        }
+    }
+
     /**
      * Single-use executor, delays the command to be executed until signalled.
      */