Skip to content

Support Result return types for @Transactional Kotlin functions #27323

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

Closed
Nimelrian opened this issue Aug 25, 2021 · 8 comments
Closed

Support Result return types for @Transactional Kotlin functions #27323

Nimelrian opened this issue Aug 25, 2021 · 8 comments
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx) status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement

Comments

@Nimelrian
Copy link

Nimelrian commented Aug 25, 2021

Affects: 5.3.9 (and earlier versions)

Motivation: Error Handling is a controversial topic. Java primarily uses exceptions to show that something went wrong. For database interactions, Spring provides the @Transactional Annotation so the configured transaction manager automatically rolls back any pending changes whenever an exception is thrown. It also supports reactive execution flows (Mono/Flow) and Kotlin coroutines.

Now the following is the controversial part: Do you show the possibility of something going wrong in the declaration of the function, or do you hide it and only refer to possible failure modes in the documentation? Java gives you a choice:

  • Use checked exceptions which also show up in the function declaration via the throws declaration. Callers of the function are forced to either handle the exception or to pass it up to their caller (and retain the throws declaration in case the exception is not wrapped in an unchecked exception
  • Use unchecked exceptions, which do not show up in the function declaration. Developers have to remember that somewhere up the stack the exception has to be handled in a graceful way if they want to keep their application running.

From my personal experience, checked exceptions were looked upon as verbose and annoying in the 2000-2015 timeframe, but have seen a resurgence in recent years to make identifying potentially failing functions easier. This is especially true in other communities with functional programming influences, e.g. Typescript, Haskell, Scala or Rust. Since these languages don't have checked exceptions, they use the return type of a function to signify potentially failing operations, often using something akin to the Either Monad (Also known as Result in Rust). As an example: In Rust a Result<MyEntity, DomainSpecificError> shows that the function either completes without an error, returning a MyEntity value, or yields a DomainSpecificError, which can then be handled (or passed further up the stack).

Kotlin does not feature checked exceptions. It does however include a Result<Value> type in its standard library. Sadly, it does not allow specifying an explicit type for the error case (It just requires the error value to be a Throwable). There is also another library which implements a Result type for Kotlin, which does allow the user to specify the error type (https://github.com/michaelbull/kotlin-result/), which is what we use in our projects.

In recent years, Spring has added many Kotlin-specific features / enhancements. I'd love to be able to mark my Kotlin functions which return a Result (be it the stdlib version or the version from Michael Bull) with @Transactional and have pending DB changes rolled back in case the function returns the error variant of the respective Result type. There is however no consensus in the Kotlin community regarding error handling, at least as far as I know. Some people use regular unchecked exceptions, some people use the stdlib Result, some people use the Result from Michael Bull.

To sum it all up, here are my final questions:

  • What is your opinion on introducing a compatibility layer between @Transactional and a Result type in general, in case a consensus is found in the Kotlin community?
  • Could someone point me in the right direction so I could try to implement this myself as a proof of concept, e.g. as an aspect which is applied to Bean functions marked as @Transactional which return a Result? The documentation on Spring AOP features I have found online isn't the most helpful for someone who hasn't dipped his toes into AOP territory yet.
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Aug 25, 2021
@snicoll snicoll changed the title Feature Request / Request for Comment: Support Result return types for @Transactional functions Support Result return types for @Transactional functions Aug 25, 2021
@sbrannen sbrannen added in: data Issues in data modules (jdbc, orm, oxm, tx) type: enhancement A general enhancement labels Aug 25, 2021
@sbrannen
Copy link
Member

This is closely related to:

Could someone point me in the right direction so I could try to implement this myself as a proof of concept, e.g. as an aspect which is applied to Bean functions marked as @Transactional which return a Result?

See Spring's support for Vavr's io.vavr.control.Try in org.springframework.transaction.interceptor.TransactionAspectSupport.invokeWithinTransaction(..).

@sbrannen sbrannen changed the title Support Result return types for @Transactional functions Support Result return types for @Transactional Kotlin functions Aug 25, 2021
@Nimelrian
Copy link
Author

Nimelrian commented Aug 26, 2021

Thanks for the quick reply. Vavr's Try is definitely related to this issue, I took a look into the PR and it seems like not too many additions would be needed. Since there is no real consensus in the Kotlin community regarding error handling I'd refrain from creating a PR for the variant we use in our team until there is more clarity about the preferred way to handle things in the general community (Unless the Spring team would be fine with adding support for a library with almost 500 stars on Github)

Since we are running on an older version of the framework (Spring Boot 2.1.6) and are stuck with it for the forseeable future, we wouldn't be able to take advantage of the feature landing in an upcoming framework version. What would be the preferred way to extend the framework from our side if we wanted to add transaction support for Result returning functions? Does Spring provide an easy extension point for this we could take advantage of? (Though this may be better suited for a Stackoverflow question).

Edit: Got it working with this simple aspect:

@Aspect
@Order(-1)
@Component
@ConditionalOnClass(Result::class)
class TransactionalResultAspect(private val transactionManager: PlatformTransactionManager) {
    @Pointcut("execution(@org.springframework.transaction.annotation.Transactional com.github.michaelbull.result.Result *(..))")
    fun transactionalMethodReturningResult() = Unit

    @Around("transactionalMethodReturningResult() && @annotation(transactionalAnnotation)")
    fun runInTransactionAndRollbackOnErr(joinPoint: ProceedingJoinPoint, transactionalAnnotation: Transactional): Any? {
        val template = TransactionTemplate(transactionManager).apply {
            isolationLevel = transactionalAnnotation.isolation.value()
            isReadOnly = transactionalAnnotation.readOnly
            propagationBehavior = transactionalAnnotation.propagation.value()
            timeout = transactionalAnnotation.timeout
        }

        lateinit var result: Result<*, *>
        try {
            template.execute {
                result = joinPoint.proceed() as Result<*, *>
                result.onFailure { throw ResultErrException }
            }
        } catch (ex: ResultErrException) {
            /* This is not unexpected, the error leading to the rollback is stored
             * in the result which we are about to return
             **/
        }
        return result
    }

    private object ResultErrException : RuntimeException("A transactional operation returned an Err value.")
}

@sbrannen
Copy link
Member

@Nimelrian, I'm glad you found a working solution with that custom aspect. Please note, however, that that only works for methods directly annotated with @Transactional: it would not take into account @Transactional declared at the class level or on an interface.

@sdeleuze, do you have any insight on which Result type is more commonly used in Kotlin applications or if there are popular alternatives?

@Nimelrian
Copy link
Author

@sbrannen Thanks for the edge case, I'll make sure to update our workaround to cover it.

@sdeleuze
Copy link
Contributor

Thanks for the detailed issue, after a deeper look, I think Result is not popular enough to integrate such support in Spring Framework.

@sdeleuze sdeleuze added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Dec 17, 2021
@timmeyerLeanIX
Copy link

I stumbled upon this old issue trying to research how to handle transaction management when working with Kotlin + Springboot. Given that Result is now a standard part of Kotlin and is widely used for error handling, Im wondering if its worth evaluating this again? Especially in regards to @sdeleuze point of lack of popularity.

@sdeleuze
Copy link
Contributor

@timmeyerLeanIX Can you please provide more background on the evolution of the usage of Result in the Kotlin ecosystem, especially the use cases where it is commonly used?

@mblok-bol-user
Copy link

I do not have any meaningful data to back up the usage of Result, but it is recommended for example by KT academy: https://kt.academy/article/ek-nullable-result.
Another worry about Vavr is that the project was abandoned for years, and so a potential reliability.

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) status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

6 participants