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.

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

DataSource url property is ignored when there is no connection pool #19576

Closed
wants to merge 1 commit into from

Conversation

nosan
Copy link
Contributor

@nosan nosan commented Jan 8, 2020

see gh-19192

Btw, I found an issue in DataSourceBuilder, the builder does not support SimpleDriverDataSource without alias aliases.addAliases("driver-class-name", "driver-class")

Here is a test to reproduce:

	@Test
	void simpleDriverDataSource() {
		this.dataSource = DataSourceBuilder.create().url("jdbc:h2:test").type(SimpleDriverDataSource.class).build();
		assertThat(this.dataSource).isInstanceOf(SimpleDriverDataSource.class);
		assertThat(this.dataSource).extracting("driver").isNotNull();
	}

`

UPDATE

My first approach was incorrect. Sorry for the inconvenience.

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

snicoll commented Jan 8, 2020

Thanks for the PR @nosan.

I found an issue in DataSourceBuilder, the builder does not support SimpleDriverDataSource

It's not meant to at the moment. The supported connection pools are commons-dbcp2, tomcat and hikari (default).

@nosan
Copy link
Contributor Author

nosan commented Jan 8, 2020

Thanks for the feedback, @snicoll

It's not meant to at the moment. The supported connection pools are commons-dbcp2, tomcat and hikari (default).

Yes, I saw that when I was working on this PR.
Consider this test in DataSourceAutoConfigurationTests:

@Test
void explicitTypeSupportedDataSource() {
	this.contextRunner
			.withPropertyValues("spring.datasource.driverClassName:org.hsqldb.jdbcDriver",
					"spring.datasource.url:jdbc:hsqldb:mem:testdb",
					"spring.datasource.type:" + SimpleDriverDataSource.class.getName())
			.run(this::containsOnlySimpleDriverDataSource);
}

Actually, this test just checks that SimpleDriverDataSource will be created, and actually, it will, but still, if you try to getConnection from this DataSource you will get an exception:

java.lang.IllegalArgumentException: Driver must not be null

	at org.springframework.util.Assert.notNull(Assert.java:198)
	at org.springframework.jdbc.datasource.SimpleDriverDataSource.getConnectionFromDriver(SimpleDriverDataSource.java:139)
	at org.springframework.jdbc.datasource.AbstractDriverBasedDataSource.getConnectionFromDriver(AbstractDriverBasedDataSource.java:205)
	at org.springframework.jdbc.datasource.AbstractDriverBasedDataSource.getConnection(AbstractDriverBasedDataSource.java:169)
	at org.springframework.boot.autoconfigure.jdbc.DataSourceAutoConfigurationTests.containsOnlySimpleDriverDataSource(DataSourceAutoConfigurationTests.java:172)

This happens because DataSourceBuilder does not add alias:

aliases.addAliases("driver-class-name", "driver-class");

P.S. I think you are right and this is not related to the initial issue.

@nosan nosan force-pushed the gh-19192 branch 4 times, most recently from 4742549 to 6819f48 Compare January 9, 2020 12:16

}

static class GenericCondition extends AnyNestedCondition {
Copy link
Member

Choose a reason for hiding this comment

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

We should rename this when merging. Something like ExplicitDataSourceTypeOrUrlCondition.

@wilkinsona wilkinsona changed the title DataSource 'spring.datasource.url' property should be considered if it was explicitly set DataSource url property is ignored when there is no connection pool Jan 14, 2020
@wilkinsona wilkinsona added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 14, 2020
@wilkinsona wilkinsona added this to the 2.1.x milestone Jan 14, 2020
@@ -158,6 +158,20 @@ void explicitTypeNoSupportedDataSource() {
.run(this::containsOnlySimpleDriverDataSource);
}

/**
* This test makes sure that if no supported pool data source is present, a datasource
Copy link
Member

Choose a reason for hiding this comment

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

If possible, when merging, we should replace this comment with a descriptive test method name

@snicoll
Copy link
Member

snicoll commented Feb 11, 2020

I was looking at this PR and I am not keen to bring support for SimpleDriverDataSource back in 2.1.x. I'd rather let it fail which is what would happen if there wasn't an embedded driver on the claspath in the first place.

With a polish that doesn't bring support for SimpleDriverDataSource it would fail as follow (rather than creating an embedded database if one is available):

Caused by: java.lang.IllegalStateException: No supported DataSource type found
 	at org.springframework.boot.jdbc.DataSourceBuilder.getType(DataSourceBuilder.java:140)
 	at org.springframework.boot.jdbc.DataSourceBuilder.build(DataSourceBuilder.java:72)
 	at org.springframework.boot.autoconfigure.jdbc.DataSourceConfiguration$Generic.dataSource(DataSourceConfiguration.java:125)

I suggest to improve that so that our failure analyzer catch that and provide a dedicated error message.

We can then add support for SimpleDriverDataSource in 2.3.x as a fallback for this scenario. Adding such support is required for r2dbc anyway.

@snicoll snicoll added the for: team-attention An issue we'd like other members of the team to review label Feb 11, 2020
@filiphr

This comment has been minimized.

@snicoll

This comment has been minimized.

@snicoll
Copy link
Member

snicoll commented Feb 18, 2020

@nosan thank you for your effort on this but I am not keen to bring SimpleDriverDataSource in 2.1.x. Looking at the issue, avoiding the embedded configuration to kick in if an url is set sounds like a safer and more isolated change to me. I've just done that and I've reopened the related issue. Thanks again.

@snicoll snicoll closed this Feb 18, 2020
@snicoll snicoll added status: declined A suggestion or change that we don't feel we should currently apply and removed for: merge-with-amendments Needs some changes when we merge for: team-attention An issue we'd like other members of the team to review type: bug A general bug labels Feb 18, 2020
@snicoll snicoll removed this from the 2.1.x milestone Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants