RFR: JDK-8079063 ZoneOffsetTransition constructor should ignore nanoseconds

[Roger Riggs]

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
>