Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow global default for @Transactional rollback behavior on checked exceptions #23473

Closed
ilyastam opened this issue Aug 16, 2019 · 10 comments
Closed
Assignees
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx) type: enhancement A general enhancement
Milestone

Comments

@ilyastam
Copy link

In Kotlin all exceptions are effectively unchecked. Therefore, transactions will not be rolled back for functions like this:

@Transactional
fun example(): Unit {
  runQuery()
  throw MyCheckedException()
  runQuery()
}

to achieve a correct behavior, the above code needs to be refactored as follows:

@Transactional(rollbackFor = [Exception::class])
fun example(): Unit {
  runQuery()
  throw MyCheckedException()
  runQuery()
}

this isn't very intuitive and can lead to unexpected results. Furthermore, even if a developer is aware of this, he/she should not forget to specify rollbackFor for every @Transactional annotation.

It should be possible to configure application-wide defaults for rollbackFor and noRollbackFor

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Aug 16, 2019
@the-fine
Copy link

@ilyastam as stated here: https://docs.spring.io/spring/docs/5.2.0.RC1/spring-framework-reference/data-access.html#transaction-declarative-rolling-back
Transactions are rolled back by unchecked exceptions by default.
So it should work out of the box with Kotlin because exceptions are unchecked https://kotlinlang.org/docs/reference/exceptions.html
Do you have a sample projects that reproduces this issue?

@kilink
Copy link
Contributor

kilink commented Aug 18, 2019

Kotlin doesn't distinguish between RuntimeException (unchecked) and Exception (checked), meaning Kotlin code can freely throw either type. Spring transactional support does distinguish between the two by not rolling back any checked exception by default (an exception subclassing Exception).

One can choose to only use RuntimeException derived exceptions in Kotlin to avoid that issue, but it can also arise when Kotlin code calls into a Java method that throws a checked Exception. Essentially it's a "foot gun", one mistake can lead to the unexpected behavior of a transaction not being rolled back.

@raderio
Copy link

raderio commented Oct 13, 2019

The issue is that in a transactional method we can call a method which can throw IOException, and in this case the transaction will not rollback already mutated data because IOException is not a subclass of RuntimeException

Why not to rollback by default by Exception and not by RuntimeException without any configs?
This will not break the Java code.

@elab
Copy link

elab commented Jan 6, 2020

How about specifying rollback behavior in a @Transactional annotation at the class level?

@snicoll
Copy link
Member

snicoll commented Jan 6, 2020

@elab that would mark all public methods in the class to be transactional.

@Fryie
Copy link

Fryie commented Mar 23, 2021

It would be really beneficial to have this as a configurable option (maybe even set by default when choosing Kotlin on start.spring.io). Since Kotlin doesn't force you to handle checked exceptions, it's easy to accidentally end up with a corrupted state because the transaction is not rolled back.

@kilink
Copy link
Contributor

kilink commented Mar 29, 2021

Here is what we did to work around the issue in our Kotlin Spring Boot app:

import org.springframework.beans.factory.config.BeanDefinition
import org.springframework.context.annotation.Bean
import org.springframework.context.annotation.Configuration
import org.springframework.context.annotation.Role
import org.springframework.core.KotlinDetector.isKotlinType
import org.springframework.transaction.annotation.AnnotationTransactionAttributeSource
import org.springframework.transaction.annotation.ProxyTransactionManagementConfiguration
import org.springframework.transaction.interceptor.DelegatingTransactionAttribute
import org.springframework.transaction.interceptor.TransactionAttribute
import org.springframework.transaction.interceptor.TransactionAttributeSource
import java.lang.reflect.AnnotatedElement
import java.lang.reflect.Method

@Configuration
class TransactionConfig : ProxyTransactionManagementConfiguration() {
    /**
     * Define a custom [TransactionAttributeSource] that will roll back transactions
     * on checked Exceptions if the annotated method or class is written in Kotlin.
     *
     * Kotlin doesn't have a notion of checked exceptions, but [Transactional] assumes
     * Java semantics and does *not* roll back on checked exceptions. This can become
     * an issue if a Kotlin class explicitly throws Exception or calls into a Java
     * method which throws checked exceptions.
     *
     * @see: https://github.com/spring-projects/spring-framework/issues/23473
     */
    @Bean
    @Role(BeanDefinition.ROLE_INFRASTRUCTURE)
    override fun transactionAttributeSource(): TransactionAttributeSource {
        return object : AnnotationTransactionAttributeSource() {
            override fun determineTransactionAttribute(element: AnnotatedElement): TransactionAttribute? {
                val txAttr = super.determineTransactionAttribute(element)
                    ?: return null
                val isKotlinClass = when (element) {
                    is Class<*> -> isKotlinType(element)
                    is Method -> isKotlinType(element.declaringClass)
                    else -> false
                }
                if (isKotlinClass) {
                    return object : DelegatingTransactionAttribute(txAttr) {
                        override fun rollbackOn(ex: Throwable): Boolean {
                            return super.rollbackOn(ex) || ex is Exception
                        }
                    }
                }
                return txAttr
            }
        }
    }
}

Essentially extremely cautious, and only rolls back for Exception by default if the annotated class is written in Kotlin. I feel this fix could be rolled into spring-tx, as it seems pretty surgical / low risk (only change behavior for Kotlin classes).

@rstoyanchev rstoyanchev added the in: data Issues in data modules (jdbc, orm, oxm, tx) label Nov 10, 2021
@snicoll snicoll added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Sep 25, 2023
@snicoll snicoll added this to the 6.2.x milestone Sep 25, 2023
@snicoll snicoll self-assigned this Sep 25, 2023
@jhoeller jhoeller changed the title allow global settings for rollbackFor and noRollbackFor fo @Transactional Allow global settings for rollbackFor and noRollbackFor on @Transactional Sep 27, 2023
@snicoll
Copy link
Member

snicoll commented Nov 7, 2023

The config infrastructure could be shared by work on #24291

@quaff
Copy link
Contributor

quaff commented Nov 24, 2023

return object : AnnotationTransactionAttributeSource() {

It should be return object : AnnotationTransactionAttributeSource(false) to align with spring 6.0, other people may copy your code, could you edit it? @kilink

@jhoeller
Copy link
Contributor

jhoeller commented Mar 5, 2024

I'm introducing a rollbackOn attribute on @EnableTransactionManagement, with an enum of RUNTIME_EXCEPTIONS and ALL_EXCEPTIONS. This makes it easier to document the behavior and the recommendations, and also easier to consistently support it for Spring's @Transactional as well as JTA's jakarta.transaction.Transactional when used with Spring, while not exposing the full complexity of rollback rules and no-rollback rules on @EnableTransactionManagement itself.

For any further customization needs at the global level, AnnotationTransactionAttributeSource provides an addDefaultRollbackRule(RollbackRuleAttribute) method for arbitrary default rollback rules.

@jhoeller jhoeller changed the title Allow global settings for rollbackFor and noRollbackFor on @Transactional Allow global default for @Transactional rollback behavior on checked exceptions Mar 5, 2024
@jhoeller jhoeller modified the milestones: 6.2.x, 6.2.0-M1 Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests