Skip to content
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.

Sign up for GitHub

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

Fix #729: incorrect expansion behaviour after BYWEEKNO #731

Merged
merged 4 commits into from
Feb 20, 2025
Merged

Fix #729: incorrect expansion behaviour after BYWEEKNO #731

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6bb0b2f
Fix #729: incorrect expansion behaviour after `BYWEEKNO`
minichma Feb 15, 2025
405e6e8
Reduce cyclomatic complexity of `GetWeekNoVariants` to make CodeCov a…
minichma Feb 15, 2025
050e753
Make method static to avoid CodeCov complaining.
minichma Feb 16, 2025
62ade45
Merge branch 'main' into work/minichma/bugfix/byweekno_expansion
axunonb Feb 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions Ical.Net.Tests/Calendars/Recurrence/RecurrenceTestCases.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,67 @@ RRULE:FREQ=YEARLY;BYWEEKNO=1;BYDAY=MO,TU;INTERVAL=3;UNTIL=20320101
DTSTART:20241231
INSTANCES:20241231,20280103,20280104,20301230,20301231


# 30.12 in WEEK 1 with expansion on BYMONTH, BYMONTHDAY, BYWEEKNO
RRULE:FREQ=YEARLY;BYMONTH=12;BYMONTHDAY=1,30;BYWEEKNO=1;UNTIL=20320101
DTSTART:20241230
INSTANCES:20241230,20251230,20301230,20311230

# 30.12 in WEEK 1 with expansion on BYYEARDAY, BYWEEKNO
RRULE:FREQ=YEARLY;BYYEARDAY=-2;BYWEEKNO=1;UNTIL=20320101
DTSTART:20241230
INSTANCES:20241230,20251230,20301230,20311230

# 30.12 in WEEK 52 with expansion on BYMONTH, BYMONTHDAY, BYWEEKNO
RRULE:FREQ=YEARLY;BYMONTH=12;BYMONTHDAY=1,30;BYWEEKNO=52;UNTIL=20340101
DTSTART:20271230
INSTANCES:20271230,20281230,20291230,20331230

# 30.12 in WEEK 52 with expansion on BYYEARDAY, BYWEEKNO
RRULE:FREQ=YEARLY;BYYEARDAY=-2;BYWEEKNO=52;UNTIL=20340101
DTSTART:20271230
INSTANCES:20271230,20281230,20291230,20331230

# 30.12 in WEEK 53 with expansion on BYMONTH, BYMONTHDAY, BYWEEKNO
RRULE:FREQ=YEARLY;BYMONTH=12;BYMONTHDAY=1,30;BYWEEKNO=53;UNTIL=20380101
DTSTART:20261230
INSTANCES:20261230,20321230,20371230

# 30.12 in WEEK 53 with expansion on BYYEARDAY, BYWEEKNO
RRULE:FREQ=YEARLY;BYYEARDAY=-2;BYWEEKNO=53;UNTIL=20380101
DTSTART:20261230
INSTANCES:20261230,20321230,20371230

# 2.1 in WEEK 1 with expansion on BYMONTH, BYMONTHDAY, BYWEEKNO
RRULE:FREQ=YEARLY;BYMONTH=1;BYMONTHDAY=2,31;BYWEEKNO=1;UNTIL=20290102
DTSTART:20250102
INSTANCES:20250102,20260102,20290102

# 2.1 in WEEK 1 with expansion on BYYEARDAY, BYWEEKNO
RRULE:FREQ=YEARLY;BYYEARDAY=2;BYWEEKNO=1;UNTIL=20290102
DTSTART:20250102
INSTANCES:20250102,20260102,20290102

# 2.1 in WEEK 52 with expansion on BYMONTH, BYMONTHDAY, BYWEEKNO
RRULE:FREQ=YEARLY;BYMONTH=1;BYMONTHDAY=2,31;BYWEEKNO=52;UNTIL=20390102
DTSTART:20280102
INSTANCES:20280102,20390102

# 2.1 in WEEK 52 with expansion on BYYEARDAY, BYWEEKNO
RRULE:FREQ=YEARLY;BYYEARDAY=2;BYWEEKNO=52;UNTIL=20390102
DTSTART:20280102
INSTANCES:20280102,20390102

# 2.1 in WEEK 53 with expansion on BYMONTH, BYMONTHDAY, BYWEEKNO
RRULE:FREQ=YEARLY;BYMONTH=1;BYMONTHDAY=2,31;BYWEEKNO=53;UNTIL=20380102
DTSTART:20270102
INSTANCES:20270102,20330102,20380102

# 2.1 in WEEK 53 with expansion on BYYEARDAY, BYWEEKNO
RRULE:FREQ=YEARLY;BYYEARDAY=2;BYWEEKNO=53;UNTIL=20380102
DTSTART:20270102
INSTANCES:20270102,20330102,20380102

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

# BYMONTHDAY with limit behaviour; reproduces #728 as reported by pinkfloydx33
RRULE:FREQ=DAILY;BYMONTHDAY=20,-2;UNTIL=20250401
DTSTART:20250220
Expand Down
101 changes: 60 additions & 41 deletions Ical.Net/Evaluation/RecurrencePatternEvaluator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,18 @@ private IEnumerable<DateTime> EnumerateDates(DateTime originalDate, DateTime see
}
}

private struct ExpandContext
{
/// <summary>
/// Indicates whether the dates have been fully expanded. If true, subsequent parts should only limit, not expand.
/// </summary>
/// <remarks>
/// This makes a difference in case of BYWEEKNO, which might span months and years. After it was applied (BYWEEKNO would
/// always expand), the subsequent parts mustn't expand.
/// </remarks>
public bool DatesFullyExpanded { get; set; }
}

/// <summary>
/// Returns a list of possible dates generated from the applicable BY* rules, using the specified date as a seed.
/// </summary>
Expand All @@ -231,12 +243,14 @@ private IEnumerable<DateTime> EnumerateDates(DateTime originalDate, DateTime see
/// <returns>A list of possible dates.</returns>
private ISet<DateTime> GetCandidates(DateTime date, RecurrencePattern pattern, bool?[] expandBehaviors)
{
var expandContext = new ExpandContext() { DatesFullyExpanded = false };

var dates = new List<DateTime> { date };
dates = GetMonthVariants(dates, pattern, expandBehaviors[0]);
dates = GetWeekNoVariants(dates, pattern, expandBehaviors[1]);
dates = GetYearDayVariants(dates, pattern, expandBehaviors[2]);
dates = GetMonthDayVariants(dates, pattern, expandBehaviors[3]);
dates = GetDayVariants(dates, pattern, expandBehaviors[4]);
dates = GetWeekNoVariants(dates, pattern, expandBehaviors[1], ref expandContext);
dates = GetYearDayVariants(dates, pattern, expandBehaviors[2], ref expandContext);
dates = GetMonthDayVariants(dates, pattern, expandBehaviors[3], ref expandContext);
dates = GetDayVariants(dates, pattern, expandBehaviors[4], ref expandContext);
dates = GetHourVariants(dates, pattern, expandBehaviors[5]);
dates = GetMinuteVariants(dates, pattern, expandBehaviors[6]);
dates = GetSecondVariants(dates, pattern, expandBehaviors[7]);
Expand Down Expand Up @@ -306,7 +320,7 @@ private List<DateTime> GetMonthVariants(List<DateTime> dates, RecurrencePattern
/// </summary>
/// <param name="dates">The list of dates to which the BYWEEKNO rules will be applied.</param>
/// <returns>The modified list of dates after applying the BYWEEKNO rules.</returns>
private List<DateTime> GetWeekNoVariants(List<DateTime> dates, RecurrencePattern pattern, bool? expand)
private List<DateTime> GetWeekNoVariants(List<DateTime> dates, RecurrencePattern pattern, bool? expand, ref ExpandContext expandContext)
{
if (expand == null || pattern.ByWeekNo.Count == 0)
{
Expand All @@ -320,47 +334,47 @@ private List<DateTime> GetWeekNoVariants(List<DateTime> dates, RecurrencePattern

// Expand behavior
var weekNoDates = new List<DateTime>();
foreach (var t in dates)
foreach ((var t, var weekNo) in dates.SelectMany(t => GetByWeekNoForYearNormalized(pattern, t.Year), (t, weekNo) => (t, weekNo)))
{
foreach (var weekNo in GetByWeekNoForYearNormalized(pattern, t.Year))
{
var date = t;
var date = t;

// Make sure we start from a reference date that is in a week that belongs to the current year.
// Its not important that the date lies in a certain week, but that the week belongs to the
// current year and that the week day is preserved.
if (date.Month == 1)
date = date.AddDays(7);
else if (date.Month >= 12)
date = date.AddDays(-7);
// Make sure we start from a reference date that is in a week that belongs to the current year.
// Its not important that the date lies in a certain week, but that the week belongs to the
// current year and that the week day is preserved.
if (date.Month == 1)
date = date.AddDays(7);
else if (date.Month >= 12)
date = date.AddDays(-7);

// Determine our current week number
var currWeekNo = Calendar.GetIso8601WeekOfYear(date, pattern.FirstDayOfWeek);
// Determine our current week number
var currWeekNo = Calendar.GetIso8601WeekOfYear(date, pattern.FirstDayOfWeek);

// Move ahead to the correct week of the year
date = date.AddDays((weekNo - currWeekNo) * 7);
// Move ahead to the correct week of the year
date = date.AddDays((weekNo - currWeekNo) * 7);

// Ignore the week if it doesn't belong to the current year.
if (Calendar.GetIso8601YearOfWeek(date, pattern.FirstDayOfWeek) == t.Year)
{
// Step backward single days until we're at the correct DayOfWeek
while (date.DayOfWeek != pattern.FirstDayOfWeek)
{
date = date.AddDays(-1);
}
// Ignore the week if it doesn't belong to the current year.
if (Calendar.GetIso8601YearOfWeek(date, pattern.FirstDayOfWeek) != t.Year)
continue;

for (var k = 0; k < 7; k++)
{
weekNoDates.Add(date);
date = date.AddDays(1);
}
}
}
// Step backward single days until we're at the correct DayOfWeek
date = GetFirstDayOfWeekDate(date, pattern.FirstDayOfWeek);

weekNoDates.AddRange(Enumerable.Range(0, 7).Select(i => date.AddDays(i)));
}

// subsequent parts should only limit, not expand
expandContext.DatesFullyExpanded = true;

// Apply BYMONTH limit behavior, as we might have expanded over month/year boundaries
// in this method and BYMONTH has already been applied before, so wouldn't be again.
weekNoDates = GetMonthVariants(weekNoDates, pattern, expand: false);

return weekNoDates;
}

private static DateTime GetFirstDayOfWeekDate(DateTime date, DayOfWeek firstDayOfWeek)
=> date.AddDays(-((int) date.DayOfWeek + 7 - (int) firstDayOfWeek) % 7);

/// <summary>
/// Normalize the BYWEEKNO values to be positive integers.
/// </summary>
Expand All @@ -378,14 +392,14 @@ private List<int> GetByWeekNoForYearNormalized(RecurrencePattern pattern, int ye
/// </summary>
/// <param name="dates">The list of dates to which the BYYEARDAY rules will be applied.</param>
/// <returns>The modified list of dates after applying the BYYEARDAY rules.</returns>
private List<DateTime> GetYearDayVariants(List<DateTime> dates, RecurrencePattern pattern, bool? expand)
private static List<DateTime> GetYearDayVariants(List<DateTime> dates, RecurrencePattern pattern, bool? expand, ref ExpandContext expandContext)
{
if (expand == null || pattern.ByYearDay.Count == 0)
{
return dates;
}

if (expand.Value)
if (expand.Value && !expandContext.DatesFullyExpanded)
{
var yearDayDates = new List<DateTime>(dates.Count);
foreach (var date in dates)
Expand All @@ -397,6 +411,8 @@ private List<DateTime> GetYearDayVariants(List<DateTime> dates, RecurrencePatter
// Ignore the BY values that don't fit into the current year (i.e. +-366 in non-leap-years).
.Where(d => d.Year == date1.Year));
}

expandContext.DatesFullyExpanded = true;
return yearDayDates;
}
// Limit behavior
Expand Down Expand Up @@ -434,14 +450,14 @@ private List<DateTime> GetYearDayVariants(List<DateTime> dates, RecurrencePatter
/// </summary>
/// <param name="dates">The list of dates to which the BYMONTHDAY rules will be applied.</param>
/// <returns>The modified list of dates after applying the BYMONTHDAY rules.</returns>
private List<DateTime> GetMonthDayVariants(List<DateTime> dates, RecurrencePattern pattern, bool? expand)
private List<DateTime> GetMonthDayVariants(List<DateTime> dates, RecurrencePattern pattern, bool? expand, ref ExpandContext expandContext)
{
if (expand == null || pattern.ByMonthDay.Count == 0)
{
return dates;
}

if (expand.Value)
if (expand.Value && !expandContext.DatesFullyExpanded)
{
var monthDayDates = new List<DateTime>();
foreach (var date in dates)
Expand All @@ -455,6 +471,8 @@ select monthDay > 0
: date.AddDays(-date.Day + 1).AddMonths(1).AddDays(monthDay)
);
}

expandContext.DatesFullyExpanded = true;
return monthDayDates;
}

Expand Down Expand Up @@ -500,14 +518,14 @@ select monthDay > 0
/// </summary>
/// <param name="dates">The list of dates to which BYDAY rules will be applied.</param>
/// <returns>The modified list of dates after applying BYDAY rules, or the original list if no BYDAY rules are specified.</returns>
private List<DateTime> GetDayVariants(List<DateTime> dates, RecurrencePattern pattern, bool? expand)
private List<DateTime> GetDayVariants(List<DateTime> dates, RecurrencePattern pattern, bool? expand, ref ExpandContext expandContext)
{
if (expand == null || pattern.ByDay.Count == 0)
{
return dates;
}

if (expand.Value)
if (expand.Value && !expandContext.DatesFullyExpanded)
{
// Expand behavior
var weekDayDates = new List<DateTime>();
Expand All @@ -519,6 +537,7 @@ private List<DateTime> GetDayVariants(List<DateTime> dates, RecurrencePattern pa
}
}

expandContext.DatesFullyExpanded = true;
return weekDayDates;
}

Expand Down
Loading