The patch solves situation when Spring Boot with jOOQ is used and database driver returns type of exception which is not translated by used SQLExceptionTranslator. In this case the last Spring Boot (2.4.3 and also 2.5.0-SNAPSHOT) throws org.jooq.exception.DataAccessException instead of expected org.springframework.dao.DataAccessException. The original SQLException is furthermore lost and getCause() returns null.
The behavior was introduced with change in AbstractFallbackSQLExceptionTranslator with Spring Framework in 5.3.x. The extending translators return no longer UncategorizedSQLException but null. null is correct return value of SQLExceptionTranslator and caller have to create UncategorizedSQLException alone like eg. JdbcTemplate does. This change fixes same in JooqExceptionTranslator for jOOQ.
Some more details are in stack overflow question https://stackoverflow.com/questions/66279277/unexpected-mapping-of-exception-from-trigger-spring-boot-2-4-x-jooq
Comment From: pivotal-issuemaster
@jirkait Please sign the Contributor License Agreement!
Click here to manually synchronize the status of this Pull Request.
See the FAQ for frequently asked questions.
Comment From: pivotal-issuemaster
@jirkait Thank you for signing the Contributor License Agreement!
Comment From: lukaseder
This suggestion seems to revert https://github.com/spring-projects/spring-framework/issues/24064 in the context of jOOQ, see also my comment here: https://github.com/spring-projects/spring-framework/issues/24064#issuecomment-767390408
Is it worth reverting the behaviour only for jOOQ? I think that exception translators have changed incompatibly for good in Spring 5.3, and the jOOQ/Spring integration should be able to handle this. The resulting NPE is already fixed in jOOQ: https://github.com/jOOQ/jOOQ/issues/11304
Comment From: wilkinsona
Thanks for the proposal, @jirkait, and for taking a look, @lukaseder.
I don't think we should be reverting the changes in Spring Framework 5.3 only for jOOQ so I'm going to close this one. We've already upgraded to jOOQ 3.14.7 so we've picked up the NPE fix.
Comment From: cdalexndr
This should be reopened.
Currrently the original exception is lost in logs due to Unspecified SQLException that doesn't wrap original exception, and this requires developers to look for needles in haystacks losing time debugging to reproduce the original exception to find out what actually happened (time and money cost)...
Comment From: wilkinsona
As described above, applying the change proposed here would make jOOQ's exception translation behave differently to all the others. That isn't something that we want to do. If you'd like to pursue this, please do so on https://github.com/spring-projects/spring-framework/issues/24064 or a new Spring Framework issue so that it can be addressed consistently for all translators.
Comment From: cdalexndr
Trying to provide more rationale about this, the way I understand it:
From https://github.com/spring-projects/spring-framework/issues/24064#issuecomment-624619883:
As a part of this change, our default SQLExceptionTranslator implementations do not throw a fallback UncategorizedSQLException anymore but rather leave this up to the caller
The translator respects Spring's contract to return null if exception is not translated, but the caller (consumer) is Spring Boot's JooqExceptionTranslator.
So its valid for the caller to handle the untranslated exception as it sees fit, in this case forwarding UncategorizedSQLException to JOOQ.
As described above, applying the change proposed here would make jOOQ's exception translation behave differently
This PR doesn't change spring's translation contract (return null for untranslated), but only how the consumer handles untranslated exception: https://github.com/jirkait/spring-boot/blob/16e0476a31721d6de880f06f30e4f84afdb6ca2c/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jooq/JooqExceptionTranslator.java#L84-L85
Think hypothesis: what if JOOQ suddenly decides to accept only some CustomJooqException? Then SQLExceptionTranslator will have use a wrapper to provide the required exception to JOOQ. If this is allowed, why is is wrapping the original exception in an UncategorizedSQLException only for JOOQ consumption, not allowed?
The way I see it, JooqExceptionTranslator is free to do what it wants (handle it) after the exception has been translated (or not in this case).
Comment From: cdalexndr
I think I know where the confusion originated from.
Before https://github.com/spring-projects/spring-framework/issues/24064, the translation would return UncategorizedSQLException, but now it returns null.
The OP decided to use a behavior that matches that before 24064 and used UncategorizedSQLException in case translation returned null. This is completely unrelated to 24064.
The JOOQ ExecuteContext accepts any RuntimeException, so this PR can be modified so that the jooq context receives the original exception instead of wrapping into a UncategorizedSQLException (kinda useless).
Without UncategorizedSQLException there will be no more confusion and no relation to 24064.
So I propose the following modification:
if (translated == null) {
context.exception( exception ); //original untranslated excception
} else {
context.exception( translated );
}
Keep focus on the main issue here, that the original exception is lost!
Comment From: wilkinsona
Thanks, @cdalexndr. That sounds reasonable to me. What do you think, @lukaseder?
Comment From: jirkait
The type of original exception is java.sql.SQLException and can not be simply passed as RuntimeException. Therefore wrapping into unchecked UncategorizedSQLException was used.
Comment From: wilkinsona
In the case where translation has resulted in null, I think we can just not call jOOQ at all. It already knows about the original exception and can then process it as it sees fit.
Comment From: cdalexndr
No, that's the current issue, if translation is null, the original exception is lost.
It can be wrapped in a simple RuntimeException and passed to JOOQ:
if (translated == null) {
context.exception( new RuntimeException( exception ) ); //original untranslated excception
} else {
context.exception( translated );
}
Comment From: wilkinsona
Isn't it lost because we're currently passing null into jOOQ? I'm a proposing that we don't call context.exception(RuntimeException) at all when translation produces a null result.
Comment From: cdalexndr
@wilkinsona yeah, passing null into jooq, results in Unspecified SQLException missing the original exception. This is the current implementation.
So when the exception cannot be translated (translation returns null, even tho exception is not null), we should use the original exception without translation.
context.exception( new RuntimeException( exception ) ); <-- exception variable is not null, only translated is null
Translation isn't required to pass exception to JOOQ.
Comment From: jirkait
I tried the proposed change to:
if (translated == null) {
context.exception(new RuntimeException(exception));
} else {
context.exception(translated);
}
The routine calling jOOQ method now throws RuntimeException with original SQLException in cause. This change addresses the issue with the lost original exception. Throwing pure RuntimeException is not nice behavior in my opinion, but it is better than current state.
In the meantime, I looked at the solution directly in jOOQ (like this PR [jOOQ/jOOQ#11574]), but without further investigation of the effect on jOOQ, this is not possible. I am also not sure if the fix in jOOQ is on the right place, as it probably solves the only problem in integration with spring.
Comment From: wilkinsona
IMO, passing a RuntimeException into jOOQ is pretty much the same as passing in UncategorizedSQLException. If SQLExceptionTranslator returns null, then I don't think jOOQ's context.exception(RuntimeException) method should be called at all. AIUI, this will then allow jOOQ itself or a subsequent execute listener to handle the exception as it sees fit.
Comment From: lukaseder
That sounds reasonable to me. What do you think, @lukaseder?
Let's look at the call chain:
- jOOQ runs a query on the JDBC driver
- The JDBC driver throws some
SQLException - jOOQ catches that and passes it to all its
ExecuteListenerinstances, including Spring Boot'sJooqExceptionTranslator, which is also anExecuteListener. It does so via anExecuteContext.exception()"in out" parameter, which listeners can, but don't have to, modify if they choose to do so. If a listener provides anullexception, jOOQ will translate that to an unspecifiedDataAccessException, so it can throw it. @jirkait provided a PR that tries to circumvent this behaviour with a hack: https://github.com/jOOQ/jOOQ/pull/11574, but I don't think that's a sound approach. Passingnullhas a semantic that a caller may want to trigger intentionally. It should not have a no-op semantic. - jOOQ then proceeds to throwing the
ExecuteContext.exception(), which may be the originally wrappedSQLException(wrapped in a jOOQDataAccessException) or the one Spring Boot'sJooqExceptionTranslatorhas translated, or an unspecified exception if the listener provided null.
So, yes I agree, if JooqExceptionTranslator doesn't know what to do, then it shouldn't do anything at all. This will lead to throwing the original SQLException wrapped in a jOOQ DataAccessException, just as if the JooqExceptionTranslator hadn't been configured.
Comment From: wilkinsona
Thanks, @lukaseder. I've opened #25717 to make that change.
Comment From: mnisius
Hello sorry for being a little bit late to the party but as an user of Spring Boot and jOOQ I would like to point out that this behavior feels inconsistent to me.
Now if I have to catch an Exception I never now what Exception to catch. My assumption was that by using the spring-boot-starter-jooq module all jOOQ Exception will be translated in some kind of org.springframework.dao.DataAccessException. This seems to be the case for most errors but now I get a org.jooq.exception.DataAccessException in some edge cases.
My suggestion would be to wrap all jOOQ Exception in some kind of Spring DataAccessException or some kind of configuration to turn exception translation off completely.
Comment From: wilkinsona
My assumption was that by using the spring-boot-starter-jooq module all jOOQ Exception will be translated in some kind of org.springframework.dao.DataAccessException.
Unfortunately, this assumption doesn't hold true following the changes made in Spring Framework 5.3. We prefer to align with those changes and to allow any subsequent execute listeners to perform additional exception translation if they wish, rather than translating to something like UncategorizedSQLException.
This seems to be the case for most errors but now I get a org.jooq.exception.DataAccessException in some edge cases
Getting either an org.springframework.dao.DataAccessException or an org.jooq.exception.DataAccessException is not dissimilar to the behaviour of Spring Framework's JdbcTransactionManager which will throw either an org.springframework.dao.DataAccessException or an org.springframework.transaction.TransactionSystemException.
My suggestion would be to wrap all jOOQ Exception in some kind of Spring DataAccessException or some kind of configuration to turn exception translation off completely
If you want to fine tune this behaviour, you can add additional exception translation of your own by configuring an ExecuteListenerProvider bean. Assigning it an order greater than 0 will cause it to be called after Boot's default exception translation. Similarly, assigning it an order less than 0 will cause it to be called before Boot's default exception translation.
If you want to disable exception translation completely, you can provide your own org.jooq.Configuration bean. If you'd like to see finer-grained control over Boot's exception translation, please open a new issue.
Comment From: mnisius
Ok thank you very much for your detailed answer. This should allow me to customize the beauvoir to my liking.