Skip to content

Enable proper override of binder beans with user-defined configurations in a multi-binder scenario #3123

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

Closed
wants to merge 1 commit into from

Conversation

ferblaca
Copy link
Contributor

closes #3114

…user-defined beans over binder's conditional beans in a multi-binder scenario

This change modifies the loading order in DefaultBinderFactory.initializeBinderContextSimple
to ensure that user configuration classes specified via spring.main.sources are loaded before
binder configuration classes. This allows @ConditionalOnMissingBean annotations in the binder
configurations to properly detect user-provided beans.

Previously, when using KafkaBinderConfiguration with @ConditionalOnMissingBean(KafkaBinderMetrics.class),
even if the user had configured a custom KafkaBinderMetrics bean via spring.main.sources,
the condition would not work correctly because binder classes were loaded first.

Fixes spring-cloudgh-3114

Signed-off-by: ferblaca <[email protected]>
@sobychacko
Copy link
Contributor

@ferblaca I thought you would add a ConditionalOnProperty on KafkaBinderMetrics, no? What made you change DefaultBinderFactory instead? Thanks!

@ferblaca
Copy link
Contributor Author

ferblaca commented Jun 18, 2025

Hi @sobychacko!!

After reviewing the issue, I noticed that it seemed to be a more general BUG in DefaultBinderFactory, since the @ConditionalOnMissingBean conditions in the binder configurations would not work in multi-binder scenarios, not just for the Kafka binder, but for all of them.

And to avoid mixing things up, I preferred to create this PR to solve this more general problem first, so the [kafka][metrics] labels in the title do not make sense for this PR and issue. I will update them, also the issue description to adapt it to PR.

@ferblaca ferblaca changed the title [kafka][metrics] Enable proper override of binder beans with user-defined configurations in a multi-binder scenario Enable proper override of binder beans with user-defined configurations in a multi-binder scenario Jun 18, 2025
@sobychacko
Copy link
Contributor

Okay, that's good to know. I will review it again and make sure that there are no side effects. Since DefaultBinderFactory is a common component and a core one in Spring Cloud Stream, we need to ensure that this change does not break any other binders, but I will review it and possibly merge it soon. Thanks!

@olegz
Copy link
Contributor

olegz commented Jun 23, 2025

@ferblaca would it be possible to include a test case(s)?

@olegz
Copy link
Contributor

olegz commented Jun 24, 2025

@ferblaca actually i played with your sample. It makes sense, so I'll merge it. You can still provide test cases as separate PR.
Thank you

@olegz olegz closed this in 5f94ca8 Jun 24, 2025
@olegz olegz added this to the 5.0.0 milestone Jun 24, 2025
olegz pushed a commit that referenced this pull request Jun 24, 2025
…user-defined beans over binder's conditional beans in a multi-binder scenario

This change modifies the loading order in DefaultBinderFactory.initializeBinderContextSimple
to ensure that user configuration classes specified via spring.main.sources are loaded before
binder configuration classes. This allows @ConditionalOnMissingBean annotations in the binder
configurations to properly detect user-provided beans.

Previously, when using KafkaBinderConfiguration with @ConditionalOnMissingBean(KafkaBinderMetrics.class),
even if the user had configured a custom KafkaBinderMetrics bean via spring.main.sources,
the condition would not work correctly because binder classes were loaded first.

Fixes gh-3114

Signed-off-by: ferblaca <[email protected]>
Resolves #3123
@ferblaca
Copy link
Contributor Author

ferblaca commented Jun 25, 2025

@ferblaca actually i played with your sample. It makes sense, so I'll merge it. You can still provide test cases as separate PR. Thank you

Ok @olegz, it'll take some time to add tests for this. In the previous PR, I looked into adding unit tests in the core module, but it wasn’t very clear how to approach it. It might be easier to verify this in an IT test for the Kafka binder module.

@olegz
Copy link
Contributor

olegz commented Jun 25, 2025

Yes, it would be difficult to do it in core, so IT test in Kafka would do. Thank you!!!

@ferblaca
Copy link
Contributor Author

ferblaca commented Jul 1, 2025

@ferblaca actually i played with your sample. It makes sense, so I'll merge it. You can still provide test cases as separate PR. Thank you

Hi @olegz, I have created PR #3127 to add the tests. Although everything compiles correctly and the tests pass locally, the PR seems to have failed in maven setup...

@sobychacko
Copy link
Contributor

@ferblaca It was a maven version issue with the CI builds, GitHub action for the PR's. I updated the version in the main branch. Could you rebase your PR with the latest main and push again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable proper override of binder beans with user-defined configurations in a multi-binder scenario
3 participants