Skip to content

Modified calculating order for DefaultStateTransitionComparator #4509

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Modified calculating order for DefaultStateTransitionComparator #4509

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 45 additions & 14 deletions ...ava/org/springframework/batch/core/job/flow/support/DefaultStateTransitionComparator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,31 @@
import java.util.Comparator;

/**
* Sorts by decreasing specificity of pattern, based on just counting wildcards (with *
* taking precedence over ?). If wildcard counts are equal then falls back to alphabetic
* comparison. Hence * > foo* > ??? > fo? > foo.
* Sorts by ascending specificity of pattern, based on counting wildcards (with * taking
* precedence over ?). Hence * > foo* > ??? > fo? > foo.
Comment on lines +23 to +24
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is true that this example sorts the patterns from most generic to most specific (ie in ascending order of specificity), but the comparator itself returns the most specific pattern first and the least specific pattern last. This means the specificity is decreasing, not increasing. I know this is subtle, but I think the initial javadoc Sorts by decreasing specificity of pattern is correct (which I seem to have missed when I reviewed #4504). Do you agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you said this is subtle so I'm OK with leaving it as is. I've been confused by this many times and I think the more important part is the example in the documentation than the word ascending/descending.

However to detail my thinking for anyone who stumbles on this later:

the comparator itself returns the most specific pattern first

The way I read it the comparator considers the more generic patterns to be greater than the more specific. So the comparator would return * > ?foo > foo, which I would call ascending order of specificity (most specific last).

This was the confusing part for me (quoting myself from a separate issue):

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* > *.

In short I think the DefaultStateTransitionComparator sorts in ascending order of specificity, but FlowJobBuilder sorts in descending order of specificity. FlowJobBuilder and descending order of specificity is what the end user interacts with, so maybe it's best to keep the documentation as 'descending' throughout.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the clarification, @robertmcnees ! I see, my interpretation of the natural ordering of elements (ie the meaning/definition of the > symbol) in the DefaultStateTransitionComparator was inaccurate.

In short I think the DefaultStateTransitionComparator sorts in ascending order of specificity, but FlowJobBuilder sorts in descending order of specificity.

I believe this is the root cause of the confusion. There is probably a good reason for that (probably the use of TreeSet in SimpleFlow?), but my thinking is that since we want transitions to be sorted by the most specific first (like in the issue's example foo* should be fired before *), then the comparator should result in foo* being lower than *. Similar to the natural ordering of numbers, the analogy I like to compare to is that if I assign weights to transitions (like foo* -> 1 and * -> 2), the "natural" ordering coming out of the comparator itself would rank foo* lower than * (that's the "natural" order I was expecting, not the opposite).

It would be great if we try to avoid the ordering to be opposite between the comparator and the flow implementation. This is important because as a Batch user, if I want to provide a custom comparator, I would not have to do this mental effort to think backward in the comparator's implementation. Otherwise, we can keep the changes in this PR and add a clear definition of what ">" means in this context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great point about someone wanting to create their own comparator. I was only thinking about it from my narrow perspective of debugging the code. I think making the DefaultStateTransitionComparator match FlowJobBuilder makes even more sense with that consideration.

I created a new issue #4527 so that we can track the solution there. It will likely involve changes that are out of scope to this PR and issue #3996.

*
* For more complex comparisons, any string containing at least one * token will be
* considered more generic than any string that has no * token. If both strings have at
* least one * token, then the string with fewer * tokens will be considered the most
* generic. If both strings have the same number of * tokens, then the comparison will
* fall back to length of the overall string with the shortest value being the most
* generic. Finally, if the * token count is equal and the string length is equal then the
* final comparison will be alphabetic.
*
* When two strings have ? tokens, then the string with the most ? tokens will be
* considered the most generic. If both strings have the same number of ? tokens, then the
* comparison will fall back to length of the overall string with the shortest value being
* the most generic. Finally, if the ? token count is equal and the string length is equal
* then the final comparison will be alphabetic
*
* If the strings contain neither * nor ? tokens then alphabetic comparison will be used.
*
* Hence * > foo* > *f* > *foo* > ??? > ?o? > foo?? > bar?? > fo?
* > foo > bar
*
* @see Comparator
* @author Michael Minella
* @author Robert McNees
* @since 3.0
*/
public class DefaultStateTransitionComparator implements Comparator<StateTransition> {
Expand All @@ -39,27 +58,39 @@ public class DefaultStateTransitionComparator implements Comparator<StateTransit
*/
@Override
public int compare(StateTransition arg0, StateTransition arg1) {
String value = arg1.getPattern();
if (arg0.getPattern().equals(value)) {
String arg0Pattern = arg0.getPattern();
String arg1Pattern = arg1.getPattern();
if (arg0.getPattern().equals(arg1Pattern)) {
return 0;
}
int patternCount = StringUtils.countOccurrencesOf(arg0.getPattern(), "*");
int valueCount = StringUtils.countOccurrencesOf(value, "*");
if (patternCount > valueCount) {
int arg0AsteriskCount = StringUtils.countOccurrencesOf(arg0Pattern, "*");
int arg1AsteriskCount = StringUtils.countOccurrencesOf(arg1Pattern, "*");
if (arg0AsteriskCount > 0 && arg1AsteriskCount == 0) {
return 1;
}
if (patternCount < valueCount) {
if (arg0AsteriskCount == 0 && arg1AsteriskCount > 0) {
return -1;
}
patternCount = StringUtils.countOccurrencesOf(arg0.getPattern(), "?");
valueCount = StringUtils.countOccurrencesOf(value, "?");
if (patternCount > valueCount) {
if (arg0AsteriskCount > 0 && arg1AsteriskCount > 0) {
if (arg0AsteriskCount < arg1AsteriskCount) {
return 1;
}
if (arg0AsteriskCount > arg1AsteriskCount) {
return -1;
}
}
int arg0WildcardCount = StringUtils.countOccurrencesOf(arg0Pattern, "?");
int arg1WildcardCount = StringUtils.countOccurrencesOf(arg1Pattern, "?");
if (arg0WildcardCount > arg1WildcardCount) {
return 1;
}
if (patternCount < valueCount) {
if (arg0WildcardCount < arg1WildcardCount) {
return -1;
}
return arg0.getPattern().compareTo(value);
if (arg0Pattern.length() != arg1Pattern.length() && (arg0AsteriskCount > 0 || arg0WildcardCount > 0)) {
return Integer.compare(arg1Pattern.length(), arg0Pattern.length());
}
return arg0.getPattern().compareTo(arg1Pattern);
}

}
20 changes: 20 additions & 0 deletions ...ch-core/src/test/java/org/springframework/batch/core/job/builder/FlowJobBuilderTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,26 @@ public FlowExecutionStatus decide(JobExecution jobExecution, @Nullable StepExecu
assertEquals(1, execution.getStepExecutions().size());
}

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

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

@Test
void testBuildWithIntermediateSimpleJob() {
SimpleJobBuilder builder = new JobBuilder("flow", jobRepository).start(step1);
Expand Down
96 changes: 76 additions & 20 deletions ...rg/springframework/batch/core/job/flow/support/DefaultStateTransitionComparatorTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,42 +37,98 @@ void testSimpleOrderingEqual() {

@Test
void testSimpleOrderingMoreGeneral() {
StateTransition transition = StateTransition.createStateTransition(state, "CONTIN???LE", "start");
StateTransition other = StateTransition.createStateTransition(state, "CONTINUABLE", "start");
assertEquals(1, comparator.compare(transition, other));
assertEquals(-1, comparator.compare(other, transition));
StateTransition generic = StateTransition.createStateTransition(state, "CONTIN???LE", "start");
StateTransition specific = StateTransition.createStateTransition(state, "CONTINUABLE", "start");
Comment on lines +40 to +41
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this! A simple naming change makes the tests immediately more readable.

assertEquals(1, comparator.compare(generic, specific));
assertEquals(-1, comparator.compare(specific, generic));
}

@Test
void testSimpleOrderingMostGeneral() {
StateTransition transition = StateTransition.createStateTransition(state, "*", "start");
StateTransition other = StateTransition.createStateTransition(state, "CONTINUABLE", "start");
assertEquals(1, comparator.compare(transition, other));
assertEquals(-1, comparator.compare(other, transition));
StateTransition generic = StateTransition.createStateTransition(state, "*", "start");
StateTransition specific = StateTransition.createStateTransition(state, "CONTINUABLE", "start");
assertEquals(1, comparator.compare(generic, specific));
assertEquals(-1, comparator.compare(specific, generic));
}

@Test
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));
StateTransition generic = StateTransition.createStateTransition(state, "CONTIN*", "start");
StateTransition specific = StateTransition.createStateTransition(state, "CONTINUABLE", "start");
assertEquals(1, comparator.compare(generic, specific));
assertEquals(-1, comparator.compare(specific, generic));
}

@Test
void testSimpleOrderingMostToNextGeneral() {
StateTransition transition = StateTransition.createStateTransition(state, "*", "start");
StateTransition other = StateTransition.createStateTransition(state, "C?", "start");
assertEquals(1, comparator.compare(transition, other));
assertEquals(-1, comparator.compare(other, transition));
StateTransition generic = StateTransition.createStateTransition(state, "*", "start");
StateTransition specific = StateTransition.createStateTransition(state, "C?", "start");
assertEquals(1, comparator.compare(generic, specific));
assertEquals(-1, comparator.compare(specific, generic));
}

@Test
void testSimpleOrderingAdjacent() {
StateTransition transition = StateTransition.createStateTransition(state, "CON*", "start");
StateTransition other = StateTransition.createStateTransition(state, "CON?", "start");
assertEquals(1, comparator.compare(transition, other));
assertEquals(-1, comparator.compare(other, transition));
StateTransition generic = StateTransition.createStateTransition(state, "CON*", "start");
StateTransition specific = StateTransition.createStateTransition(state, "CON?", "start");
assertEquals(1, comparator.compare(generic, specific));
assertEquals(-1, comparator.compare(specific, generic));
}

@Test
void testOrderByNumberOfGenericWildcards() {
StateTransition generic = StateTransition.createStateTransition(state, "*", "start");
StateTransition specific = StateTransition.createStateTransition(state, "**", "start");
assertEquals(1, comparator.compare(generic, specific));
assertEquals(-1, comparator.compare(specific, generic));
}

@Test
void testOrderByNumberOfSpecificWildcards() {
StateTransition generic = StateTransition.createStateTransition(state, "CONTI??ABLE", "start");
StateTransition specific = StateTransition.createStateTransition(state, "CONTI?UABLE", "start");
assertEquals(1, comparator.compare(generic, specific));
assertEquals(-1, comparator.compare(specific, generic));
}

@Test
void testOrderByLengthWithAsteriskEquality() {
StateTransition generic = StateTransition.createStateTransition(state, "CON*", "start");
StateTransition specific = StateTransition.createStateTransition(state, "CONTINUABLE*", "start");
assertEquals(1, comparator.compare(generic, specific));
assertEquals(-1, comparator.compare(specific, generic));
}

@Test
void testOrderByLengthWithWildcardEquality() {
StateTransition generic = StateTransition.createStateTransition(state, "CON??", "start");
StateTransition specific = StateTransition.createStateTransition(state, "CONTINUABLE??", "start");
assertEquals(1, comparator.compare(generic, specific));
assertEquals(-1, comparator.compare(specific, generic));
}

@Test
void testOrderByAlphaWithAsteriskEquality() {
StateTransition generic = StateTransition.createStateTransition(state, "DOG**", "start");
StateTransition specific = StateTransition.createStateTransition(state, "CAT**", "start");
assertEquals(1, comparator.compare(generic, specific));
assertEquals(-1, comparator.compare(specific, generic));
}

@Test
void testOrderByAlphaWithWildcardEquality() {
StateTransition generic = StateTransition.createStateTransition(state, "DOG??", "start");
StateTransition specific = StateTransition.createStateTransition(state, "CAT??", "start");
assertEquals(1, comparator.compare(generic, specific));
assertEquals(-1, comparator.compare(specific, generic));
}

@Test
void testPriorityOrderingWithAlphabeticComparison() {
StateTransition generic = StateTransition.createStateTransition(state, "DOG", "start");
StateTransition specific = StateTransition.createStateTransition(state, "CAT", "start");
assertEquals(1, comparator.compare(generic, specific));
assertEquals(-1, comparator.compare(specific, generic));
}

}