RFR: JDK-8079063 ZoneOffsetTransition constructor should ignore nanoseconds

[Peter Levart]

Cool!

Do we need any CCC approval as this *is* a spec change isn't it?

I haven't done such a thing yet, so please give me some pointers. I also 
intend to add a jtreg test that verifies this new behavior.

Regards, Peter

On 05/06/2015 05:06 PM, Stephen Colebourne wrote:
> I am also happy with this change.
>
> Stephen
>
>
> On 6 May 2015 at 15:43, Roger Riggs <Roger.Riggs at oracle.com> wrote:
>> Hi Peter,
>>
>> Thanks for the analysis and followup.
>>
>> Yes, I think thesimple check as you propose is the desired clarification.
>> I agree that the change should not affect any existing code outside the JDK
>> and does not raise a compatibility issue.
>>
>> Roger
>>
>>
>>
>> On 5/4/2015 6:22 AM, Peter Levart wrote:
>>> Hi,
>>>
>>> Now that JDK-8074003 is in, I'd like to discuss how to tackle JDK-8079063.
>>>
>>> Package-private ZoneOffsetTransition constructor that takes LocalDateTime
>>> transition is invoked from the following 4 places:
>>>
>>> 1 - the public static factory method:
>>>
>>>      /**
>>>       * Obtains an instance defining a transition between two offsets.
>>>       * <p>
>>>       * Applications should normally obtain an instance from {@link
>>> ZoneRules}.
>>>       * This factory is only intended for use when creating {@link
>>> ZoneRules}.
>>>       *
>>>       * @param transition  the transition date-time at the transition,
>>> which never
>>>       *  actually occurs, expressed local to the before offset, not null
>>>       * @param offsetBefore  the offset before the transition, not null
>>>       * @param offsetAfter  the offset at and after the transition, not
>>> null
>>>       * @return the transition, not null
>>>       * @throws IllegalArgumentException if {@code offsetBefore} and {@code
>>> offsetAfter}
>>>       *         are equal, or {@code transition.getNano()} returns non-zero
>>> value
>>>       */
>>>      public static ZoneOffsetTransition of(LocalDateTime transition,
>>> ZoneOffset offsetBefore, ZoneOffset offsetAfter) {
>>>
>>> ...this one already disallows transition parameters that have
>>> transition.getNano() != 0.
>>>
>>>
>>> 2 - Lines 498..500 of ZoneOffsetTransitionRule:
>>>
>>>          LocalDateTime localDT = LocalDateTime.of(date, time);
>>>          LocalDateTime transition = timeDefinition.createDateTime(localDT,
>>> standardOffset, offsetBefore);
>>>          return new ZoneOffsetTransition(transition, offsetBefore,
>>> offsetAfter);
>>>
>>> ...where 'time' is an instance field of ZoneOffsetTransitionRule. The
>>> ZoneOffsetTransitionRule public static factory method has the following
>>> definition:
>>>
>>>      /**
>>>       * Obtains an instance defining the yearly rule to create transitions
>>> between two offsets.
>>>       * <p>
>>>       * Applications should normally obtain an instance from {@link
>>> ZoneRules}.
>>>       * This factory is only intended for use when creating {@link
>>> ZoneRules}.
>>>       *
>>>       * @param month  the month of the month-day of the first day of the
>>> cutover week, not null
>>>       * @param dayOfMonthIndicator  the day of the month-day of the cutover
>>> week, positive if the week is that
>>>       *  day or later, negative if the week is that day or earlier,
>>> counting from the last day of the month,
>>>       *  from -28 to 31 excluding 0
>>>       * @param dayOfWeek  the required day-of-week, null if the month-day
>>> should not be changed
>>>       * @param time  the cutover time in the 'before' offset, not null
>>>       * @param timeEndOfDay  whether the time is midnight at the end of day
>>>       * @param timeDefnition  how to interpret the cutover
>>>       * @param standardOffset  the standard offset in force at the cutover,
>>> not null
>>>       * @param offsetBefore  the offset before the cutover, not null
>>>       * @param offsetAfter  the offset after the cutover, not null
>>>       * @return the rule, not null
>>>       * @throws IllegalArgumentException if the day of month indicator is
>>> invalid
>>>       * @throws IllegalArgumentException if the end of day flag is true
>>> when the time is not midnight
>>>       */
>>>      public static ZoneOffsetTransitionRule of(
>>>              Month month,
>>>              int dayOfMonthIndicator,
>>>              DayOfWeek dayOfWeek,
>>>              LocalTime time,
>>>              boolean timeEndOfDay,
>>>              TimeDefinition timeDefnition,
>>>              ZoneOffset standardOffset,
>>>              ZoneOffset offsetBefore,
>>>              ZoneOffset offsetAfter) {
>>>          Objects.requireNonNull(month, "month");
>>>          Objects.requireNonNull(time, "time");
>>>          Objects.requireNonNull(timeDefnition, "timeDefnition");
>>>          Objects.requireNonNull(standardOffset, "standardOffset");
>>>          Objects.requireNonNull(offsetBefore, "offsetBefore");
>>>          Objects.requireNonNull(offsetAfter, "offsetAfter");
>>>          if (dayOfMonthIndicator < -28 || dayOfMonthIndicator > 31 ||
>>> dayOfMonthIndicator == 0) {
>>>              throw new IllegalArgumentException("Day of month indicator
>>> must be between -28 and 31 inclusive excluding zero");
>>>          }
>>>          if (timeEndOfDay && time.equals(LocalTime.MIDNIGHT) == false) {
>>>              throw new IllegalArgumentException("Time must be midnight when
>>> end of day flag is true");
>>>          }
>>>          return new ZoneOffsetTransitionRule(month, dayOfMonthIndicator,
>>> dayOfWeek, time, timeEndOfDay, timeDefnition, standardOffset, offsetBefore,
>>> offsetAfter);
>>>      }
>>>
>>> ...which allows 'time' parameter (that becomes ZoneOffsetTransitionRule's
>>> 'time' field) with no restriction as to whether it can contain nanos != 0 or
>>> not.
>>>
>>>
>>> 3, 4 - Lines 668..678 of ZoneRules private getOffsetInfo method:
>>>
>>>              LocalDateTime dtBefore = savingsLocalTransitions[index];
>>>              LocalDateTime dtAfter = savingsLocalTransitions[index + 1];
>>>              ZoneOffset offsetBefore = wallOffsets[index / 2];
>>>              ZoneOffset offsetAfter = wallOffsets[index / 2 + 1];
>>>              if (offsetAfter.getTotalSeconds() >
>>> offsetBefore.getTotalSeconds()) {
>>>                  // gap
>>>                  return new ZoneOffsetTransition(dtBefore, offsetBefore,
>>> offsetAfter);
>>>              } else {
>>>                  // overlap
>>>                  return new ZoneOffsetTransition(dtAfter, offsetBefore,
>>> offsetAfter);
>>>              }
>>>
>>> ...where dtBefore/dtAfter "transition" parameters are taken from
>>> savingsLocalTransitions[] array that is filled-in in ZoneRules constructors
>>> from passed-in ZoneOffsetTransition objects. So here no nanos != 0 can sneak
>>> in if ZoneOffsetTransition invariant holds.
>>>
>>> The only place where nanos can sneak-in therefore seems to be the public
>>> ZoneOffsetTransitionRule.of() factory method. The question is whether the
>>> spec. could be changed so that ZoneOffsetTransitionRule.of() factory method
>>> would add another @throws definition:
>>>
>>>       * @throws IllegalArgumentException if {@code time.getNano()} returns
>>> non-zero value
>>>
>>>
>>> This, I think, would be consistent with ZoneOffsetTransition.of() factory
>>> method and I believe early enough in the live of the API so that no custom
>>> software would be affected:
>>>
>>>
>>> http://cr.openjdk.java.net/~plevart/jdk9-dev/ZoneOffsetTransitionRule.time/webrev.01/
>>>
>>> What do you think?
>>>
>>> Regards, Peter
>>>