How to deal with non-RFC-compliant input

[pinkfloydx33]

Describe the bug
Preview version is returning incorrect occurrences under certain circumstances compared to both expectations and the previous version. This seems to be w/r/t to combining:

• FREQ=DAILY with BYMONTHDAY, (which should be allowable)
  • previously returned an empty set which may have been incorrect also
  • now returns every day except for the day actually listed under BYMONTHDAY
• FREQ=MONTHLY with BYYEARDAY (which is invalid per spec)
  • behaves as if it's just "monthly"
  • had same behavior in v4 which I believe is incorrect
• FREQ=DAILY with BYYEARDAY (which is invalid per spec)
  • behaves as if it's just "daily"
  • had same behavior in v4 which I believe is incorrect

To Reproduce
Steps to reproduce the behavior:

Here's an example of combining DAILY with BYMONTHDAY that previously returned an empty set. This would be the equivalent of:

DTSTART:20250201T000000Z
RRULE:FREQ=DAILY;BYMONTHDAY=20

var evt = new CalendarEvent
{
	Start = new CalDateTime(new DateOnly(2025, 2, 1)),
	RecurrenceRules =
	[
		new RecurrencePattern("FREQ=DAILY;BYMONTHDAY=20")
	]
};

var r = evt.GetOccurrences(
	new DateTime(2025, 2, 1, 0, 0, 0, DateTimeKind.Utc), // 2025-02-01
	new DateTime(2025, 2, 3, 0, 0, 0, DateTimeKind.Utc)  // 2025-02-02
).ToList();

// At the very least this should return EMPTY like v4
// but instead it returns 2025-02-01 and 2025-02-02 which are _not_ monthday=20
Assert.Empty(r);

var r2 = evt.GetOccurrences(
	new DateTime(2025, 2, 19, 0, 0, 0, DateTimeKind.Utc), // 2025-02-19
	new DateTime(2025, 2, 22, 0, 0, 0, DateTimeKind.Utc)  // 2025-02-22
).ToList();

// IIF anything, I'd expect this to be Day 20 (from the rule) but instead its day 19 and 21 (everything except 20!)
var single = Assert.Single(r);
Assert.Equal(20, r[0].Period.StartTime.Date.Day);  

Here's an example of combining MONTHLY with BYYEARDAY that previously returned an empty set

DTSTART:20250101T000000Z
RRULE:FREQ=MONTHLY;BYYEARDAY=20

var evt = new CalendarEvent
{
	Start = new CalDateTime(new DateOnly(2025, 1, 1)),
	RecurrenceRules =
	[
		new RecurrencePattern("FREQ=MONTHLY;BYYEARDAY=20")
	]
};

var r = evt.GetOccurrences(
	new DateTime(2025, 1, 1, 0, 0, 0, DateTimeKind.Utc), // 2025-01-01
	new DateTime(2025, 1, 2, 0, 0, 0, DateTimeKind.Utc)  // 2025-01-02
).ToList();

// At the very least this should return EMPTY but instead it returns 2025-01-01 which is not yearday=20
Assert.Empty(r);

var r2 = evt.GetOccurrences(
	new DateTime(2025, 1, 1, 0, 0, 0, DateTimeKind.Utc), // 2025-01-01
	new DateTime(2025, 2, 2, 0, 0, 0, DateTimeKind.Utc)  // 2025-02-02
).ToList();

// IIF this were valid per the spec, I'd expect this to be Day 20 (from the rule) 
// v4 and v5 returns the first day of each month instead
var single = Assert.Single(r);
Assert.Equal(20, r[0].Period.StartTime.Date.Day);  

Expected behavior
These both should probably return an empty set similar to version 4. However certain online rule engines treat them as valid and will produce the 20th in every month for the first rule, and January 20 every year for the second. You can try them here and here.

The specification indicates that BYMONTHDAY is only invalid if FREQ=WEEKLY and that BYYEARDAY is only invalid

The BYMONTHDAY rule part MUST NOT be specified when the FREQ rule part is set to WEEKLY.

which makes me think that the empty set behavior was possibly a bug in v4. In any case iif it's going to return values, it should not return 2025-02-01 as the first instance and it most certainly should not be excluding 2025-02-20!!

Conversely, the specification indicates that BYYEARDAY cannot be specified with daily/weekly/monthly:

The BYYEARDAY rule part MUST NOT be specified when the FREQ rule part is set to DAILY, WEEKLY, or MONTHLY.

Which means that the second event about really should be an empty set; it most certainly shouldn't return every single day

Environment (please complete the following information):

• OS: Win11
• .NET version: .NET9
• ical.net version: 5.0.0-pre.39

[minichma]

Thanks for reporting! Had a brief look, seems quite serious. We will only address those that are allowed by the RFC though, at least for now.

[minichma]

@pinkfloydx33 Would you have a change to look at #730 and tell, whether it is sufficient to close this issue?

[axunonb]

@pinkfloydx33 Maybe could provide some (more) missing unit tests in another PR? Would be highly appreciated.

[pinkfloydx33]

Hey sorry folks. I have been really sick and out of the office since reporting. I will try and circle back in the next few days

[pinkfloydx33]

One question that might help me validate when I have a chance: what is the expectation of the library if passed an invalid rule? For example FREQ=Daily + BYYEARDAY which is disallowed per the spec. Would the library just silently ignore the invalid parts (which I think is the case)? Or will it throw an error, return an empty set, or something else?

The whole reason I caught this issue is because we have some unit tests for "nonsensical rule" and "time frame" combinations where we were expecting an empty set of occurences that started failing with the upgrade. We don't actually expect these to happen in our app (with a well behaving requestor, that is) but we were covering edge cases to ensure we'd mark the rule/duration as "empty"

[axunonb]

Regarding validation there are 2 aspects.

1. Serialization
   It would be desirable for ical.net to allow the user to create RFC5545-compliant iCalendars. There are already some validations for this, but they are certainly not yet complete.

2. Deserialization
   RFC-5545-compliant ICS should not be a problem.
   The challenge is dealing with non-compliant calendars. In previous versions of ical.net, attempts were often made to guess a valid value from invalid input values. However, this can lead to unexpected results or error messages to us.
   Here it would be better to recognize non-compliant input and leave it to the user to decide how to deal with it. We do not currently have a solution for this. See also the discussion at TBD: Make parsing of CalDateTime strings more restrictive #734.