Icinga / icinga-notifications

Icinga Notifications: new and improved notifications and incident management for Icinga (work in progress, not ready for production yet)
GNU General Public License v2.0
9 stars 0 forks source link

DST handling #18

Open julianbrost opened 1 year ago

julianbrost commented 1 year ago

Even though github.com/teambition/rrule-go looked like the most promising library for evaluating rrules, it does not handle DST changes correctly.

For an event repeating daily at 02:30 in Europe/Berlin it generates a recurrence at 2023-03-26 03:30:00 (there is no 02:30 on that day in that timezone). That's incorrect according to the standard:

Recurrence rules may generate recurrence instances with an invalid date (e.g., February 30) or nonexistent local time (e.g., 1:30 AM on a day where the local time is moved forward by an hour at 1:00 AM). Such recurrence instances MUST be ignored and MUST NOT be counted as part of the recurrence set.

Example:

package main

import (
    "fmt"
    "github.com/teambition/rrule-go"
    "time"
)

func main() {
    rule, err := rrule.NewRRule(rrule.ROption{
        Freq:    rrule.DAILY,
        Dtstart: parse("2023-01-01 02:30:00"),
    })
    if err != nil {
        panic(err)
    }
    for _, t := range rule.Between(parse("2023-03-25 00:00:00"), parse("2023-03-28 00:00:00"), true) {
        fmt.Println(t)
    }
}

func parse(s string) time.Time {
    loc, err := time.LoadLocation("Europe/Berlin")
    if err != nil {
        panic(err)
    }

    t, err := time.ParseInLocation(time.DateTime, s, loc)
    if err != nil {
        panic(err)
    }
    return t
}

Incorrect output (the second line should not exist):

2023-03-25 02:30:00 +0100 CET
2023-03-26 03:30:00 +0200 CEST
2023-03-27 02:30:00 +0200 CEST
julianbrost commented 9 months ago

There's actually an errata regarding the very specific section I quoted, it should actually say:

Recurrence rules may generate recurrence instances with a nonexistent local time ((e.g., 1:30 AM on a day where the local time is moved forward by an hour at 1:00 AM). Such recurrence instances are handled as specified in Section 3.3.5.

And that referenced section says:

If, based on the definition of the referenced time zone, the local time described occurs more than once (when changing from daylight to standard time), the DATE-TIME value refers to the first occurrence of the referenced time. Thus, TZID=America/New_York:20071104T013000 indicates November 4, 2007 at 1:30 A.M. EDT (UTC-04:00). If the local time described does not occur (when changing from standard to daylight time), the DATE-TIME value is interpreted using the UTC offset before the gap in local times. Thus, TZID=America/New_York:20070311T023000 indicates March 11, 2007 at 3:30 A.M. EDT (UTC-04:00), one hour after 1:30 A.M. EST (UTC-05:00).

julianbrost commented 9 months ago

Testing both transitions (jumping forward an jumping backward):

package main

import (
    "fmt"
    "github.com/teambition/rrule-go"
    "time"
)

func main() {
    rule, err := rrule.NewRRule(rrule.ROption{
        Freq:    rrule.DAILY,
        Dtstart: parse("2023-01-01 02:30:00"),
    })
    if err != nil {
        panic(err)
    }
    for _, t := range rule.Between(parse("2023-03-25 00:00:00"), parse("2023-03-28 00:00:00"), true) {
        fmt.Println(t)
    }
    fmt.Println()
    for _, t := range rule.Between(parse("2023-10-28 00:00:00"), parse("2023-10-31 00:00:00"), true) {
        fmt.Println(t)
    }
}

func parse(s string) time.Time {
    loc, err := time.LoadLocation("Europe/Berlin")
    if err != nil {
        panic(err)
    }

    t, err := time.ParseInLocation(time.DateTime, s, loc)
    if err != nil {
        panic(err)
    }
    return t
}
2023-03-25 02:30:00 +0100 CET
2023-03-26 03:30:00 +0200 CEST
2023-03-27 02:30:00 +0200 CEST

2023-10-28 02:30:00 +0200 CEST
2023-10-29 02:30:00 +0100 CET
2023-10-30 02:30:00 +0100 CET

First one should be correct based on my understanding. However, the second one should be 2023-10-29 02:30:00 +0200 CEST because that's the first occurrence of 2:30.

Also, the library seems to rely on time.Date(), which does not guarantee the occurrence that will be returned:

In such cases, the choice of time zone, and therefore the time, is not well-defined. Date returns a time that is correct in one of the two zones involved in the transition, but it does not guarantee which.

julianbrost commented 9 months ago

Also, with more frequent repetitions than FREQ=DAILY, there are even more edge cases (FREQ=HOURLY can have 23, 24 or maybe 25 recurrences on the same day (I'm not entirely sure if the standard would allow 25)). Anyways, most calendar software doesn't even allow specifying that in the GUI (I've tested Thunderbird, Google/Android, Nextcloud, Outlook Web), so restricting us to daily or less frequently might be reasonable.