Fix regression introduced in v5.1.3 #889

axunonb · 2025-12-02T23:28:45Z

Implemented review comments for PR #884 by @minichma
These RRULEs had no test cases and worked in v5.1.2, but failed in v5.1.3

DTSTART:20250101
RRULE:FREQ=YEARLY;BYYEARDAY=-1,1;BYMONTHDAY=-1,1;COUNT=3
INSTANCE:20250101,20251231,20260101

DTSTART:20250601
RRULE:FREQ=YEARLY;BYMONTH=6;BYMONTHDAY=2,-2;UNTIL=20250627
INSTANCE:20250602

Closes #888
Fixes #890

codecov · 2025-12-02T23:29:51Z

Codecov Report

❌ Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
Ical.Net/Evaluation/RecurrencePatternEvaluator.cs	92.3%	0 Missing and 1 partial ⚠️

@@           Coverage Diff           @@
##            main    #889     +/-   ##
=======================================
- Coverage   68.3%   68.3%   -0.0%     
=======================================
  Files        115     115             
  Lines       4380    4376      -4     
  Branches    1013    1010      -3     
=======================================
- Hits        2991    2988      -3     
  Misses      1028    1028             
+ Partials     361     360      -1

Files with missing lines	Coverage Δ
Ical.Net/Evaluation/Evaluator.cs	`95.2% <100.0%> (-0.4%)`	⬇️
Ical.Net/Evaluation/RecurrencePatternEvaluator.cs	`91.1% <92.3%> (+0.3%)`	⬆️

🚀 New features to boost your workflow:

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

axunonb · 2025-12-02T23:54:13Z

GetAbsWeekDaysDaily(date, weekDay) never gets executed with our unit tests:

ical.net/Ical.Net/Evaluation/RecurrencePatternEvaluator.cs

Line 669 in 454247d

{ Frequency: FrequencyType.Daily } => GetAbsWeekDaysDaily(date, weekDay),

@minichma

Implemented review comments for PR #884 by @minichma

As pointed out in the review for PR #884 Included unit test to verify

minichma · 2025-12-11T19:43:15Z

Ical.Net.Tests/RecurrenceTests.cs

+                           BEGIN:VCALENDAR
+                           VERSION:2.0
+                           BEGIN:VEVENT
+                           DTSTART:20250601


Nice test! Maybe set DTSTART to 20250602, so it is in line with the RRULE?

minichma · 2025-12-11T21:15:31Z

Ical.Net/Evaluation/Evaluator.cs

                    // When a rule uses BYWEEKNO, recurrence enumeration
                    // is based on week numbers relative to the year.
-                    // So we preserve January 1st when adding years; otherwise, preserve month/day.
+                    // So we preserve the weekday when using BYWEEKNO and preserve month/day otherwise.


oh, I'm sorry, I guess I was wrong with my comment in #513. We indeed preserve Jan 1st, not the weekday. So I guess the previous version was better.

minichma · 2025-12-11T21:30:11Z

Ical.Net/Evaluation/Evaluator.cs

+                    // So we preserve the weekday when using BYWEEKNO and preserve month/day otherwise.
                    dt = (pattern.ByWeekNo.Count != 0)
                        ? old.AddDays(-old.DayOfYear + 1).AddYears(interval)
                        : old.AddYears(interval);


I don't fully understand this change (from #513) though, particularly the non-weekno case. It seems to be somewhat dangerous to just add an interval to the year, especially on Feb 29, as it might not exist in the next interval. Actually, when adding 1y to 2024-02-29, we end up in 2025-02-28 (just tried). However, when reaching 2028, we still stay on the 28th, even though the year has a 29th. The incorrect monthday doesn't seem to cause an issue, as it is properly handled in RecurrencePatternEvaluator, but as it doesn't seem to be considered anyways, I'd rather recommend setting it to a fixed value (Jan 1) rather than maintaining a 'wrong' month day. But then, do we need this distinction at all? We actually don't have a test that convers this distinction, i.e. when setting it always to Jan 1st, all tests are still green.

Moreover, RecurrencePatternEvaluator relies on the assumption that after incrementing, the new refDate is usually at the first day of an interval. For that reason, the following test case fails with the new behavior:

RRULE:FREQ=YEARLY;BYYEARDAY=1,10;UNTIL=20260105 DTSTART:20250110 INSTANCES:20250110,20260101

see also my comments in GetIntervalLowerLimit()

recommend setting it to a fixed value (Jan 1)

Fixed with next commit

minichma · 2025-12-11T22:00:38Z

Ical.Net/Evaluation/RecurrencePatternEvaluator.cs

+                    // Map BYMONTHDAY entries (positive and negative) to absolute days
+                    // in the target month, then pick the smallest. This handles cases
+                    // where BYMONTHDAY mixes positive and negative values (e.g. 2,-2).
+                    var mappedDays = pattern.ByMonthDay


Getting back to https://github.com/ical-org/ical.net/pull/884/files/84eb777d104f900184e58cb7cbc11c6126e17816#r2575960238

I was digging into the code (and git history) to remember, why I introduced this method. The code historically relies on the assumption that after Evaluator.IncrementDate() the new refDate is usually set to the first day of the given interval. For cases where this assumption doesn't hold, this method is intended to return the interval's actual start date (lower limit). So far this was only required for yearly recurrences when using BYWEEKNO, because for yearly recurrences the date was always set to Jan 1 but Week 1 could start in the previous year. In all other cases the code worked without that adjustment, but that's not quite obvious too. At least the time of the day is never set to 0:00, which is compensated by the fact, that we add 1d in GetSearchEndDate() but this is not documented at all and by no means obvious.

For now I'd recommend to set the date to Jan 1st in Evaluator.IncrementDate() in case of yearly recurrences. In this method I recommend to only deal with the special case of yearly/byweekno or alternatively return Jan 1st for the yearly case if byweekno is not used (but not only in BYMONTH is set).

At a later time we might investigate how to improve GetIntervalLowerLimit and GetSearchEndDate. The 1d adjustment in GetSearchEndDate is only intended to compensate for potential DST changes, not for refDate not being set to the beginning of the interval. I feel we should improve this so it is always set to the beginning of the interval, including the time part, and ideally also for the yearly/byweekno case. If we manage to do so, we can get rid of GetIntervalLowerLimit altogether.

only deal with the special case of yearly

Fixed with next commit

minichma · 2025-12-11T22:06:41Z

Ical.Net.Tests/RecurrenceTests.cs

+                           BEGIN:VCALENDAR
+                           VERSION:2.0
+                           BEGIN:VEVENT
+                           RRULE:FREQ=YEARLY;BYYEARDAY=-1,1;BYMONTHDAY=-1,1;COUNT=3


These tests would be great candidates for being included in RecurrenceTestCases.txt, which would safe a lot of boiler-plate.

Fixed with next commit

axunonb · 2025-12-12T20:49:02Z

@minichma Thanks for the review and taking the time for a deep dive.
See commit comments for details about how your remarks are now implemented..
Hope this version is more bullet proof.

Restore former behavior in `Evaluator.IncrementDate`: `case FrequencyType.Yearly: dt = old.AddDays(-old.DayOfYear + 1).AddYears(interval);` `RecurrencePatternEvaluator.GetIntervalLowerLimit` For yearly FREQUENCY but no BYMONTH or BYWEEKNO use the original DTSTART's month for interval boundary, preventing occurrences earlier than the intended range. Move test cases of this PR to `RecurrenceTestCases.txt` Added tests mentioned as failing in the review

sonarqubecloud · 2025-12-12T22:14:31Z

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

axunonb · 2025-12-13T12:30:24Z

@minichma Thanks!

minichma · 2025-12-13T20:24:22Z

Urw 🙂

axunonb changed the title ~~Fix regression introtuced in v5.1.3~~ Fix regression introduced in v5.1.3 Dec 2, 2025

axunonb marked this pull request as ready for review December 2, 2025 23:54

axunonb requested a review from minichma December 2, 2025 23:55

Fix regression introduced in v5.1.3

9b90f99

Implemented review comments for PR #884 by @minichma

axunonb force-pushed the wip/axunonb/pr/fix-v513-regression branch from 679df02 to 9b90f99 Compare December 3, 2025 00:02

axunonb added 2 commits December 3, 2025 01:31

Correction in RecurrencePatternEvaluator.GetIntervalLowerLimit

7c628f9

As pointed out in the review for PR #884 Included unit test to verify

Merge branch 'main' into wip/axunonb/pr/fix-v513-regression

65f4c3e

minichma reviewed Dec 11, 2025

View reviewed changes

axunonb requested a review from minichma December 12, 2025 20:49

axunonb force-pushed the wip/axunonb/pr/fix-v513-regression branch from a22762a to e3adb5b Compare December 12, 2025 20:56

axunonb force-pushed the wip/axunonb/pr/fix-v513-regression branch from e3adb5b to e07ffca Compare December 12, 2025 22:13

minichma approved these changes Dec 13, 2025

View reviewed changes

axunonb merged commit a6dab58 into main Dec 13, 2025
12 checks passed

axunonb deleted the wip/axunonb/pr/fix-v513-regression branch December 13, 2025 12:30

Fix regression introduced in v5.1.3 #889

Fix regression introduced in v5.1.3 #889

Uh oh!

Conversation

axunonb commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

axunonb commented Dec 2, 2025

Uh oh!

minichma Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

minichma Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

minichma Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

axunonb Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minichma Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

axunonb Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minichma Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

axunonb Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

axunonb commented Dec 12, 2025

Uh oh!

sonarqubecloud bot commented Dec 12, 2025

Quality Gate passed

Uh oh!

axunonb commented Dec 13, 2025

Uh oh!

Uh oh!

minichma commented Dec 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

axunonb commented Dec 2, 2025 •

edited

Loading

codecov bot commented Dec 2, 2025 •

edited

Loading

axunonb Dec 12, 2025 •

edited

Loading

axunonb Dec 12, 2025 •

edited

Loading