Skip to content

Fix NPE if no database url is given and no embedded database is available #10626

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

Closed
wants to merge 2 commits into from
Closed

Fix NPE if no database url is given and no embedded database is available #10626

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
96f078e
Fix NPE if no database url is given
Oct 6, 2017
e045b36
Fix formatting
Oct 17, 2017
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
8 changes: 8 additions & 0 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 @@ -24,6 +24,9 @@

import javax.sql.DataSource;

import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;

import org.springframework.beans.factory.BeanClassLoaderAware;
import org.springframework.beans.factory.BeanCreationException;
import org.springframework.beans.factory.InitializingBean;
Expand All @@ -46,12 +49,15 @@
* @author Stephane Nicoll
* @author Benedikt Ritter
* @author Eddú Meléndez
* @author Kristine Jetzke
* @since 1.1.0
*/
@ConfigurationProperties(prefix = "spring.datasource")
public class DataSourceProperties
implements BeanClassLoaderAware, EnvironmentAware, InitializingBean {

private static final Log logger = LogFactory.getLog(DataSourceProperties.class);

private ClassLoader classLoader;

private Environment environment;
Expand Down Expand Up @@ -288,6 +294,8 @@ public String determineUrl() {
if (StringUtils.hasText(this.url)) {
return this.url;
}

logger.debug("No url defined, looking for embedded database.");
Copy link
Member

@snicoll snicoll Oct 17, 2017

Choose a reason for hiding this comment

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

I don't think adding a logging statement here is part of what is discussed in the issue you've referenced.

Copy link
Contributor Author

@tinexw tinexw Oct 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

My idea here was to give more insight into what's happening in the case described in the issue. As far as I understood it is hard to find out why there is this error because of the "magic" happening with with embedded database (i.e. no url means: try to connect to an embedded database)

But the original issue was about something different anyway, I was trying to do the

Let's fix the error message though

So I can remove the logging statement if you want.

String url = this.embeddedDatabaseConnection.getUrl(determineDatabaseName());
if (!StringUtils.hasText(url)) {
throw new DataSourceBeanCreationException(this.embeddedDatabaseConnection,
Expand Down
10 changes: 10 additions & 0 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 @@ -21,13 +21,15 @@
import org.springframework.boot.jdbc.EmbeddedDatabaseConnection;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

/**
* Tests for {@link DataSourceProperties}.
*
* @author Maciej Walkowiak
* @author Stephane Nicoll
* @author Eddú Meléndez
* @author Kristine Jetzke
*/
public class DataSourcePropertiesTests {

Expand Down Expand Up @@ -59,6 +61,14 @@ public void determineUrl() throws Exception {
.isEqualTo(EmbeddedDatabaseConnection.H2.getUrl());
}

@Test
public void determineUrlUrlNull() throws Exception {
DataSourceProperties properties = new DataSourceProperties();
assertThatThrownBy(() -> properties.determineUrl())
.isInstanceOf(DataSourceProperties.DataSourceBeanCreationException.class)
.hasMessageContaining("Cannot determine embedded database url");
}

@Test
public void determineUrlWithExplicitConfig() throws Exception {
DataSourceProperties properties = new DataSourceProperties();
Expand Down
3 changes: 2 additions & 1 deletion ...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 @@ -34,6 +34,7 @@
* @author Phillip Webb
* @author Dave Syer
* @author Stephane Nicoll
* @author Kristine Jetzke
* @see #get(ClassLoader)
*/
public enum EmbeddedDatabaseConnection {
Expand Down Expand Up @@ -106,7 +107,7 @@ public String getUrl() {
*/
public String getUrl(String databaseName) {
Assert.hasText(databaseName, "DatabaseName must not be null.");
return String.format(this.url, databaseName);
return this.url != null ? String.format(this.url, databaseName) : null;
Copy link
Member

@snicoll snicoll Oct 17, 2017

Choose a reason for hiding this comment

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

That wasn't the intention as raised on #8690 - The idea is to avoid a NPE by an abuse of DataSourceProperties (as explained in the comments). I'd rather have an IllegalStateException here that states the url should not be null or something.

Thoughts?

Copy link
Contributor Author

@tinexw tinexw Oct 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think the result for the user would basically be the same.
With this implementation they get an DataSourceBeanCreationException with the message Cannot determine embedded database url
If I change it to your proposal they get an IllegalStateExceptionwith a message that states the url should not be null or something.

I would prefer to not add the null check here because one of the enum constants is NONE(null, null, null), so calling the getUrl method on this enum constant would result in an exception which is not so nice because if we wanted to enforce the not null policy it should not be possible to even create objects that violate this policy imho.

Copy link
Member

@snicoll snicoll Oct 17, 2017

Choose a reason for hiding this comment

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

The difference is that you give a perfectly valid exception message for a use case that isn't. I don't know if you've picked up that issue or if you faced it yourself but the issue as described doesn't result from a legit use case. I think we should then treat it as such.

Copy link
Contributor Author

@tinexw tinexw Oct 17, 2017

Choose a reason for hiding this comment

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

But it is a legit use case to define DataSourceProperties without a url - at least in the current implementation. Because then this fallback to the embedded database takes place (in my opinion it would be better to not have this magic at all but it’s there already so I don’t think it can/should be changed and is definitely not in scope of this issue.)
The error case from the issue is that no url is defined and no embedded database is available.
I just think the exception should be thrown from the DataSourceProperties and not from the EmbeddedDatabaseConnection because if you don’t even add an embedded database, why should you get an error that originates in an EmbeddedDatabaseConnection class?
But maybe I misunderstand this completely...?

Copy link
Member

@snicoll snicoll Oct 18, 2017

Choose a reason for hiding this comment

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

But maybe I misunderstand this completely...?

Or I do. Let me run that thing concretely, thanks for the feedback.

}

/**
Expand Down