Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Daylight saving transition bugs #623

Open
RemcoBlok opened this issue Oct 24, 2024 · 7 comments
Open

Daylight saving transition bugs #623

RemcoBlok opened this issue Oct 24, 2024 · 7 comments

Comments

@RemcoBlok
Copy link

RemcoBlok commented Oct 24, 2024 •
edited
Loading

In Ical.Net 4.3.1 I see bugs when getting occurrences for an event around the daylight saving transition when the clock
goes an hour forward or back.

For instance the following test currently fails when the clock goes an hour forward so that between 00:00 and 02:00 a single hour elapsed.

        [Fact]
        public void ClockGoingForwardTest()
        {
            // Arrange
            string tzId = "Europe/London";

            CalendarEvent myEvent = new CalendarEvent();
            myEvent.Start = new CalDateTime(2025, 3, 30, 0, 0, 0, tzId);
            myEvent.End = new CalDateTime(2025, 3, 30, 2, 0, 0, tzId);

            Calendar calendar = new Calendar();
            calendar.Events.Add(myEvent);

            // Act
            IDateTime start = new CalDateTime(2025, 3, 30, 0, 0, 0, tzId);
            IDateTime end = new CalDateTime(2025, 3, 31, 0, 0, 0, tzId);

            ICollection<Occurrence> occurrences = calendar.GetOccurrences<CalendarEvent>(start, end);

            // Assert
            Assert.Equal(1, myEvent.Duration.TotalHours);

            Assert.Single(occurrences);
            Occurrence occurrence = occurrences.Single();

            Assert.Same(myEvent, occurrence.Source);

            Assert.Equal(myEvent.Duration, occurrence.Period.Duration);
            Assert.Equal(myEvent.Start, occurrence.Period.StartTime);
            Assert.Equal(myEvent.End, occurrence.Period.EndTime);
        }

The test fails when asserting

            Assert.Equal(myEvent.End, occurrence.Period.EndTime);

with

Assert.Equal() Failure: Values differ
Expected: 30/03/2025 02:00:00 Europe/London
Actual:   30/03/2025 01:00:00 Europe/London

Similarly, the following test currently fails when the clock goes back an hour so that between 01:00 and 02:00 two hours elapsed. Note that 01:00 is an ambiguous time, but iCal.Net should use the first occurrence of 01:00 (as NodaTime does by default), therefore two hours elapse between 01:00 and 02:00.

        [Fact]
        public void ClockGoingBackTest()
        {
            // Arrange
            string tzId = "Europe/London";
            
            CalendarEvent myEvent = new CalendarEvent();
            myEvent.Start = new CalDateTime(2024, 10, 27, 1, 0, 0, tzId);
            myEvent.End = new CalDateTime(2024, 10, 27, 2, 0, 0, tzId);

            Calendar calendar = new Calendar();
            calendar.Events.Add(myEvent);

            // Act
            IDateTime start = new CalDateTime(2024, 10, 27, 0, 0, 0, tzId);
            IDateTime end = new CalDateTime(2024, 10, 28, 0, 0, 0, tzId);
            
            ICollection<Occurrence> occurrences = calendar.GetOccurrences<CalendarEvent>(start, end);

            // Assert
            Assert.Equal(2, myEvent.Duration.TotalHours);

            Assert.Single(occurrences);
            Occurrence occurrence = occurrences.Single();

            Assert.Same(myEvent, occurrence.Source);

            Assert.Equal(myEvent.Duration, occurrence.Period.Duration);
            Assert.Equal(myEvent.Start, occurrence.Period.StartTime);
            Assert.Equal(myEvent.End, occurrence.Period.EndTime);
        }

The test fails when asserting

            Assert.Equal(myEvent.End, occurrence.Period.EndTime);

with

Assert.Equal() Failure: Values differ
Expected: 27/10/2024 02:00:00 Europe/London
Actual:   27/10/2024 03:00:00 Europe/London

The tests should pass, but currently fail.

Since occurrence.Period.Duration is correct, as a workround, I should not use occurrence.Period.EndTime, but calculate the end time of the occurrence period from occurrence.Period.StartTime and occurrence.Period.Duration.
@RemcoBlok
Copy link
Author

RemcoBlok commented Oct 24, 2024 •
edited
Loading

Here are two more tests that fail, but should pass, using all-day events on the days when the clock goes an hour forward or back.

        [Fact]
        public void ClockGoingForwardAllDayTest()
        {
            // Arrange
            string timeZoneId = "Europe/London";

            CalendarEvent myEvent = new CalendarEvent();
            myEvent.Start = new CalDateTime(2025, 3, 30, timeZoneId);
            myEvent.Start.HasTime = false;
            myEvent.End = new CalDateTime(2025, 3, 31, timeZoneId);
            myEvent.End.HasTime = false;

            Debug.Assert(myEvent.IsAllDay);

            Calendar calendar = new Calendar();
            calendar.Events.Add(myEvent);

            // Act
            IDateTime start = new CalDateTime(2025, 3, 30, 0, 0, 0, timeZoneId);
            IDateTime end = new CalDateTime(2025, 3, 31, 0, 0, 0, timeZoneId);

            ICollection<Occurrence> occurrences = calendar.GetOccurrences<CalendarEvent>(start, end);

            // Assert
            Assert.Equal(23, myEvent.Duration.TotalHours);

            Assert.Single(occurrences);
            Occurrence occurrence = occurrences.Single();

            Assert.Same(myEvent, occurrence.Source);

            Assert.Equal(myEvent.Duration, occurrence.Period.Duration);
            Assert.False(occurrence.Period.StartTime.HasTime);
            Assert.Equal(myEvent.Start, occurrence.Period.StartTime);
            Assert.False(occurrence.Period.EndTime.HasTime);
            Assert.Equal(myEvent.End, occurrence.Period.EndTime);
        }

and

        [Fact]
        public void ClockGoingBackAllDayTest()
        {
            // Arrange
            string timeZoneId = "GMT Standard Time";

            CalendarEvent myEvent = new CalendarEvent();
            myEvent.Start = new CalDateTime(2024, 10, 27, timeZoneId);
            myEvent.Start.HasTime = false;
            myEvent.End = new CalDateTime(2024, 10, 28, timeZoneId);
            myEvent.End.HasTime = false;

            Debug.Assert(myEvent.IsAllDay);

            Calendar calendar = new Calendar();
            calendar.Events.Add(myEvent);

            // Act
            IDateTime start = new CalDateTime(2024, 10, 27, 0, 0, 0, timeZoneId);
            IDateTime end = new CalDateTime(2024, 10, 28, 0, 0, 0, timeZoneId);

            ICollection<Occurrence> occurrences = calendar.GetOccurrences<CalendarEvent>(start, end);

            // Assert
            Assert.Equal(25, myEvent.Duration.TotalHours);

            Assert.Single(occurrences);
            Occurrence occurrence = occurrences.Single();

            Assert.Same(myEvent, occurrence.Source);

            Assert.Equal(myEvent.Duration, occurrence.Period.Duration);
            Assert.False(occurrence.Period.StartTime.HasTime);
            Assert.Equal(myEvent.Start, occurrence.Period.StartTime);
            Assert.False(occurrence.Period.EndTime.HasTime);
            Assert.Equal(myEvent.End, occurrence.Period.EndTime);
        }

UPDATE 1
I now specify the timeZoneId for the Start and End of all-day calendar events. I previously thought this was not required or perhaps even not allowed for all-day events. The bug remains present in the above unit tests when I do specify the timeZoneId for the Start and End of all-day events.

However, GMT Standard Time happens to be my local system time zone. It may be different on machines running different time zones. So just to make sure the bug is not tied to my local system time zone I tried the same tests with Pacific Standard Time and the daylight saving transitions in Pacific Standard Time and these tests fail as well.

        [Fact]
        public void ClockGoingForwardAllDayNonLocalTest()
        {
            // Arrange
            string timeZoneId = "Pacific Standard Time";

            CalendarEvent myEvent = new CalendarEvent();
            myEvent.Start = new CalDateTime(2025, 3, 9, timeZoneId);
            myEvent.Start.HasTime = false;
            myEvent.End = new CalDateTime(2025, 3, 10, timeZoneId);
            myEvent.End.HasTime = false;

            Debug.Assert(myEvent.IsAllDay);

            Calendar calendar = new Calendar();
            calendar.Events.Add(myEvent);

            // Act
            IDateTime start = new CalDateTime(2025, 3, 9, 0, 0, 0, timeZoneId);
            IDateTime end = new CalDateTime(2025, 3, 10, 0, 0, 0, timeZoneId);

            ICollection<Occurrence> occurrences = calendar.GetOccurrences<CalendarEvent>(start, end);

            // Assert
            Assert.Equal(23, myEvent.Duration.TotalHours);

            Assert.Single(occurrences);
            Occurrence occurrence = occurrences.Single();

            Assert.Same(myEvent, occurrence.Source);

            Assert.Equal(myEvent.Duration, occurrence.Period.Duration);
            Assert.False(occurrence.Period.StartTime.HasTime);
            Assert.Equal(myEvent.Start, occurrence.Period.StartTime);
            Assert.False(occurrence.Period.EndTime.HasTime);
            Assert.Equal(myEvent.End, occurrence.Period.EndTime);
        }

and

        [Fact]
        public void ClockGoingBackAllDayNonLocalTest()
        {
            // Arrange
            string timeZoneId = "Pacific Standard Time";

            CalendarEvent myEvent = new CalendarEvent();
            myEvent.Start = new CalDateTime(2024, 11, 3, timeZoneId);
            myEvent.Start.HasTime = false;
            myEvent.End = new CalDateTime(2024, 11, 4, timeZoneId);
            myEvent.End.HasTime = false;

            Debug.Assert(myEvent.IsAllDay);

            Calendar calendar = new Calendar();
            calendar.Events.Add(myEvent);

            // Act
            IDateTime start = new CalDateTime(2024, 11, 3, 0, 0, 0, timeZoneId);
            IDateTime end = new CalDateTime(2024, 11, 4, 0, 0, 0, timeZoneId);

            ICollection<Occurrence> occurrences = calendar.GetOccurrences<CalendarEvent>(start, end);

            // Assert
            Assert.Equal(25, myEvent.Duration.TotalHours);

            Assert.Single(occurrences);
            Occurrence occurrence = occurrences.Single();

            Assert.Same(myEvent, occurrence.Source);

            Assert.Equal(myEvent.Duration, occurrence.Period.Duration);
            Assert.False(occurrence.Period.StartTime.HasTime);
            Assert.Equal(myEvent.Start, occurrence.Period.StartTime);
            Assert.False(occurrence.Period.EndTime.HasTime);
            Assert.Equal(myEvent.End, occurrence.Period.EndTime);
        }

I tried a few more tests where I now schedule an all day event in Pacific Standard Time on the day of a daylight saving transition in GMT Standard Time, my local system time zone. And these tests pass.

        [Fact]
        public void ClockGoingForwardAllDayNonLocalTest()
        {
            // Arrange
            string timeZoneId = "Pacific Standard Time";

            CalendarEvent myEvent = new CalendarEvent();
            myEvent.Start = new CalDateTime(2025, 3, 30, timeZoneId);
            myEvent.Start.HasTime = false;
            myEvent.End = new CalDateTime(2025, 3, 31, timeZoneId);
            myEvent.End.HasTime = false;

            Debug.Assert(myEvent.IsAllDay);

            Calendar calendar = new Calendar();
            calendar.Events.Add(myEvent);

            // Act
            IDateTime start = new CalDateTime(2025, 3, 30, 0, 0, 0, timeZoneId);
            IDateTime end = new CalDateTime(2025, 3, 31, 0, 0, 0, timeZoneId);

            ICollection<Occurrence> occurrences = calendar.GetOccurrences<CalendarEvent>(start, end);

            // Assert
            Assert.Equal(24, myEvent.Duration.TotalHours);

            Assert.Single(occurrences);
            Occurrence occurrence = occurrences.Single();

            Assert.Same(myEvent, occurrence.Source);

            Assert.Equal(myEvent.Duration, occurrence.Period.Duration);
            Assert.False(occurrence.Period.StartTime.HasTime);
            Assert.Equal(myEvent.Start, occurrence.Period.StartTime);
            Assert.False(occurrence.Period.EndTime.HasTime);
            Assert.Equal(myEvent.End, occurrence.Period.EndTime);
        }

and

        [Fact]
        public void ClockGoingBackAllDayNonLocalTest()
        {
            // Arrange
            string timeZoneId = "Pacific Standard Time";

            CalendarEvent myEvent = new CalendarEvent();
            myEvent.Start = new CalDateTime(2024, 10, 27, timeZoneId);
            myEvent.Start.HasTime = false;
            myEvent.End = new CalDateTime(2024, 10, 28, timeZoneId);
            myEvent.End.HasTime = false;

            Debug.Assert(myEvent.IsAllDay);

            Calendar calendar = new Calendar();
            calendar.Events.Add(myEvent);

            // Act
            IDateTime start = new CalDateTime(2024, 10, 27, 0, 0, 0, timeZoneId);
            IDateTime end = new CalDateTime(2024, 10, 28, 0, 0, 0, timeZoneId);

            ICollection<Occurrence> occurrences = calendar.GetOccurrences<CalendarEvent>(start, end);

            // Assert
            Assert.Equal(24, myEvent.Duration.TotalHours);

            Assert.Single(occurrences);
            Occurrence occurrence = occurrences.Single();

            Assert.Same(myEvent, occurrence.Source);

            Assert.Equal(myEvent.Duration, occurrence.Period.Duration);
            Assert.False(occurrence.Period.StartTime.HasTime);
            Assert.Equal(myEvent.Start, occurrence.Period.StartTime);
            Assert.False(occurrence.Period.EndTime.HasTime);
            Assert.Equal(myEvent.End, occurrence.Period.EndTime);
        }

But the other tests are supposed to pass as well.

UPDATE 2
Just saw another issue. I created an all-day event for Sunday 27 October 2024 in GMT when the clocks go back one hour. I then added a recurrence for every Sunday. The next occurrence on Sunday 3 November 2024 in GMT ended up being 25 hours instead of 24 hours. Then ask the calendar for occurrences on Monday 4 November 2024 in GMT and you get one occurrence, namely the "all-day" event for the Sunday, which now lasts into the Monday because it is 25 hours instead of 24 hours.

So it appears I cannot always rely on the duration or end time of an occurrence. If I specify an event as an all-day event, an occurrence of that event should be all-day. Since ical.net uses NodaTime, it should use a one-day Period, not a Duration. A duration can be 23, 24 or 25 hours but a one-day Period is always one day.

Similarly if I specify an event from 01:00 to 02:00 on Sunday 27 October 2024 in GMT that should again be a 1 hour Period, not a 1 hour Duration.

Perhaps CalendarEvent should get constructor overloads that allows you to specify if you define an event using a Period or Duration.

@axunonb
Copy link
Collaborator

axunonb commented Oct 28, 2024

@RemcoBlok Thanks a lot. Acknowledge the issues.
We're currently only two on this project. Please consider to accept the invitation I just sent to you.

@RemcoBlok
Copy link
Author

@axunonb could you send me a direct message please?

@axunonb
Copy link
Collaborator

axunonb commented Nov 2, 2024

@RemcoBlok ✔️ Done via LinkedIn

@minichma minichma mentioned this issue Nov 4, 2024
Merged
@RemcoBlok
Copy link
Author

Excited to see lots of activity in this repo.
Regarding daylight saving transitions and ambiguous times, NodaTime's ZonedDateTime has no ambiguity as a ZonedDateTime also keeps the offset as well as the local date time and time zone. So ZonedDateTime can distinguish the first occurrence and the second occurrence of the ambiguous 01:00:00 time when the clocks go back one hour using the offset.
So the first occurrence is at 2024-10-27 01:00:00 GMT Standard Time (+01) and the second occurrence is at 2024-10-27 01:00:00 GMT Standard Time (+00).

If I am not mistaken the iCalendar standard allows for including an offset as well as a time zone. However CalDateTime only takes a DateTime and tzId string. Could we add a constructor overload to CalDateTime to accept a DateTimeOffset and tzId? That way CalDateTime allows for unambiguous times when the clocks go back one hour.

Thanks

@axunonb
Copy link
Collaborator

axunonb commented Nov 5, 2024

Thanks! So this is what you have in mind for CTOR overload?

public CalDateTime(DateTimeOffset value, string? tzId)
{
    var dateTime = value.DateTime;

    // Adjust the DateTime by the offset
    var adjustedDateTime = dateTime + value.Offset;

    Initialize(new DateOnly(adjustedDateTime.Year, adjustedDateTime.Month, adjustedDateTime.Day),
        new TimeOnly(adjustedDateTime.Hour, adjustedDateTime.Minute, adjustedDateTime.Second),
        adjustedDateTime.Kind,
        tzId,
        null);
}

@RemcoBlok
Copy link
Author

RemcoBlok commented Nov 6, 2024 •
edited
Loading

Hm, that was not what I had in mind. I was thinking of the serialized Date-Time having both the offset and tzid. But I was mistaken thinking that the iCalendar standard would support that. I now found clearly in https://icalendar.org/iCalendar-RFC-5545/3-3-5-date-time.html that storing an offset is not permitted. That means certain times will be ambiguous, in which case Form # 3 says it must be interpreted as the first occurrence, which is what ical.net is currently doing. But that leaves no way to save a Date-Time that represents the second occurrence, unless against the standard we do store an offset. I think changing the offset in your code will not work either as how would deserialization of such a Date-Time be correctly interpreted? Or the evaluation of occurrences. Apologies for my misinformed suggestion on offsets. Of course the above issues with daylight saving transitions are still issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@RemcoBlok @axunonb