Polish "Fix detection logic for embedded databases"

See gh-23693

Author: snicoll

spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jdbc/DataSourceProperties.java

@@ -322,13 +322,13 @@ public void setUsername(String username) {
 	 * @since 1.4.0
 	 */
 	public String determineUsername() {
-		if (EmbeddedDatabaseConnection.isEmbedded(determineDriverClassName(), determineUrl())
-				&& !StringUtils.hasText(this.username)) {
-			return "sa";
-		}
-		else {
+		if (StringUtils.hasText(this.username)) {
 			return this.username;
 		}
+		if (EmbeddedDatabaseConnection.isEmbedded(determineDriverClassName(), determineUrl())) {
+			return "sa";
+		}
+		return null;
 	}
 
 	/**
@@ -350,13 +350,13 @@ public void setPassword(String password) {
 	 * @since 1.4.0
 	 */
 	public String determinePassword() {
-		if (EmbeddedDatabaseConnection.isEmbedded(determineDriverClassName(), determineUrl())
-				&& !StringUtils.hasText(this.password)) {
-			return "";
-		}
-		else {
+		if (StringUtils.hasText(this.password)) {
 			return this.password;
 		}
+		if (EmbeddedDatabaseConnection.isEmbedded(determineDriverClassName(), determineUrl())) {
+			return "";
+		}
+		return null;
 	}
 
 	public String getJndiName() {

spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/flyway/FlywayAutoConfigurationTests.java

@@ -127,6 +127,7 @@ void createDataSourceFallbackToEmbeddedProperties() {
 					assertThat(context).hasSingleBean(Flyway.class);
 					DataSource dataSource = context.getBean(Flyway.class).getConfiguration().getDataSource();
 					assertThat(dataSource).isNotNull();
+					assertThat(dataSource).hasFieldOrPropertyWithValue("user", "sa");
 					assertThat(dataSource).hasFieldOrPropertyWithValue("password", "");
 				});
 	}

spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/jdbc/DataSourcePropertiesTests.java

@@ -78,32 +78,6 @@ void determineUrlWithExplicitConfig() throws Exception {
 		assertThat(properties.determineUrl()).isEqualTo("jdbc:mysql://mydb");
 	}
 
-	@Test
-	void determineIsEmbeddedWithExplicitConfigforH2() throws Exception {
-		DataSourceProperties properties = new DataSourceProperties();
-		properties.setUrl("jdbc:h2:~/test");
-		properties.setUsername("");
-		properties.setPassword("");
-		properties.afterPropertiesSet();
-		assertThat(properties.getUrl()).isEqualTo("jdbc:h2:~/test");
-		assertThat(properties.determineUrl()).isEqualTo("jdbc:h2:~/test");
-		assertThat(properties.determineUsername()).isEqualTo("");
-		assertThat(properties.determinePassword()).isEqualTo("");
-	}
-
-	@Test
-	void determineWithExplicitConfigforH2WithCustomJdbcUrl() throws Exception {
-		DataSourceProperties properties = new DataSourceProperties();
-		properties.setUrl("jdbc:h2:~/test");
-		properties.setUsername("as");
-		properties.setPassword("as");
-		properties.afterPropertiesSet();
-		assertThat(properties.getUrl()).isEqualTo("jdbc:h2:~/test");
-		assertThat(properties.determineUrl()).isEqualTo("jdbc:h2:~/test");
-		assertThat(properties.determineUsername()).isEqualTo("as");
-		assertThat(properties.determinePassword()).isEqualTo("as");
-	}
-
 	@Test
 	void determineUrlWithGenerateUniqueName() throws Exception {
 		DataSourceProperties properties = new DataSourceProperties();
@@ -151,11 +125,21 @@ void determineUsernameWithExplicitConfig() throws Exception {
 		assertThat(properties.determineUsername()).isEqualTo("foo");
 	}
 
+	@Test
+	void determineUsernameWithNonEmbeddedUrl() throws Exception {
+		DataSourceProperties properties = new DataSourceProperties();
+		properties.setUrl("jdbc:h2:~/test");
+		properties.afterPropertiesSet();
+		assertThat(properties.getPassword()).isNull();
+		assertThat(properties.determineUsername()).isNull();
+	}
+
 	@Test
 	void determinePassword() throws Exception {
 		DataSourceProperties properties = new DataSourceProperties();
 		properties.afterPropertiesSet();
 		assertThat(properties.getPassword()).isNull();
+		assertThat(properties.determinePassword()).isEqualTo("");
 	}
 
 	@Test
@@ -167,6 +151,15 @@ void determinePasswordWithExplicitConfig() throws Exception {
 		assertThat(properties.determinePassword()).isEqualTo("bar");
 	}
 
+	@Test
+	void determinePasswordWithNonEmbeddedUrl() throws Exception {
+		DataSourceProperties properties = new DataSourceProperties();
+		properties.setUrl("jdbc:h2:~/test");
+		properties.afterPropertiesSet();
+		assertThat(properties.getPassword()).isNull();
+		assertThat(properties.determinePassword()).isNull();
+	}
+
 	@Test
 	void determineCredentialsForSchemaScripts() {
 		DataSourceProperties properties = new DataSourceProperties();

spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/liquibase/LiquibaseAutoConfigurationTests.java

@@ -246,6 +246,7 @@ void overrideDataSourceAndFallbackToEmbeddedProperties() {
 				.run(assertLiquibase((liquibase) -> {
 					DataSource dataSource = liquibase.getDataSource();
 					assertThat(((HikariDataSource) dataSource).isClosed()).isTrue();
+					assertThat(((HikariDataSource) dataSource).getUsername()).isEqualTo("sa");
 					assertThat(((HikariDataSource) dataSource).getPassword()).isEqualTo("");
 				}));
 	}

spring-boot-project/spring-boot-docs/src/docs/asciidoc/howto.adoc

@@ -1982,8 +1982,8 @@ This is controlled through two external properties:
 You can set `spring.jpa.hibernate.ddl-auto` explicitly and the standard Hibernate property values are `none`, `validate`, `update`, `create`, and `create-drop`.
 Spring Boot chooses a default value for you based on whether it thinks your database is embedded.
 It defaults to `create-drop` if no schema manager has been detected or `none` in all other cases.
-An embedded database is detected by looking at the `Connection` type.
-`hsqldb`, `h2`, and `derby` are embedded, and others are not.
+An embedded database is detected by looking at the `Connection` type and JDBC url.
+`hsqldb`, `h2`, and `derby` are candidates, and others are not.
 Be careful when switching from in-memory to a '`real`' database that you do not make assumptions about the existence of the tables and data in the new platform.
 You either have to set `ddl-auto` explicitly or use one of the other mechanisms to initialize the database.
 

spring-boot-project/spring-boot-test-autoconfigure/src/main/java/org/springframework/boot/test/autoconfigure/jdbc/TestDatabaseAutoConfiguration.java

@@ -101,7 +101,7 @@ private void process(BeanDefinitionRegistry registry, ConfigurableListableBeanFa
 
 		private BeanDefinition createEmbeddedBeanDefinition(boolean primary) {
 			BeanDefinition beanDefinition = new RootBeanDefinition(EmbeddedDataSourceFactoryBean.class);
-			beanDefinition.setPrimary(true);
+			beanDefinition.setPrimary(primary);
 			return beanDefinition;
 		}
 

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

@@ -17,8 +17,11 @@
 package org.springframework.boot.jdbc;
 
 import java.sql.Connection;
+import java.sql.DatabaseMetaData;
 import java.sql.SQLException;
 import java.util.Locale;
+import java.util.function.Predicate;
+import java.util.stream.Stream;
 
 import javax.sql.DataSource;
 
@@ -44,32 +47,33 @@ public enum EmbeddedDatabaseConnection {
 	/**
 	 * No Connection.
 	 */
-	NONE(null, null, null),
+	NONE(null, null, null, (url) -> false),
 
 	/**
 	 * H2 Database Connection.
 	 */
 	H2(EmbeddedDatabaseType.H2, DatabaseDriver.H2.getDriverClassName(),
-			"jdbc:h2:mem:%s;DB_CLOSE_DELAY=-1;DB_CLOSE_ON_EXIT=FALSE"),
+			"jdbc:h2:mem:%s;DB_CLOSE_DELAY=-1;DB_CLOSE_ON_EXIT=FALSE", (url) -> url.contains(":h2:mem")),
 
 	/**
 	 * Derby Database Connection.
 	 */
-	DERBY(EmbeddedDatabaseType.DERBY, DatabaseDriver.DERBY.getDriverClassName(), "jdbc:derby:memory:%s;create=true"),
+	DERBY(EmbeddedDatabaseType.DERBY, DatabaseDriver.DERBY.getDriverClassName(), "jdbc:derby:memory:%s;create=true",
+			(url) -> true),
 
 	/**
 	 * HSQL Database Connection.
 	 * @deprecated since 2.4.0 in favor of {@link EmbeddedDatabaseConnection#HSQLDB}.
 	 */
 	@Deprecated
 	HSQL(EmbeddedDatabaseType.HSQL, DatabaseDriver.HSQLDB.getDriverClassName(), "org.hsqldb.jdbcDriver",
-			"jdbc:hsqldb:mem:%s"),
+			"jdbc:hsqldb:mem:%s", (url) -> url.contains(":hsqldb:mem:")),
 
 	/**
 	 * HSQL Database Connection.
 	 */
 	HSQLDB(EmbeddedDatabaseType.HSQL, DatabaseDriver.HSQLDB.getDriverClassName(), "org.hsqldb.jdbcDriver",
-			"jdbc:hsqldb:mem:%s");
+			"jdbc:hsqldb:mem:%s", (url) -> url.contains(":hsqldb:mem:"));
 
 	private final EmbeddedDatabaseType type;
 
@@ -79,15 +83,20 @@ public enum EmbeddedDatabaseConnection {
 
 	private final String url;
 
-	EmbeddedDatabaseConnection(EmbeddedDatabaseType type, String driverClass, String url) {
-		this(type, driverClass, null, url);
+	private final Predicate<String> embeddedUrl;
+
+	EmbeddedDatabaseConnection(EmbeddedDatabaseType type, String driverClass, String url,
+			Predicate<String> embeddedUrl) {
+		this(type, driverClass, null, url, embeddedUrl);
 	}
 
-	EmbeddedDatabaseConnection(EmbeddedDatabaseType type, String driverClass, String fallbackDriverClass, String url) {
+	EmbeddedDatabaseConnection(EmbeddedDatabaseType type, String driverClass, String fallbackDriverClass, String url,
+			Predicate<String> embeddedUrl) {
 		this.type = type;
 		this.driverClass = driverClass;
 		this.alternativeDriverClass = fallbackDriverClass;
 		this.url = url;
+		this.embeddedUrl = embeddedUrl;
 	}
 
 	/**
@@ -116,34 +125,41 @@ public String getUrl(String databaseName) {
 		return (this.url != null) ? String.format(this.url, databaseName) : null;
 	}
 
+	boolean isEmbeddedUrl(String url) {
+		return this.embeddedUrl.test(url);
+	}
+
+	boolean isDriverCompatible(String driverClass) {
+		return (driverClass != null
+				&& (driverClass.equals(this.driverClass) || driverClass.equals(this.alternativeDriverClass)));
+	}
+
 	/**
 	 * Convenience method to determine if a given driver class name represents an embedded
 	 * database type.
 	 * @param driverClass the driver class
 	 * @return true if the driver class is one of the embedded types
+	 * @deprecated since 2.4.0 in favor of {@link #isEmbedded(String, String)}
 	 */
 	@Deprecated
 	public static boolean isEmbedded(String driverClass) {
-		return driverClass != null && (matches(HSQL, driverClass) || matches(H2, driverClass)
-				|| matches(DERBY, driverClass) || matches(HSQLDB, driverClass));
+		return isEmbedded(driverClass, null);
 	}
 
 	/**
-	 * Convenience method to determine if a given driver class name and url represents an
-	 * embedded database type.The exception is made for the H2 database for embedded
-	 * types.
+	 * Convenience method to determine if a given driver class name and url represent an
+	 * embedded database type.
 	 * @param driverClass the driver class
-	 * @param url the jdbc url
-	 * @return true if the driver class is one of the embedded types
+	 * @param url the jdbc url (can be {@code null)}
+	 * @return true if the driver class and url refer to an embedded database
 	 */
 	public static boolean isEmbedded(String driverClass, String url) {
-		return (driverClass != null
-				&& (matches(HSQL, driverClass) || (matches(H2, driverClass) && url.contains(":h2:mem"))
-						|| matches(DERBY, driverClass) || matches(HSQLDB, driverClass)));
+		return driverClass != null && getEmbeddedDatabaseConnection(driverClass).isEmbeddedUrl(url);
 	}
 
-	private static boolean matches(EmbeddedDatabaseConnection candidate, String driverClass) {
-		return driverClass.equals(candidate.driverClass) || driverClass.equals(candidate.alternativeDriverClass);
+	private static EmbeddedDatabaseConnection getEmbeddedDatabaseConnection(String driverClass) {
+		return Stream.of(H2, HSQLDB, DERBY).filter((connection) -> connection.isDriverCompatible(driverClass))
+				.findFirst().orElse(NONE);
 	}
 
 	/**
@@ -184,15 +200,16 @@ private static class IsEmbedded implements ConnectionCallback<Boolean> {
 
 		@Override
 		public Boolean doInConnection(Connection connection) throws SQLException, DataAccessException {
-			String productName = connection.getMetaData().getDatabaseProductName();
+			DatabaseMetaData metaData = connection.getMetaData();
+			String productName = metaData.getDatabaseProductName();
 			if (productName == null) {
 				return false;
 			}
 			productName = productName.toUpperCase(Locale.ENGLISH);
 			EmbeddedDatabaseConnection[] candidates = EmbeddedDatabaseConnection.values();
 			for (EmbeddedDatabaseConnection candidate : candidates) {
 				if (candidate != NONE && productName.contains(candidate.name())) {
-					return true;
+					return candidate.isEmbeddedUrl(metaData.getURL());
 				}
 			}
 			return false;

spring-boot-project/spring-boot/src/test/java/org/springframework/boot/jdbc/EmbeddedDatabaseConnectionTests.java

@@ -16,10 +16,24 @@
 
 package org.springframework.boot.jdbc;
 
+import java.sql.Connection;
+import java.sql.DatabaseMetaData;
+import java.sql.SQLException;
+
+import javax.sql.DataSource;
+
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
+
+import org.springframework.jdbc.datasource.embedded.EmbeddedDatabase;
+import org.springframework.jdbc.datasource.embedded.EmbeddedDatabaseBuilder;
+import org.springframework.jdbc.datasource.embedded.EmbeddedDatabaseType;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
+import static org.mockito.BDDMockito.given;
+import static org.mockito.Mockito.mock;
 
 /**
  * Tests for {@link EmbeddedDatabaseConnection}.
@@ -78,15 +92,65 @@ void getUrlWithEmptyDatabaseNameForHsqldb() {
 				.withMessageContaining("DatabaseName must not be empty");
 	}
 
+	@ParameterizedTest(name = "{1}")
+	@MethodSource("embeddedDriverAndUrlParameters")
+	void isEmbeddedWithDriverAndUrl(EmbeddedDatabaseConnection connection, String url, boolean embedded) {
+		assertThat(EmbeddedDatabaseConnection.isEmbedded(connection.getDriverClassName(), url)).isEqualTo(embedded);
+	}
+
+	static Object[] embeddedDriverAndUrlParameters() {
+		return new Object[] { new Object[] { EmbeddedDatabaseConnection.H2, "jdbc:h2:~/test", false },
+				new Object[] { EmbeddedDatabaseConnection.H2, "jdbc:h2:mem:test;DB_CLOSE_DELAY=-1", true },
+				new Object[] { EmbeddedDatabaseConnection.HSQLDB, "jdbc:hsqldb:hsql://localhost", false },
+				new Object[] { EmbeddedDatabaseConnection.HSQLDB, "jdbc:hsqldb:mem:test", true },
+				new Object[] { EmbeddedDatabaseConnection.DERBY, "jdbc:derby:memory:test", true } };
+	}
+
+	@Test
+	void isEmbeddedWithH2DataSource() {
+		testEmbeddedDatabase(new EmbeddedDatabaseBuilder().setType(EmbeddedDatabaseType.H2).build());
+	}
+
 	@Test
-	void isEmbeddedForh2CustomDatabaseName() {
-		assertThat(EmbeddedDatabaseConnection.isEmbedded("org.h2.Driver", "jdbc:h2:~/test")).isFalse();
+	void isEmbeddedWithHsqlDataSource() {
+		testEmbeddedDatabase(new EmbeddedDatabaseBuilder().setType(EmbeddedDatabaseType.HSQL).build());
 	}
 
 	@Test
-	void isEmbeddedForh2EmbeddedDatabaseName() {
-		assertThat(EmbeddedDatabaseConnection.isEmbedded("org.h2.Driver",
-				"jdbc:h2:mem:b3c7d078-1362-4be7-a088-e25dcc3aee32;DB_CLOSE_DELAY=-1")).isTrue();
+	void isEmbeddedWithDerbyDataSource() {
+		testEmbeddedDatabase(new EmbeddedDatabaseBuilder().setType(EmbeddedDatabaseType.DERBY).build());
+	}
+
+	void testEmbeddedDatabase(EmbeddedDatabase database) {
+		try {
+			assertThat(EmbeddedDatabaseConnection.isEmbedded(database)).isTrue();
+		}
+		finally {
+			database.shutdown();
+		}
+	}
+
+	@Test
+	void isEmbeddedWithUnknownDataSource() throws SQLException {
+		assertThat(EmbeddedDatabaseConnection.isEmbedded(mockDataSource("unknown-db", null))).isFalse();
+	}
+
+	@Test
+	void isEmbeddedWithH2File() throws SQLException {
+		assertThat(EmbeddedDatabaseConnection
+				.isEmbedded(mockDataSource(EmbeddedDatabaseConnection.H2.getDriverClassName(), "jdbc:h2:~/test")))
+						.isFalse();
+	}
+
+	DataSource mockDataSource(String productName, String connectionUrl) throws SQLException {
+		DatabaseMetaData metaData = mock(DatabaseMetaData.class);
+		given(metaData.getDatabaseProductName()).willReturn(productName);
+		given(metaData.getURL()).willReturn(connectionUrl);
+		Connection connection = mock(Connection.class);
+		given(connection.getMetaData()).willReturn(metaData);
+		DataSource dataSource = mock(DataSource.class);
+		given(dataSource.getConnection()).willReturn(connection);
+		return dataSource;
 	}
 
 }