8321480: ISO 4217 Amendment 176 Update #17023

justin-curtis-lu · 2023-12-07T19:43:14Z

Please review this PR which incorporates the ISO 4217 Amendment 176 Update. As the replacement of ANG to XCG won't occur until 2025, this change does not need to go into JDK22. HR was also updated to remove the past cutover dates.

This update also demonstrated that Currency::getAvailableCurrencies may return a future currency that should not be returned. GenerateCurrencyData.java was updated to ensure such currencies are not added to the other currency list.

Additionally, this change also converted a parameterized test to a normal JUnit test, due to output overflow.

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

Issue

JDK-8321480: ISO 4217 Amendment 176 Update (Bug - P3)

Reviewers

Naoto Sato (@naotoj - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17023

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-12-07T19:44:22Z

👋 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 · 2023-12-07T19:46:33Z

@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 · 2023-12-07T19:50:49Z

Webrevs

naotoj · 2023-12-07T22:20:29Z

src/java.base/share/classes/sun/util/resources/CurrencyNames.properties

@@ -494,6 +494,7 @@ xbb=European Monetary Unit
 xbc=European Unit of Account (XBC)
 xbd=European Unit of Account (XBD)
 xcd=East Caribbean Dollar
+xcg=Caribbean Guilder


I think XCG=XCG is also needed for not throwing MissingResourceException for getSymbol()

Thanks for the correction, added in.

naotoj · 2023-12-07T22:21:41Z

test/jdk/java/util/Currency/ValidateISO4217.java

+                } else {
+                    // Still need to add the future currency to testCurrencies
+                    // without updating ISO4217Codes
+                    String futureCurr = tokens.nextToken();
+                    testCurrencies.add(Currency.getInstance(futureCurr));


I'd not add the future currency, and fix it in the code not to add future currency in available currencies.

Updated the Currency build process to disallow any Currencies that are future Currencies. This prevents the future currency, "XCG" from leaking out into Currency.getAvailableCurrencies(). (This was only exposed now, since the previous future Currencies were already ones expected to be found in Currency.getAvailableCurrencies(). For example, Amendment 174 where HRK was replaced by EUR.

(Tiers 1-5 clean with this change)

IIUC, this should have happened before, as a currency update like "XXA" -> "XXB" on a future date is pretty usual. I think this issue was simply not discovered by any users till now.

I think this wasn't discovered/hasn't failed the test before because the amendments with future date changes most of the time were implemented after the date already occurred, so there was no cut-over, simply a new currency directly replacing the old (For example, ISO 171 SLE replacing SLL was direct). In the case where the cut-over hadn't happened, like 174 where EUR replaced HRK, EUR is already expected to be returned by Currency.getAvailableCurrencies() so there was no issue.

I claimed that this particular amendment exposed it, because if this case had happened before with an amendment update, it would have caused validateISO4217.java to fail (as it does with this case). Since that test checks if the set returned by the method does not contain any future currencies.

Yes, it is rare but happened before. The test had failed with the cut-over date in the past. Possibly at that time, getAvailableCurrencies test may not have been there so it wasn't obvious when the modification was made.

naotoj · 2023-12-07T22:22:30Z

test/jdk/java/util/Currency/ValidateISO4217.java

+    @Test
+    public void twoLetterCodesTest() {
+        for (String country : codeCombos()) {
+            if (codes[toIndex(country)] == UNDEFINED) {
+                // if a code is undefined / 0, creating a Currency from it
+                // should throw an IllegalArgumentException
+                assertThrows(IllegalArgumentException.class,
+                        () -> Currency.getInstance(Locale.of("", country)),
+                        "Error: This should be an undefined code and throw IllegalArgumentException: " + country);
+            } else if (codes[toIndex(country)] == SKIPPED) {
+                // if a code is marked as skipped / 2, creating a Currency from it
+                // should return null
+                assertNull(Currency.getInstance(Locale.of("", country)),
+                        "Error: Currency.getInstance() for this locale should return null: " + country);
+            }


What is this change for?

The previous output was overflowing due to the method being a @ParameterizedTest since it was testing against 26*26 inputs. Changing to @Test makes diagnostics easier, as there is no more overflow.

naotoj · 2023-12-11T19:38:38Z

make/jdk/src/classes/build/tools/generatecurrencydata/GenerateCurrencyData.java

+            // Do not allow a future currency to be classified as an otherCurrency,
+            // otherwise it will leak out into Currency:getAvailableCurrencies
+            boolean notFutureCurrency = !Arrays.asList(specialCaseNewCurrencies).contains(currencyCode);
+            boolean notSimpleCurrency = tableEntry == INVALID_COUNTRY_ENTRY


I'd rather not bundle up those conditions into notSimpleCurrency, as it obscures the other two cases. Simply OR-ing those conditions and notFutureCurrency ( or !futureCurrency may be more readable) would be sufficient IMO.

Well, at first I had intended for the notSimpleCurrency to encapsulate all 3 cases. But upon further investigation, I think the current code is redundant.

The third conditional, (tableEntry & SIMPLE_CASE_COUNTRY_FINAL_CHAR_MASK) != (currencyCode.charAt(2) - 'A') checks if the currency's first two chars match the country code (which also implies the current currency is simple). It returns false if they match. When this is false, the other first two conditions are always false. If a currencyCode is simple, the countryCode is always defined, and the currencyCode itself is not special. Checking the first two AND the third seems redundant when the first two are always false if the third is false.

For example, take the currency ADP which will search for the table entry of the country AD. Since AD=EUR, the third conditional returns true. We now know that we do not have a simple currency, which is more than enough to make a decision whether to add the current currency to the otherCurrency List. But the existing code now checks that AD is a defined country and whether EUR is special or not (since AD's tableEntry value is associated with EUR). I'm not sure why we would check that EUR is a special currency, when we are deciding to add ADP as an otherCurrency. It is hard to infer without any existing comments as well.

With commit 85f389d, the same exact values are added to the otherCurrency list. However, If we would prefer not to update such old code for the sake of safety, then I would at least like to add comments that warn which conditionals may be irrelevant. Wondering what your thoughts are regarding this.

Can you compare the built binary currency.data before and after your change? If they are identical, I think we can go ahead and remove the redundancy in the tool.

Yes, I will go ahead and compare, as well as do a few more sanity checks

naotoj

LGTM

openjdk · 2023-12-12T19:40:08Z

@justin-curtis-lu This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8321480: ISO 4217 Amendment 176 Update

Reviewed-by: naoto

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 21 new commits pushed to the master branch:

aadf368: 6230751: [Fmt-Ch] Recursive MessageFormats in ChoiceFormats ignore indicated subformats
a3447ec: 8321827: Remove unnecessary suppress warnings annotations from the printing processor
b25ed57: 8270269: Desktop.browse method fails if earlier CoInitialize call as COINIT_MULTITHREADED
df4ed7e: 8321739: Source launcher fails with "Not a directory" error
5718039: 8321542: C2: Missing ChaCha20 stub for x86_32 leads to crashes
c516852: 8321889: JavaDoc method references with wrong (nested) type
7d90396: 8321422: Test gc/g1/pinnedobjs/TestPinnedObjectTypes.java times out after completion
6f48240: 8321729: Remove 'orb' field in RMIConnector
e1fd663: 8311306: Test com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java failed: out of expected range
d5214a4: 8321814: G1: Remove unused G1RemSetScanState::_collection_set_iter_state
... and 11 more: https://git.openjdk.org/jdk/compare/ce108446ca1fe604ecc24bbefb0bf1c6318271c7...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

justin-curtis-lu · 2023-12-14T21:14:48Z

/integrate

openjdk · 2023-12-14T21:16:21Z

Going to push as commit 8b24851.
Since your change was applied there have been 45 commits pushed to the master branch:

c328f95: 8296787: Unify debug printing format of X.509 cert serial numbers
fde5b16: 8321514: UTF16 string gets constructed incorrectly from codepoints if CompactStrings is not enabled
45a9ade: 8202598: keytool -certreq output contains inconsistent line separators
62b7c5e: 8319647: Few java/lang/System/LoggerFinder/modules tests ignore vm flags
69014cd: 8320682: [AArch64] C1 compilation fails with "Field too big for insn"
5a97dbf: 8322034: Parallel: Remove unused methods in PSAdaptiveSizePolicy
2838a91: 8288989: Make tests not depend on the source code
d2ba3b1: 8312150: Remove -Xnoagent option
d632d74: 8321820: TestLoadNIdeal fails on 32-bit because -XX:+UseCompressedOops is not recognized
ddbbd36: 8320279: Link issues in java.xml module-info.java
... and 35 more: https://git.openjdk.org/jdk/compare/ce108446ca1fe604ecc24bbefb0bf1c6318271c7...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-12-14T21:16:27Z

@justin-curtis-lu Pushed as commit 8b24851.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

init

249fbc7

openjdk bot added the rfr Pull request is ready for review label Dec 7, 2023

openjdk bot added core-libs core-libs-dev@openjdk.org i18n i18n-dev@openjdk.org labels Dec 7, 2023

justin-curtis-lu marked this pull request as draft December 7, 2023 22:23

openjdk bot removed the rfr Pull request is ready for review label Dec 7, 2023

naotoj reviewed Dec 7, 2023

View reviewed changes

add xcg to CurrencyNames

6fa5c0e

justin-curtis-lu marked this pull request as ready for review December 11, 2023 01:56

openjdk bot added the rfr Pull request is ready for review label Dec 11, 2023

justin-curtis-lu added 3 commits December 10, 2023 18:28

reflect review comments

54525c7

add comment

878a6cc

Merge branch 'master' into 8321480-ISO4217-176

94f5870

naotoj reviewed Dec 11, 2023

View reviewed changes

remove redundant conditionals

85f389d

naotoj approved these changes Dec 12, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 12, 2023

openjdk bot added the integrated Pull request has been integrated label Dec 14, 2023

openjdk bot closed this Dec 14, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8321480: ISO 4217 Amendment 176 Update #17023

8321480: ISO 4217 Amendment 176 Update #17023

justin-curtis-lu commented Dec 7, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 7, 2023

openjdk bot commented Dec 7, 2023

mlbridge bot commented Dec 7, 2023 •

edited

Loading

naotoj Dec 7, 2023

justin-curtis-lu Dec 11, 2023

naotoj Dec 7, 2023

justin-curtis-lu Dec 11, 2023 •

edited

Loading

naotoj Dec 11, 2023

justin-curtis-lu Dec 11, 2023 •

edited

Loading

naotoj Dec 11, 2023

naotoj Dec 7, 2023

justin-curtis-lu Dec 11, 2023

naotoj Dec 11, 2023

justin-curtis-lu Dec 11, 2023 •

edited

Loading

naotoj Dec 11, 2023

justin-curtis-lu Dec 12, 2023

naotoj left a comment

openjdk bot commented Dec 12, 2023

justin-curtis-lu commented Dec 14, 2023

openjdk bot commented Dec 14, 2023

openjdk bot commented Dec 14, 2023

8321480: ISO 4217 Amendment 176 Update #17023

8321480: ISO 4217 Amendment 176 Update #17023

Conversation

justin-curtis-lu commented Dec 7, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Dec 7, 2023

openjdk bot commented Dec 7, 2023

mlbridge bot commented Dec 7, 2023 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justin-curtis-lu Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justin-curtis-lu Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justin-curtis-lu Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naotoj left a comment

Choose a reason for hiding this comment

openjdk bot commented Dec 12, 2023

justin-curtis-lu commented Dec 14, 2023

openjdk bot commented Dec 14, 2023

openjdk bot commented Dec 14, 2023

justin-curtis-lu commented Dec 7, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 7, 2023 •

edited

Loading

justin-curtis-lu Dec 11, 2023 •

edited

Loading

justin-curtis-lu Dec 11, 2023 •

edited

Loading

justin-curtis-lu Dec 11, 2023 •

edited

Loading