@Transactional should treat UndeclaredThrowableException as a checked exception

[Sam-Kruglov]

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:

spring-framework/spring-tx/src/main/java/org/springframework/transaction/interceptor/DefaultTransactionAttribute.java

Lines 171 to 188 in 01e50fb

	/**
	* The default behavior is as with EJB: rollback on unchecked exception
	* ({@link RuntimeException}), assuming an unexpected outcome outside of any
	* business rules. Additionally, we also attempt to rollback on {@link Error} which
	* is clearly an unexpected outcome as well. By contrast, a checked exception is
	* considered a business exception and therefore a regular expected outcome of the
	* transactional business method, i.e. a kind of alternative return value which
	* still allows for regular completion of resource operations.
	* <p>This is largely consistent with TransactionTemplate's default behavior,
	* except that TransactionTemplate also rolls back on undeclared checked exceptions
	* (a corner case). For declarative transactions, we expect checked exceptions to be
	* intentionally declared as business exceptions, leading to a commit by default.
	* @see org.springframework.transaction.support.TransactionTemplate#execute
	*/
	@Override
	public boolean rollbackOn(Throwable ex) {
	return (ex instanceof RuntimeException || ex instanceof Error);
	}

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.

[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?

[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.

[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 :)

[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 #26854 (comment), I am closing this issue.

[Sam-Kruglov]

Wait but we do know where it comes from:

spring-framework/spring-aop/src/main/java/org/springframework/aop/framework/CglibAopProxy.java

Line 767 in 01e50fb

throw new UndeclaredThrowableException(ex);

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

Thanks for your time either way!

[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