Fix next handling in ComponentFlow

- This fixes a bug where returning null from a next()
  didn't stop a flow.
- Backport #510
- Fixes #513

(cherry picked from commit 7c4700b)

Author: jvalkeal

spring-shell-core/src/main/java/org/springframework/shell/component/flow/ComponentFlow.java

@@ -421,11 +421,19 @@ private DefaultComponentFlowResult runGetResults() {
 				context = node.data.getOperation().apply(context);
 				if (node.data.next != null) {
 					Optional<String> n = node.data.next.apply(context);
-					if (n.isPresent()) {
+					if (n == null) {
+						// actual function returned null optional which is case
+						// when no next function was set. this is different
+						// than when null is returned for next.
+						node = node.next;
+					}
+					else if (n.isPresent()) {
+						// user returned value
 						node = oiol.get(n.get());
 					}
 					else {
-						node = node.next;
+						// user returned null
+						node = null;
 					}
 				}
 				else {
@@ -474,7 +482,7 @@ private Stream<OrderedInputOperation> stringInputsStream() {
 				Function<StringInputContext, String> f1 = input.getNext();
 				Function<ComponentContext<?>, Optional<String>> f2 = context -> f1 != null
 						? Optional.ofNullable(f1.apply(selector.getThisContext(context)))
-						: Optional.empty();
+						: null;
 				return OrderedInputOperation.of(input.getId(), input.getOrder(), operation, f2);
 			});
 		}
@@ -512,7 +520,7 @@ private Stream<OrderedInputOperation> pathInputsStream() {
 				Function<PathInputContext, String> f1 = input.getNext();
 				Function<ComponentContext<?>, Optional<String>> f2 = context -> f1 != null
 						? Optional.ofNullable(f1.apply(selector.getThisContext(context)))
-						: Optional.empty();
+						: null;
 				return OrderedInputOperation.of(input.getId(), input.getOrder(), operation, f2);
 			});
 		}
@@ -550,7 +558,7 @@ private Stream<OrderedInputOperation> confirmationInputsStream() {
 				Function<ConfirmationInputContext, String> f1 = input.getNext();
 				Function<ComponentContext<?>, Optional<String>> f2 = context -> f1 != null
 						? Optional.ofNullable(f1.apply(selector.getThisContext(context)))
-						: Optional.empty();
+						: null;
 				return OrderedInputOperation.of(input.getId(), input.getOrder(), operation, f2);
 			});
 		}
@@ -609,7 +617,7 @@ private Stream<OrderedInputOperation> singleItemSelectorsStream() {
 				Function<SingleItemSelectorContext<String, SelectorItem<String>>, String> f1 = input.getNext();
 				Function<ComponentContext<?>, Optional<String>> f2 = context -> f1 != null
 						? Optional.ofNullable(f1.apply(selector.getThisContext(context)))
-						: Optional.empty();
+						: null;
 				return OrderedInputOperation.of(input.getId(), input.getOrder(), operation, f2);
 			});
 		}
@@ -654,7 +662,7 @@ private Stream<OrderedInputOperation> multiItemSelectorsStream() {
 				Function<MultiItemSelectorContext<String, SelectorItem<String>>, String> f1 = input.getNext();
 				Function<ComponentContext<?>, Optional<String>> f2 = context -> f1 != null
 						? Optional.ofNullable(f1.apply(selector.getThisContext(context)))
-						: Optional.empty();
+						: null;
 				return OrderedInputOperation.of(input.getId(), input.getOrder(), operation, f2);
 			});
 		}

spring-shell-core/src/main/java/org/springframework/shell/component/flow/ConfirmationInputSpec.java

@@ -105,7 +105,8 @@ public interface ConfirmationInputSpec extends BaseInputSpec<ConfirmationInputSp
 	ConfirmationInputSpec storeResult(boolean store);
 
 	/**
-	 * Define a function which may return id of a next component to go.
+	 * Define a function which may return id of a next component to go. Returning a
+	 * {@code null} or non existent id indicates that flow should stop.
 	 *
 	 * @param next next component function
 	 * @return a builder

spring-shell-core/src/main/java/org/springframework/shell/component/flow/MultiItemSelectorSpec.java

@@ -124,7 +124,8 @@ public interface MultiItemSelectorSpec extends BaseInputSpec<MultiItemSelectorSp
 	MultiItemSelectorSpec storeResult(boolean store);
 
 	/**
-	 * Define a function which may return id of a next component to go.
+	 * Define a function which may return id of a next component to go. Returning a
+	 * {@code null} or non existent id indicates that flow should stop.
 	 *
 	 * @param next next component function
 	 * @return a builder

spring-shell-core/src/main/java/org/springframework/shell/component/flow/PathInputSpec.java

@@ -107,7 +107,8 @@ public interface PathInputSpec extends BaseInputSpec<PathInputSpec> {
 	PathInputSpec storeResult(boolean store);
 
 	/**
-	 * Define a function which may return id of a next component to go.
+	 * Define a function which may return id of a next component to go. Returning a
+	 * {@code null} or non existent id indicates that flow should stop.
 	 *
 	 * @param next next component function
 	 * @return a builder

spring-shell-core/src/main/java/org/springframework/shell/component/flow/SingleItemSelectorSpec.java

@@ -142,7 +142,8 @@ public interface SingleItemSelectorSpec extends BaseInputSpec<SingleItemSelector
 	SingleItemSelectorSpec storeResult(boolean store);
 
 	/**
-	 * Define a function which may return id of a next component to go.
+	 * Define a function which may return id of a next component to go. Returning a
+	 * {@code null} or non existent id indicates that flow should stop.
 	 *
 	 * @param next next component function
 	 * @return a builder

spring-shell-core/src/main/java/org/springframework/shell/component/flow/StringInputSpec.java

@@ -114,7 +114,8 @@ public interface StringInputSpec extends BaseInputSpec<StringInputSpec> {
 	StringInputSpec storeResult(boolean store);
 
 	/**
-	 * Define a function which may return id of a next component to go.
+	 * Define a function which may return id of a next component to go. Returning a
+	 * {@code null} or non existent id indicates that flow should stop.
 	 *
 	 * @param next next component function
 	 * @return a builder

spring-shell-core/src/test/java/org/springframework/shell/component/flow/ComponentFlowTests.java

@@ -161,7 +161,7 @@ public void testSkipsGivenComponents() throws InterruptedException {
 		}
 
 	@Test
-	public void testChoosesDynamically() throws InterruptedException {
+	public void testChoosesDynamicallyShouldJumpOverAndStop() throws InterruptedException {
 		ComponentFlow wizard = ComponentFlow.builder()
 			.terminal(getTerminal())
 			.resourceLoader(getResourceLoader())
@@ -202,12 +202,106 @@ public void testChoosesDynamically() throws InterruptedException {
 		ComponentFlowResult inputWizardResult = result.get();
 		assertThat(inputWizardResult).isNotNull();
 		String id1 = inputWizardResult.getContext().get("id1");
-		// TODO: should be able to check if variable exists
-		// String id2 = inputWizardResult.getContext().get("id2");
 		String id3 = inputWizardResult.getContext().get("id3");
 		assertThat(id1).isEqualTo("id3");
-		// assertThat(id2).isNull();
 		assertThat(id3).isEqualTo("value3");
+		assertThat(inputWizardResult.getContext().containsKey("id2")).isFalse();
+	}
+
+	@Test
+	public void testChoosesDynamicallyShouldNotContinueToNext() throws InterruptedException {
+		ComponentFlow wizard = ComponentFlow.builder()
+			.terminal(getTerminal())
+			.resourceLoader(getResourceLoader())
+			.templateExecutor(getTemplateExecutor())
+			.withStringInput("id1")
+				.name("name")
+				.next(ctx -> ctx.get("id1"))
+				.and()
+			.withStringInput("id2")
+				.name("name")
+				.resultValue("value2")
+				.resultMode(ResultMode.ACCEPT)
+				.next(ctx -> null)
+				.and()
+			.withStringInput("id3")
+				.name("name")
+				.resultValue("value3")
+				.resultMode(ResultMode.ACCEPT)
+				.next(ctx -> null)
+				.and()
+			.build();
+
+		ExecutorService service = Executors.newFixedThreadPool(1);
+		CountDownLatch latch = new CountDownLatch(1);
+		AtomicReference<ComponentFlowResult> result = new AtomicReference<>();
+		service.execute(() -> {
+			result.set(wizard.run());
+			latch.countDown();
+		});
+
+		// id1
+		TestBuffer testBuffer = new TestBuffer().append("id2").cr();
+		write(testBuffer.getBytes());
+		// id2
+		testBuffer = new TestBuffer().cr();
+		write(testBuffer.getBytes());
+		latch.await(4, TimeUnit.SECONDS);
+		ComponentFlowResult inputWizardResult = result.get();
+		assertThat(inputWizardResult).isNotNull();
+		String id1 = inputWizardResult.getContext().get("id1");
+		String id2 = inputWizardResult.getContext().get("id2");
+		assertThat(id1).isEqualTo("id2");
+		assertThat(id2).isEqualTo("value2");
+		assertThat(inputWizardResult.getContext().containsKey("id3")).isFalse();
+	}
+
+	@Test
+	public void testChoosesNonExistingComponent() throws InterruptedException {
+		ComponentFlow wizard = ComponentFlow.builder()
+			.terminal(getTerminal())
+			.resourceLoader(getResourceLoader())
+			.templateExecutor(getTemplateExecutor())
+			.withStringInput("id1")
+				.name("name")
+				.next(ctx -> ctx.get("id1"))
+				.and()
+			.withStringInput("id2")
+				.name("name")
+				.resultValue("value2")
+				.resultMode(ResultMode.ACCEPT)
+				.next(ctx -> null)
+				.and()
+			.withStringInput("id3")
+				.name("name")
+				.resultValue("value3")
+				.resultMode(ResultMode.ACCEPT)
+				.next(ctx -> null)
+				.and()
+			.build();
+
+		ExecutorService service = Executors.newFixedThreadPool(1);
+		CountDownLatch latch = new CountDownLatch(1);
+		AtomicReference<ComponentFlowResult> result = new AtomicReference<>();
+		service.execute(() -> {
+			result.set(wizard.run());
+			latch.countDown();
+		});
+
+		// id1
+		TestBuffer testBuffer = new TestBuffer().append("fake").cr();
+		write(testBuffer.getBytes());
+
+		// don't execute id2 or id3
+		testBuffer = new TestBuffer().cr();
+		write(testBuffer.getBytes());
+		latch.await(4, TimeUnit.SECONDS);
+		ComponentFlowResult inputWizardResult = result.get();
+		assertThat(inputWizardResult).isNotNull();
+		String id1 = inputWizardResult.getContext().get("id1");
+		assertThat(id1).isEqualTo("fake");
+		assertThat(inputWizardResult.getContext().containsKey("id2")).isFalse();
+		assertThat(inputWizardResult.getContext().containsKey("id3")).isFalse();
 	}
 
 	@Test

spring-shell-docs/src/main/asciidoc/asciinema/component-flow-conditional-1.cast

@@ -21,9 +21,6 @@
 [12.734384, "o", "\u001b[?1l\u001b>\u001b[?12;25h\u001b[K"]
 [12.740754, "o", "\u001b[32;1m?\u001b[0m \u001b[97;1mField1\u001b[0m \u001b[34mdefaultField1Value\u001b[0m\r\n"]
 [12.74362, "o", "\u001b[?1h\u001b=\u001b[?25l"]
-[12.750944, "o", "\u001b[32;1m?\u001b[0m \u001b[97;1mField2\u001b[0m \u001b[34m[Default defaultField2Value]\u001b[0m\r"]
-[13.542429, "o", "\u001b[?1l\u001b>\u001b[?12;25h\u001b[K"]
-[13.545672, "o", "\u001b[32;1m?\u001b[0m \u001b[97;1mField2\u001b[0m \u001b[34mdefaultField2Value\u001b[0m\r\n"]
 [13.547149, "o", "\u001b[?1h\u001b=\u001b[?2004h\u001b[33mmy-shell:>\u001b[0m"]
 [14.091367, "o", "\u001b[1mflow conditional\u001b[0m"]
 [14.419251, "o", "\r\r\n\u001b[?1l\u001b>\u001b[?1000l\u001b[?2004l"]