-
Notifications
You must be signed in to change notification settings - Fork 234
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.
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
EXDATE should serialize TZID #614
Comments
@RemcoBlok Thanks for the really helpful description of the issue. It deserves a gold medal. |
@axunonb Thanks for looking into it. I realise a better unit test is one that does not rely on the local time zone. A build server in the UTC timezone may unknowingly produce false positives if the test uses the local time zone. So the test that should pass, but currently fails is: [Fact]
public void ShouldNotOccurOnExceptionDateWithSerializationNotLocal()
{
// Arrange
Guid id = Guid.NewGuid();
string timeZoneId = "GMT Standard Time";
if (timeZoneId == TimeZoneInfo.Local.Id)
{
timeZoneId = "Pacific Standard Time";
}
DateTime start = new DateTime(2024, 10, 19, 18, 0, 0, DateTimeKind.Unspecified);
DateTime end = new DateTime(2024, 10, 19, 19, 0, 0, DateTimeKind.Unspecified);
DateTime exceptionDate = new DateTime(2024, 10, 19, 21, 0, 0, DateTimeKind.Unspecified);
RecurrencePattern recurrencePattern = new RecurrencePattern(FrequencyType.Hourly);
recurrencePattern.Count = 2;
recurrencePattern.Interval = 3;
IDateTime exceptionOccurrsOn = new CalDateTime(exceptionDate, timeZoneId);
PeriodList exceptionDates = new PeriodList();
exceptionDates.Add(exceptionOccurrsOn);
CalendarEvent myRecurringEvent = new CalendarEvent();
myRecurringEvent.Summary = "My Recurring Event";
myRecurringEvent.Uid = id.ToString();
myRecurringEvent.Start = new CalDateTime(start, timeZoneId);
myRecurringEvent.End = new CalDateTime(end, timeZoneId);
myRecurringEvent.RecurrenceRules.Add(recurrencePattern);
myRecurringEvent.ExceptionDates.Add(exceptionDates);
Calendar calendar = new Calendar();
calendar.Events.Add(myRecurringEvent);
// Act
CalendarSerializer serializer = new CalendarSerializer();
string schedulerData = serializer.SerializeToString(calendar);
Calendar deserializedCalendar = Calendar.Load(schedulerData);
DateTime date = new DateTime(2024, 10, 19);
HashSet<Occurrence> occurrences = deserializedCalendar.GetOccurrences<CalendarEvent>(date);
// Assert
Assert.Single(occurrences);
} |
I wrote similar unit tests using RecurrencePattern.Until instead of EXDATE. As mentioned in the linked issue #588 RecurrencePattern.Until also does not serialize a TZID either. However, the icalendar standard seems to suggest RecurrencePattern.Until should always be in UTC if the Calendar Event DtStart specified a TZID (the standard does not say that about EXDATE). However, if you do set RecurrencePattern.Until in UTC after serializing and deserializing you get an occurrence after the RecurrencePattern.Until. The following test should pass but currently fails: [Fact]
public void ShouldNotOccurHourlyAfterUtcUntilWithSerializationNotLocal()
{
// Arrange
Calendar calendar = new Calendar();
Guid id = Guid.NewGuid();
string timeZoneId = "GMT Standard Time";
if (timeZoneId == TimeZoneInfo.Local.Id)
{
timeZoneId = "Pacific Standard Time";
}
DateTime start = new DateTime(2024, 10, 19, 23, 0, 0, DateTimeKind.Unspecified);
DateTime end = new DateTime(2024, 10, 20, 00, 0, 0, DateTimeKind.Unspecified);
DateTime until = new DateTime(2024, 10, 19, 23, 59, 59, DateTimeKind.Unspecified);
IDateTime calUntil = new CalDateTime(until, timeZoneId);
RecurrencePattern recurrencePattern = new RecurrencePattern(FrequencyType.Hourly);
recurrencePattern.Until = calUntil.AsUtc;
recurrencePattern.Interval = 2;
CalendarEvent myRecurringEvent = new CalendarEvent();
myRecurringEvent.Summary = "My Recurring Event";
myRecurringEvent.Uid = id.ToString();
myRecurringEvent.Start = new CalDateTime(start, timeZoneId);
myRecurringEvent.End = new CalDateTime(end, timeZoneId);
myRecurringEvent.RecurrenceRules.Add(recurrencePattern);
calendar.Events.Add(myRecurringEvent);
// Act
CalendarSerializer serializer = new CalendarSerializer();
string schedulerData = serializer.SerializeToString(calendar);
Calendar deserializedCalendar = Calendar.Load(schedulerData);
DateTime startTime = new DateTime(2024, 10, 19);
DateTime endTime = startTime.AddDays(2).AddTicks(-1);
HashSet<Occurrence> occurrences = deserializedCalendar.GetOccurrences<CalendarEvent>(startTime, endTime);
// Assert
Assert.Single(occurrences);
} So after deserializing you need to convert the RecurrencePattern.Until back to the time zone of the Calendar Event DtStart. In addition, for an All-Day event, RecurrencePattern.Until should not have a time component at all. |
I specified some test cases and asked Co-Pilot for the ICS and the expected recurring results. No that bad, is it? Unit test 1: Single Exclusion Date
Unit test 2: Multiple Exclusion Dates
Unit test 3: EXDATE with DATE Value Type
Unit test 4: EXDATE with Multiple Time Zones
Unit test 5: EXDATE with UTC Time and DTSTART with Time Zone
|
Installed v4.1.11 locally, where the test from example 2 failed. Gave it a another try on dotnetfiddle: https://dotnetfiddle.net/016xWU referencing Ical.Net v4.1.11 under NetCore3.1
We'll see whether this finding can be helpful |
@axunonb Thanks for looking into this so quickly. The Co-Pilot generated occurrences look good. I cannot think of other test cases for EXDATE. I suppose similar unit test variations apply to RecurrenceRule.Until as well? |
The reason why the timezone is not rendered: The timezone must be for the |
Ah, thanks for clarifying. I see that when setting the TzId on the PeriodList the TzId of the CalPerdiod.StartTime is serialized and when deserializing the CalPerdiod.StartTime has the correct TzId. However, on deserialing the TzId on the PeriodList is not restored. That means you can serialize and deserialize once, but not twice. |
Yes, that's correct for v4. There is an other PR targeting v5 already in preparation, that aims to fix EXDATE and RDATE issues completely. |
Improve reliability and usability for `Period` and `PeriodList` to become less error-prone by enforcing timezones being used consistently Period - Make parameterless CTOR `internal` to ensure proper initialization by users - A period can be defined 1. by a start time and an end time, 2. by a start time and a duration, 3. by a start time only, with the duration unspecified. This is for EXDATE and RDATE date-only and date/time. - For cosistency, either the `EndTime` or the `Duration` can be set at a time. The last one set with a value not `null` will prevail, while the other will become `null`. - Timezones of `StartTime`and (optional) `EndTime` must be the same - `CompareTo` uses `AsUtc` for comparing the `StartTime` - Remove returning a "magic" duration of 1 day, if `EndTime` is null and `StartTime` is date-only. This broke `EXDATE` and `RDATE` of an event, when the only a start time exists. - Added `EffectiveEndTime` and `EffectiveDuration` properties to provide calculated values based on the set values. - Update the `EndTime` and `Duration` properties to directly return the set values. - Change the access modifiers of `GetEffectiveDuration()` and `GetEffectiveEndTime` methods from internal to private. PeriodList - `TzId`: `public` setter changed to `private` - `EnsureConsistentTimezones`: The first period determines the timezone of the `PeriodList` and all other `Period`s added must have the same timezone - Add `SetService(new PeriodListEvaluator(this))` for `StringReader` CTOR overload - Add `static PeriodList FromStringReader(StringReader)` - Add `static PeriodList FromDateTime(IDateTime)` - Add `PeriodList AddPeriod(Period)` for chaining - Add `PeriodList Add(IDateTime)` for chaining - nullable enable EventEvaluator: - `EventEvaluator.WithEndTime(Period)` only sets the `EndTime`, as `Period.EffectiveDuration` returns the duration. TodoEvaluator: - Remove method `PeriodWithDuration(Period)` as it became redudant with the refactored `Period` class. DataTypeSerializer and other serializers, CalendarObjectBase: - `Activator.CreateInstance(TargetType, true)` allows for not `public` CTORs, so that parameterless CTORs can be excluded from the public API, if proper initialization can't be assured (like with `Period`). PropertySerializer: - TBD: Never set the UTC timezone ID (use appending 'Z') - Resolves ical-org#590 - Resolves ical-org#591 - Resolves ical-org#614 - Resolves ical-org#676
Improve reliability and usability for `Period` and `PeriodList` to become less error-prone by enforcing timezones being used consistently Period - Make parameterless CTOR `internal` to ensure proper initialization by users - A period can be defined 1. by a start time and an end time, 2. by a start time and a duration, 3. by a start time only, with the duration unspecified. This is for EXDATE and RDATE date-only and date/time. - For cosistency, either the `EndTime` or the `Duration` can be set at a time. The last one set with a value not `null` will prevail, while the other will become `null`. - Timezones of `StartTime`and (optional) `EndTime` must be the same - `CompareTo` uses `AsUtc` for comparing the `StartTime` - Remove returning a "magic" duration of 1 day, if `EndTime` is null and `StartTime` is date-only. This broke `EXDATE` and `RDATE` of an event, when the only a start time exists. - Added `EffectiveEndTime` and `EffectiveDuration` properties to provide calculated values based on the set values. - Update the `EndTime` and `Duration` properties to directly return the set values. - Change the access modifiers of `GetEffectiveDuration()` and `GetEffectiveEndTime` methods from internal to private. PeriodList - `TzId`: `public` setter changed to `private` - `EnsureConsistentTimezones`: The first period determines the timezone of the `PeriodList` and all other `Period`s added must have the same timezone - Add `SetService(new PeriodListEvaluator(this))` for `StringReader` CTOR overload - Add `static PeriodList FromStringReader(StringReader)` - Add `static PeriodList FromDateTime(IDateTime)` - Add `PeriodList AddPeriod(Period)` for chaining - Add `PeriodList Add(IDateTime)` for chaining - nullable enable EventEvaluator: - `EventEvaluator.WithEndTime(Period)` only sets the `EndTime`, as `Period.EffectiveDuration` returns the duration. TodoEvaluator: - Remove method `PeriodWithDuration(Period)` as it became redudant with the refactored `Period` class. DataTypeSerializer and other serializers, CalendarObjectBase: - `Activator.CreateInstance(TargetType, true)` allows for not `public` CTORs, so that parameterless CTORs can be excluded from the public API, if proper initialization can't be assured (like with `Period`). PropertySerializer: - TBD: Never set the UTC timezone ID (use appending 'Z') - Resolves ical-org#590 - Resolves ical-org#591 - Resolves ical-org#614 - Resolves ical-org#676
Improve reliability and usability for `Period` and `PeriodList` to become less error-prone by enforcing timezones being used consistently Period - Make parameterless CTOR `internal` to ensure proper initialization by users - A period can be defined 1. by a start time and an end time, 2. by a start time and a duration, 3. by a start time only, with the duration unspecified. This is for EXDATE and RDATE date-only and date/time. - For cosistency, either the `EndTime` or the `Duration` can be set at a time. The last one set with a value not `null` will prevail, while the other will become `null`. - Timezones of `StartTime`and (optional) `EndTime` must be the same - `CompareTo` uses `AsUtc` for comparing the `StartTime` - Remove returning a "magic" duration of 1 day, if `EndTime` is null and `StartTime` is date-only. This broke `EXDATE` and `RDATE` of an event, when the only a start time exists. - Added `EffectiveEndTime` and `EffectiveDuration` properties to provide calculated values based on the set values. - Update the `EndTime` and `Duration` properties to directly return the set values. - Change the access modifiers of `GetEffectiveDuration()` and `GetEffectiveEndTime` methods from internal to private. PeriodList - `TzId`: `public` setter changed to `private` - `EnsureConsistentTimezones`: The first period determines the timezone of the `PeriodList` and all other `Period`s added must have the same timezone - Add `SetService(new PeriodListEvaluator(this))` for `StringReader` CTOR overload - Add `static PeriodList FromStringReader(StringReader)` - Add `static PeriodList FromDateTime(IDateTime)` - Add `PeriodList AddPeriod(Period)` for chaining - Add `PeriodList Add(IDateTime)` for chaining - nullable enable EventEvaluator: - `EventEvaluator.WithEndTime(Period)` only sets the `EndTime`, as `Period.EffectiveDuration` returns the duration. TodoEvaluator: - Remove method `PeriodWithDuration(Period)` as it became redudant with the refactored `Period` class. DataTypeSerializer and other serializers, CalendarObjectBase: - `Activator.CreateInstance(TargetType, true)` allows for not `public` CTORs, so that parameterless CTORs can be excluded from the public API, if proper initialization can't be assured (like with `Period`). PropertySerializer: - TBD: Never set the UTC timezone ID (use appending 'Z') - Resolves ical-org#590 - Resolves ical-org#591 - Resolves ical-org#614 - Resolves ical-org#676
Improve reliability and usability for `Period` and `PeriodList` to become less error-prone by enforcing timezones being used consistently Period - Make parameterless CTOR `internal` to ensure proper initialization by users - A period can be defined 1. by a start time and an end time, 2. by a start time and a duration, 3. by a start time only, with the duration unspecified. This is for EXDATE and RDATE date-only and date/time. - For cosistency, either the `EndTime` or the `Duration` can be set at a time. The last one set with a value not `null` will prevail, while the other will become `null`. - Timezones of `StartTime`and (optional) `EndTime` must be the same - `CompareTo` uses `AsUtc` for comparing the `StartTime` - Remove returning a "magic" duration of 1 day, if `EndTime` is null and `StartTime` is date-only. This broke `EXDATE` and `RDATE` of an event, when the only a start time exists. - Added `EffectiveEndTime` and `EffectiveDuration` properties to provide calculated values based on the set values. - Update the `EndTime` and `Duration` properties to directly return the set values. - Change the access modifiers of `GetEffectiveDuration()` and `GetEffectiveEndTime` methods from internal to private. PeriodList - `TzId`: `public` setter changed to `private` - `EnsureConsistentTimezones`: The first period determines the timezone of the `PeriodList` and all other `Period`s added must have the same timezone - Add `SetService(new PeriodListEvaluator(this))` for `StringReader` CTOR overload - Add `static PeriodList FromStringReader(StringReader)` - Add `static PeriodList FromDateTime(IDateTime)` - Add `PeriodList AddPeriod(Period)` for chaining - Add `PeriodList Add(IDateTime)` for chaining - nullable enable EventEvaluator: - `EventEvaluator.WithEndTime(Period)` only sets the `EndTime`, as `Period.EffectiveDuration` returns the duration. TodoEvaluator: - Remove method `PeriodWithDuration(Period)` as it became redudant with the refactored `Period` class. DataTypeSerializer and other serializers, CalendarObjectBase: - `Activator.CreateInstance(TargetType, true)` allows for not `public` CTORs, so that parameterless CTORs can be excluded from the public API, if proper initialization can't be assured (like with `Period`). PropertySerializer: - TBD: Never set the UTC timezone ID (use appending 'Z') - Resolves ical-org#590 - Resolves ical-org#591 - Resolves ical-org#614 - Resolves ical-org#676
Improve reliability and usability for `Period` and `PeriodList` to become less error-prone by enforcing timezones being used consistently Period - Make parameterless CTOR `internal` to ensure proper initialization by users - A period can be defined 1. by a start time and an end time, 2. by a start time and a duration, 3. by a start time only, with the duration unspecified. This is for EXDATE and RDATE date-only and date/time. - For cosistency, either the `EndTime` or the `Duration` can be set at a time. The last one set with a value not `null` will prevail, while the other will become `null`. - Timezones of `StartTime`and (optional) `EndTime` must be the same - `CompareTo` uses `AsUtc` for comparing the `StartTime` - Remove returning a "magic" duration of 1 day, if `EndTime` is null and `StartTime` is date-only. This broke `EXDATE` and `RDATE` of an event, when the only a start time exists. - Added `EffectiveEndTime` and `EffectiveDuration` properties to provide calculated values based on the set values. - Update the `EndTime` and `Duration` properties to directly return the set values. - Change the access modifiers of `GetEffectiveDuration()` and `GetEffectiveEndTime` methods from internal to private. PeriodList - `TzId`: `public` setter changed to `private` - `EnsureConsistentTimezones`: The first period determines the timezone of the `PeriodList` and all other `Period`s added must have the same timezone - Add `SetService(new PeriodListEvaluator(this))` for `StringReader` CTOR overload - Add `static PeriodList FromStringReader(StringReader)` - Add `static PeriodList FromDateTime(IDateTime)` - Add `PeriodList AddPeriod(Period)` for chaining - Add `PeriodList Add(IDateTime)` for chaining - nullable enable EventEvaluator: - `EventEvaluator.WithEndTime(Period)` only sets the `EndTime`, as `Period.EffectiveDuration` returns the duration. TodoEvaluator: - Remove method `PeriodWithDuration(Period)` as it became redudant with the refactored `Period` class. DataTypeSerializer and other serializers, CalendarObjectBase: - `Activator.CreateInstance(TargetType, true)` allows for not `public` CTORs, so that parameterless CTORs can be excluded from the public API, if proper initialization can't be assured (like with `Period`). PropertySerializer: - TBD: Never set the UTC timezone ID (use appending 'Z') - Resolves ical-org#590 - Resolves ical-org#591 - Resolves ical-org#614 - Resolves ical-org#676
Improve reliability and usability for `Period` and `PeriodList` to become less error-prone by enforcing timezones being used consistently Period - Make parameterless CTOR `internal` to ensure proper initialization by users - A period can be defined 1. by a start time and an end time, 2. by a start time and a duration, 3. by a start time only, with the duration unspecified. This is for EXDATE and RDATE date-only and date/time. - For cosistency, either the `EndTime` or the `Duration` can be set at a time. The last one set with a value not `null` will prevail, while the other will become `null`. - Timezones of `StartTime`and (optional) `EndTime` must be the same - `CompareTo` uses `AsUtc` for comparing the `StartTime` - Remove returning a "magic" duration of 1 day, if `EndTime` is null and `StartTime` is date-only. This broke `EXDATE` and `RDATE` of an event, when the only a start time exists. - Added `EffectiveEndTime` and `EffectiveDuration` properties to provide calculated values based on the set values. - Update the `EndTime` and `Duration` properties to directly return the set values. - Change the access modifiers of `GetEffectiveDuration()` and `GetEffectiveEndTime` methods from internal to private. PeriodList - `TzId`: `public` setter changed to `private` - `EnsureConsistentTimezones`: The first period determines the timezone of the `PeriodList` and all other `Period`s added must have the same timezone - Add `SetService(new PeriodListEvaluator(this))` for `StringReader` CTOR overload - Add `static PeriodList FromStringReader(StringReader)` - Add `static PeriodList FromDateTime(IDateTime)` - Add `PeriodList AddPeriod(Period)` for chaining - Add `PeriodList Add(IDateTime)` for chaining - nullable enable EventEvaluator: - `EventEvaluator.WithEndTime(Period)` only sets the `EndTime`, as `Period.EffectiveDuration` returns the duration. TodoEvaluator: - Remove method `PeriodWithDuration(Period)` as it became redudant with the refactored `Period` class. DataTypeSerializer and other serializers, CalendarObjectBase: - `Activator.CreateInstance(TargetType, true)` allows for not `public` CTORs, so that parameterless CTORs can be excluded from the public API, if proper initialization can't be assured (like with `Period`). PropertySerializer: - TBD: Never set the UTC timezone ID (use appending 'Z') - Resolves ical-org#590 - Resolves ical-org#591 - Resolves ical-org#614 - Resolves ical-org#676
Improve reliability and usability for `Period` and `PeriodList` to become less error-prone by enforcing timezones being used consistently Period - Make parameterless CTOR `internal` to ensure proper initialization by users - A period can be defined 1. by a start time and an end time, 2. by a start time and a duration, 3. by a start time only, with the duration unspecified. This is for EXDATE and RDATE date-only and date/time. - For cosistency, either the `EndTime` or the `Duration` can be set at a time. The last one set with a value not `null` will prevail, while the other will become `null`. - Timezones of `StartTime`and (optional) `EndTime` must be the same - `CompareTo` uses `AsUtc` for comparing the `StartTime` - Remove returning a "magic" duration of 1 day, if `EndTime` is null and `StartTime` is date-only. This broke `EXDATE` and `RDATE` of an event, when the only a start time exists. - Added `EffectiveEndTime` and `EffectiveDuration` properties to provide calculated values based on the set values. - Update the `EndTime` and `Duration` properties to directly return the set values. - Change the access modifiers of `GetEffectiveDuration()` and `GetEffectiveEndTime` methods from internal to private. PeriodList - `TzId`: `public` setter changed to `private` - `EnsureConsistentTimezones`: The first period determines the timezone of the `PeriodList` and all other `Period`s added must have the same timezone - Add `SetService(new PeriodListEvaluator(this))` for `StringReader` CTOR overload - Add `static PeriodList FromStringReader(StringReader)` - Add `static PeriodList FromDateTime(IDateTime)` - Add `PeriodList AddPeriod(Period)` for chaining - Add `PeriodList Add(IDateTime)` for chaining - nullable enable EventEvaluator: - `EventEvaluator.WithEndTime(Period)` only sets the `EndTime`, as `Period.EffectiveDuration` returns the duration. TodoEvaluator: - Remove method `PeriodWithDuration(Period)` as it became redudant with the refactored `Period` class. DataTypeSerializer and other serializers, CalendarObjectBase: - `Activator.CreateInstance(TargetType, true)` allows for not `public` CTORs, so that parameterless CTORs can be excluded from the public API, if proper initialization can't be assured (like with `Period`). PropertySerializer: - TBD: Never set the UTC timezone ID (use appending 'Z') - Resolves ical-org#590 - Resolves ical-org#591 - Resolves ical-org#614 - Resolves ical-org#676
I have an EXDATE at a specific time with a specific TZID. Presently ical.net does not serialize the TZID with the EXDATE. When deserializing the calendar this results in
GetOccurrences()
returning an occurrence on what was supposed to be the EXDATE had it not lost its TZID.I first saw this issue in ical.net 4.2.0. It is also present in 4.3.1.
First of all, without serialization and deserialization, this passes:
Example 1
This correctly returns a single occurrence. It does not return an occurrence on the EXDATE.
Add serialization and deserializtion, and this fails:
Example 2
This now returns two occurrences instead of one. It returns an occurrence on the EXDATE, or what was supposed to be the EXDATE if it had not lost its TZID. It turns out ical.net does not serialize the TZID with the EXDATE.
I attempted the following temporary workaround. I convert the EXDATE to UTC, while keeping the CalendarEvent Start in
GMT Standard Time
, because I want to preserve and roundtrip the original TZID, but this still fails:Example 3
This still returns two occurrences instead of one. This is possibly a second issue in ical.net, separate from the issue that ical.net does not serialize the TZID of an EXDATE. It seems the ical.net occurrence evaluator does not handle an EXDATE in a time zone that does not equal the time zone of the CalendarEvent Start.
A final workaround that does work is to convert the EXDATE back to the TZID of the CalendarEvent Start after deserializing:
Example 4
Not a good workaround. Another possible workaround is to not preserve any time zone information and convert everything to UTC. Also not a good workaround.
Of course, most importantly, ical.net should serialize the TZID with the EXDATE.
The text was updated successfully, but these errors were encountered: