Skip to content

Fix FlowRuleComparator violating the Comparator contract#3629

Open
vasiliy-mikhailov wants to merge 1 commit into
alibaba:1.8from
vasiliy-mikhailov:fix/flowrulecomparator-contract
Open

Fix FlowRuleComparator violating the Comparator contract#3629
vasiliy-mikhailov wants to merge 1 commit into
alibaba:1.8from
vasiliy-mikhailov:fix/flowrulecomparator-contract

Conversation

@vasiliy-mikhailov

Copy link
Copy Markdown

Problem

FlowRuleComparator.compare returns 0 (equal) for rules that are not actually equal:

if (o1.getLimitApp() == null) {
    return 0;                       // returns 0 even when o2.getLimitApp() is non-null
}
...
} else {
    return 0;                       // two different non-default limitApps compare as equal
}

So a rule whose limitApp is null compares equal to any other rule, and two rules with different non-default limitApp values (e.g. originA vs originB) compare equal. Returning 0 for unequal elements violates the Comparator contract and can cause rules to be dropped when stored in a sorted structure.

Fix

  • Return 0 only when both limitApps are null; otherwise order a null consistently against a non-null value.
  • Compare two different non-default limitApps with String.compareTo instead of returning 0.

Test

Adds testNullVsNonNullLimitApp and testDifferentNonDefaultLimitApps to FlowRuleComparatorTest, each also checking antisymmetry. Both fail on 1.8 (compare returns 0 for unequal rules) and pass with this change.

FlowRuleComparator.compare returned 0 (equal) for rules that are not equal:

- when one limitApp is null and the other is not (the early
  o1.getLimitApp() == null check returned 0 regardless of o2), and
- when both limitApps are non-null, non-default and different (the final
  else returned 0).

Returning 0 for unequal elements breaks the Comparator contract and can drop
rules when they are stored in a sorted structure. Handle the null cases
explicitly and compare two different non-default limitApps with
String.compareTo.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant