Re: RFR: 8260468: Wrong behavior of LocalDateTimeStringConverter [v2]
On Wed, 10 Feb 2021 02:24:57 GMT, Nir Lisker 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. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Removed printing and narrowed the exception looks ok to me - Marked as reviewed by pbansal (Committer). PR: https://git.openjdk.java.net/jfx/pull/393
Re: RFR: 8260468: Wrong behavior of LocalDateTimeStringConverter [v2]
On Wed, 10 Feb 2021 02:24:57 GMT, Nir Lisker 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. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Removed printing and narrowed the exception Looks good. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/393
Re: RFR: 8260468: Wrong behavior of LocalDateTimeStringConverter [v2]
On Tue, 9 Feb 2021 13:46:15 GMT, Kevin Rushforth wrote:
>> Nir Lisker has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Removed printing and narrowed the exception
>
> 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.
The test checks that the chronology didn't change as a result of catching the
exception internally. The thrown exception is from `formatter.parse` later on
and will happen regardless of this fix. The only reason I catch and ignore an
exception here is so that we can reach the assertion line after that.
If we want to test that an exception is thrown, it should be as part of an
input-output test (and we can do the same for other converters).
-
PR: https://git.openjdk.java.net/jfx/pull/393
Re: RFR: 8260468: Wrong behavior of LocalDateTimeStringConverter [v2]
> 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. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Removed printing and narrowed the exception - Changes: - all: https://git.openjdk.java.net/jfx/pull/393/files - new: https://git.openjdk.java.net/jfx/pull/393/files/854c98bc..14475167 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=393=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=393=00-01 Stats: 7 lines in 1 file changed: 2 ins; 3 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/393.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/393/head:pull/393 PR: https://git.openjdk.java.net/jfx/pull/393
