Drop the symbol separator in the currency and percent re-parse - #430
Drop the symbol separator in the currency and percent re-parse#430sahvx655-wq wants to merge 1 commit into
Conversation
The lenient fallback rebuilt the pattern without the symbol but kept the space next to it, so the separator stayed mandatory for suffix-symbol locales.
garydgregory
changed the title
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes lenient re-parse behavior for percent/currency patterns where the symbol is suffixed and separated by a (non‑breaking) space, ensuring validators don’t incorrectly reject valid inputs or accept a stray separator as a substitute for the missing symbol.
Changes:
- Update
CurrencyValidator.parseandPercentValidator.parseto remove the space adjacent to the symbol when rebuilding the fallback pattern. - Add regression tests covering suffix-symbol patterns with a non-breaking space separator for both currency and percent validators.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/main/java/org/apache/commons/validator/routines/PercentValidator.java | Drops adjacent space chars when stripping % for the lenient retry pattern. |
| src/main/java/org/apache/commons/validator/routines/CurrencyValidator.java | Drops adjacent space chars when stripping ¤ for the lenient retry pattern. |
| src/test/java/org/apache/commons/validator/routines/PercentValidatorTest.java | Adds regression test for suffix percent symbol separated by NBSP. |
| src/test/java/org/apache/commons/validator/routines/CurrencyValidatorTest.java | Adds regression test for suffix currency symbol separated by NBSP. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Tests whether the character at the given position of a pattern sits next to the currency symbol. | ||
| * | ||
| * @param pattern The pattern being stripped of its currency symbol. | ||
| * @param index The position to test. | ||
| * @return {@code true} if the character borders the currency symbol. | ||
| */ | ||
| private static boolean isNextToSymbol(final String pattern, final int index) { | ||
| return index > 0 && pattern.charAt(index - 1) == CURRENCY_SYMBOL | ||
| || index + 1 < pattern.length() && pattern.charAt(index + 1) == CURRENCY_SYMBOL; | ||
| } |
| void testSuffixSymbolPattern() { | ||
| final BigDecimalValidator validator = PercentValidator.getInstance(); |
| void testSuffixSymbolPattern() { | ||
| final BigDecimalValidator validator = CurrencyValidator.getInstance(); |
garydgregory
left a comment
garydgregory
left a comment
There was a problem hiding this comment.
@sahvx655-wq
Thank you for the PR.
Please review my comments as well as the copilot comments.
| } | ||
|
|
||
| /** | ||
| * Test percentage values with a pattern that suffixes the symbol, which locales such as fr-FR separate from the number with a non-breaking space. The |
There was a problem hiding this comment.
The comment make a claim about FR and the code about US. Add a test for each, or parameterize the current test for both, better yet, for all known currency symbols.
| } | ||
|
|
||
| /** | ||
| * Test currency values with a pattern that suffixes the symbol, which locales such as de-DE separate from the number with a non-breaking space. The symbol |
There was a problem hiding this comment.
The comment make a claim about DE and the code about US. Add a test for each, or parameterize the current test for both, better yet, for all known currency symbols.
Came at this from the other end: the German and French number validators were rejecting amounts the British one accepted, and the parse trace showed both the initial attempt and the lenient retry failing at the same index, which puts the fault in the fallback pattern rather than in the input.
CurrencyValidator.parseandPercentValidator.parserebuild the pattern with the symbol removed and re-parse, but they delete only the symbol character itself.NumberFormat.getCurrencyInstance(Locale.GERMANY).toPattern()is#,##0.00 ¤andgetPercentInstance(Locale.FRANCE).toPattern()is#,##0 %, so what survives the rebuild is a mandatory non-breaking-space suffix.validate("1.234,56", Locale.GERMANY)andvalidate("12", Locale.FRANCE)come back null, while the prefix-symbol locales such as en-GB are fine.validate("1.234,56 ", Locale.GERMANY)yields 1234.56 andvalidate("12 ", Locale.FRANCE)yields 0.12 with no symbol present at all.Both methods now drop the space adjacent to the symbol along with it. Left alone, the behaviour cuts both ways for every suffix-symbol locale (de, fr, it and the rest): correctly formatted amounts are silently refused, and a stray separator passes as a substitute for the currency or percent sign, which is a poor property in something callers lean on to reject malformed money. The two regression tests build an explicit suffix pattern instead of reading the JDK's locale data, so they will not drift with CLDR updates; both fail on master and pass with the change.
Thanks for your contribution to Apache Commons! Your help is appreciated!
Before you push a pull request, review this list:
mvn; that'smvnon the command line by itself.