I read this blogpost describing this problem:

@SneakyThrows
@Transactional
public void lombokSurprise() {
    jdbcTemplate.execute("insert into test_table values('lombok!')");
    throw new Exception("Simple exception");
}

This code is supposed to commit the transaction but it does not. The problem is that Spring wraps checked exceptions that aren't declared in the signature within an UndeclaredThrowableException over here and from that point treats it as unchecked.

I think we should treat this one as a checked exception to preserve the promised behavior. I believe it's controlled here:

https://github.com/spring-projects/spring-framework/blob/01e50fb60ad0d82f96fe4df6054ed6029e2f6a12/spring-tx/src/main/java/org/springframework/transaction/interceptor/DefaultTransactionAttribute.java#L171-L188

So, we could append || ex instanceof UndeclaredThrowableException to solve it.

It will be a breaking change, but I think most people expect it to work that way anyway.

Comment From: sbrannen

So, we could append || ex instanceof UndeclaredThrowableException to solve it.

If you are proposing that be added in the rollbackOn() method in DefaultTransactionAttribute, that would not have any effect since UndeclaredThrowableException extends RuntimeException.

Are you perhaps proposing that the cause of the UndeclaredThrowableException be taken into consideration?

Comment From: sbrannen

@Sam-Kruglov, are you aware that you can configure @Transactional as follows?

@Transactional(noRollbackFor = UndeclaredThrowableException.class)
@SneakyThrows
public void lombokSurprise() {
    jdbcTemplate.execute("insert into test_table values('lombok!')");
    throw new Exception("Simple exception");
}

That should help you achieve your goal.

Comment From: Sam-Kruglov

Editing @Transactional seems like a workaround, I think it’s more intuitive if it’s the default behavior.

The exception thrown is checked if it extends Exception, not if it’s declared in the method signature, so it feels like we’re checking the consequence rather than the source of truth.

I think your suggestion about using cause is better, sorry, I guess I didn’t read enough of the code :)

Comment From: sbrannen

Editing @Transactional seems like a workaround, I think it’s more intuitive if it’s the default behavior.

That's not actually a workaround but rather the supported mechanism for configuring commit/rollback semantics with declarative transaction management in Spring.

The exception thrown is checked if it extends Exception, not if it’s declared in the method signature, so it feels like we’re checking the consequence rather than the source of truth.

Well, we don't know why the UndeclaredThrowableException was thrown (at least not without analyzing the stack trace, which we would not want to do). It could have been thrown from a JDK dynamic proxy not created by Spring or from some other source.

In other words, we would not want to change the current behavior since it would indeed be breaking change.

I think your suggestion about using cause is better, sorry, I guess I didn’t read enough of the code :)

No worries. 😉

In any case, in light of the configuration option outlined in https://github.com/spring-projects/spring-framework/issues/26854#issuecomment-827736447, I am closing this issue.

Comment From: Sam-Kruglov

Wait but we do know where it comes from: https://github.com/spring-projects/spring-framework/blob/01e50fb60ad0d82f96fe4df6054ed6029e2f6a12/spring-aop/src/main/java/org/springframework/aop/framework/CglibAopProxy.java#L767

Also, I’m aware of this configuration, still seems like a workaround to me.

Thanks for your time either way!

Comment From: Sam-Kruglov

Even if that’s something standard in the JDK, I’m fine with wrapping it, still, feels like we should treat it as checked because this wrapper is literally for wrapping checked exceptions if I understood correctly