ical-org / ical.net

ical.net - an open source iCal library for .Net
Other
797 stars 231 forks source link

Serialized EXDATE is unpermitted PERIOD value type when originated as System.DateTime #591

Open jvraines opened 2 months ago

jvraines commented 2 months ago

This code

Calendar myCal = new Calendar();
CalendarEvent myEvent = new CalendarEvent {
    Start = new CalDateTime(2024, 12, 1, 12, 0, 0),
    End = new CalDateTime(2025, 1, 1, 18, 0, 0),
    RecurrenceRules = new List<RecurrencePattern> {
                            new RecurrencePattern {
                                Frequency = FrequencyType.Daily
                            }
                      }
};
myEvent.ExceptionDates.Add(new PeriodList { 
    new CalDateTime(2024, 12, 25)
    new CalDateTime(new DateTime(2024, 12, 26))
});
myCal.Events.Add(myEvent);
Console.WriteLine(new CalendarSerializer().SerializeToString(myCal));

produces

BEGIN:VCALENDAR PRODID:-//github.com/rianjs/ical.net//NONSGML ical.net 4.0//EN VERSION:2.0 BEGIN:VEVENT DTEND:20250101T180000 DTSTAMP:20240901T232258Z DTSTART:20241201T120000 EXDATE:20241225T000000,20241226/P1D RRULE:FREQ=DAILY SEQUENCE:0 UID:d65129f0-d00c-46d1-a7d2-81a73a41990d END:VEVENT END:VCALENDAR

The first excluded date is an acceptable DATE-TIME value type. The second, however, is a PERIOD value type which is not permitted by the EXDATE spec.

RemcoBlok commented 1 month ago

Another (presumably related) issue is that if you create the CalDateTime with a TZID (when it is an exception date for an event that is not an all-day event, but for a specific time on the day), iCal.NET does not serialize the TZID with the EXDATE. When deserializing this may result in GetOccurrences() producing an occurrence on the EXDATE, or what was supposed to be the EXDATE if it did not loose the TZID.

What can I do as a temporary workaround here? Fix the EXDATE after deserializing? Or remove the occurrence from the result of GetOccurrences()?

axunonb commented 1 month ago

Hi Remco, please kindly follow the Filing a bug report and submit a new issue. This makes it more simple and faster to understand the whole context of what your doing. If you think, ,your issue is related to this one, add a reference.

jvraines commented 1 month ago

@RemcoBlok Which CalDateTime constructor are you using? One with string tzId? If so, you could try passing appropriate time values to the constructor without that parameter and see what happens.

RemcoBlok commented 1 month ago

Thanks for your reply. I created a separate issue https://github.com/ical-org/ical.net/issues/614

axunonb commented 1 month ago

For EXDATE with a timezone it looks like the parameter for the timezone gets recognized and set, but not processed when rendering to a string. Als EXDATE with different timezones is not implemented correctly. We'll look closer into that, also in context of the referenced like #614, #588 https://github.com/ical-org/ical.net/blob/90700965f7a5393149f29b50b0daf05eb3903925/Ical.Net/Serialization/DataTypes/DateTimeSerializer.cs#L64-L89

axunonb commented 3 weeks ago

@minichma, after examining the serialization and deserialization of the EXDATE property, my current conclusion is that it will cause a breaking change. The IList<PeriodList> CalendarEvent.ExceptionDates is being serialized by the PeriodListSerializer, which does not handle EXDATE correctly. What are your thoughts on the following points?

  1. Is using IList<PeriodList> for ExceptionDates the best choice, or would IList<IDateTime> be more appropriate?
  2. Should we create a dedicated ExDateSerializer that inherits from PropertySerializer?
  3. Would it be sensible to create a v5 branch for all upcoming changes and eventually publish alpha versions on NuGet? I believe updating future v4 versions without breaking changes would be challenging.
jvraines commented 3 weeks ago
  1. Is using IList<PeriodList> for ExceptionDates the best choice, or would IList<IDateTime> be more appropriate? The latter. ExceptionDate is defined to be a Date or a DateTime, not a Period, by the iCal specification. I don't know why PeriodList was chosen, except perhaps it facilitates code flow elsewhere.
  2. Should we create a dedicated ExDateSerializer that inherits from PropertySerializer? I have no opinion since I have not comprehensively reviewed the code.
  3. Would it be sensible to create a v5 branch for all upcoming changes and eventually publish alpha versions on NuGet? I believe updating future v4 versions without breaking changes would be challenging. You are probably a better judge of this than I, although I would support the position that a breaking change should be handled explicity and with due caution.
minichma commented 3 weeks ago

Is using IList<PeriodList> for ExceptionDates the best choice, or would IList<IDateTime> be more appropriate?

Yes, IList<IDateTime> would be more appropriate. I assume ExceptionDates has been copied from RecurrenceDates, where PeriodList is correct, but EXDATEs don't support durations, as this issue correctly points out. The question would be, whether it is perfectly correct to serialize RDATEs into List<Period>, as it could be any of date-time / date / period, but not mixed though.

Should we create a dedicated ExDateSerializer that inherits from PropertySerializer? I have no opinion since I have not comprehensively reviewed the code.

Not sure, whether a separate ExDateSerializer is required. Basically we can have multiple list of ExDates and each list can be different in terms of type (DATE vs DATE-TIME) and time zone. In the end we'd end up with a something like List<List>.

Would it be sensible to create a v5 branch for all upcoming changes and eventually publish alpha versions on NuGet? I believe updating future v4 versions without breaking changes would be challenging.

Fully agree. #598 is waiting for a v5 branch too. Porting everything back to v4 would certainly be challenging in many cases. But v5 would probably be a work in progress for quite a while.