RFR: 8260468: Wrong behavior of LocalDateTimeStringConverter
Nir Lisker
nlisker at openjdk.java.net
Thu Feb 4 20:10:00 UTC 2021
On Thu, 4 Feb 2021 20:01:10 GMT, Nir Lisker <nlisker at 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 added test will look meaningless after the patch, since it's trying to force a change that can't happen anymore, and this can be confusing in the future. Does it still make sense to commit it? It's more of a demonstration of the bug and the fix.
I also noticed that other tests in `LocalDateTimeStringConverterTest` are not named with `test` in the beginning. Is this wrong?
-------------
PR: https://git.openjdk.java.net/jfx/pull/393
More information about the openjfx-dev
mailing list