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

Document limitations of new approach to configure multiple DataSources #43481

Open
mdaepp opened this issue Dec 11, 2024 · 6 comments
Open

Document limitations of new approach to configure multiple DataSources #43481

mdaepp opened this issue Dec 11, 2024 · 6 comments
Labels
for: team-meeting An issue we'd like to discuss as a team to make progress status: on-hold We can't start working on this issue yet status: waiting-for-internal-feedback An issue that needs input from a member or another Spring Team status: waiting-for-triage An issue we've not yet triaged

Comments

@mdaepp
Copy link

mdaepp commented Dec 11, 2024

The documentation for Spring Boot 3.4 now recommends the @Qualifier("xyz") and @Bean(defaultCandidate = false) approach to define multiple DataSources: https://docs.spring.io/spring-boot/3.4/how-to/data-access.html#howto.data-access.configure-two-datasources

At first glance, this approach is neat because the first DataSource is set up by auto-configuration as if there were no second DataSource. However, the new approach has some probably unwanted side-effects that users should be made aware of.

With the previous approach (see https://docs.spring.io/spring-boot/3.3/how-to/data-access.html#howto.data-access.configure-two-datasources), the additional DataSources would still participate in other auto-configurations such as DataSourcePoolMetricsAutoConfiguration and DataSourceHealthContributorAutoConfiguration. That is no longer the case with the new approach, because an additional DataSource with defaultCandidate = false is neither present in Map<String, DataSource> nor is it available via ObjectProvider<DataSource>.

I leave it to the subject matter experts to decide what should be done:

  1. Update documentation to mention the limitations of the new approach
  2. Revert documentation to previous approach
  3. Fix other auto-configurations that apply to a "list" of DataSources of some sort (such as ObjectProvider<DataSource>)—not sure if that is even possible or a good idea for that matter
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 11, 2024
@quaff
Copy link
Contributor

quaff commented Dec 11, 2024

Fix other auto-configurations that apply to a "list" of DataSources of some sort (such as ObjectProvider<DataSource>)

ObjectProvider<DataSource> filter out non-defaultCandidate beans, we could use beanFactory.getBeansOfType(DataSource.class) instead of injection of Map<String, DataSource>.

@quaff
Copy link
Contributor

quaff commented Dec 11, 2024

Fix other auto-configurations that apply to a "list" of DataSources of some sort (such as ObjectProvider<DataSource>)

ObjectProvider<DataSource> filter out non-defaultCandidate beans, we could use beanFactory.getBeansOfType(DataSource.class) instead of injection of Map<String, DataSource>.

I'd like to prepare an PR if the team accept this solution, see https://github.com/spring-projects/spring-boot/compare/main...quaff:patch-108?expand=1.

quaff added a commit to quaff/spring-boot that referenced this issue Dec 11, 2024
Before this commit, non default and autowire candidate DataSources are not applied.

Closes spring-projectsGH-43481
@philwebb
Copy link
Member

philwebb commented Dec 11, 2024

I'm not a fan of using the BeanFactory directly, but I can't immediately think of a better option. @jhoeller Any thoughts on the best way to inject something that will include non-defaultCandidate beans? Perhaps we need some new methods on ObjectProvider or a new annotation to signal intent?

@philwebb philwebb added the status: waiting-for-internal-feedback An issue that needs input from a member or another Spring Team label Dec 11, 2024
@quaff
Copy link
Contributor

quaff commented Dec 12, 2024

I'm not a fan of using the BeanFactory directly, but I can't immediately think of a better option. @jhoeller Any thoughts on the best way to inject something that will include non-defaultCandidate beans? Perhaps we need some new methods on ObjectProvider or a new annotation to signal intent?

I vote for new methods on ObjectProvider, It's not good to introduce new annotation, make things more complex hard to understand.

@jhoeller
Copy link

jhoeller commented Dec 16, 2024

The intent was for such general container lookups to happen via beanFactory.getBeansOfType(DataSource.class) indeed, if necessary. defaultCandidate=false restricts user injection without qualifiers, and if framework components declare user-style injection points with those semantics, they get the same restricted view.

We could introduce new methods on ObjectProvider but it would blur the line with the user's view, so I would only consider that if it was a common need in user components. If defaultCandidate=false has unwanted side effects in a scenario, it might be the wrong choice. That said, for framework facilities, it's just a matter of using the appropriate lookup mechanism. In framework components, I don't consider direct interaction with an injected BeanFactory reference as inferior to ObjectProvider interaction.

@philwebb philwebb added for: team-meeting An issue we'd like to discuss as a team to make progress status: on-hold We can't start working on this issue yet labels Dec 16, 2024
@philwebb
Copy link
Member

Holding to the new year as we consider options

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: team-meeting An issue we'd like to discuss as a team to make progress status: on-hold We can't start working on this issue yet status: waiting-for-internal-feedback An issue that needs input from a member or another Spring Team status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

No branches or pull requests

5 participants