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

[Roger Riggs]

+1

On 3/9/20 12:05 PM, Joe Wang wrote:
> 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
>>>>>>>
>>>>>>>
>