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

Jump to bottom

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

wants to merge 6 commits into from

Conversation

somayaj
Copy link
Contributor

@somayaj somayaj commented Oct 14, 2020

… #23538

Bug fix for Can't connect to a local h2 database created without username #23538

The changes were,
#1 The jdbc username/password is now determined by evaluating the existence of the jdbc url
#2 The postProcessBeanDefinitionRegistry method in the TestDatabaseAutoConfiguration does not replace the datasource by the embedded database reference anymore.

This enables allowing a "" empty string username and password as documented in the bug description for the jdbc databases and also enables the database declarated type to define the intrinsic db type selected.
So, a configuration of the below are both supported.
#1 database=h2
spring.datasource.schema=classpath*:db/${database}/schema.sql
spring.datasource.data=classpath*:db/${database}/data.sql
#2
spring.datasource.url=jdbc:h2:~/test
spring.datasource.schema=classpath*:db/${database}/schema.sql
spring.datasource.data=classpath*:db/${database}/data.sql
spring.datasource.username=
spring.datasource.password=
spring.jpa.database-platform=org.hibernate.dialect.H2Dialect

Tests: I've made sure all unit tests and integration tests are working as expected.
I've used spring-petclinic (2.3.0.BUILD-SNAPSHOT)(springboot-2.4.0-SNAPSHOT) for the integration testing to validate changes.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 14, 2020
@wilkinsona
Copy link
Member

wilkinsona commented Oct 15, 2020 •
edited
Loading

Thanks for the pull request. Unfortunately, it causes several tests to fail. The failures are:

:spring-boot-project:spring-boot-autoconfigure:test
    org.springframework.boot.autoconfigure.flyway.FlywayAutoConfigurationTests > createDataSourceFallbackToEmbeddedProperties()
    org.springframework.boot.autoconfigure.flyway.FlywayAutoConfigurationTests > createDataSourceFallbackToEmbeddedProperties()
    org.springframework.boot.autoconfigure.flyway.FlywayAutoConfigurationTests > createDataSourceFallbackToEmbeddedProperties()
    org.springframework.boot.autoconfigure.flyway.FlywayAutoConfigurationTests > createDataSourceFallbackToEmbeddedProperties()
    org.springframework.boot.autoconfigure.jdbc.DataSourceAutoConfigurationTests > testEmbeddedTypeDefaultsUsername()
    org.springframework.boot.autoconfigure.jdbc.DataSourceAutoConfigurationTests > testEmbeddedTypeDefaultsUsername()
    org.springframework.boot.autoconfigure.jdbc.DataSourceAutoConfigurationTests > testEmbeddedTypeDefaultsUsername()
    org.springframework.boot.autoconfigure.jdbc.DataSourceAutoConfigurationTests > testEmbeddedTypeDefaultsUsername()
    org.springframework.boot.autoconfigure.jdbc.DataSourcePropertiesTests > determinePassword()
    org.springframework.boot.autoconfigure.jdbc.DataSourcePropertiesTests > determinePassword()
    org.springframework.boot.autoconfigure.jdbc.DataSourcePropertiesTests > determinePassword()
    org.springframework.boot.autoconfigure.jdbc.DataSourcePropertiesTests > determinePassword()
    org.springframework.boot.autoconfigure.jdbc.DataSourcePropertiesTests > determineUsername()
    org.springframework.boot.autoconfigure.jdbc.DataSourcePropertiesTests > determineUsername()
    org.springframework.boot.autoconfigure.jdbc.DataSourcePropertiesTests > determineUsername()
    org.springframework.boot.autoconfigure.jdbc.DataSourcePropertiesTests > determineUsername()
    org.springframework.boot.autoconfigure.liquibase.LiquibaseAutoConfigurationTests > overrideDataSourceAndFallbackToEmbeddedProperties()
    org.springframework.boot.autoconfigure.liquibase.LiquibaseAutoConfigurationTests > overrideDataSourceAndFallbackToEmbeddedProperties()
    org.springframework.boot.autoconfigure.liquibase.LiquibaseAutoConfigurationTests > overrideDataSourceAndFallbackToEmbeddedProperties()
    org.springframework.boot.autoconfigure.liquibase.LiquibaseAutoConfigurationTests > overrideDataSourceAndFallbackToEmbeddedProperties()

:spring-boot-project:spring-boot-test-autoconfigure:test
    org.springframework.boot.test.autoconfigure.data.jdbc.DataJdbcTestIntegrationTests > didNotInjectExampleComponent()
    org.springframework.boot.test.autoconfigure.data.jdbc.DataJdbcTestIntegrationTests > didNotInjectExampleComponent()
    org.springframework.boot.test.autoconfigure.data.jdbc.DataJdbcTestIntegrationTests > didNotInjectExampleComponent()
    org.springframework.boot.test.autoconfigure.data.jdbc.DataJdbcTestIntegrationTests > didNotInjectExampleComponent()
    org.springframework.boot.test.autoconfigure.data.jdbc.DataJdbcTestIntegrationTests > flywayAutoConfigurationWasImported()
    org.springframework.boot.test.autoconfigure.data.jdbc.DataJdbcTestIntegrationTests > flywayAutoConfigurationWasImported()
    org.springframework.boot.test.autoconfigure.data.jdbc.DataJdbcTestIntegrationTests > flywayAutoConfigurationWasImported()
    org.springframework.boot.test.autoconfigure.data.jdbc.DataJdbcTestIntegrationTests > flywayAutoConfigurationWasImported()
    org.springframework.boot.test.autoconfigure.data.jdbc.DataJdbcTestIntegrationTests > liquibaseAutoConfigurationWasImported()
    org.springframework.boot.test.autoconfigure.data.jdbc.DataJdbcTestIntegrationTests > liquibaseAutoConfigurationWasImported()
    org.springframework.boot.test.autoconfigure.data.jdbc.DataJdbcTestIntegrationTests > liquibaseAutoConfigurationWasImported()
    org.springframework.boot.test.autoconfigure.data.jdbc.DataJdbcTestIntegrationTests > liquibaseAutoConfigurationWasImported()
    org.springframework.boot.test.autoconfigure.data.jdbc.DataJdbcTestIntegrationTests > replacesDefinedDataSourceWithEmbeddedDefault()
    org.springframework.boot.test.autoconfigure.data.jdbc.DataJdbcTestIntegrationTests > replacesDefinedDataSourceWithEmbeddedDefault()
    org.springframework.boot.test.autoconfigure.data.jdbc.DataJdbcTestIntegrationTests > replacesDefinedDataSourceWithEmbeddedDefault()
    org.springframework.boot.test.autoconfigure.data.jdbc.DataJdbcTestIntegrationTests > replacesDefinedDataSourceWithEmbeddedDefault()
    org.springframework.boot.test.autoconfigure.data.jdbc.DataJdbcTestIntegrationTests > testRepository()
    org.springframework.boot.test.autoconfigure.data.jdbc.DataJdbcTestIntegrationTests > testRepository()
    org.springframework.boot.test.autoconfigure.data.jdbc.DataJdbcTestIntegrationTests > testRepository()
    org.springframework.boot.test.autoconfigure.data.jdbc.DataJdbcTestIntegrationTests > testRepository()
    org.springframework.boot.test.autoconfigure.jdbc.AutoConfigureTestDatabaseWithMultipleDatasourcesIntegrationTests > replacesDefinedDataSourceWithExplicit()
    org.springframework.boot.test.autoconfigure.jdbc.AutoConfigureTestDatabaseWithMultipleDatasourcesIntegrationTests > replacesDefinedDataSourceWithExplicit()
    org.springframework.boot.test.autoconfigure.jdbc.AutoConfigureTestDatabaseWithMultipleDatasourcesIntegrationTests > replacesDefinedDataSourceWithExplicit()
    org.springframework.boot.test.autoconfigure.jdbc.AutoConfigureTestDatabaseWithMultipleDatasourcesIntegrationTests > replacesDefinedDataSourceWithExplicit()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestIntegrationTests > didNotInjectExampleRepository()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestIntegrationTests > didNotInjectExampleRepository()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestIntegrationTests > didNotInjectExampleRepository()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestIntegrationTests > didNotInjectExampleRepository()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestIntegrationTests > flywayAutoConfigurationWasImported()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestIntegrationTests > flywayAutoConfigurationWasImported()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestIntegrationTests > flywayAutoConfigurationWasImported()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestIntegrationTests > flywayAutoConfigurationWasImported()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestIntegrationTests > liquibaseAutoConfigurationWasImported()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestIntegrationTests > liquibaseAutoConfigurationWasImported()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestIntegrationTests > liquibaseAutoConfigurationWasImported()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestIntegrationTests > liquibaseAutoConfigurationWasImported()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestIntegrationTests > replacesDefinedDataSourceWithEmbeddedDefault()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestIntegrationTests > replacesDefinedDataSourceWithEmbeddedDefault()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestIntegrationTests > replacesDefinedDataSourceWithEmbeddedDefault()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestIntegrationTests > replacesDefinedDataSourceWithEmbeddedDefault()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestIntegrationTests > testJdbcTemplate()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestIntegrationTests > testJdbcTemplate()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestIntegrationTests > testJdbcTemplate()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestIntegrationTests > testJdbcTemplate()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestWithAutoConfigureTestDatabaseReplaceExplicitIntegrationTests > replacesDefinedDataSourceWithExplicit()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestWithAutoConfigureTestDatabaseReplaceExplicitIntegrationTests > replacesDefinedDataSourceWithExplicit()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestWithAutoConfigureTestDatabaseReplaceExplicitIntegrationTests > replacesDefinedDataSourceWithExplicit()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestWithAutoConfigureTestDatabaseReplaceExplicitIntegrationTests > replacesDefinedDataSourceWithExplicit()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestWithAutoConfigureTestDatabaseReplacePropertyAnyIntegrationTests > replacesDefinedDataSourceWithExplicit()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestWithAutoConfigureTestDatabaseReplacePropertyAnyIntegrationTests > replacesDefinedDataSourceWithExplicit()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestWithAutoConfigureTestDatabaseReplacePropertyAnyIntegrationTests > replacesDefinedDataSourceWithExplicit()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestWithAutoConfigureTestDatabaseReplacePropertyAnyIntegrationTests > replacesDefinedDataSourceWithExplicit()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestWithIncludeFilterIntegrationTests > testRepository()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestWithIncludeFilterIntegrationTests > testRepository()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestWithIncludeFilterIntegrationTests > testRepository()
    org.springframework.boot.test.autoconfigure.jdbc.JdbcTestWithIncludeFilterIntegrationTests > testRepository()
    org.springframework.boot.test.autoconfigure.jdbc.TestDatabaseAutoConfigurationTests > replaceWithUniqueDatabase()
    org.springframework.boot.test.autoconfigure.jdbc.TestDatabaseAutoConfigurationTests > replaceWithUniqueDatabase()
    org.springframework.boot.test.autoconfigure.jdbc.TestDatabaseAutoConfigurationTests > replaceWithUniqueDatabase()
    org.springframework.boot.test.autoconfigure.jdbc.TestDatabaseAutoConfigurationTests > replaceWithUniqueDatabase()
    org.springframework.boot.test.autoconfigure.jooq.JooqTestWithAutoConfigureTestDatabaseIntegrationTests > replacesAutoConfiguredDataSource()
    org.springframework.boot.test.autoconfigure.jooq.JooqTestWithAutoConfigureTestDatabaseIntegrationTests > replacesAutoConfiguredDataSource()
    org.springframework.boot.test.autoconfigure.jooq.JooqTestWithAutoConfigureTestDatabaseIntegrationTests > replacesAutoConfiguredDataSource()
    org.springframework.boot.test.autoconfigure.jooq.JooqTestWithAutoConfigureTestDatabaseIntegrationTests > replacesAutoConfiguredDataSource()
    org.springframework.boot.test.autoconfigure.orm.jpa.DataJpaTestIntegrationTests > replacesDefinedDataSourceWithEmbeddedDefault()
    org.springframework.boot.test.autoconfigure.orm.jpa.DataJpaTestIntegrationTests > replacesDefinedDataSourceWithEmbeddedDefault()
    org.springframework.boot.test.autoconfigure.orm.jpa.DataJpaTestIntegrationTests > replacesDefinedDataSourceWithEmbeddedDefault()
    org.springframework.boot.test.autoconfigure.orm.jpa.DataJpaTestIntegrationTests > replacesDefinedDataSourceWithEmbeddedDefault()
    org.springframework.boot.test.autoconfigure.orm.jpa.TestDatabaseAutoConfigurationNoEmbeddedTests > applyAnyReplace()
    org.springframework.boot.test.autoconfigure.orm.jpa.TestDatabaseAutoConfigurationNoEmbeddedTests > applyAnyReplace()
    org.springframework.boot.test.autoconfigure.orm.jpa.TestDatabaseAutoConfigurationNoEmbeddedTests > applyAnyReplace()
    org.springframework.boot.test.autoconfigure.orm.jpa.TestDatabaseAutoConfigurationNoEmbeddedTests > applyAnyReplace()

The jdbc username/password is now determined by evaluating the existence of the jdbc url

This doesn't feel quite right to me. Instead, I think we'll need to update EmbeddedDatabaseConnection so that it returns false for an H2 database that doesn't have mem in its protocol. This would result in isEmbedded(String driverClass) being deprecated and a new method being introduced that takes the driverClass and the URL.

As @snicoll said this isn't going to be easy to fix and I'm not certain that what I've described above is the right fix without trying it. If you'd like to update this pull request to address the test failures and try this approach, then you are more than welcome to do so. Equally, if you'd prefer that we figure out what needs to be done then we can take care of it in due course.

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Oct 15, 2020
@somayaj
Copy link
Contributor Author

somayaj commented Oct 15, 2020 via email

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 15, 2020
@wilkinsona
Copy link
Member

I can also make the change to check for the "mem" in the url if you want and give it a try if that's ok.

Yes, that's certainly ok. It's what I would try if I was attempting to fix the problem.

@somayaj
Fix for Can't connect to a local h2 database created without username #…
46ae53f 
…23538", the url is now added to the isEmbedded() and for the autoconfiguration of the database - the primary db rewire is set to true.
@somayaj
Fix for Can't connect to a local h2 database created without username #…
874f67d 
…23538". Fixes for the failing tests.
@somayaj
Fix for Can't connect to a local h2 database created without username #…
ec33a52 
…23538". formatting fixes for the failing tests.
@snicoll snicoll added the for: team-attention An issue we'd like other members of the team to review label Oct 15, 2020
snicoll
snicoll requested changes Oct 15, 2020
View reviewed changes
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. This is heading in the right direction but I've found a number of issues.

Please run the build locally and do not use our CI as a way to test your changes.

...rc/test/java/org/springframework/boot/autoconfigure/flyway/FlywayAutoConfigurationTests.java
@@ -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.

...t/java/org/springframework/boot/autoconfigure/liquibase/LiquibaseAutoConfigurationTests.java
@@ -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.

...ain/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(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.

...ject/spring-boot/src/main/java/org/springframework/boot/jdbc/EmbeddedDatabaseConnection.java Outdated
*/
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?

@somayaj
Revert "Fix for Can't connect to a local h2 database created without …
2a2a166 
…username #23538". formatting fixes for the failing tests."

This reverts commit ec33a52.
@somayaj
Copy link
Contributor Author

somayaj commented Oct 15, 2020

Sure, yes. My local works (when it works that is, the mac is crashing every time I try to build here...)

@somayaj
Copy link
Contributor Author

somayaj commented Oct 15, 2020

Fyi, not able to find the root cause here for these, [ant:checkstyle] /root/.gradle/caches/modules-2/files-2.1/com.samskivert/jmustache/1.15/7b3b15951d13b774c76db2f4e14d977952f8b4d8/jmustache-1.15.jar(com/samskivert/mustache/Mustache.java):306: warning: no @return
[ant:checkstyle] Reader getTemplate (String name) throws Exception;

@snicoll snicoll changed the title Fix for Can't connect to a local h2 database created without username… Databases that support embedded and non-embedded modes are always detected as embedded Oct 16, 2020
snicoll pushed a commit to snicoll/spring-boot that referenced this pull request Oct 16, 2020
@somayaj @snicoll
Fix detection logic for embedded databases
b62902f 
See spring-projectsgh-23693
snicoll added a commit to snicoll/spring-boot that referenced this pull request Oct 16, 2020
@snicoll
Polish "Fix detection logic for embedded databases"
548c192 
See spring-projectsgh-23693
snicoll pushed a commit that referenced this pull request Oct 16, 2020
@somayaj @snicoll
Fix detection logic for embedded databases
ab02084 
See gh-23693
snicoll added a commit that referenced this pull request Oct 16, 2020
@snicoll
Polish "Fix detection logic for embedded databases"
c0b267f 
See gh-23693
@snicoll snicoll closed this in 08b0099 Oct 16, 2020
@snicoll snicoll added type: bug A general bug and removed for: team-attention An issue we'd like other members of the team to review status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Oct 16, 2020
@snicoll snicoll added this to the 2.4.0-RC1 milestone Oct 16, 2020
@snicoll snicoll self-assigned this Oct 16, 2020
@snicoll snicoll mentioned this pull request Oct 16, 2020
Closed
@snicoll
Copy link
Member

snicoll commented Oct 16, 2020

@somayaj thanks for your efforts on this issue. There was a number of things that still need to be covered (or reverted as they were changing behaviour beyond the scope of this issue). See c0b267f for more details.

@snicoll snicoll mentioned this pull request Oct 16, 2020
Closed
@snicoll

This comment has been minimized.

Sign in to view
snicoll added a commit that referenced this pull request Oct 17, 2020
@snicoll
Polish
bebdee3 
See gh-23693
@snicoll snicoll added the status: noteworthy A noteworthy issue to call out in the release notes label Oct 20, 2020
@snicoll
Copy link
Member

snicoll commented Oct 20, 2020

This issue is noteworthy due to the following behaviour changes:

  • Now that the embedded mode is detected based on the actual JDBC URI and not only the driver at play, H2, HSQL and Derby with file-based persistence (server mode) won't be initialised by default. spring.datasource.initialization-mode=always needs to be set if that's what you want.
  • Similarly, the default user name doesn't default to sa anymore for those. If you've created a database before upgrading to this version, it is likely it was created with sa as the default username and upgrading will lead to an authentication failure. Setting spring.datasource.username=sa will restore the previous behaviour.

@snicoll snicoll mentioned this pull request Oct 20, 2020
Closed
izeye added a commit to izeye/my-dictionary that referenced this pull request Oct 30, 2020
@izeye
Upgrade to Spring Boot 2.4.0-RC1
e23f556 
This commit also removes a workaround for spring-projects/spring-boot#23735 and adds spring.datasource.username property to work with the changes made in spring-projects/spring-boot#23693.
@izeye
Copy link
Contributor

izeye commented Oct 30, 2020

This has been labeled as status: noteworthy, but a note for this seems to be missing in Spring Boot 2.4.0 RC1 Release Notes.

@snicoll
Copy link
Member

snicoll commented Oct 30, 2020

Thank you very much for the nudge @izeye. I've added a section in the release notes for RC1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snicoll snicoll snicoll requested changes

Assignees

@snicoll snicoll

Labels
status: noteworthy A noteworthy issue to call out in the release notes type: bug A general bug
Projects
None yet
Milestone
2.4.0-RC1
Development

Successfully merging this pull request may close these issues.
5 participants
@somayaj @wilkinsona @snicoll @izeye @spring-projects-issues