BATCH-1285: Raise an exception if a step cannot be reached.

spring-batch-core/src/main/java/org/springframework/batch/core/configuration/xml/FlowParser.java

@@ -16,8 +16,13 @@
 package org.springframework.batch.core.configuration.xml;
 
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
+import java.util.Set;
 
 import org.springframework.batch.core.job.flow.FlowExecutionStatus;
 import org.springframework.batch.core.job.flow.support.SimpleFlow;
@@ -39,6 +44,8 @@ import org.w3c.dom.NodeList;
  */
 public class FlowParser extends AbstractSingleBeanDefinitionParser {
 
+	private static final String ID_ATTR = "id";
+
 	private static final String STEP_ELE = "step";
 
 	private static final String DECISION_ELE = "decision";
@@ -80,7 +87,7 @@ public class FlowParser extends AbstractSingleBeanDefinitionParser {
 	 * 
 	 * @param flowName the name of the flow
 	 * @param jobFactoryRef the reference to the {@link JobParserJobFactoryBean}
-	 *        from the enclosing tag
+	 * from the enclosing tag
 	 */
 	public FlowParser(String flowName, String jobFactoryRef) {
 		this.flowName = flowName;
@@ -112,23 +119,34 @@ public class FlowParser extends AbstractSingleBeanDefinitionParser {
 		parserContext.pushContainingComponent(compositeDef);
 
 		boolean stepExists = false;
+		Map<String, Set<String>> reachableElementMap = new HashMap<String, Set<String>>();
+		String startElement = null;
 		NodeList children = element.getChildNodes();
 		for (int i = 0; i < children.getLength(); i++) {
 			Node node = children.item(i);
 			if (node instanceof Element) {
 				String nodeName = node.getLocalName();
+				Element child = (Element) node;
 				if (nodeName.equals(STEP_ELE)) {
-					stateTransitions.addAll(stepParser.parse((Element) node, parserContext, jobFactoryRef));
+					stateTransitions.addAll(stepParser.parse(child, parserContext, jobFactoryRef));
 					stepExists = true;
 				}
 				else if (nodeName.equals(DECISION_ELE)) {
-					stateTransitions.addAll(decisionParser.parse((Element) node, parserContext));
+					stateTransitions.addAll(decisionParser.parse(child, parserContext));
 				}
 				else if (nodeName.equals(SPLIT_ELE)) {
-					stateTransitions.addAll(splitParser.parse((Element) node, new ParserContext(parserContext
-							.getReaderContext(), parserContext.getDelegate(), builder.getBeanDefinition())));
+					stateTransitions.addAll(splitParser
+							.parse(child, new ParserContext(parserContext.getReaderContext(), parserContext
+									.getDelegate(), builder.getBeanDefinition())));
 					stepExists = true;
 				}
+
+				if (Arrays.asList(STEP_ELE, DECISION_ELE, SPLIT_ELE).contains(nodeName)) {
+					reachableElementMap.put(child.getAttribute(ID_ATTR), findReachableElements(child));
+					if (startElement == null) {
+						startElement = child.getAttribute(ID_ATTR);
+					}
+				}
 			}
 		}
 
@@ -137,6 +155,15 @@ public class FlowParser extends AbstractSingleBeanDefinitionParser {
 					element);
 		}
 
+		// Ensure that all elements are reachable
+		Set<String> allReachableElements = new HashSet<String>();
+		findAllReachableElements(startElement, reachableElementMap, allReachableElements);
+		for (String elementId : reachableElementMap.keySet()) {
+			if (!allReachableElements.contains(elementId)) {
+				parserContext.getReaderContext().error("The element [" + elementId + "] is unreachable", element);
+			}
+		}
+
 		builder.addConstructorArgValue(flowName);
 		ManagedList managedList = new ManagedList();
 		@SuppressWarnings( { "unchecked", "unused" })
@@ -149,13 +176,65 @@ public class FlowParser extends AbstractSingleBeanDefinitionParser {
 
 	}
 
+	/**
+	 * Find all of the elements that are pointed to by this element.
+	 * 
+	 * @param element
+	 * @return a collection of reachable element names
+	 */
+	private Set<String> findReachableElements(Element element) {
+		Set<String> reachableElements = new HashSet<String>();
+
+		String nextAttribute = element.getAttribute(NEXT_ATTR);
+		if (StringUtils.hasText(nextAttribute)) {
+			reachableElements.add(nextAttribute);
+		}
+
+		@SuppressWarnings("unchecked")
+		List<Element> nextElements = (List<Element>) DomUtils.getChildElementsByTagName(element, NEXT_ELE);
+		for (Element nextElement : nextElements) {
+			String toAttribute = nextElement.getAttribute(TO_ATTR);
+			reachableElements.add(toAttribute);
+		}
+
+		@SuppressWarnings("unchecked")
+		List<Element> stopElements = (List<Element>) DomUtils.getChildElementsByTagName(element, STOP_ELE);
+		for (Element stopElement : stopElements) {
+			String restartAttribute = stopElement.getAttribute(RESTART_ATTR);
+			reachableElements.add(restartAttribute);
+		}
+
+		return reachableElements;
+	}
+
+	/**
+	 * Find all of the elements reachable from the startElement.
+	 * 
+	 * @param startElement
+	 * @param reachableElementMap
+	 * @param accumulator a collection of reachable element names
+	 */
+	private void findAllReachableElements(String startElement, Map<String, Set<String>> reachableElementMap,
+			Set<String> accumulator) {
+		Set<String> reachableIds = reachableElementMap.get(startElement);
+		accumulator.add(startElement);
+		if (reachableIds != null) {
+			for (String reachable : reachableIds) {
+				// don't explore a previously explored element; prevent loop
+				if (!accumulator.contains(reachable)) {
+					findAllReachableElements(reachable, reachableElementMap, accumulator);
+				}
+			}
+		}
+	}
+
 	/**
 	 * @param parserContext the parser context for the bean factory
 	 * @param stateDef The bean definition for the current state
 	 * @param element the &lt;step/gt; element to parse
 	 * @return a collection of
-	 *         {@link org.springframework.batch.core.job.flow.support.StateTransition}
-	 *         references
+	 * {@link org.springframework.batch.core.job.flow.support.StateTransition}
+	 * references
 	 */
 	protected static Collection<BeanDefinition> getNextElements(ParserContext parserContext, BeanDefinition stateDef,
 			Element element) {
@@ -165,12 +244,12 @@ public class FlowParser extends AbstractSingleBeanDefinitionParser {
 	/**
 	 * @param parserContext the parser context for the bean factory
 	 * @param stepId the id of the current state if it is a step state, null
-	 *        otherwise
+	 * otherwise
 	 * @param stateDef The bean definition for the current state
 	 * @param element the &lt;step/gt; element to parse
 	 * @return a collection of
-	 *         {@link org.springframework.batch.core.job.flow.support.StateTransition}
-	 *         references
+	 * {@link org.springframework.batch.core.job.flow.support.StateTransition}
+	 * references
 	 */
 	protected static Collection<BeanDefinition> getNextElements(ParserContext parserContext, String stepId,
 			BeanDefinition stateDef, Element element) {
@@ -234,8 +313,8 @@ public class FlowParser extends AbstractSingleBeanDefinitionParser {
 	 * @param stateDef The bean definition for the current state
 	 * @param parserContext the parser context for the bean factory
 	 * @param a collection of
-	 *        {@link org.springframework.batch.core.job.flow.support.StateTransition}
-	 *        references
+	 * {@link org.springframework.batch.core.job.flow.support.StateTransition}
+	 * references
 	 */
 	private static Collection<BeanDefinition> parseTransitionElement(Element transitionElement, String stateId,
 			BeanDefinition stateDef, ParserContext parserContext) {
@@ -255,18 +334,18 @@ public class FlowParser extends AbstractSingleBeanDefinitionParser {
 
 	/**
 	 * @param status The batch status that this transition will set. Use
-	 *        BatchStatus.UNKNOWN if not applicable.
+	 * BatchStatus.UNKNOWN if not applicable.
 	 * @param on The pattern that this transition should match. Use null for
-	 *        "no restriction" (same as "*").
+	 * "no restriction" (same as "*").
 	 * @param next The state to which this transition should go. Use null if not
-	 *        applicable.
+	 * applicable.
 	 * @param exitCode The exit code that this transition will set. Use null to
-	 *        default to batchStatus.
+	 * default to batchStatus.
 	 * @param stateDef The bean definition for the current state
 	 * @param parserContext the parser context for the bean factory
 	 * @param a collection of
-	 *        {@link org.springframework.batch.core.job.flow.support.StateTransition}
-	 *        references
+	 * {@link org.springframework.batch.core.job.flow.support.StateTransition}
+	 * references
 	 */
 	private static Collection<BeanDefinition> createTransition(FlowExecutionStatus status, String on, String next,
 			String exitCode, BeanDefinition stateDef, ParserContext parserContext, boolean abandon) {
@@ -336,7 +415,7 @@ public class FlowParser extends AbstractSingleBeanDefinitionParser {
 	 * @param on the pattern value
 	 * @param next the next step id
 	 * @return a bean definition for a
-	 *         {@link org.springframework.batch.core.job.flow.support.StateTransition}
+	 * {@link org.springframework.batch.core.job.flow.support.StateTransition}
 	 */
 	public static BeanDefinition getStateTransitionReference(ParserContext parserContext,
 			BeanDefinition stateDefinition, String on, String next) {

spring-batch-core/src/test/java/org/springframework/batch/core/configuration/xml/JobParserTests.java

@@ -17,10 +17,12 @@ package org.springframework.batch.core.configuration.xml;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 import java.util.ArrayList;
 import java.util.List;
 
+import org.junit.BeforeClass;
 import org.junit.Test;
 import org.springframework.aop.framework.Advised;
 import org.springframework.batch.core.Job;
@@ -29,6 +31,7 @@ import org.springframework.batch.core.job.AbstractJob;
 import org.springframework.batch.core.listener.JobExecutionListenerSupport;
 import org.springframework.batch.core.repository.JobRepository;
 import org.springframework.batch.core.repository.support.SimpleJobRepository;
+import org.springframework.beans.factory.parsing.BeanDefinitionParsingException;
 import org.springframework.context.ApplicationContext;
 import org.springframework.context.ConfigurableApplicationContext;
 import org.springframework.context.support.ClassPathXmlApplicationContext;
@@ -40,8 +43,13 @@ import org.springframework.test.util.ReflectionTestUtils;
  */
 public class JobParserTests {
 
-	ConfigurableApplicationContext jobParserParentAttributeTestsCtx = new ClassPathXmlApplicationContext(
-			"org/springframework/batch/core/configuration/xml/JobParserParentAttributeTests-context.xml");
+	private static ConfigurableApplicationContext jobParserParentAttributeTestsCtx;
+
+	@BeforeClass
+	public static void loadAppCtx() {
+		jobParserParentAttributeTestsCtx = new ClassPathXmlApplicationContext(
+				"org/springframework/batch/core/configuration/xml/JobParserParentAttributeTests-context.xml");
+	}
 
 	@Test
 	public void testInheritListeners() throws Exception {
@@ -136,4 +144,28 @@ public class JobParserTests {
 		assertTrue(jobRepository instanceof JobRepository);
 		return (JobRepository) jobRepository;
 	}
+
+	@Test
+	public void testUnreachableStep() {
+		try {
+			new ClassPathXmlApplicationContext(
+					"org/springframework/batch/core/configuration/xml/JobParserUnreachableStepTests-context.xml");
+			fail("Error expected");
+		}
+		catch (BeanDefinitionParsingException e) {
+			assertTrue(e.getMessage().contains("The element [s2] is unreachable"));
+		}
+	}
+
+	@Test
+	public void testNextOutOfScope() {
+		try {
+			new ClassPathXmlApplicationContext(
+					"org/springframework/batch/core/configuration/xml/JobParserNextOutOfScopeTests-context.xml");
+			fail("Error expected");
+		}
+		catch (BeanDefinitionParsingException e) {
+			assertTrue(e.getMessage().contains("Missing state for [StateTransition: [state=s2, pattern=*, next=s3]]"));
+		}
+	}
 }

spring-batch-core/src/test/java/org/springframework/batch/core/configuration/xml/StepParserTests.java

@@ -21,6 +21,7 @@ import static org.junit.Assert.assertTrue;
 import java.util.List;
 import java.util.Map;
 
+import org.junit.BeforeClass;
 import org.junit.Test;
 import org.springframework.aop.framework.Advised;
 import org.springframework.batch.core.Step;
@@ -52,8 +53,13 @@ import org.springframework.transaction.interceptor.DefaultTransactionAttribute;
  */
 public class StepParserTests {
 
-	private static final ApplicationContext stepParserParentAttributeTestsCtx = new ClassPathXmlApplicationContext(
-			"org/springframework/batch/core/configuration/xml/StepParserParentAttributeTests-context.xml");
+	private static ApplicationContext stepParserParentAttributeTestsCtx;
+
+	@BeforeClass
+	public static void loadAppCtx() {
+		stepParserParentAttributeTestsCtx = new ClassPathXmlApplicationContext(
+				"org/springframework/batch/core/configuration/xml/StepParserParentAttributeTests-context.xml");
+	}
 
 	@SuppressWarnings("unchecked")
 	@Test

spring-batch-core/src/test/resources/org/springframework/batch/core/configuration/xml/ChunkElementParentAttributeParserTests-context.xml

@@ -7,7 +7,7 @@
 	<beans:import resource="common-context.xml" />
 
 	<job id="job">
-		<step id="s1" parent="baseStep">
+		<step id="s1" parent="baseStep" next="s2">
 			<tasklet>
 				<chunk reader="reader" writer="writer" commit-interval="5" skip-limit="5">
 					<skippable-exception-classes merge="true">

spring-batch-core/src/test/resources/org/springframework/batch/core/configuration/xml/JobParserNextOutOfScopeTests-context.xml

@@ -0,0 +1,21 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<beans:beans xmlns="http://www.springframework.org/schema/batch" xmlns:beans="http://www.springframework.org/schema/beans"
+	xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+	xsi:schemaLocation="http://www.springframework.org/schema/batch http://www.springframework.org/schema/batch/spring-batch-2.0.xsd
+	http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans-2.5.xsd">
+
+	<beans:import resource="common-context.xml" />
+
+	<job id="job1">
+		<step id="s1" parent="step1" next="split1"/>
+		<split id="split1">
+			<flow>
+				<step id="s2" parent="step2" next="s3"/>
+			</flow>
+			<flow>
+				<step id="s3" parent="step3"/>
+			</flow>
+		</split>
+	</job>
+
+</beans:beans>

spring-batch-core/src/test/resources/org/springframework/batch/core/configuration/xml/JobParserUnreachableStepTests-context.xml

@@ -0,0 +1,15 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<beans:beans xmlns="http://www.springframework.org/schema/batch" xmlns:beans="http://www.springframework.org/schema/beans"
+	xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+	xsi:schemaLocation="http://www.springframework.org/schema/batch http://www.springframework.org/schema/batch/spring-batch-2.0.xsd
+	http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans-2.5.xsd">
+
+	<beans:import resource="common-context.xml" />
+
+	<job id="job1">
+		<step id="s1" parent="step1"/>
+		<step id="s2" parent="step2"/>
+		<step id="s3" parent="step3"/>
+	</job>
+
+</beans:beans>

spring-batch-core/src/test/resources/org/springframework/batch/core/configuration/xml/StepListenerParserTests-context.xml

@@ -7,7 +7,7 @@
 	<beans:import resource="common-context.xml" />
 	
 	<job id="job">
-		<step id="s1" parent="baseStep">
+		<step id="s1" parent="baseStep" next="s2">
 			<tasklet ref="dummyTasklet">
 				<listeners merge="true">
 					<listener class="org.springframework.batch.core.configuration.xml.DummyAnnotationStepExecutionListener"/>

spring-batch-core/src/test/resources/org/springframework/batch/core/configuration/xml/StepParserParentAttributeTests-context.xml

@@ -35,11 +35,11 @@
 		<step id="s8" parent="standalone8" next="s9" />
 		<step id="s9" parent="standalone9" next="s10" />
 		<step id="s10" parent="standalone10" next="s11"/>
-		<step id="s11" parent="dummyStep"/>
-		<step id="s12" parent="dummyStep">
+		<step id="s11" parent="dummyStep" next="s12"/>
+		<step id="s12" parent="dummyStep" next="s13">
 			<tasklet ref="dummyTasklet"/>
 		</step>
-		<step id="s13" parent="dummyStepWithTaskletOnParent"/>
+		<step id="s13" parent="dummyStepWithTaskletOnParent" next="s14"/>
 		<step id="s14" parent="standaloneStepWithTaskletAndDummyParent"/>
 	</job>
 	
@@ -104,20 +104,20 @@
 	</step>
 
 	<job id="jobWithoutRepo">
-		<step id="defaultRepoStep"><tasklet ref="dummyTasklet"/></step>
-		<step id="defaultRepoStepWithParent" parent="defaultRepoStandaloneStep"><tasklet ref="dummyTasklet"/></step>
+		<step id="defaultRepoStep" next="defaultRepoStepWithParent"><tasklet ref="dummyTasklet"/></step>
+		<step id="defaultRepoStepWithParent" parent="defaultRepoStandaloneStep" next="overrideRepoStep"><tasklet ref="dummyTasklet"/></step>
 		<step id="overrideRepoStep" parent="specifiedRepoStandaloneStep"><tasklet ref="dummyTasklet"/></step>
 	</job>
 
 	<job id="jobWithRepo" job-repository="dummyJobRepository">
-		<step id="injectedRepoStep"><tasklet ref="dummyTasklet"/></step>
-		<step id="injectedRepoStepWithParent" parent="defaultRepoStandaloneStep"><tasklet ref="dummyTasklet"/></step>
+		<step id="injectedRepoStep" next="injectedRepoStepWithParent"><tasklet ref="dummyTasklet"/></step>
+		<step id="injectedRepoStepWithParent" parent="defaultRepoStandaloneStep" next="injectedOverrideRepoStep"><tasklet ref="dummyTasklet"/></step>
 		<step id="injectedOverrideRepoStep" parent="specifiedRepoStandaloneStep"><tasklet ref="dummyTasklet"/></step>
 	</job>
 	
 	<job id="jobWithRepoOnParent" parent="baseJobWithRepo">
-		<step id="injectedRepoFromParentStep"><tasklet ref="dummyTasklet"/></step>
-		<step id="injectedRepoFromParentStepWithParent" parent="defaultRepoStandaloneStep"><tasklet ref="dummyTasklet"/></step>
+		<step id="injectedRepoFromParentStep" next="injectedRepoFromParentStepWithParent"><tasklet ref="dummyTasklet"/></step>
+		<step id="injectedRepoFromParentStepWithParent" parent="defaultRepoStandaloneStep" next="injectedOverrideRepoFromParentStep"><tasklet ref="dummyTasklet"/></step>
 		<step id="injectedOverrideRepoFromParentStep" parent="specifiedRepoStandaloneStep"><tasklet ref="dummyTasklet"/></step>
 	</job>
 
@@ -135,9 +135,9 @@
 	<beans:bean id="dummyJobRepository2" class="org.springframework.batch.core.configuration.xml.DummyJobRepository"/>
 
 	<job id="defaultTxMgrTestJob">
-		<step id="defaultTxMgrStep"><tasklet ref="dummyTasklet"/></step>
-		<step id="specifiedTxMgrStep"><tasklet ref="dummyTasklet" transaction-manager="dummyTxMgr"/></step>
-		<step id="defaultTxMgrWithParentStep" parent="specifiedRepoStandaloneStep"/>
+		<step id="defaultTxMgrStep" next="specifiedTxMgrStep"><tasklet ref="dummyTasklet"/></step>
+		<step id="specifiedTxMgrStep" next="defaultTxMgrWithParentStep"><tasklet ref="dummyTasklet" transaction-manager="dummyTxMgr"/></step>
+		<step id="defaultTxMgrWithParentStep" parent="specifiedRepoStandaloneStep" next="overrideTxMgrOnParentStep"/>
 		<step id="overrideTxMgrOnParentStep" parent="specifiedRepoStandaloneStep"><tasklet transaction-manager="dummyTxMgr2"/></step>
 	</job>