Flow order priority does not match documented behavior

[robertmcnees]

Bug description

When using a JobExecutionDecider the behavior is not accurate according to the documentation of DefaultStateTransitionComparator. Specifically

Hence * > foo* > ??? > fo? > foo

Additionally, the actual behavior when running a Job is not consistent with the test results found in DefaultStateTransitionComparatorTests

In almost all test scenarios the documented behavior is not followed.

Environment

Boot 3.1.4
Java 17

Steps to reproduce

See sample GitHub project to be linked in comments below. The JUnit tests have multiple failing scenarios that should pass.

The tests come in 3 different groups, all of which contain failing tests:

1. Tests based solely on the example found in the documentation
2. Tests based on the scenarios in DefaultStateTransitionComparator
3. Tests created from interpretation of the documentation

Expected behavior

The stated documentation should accurate in regards to the priority Flow. All tests in the sample project should pass.

[robertmcnees]

Sample GitHub Repository

Test Cases

[hpoettker]

Hi @robertmcnees, in your tests you seem to assume that Spring Batch will jump to the most generic pattern if there is more than one match. This is not the case. Spring Batch will choose the most specific one.

Documented behavior of Spring Batch

The precedence of transitions is described in the reference documentation: https://docs.spring.io/spring-batch/docs/current/reference/html/step.html#conditionalFlow

It gives the following example

@Bean
public Job job(JobRepository jobRepository) {
    return new JobBuilder("job", jobRepository)
     .start(stepA())
     .on("*").to(stepB())
     .from(stepA()).on("FAILED").to(stepC())
     .end()
     .build();
}

and states

The framework automatically orders transitions from most specific to least specific. This means that, even if the ordering were swapped for stepA in the preceding example, an ExitStatus of FAILED would still go to stepC.

The implementation

The DefaultStateTransitionComparator provides an order on patterns such that specific patterns are ranked lower than generic patterns, i.e. foo < fo? < ??? < foo* < *. The order ??? < foo* holds because any * is considered to be more generic than any amount of ?.

In SimpleFlow, Spring Batch organizes the possible transitions in a TreeSet, whose order is defined by DefaultStateTransitionComparator. When it has to choose a transition, it will then iterate over the set from most specific to most generic, and it will pick the first transition that matches. This leads to the most specific match being chosen.

It seems to me that the Javadoc of DefaultStateTransitionComparator, the reference documentation, and the implemented behavior are in sync.

[robertmcnees]

Thanks for the detailed reply. We are in agreement on the pattern matching priority but I am missing something in the implementation details. Let's focus on the example ??? < foo* or similarly in the documentation foo* > ???. I agree in this example we would expect foo* to be prioritized over ???.

However this test fails. In this example, ??? is given priority over foo*. Is the bug in my test?

[hpoettker]

According to the logic of DefaultStateTransitionComparator, it is actually ??? that is more specific than foo*. It considers any appearance of * to be more generic than any amount of ?, no matter where they are placed in the pattern.

The merit of this can be debated but it is (maybe too briefly) stated in the Javadoc:

Sorts by decreasing specificity of pattern, based on just counting wildcards (with * taking precedence over ?).

[hpoettker]

For the sake of completeness: The Javadoc of DefaultStateTransitionComparator does contain an error. It claims that foo* is more specific than *, i.e. foo* < *, which would make sense, but as the two strings have the same number of *s they are compared as strings, which actually yields * < foo*.

This is the root cause of #3996, which mentions the interesting workaround to use **, where * is not generic enough. The workaround uses that * < foo* < ** as more *s are interpreted as being more generic.

[robertmcnees]

Thank you @hpoettker for the explanation. I was investigating #3996 and when my unit tests didn't pass it lead me to create this issue instead. Your comments helped to straighten out my thinking and indeed there is no bug in the code, although I do believe the documentation to be incorrect.

There were 2 issues that caused me confusion:

First, I believe the documentation text doesn't match the example. The example provided * > foo* > ??? > fo? > foo is sorted from most generic to most specific, or sorted in ascending order of specificity. The tests confirm that the examples are correct.

Second, the order of specificity switches between the DefaultStateTransitionComparator and what is used in the FlowJobBuilder. That is while DefaultStateTransitionComparator will return * > foo* > ??? > fo? > foo, the FlowJobBuilder will use the most specific case first. This turns the example backwards to foo > fo? > ??? > foo* > *. This was the mistake I made in my test suite. I was using the example from DefaultStateTransitionComparator to make assumptions how FlowJobBuilder would behave. This explains why all of my tests were completely backwards.

To illustrate the switch in ordering (more for my own sake), I created a new passing test for the JobFlowBuilder to illustrate foo > foo*.

	@Test
	void testBuildWithDeciderPrioritySubstringAndWildcard() {
		JobExecutionDecider decider = (jobExecution, stepExecution) -> new FlowExecutionStatus("CONTINUABLE");
		JobFlowBuilder builder = new JobBuilder("flow_priority", jobRepository).start(decider);
		builder.on("CONTINUABLE").end();
		builder.on("CONTIN*").fail();
		builder.build().preventRestart().build().execute(execution);
		assertEquals(BatchStatus.COMPLETED, execution.getStatus());
	}

Compare this to the existing test from DefaultStateTransitionComparatorTests that illustrates foo* > foo:

    void testSubstringAndWildcard() {
		StateTransition transition = StateTransition.createStateTransition(state, "CONTIN*", "start");
		StateTransition other = StateTransition.createStateTransition(state, "CONTINUABLE", "start");
		assertEquals(1, comparator.compare(transition, other));
		assertEquals(-1, comparator.compare(other, transition));
	}

This is a lot of text for a proposed one word documentation update. I needed to straighten it out in my head. As long as I'm not too far off I'll likely head to #3996 next.