The untranslated SQLException is wrapped to UncategorizedSQLException

[jirkait]

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

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

[pivotal-issuemaster]

@jirkait Thank you for signing the Contributor License Agreement!

[lukaseder]

This suggestion seems to revert spring-projects/spring-framework#24064 in the context of jOOQ, see also my comment here: spring-projects/spring-framework#24064 (comment)

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: jOOQ/jOOQ#11304

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

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

[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 spring-projects/spring-framework#24064 or a new Spring Framework issue so that it can be addressed consistently for all translators.

[cdalexndr]

Trying to provide more rationale about this, the way I understand it:

From spring-projects/spring-framework#24064 (comment):

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

[cdalexndr]

I think I know where the confusion originated from.

Before spring-projects/spring-framework#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!

[wilkinsona]

Thanks, @cdalexndr. That sounds reasonable to me. What do you think, @lukaseder?

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

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

[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 );
        }

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

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

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

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

[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 ExecuteListener instances, including Spring Boot's JooqExceptionTranslator, which is also an ExecuteListener. It does so via an ExecuteContext.exception() "in out" parameter, which listeners can, but don't have to, modify if they choose to do so. If a listener provides a null exception, jOOQ will translate that to an unspecified DataAccessException, so it can throw it. @jirkait provided a PR that tries to circumvent this behaviour with a hack: ExecuteContext.exception() translates to DataAccessException with cause in case of null jOOQ/jOOQ#11574, but I don't think that's a sound approach. Passing null has 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 wrapped SQLException (wrapped in a jOOQ DataAccessException) or the one Spring Boot's JooqExceptionTranslator has 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.

[wilkinsona]

Thanks, @lukaseder. I've opened #25717 to make that change.

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

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

[mnisius]

Ok thank you very much for your detailed answer. This should allow me to customize the beauvoir to my liking.