SpringCloud/spring-cloud-stream
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

GH-1083: Disallow reuse bean name for bindings

Browse Source 
Resolves spring-cloud/spring-cloud-stream#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
This commit is contained in:
Artem Bilan
2017-09-27 13:21:41 -04:00
committed by Soby Chacko
parent 9fc51cb6e2
commit 4c33e5eb58
7 changed files with 167 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files

15
spring-cloud-stream-test-support/src/test/java/org/springframework/cloud/stream/test/aggregate/main/AggregateWithMainTest.java
View File

@@ -21,7 +21,8 @@ import java.util.concurrent.TimeUnit;
import org.junit.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.autoconfigure.SpringBootApplication;
import org.springframework.boot.SpringBootConfiguration;
import org.springframework.boot.autoconfigure.EnableAutoConfiguration;
import org.springframework.cloud.stream.aggregate.AggregateApplication;
import org.springframework.cloud.stream.aggregate.AggregateApplicationBuilder;
import org.springframework.cloud.stream.annotation.EnableBinding;
@@ -37,6 +38,7 @@ import static org.assertj.core.api.Assertions.assertThat;
/**
* @author Marius Bogoevici
* @author Artem Bilan
*/
public class AggregateWithMainTest {
@@ -59,13 +61,15 @@ public class AggregateWithMainTest {
context.close();
}
@SpringBootApplication
@SpringBootConfiguration
@EnableAutoConfiguration
public static class MainConfiguration {
}
@Configuration
@EnableBinding(Processor.class)
public static class UppercaseProcessor {
static class UppercaseProcessor {
@Autowired
Processor processor;
@@ -74,15 +78,18 @@ public class AggregateWithMainTest {
public String transform(String in) {
return in.toUpperCase();
}
}
@Configuration
@EnableBinding(Processor.class)
public static class SuffixProcessor {
static class SuffixProcessor {
@Transformer(inputChannel = Processor.INPUT, outputChannel = Processor.OUTPUT)
public String transform(String in) {
return in + " WORLD!";
}
}
}

8
spring-cloud-stream/src/main/java/org/springframework/cloud/stream/annotation/Input.java
View File

@@ -1,5 +1,5 @@
/*
* Copyright 2015 the original author or authors.
* Copyright 2015-2017 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -40,6 +40,12 @@ import org.springframework.beans.factory.annotation.Qualifier;
@Documented
public @interface Input {
/**
* Specify the binding target name;
* used as a bean name for binding target
* and as a destination name by default.
* @return the binding target name
*/
String value() default "";
}

9
spring-cloud-stream/src/main/java/org/springframework/cloud/stream/annotation/Output.java
View File

@@ -1,5 +1,5 @@
/*
* Copyright 2015 the original author or authors.
* Copyright 2015-2017 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -30,6 +30,7 @@ import org.springframework.beans.factory.annotation.Qualifier;
*
* @author Dave Syer
* @author Marius Bogoevici
* @author Artem Bilan
*/
@Qualifier
@@ -40,6 +41,12 @@ import org.springframework.beans.factory.annotation.Qualifier;
@Documented
public @interface Output {
/**
* Specify the binding target name;
* used as a bean name for binding target
* and as a destination name by default.
* @return the binding target name
*/
String value() default "";
}

9
spring-cloud-stream/src/main/java/org/springframework/cloud/stream/binding/BindingBeanDefinitionRegistryUtils.java
View File

@@ -1,5 +1,5 @@
/*
* Copyright 2015 the original author or authors.
* Copyright 2015-2017 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -20,6 +20,7 @@ import java.lang.annotation.Annotation;
import java.lang.reflect.Method;
import java.util.Map;
import org.springframework.beans.factory.BeanDefinitionStoreException;
import org.springframework.beans.factory.support.AutowireCandidateQualifier;
import org.springframework.beans.factory.support.BeanDefinitionRegistry;
import org.springframework.beans.factory.support.RootBeanDefinition;
@@ -36,6 +37,7 @@ import org.springframework.util.StringUtils;
*
* @author Marius Bogoevici
* @author Dave Syer
* @author Artem Bilan
*/
public abstract class BindingBeanDefinitionRegistryUtils {
@@ -57,6 +59,11 @@ public abstract class BindingBeanDefinitionRegistryUtils {
String qualifierValue, String name, String bindingTargetInterfaceBeanName,
String bindingTargetInterfaceMethodName, BeanDefinitionRegistry registry) {
if (registry.containsBeanDefinition(name)) {
throw new BeanDefinitionStoreException(bindingTargetInterfaceBeanName, name,
"bean definition with this name already exists - " + registry.getBeanDefinition(name));
}
RootBeanDefinition rootBeanDefinition = new RootBeanDefinition();
rootBeanDefinition.setFactoryBeanName(bindingTargetInterfaceBeanName);
rootBeanDefinition.setUniqueFactoryMethodName(bindingTargetInterfaceMethodName);

15
spring-cloud-stream/src/main/java/org/springframework/cloud/stream/config/BindingBeansRegistrar.java
View File

@@ -1,5 +1,5 @@
/*
* Copyright 2015 the original author or authors.
* Copyright 2015-2017 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -29,6 +29,7 @@ import org.springframework.util.ClassUtils;
/**
* @author Marius Bogoevici
* @author Dave Syer
* @author Artem Bilan
*/
public class BindingBeansRegistrar implements ImportBeanDefinitionRegistrar {
@@ -40,11 +41,13 @@ public class BindingBeansRegistrar implements ImportBeanDefinitionRegistrar {
ClassUtils.resolveClassName(metadata.getClassName(), null),
EnableBinding.class);
for (Class<?> type : collectClasses(attrs, metadata.getClassName())) {
BindingBeanDefinitionRegistryUtils.registerBindingTargetBeanDefinitions(type,
type.getName(), registry);
BindingBeanDefinitionRegistryUtils.registerBindingTargetsQualifiedBeanDefinitions(
ClassUtils.resolveClassName(metadata.getClassName(), null), type,
registry);
if (!registry.containsBeanDefinition(type.getName())) {
BindingBeanDefinitionRegistryUtils.registerBindingTargetBeanDefinitions(type,
type.getName(), registry);
BindingBeanDefinitionRegistryUtils.registerBindingTargetsQualifiedBeanDefinitions(
ClassUtils.resolveClassName(metadata.getClassName(), null), type,
registry);
}
}
}

61
spring-cloud-stream/src/test/java/org/springframework/cloud/stream/aggregation/AggregationTest.java
View File

@@ -32,8 +32,8 @@ import org.springframework.boot.autoconfigure.web.EmbeddedServletContainerAutoCo
import org.springframework.cloud.stream.aggregate.AggregateApplicationBuilder;
import org.springframework.cloud.stream.aggregate.AggregateApplicationBuilder.SourceConfigurer;
import org.springframework.cloud.stream.aggregate.SharedBindingTargetRegistry;
import org.springframework.cloud.stream.aggregate.SharedChannelRegistry;
import org.springframework.cloud.stream.annotation.EnableBinding;
import org.springframework.cloud.stream.annotation.Output;
import org.springframework.cloud.stream.binding.BindableProxyFactory;
import org.springframework.cloud.stream.binding.BindingTargetFactory;
import org.springframework.cloud.stream.messaging.Processor;
@@ -51,6 +51,7 @@ import static org.junit.Assert.assertTrue;
/**
* @author Marius Bogoevici
* @author Ilayaperumal Gopinathan
* @author Artem Bilan
*/
public class AggregationTest {
@@ -84,13 +85,14 @@ public class AggregationTest {
}
@Test
@SuppressWarnings("deprecation")
public void testModuleAggregationUsingSharedChannelRegistry() {
// test backward compatibility
aggregatedApplicationContext = new AggregateApplicationBuilder(
MockBinderRegistryConfiguration.class, "--server.port=0")
.from(TestSource.class).to(TestProcessor.class).run();
SharedChannelRegistry sharedChannelRegistry = aggregatedApplicationContext
.getBean(SharedChannelRegistry.class);
org.springframework.cloud.stream.aggregate.SharedChannelRegistry sharedChannelRegistry =
aggregatedApplicationContext.getBean(org.springframework.cloud.stream.aggregate.SharedChannelRegistry.class);
BindingTargetFactory channelFactory = aggregatedApplicationContext
.getBean(BindingTargetFactory.class);
assertThat(channelFactory).isNotNull();
@@ -99,6 +101,7 @@ public class AggregationTest {
}
@Test
@SuppressWarnings("unchecked")
public void testParentArgsAndSources() {
List<String> argsToVerify = new ArrayList<>();
argsToVerify.add("--foo1=bar1");
@@ -113,7 +116,6 @@ public class AggregationTest {
.namespace("foo").to(TestProcessor.class).namespace("bar")
.run("--foo3=bar3", "--server.port=0");
DirectFieldAccessor aggregateApplicationBuilderAccessor = new DirectFieldAccessor(aggregateApplicationBuilder);
@SuppressWarnings("unchecked")
final List<String> parentArgs = (List<String>) aggregateApplicationBuilderAccessor.getPropertyValue(
"parentArgs");
assertThat(parentArgs).containsExactlyInAnyOrder(argsToVerify.toArray(new String[argsToVerify.size()]));
@@ -125,8 +127,8 @@ public class AggregationTest {
}
@Test
@SuppressWarnings("unchecked")
public void testParentArgsAndSourcesWithWebDisabled() {
List<String> argsToVerify = new ArrayList<>();
AggregateApplicationBuilder aggregateApplicationBuilder = new AggregateApplicationBuilder(
MockBinderRegistryConfiguration.class, "--foo1=bar1");
final ConfigurableApplicationContext context = aggregateApplicationBuilder
@@ -155,7 +157,8 @@ public class AggregationTest {
((SourceConfigurer) aggregateApplicationBuilderAccessor.getPropertyValue("sourceConfigurer"))
.getArgs(),
new String[] { "--foo1=bar1" }));
final List<AggregateApplicationBuilder.ProcessorConfigurer> processorConfigurers = (List<AggregateApplicationBuilder.ProcessorConfigurer>) aggregateApplicationBuilderAccessor
final List<AggregateApplicationBuilder.ProcessorConfigurer> processorConfigurers =
(List<AggregateApplicationBuilder.ProcessorConfigurer>) aggregateApplicationBuilderAccessor
.getPropertyValue("processorConfigurers");
for (AggregateApplicationBuilder.ProcessorConfigurer processorConfigurer : processorConfigurers) {
if (processorConfigurer.getNamespace().equals("b")) {
@@ -185,7 +188,8 @@ public class AggregationTest {
((SourceConfigurer) aggregateApplicationBuilderAccessor.getPropertyValue("sourceConfigurer"))
.getArgs(),
new String[] { "--fooValue=bara" }));
final List<AggregateApplicationBuilder.ProcessorConfigurer> processorConfigurers = (List<AggregateApplicationBuilder.ProcessorConfigurer>) aggregateApplicationBuilderAccessor
final List<AggregateApplicationBuilder.ProcessorConfigurer> processorConfigurers =
(List<AggregateApplicationBuilder.ProcessorConfigurer>) aggregateApplicationBuilderAccessor
.getPropertyValue("processorConfigurers");
for (AggregateApplicationBuilder.ProcessorConfigurer processorConfigurer : processorConfigurers) {
if (processorConfigurer.getNamespace().equals("b")) {
@@ -215,7 +219,8 @@ public class AggregationTest {
((SourceConfigurer) aggregateApplicationBuilderAccessor.getPropertyValue("sourceConfigurer"))
.getArgs(),
new String[] { "--fooValue=bara" }));
final List<AggregateApplicationBuilder.ProcessorConfigurer> processorConfigurers = (List<AggregateApplicationBuilder.ProcessorConfigurer>) aggregateApplicationBuilderAccessor
final List<AggregateApplicationBuilder.ProcessorConfigurer> processorConfigurers =
(List<AggregateApplicationBuilder.ProcessorConfigurer>) aggregateApplicationBuilderAccessor
.getPropertyValue("processorConfigurers");
for (AggregateApplicationBuilder.ProcessorConfigurer processorConfigurer : processorConfigurers) {
if (processorConfigurer.getNamespace().equals("b")) {
@@ -247,7 +252,8 @@ public class AggregationTest {
((SourceConfigurer) aggregateApplicationBuilderAccessor.getPropertyValue("sourceConfigurer"))
.getArgs(),
new String[] { "--fooValue=bara" }));
final List<AggregateApplicationBuilder.ProcessorConfigurer> processorConfigurers = (List<AggregateApplicationBuilder.ProcessorConfigurer>) aggregateApplicationBuilderAccessor
final List<AggregateApplicationBuilder.ProcessorConfigurer> processorConfigurers =
(List<AggregateApplicationBuilder.ProcessorConfigurer>) aggregateApplicationBuilderAccessor
.getPropertyValue("processorConfigurers");
for (AggregateApplicationBuilder.ProcessorConfigurer processorConfigurer : processorConfigurers) {
if (processorConfigurer.getNamespace().equals("b")) {
@@ -279,9 +285,8 @@ public class AggregationTest {
((SourceConfigurer) aggregateApplicationBuilderAccessor.getPropertyValue("sourceConfigurer"))
.getArgs(),
new String[] { "--foo-value=sysbara" }));
final List<AggregateApplicationBuilder.ProcessorConfigurer> processorConfigurers = (List<AggregateApplicationBuilder.ProcessorConfigurer>) aggregateApplicationBuilderAccessor
.getPropertyValue("processorConfigurers");
for (AggregateApplicationBuilder.ProcessorConfigurer processorConfigurer : ((List<AggregateApplicationBuilder.ProcessorConfigurer>) aggregateApplicationBuilderAccessor
for (AggregateApplicationBuilder.ProcessorConfigurer processorConfigurer :
((List<AggregateApplicationBuilder.ProcessorConfigurer>) aggregateApplicationBuilderAccessor
.getPropertyValue(
"processorConfigurers"))) {
if (processorConfigurer.getNamespace().equals("b")) {
@@ -329,13 +334,14 @@ public class AggregationTest {
}
@Test
@SuppressWarnings("deprecation")
public void testNamespaces() {
aggregatedApplicationContext = new AggregateApplicationBuilder(
MockBinderRegistryConfiguration.class, "--server.port=0")
.from(TestSource.class).namespace("foo").to(TestProcessor.class)
.namespace("bar").run();
SharedChannelRegistry sharedChannelRegistry = aggregatedApplicationContext
.getBean(SharedChannelRegistry.class);
org.springframework.cloud.stream.aggregate.SharedChannelRegistry sharedChannelRegistry =
aggregatedApplicationContext.getBean(org.springframework.cloud.stream.aggregate.SharedChannelRegistry.class);
BindingTargetFactory channelFactory = aggregatedApplicationContext
.getBean(BindingTargetFactory.class);
Object fooOutput = sharedChannelRegistry.get("foo.output");
@@ -353,16 +359,22 @@ public class AggregationTest {
public void testBindableProxyFactoryCaching() {
AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(
MockBinderRegistryConfiguration.class,
TestSource.class, TestProcessor.class);
TestSource2.class, TestProcessor.class);
Map<String, BindableProxyFactory> factories = context.getBeansOfType(BindableProxyFactory.class);
assertThat(factories).hasSize(2);
Map<String, Source> sources = context.getBeansOfType(Source.class);
assertThat(sources).hasSize(2);
assertThat(sources).hasSize(1);
for (Source source : sources.values()) {
source.output();
}
Map<String, FooSource> fooSources = context.getBeansOfType(FooSource.class);
assertThat(fooSources).hasSize(1);
for (FooSource source : fooSources.values()) {
source.output();
}
Map<String, Processor> processors = context.getBeansOfType(Processor.class);
assertThat(processors).hasSize(1);
for (Processor processor : processors.values()) {
@@ -377,6 +389,9 @@ public class AggregationTest {
if (factory.getObjectType() == Source.class) {
assertThat(targetCache).hasSize(1);
}
if (factory.getObjectType() == FooSource.class) {
assertThat(targetCache).hasSize(1);
}
else if (factory.getObjectType() == Processor.class) {
assertThat(targetCache).hasSize(2);
}
@@ -399,6 +414,20 @@ public class AggregationTest {
}
public interface FooSource {
@Output("fooOutput")
MessageChannel output();
}
@EnableBinding(FooSource.class)
@EnableAutoConfiguration
public static class TestSource2 {
}
@Configuration
public static class DummyConfig {

79
spring-cloud-stream/src/test/java/org/springframework/cloud/stream/binding/InvalidBindingConfigurationTests.java Normal file
View File

@@ -0,0 +1,79 @@
/*
* Copyright 2017 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.springframework.cloud.stream.binding;
import org.assertj.core.api.ThrowableAssert;
import org.junit.Test;
import org.springframework.beans.factory.BeanDefinitionStoreException;
import org.springframework.boot.SpringApplication;
import org.springframework.boot.autoconfigure.EnableAutoConfiguration;
import org.springframework.cloud.stream.annotation.EnableBinding;
import org.springframework.cloud.stream.annotation.Input;
import org.springframework.cloud.stream.annotation.Output;
import org.springframework.cloud.stream.utils.MockBinderRegistryConfiguration;
import org.springframework.context.annotation.Import;
import org.springframework.messaging.MessageChannel;
import org.springframework.messaging.SubscribableChannel;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
/**
* @author Artem Bilan
*
* @since 1.3
*/
public class InvalidBindingConfigurationTests {
@Test
public void testDuplicateBeanByBindingConfig() {
assertThatThrownBy(
new ThrowableAssert.ThrowingCallable() {
@Override
public void call() throws Throwable {
SpringApplication.run(TestBindingConfig.class);
}
})
.isInstanceOf(BeanDefinitionStoreException.class)
.hasMessageContaining("bean definition with this name already exists")
.hasMessageContaining(TestInvalidBinding.NAME)
.hasNoCause();
}
@EnableBinding(TestInvalidBinding.class)
@EnableAutoConfiguration
@Import(MockBinderRegistryConfiguration.class)
public static class TestBindingConfig {
}
public interface TestInvalidBinding {
String NAME = "testName";
@Input(NAME)
SubscribableChannel in();
@Output(NAME)
MessageChannel out();
}
}