On Thu, 4 Feb 2021 20:01:10 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
> Fixes a mutability issue for `LocalDateTimeStringConverter` (and > `LocalDateStringConverter`) where the chronology can change during the > lifetime of the instance and cause an inconsistent state. The following > changes were made: > > * The input is now formatted and parsed directly with the formatter and > parser (which are configured with a chronology) without the chronology field > value of the class. > * The chronology field value does not change anymore when there is an > exception due to inability to format the date. > * Logging on failed formatting was removed as it did not seem useful. The > formatter will throw the same exception that the chronology field does > anyway, so there is not much use to catching the exception before hand. > > Added a test that fails without this patch. The test creates a converter with > an explicit `Chronology` and `null` parser and formatter. The default > formatter that is created with the given chronology formats a legal date > (with respect to the chronology) to a string, which the parser should be able > to convert back to a date. However, by forcing an exception in the formatting > process using an illegal date, the chronology changes, and now when the > parser is used it is configured with the new chronology and it can't parse > the string correctly. The fix looks good to me, as does the test with a couple suggestions. modules/javafx.base/src/test/java/test/javafx/util/converter/LocalDateTimeStringConverterTest.java line 167: > 165: // force a chronology change with an invalid Japanese date > 166: try { > 167: System.out.println(converter.toString(LocalDateTime.of(1, 1, > 1, 1, 1, 1))); Since you are expecting an exception, it seems a good idea to fail if you didn't get one. Something like: fail("Did not get the expected exception"); Also, you might consider narrowing the `catch` to just the exception that you expect, so that an unexpected exception will also fail the test. ------------- PR: https://git.openjdk.java.net/jfx/pull/393