fix: Evaluation of EXDATE when date-only while DTSTART is date-time

[minichma]

Fix multiple issues related to the evaluation of EXDATEs:

• If EXDATEs of type DATE-only were used together with DTSTART of type DATE-TIME, and GetOccurrences() was used with a periodStart on the day of an EXDATE, then this EXDATE could be ignored.
• If EXDATEs of type DATE were mixed with those of type DATE-TIME, then the order could be confused, which could cause individual EXDATEs to be ignored

Its not clear from RFC 5545, whether EXDATEs and DTSTARTs of mixed, mismatching value types should be supported. Probably it shouldn't, because many things become unclear in such cases, especially when DTSTART has a time zone. Anyhow, because it seems to be used in the wild and has been supported in v4, we should probably continue to support it.

fixes #829, #832

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

❌ Your project status has failed because the head coverage (68%) is below the target coverage (80%). You can increase the head coverage or adjust the target coverage.

@@         Coverage Diff         @@
##           main   #830   +/-   ##
===================================
  Coverage    68%    68%           
===================================
  Files       106    106           
  Lines      4209   4216    +7     
  Branches    945    941    -4     
===================================
+ Hits       2842   2849    +7     
  Misses     1039   1039           
  Partials    328    328           

Files with missing lines	Coverage Δ
Ical.Net/Evaluation/RecurringEvaluator.cs	97% <100%> (+<1%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[minichma]

@ical-org/maintainers It might be good to fix this issue, but in general, mixing DATE-only, DATE-TIME, floating and non-floating time, doesn't seem to be a good idea to me. Would be good to know, how much this is actually used in the wild.

E.g. see this test case:

https://github.com/ical-org/ical.net/pull/830/files#diff-e28a8bc50c87925affccd06958f171b527c5abe69dcb4b7b2cd90705e1ddf07fR4108-R4115

The problem arises from the fact, that the order relation is unclear in such cases. The problem gets worse, if timezones would be involved.

What do you think?

[NRG-Drink]

I read the RFC definition of EXDATE and stumbled across this:

The "EXDATE" property can be used to exclude the value specified in "DTSTART".

I interpret this as: The EXDATE must match exactly with the DTSTART of the occurrence to exclude.
I don't agree to exclude all events on the given EXDATE date-value, because there can be multiple occurrences on the same day and than all would be ignored even if I just want to ignore one of them. This is the case for recurrences smaller than daily or on any pattern when we move an event.
An edge-case would be, when I move an event's start-date to another event's start-date. In this case I would have to exclude both or non. But I think this is intended and is not part of our scope.

If we match DTSTART exactly with EXDATE we can allow date-only and timezones without much trouble (at least in my head for the moment of writing 😅)

On the other hand, I can see the case to exclude all events by only providing the date since most calendars have daily as the smalles recurrence pattern. But even there we have to think about the moved events.

[axunonb]

Very good discussion!

Having another look at the iCalendar specification section 3.8.5.1 we can see

exdate = "EXDATE" exdtparam ":" exdtval *("," exdtval) CRLF
exdtparam = *(
;
; The following are OPTIONAL,
; but MUST NOT occur more than once.
;
(";" "VALUE" "=" ("DATE-TIME" / "DATE")) /
;
(";" tzidparam) /
;
; The following is OPTIONAL,
; and MAY occur more than once.
;
(";" other-param)
;
)
exdtval = date-time / date
;Value MUST match value type

Value MUST match value type:
EXDATE property interacts with DTSTART, including the requirement that their value types must match (i.e., both must be either DATE or DATE-TIME). So this supports @NRG-Drink 's arguments.

I also agree with @minichma "but in general, mixing DATE-only, DATE-TIME, floating and non-floating time, doesn't seem to be a good idea".

There are some supporting references from "the wild":

1. In the past we already had a related discussion in Serialized EXDATE sometimes breaks spec #239.

2. A Stack Overflow discussion with the very same topic.
   It perfectly demonstrates Google Calendar's preference for matching the DTSTART type for EXDATE, even if VALUE=DATE is explicitly used.

3. There is a Google Calendar Community Thread. Multiple users report that when importing .ics files into Google Calendar, EXDATE entries (especially those intended to exclude specific dates for recurring events that have a time component) are not being correctly honored. The excluded events still appear in their calendar. The common suggested solution from contributors is to ensure that the EXDATE entry includes the time and timezone, matching the DTSTART of the recurring event.

4. Outlook 365: Importing an .ics file with EXDATE with type DATE-TIME or DATE while DTSTART is DATE-TIME: The date is excluded in both cases.
   This behavior is specified MS-STANOICAL Section 2.2.85: "Outlook can import EXDATE as either a DATE-TIME or DATE, provided the value falls on the same day as the original start date of an instance of the recurrence, in the recurrence's time zone."
   Unfortunately it's not documented how MS implements this behavior in code.

What could be our strategy - to be discussed:

1. Strict RFC Compliance by Default

• Ensure that serialization and validation logic enforces matching value types between DTSTART and EXDATE, per RFC 5545.
• This keeps iCal.NET predictable and standards-compliant for systems like Google Calendar.

2. Optional Leniency Mode

• Introduce a configurable flag to EvaluationOptions (e.g., AllowLooseExdateMatching) that enables Outlook-style behavior.
• When enabled, allow EXDATE values of type DATE to match DATE-TIME instances by comparing only the date portion.

3. Parsing Logic

• If we had a Logger we could detect mismatched types and log a warning. Unfortunately we don't.
• Normalize EXDATE values to DATE-TIME if DTSTART is DATE-TIME, using midnight (T000000) or the original DTSTART time. This might be better than to ignore EXDATE because of a type mismatch.

4. Developer Guidance

• Document the behaviors above in the wiki and XML comments.
• Offer examples showing both strict and lenient modes.

BTW: #829 shouldn't be considered a bug, should it?

[minichma]

exdtval = date-time / date
;Value MUST match value type

Value MUST match value type:
EXDATE property interacts with DTSTART, including the requirement that their value types must match (i.e., both must be either DATE or DATE-TIME).

I think that the spec only says that the value type specified with the property must match. It doesn't seem to refer to DTSTART.

[axunonb]

spec only says that the value type specified with the property must match

Didn't interpret this way, but yes: good point. But still: Microsoft acknowledges deviations from RFC 5545 where Outlook accepts EXDATE values of type DATE even when DTSTART is DATE-TIME, as long as the date portion matches a recurrence instance.

[minichma]

BTW: #829 shouldn't be considered a bug, should it?

I think it should. The core of the problem there is that periodStart is not handled properly. Even if we'd specify that mixing of DATE-only and DATE-TIME wouldn't be allowed, there would still be a inconsistency, because without periodStart the EXDATE would be considered.

[Fuzzillogic]

FWIW, as mentioned in #829 (comment), our use case is that some recurring events (with type DATE-TIME as DTSTART / DTEND) should not occur on holidays (i.e. EXDATE of type DATE). So, we feed the relevant list of holidays as EXDATES for those recurring events. To determine whether a specific occurrence is in such EXDATE date, I'd expect it to simply use the DATE part of the calculated start DATE-TIME of the occurrence. That might handle the time zone question.

Thus, being able to use EXDATE of type DATE while the recurring event is DATE-TIME certainly simplifies things for us.

[minichma]

Sorry, I commented only on parts of your comment so far. Thanks for the list of how 'the big ones' deal with this case. So I guess its clear then, that we'll have to support the case too.

Optional Leniency Mode

I think we should implement one way of how we do it. If we add more modes, testing will become a nightmare. I also don't see a major benefit in having different modes here. Moreover, the RFC is not really clear in that respect.

Regarding how Outlook deals with the case:

Outlook can import EXDATE as either a DATE-TIME or DATE, provided the value falls on the same day as the original start date of an instance of the recurrence, in the recurrence's time zone.

That the date is compared 'in the recurrence's time zone' prevents us from using OrderedExclude, because the order of dates in the individual recurrence's time zone doesn't necessarily have to be the same as in UTC (compare #832). Anyhow, that shouldn't be a problem. We can just use Linq's regular .Except(), because the list of dates will usually be short.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[minichma]

Implemented it in a way now that each recurrence's date value in the recurrence's individual time zone is matched against the list of date-only EXDATEs. If the date portion matches, the recurrence is excluded.

Tried to find out what outlook.com and Google Calendar are doing. This is what I found:

• outlook.com: Doesn't seem to support importing RDATEs altogether. So the question of how to deal with different TZ within an event doesn't really apply. The date-only EXDATE is considered though and obviously matched against the recurrence's date part (in the TZ of DTSTART).
• Google Calendar: Supports importing RDATEs. If an RDATE or EXDATE are DATE-only, while DTSTART is DATE-TIME, then it seems to assume the DATE-only values are at 0:00 in the TZ of DTSTART.

So those implementations seem to be conflicting in this respect and the now proposed is yet different.

@ical-org/maintainers What do you think?

[axunonb]

Changing the signature and access modifier would mean a breaking change?

[minichma]

Puh, right, theoretically yes. But for this to be an issue, somebody would have to inherit Recurring Evaluator and then call this method. I really wouldn't expect this to be the case. Would wait for potential complaints.

[axunonb]

What do you think?

• No problem with Outlook
• Yes, that's what Google Calendar has implemented. Normalizing to midnight is (IMHO a worse but more simple) alternative to using DTSTART time part. We can't cover all.

I'd say the updated implementation in this PR is fine and will get another entry to FAQ.
@NRG-Drink Your opinion?