DatabaseDriver swallows real exception

[eduanb]

With a Spring Data JDBC application and this config (everything is correct except for the password):

spring:
  datasource:
    url: jdbc:postgresql://localhost:5432/my-db
    password: wrong-password
    username: real-user

The app fails to startup with this error message:
Caused by: java.lang.IllegalStateException: Unable to detect database type
This error makes the user think they must add spring.datasource.type or even spring.datasource.driver-class-name properties, but neither will solve it.

After further investigation, the root cause in DatabaseDriver:324 is swallowed and not reported to the user. Adding a breakpoint, I can see that the root cause is in fact:

org.postgresql.util.PSQLException: FATAL: password authentication failed for user "real-user"

It seems like the swallow was intentional so the type can be DatabaseDriver.UNKNOWN. It is very misleading to the user so I would suggest that propagating the error and having startup fail because of the PSQLException is much better than the IllegalStateException.

[nkjackzhang]

The only place where "Unable to detect database type" appears is

spring-boot/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/jdbc/init/PlatformPlaceholderDatabaseDriverResolver.java

Line 132 in c0f59a1

Assert.state(databaseDriver != DatabaseDriver.UNKNOWN, "Unable to detect database type");

, it uses database meta data also real datasource connection to detemine database driver. The problem is why it used a invalid DataSource instense here.

[wilkinsona]

Thanks for the report. Unfortunately, I don't think we can change DatabaseDriver.fromDataSource(DataSource) to throw an exception as that would be a breaking change to public API.

I can think of two alternatives that I'd like to discuss with the team:

One

We could introduce a new method on DatabaseDriver that takes both a DataSource and some sort of exception handler:

/**
 * Find a {@link DatabaseDriver} for the given {@code DataSource}.
 * @param dataSource data source to inspect
 * @param failureMapping failure to DatabaseDriver mapping to apply when a failure
 * occurs
 * @return the database driver, {@link #UNKNOWN} if not found, or the result of
 * applying the failure mapping
 * @since 2.7.10
 */
public static DatabaseDriver fromDataSource(DataSource dataSource,
		Function<Exception, DatabaseDriver> failureMapping) {
	try {
		String productName = JdbcUtils.commonDatabaseName(
				JdbcUtils.extractDatabaseMetaData(dataSource, DatabaseMetaData::getDatabaseProductName));
		return DatabaseDriver.fromProductName(productName);
	}
	catch (Exception ex) {
		return failureMapping.apply(ex);
	}
}

Calling code can then return UNKNOWN or throw an unchecked exception from its failureMapping implementation.

Two

Alternatively, we could update PlatformPlaceholderDatabaseDriverResolver to stop using fromDataSource in favor of its own implementation that rethrows the underlying exception:

private DatabaseDriver getDatabaseDriver(DataSource dataSource) {
	try {
		String productName = JdbcUtils.commonDatabaseName(
				JdbcUtils.extractDatabaseMetaData(dataSource, DatabaseMetaData::getDatabaseProductName));
		return DatabaseDriver.fromProductName(productName);
	}
	catch (Exception ex) {
		throw new IllegalStateException("Failed to determine DatabaseDriver", ex);
	}
}

This avoids adding new public API but it does leave DatabaseDriver.fromDataSource(DataSource) unused within Boot so we may want to deprecate it for removal.

[snicoll]

This avoids adding new public API but it does leave DatabaseDriver.fromDataSource(DataSource) unused within Boot so we may want to deprecate it for removal.

If that's the only place we use this, I'd be in favor of 2.

[philwebb]

I'm not sure I like 1 all that much because I don't think there's an alternative return value that makes much sense. I probably favor 2, but the duplicate is a little annoying.

Here's a possible third option: philwebb@1dbe863

[wilkinsona]

Thanks, both.

The 3rd option will leave us with DatabaseDriver.fromDataSource(DataSource) being unused in Boot so I'd be tempted to deprecate it for removal. If we're doing that, I wonder if option 2 might be better as it'll (eventually) leave us without any duplication and also means we don't introduce any new public API that's only used in one place.

[somayaj]

@wilkinsona Is it ok to make the change here option #2 as agreed ? with a pr contribution? Is this still open?

[wilkinsona]

Thanks for the offer, @somayaj, but I don't think we've reached an agreement yet.

[philwebb]

If we're doing that, I wonder if option 2 might be better as it'll (eventually) leave us without any duplication and also means we don't introduce any new public API that's only used in one place.

+1 for option 2 then