From d5303ca4f3b8d3fca24e8969fcd92fbca7c632fb Mon Sep 17 00:00:00 2001
From: Artem Bilan <abilan@pivotal.io>
Date: Fri, 23 Feb 2018 14:40:07 -0500
Subject: [PATCH] INT-4413: Don't register flow under the same id

JIRA: https://jira.spring.io/browse/INT-4413

When we override entity in the `IntegrationFlowContext.registry`,
we may get dangling beans in the application context,
when the structure of a new `IntegrationFlow` is different.

* Fix `IntegrationFlowContext` to disallow to override the flow
registration under the same name
* Add JavaDocs to the `IntegrationFlowRegistrationBuilder` methods
* Add `toString()` to the `IntegrationFlowRegistration` and
`StandardIntegrationFlow` to make the logging of these components
friendlier
---
 .../dsl/StandardIntegrationFlow.java          | 10 ++--
 .../dsl/context/IntegrationFlowContext.java   | 48 +++++++++++++++++--
 .../context/IntegrationFlowRegistration.java  | 10 +++-
 .../dsl/manualflow/ManualFlowTests.java       | 27 +++++++++++
 4 files changed, 87 insertions(+), 8 deletions(-)

diff --git a/spring-integration-core/src/main/java/org/springframework/integration/dsl/StandardIntegrationFlow.java b/spring-integration-core/src/main/java/org/springframework/integration/dsl/StandardIntegrationFlow.java
index d657de9740..945168a99a 100644
--- a/spring-integration-core/src/main/java/org/springframework/integration/dsl/StandardIntegrationFlow.java
+++ b/spring-integration-core/src/main/java/org/springframework/integration/dsl/StandardIntegrationFlow.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2016-2017 the original author or authors.
+ * Copyright 2016-2018 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.
@@ -44,7 +44,7 @@ import org.springframework.context.SmartLifecycle;
  * However, when we register an {@link IntegrationFlow} dynamically using the
  * {@link org.springframework.integration.dsl.context.IntegrationFlowContext} API,
  * the lifecycle processor from the application context is not involved;
- * therefore we should control the lifecyle of the beans manually, or rely on the
+ * therefore we should control the lifecycle of the beans manually, or rely on the
  * {@link org.springframework.integration.dsl.context.IntegrationFlowContext} API.
  * Its created registration <b>is</b> {@code autoStartup} by default and
  * starts the flow when it is registered. If you disable the registration's auto-
@@ -146,6 +146,11 @@ public class StandardIntegrationFlow implements IntegrationFlow, SmartLifecycle
 		return 0;
 	}
 
+	@Override
+	public String toString() {
+		return "StandardIntegrationFlow{integrationComponents=" + this.integrationComponents + '}';
+	}
+
 	private static final class AggregatingCallback implements Runnable {
 
 		private final AtomicInteger count;
@@ -166,5 +171,4 @@ public class StandardIntegrationFlow implements IntegrationFlow, SmartLifecycle
 
 	}
 
-
 }
diff --git a/spring-integration-core/src/main/java/org/springframework/integration/dsl/context/IntegrationFlowContext.java b/spring-integration-core/src/main/java/org/springframework/integration/dsl/context/IntegrationFlowContext.java
index 57cc104a6b..4cba85a25f 100644
--- a/spring-integration-core/src/main/java/org/springframework/integration/dsl/context/IntegrationFlowContext.java
+++ b/spring-integration-core/src/main/java/org/springframework/integration/dsl/context/IntegrationFlowContext.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2016-2017 the original author or authors.
+ * Copyright 2016-2018 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.
@@ -36,7 +36,7 @@ import org.springframework.messaging.MessageChannel;
 import org.springframework.util.Assert;
 
 /**
- * A public API for dynamic (manual) registration of {@link IntegrationFlow},
+ * A public API for dynamic (manual) registration of {@link IntegrationFlow}s,
  * not via standard bean registration phase.
  * <p>
  * The bean of this component is provided via framework automatically.
@@ -45,7 +45,7 @@ import org.springframework.util.Assert;
  * <p>
  * The typical use-case, and, therefore algorithm, is:
  * <ul>
- * <li> create {@link IntegrationFlow} depending of the business logic
+ * <li> create an {@link IntegrationFlow} instance depending of the business logic
  * <li> register that {@link IntegrationFlow} in this {@link IntegrationFlowContext},
  * with optional {@code id} and {@code autoStartup} flag
  * <li> obtain a {@link MessagingTemplate} for that {@link IntegrationFlow}
@@ -98,6 +98,11 @@ public final class IntegrationFlowContext implements BeanFactoryAware {
 			flowId = generateBeanName(integrationFlow, null);
 			builder.id(flowId);
 		}
+		else if (this.registry.containsKey(flowId)) {
+			throw new IllegalArgumentException("An IntegrationFlow '" + this.registry.get(flowId) +
+					"' with flowId '" + flowId + "' is already registered.\n" +
+					"An existing IntegrationFlowRegistration must be destroyed before overriding.");
+		}
 		IntegrationFlow theFlow = (IntegrationFlow) registerBean(integrationFlow, flowId, null);
 		builder.integrationFlowRegistration.setIntegrationFlow(theFlow);
 
@@ -209,31 +214,66 @@ public final class IntegrationFlowContext implements BeanFactoryAware {
 
 		private boolean autoStartup = true;
 
-		private IntegrationFlowRegistrationBuilder(IntegrationFlow integrationFlow) {
+		IntegrationFlowRegistrationBuilder(IntegrationFlow integrationFlow) {
 			this.integrationFlowRegistration = new IntegrationFlowRegistration(integrationFlow);
 			this.integrationFlowRegistration.setBeanFactory(IntegrationFlowContext.this.beanFactory);
 			this.integrationFlowRegistration.setIntegrationFlowContext(IntegrationFlowContext.this);
 		}
 
+		/**
+		 * Specify an {@code id} for the {@link IntegrationFlow} to register.
+		 * Must be unique per context.
+		 * The registration with this {@code id} must be destroyed before reusing for
+		 * a new {@link IntegrationFlow} instance.
+		 * @param id the id for the {@link IntegrationFlow} to register
+		 * @return the current builder instance
+		 */
 		public IntegrationFlowRegistrationBuilder id(String id) {
 			this.integrationFlowRegistration.setId(id);
 			return this;
 		}
 
+		/**
+		 * The {@code boolean} flag to indication if an {@link IntegrationFlow} must be started
+		 * automatically after registration. Defaults to {@code true}.
+		 * @param autoStartup start or not the {@link IntegrationFlow} automatically after registration.
+		 * @return the current builder instance
+		 */
 		public IntegrationFlowRegistrationBuilder autoStartup(boolean autoStartup) {
 			this.autoStartup = autoStartup;
 			return this;
 		}
 
+		/**
+		 * Add an object which will be registered as an {@link IntegrationFlow} dependant bean in the
+		 * application context. Usually it is some support component, which needs an application context.
+		 * For example dynamically created connection factories or header mappers for AMQP, JMS, TCP etc.
+		 * @param bean an additional arbitrary bean to register into the application context.
+		 * @return the current builder instance
+		 */
 		public IntegrationFlowRegistrationBuilder addBean(Object bean) {
 			return addBean(null, bean);
 		}
 
+		/**
+		 * Add an object which will be registered as an {@link IntegrationFlow} dependant bean in the
+		 * application context. Usually it is some support component, which needs an application context.
+		 * For example dynamically created connection factories or header mappers for AMQP, JMS, TCP etc.
+		 * @param name the name for the bean to register.
+		 * @param bean an additional arbitrary bean to register into the application context.
+		 * @return the current builder instance
+		 */
 		public IntegrationFlowRegistrationBuilder addBean(String name, Object bean) {
 			this.additionalBeans.put(bean, name);
 			return this;
 		}
 
+		/**
+		 * Register an {@link IntegrationFlow} and all the dependant and support components
+		 * in the application context and return an associated {@link IntegrationFlowRegistration}
+		 * control object.
+		 * @return the {@link IntegrationFlowRegistration} instance.
+		 */
 		public IntegrationFlowRegistration register() {
 			IntegrationFlowContext.this.register(this);
 			return this.integrationFlowRegistration;
diff --git a/spring-integration-core/src/main/java/org/springframework/integration/dsl/context/IntegrationFlowRegistration.java b/spring-integration-core/src/main/java/org/springframework/integration/dsl/context/IntegrationFlowRegistration.java
index 014c694a04..8156052144 100644
--- a/spring-integration-core/src/main/java/org/springframework/integration/dsl/context/IntegrationFlowRegistration.java
+++ b/spring-integration-core/src/main/java/org/springframework/integration/dsl/context/IntegrationFlowRegistration.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2016-2017 the original author or authors.
+ * Copyright 2016-2018 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.
@@ -157,4 +157,12 @@ public class IntegrationFlowRegistration {
 		this.integrationFlowContext.remove(this.id);
 	}
 
+	@Override
+	public String toString() {
+		return "IntegrationFlowRegistration{integrationFlow=" + this.integrationFlow +
+				", id='" + this.id + '\'' +
+				", inputChannel=" + this.inputChannel +
+				'}';
+	}
+
 }
diff --git a/spring-integration-core/src/test/java/org/springframework/integration/dsl/manualflow/ManualFlowTests.java b/spring-integration-core/src/test/java/org/springframework/integration/dsl/manualflow/ManualFlowTests.java
index 1ae3bd7ff7..6adea417d9 100644
--- a/spring-integration-core/src/test/java/org/springframework/integration/dsl/manualflow/ManualFlowTests.java
+++ b/spring-integration-core/src/test/java/org/springframework/integration/dsl/manualflow/ManualFlowTests.java
@@ -32,6 +32,7 @@ import java.util.Date;
 import java.util.Objects;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Supplier;
 
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -54,6 +55,7 @@ import org.springframework.integration.dsl.IntegrationFlowAdapter;
 import org.springframework.integration.dsl.IntegrationFlowDefinition;
 import org.springframework.integration.dsl.IntegrationFlows;
 import org.springframework.integration.dsl.MessageProducerSpec;
+import org.springframework.integration.dsl.StandardIntegrationFlow;
 import org.springframework.integration.dsl.channel.MessageChannels;
 import org.springframework.integration.dsl.context.IntegrationFlowContext;
 import org.springframework.integration.dsl.context.IntegrationFlowRegistration;
@@ -391,6 +393,31 @@ public class ManualFlowTests {
 		assertNull(resultChannel.receive(0));
 	}
 
+	@Test
+	public void testRegistrationDuplicationRejected() {
+		String testId = "testId";
+
+		StandardIntegrationFlow testFlow =
+				IntegrationFlows.from(Supplier.class)
+						.get();
+
+		this.integrationFlowContext
+				.registration(testFlow)
+				.id(testId)
+				.register();
+
+		try {
+			this.integrationFlowContext
+					.registration(testFlow)
+					.id(testId)
+					.register();
+		}
+		catch (Exception e) {
+			assertThat(e, instanceOf(IllegalArgumentException.class));
+			assertThat(e.getMessage(), containsString("with flowId '" + testId + "' is already registered."));
+		}
+	}
+
 	@Configuration
 	@EnableIntegration
 	public static class RootConfiguration {