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

GH-1083: Disallow reuse bean name for bindings #1084

Closed
wants to merge 1 commit into from

Conversation

artembilan
Copy link
Contributor

Resolves #1083

By default Spring Framework allows beans overriding via the same name.
The binding target definitions (@Input and @Output) populate beans as well
and when we use the same name for target we end up with unexpected behavior
but without errors.
Since it isn't so obvious via Spring Framework bean definition DSLs
(XML or Java & Annotations) how to override beans with the same name,
that is absolutely easy to use the same value for @Input and @Output
definitions even in different binding interfaces.
That's hard to analyze fro the target application since mostly
@Input and @Output produce MessageChannel beans.

  • Fail fast with the BeanDefinitionStoreException when we meet existing
    bean definition for the same name
  • Add JavaDocs to the @Input and @Output to explain that their value
    is a bean name, as well as destination by default

Since @EnableBinding is @Inherited, the inheritor picks up it from the
super class during configuration class parsing.
The parsing process logic is such that after the root class we go to parse its
super classes, and therefore come back to the @EnableBinding again.
In this case we process all the @Imports one more time and collect them to
the root configurationClass.
Essentially we get a duplication for the ImportBeanDefinitionRegistrars
such as BindingBeansRegistrar.
The last one parsed @EnableBinding and registers appropriate beans for the
@Input and @Output, as well as for the binding interface - BindableProxyFactory.
But since we have it twice in the configurationClass we end up with
BeanDefinitionStoreException mentioned before.
That's how Spring Framework works with inheritance for configuration classes
and that's may be why it allows to override beans by default

  • Skip parsing @EnableBinding one more time if the bean definition for
    binding interface is already present in the registry
  • Fix AggregateWithMainTest do not process @ComponentScan what causes
    picking up the configuration classes for children contexts in the aggregation
  • Fix testBindableProxyFactoryCaching() do not register Source and Processor
    in the same application context because both of them cause registration for the
    Source.OUTPUT bean

Resolves spring-cloud#1083

By default Spring Framework allows beans overriding via the same name.
The binding target definitions (`@Input` and `@Output`) populate beans as well
and when we use the same name for target we end up with unexpected behavior
but without errors.
Since it isn't so obvious via Spring Framework bean definition DSLs
(XML or Java & Annotations) how to override beans with the same name,
that is absolutely easy to use the same value for `@Input` and `@Output`
definitions even in different binding interfaces.
That's hard to analyze fro the target application since mostly
`@Input` and `@Output` produce `MessageChannel` beans.

* Fail fast with the `BeanDefinitionStoreException` when we meet existing
bean definition for the same name
* Add JavaDocs to the `@Input` and `@Output` to explain that their `value`
is a bean name, as well as destination by default

Since `@EnableBinding` is `@Inherited`, the inheritor picks up it from the
super class during configuration class parsing.
The parsing process logic is such that after the root class we go to parse its
super classes, and therefore come back to the `@EnableBinding` again.
In this case we process all the `@Import`s one more time and collect them to
the root `configurationClass`.
Essentially we get a duplication for the `ImportBeanDefinitionRegistrar`s
such as `BindingBeansRegistrar`.
The last one parsed `@EnableBinding` and registers appropriate beans for the
`@Input` and `@Output`, as well as for the binding interface - `BindableProxyFactory`.
But since we have it twice in the `configurationClass` we end up with
`BeanDefinitionStoreException` mentioned before.
That's how Spring Framework works with inheritance for configuration classes
and that's may be why it allows to override beans by default

* Skip parsing `@EnableBinding` one more time if the bean definition for
binding interface is already present in the `registry`
* Fix `AggregateWithMainTest` do not process `@ComponentScan` what causes
picking up the configuration classes for children contexts in the aggregation
* Fix `testBindableProxyFactoryCaching()` do not register `Source` and `Processor`
in the same application context because both of them cause registration for the
`Source.OUTPUT` bean
@codecov-io
Copy link

Codecov Report

Merging #1084 into master will increase coverage by 0.02%.
The diff coverage is 87.5%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1084      +/-   ##
============================================
+ Coverage     51.46%   51.48%   +0.02%     
- Complexity      705      706       +1     
============================================
  Files           134      134              
  Lines          4001     4005       +4     
  Branches        540      541       +1     
============================================
+ Hits           2059     2062       +3     
  Misses         1786     1786              
- Partials        156      157       +1
Impacted Files Coverage Δ Complexity Δ
...am/binding/BindingBeanDefinitionRegistryUtils.java 85.36% <100%> (+1.15%) 8 <2> (+1) ⬆️
...ork/cloud/stream/config/BindingBeansRegistrar.java 92.3% <80%> (-7.7%) 4 <0> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3701615...efe64d9. Read the comment docs.

@sobychacko sobychacko self-requested a review September 27, 2017 19:40
@sobychacko sobychacko self-assigned this Sep 28, 2017
@sobychacko
Copy link
Contributor

LGTM - merged.

@sobychacko sobychacko closed this Sep 28, 2017
@sobychacko sobychacko removed the in pr label Sep 28, 2017
sobychacko pushed a commit to sobychacko/spring-cloud-stream that referenced this pull request Feb 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants