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

spring.datasource is ambiguous #2183

Closed
snicoll opened this issue Dec 17, 2014 · 9 comments
Closed

spring.datasource is ambiguous #2183

snicoll opened this issue Dec 17, 2014 · 9 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@snicoll
Copy link
Member

snicoll commented Dec 17, 2014

spring.datasource is used to configure Boot's own configuration support (see DataSourceProperties) but it's also used to configure the connection pool specific implementation when it's auto-configured.

Depending on the environment, spring-datasource can react to Tomcat, Hikari or Commons DBCP. These pools have commonalities but they do have conflicts too (url vs. jdbcUrl). It would probably be better if those boundaries where clearly identified. This has several advantages:

  • It's much more clear to the user they can configure implementation specific settings if we write that these are mapped from, let's say spring.datasource.tomcat for Tomcat
  • The generated meta-data use an isolated namespace for each implementation which clearly shows what a connections pool can do (IDE auto-completion)
  • If one of those connection pools decide to give a different semantic to a property that is managed by a different implementation, we are not affected since they have a different property name

The clear downside is that you have to configure the same settings twice if you happen to use 2 different connection pools and they share the same property.

We can approach this issue in two ways:

  1. We break it: no support for spring.datasource for implementation specific settings.
  2. We don't: not sure we should use @ConfigurationProperties for that then because we should only apply a property if it wasn't found in its specific namespace. Can be tricky.

A good approach would be to support the existing namespace and the implementation specific namespace ASAP and ask user to move to that model. Update meta-data harvesting so that only the specific namespaces are discovered (easy).

On a final note: we should think carefully about choosing the right namespace. If we go for spring.datasource.tomcat then it's clear that it becomes 'reserved' for the tomcat specific implementation (same for hikari, dbcp and dbcp2). Hopefully product names are specific enough.

@snicoll
Copy link
Member Author

snicoll commented May 18, 2015

The conclusion of our discussions is to go the break route (that is option 1). Please let me know asap if I got this wrong.

@snicoll
Copy link
Member Author

snicoll commented May 18, 2015

@cemo that's not the place to ask. We already discussed that (I just can't find the issue right now).

snicoll added a commit to snicoll/spring-boot that referenced this issue May 19, 2015
Previously, all datasource-related settings were bound to
`spring.datasource`, including datasource-specific settings.

Update `DataSourceBuilder` to bind to an arbitrary namespace and provide
a default one for known types (i.e. the tomcat implementation now binds
to `spring.datasource.tomcat`). Since `spring.datasource` is used for
common settings, it is still bound by default as well which makes this
change backward compatible.

Users were redirected to the use of `ConfigurationProperties` to
configure extra datasource instances. This is still valid but they now
can also use `DataSourceBuilder` directly to achieve the same effect and
still retain common settings if need to be.

Closes spring-projectsgh-2183
@snicoll
Copy link
Member Author

snicoll commented May 20, 2015

Okay so we had a lengthy discussion with @dsyer on the proposal (snicoll@e71504b) and it's definitely more complicated than I thought.

Spring cloud uses @ConfigurationProperties to identify the components that can be rebound at runtime and this change will actually break that feature. It seems that we have all that we need but some meta-data is missing to let Spring Cloud do its thing.

@philwebb
Copy link
Member

I think we should push this back to 1.4 for now and just live with things in 1.3.

@philwebb philwebb modified the milestones: 1.4.0, 1.3.0.RC1 Jul 10, 2015
@snicoll snicoll removed their assignment Aug 7, 2015
@philwebb philwebb modified the milestones: 1.4.0.M1, 1.4.0 Jan 8, 2016
@snicoll snicoll self-assigned this Jan 26, 2016
snicoll added a commit to snicoll/spring-boot that referenced this issue Jan 28, 2016
Previously, all datasource-related settings were bound to
`spring.datasource`, including datasource-specific settings.

Update `DataSourceBuilder` to bind to an arbitrary namespace and provide
a default one for known types (i.e. the tomcat implementation now binds
to `spring.datasource.tomcat`). Since `spring.datasource` is used for
common settings, it is still bound by default as well which makes this
change backward compatible.

Users were redirected to the use of `ConfigurationProperties` to
configure extra datasource instances. This is still valid but they now
can also use `DataSourceBuilder` directly to achieve the same effect and
still retain common settings if need to be.

Closes spring-projectsgh-2183
snicoll added a commit to snicoll/spring-boot that referenced this issue Jan 28, 2016
Previously, Spring Boot mapped both `DataSourceProperties` and the actual
`DataSource` implementation to the same prefix. This results in a huge
amount of keys in the `spring.datasource` namespace  with no way to
identify those that are valid for the pooled data source in use.

This commit maps the four pooled data sources we support in four isolated
namespace, keeping `spring.datasource` only for the common settings.

These are `spring.datasource.tomcat`, `spring.datasource.hikari`,
`spring.datasource.dbcp` and `spring.datasource.dbcp2` for the Tomcat,
Hikari, Commons DBCP and Commons DBCP2 implementations respectively.

Closes spring-projectsgh-2183
@snicoll snicoll closed this as completed in 34d87df Feb 3, 2016
@philwebb philwebb changed the title spring.datasource is ambiguous spring.datasource is ambiguous Feb 3, 2016
@snicoll
Copy link
Member Author

snicoll commented Feb 3, 2016

Updated the release notes as well.

@cbornet
Copy link

cbornet commented Feb 5, 2016

Is this working for properties that cannot be changed at runtime (eg . Hikari's connectionTestQuery) ?

@snicoll
Copy link
Member Author

snicoll commented Feb 5, 2016

That commit does not change anything related to binding. If it's working now, it will work as well and I can't see anything that would prevent proper binding.

@cbornet
Copy link

cbornet commented Feb 5, 2016

I had missed the binding part. Thanks to have pointed that.

@philwebb philwebb reopened this Feb 5, 2016
@philwebb
Copy link
Member

philwebb commented Feb 5, 2016

For some reason DataSourceAutoConfigurationTests now fails in eclipse. I think it might be because PooledDataSourceConfiguration is loading the nested configuration classes directly rather than via the import.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants