8381644: Locale matching APIs handle null inputs inconsistently

[justin-curtis-lu]

This PR standardizes the null related behavior for the Locale filtering and lookup APIs.

There are two issues. First, null elements within either Collection which are passed to these APIs may cause NPE, but this is not specified. Second, if the tags or locales list is null and priorityList empty, NPE is not thrown (even though it is specified to).

For example, this is the current null related behavior for Locale:lookup(List<LanguageRange>, Collection<Locale>) (where all cases should throw NPE),

[], null -> NPE not thrown
[en], null
null, []
null, [en]
[null], [en]
[null], [] -> NPE not thrown
[], [null] -> NPE not thrown
[en], [null]

This update ensures that when either Collection is null, or any elements within are null, NPE is always thrown. That is, the empty edge case does not mask the NPE. The fix for null elements is done in the empty case conditional within LocaleMatcher, since the standard cases naturally encounter NPE. The specification is updated to fill in the missing behavior. As this also creates a behavioral change (to match the expected API contract), a CSR is filed.

---

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-8381863 to be approved

Issues

• JDK-8381644: Locale matching APIs handle null inputs inconsistently (Bug - P4)
• JDK-8381863: Locale matching APIs handle null inputs inconsistently (CSR)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 30632

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/30632.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

• 00: Full (a97621b7)

[naotoj]

The fix for null elements is done in the empty case conditional within LocaleMatcher, since the standard cases naturally encounter NPE

What if tags contains null element, but matching element enumerates earlier in the Collection? Looks like the implementation simply returns when the matching one comes first in the loop, not caring the null in the rest of the collection.

[justin-curtis-lu]

What if tags contains null element, but matching element enumerates earlier in the Collection? Looks like the implementation simply returns when the matching one comes first in the loop, not caring the null in the rest of the collection.

@naotoj You're right. I wanted to keep the checks out of the standard-path case if possible. But it looks like to ensure this behavior for an early match (and possible subsequent null elements later), we will need to validate all cases up-front. One alternative is weakening the specified NullPointerException to "may throw" for null elements within the collections, if the up-front performance cost is a concern.

[naotoj]

One alternative is weakening the specified NullPointerException to "may throw" for null elements within the collections, if the up-front performance cost is a concern.

Yeah, this seems more accommodating. We could explicitly state in the spec that early matching is allowed.

[justin-curtis-lu]

@naotoj Something to note is that for all APIs, the priority list is eagerly iterated on, so no null elements can be hidden there. The locale collection accepting APIs eagerly convert locales to tags as well, so the problem only arises for tags. The filter and filterTags APIs also traverse the entire tag list during scanning. Thus this early return issue lies only in lookupTag. So there is another question of whether we want to use this "weaker" specification for all the APIs, or just lookupTag.

[naotoj]

Or what if we "accept" null elements, which would be simply ignored? That would avoid NPE crash.