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

[naoto.sato at oracle.com]

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
>>
>>