Wrong jOOQ exception translator with empty db name

[MelleD]

If there is no db product name in jOOQ available the sql error state translator is used by default:

See:
https://github.com/spring-projects/spring-boot/blob/main/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jooq/DefaultExceptionTranslatorExecuteListener.java#L116

	return (dbName != null) ? new SQLErrorCodeSQLExceptionTranslator(dbName)
					: new SQLStateSQLExceptionTranslator();

During discussion of this ticket, we figured out the translator chain is not correct.
Matching the core Spring fallback chain, it should using the SQLErrorCodeSQLExceptionTranslator for empty dbName.

Also the javadoc from SQLErrorCodeSQLExceptionTranslator gives a hint not to use this a main translator:
SQLErrorCodeSQLExceptionTranslator
"This translator is commonly used as a fallback behind a primary translator such as SQLErrorCodeSQLExceptionTranslator or SQLExceptionSubclassTranslator.
"

So IMHO this line should changed to:

		return (dbName != null) ? new SQLErrorCodeSQLExceptionTranslator(dbName)
					: new SQLErrorCodeSQLExceptionTranslator();

[MelleD]

If desired, I can do a PR from my branch:
https://github.com/MelleD/spring-boot/tree/44869-fix-default-exception-translator

[wilkinsona]

It's been this way since the beginning:

spring-boot/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jooq/JooqExceptionTranslator.java

Lines 57 to 63 in a08b095

	private SQLExceptionTranslator getTranslator(ExecuteContext context) {
	SQLDialect dialect = context.configuration().dialect();
	if (dialect != null) {
	return new SQLErrorCodeSQLExceptionTranslator(dialect.name());
	}
	return new SQLStateSQLExceptionTranslator();
	}

If we're going to change this, we'll have to consider when to do it. I wonder if there may be apps relying on the current behavior?

[MelleD]

It's been this way since the beginning:

I meant with spring core how the other modules like spring data or jdbc is using the chain.

SQLErrorCodeSQLExceptionTranslator --> fallback to --> SQLExceptionSubclassTranslator --> fall back to --> SQLStateSQLExceptionTranslator

If we're going to change this, we'll have to consider when to do it. I wonder if there may be apps relying on the current behavior?

Do you think it's really breaking (unit test was not breaking), because in the the of the chain the last translator is the SQLStateSQLExceptionTranslator

[jhoeller]

@wilkinsona if you'd like to be defensive in terms of avoiding runtime auto-detection of the database etc, it might be better to fall back to SQLExceptionSubclassTranslator directly there. In any case, I definitely recommend that over SQLStateSQLExceptionTranslator. For Spring core setups, we replaced all fallback to SQLStateSQLExceptionTranslator with SQLExceptionSubclassTranslator a few years ago already. Note that SQLExceptionSubclassTranslator in turns falls back to SQLStateSQLExceptionTranslator, so it only adds JDBC-level exception rules but is still capable of introspecting SQL state as well.

[MelleD]

Here is a proposal for the change with a test case:
#44954

[snicoll]

Closing in favor of PR #44954. It doesn't expose the right fallback but the additional tests expose the case where a dbName is not available.

Looking at the framework's code base, the fallback on SQLExceptionSubclassTranslator should have been done as moving to Framework 6 anyway so I'll use the opportunity of this issue to do that.