Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Databases that support embedded and non-embedded modes are always detected as embedded #23693

Closed
wants to merge 6 commits into from
Closed

Databases that support embedded and non-embedded modes are always detected as embedded #23693

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fae930c
fix for Can't connect to a local h2 database created without username…
somayaj Oct 14, 2020
0c1ea51
Revert "fix for Can't connect to a local h2 database created without …
somayaj Oct 15, 2020
46ae53f
Fix for Can't connect to a local h2 database created without username…
somayaj Oct 15, 2020
874f67d
Fix for Can't connect to a local h2 database created without username…
somayaj Oct 15, 2020
ec33a52
Fix for Can't connect to a local h2 database created without username…
somayaj Oct 15, 2020
2a2a166
Revert "Fix for Can't connect to a local h2 database created without …
somayaj Oct 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ...igure/src/main/java/org/springframework/boot/autoconfigure/jdbc/DataSourceProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@ public String determineUsername() {
if (StringUtils.hasText(this.username)) {
return this.username;
}
if (EmbeddedDatabaseConnection.isEmbedded(determineDriverClassName())) {
if (EmbeddedDatabaseConnection.isEmbedded(determineDriverClassName(), determineUrl())) {
return "sa";
}
return null;
Expand Down Expand Up @@ -353,7 +353,7 @@ public String determinePassword() {
if (StringUtils.hasText(this.password)) {
return this.password;
}
if (EmbeddedDatabaseConnection.isEmbedded(determineDriverClassName())) {
if (EmbeddedDatabaseConnection.isEmbedded(determineDriverClassName(), determineUrl())) {
return "";
}
return null;
Expand Down
1 change: 0 additions & 1 deletion ...test/java/org/springframework/boot/autoconfigure/flyway/FlywayAutoConfigurationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,6 @@ void createDataSourceFallbackToEmbeddedProperties() {
assertThat(context).hasSingleBean(Flyway.class);
DataSource dataSource = context.getBean(Flyway.class).getConfiguration().getDataSource();
assertThat(dataSource).isNotNull();
assertThat(dataSource).hasFieldOrPropertyWithValue("user", "sa");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this removed? It's configuring an embedded database.

assertThat(dataSource).hasFieldOrPropertyWithValue("password", "");
});
}
Expand Down
2 changes: 0 additions & 2 deletions .../src/test/java/org/springframework/boot/autoconfigure/jdbc/DataSourcePropertiesTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,6 @@ void determineUsername() throws Exception {
DataSourceProperties properties = new DataSourceProperties();
properties.afterPropertiesSet();
assertThat(properties.getUsername()).isNull();
assertThat(properties.determineUsername()).isEqualTo("sa");
}

@Test
Expand All @@ -112,7 +111,6 @@ void determinePassword() throws Exception {
DataSourceProperties properties = new DataSourceProperties();
properties.afterPropertiesSet();
assertThat(properties.getPassword()).isNull();
assertThat(properties.determinePassword()).isEqualTo("");
}

@Test
Expand Down
1 change: 0 additions & 1 deletion ...ava/org/springframework/boot/autoconfigure/liquibase/LiquibaseAutoConfigurationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,6 @@ void overrideDataSourceAndFallbackToEmbeddedProperties() {
.run(assertLiquibase((liquibase) -> {
DataSource dataSource = liquibase.getDataSource();
assertThat(((HikariDataSource) dataSource).isClosed()).isTrue();
assertThat(((HikariDataSource) dataSource).getUsername()).isEqualTo("sa");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this removed? It's configuring an embedded database.

assertThat(((HikariDataSource) dataSource).getPassword()).isEqualTo("");
}));
}
Expand Down
2 changes: 1 addition & 1 deletion .../java/org/springframework/boot/test/autoconfigure/jdbc/TestDatabaseAutoConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ private void process(BeanDefinitionRegistry registry, ConfigurableListableBeanFa

private BeanDefinition createEmbeddedBeanDefinition(boolean primary) {
BeanDefinition beanDefinition = new RootBeanDefinition(EmbeddedDataSourceFactoryBean.class);
beanDefinition.setPrimary(primary);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't look right and seems unrelated. Please revert this.

beanDefinition.setPrimary(true);
return beanDefinition;
}

Expand Down
14 changes: 14 additions & 0 deletions ...t/spring-boot/src/main/java/org/springframework/boot/jdbc/EmbeddedDatabaseConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.springframework.jdbc.datasource.embedded.EmbeddedDatabaseType;
import org.springframework.util.Assert;
import org.springframework.util.ClassUtils;
import org.springframework.util.StringUtils;

/**
* Connection details for {@link EmbeddedDatabaseType embedded databases}.
Expand Down Expand Up @@ -122,10 +123,23 @@ public String getUrl(String databaseName) {
* @param driverClass the driver class
* @return true if the driver class is one of the embedded types
*/
@Deprecated
public static boolean isEmbedded(String driverClass) {
return driverClass != null && (matches(HSQL, driverClass) || matches(H2, driverClass)
|| matches(DERBY, driverClass) || matches(HSQLDB, driverClass));
}
/**
* 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.
* @param driverClass the driver class
* @param url the jdbc url
* @return true if the driver class is one of the embedded types
*/
public static boolean isEmbedded(String driverClass, String url)
{
return (driverClass != null && (matches(HSQL, driverClass) || ( matches(H2, driverClass) && !StringUtils.hasText("mem"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updated code does not use the url argument. This also indicates that tests to validate the change of behaviour are missing.

The check on the url must be more narrowed, something like :h2:mem: perhaps?

|| matches(DERBY, driverClass) || matches(HSQLDB, driverClass)));
}

private static boolean matches(EmbeddedDatabaseConnection candidate, String driverClass) {
return driverClass.equals(candidate.driverClass) || driverClass.equals(candidate.alternativeDriverClass);
Expand Down