Fix NPE if no database url is given and no embedded database is available #10626
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
tinexw
force-pushed
the
8690
branch
3 times, most recently
from
7d292ed to
2e3b1ff
Compare
snicoll
left a comment
snicoll
left a comment
There was a problem hiding this comment.
Thanks for the PR, I've added a few suggestions.
| return this.url; | ||
| } | ||
|
|
||
| logger.debug("No url defined, looking for embedded database."); |
There was a problem hiding this comment.
I don't think adding a logging statement here is part of what is discussed in the issue you've referenced.
There was a problem hiding this comment.
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.
| @ConfigurationProperties(prefix = "spring.datasource") | ||
| public class DataSourceProperties | ||
| implements BeanClassLoaderAware, EnvironmentAware, InitializingBean { | ||
| implements BeanClassLoaderAware, EnvironmentAware, InitializingBean { |
There was a problem hiding this comment.
Can you please fix this formatting change?
| 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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...?
There was a problem hiding this comment.
But maybe I misunderstand this completely...?
Or I do. Let me run that thing concretely, thanks for the feedback.
snicoll
changed the title
snicoll
added
priority: normal
type: enhancement
3 participants
Fixes gh-8690