Skip to content

reject NaN and infinity in double and float validators#384

Open
sahvx655-wq wants to merge 1 commit into
apache:masterfrom
sahvx655-wq:double-float-reject-nan
Open

reject NaN and infinity in double and float validators#384
sahvx655-wq wants to merge 1 commit into
apache:masterfrom
sahvx655-wq:double-float-reject-nan

Conversation

@sahvx655-wq
Copy link
Copy Markdown
Contributor

reading the number validators, validate of double and float passes the parsed value straight through processParsedValue without checking it is finite. NumberFormat recognises the locale NaN and infinity symbols, so validate("NaN"), validate("∞") and validate("-∞") return NaN/infinity and isValid returns true. a NaN that validates then silently defeats every isInRange/minValue/maxValue check, since NaN compares false against everything. reject non-finite results in both validators.

@garydgregory
Copy link
Copy Markdown
Member

@sahvx655-wq

Did you read the Javadoc?

This looks like you or your AI are imagining requirements that don't exist or that I can't find.

For example, "NaN" is a valid value to parse with a NumberFormat (which we use):

jshell> import java.text.*;
jshell> NumberFormat.getInstance().parse("NaN");
$3 ==> NaN

But it's not the same for Double.POSITIVE_INFINITY:

jshell> NumberFormat.getInstance().parse("" + Double.POSITIVE_INFINITY)
|  Exception java.text.ParseException: Unparseable number: "+Infinity"
|        at NumberFormat.parse (NumberFormat.java:434)
|        at (#2:1)

So there is a discrepancy...

Aside from the issue that the PR this may break existing applications, how do you propose an app use this class to validate, for example, config file values, or user input where in fact, a Double value like Double.NaN is expected?

FYI, I updated tests for edge case tests in FloatValidatorTest and DoubleValidatorTest to reflect the current code.

TY!

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.

2 participants