Review request for JDK-8066982: ZonedDateTime.parse() returns wrong ZoneOffset around DST fall transition

[Roger Riggs]

Hi Stephen,

Confirmed, the reviews are in progress to update the spec for 9 and
fix the implementation in 8.

Thanks, Roger


On 12/21/2015 2:05 PM, Stephen Colebourne wrote:
> On 11 December 2015 at 20:07, Roger Riggs <Roger.Riggs at oracle.com> wrote:
>> Stephen, can you  confirm that the added text and test in DateTimeFormatter
>> is not a specification change?
> I thought I replied to this earlier, but maybe not.
>
> This is not a change to the spec, but a clarification to add a clear
> spec in an area where clarity was missed. One could argue that some
> aspects of this behaviour is implicit from other parts of the API
> however it would probably be tricky to tie it up completely.
>
> I have no problem with not porting the spec addition to JDK 8, the but
> fix really needs to go back. Not being able to round trip a
> ZonedDateTime is really serious, and I remain amazed that this issue
> managed to slip through.
>
> Stephen
>
>
>> Our processes have a bit more to do if it is a spec change and it would
>> limit the backport to JDK 8.
>>
>> This bug fix will cause an existing TCK/JCK test to fail but that is
>> considered also a bug and is fixed.
>>      test/java/time/tck/java/time/TCKZonedDateTime.java
>>
>> Ramanand, some comments on the new test:
>>   - The 'private' qualifier on the tests and data providers is not used in
>> the current tests and
>>      is not consistently present in the new one.
>>      Since it has little/no function, the tests would be a bit cleaner
>> without it.
>>
>>   - Typically, test that have data dependencies (such as the timezone data)
>> cannot be used for
>>      compatibility to the specification, but the data is stable and it seems
>> unavoidable in this case.
>>
>>   - Are all of the data cases necessary to validate the specification?
>>     Redundant cases extend the testing time without adding more confidence to
>> the quality.
>>
>> Thanks,  Roger
>>
>>
>> On 12/10/2015 11:00 AM, Stephen Colebourne wrote:
>>> I believe this is suitable for committing, thanks, other reviews welcome!
>>> Stephen
>>>
>>>
>>>
>>> On 10 December 2015 at 15:36, Ramanand Patil <ramanand.patil at oracle.com>
>>> wrote:
>>>> Hi all,
>>>>
>>>> Please review the updated webrev:
>>>> http://cr.openjdk.java.net/~aefimov/8066982/webrev.01/
>>>>
>>>> I have modified the fix and test cases as per inputs given by Stephen.
>>>> Also, I have added the javadocs changes in this patch which were proposed in
>>>> the bug.
>>>>
>>>> Bug link is: https://bugs.openjdk.java.net/browse/JDK-8066982
>>>>
>>>>
>>>> Regards,
>>>> Ramanand.
>>>>
>>>>
>>>> -----Original Message-----
>>>> From: Stephen Colebourne [mailto:scolebourne at joda.org]
>>>> Sent: Wednesday, December 09, 2015 4:46 PM
>>>> To: core-libs-dev
>>>> Cc: i18n-dev
>>>> Subject: Re: <i18n dev> Review request for JDK-8066982:
>>>> ZonedDateTime.parse() returns wrong ZoneOffset around DST fall transition
>>>>
>>>> The logic looks fine.
>>>>
>>>> In the main code, this part
>>>>     .getLong(INSTANT_SECONDS);
>>>> can be replaced with
>>>>     .toEpochSecond();
>>>> which will be slightly faster.
>>>>
>>>> In the test case, this part
>>>>    .plus(15, ChronoUnit.MINUTES);
>>>> can be replaced with
>>>>    .plusMinutes(15)
>>>>
>>>> And
>>>>    .with(ChronoField.OFFSET_SECONDS,
>>>> ZoneOffset.of(offsetSamples[j]).getTotalSeconds())
>>>> can be replaced with
>>>>    .with(ZoneOffset.of(offsetSamples[j]))
>>>>
>>>> In addition to the looping tests, I'd like to see the examples from the
>>>> bug report as test cases. Those tests would be simple to follow and explain,
>>>> whereas the looping tests are a little hard to follow.
>>>>
>>>> thanks for fixing this
>>>> Stephen
>>>>
>>>>
>>>>
>>>> On 9 December 2015 at 07:44, Ramanand Patil <ramanand.patil at oracle.com>
>>>> wrote:
>>>>> HI all,
>>>>>
>>>>>
>>>>>
>>>>> Please review a fix for Bug  - HYPERLINK
>>>>> "https://bugs.openjdk.java.net/browse/JDK-8066982"JDK-8066982
>>>>>
>>>>>
>>>>>
>>>>> Bug - Parsing a string with ZonedDateTime.parse() that contains zone
>>>>> offset and zone ID "Europe/Berlin" returns a wrong ZonedDateAndTime
>>>>> (different offset). This error starts exactly at the transition time
>>>>> (included) and ends one hour later (excluded).
>>>>>
>>>>>
>>>>>
>>>>> Webrev - http://cr.openjdk.java.net/~aefimov/8066982/webrev.00/
>>>>>
>>>>>
>>>>>
>>>>> One existing test case in TCKZonedDateTime.java is also modified,
>>>>> because - when offset is invalid the local time is changed to make the
>>>>> result valid.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Regards,
>>>>>
>>>>> Ramanand.
>>