8387261: Locale.LanguageRange should reject Double.NaN

[justin-curtis-lu]

This PR addresses an edge case where the Locale.LanguageRange(String, double) constructor accepts Double.NaN as a weight. A LanguageRange weight is specified by https://datatracker.ietf.org/doc/html/rfc2616#section-3.9 and must be between 0.0 and 1.0, inclusive. The existing bounds checks do not handle this case. This change adds an explicit Double.isNaN(weight) check.

The issue does not affect parsed range strings in the same way, since the input is normalized to lower case before parsing and "nan" is not accepted by Double.parseDouble. However, I added a test for that case as well, since one did not previously exist.

---

• I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change requires CSR request JDK-8387368 to be approved

Issues

• JDK-8387261: Locale.LanguageRange should reject Double.NaN (Bug - P4)
• JDK-8387368: Locale.LanguageRange should reject Double.NaN (CSR)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/31697/head:pull/31697
$ git checkout pull/31697

Update a local copy of the PR:
$ git checkout pull/31697
$ git pull https://git.openjdk.org/jdk.git pull/31697/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 31697

View PR using the GUI difftool:
$ git pr show -t 31697

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/31697.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jlu! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@justin-curtis-lu The following labels will be automatically applied to this pull request:

• core-libs
• i18n

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (62c11e60)
• 00: Full (7136743a)

[naotoj]

I think the changes are good.

[naotoj]

Since the constructor now throws IllegalArgumentException for NaN weights, this should also be specified in the @throws tag, as NaN does not satisfy either NaN < MIN_WEIGHT or NaN > MAX_WEIGHT.

[justin-curtis-lu]

How about updating the @throws wording to be

... or if the given {@code weight} is not between {@code MIN_WEIGHT} and {@code MAX_WEIGHT}, inclusive.

which would cover the NaN case without having to explicitly call it out.

[naotoj]

That wording is technically fine, but I would still prefer to call out NaN explicitly. NaN is unordered, so it is a different case from an ordinary out-of-range value, and it is the specific edge case being fixed here.

How about:

... or if the given {@code weight} is {@code NaN}, less than {@code MIN_WEIGHT}, or greater than {@code MAX_WEIGHT}.

[jddarcy]

Does any spec update need to be done to accommodate -0.0?

(It compares equal to +0.0 should likely "just works.")

[naotoj]

Does any spec update need to be done to accommodate -0.0?

(It compares equal to +0.0 should likely "just works.")

I don't think a spec update is needed for -0.0. Since MIN_WEIGHT is 0.0, -0.0 compares equal to the inclusive lower bound and does not satisfy weight < MIN_WEIGHT, so it remains accepted.

[justin-curtis-lu]

Yes, for the constructor, I believe we are covered on the -0.0 case with the current throws wording. What is more questionable are the parse methods that accept the weight within the range string.

These methods are specified to throw if the weight is not a RFC 2616 qvalue,

qvalue = ( "0" [ "." 03DIGIT ] )
| ( "1" [ "." 03("0") ] )

However, since the implementation relies on Double.parseDouble, values such as the -0.0 case as well as +0.0, 0.0e0, and 0.0000 are all technically not valid qvalues, yet are accepted. I don't think there is much benefit in adding validation on the weights to match that exact syntax structure.

We may need to soften the specification’s reliance on RFC 2616, perhaps by defining accepted weights in terms of the RFC 2616 minimum and maximum quality weight boundaries, rather than saying they exactly match RFC 2616 quality values.

We could also take a different approach and clarify via an @implNote that this implementation is more lenient as it uses Double.parseDouble.

I am not sure if either of you would have a preference.

[naotoj]

Yes, for the constructor, I believe we are covered on the -0.0 case with the current throws wording. What is more questionable are the parse methods that accept the weight within the range string.

These methods are specified to throw if the weight is not a RFC 2616 qvalue,

qvalue = ( "0" [ "." 0_3DIGIT ] )
| ( "1" [ "." 0_3("0") ] )

However, since the implementation relies on Double.parseDouble, values such as the -0.0 case as well as +0.0, 0.0e0, and 0.0000 are all technically not valid qvalues, yet are accepted. I don't think there is much benefit in adding validation on the weights to match that exact syntax structure.

We may need to soften the specification’s reliance on RFC 2616, perhaps by defining accepted weights in terms of the RFC 2616 minimum and maximum quality weight boundaries, rather than saying they exactly match RFC 2616 quality values.

We could also take a different approach and clarify via an @implNote that this implementation is more lenient as it uses Double.parseDouble.

I am not sure if either of you would have a preference.

I'd prefer the latter, since some implementation might want to strictly follow the RFC2616 spec. So we can say the JDK's implementation allows more than three fraction digits, or scientific notations as an @implNote.

btw, I wrote:

I don't think a spec update is needed for -0.0. Since MIN_WEIGHT is 0.0, -0.0 compares equal to the inclusive lower bound and does not satisfy weight < MIN_WEIGHT, so it remains accepted

Not RFC 2616, which is now obsolete, but the latest RFC 9110 reads:

The weight is normalized to a real number in the range 0 through 1, where 0.001 is the least preferred and 1 is the most preferred; a value of 0 means "not acceptable". If no "q" parameter is present, the default weight is 1.

So the value of 0 is "not acceptable" in their terminology 🙂