Fix nested transaction savepoint handling in JDBC transaction manager (…

…#3047) * Fix nested transaction savepoint handling in JDBC transaction manager * Fix comments * Fix the test. * Use connection.setSavepoint() without passing name. * Update savepoint operations for Hibernate transaction manager as well. * Renamed test * Removed transaction savepoint error checks from AbstractDefaultTransactionOperations class
micronaut-projects · Aug 5, 2024 · 9fdf0e9 · 9fdf0e9
1 parent 7dfac6f
commit 9fdf0e9
Show file tree

Hide file tree

Showing 15 changed files with 228 additions and 10 deletions.
diff --git a/data-jdbc/src/test/groovy/io/micronaut/data/jdbc/mariadb/MariaTransactionsSpec.groovy b/data-jdbc/src/test/groovy/io/micronaut/data/jdbc/mariadb/MariaTransactionsSpec.groovy
@@ -0,0 +1,14 @@
+package io.micronaut.data.jdbc.mariadb
+
+import io.micronaut.data.jdbc.AbstractJdbcTransactionSpec
+import io.micronaut.data.jdbc.mysql.MySqlBookRepository
+import io.micronaut.data.tck.repositories.BookRepository
+
+class MariaTransactionsSpec extends AbstractJdbcTransactionSpec implements MariaTestPropertyProvider {
+
+    @Override
+    Class<? extends BookRepository> getBookRepositoryClass() {
+        return MySqlBookRepository.class
+    }
+
+}
diff --git a/data-jdbc/src/test/groovy/io/micronaut/data/jdbc/mysql/MySqlTransactionsSpec.groovy b/data-jdbc/src/test/groovy/io/micronaut/data/jdbc/mysql/MySqlTransactionsSpec.groovy
@@ -0,0 +1,23 @@
+package io.micronaut.data.jdbc.mysql
+
+import io.micronaut.data.jdbc.AbstractJdbcTransactionSpec
+import io.micronaut.data.tck.repositories.BookRepository
+
+class MySqlTransactionsSpec extends AbstractJdbcTransactionSpec implements MySQLTestPropertyProvider {
+
+    @Override
+    Class<? extends BookRepository> getBookRepositoryClass() {
+        return MySqlBookRepository.class
+    }
+
+    @Override
+    boolean failsInsertInReadOnlyTx() {
+        return true
+    }
+
+    @Override
+    boolean cannotInsertInReadOnlyTx(Exception e) {
+        assert e.cause.message == "Connection is read-only. Queries leading to data modification are not allowed"
+        return true
+    }
+}
diff --git a/data-jdbc/src/test/groovy/io/micronaut/data/jdbc/oraclexe/OracleTransactionsSpec.groovy b/data-jdbc/src/test/groovy/io/micronaut/data/jdbc/oraclexe/OracleTransactionsSpec.groovy
@@ -0,0 +1,15 @@
+package io.micronaut.data.jdbc.oraclexe
+
+import io.micronaut.data.jdbc.AbstractJdbcTransactionSpec
+import io.micronaut.data.jdbc.postgres.PostgresBookRepository
+import io.micronaut.data.jdbc.postgres.PostgresTestPropertyProvider
+import io.micronaut.data.tck.repositories.BookRepository
+
+class OracleTransactionsSpec extends AbstractJdbcTransactionSpec implements OracleTestPropertyProvider {
+
+    @Override
+    Class<? extends BookRepository> getBookRepositoryClass() {
+        return OracleXEBookRepository.class
+    }
+
+}
diff --git a/data-jdbc/src/test/groovy/io/micronaut/data/jdbc/postgres/PostgresTransactionsSpec.groovy b/data-jdbc/src/test/groovy/io/micronaut/data/jdbc/postgres/PostgresTransactionsSpec.groovy
@@ -25,6 +25,11 @@ class PostgresTransactionsSpec extends AbstractJdbcTransactionSpec implements Po
         return PostgresBookRepository.class
     }
 
+    @Override
+    boolean failsInsertInReadOnlyTx() {
+        return true
+    }
+
     @Override
     boolean cannotInsertInReadOnlyTx(Exception e) {
         assert e.cause.message == "ERROR: cannot execute INSERT in a read-only transaction"

diff --git a/data-jdbc/src/test/groovy/io/micronaut/data/jdbc/sqlserver/SqlServerTransactionsSpec.groovy b/data-jdbc/src/test/groovy/io/micronaut/data/jdbc/sqlserver/SqlServerTransactionsSpec.groovy
@@ -0,0 +1,13 @@
+package io.micronaut.data.jdbc.sqlserver
+
+import io.micronaut.data.jdbc.AbstractJdbcTransactionSpec
+import io.micronaut.data.tck.repositories.BookRepository
+
+class SqlServerTransactionsSpec extends AbstractJdbcTransactionSpec implements MSSQLTestPropertyProvider {
+
+    @Override
+    Class<? extends BookRepository> getBookRepositoryClass() {
+        return MSBookRepository.class
+    }
+
+}
diff --git a/...bc/src/test/groovy/io/micronaut/data/spring/jdbc/SpringPostgresJdbcTransactionSpec.groovy b/...bc/src/test/groovy/io/micronaut/data/spring/jdbc/SpringPostgresJdbcTransactionSpec.groovy
@@ -71,6 +71,11 @@ class SpringPostgresJdbcTransactionSpec extends AbstractTransactionSpec implemen
         return false
     }
 
+    @Override
+    boolean failsInsertInReadOnlyTx() {
+        return true;
+    }
+
     @Override
     boolean cannotInsertInReadOnlyTx(Exception e) {
         assert e.cause.message == "ERROR: cannot execute INSERT in a read-only transaction"

diff --git a/data-tck/src/main/groovy/io/micronaut/data/tck/tests/AbstractTransactionSpec.groovy b/data-tck/src/main/groovy/io/micronaut/data/tck/tests/AbstractTransactionSpec.groovy
@@ -63,6 +63,10 @@ abstract class AbstractTransactionSpec extends Specification implements TestProp
         return false
     }
 
+    boolean failsInsertInReadOnlyTx() {
+        return false
+    }
+
     void "custom name transaction"() {
         when:
             bookService.bookAddedCustomNamedTransaction(new Runnable() {
@@ -77,8 +81,8 @@ abstract class AbstractTransactionSpec extends Specification implements TestProp
             assert bookService.countBooksTransactional() == 1
     }
 
-    void "test book added in read only transaction"() {
-        if (!supportsReadOnlyFlag()) {
+    void "test book added in read only transaction throws error"() {
+        if (!supportsReadOnlyFlag() || !failsInsertInReadOnlyTx()) {
             return
         }
         when:
@@ -88,8 +92,19 @@ abstract class AbstractTransactionSpec extends Specification implements TestProp
             cannotInsertInReadOnlyTx(e)
     }
 
-    void "test read only transaction adding book in inner transaction"() {
-        if (!supportsReadOnlyFlag()) {
+    void "test book added in read only transaction not throwing error"() {
+        if (!supportsReadOnlyFlag() || failsInsertInReadOnlyTx()) {
+            return
+        }
+        when:
+        bookService.bookAddedInReadOnlyTransaction()
+        then:
+        noExceptionThrown()
+        bookService.countBooksTransactional() == 1
+    }
+
+    void "test read only transaction adding book in inner transaction throws error"() {
+        if (!supportsReadOnlyFlag() || !failsInsertInReadOnlyTx()) {
             return
         }
         when:
@@ -99,6 +114,17 @@ abstract class AbstractTransactionSpec extends Specification implements TestProp
             cannotInsertInReadOnlyTx(e)
     }
 
+    void "test read only transaction adding book in inner transaction not throwing error"() {
+        if (!supportsReadOnlyFlag() || failsInsertInReadOnlyTx()) {
+            return
+        }
+        when:
+        bookService.readOnlyTxCallingAddingBookInAnotherTransaction()
+        then:
+        noExceptionThrown()
+        bookService.countBooksTransactional() == 1
+    }
+
     void "test book added in never propagation"() {
         if (!supportsNoTxProcessing()) {
             return

diff --git a/data-tck/src/main/java/io/micronaut/data/tck/services/TxBookService.java b/data-tck/src/main/java/io/micronaut/data/tck/services/TxBookService.java
@@ -69,7 +69,7 @@ public void bookAddedInNestedTransactionSync() {
         transactionManager.commit(transaction);
     }
 
-    @Transactional
+    @Transactional(propagation = TransactionDefinition.Propagation.NESTED)
     public void bookAddedInAnotherNestedTransaction() {
         bookAddedInNestedTransaction();
     }

diff --git a/data-tx-hibernate/build.gradle b/data-tx-hibernate/build.gradle
@@ -27,6 +27,10 @@ dependencies {
     testRuntimeOnly mnSql.micronaut.jdbc.tomcat
     testRuntimeOnly mnSql.postgresql
     testResourcesService mnSql.postgresql
+    testRuntimeOnly mnSql.mssql.jdbc
+    testResourcesService mnSql.mssql.jdbc
+    testRuntimeOnly mnSql.ojdbc11
+    testResourcesService mnSql.ojdbc11
 }
 
 micronaut {
@@ -35,6 +39,8 @@ micronaut {
         enabled = true
         inferClasspath = false
         additionalModules.add(KnownModules.JDBC_POSTGRESQL)
+        additionalModules.add(KnownModules.JDBC_MSSQL)
+        additionalModules.add(KnownModules.JDBC_ORACLE_FREE)
         clientTimeout = 300
         version = libs.versions.micronaut.testresources.get()
     }

diff --git a/...bernate/src/main/java/io/micronaut/transaction/hibernate/HibernateTransactionManager.java b/...bernate/src/main/java/io/micronaut/transaction/hibernate/HibernateTransactionManager.java
@@ -45,6 +45,7 @@
 import java.sql.Connection;
 import java.sql.ResultSet;
 import java.sql.SQLException;
+import java.sql.SQLFeatureNotSupportedException;
 import java.sql.Savepoint;
 import java.util.ArrayList;
 import java.util.Collections;
@@ -64,6 +65,9 @@
 @TypeHint(HibernateTransactionManager.class)
 public final class HibernateTransactionManager extends AbstractDefaultTransactionOperations<Session> {
 
+    // Error with this message is thrown from SQL server when operation is not supported (like Connection.releaseSavepoint)
+    private static final String OPERATION_NOT_SUPPORTED = "This operation is not supported.";
+
     private boolean prepareConnection = true;
 
     private boolean allowResultAccessAfterCompletion = false;
@@ -211,7 +215,7 @@ protected void doRollback(DefaultTransactionStatus<Session> tx) {
     protected void doNestedBegin(DefaultTransactionStatus<Session> status) {
         try {
             Session session = status.getConnection();
-            Savepoint savepoint = getConnection(session).setSavepoint(status.getTransactionDefinition().getName());
+            Savepoint savepoint = getConnection(session).setSavepoint();
             status.setSavepoint(savepoint);
         } catch (SQLException e) {
             throw new CannotCreateTransactionException("Could not create JDBC savepoint", e);
@@ -228,7 +232,13 @@ protected void doNestedCommit(DefaultTransactionStatus<Session> status) {
             try {
                 getConnection(session).releaseSavepoint((Savepoint) status.getSavepoint());
             } catch (Exception e) {
-                throw new TransactionSystemException("Could release JDBC savepoint", e);
+                if (isUnsupportedOperation(e)) {
+                    if (logger.isDebugEnabled()) {
+                        logger.debug("JDBC SavePoint release not supported by the Session [{}]", session, e);
+                    }
+                } else {
+                    throw new TransactionSystemException("Could not release JDBC savepoint", e);
+                }
             }
         } else {
             throw new TransactionSystemException("Missing a JDBC savepoint");
@@ -307,4 +317,20 @@ private Connection getConnection(Session session) {
         return ((SessionImplementor) session).getJdbcCoordinator().getLogicalConnection().getPhysicalConnection();
     }
 
+    /**
+     * Checks if thrown exception is from the JDBC driver telling that feature is not supported.
+     * For example, some drivers don't support {@link Connection#releaseSavepoint(Savepoint)}
+     * and we want to handle that case and continue execution.
+     *
+     * @param exception The thrown exception
+     * @return true if exception is thrown for unsupported operation
+     */
+    private static boolean isUnsupportedOperation(Exception exception) {
+        if (exception instanceof SQLFeatureNotSupportedException) {
+            return true;
+        } else if (exception instanceof SQLException sqlException) {
+            return OPERATION_NOT_SUPPORTED.equals(sqlException.getMessage());
+        }
+        return false;
+    }
 }
diff --git a/...rnate/src/test/groovy/io/micronaut/transaction/hibernate6/HibernateTransactionSpec.groovy b/...rnate/src/test/groovy/io/micronaut/transaction/hibernate6/HibernateTransactionSpec.groovy
@@ -38,7 +38,7 @@ class HibernateTransactionSpec extends AbstractTransactionSpec implements TestRe
     @Override
     Map<String, String> getProperties() {
         return TestResourcesDatabaseTestPropertyProvider.super.getProperties() + [
-                "datasources.default.name"                     : "mydb",
+                "datasources.default.name"                     : "mypgdb",
                 'jpa.default.properties.hibernate.hbm2ddl.auto': 'create-drop',
                 'jpa.default.properties.hibernate.dialect'     : 'org.hibernate.dialect.PostgreSQLDialect'
         ]

diff --git a/...src/test/groovy/io/micronaut/transaction/hibernate6/OracleHibernateTransactionSpec.groovy b/...src/test/groovy/io/micronaut/transaction/hibernate6/OracleHibernateTransactionSpec.groovy
@@ -0,0 +1,27 @@
+package io.micronaut.transaction.hibernate6
+
+import io.micronaut.data.model.query.builder.sql.Dialect
+import io.micronaut.data.tck.tests.TestResourcesDatabaseTestPropertyProvider
+
+class OracleHibernateTransactionSpec extends HibernateTransactionSpec implements TestResourcesDatabaseTestPropertyProvider {
+
+    @Override
+    Dialect dialect() {
+        return Dialect.ORACLE
+    }
+
+    @Override
+    Map<String, String> getProperties() {
+        return TestResourcesDatabaseTestPropertyProvider.super.getProperties() + [
+                "datasources.default.name"                     : "myoracledb",
+                'jpa.default.properties.hibernate.hbm2ddl.auto': 'create-drop',
+                'jpa.default.properties.hibernate.dialect'     : 'org.hibernate.dialect.OracleDialect'
+        ]
+    }
+
+    @Override
+    boolean supportsReadOnlyFlag() {
+        return true
+    }
+
+}
diff --git a/.../test/groovy/io/micronaut/transaction/hibernate6/SqlServerHibernateTransactionSpec.groovy b/.../test/groovy/io/micronaut/transaction/hibernate6/SqlServerHibernateTransactionSpec.groovy
@@ -0,0 +1,29 @@
+package io.micronaut.transaction.hibernate6
+
+
+import io.micronaut.data.model.query.builder.sql.Dialect
+import io.micronaut.data.tck.tests.TestResourcesDatabaseTestPropertyProvider
+
+class SqlServerHibernateTransactionSpec extends HibernateTransactionSpec implements TestResourcesDatabaseTestPropertyProvider {
+
+    @Override
+    Dialect dialect() {
+        return Dialect.SQL_SERVER
+    }
+
+    @Override
+    Map<String, String> getProperties() {
+        return TestResourcesDatabaseTestPropertyProvider.super.getProperties() + [
+                "datasources.default.name"                       : "mymssqldb",
+                'jpa.default.properties.hibernate.hbm2ddl.auto'  : 'create-drop',
+                'jpa.default.properties.hibernate.dialect'       : 'org.hibernate.dialect.SQLServerDialect',
+                'test-resources.containers.mssql.accept-license' : 'true'
+        ]
+    }
+
+    @Override
+    boolean supportsReadOnlyFlag() {
+        return true
+    }
+
+}
diff --git a/data-tx-jdbc/src/main/java/io/micronaut/transaction/jdbc/DataSourceTransactionManager.java b/data-tx-jdbc/src/main/java/io/micronaut/transaction/jdbc/DataSourceTransactionManager.java
@@ -36,6 +36,7 @@
 import javax.sql.DataSource;
 import java.sql.Connection;
 import java.sql.SQLException;
+import java.sql.SQLFeatureNotSupportedException;
 import java.sql.Savepoint;
 import java.sql.Statement;
 import java.util.ArrayList;
@@ -56,6 +57,9 @@
 @TypeHint(DataSourceTransactionManager.class)
 public final class DataSourceTransactionManager extends AbstractDefaultTransactionOperations<Connection> {
 
+    // Error with this message is thrown from SQL server when operation is not supported (like Connection.releaseSavepoint)
+    private static final String OPERATION_NOT_SUPPORTED = "This operation is not supported.";
+
     private final DataSource dataSource;
 
     private boolean enforceReadOnly = false;
@@ -176,7 +180,7 @@ protected void doRollback(DefaultTransactionStatus<Connection> status) {
     protected void doNestedBegin(DefaultTransactionStatus<Connection> status) {
         try {
             Connection connection = status.getConnection();
-            Savepoint savepoint = connection.setSavepoint(status.getTransactionDefinition().getName());
+            Savepoint savepoint = connection.setSavepoint();
             status.setSavepoint(savepoint);
         } catch (SQLException e) {
             throw new CannotCreateTransactionException("Could not create JDBC savepoint", e);
@@ -193,7 +197,13 @@ protected void doNestedCommit(DefaultTransactionStatus<Connection> status) {
             try {
                 connection.releaseSavepoint((Savepoint) status.getSavepoint());
             } catch (Exception e) {
-                throw new TransactionSystemException("Could release JDBC savepoint", e);
+                if (isUnsupportedOperation(e)) {
+                    if (logger.isDebugEnabled()) {
+                        logger.debug("JDBC SavePoint release not supported by the Connection [{}]", connection, e);
+                    }
+                } else {
+                    throw new TransactionSystemException("Could not release JDBC savepoint", e);
+                }
             }
         } else {
             throw new TransactionSystemException("Missing a JDBC savepoint");
@@ -247,4 +257,21 @@ protected void prepareTransactionalConnection(Connection con, TransactionDefinit
     public Connection getConnection() {
         return connectionOperations.getConnectionStatus().getConnection();
     }
+
+    /**
+     * Checks if thrown exception is from the JDBC driver telling that feature is not supported.
+     * For example, some drivers don't support {@link Connection#releaseSavepoint(Savepoint)}
+     * and we want to handle that case and continue execution.
+     *
+     * @param exception The thrown exception
+     * @return true if exception is thrown for unsupported operation
+     */
+    private static boolean isUnsupportedOperation(Exception exception) {
+        if (exception instanceof SQLFeatureNotSupportedException) {
+            return true;
+        } else if (exception instanceof SQLException sqlException) {
+            return OPERATION_NOT_SUPPORTED.equals(sqlException.getMessage());
+        }
+        return false;
+    }
 }
diff --git a/.../src/main/java/io/micronaut/transaction/support/AbstractDefaultTransactionOperations.java b/.../src/main/java/io/micronaut/transaction/support/AbstractDefaultTransactionOperations.java
@@ -22,6 +22,7 @@
 import io.micronaut.transaction.TransactionDefinition;
 import io.micronaut.transaction.impl.DefaultTransactionStatus;
 
+
 /**
  * Abstract default transaction operations.
  *
@@ -50,4 +51,5 @@ protected DefaultTransactionStatus<C> createExistingTransactionStatus(Connection
     protected DefaultTransactionStatus<C> createNoTxTransactionStatus(ConnectionStatus<C> connectionStatus, TransactionDefinition definition) {
         return DefaultTransactionStatus.noTx(connectionStatus, definition);
     }
+
 }