Reject non-integer strings in integer locale converters

[rootvector2]

The integer locale converters IntegerLocaleConverter, LongLocaleConverter, ShortLocaleConverter, ByteLocaleConverter and BigIntegerLocaleConverter parse with a DecimalFormat that accepts a fractional part, then narrow the result with intValue()/longValue()/byteValue()/toBigInteger(). Their only guard checks that the value fits the target range, so 5.5 slips through and is silently truncated to 5 instead of rejected. Found by diffing them against the non-locale IntegerConverter, which rejects 5.5 through Integer.valueOf.

Each parse now rejects a non-integer result before narrowing, and BigIntegerLocaleConverter uses toBigIntegerExact. Grouping separators and large in-range integers still convert. The stale (B) assertions that pinned the old truncated values now expect the default.

• Read the contribution guidelines for this project.
• Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
• I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute?
• Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
• Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.

[garydgregory]

Hi @rootvector2

Hm, my initial reaction was: This might break a bunch of existing call sites that rely on mapping a "5.5" input to 5.

Then looking at the Javadoc of, for example, IntegerLocaleConverter, we read:

/**
 * Standard {@link org.apache.commons.beanutils2.locale.LocaleConverter} implementation that converts an incoming locale-sensitive String into a {@link Integer}
 * object, optionally using a default value or throwing a {@link org.apache.commons.beanutils2.ConversionException} if a conversion error occurs.
 */
public class IntegerLocaleConverter extends DecimalLocaleConverter<Integer> {

Specifically: "optionally using a default value or throwing a ConversionException if a conversion error occurs."

Which begs the question, when do you map a conversion error to the default value and when do you map it to throwing a ConversionException?

Do you have any thoughts here? We have some leeway for changing behavior and APIs in master since 2.0 isn't out yet but a 1.X version of this PR would need to be least surprising.

So focusing on master, and looking at org.apache.commons.beanutils2.locale.BaseLocaleConverter, I'm starting to think that the use of default values is pretty narrow.

WDYT?

[rootvector2]

The split is decided at construction time, not per error. Every builder sets useDefault || defaultValue != null, and BaseLocaleConverter.convert catches anything parse throws: with useDefault it returns the default, otherwise it rethrows as ConversionException. So it's all-or-nothing per instance, and I agree it's narrow: the default only kicks in on null input or a parse failure.

That's also why this patch doesn't introduce a new policy. It reclassifies a fractional result as a parse failure and lets the existing dispatch decide. A converter built with a default keeps returning the default (the (B) assertions and testNonIntegerRejected pin that), and only converters built without one start throwing. Call sites relying on "5.5" mapping to 5 were already getting the default for "abc" or out-of-range input, so fractions just join the errors that were already handled that way.

On 1.X I agree least surprise wins. #407 landed the out-of-range rejection there, but truncating fractions is much older behavior, so I'd hold off on a 1.X port unless you want one.

[garydgregory]

Thanks @rootvector2 , merged 🚀 It seems reasonable to port to 1.X since check for out-of-range in master, I'm not sure how far the code's drifted since the 1.x vs. master split though.

[rootvector2]

thanks for merging. the 1.x side hasn't drifted much, its parse still narrows with intValue() behind a range-only guard, so 5.5 truncates the same way there. the port should be about as mechanical as #407, I'll put one up.