rianjs / ical.net

ical.NET - an open source iCal library for .NET
MIT License
779 stars 230 forks source link

Bug: TZOFFSETTO in VTIMEZONE is incorrect for DAYLIGHT component #439

Open bjornlyngwa opened 5 years ago

bjornlyngwa commented 5 years ago

Hi, This issue seems to affect timezones with a positive timezone offset, in my specific case it's affecting daylight savings time differences between AEST+10 and AEST+11(DST). Instead of having TZOFFSETTO set to +11 as expected, it's actually +09. This is causing some Outlook calendars to record the event two hours earlier than expected.

I believe the cause of this issue is in VTimeZone.cs on line 156:

if (isDaylight)

            {
                timeZoneInfo.Name = "DAYLIGHT";
                timeZoneInfo.OffsetFrom = new UtcOffset(utcOffset);
                timeZoneInfo.OffsetTo = new UtcOffset(utcOffset - delta);
            }

By subtracting the difference in offsets, rather than adding it, the time is moved further back instead of being pushed forward.

I have taken a fork of this project and reversed the operation on the same line of code. This seems to have resolved the problem for my case, but I'm not confident this will work for all other cases. When running the unit tests in debug mode I noticed several cases of delta being negative, which caused a similar issue as I am experiencing, but for timezones with a negative offset instead.

Do you have any advice on how to resolve this? I am considering just forcing delta to always be negative which means the current subtraction operation will give a correct result. E.g., adding this in just before subtracting delta to set timeZoneInfo.OffsetTo:
if (delta > new TimeSpan()) delta = delta.Negate();

Y90SMH commented 4 years ago

I've had an issue too with "Australia/Melbourne" timezone and tried negating the delta at first, but calendars with older records still had issues. I haven't quite got to the bottom of this but what I have below seems more stable. This moves the isDaylight declaration up a few lines and the initial delta is created based on that. It actually seemed to be related to previousInternal being null as well, but I opted not rewrite the entire if...else block.

var previousInterval = intervals.SingleOrDefault(x => (x.HasEnd ? x.End : Instant.MaxValue) == oldestInterval.Start);

var isDaylight = oldestInterval.Savings.Ticks > 0;

var delta = new TimeSpan(isDaylight ? -1 : 1, 0, 0);

if (previousInterval != null)
{
    delta = (previousInterval.WallOffset - oldestInterval.WallOffset).ToTimeSpan();
}
else if (isOnlyInterval)
{
    delta = new TimeSpan();
}
arjan279 commented 4 years ago

I had a similar issue. To create the timezone object I was trying to be efficient by using the overload VTimeZone.FromLocalTimeZone(DateTime earlistDateTimeToSupport, bool includeHistoricalData), with earlistDateTimeToSupport set to 2020-08-17, includeHistoricalData set to false, and my timezone being "Europe/Amsterdam". After pulling out my hairs I discovered that the issue did not occur for me for overload VTimeZone.FromLocalTimeZone(). Turns out that VTimeZone.FromLocalTimeZone() is internally using january 1st of the current year as value for earlistDateTimeToSupport, in my case 2020-01-01, as seen in VTimeZone.cs line 21.

For my timezone in the northern hemisphere 2020-08-17 has DST enabled while 2020-01-01 has not. And for @bjornlyngwa timezone AEST and @Y90SMH "Australia/Melbourne" in the southern hemisphere it's just the other way around. I tried these timezones with earlistDateTimeToSupport set to 2020-08-17 and this made the issue disappear.

The underlying issue seems to be that VTimeZone.cs line 133 fails to lookup previousInterval when a DST date is used for earlistDateTimeToSupport because the previous non-DST interval has not been provided by the calling method, and because of that the proper delta cannot de determined in line 139.

Hope this will others as a workaround, and the dev to fix the issue.