Repository navigation
fix: align unicode case folding with String.regionMatches - #1302
hanzhe-one wants to merge 3 commits into
Conversation
Mikkey-f
left a comment
There was a problem hiding this comment.
Thanks @hanzhe-one — I verified the bug and the fix locally (Windows 11, JDK 17).
Root cause: The fallback lowercased the original characters, while String.regionMatches lowercases the uppercased forms. For İ (U+0130) and ı (U+0131) the uppercase forms differ (İ vs I) and the direct lowercase forms differ (i vs ı), but toLowerCase(toUpperCase(...)) yields i for both — so the String fast path returned true and the StringBuilder path returned false.
Verification
- Red: with only
StringUtilsreverted (the new test kept),test_regionMatches_unicodeIgnoreCasefails withexpected: <true> but was: <false>. - Green: the test passes;
StringUtilsTest9/9. - Full suite: 61 + 1,789 tests, 0 failures;
spotless:checkclean.
Code review: The new comparison is byte-for-byte the JDK's second-stage check, and the fast path, parameter validation, and NPE behavior are untouched. I also confirmed no production caller passes ignoreCase=true today (the only in-repo caller is StringUtils.equals, case-sensitive), so this aligns the public helper's contract with the JDK without changing a spreadsheet code path.
One nit (non-blocking): The contract is "same result as String.regionMatches"; the test currently hard-codes expected values. Comparing against ((String) cs).regionMatches(...) for the same inputs would pin the equivalence itself.
LGTM — minimal change that matches the JDK, with both directions covered.
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are reported for the Unicode case-folding consistency fix.
No open issues 🎉
What changed in this PR
Fixes #1301 by aligning the non-String CharSequence case-folding fallback with String.regionMatches for dotted and dotless Unicode I characters.
Changes:
- Compare lowercase forms of the uppercase characters, preserving the String fast path and existing parameter handling.
- Add regression coverage for String and StringBuilder combinations in both directions, including case-sensitive controls.
| File | Description |
|---|---|
fesod-common/src/test/java/org/apache/fesod/common/util/StringUtilsTest.java |
Tests Unicode comparison consistency across input types and directions. |
fesod-common/src/main/java/org/apache/fesod/common/util/StringUtils.java |
Corrects second-stage case folding in the fallback. |
Review effort: Balanced
Purpose of the pull request
Closed: #1301
StringUtils.regionMatchesdelegates toString.regionMatcheswhen both values are Strings, but uses a manual fallback for otherCharSequencetypes. The fallback compared the uppercase forms, then lowercased the original characters. Forİ(U+0130) andı(U+0131) that differs from the JDK: the uppercase forms differ and the directly lowercased forms differ, but lowercasing the uppercase results yieldsifor both. As a result, the String path returnedtruewhile the StringBuilder path returnedfalse.What's changed?
Validation:
./mvnw -B -pl fesod-common test: 61 tests pass, including the new regression test../mvnw -B spotless:check: passes.No production caller currently uses
ignoreCase=true, so this fixes the public utility's contract consistency rather than an observed spreadsheet data-loss path.Opening as a draft while the approach on #1301 is still awaiting maintainer confirmation.
Checklist