[15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity

[Joe Wang]

The changes look good to me.

Best,
Joe

On 3/9/20 1:44 AM, Stephen Colebourne wrote:
> Fine by me, but I'm not an OpenJDK Reviewer
> Stephen
>
> On Mon, 9 Mar 2020 at 03:05, <naoto.sato at oracle.com> wrote:
>> Thanks, Stephen.
>>
>> Updated the webrev for those two suggestions:
>>
>> http://cr.openjdk.java.net/~naoto/8239836/webrev.04/
>>
>> Naoto
>>
>> On 3/8/20 4:22 PM, Stephen Colebourne wrote:
>>>    standardOffsets[0] == wallOffsets[0] needs to use .equals()
>>>
>>> Unrelated, there is a Javadoc/spec error at line 578 of the "new"
>>> file. It should be getValidOffsets, not getOffset.
>>>
>>> Apart from that, it looks good.
>>>
>>> thanks
>>> Stephen
>>>
>>>
>>> On Wed, 4 Mar 2020 at 19:06, <naoto.sato at oracle.com> wrote:
>>>> Hi Stephen,
>>>>
>>>> Thank you for the detailed explanation and correcting the
>>>> implementation. I incorporated all the suggestions below, as well as
>>>> adding a new test for isFixedOffset() implementation (with some clean-up):
>>>>
>>>> https://cr.openjdk.java.net/~naoto/8239836/webrev.03/
>>>>
>>>> Please take a look at it.
>>>>
>>>> Naoto
>>>>
>>>> On 3/3/20 3:30 PM, Stephen Colebourne wrote:
>>>>> I fear more changes are needed here.
>>>>>
>>>>> 1) The code is isFixedOffset() is indeed wrong, but the fix is
>>>>> insufficient. The zone is fixed if baseStandardOffset=baseWallOffset
>>>>> and all three other lists are empty. A fixed offset rule is the
>>>>> equivalent of a ZoneOffset. See ZoneId.normalized() for the code that
>>>>> assumes that.
>>>>>
>>>>> 2) The code in getOffset(Instant) is wrong, but so is the proposed
>>>>> fix. The purpose of the if statement is to optimise the following few
>>>>> lines which refer to savingsInstantTransitions and wallOffsets. The
>>>>> code does have a bug as it should return the first wall offset.
>>>>> Corrected code:
>>>>>      public ZoneOffset getOffset(Instant instant) {
>>>>>        if (savingsInstantTransitions.length == 0) {
>>>>>          return wallOffsets[0];
>>>>>        }
>>>>> With the correct definition of isFixedOffset() it becomes apparent
>>>>> that this if statement is in fact not related to the fixed offset.
>>>>>
>>>>> Currently this test case fails (a zone in DST for all eternity):
>>>>>            ZoneRules rules = ZoneRules.of(
>>>>>                    ZoneOffset.ofHours(1),
>>>>>                    ZoneOffset.ofHours(2),
>>>>>                    Collections.emptyList(),
>>>>>                    Collections.emptyList(),
>>>>>                    Collections.emptyList());
>>>>>            assertEquals(rules.getStandardOffset(Instant.EPOCH),
>>>>> ZoneOffset.ofHours(1));
>>>>>            assertEquals(rules.getOffset(Instant.EPOCH),  ZoneOffset.ofHours(2));
>>>>>
>>>>> 3) The code in getStandardOffset(Instant) also has an error as it
>>>>> checks the wrong array. It should check standardTransitions as that is
>>>>> what is used in the following few lines. Corrected code:
>>>>>      public ZoneOffset getStandardOffset(Instant instant) {
>>>>>        if (standardTransitions.length == 0) {
>>>>>          return standardOffsets[0];
>>>>>        }
>>>>>
>>>>> 4) The code in getOffsetInfo(LocalDateTime dt) has a similar fault.
>>>>> Since it protects savingsLocalTransitions, it should return
>>>>> wallOffsets[0].
>>>>>
>>>>> 5) The code in getDaylightSavings(Instant instant) has an optimising
>>>>> if statement that should refer to isFixedOffset().
>>>>>
>>>>> 6) Also, in the test.
>>>>>     assertTrue(!rules.isFixedOffset());
>>>>> should be
>>>>>     assertFalse(rules.isFixedOffset());
>>>>>
>>>>> The class has worked so far as every normal case has
>>>>> baseStandardOffset = baseWallOffset, even though it is conceptually
>>>>> valid for this not to be true.
>>>>>
>>>>> Stephen
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On Thu, 27 Feb 2020 at 20:42, <naoto.sato at oracle.com> wrote:
>>>>>> Hello,
>>>>>>
>>>>>> Please review the fix to the following issue:
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8239836
>>>>>>
>>>>>> The proposed changeset is located at:
>>>>>>
>>>>>> https://cr.openjdk.java.net/~naoto/8239836/webrev.00/
>>>>>>
>>>>>> It turned out that 'transitionList' is not necessarily a superset of
>>>>>> 'standardOffsetTransitionList', as in some occasions where a standard
>>>>>> offset change and a savings change happen at the same time (canceling
>>>>>> each other), resulting in no wall time transition. This means that the
>>>>>> "rules" in the sample code is a valid ZoneRules instance.
>>>>>>
>>>>>> However, there is an assumption in ZoneRules where no (wall time)
>>>>>> transition means the fixed offset zone. With that assumption, the
>>>>>> example rule is considered a fixed offset zone which is not correct. So
>>>>>> the fix proposed here is not to throw an IAE but to recognize the rule
>>>>>> as a valid, non-fixed offset ZoneRules instance.
>>>>>>
>>>>>> Naoto
>>>>>>
>>>>>>