Enhance `RecurrencePatternSerializer` #758

axunonb · 2025-03-30T15:56:37Z

RRULE parts are now processed in any order, which is compliant with RFC5545 section 3.3.10. Resolves RecurrencePatternSerializer expects that the FREQ` is the first in Recurrence Rules #755
Disallowed recurrence pattern parts will throw ArgumentOutOfRangeExceptions
Refactor methods with high cognitive complexity
Remove deserialization of natural language input

codecov · 2025-03-30T15:57:54Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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

@@         Coverage Diff         @@
##           main   #758   +/-   ##
===================================
+ Coverage    66%    67%   +1%     
===================================
  Files       104    104           
  Lines      4592   4519   -73     
  Branches   1146   1106   -40     
===================================
- Hits       3033   3014   -19     
+ Misses     1124   1072   -52     
+ Partials    435    433    -2

Files with missing lines	Coverage Δ
Ical.Net/Constants.cs	`45% <ø> (ø)`
Ical.Net/DataTypes/RecurrencePattern.cs	`74% <ø> (ø)`
...alization/DataTypes/RecurrencePatternSerializer.cs	`95% <100%> (+23%)`	⬆️

🚀 New features to boost your workflow:

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

axunonb · 2025-03-30T16:05:12Z

@minichma TBD:
RecurrencePatternSerializer.TryDeserializeNaturalLanguagePattern can process kind of natural language input. This ability is not in use and fully untested. Also this is part of the regular deserialization and gets called when the FREQ part is missing. I'm in doubt whether this functionality brings any added value. Should we remove it completely or at least use a different entry point for this kind of parsing?

BTW: Just noticed, we can ask Copilot for a review, too :)

Copilot

Pull Request Overview

This pull request enhances the RecurrencePatternSerializer by supporting RRULE parts in any order, refactoring methods to reduce cognitive complexity, and separating the deserialization logic for RFC-compliant input and natural language input.

Updated regex names and helper method extraction to improve parsing flexibility
Revised processing of key-value pairs and numeric temporal units
Added comprehensive test cases for validating RRULE parsing in any order

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
Ical.Net/Serialization/DataTypes/RecurrencePatternSerializer.cs	Refactored deserialization methods to support order-independent RRULE parts and natural language input
Ical.Net/DataTypes/RecurrencePattern.cs	Added documentation for the BySetPosition property
Ical.Net.Tests/SerializationTests.cs	Introduced new test cases ensuring RRULE properties are correctly deserialized regardless of order

Comments suppressed due to low confidence (1)

Ical.Net/Serialization/DataTypes/RecurrencePatternSerializer.cs:421

[nitpick] Returning false immediately in TryAddNumericTemporalUnit when parsing fails changes the original behavior (which skipped the invalid value). Consider continuing the loop instead to avoid prematurely terminating processing of comma-separated fields.

if (!int.TryParse(match.Groups["Num"].Value, out var num))

minichma · 2025-03-30T17:28:03Z

Didn't look into the actual implementation so far, but generally speaking I think the standard serializers/deserializers should stick to the RFC. So when parsing a calendar from any other format, this should happen fully intentional (not as a fallback from the standard parser) and independently. I therefore agree that it should rather not be an integral part of standard deserialization.

If this included as part of Ical.Net, it should be implemented in separate classes that could be used independently. The functionality reminds me of #576. Could this be combined?

- RRULE parts are now processed in any order, which is compliant to RFC5545 section 3.3.10. - Refactor methods with high cognitive complexity - Split deserialization of RFC-compliant and natural language input

axunonb · 2025-03-30T18:48:14Z

The functionality reminds me of #576. Could this be combined?

Yes, it has some similarities. I wonder whether natural langue for recurrences brings much advantage. End users would be better off with a good GUI, while devops might prefer a builder with a fluent API.

Anyway, the natural language part is undocumented, untested, and can never be all fallback for RFC-compliance. I'll remove it.

`

Copilot

Pull Request Overview

This PR enhances the RecurrencePatternSerializer to process RRULE parts in any order per RFC5545, reduces cognitive complexity by refactoring methods, and removes natural language input deserialization.

Refactored deserialization logic using a dedicated FrequencyPattern regex and helper methods
Removed legacy regex patterns and natural language parsing logic
Updated tests to verify order-independent RRULE deserialization and recurrence range checks

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
Ical.Net/Serialization/DataTypes/RecurrencePatternSerializer.cs	Refactored deserialization logic and removed natural language parsing regex definitions
Ical.Net/DataTypes/RecurrencePattern.cs	Added XML documentation for the BySetPosition property
Ical.Net.Tests/RecurrenceTests.cs	Added tests to validate order independence and recurrence range checks

Comments suppressed due to low confidence (2)

Ical.Net/Serialization/DataTypes/RecurrencePatternSerializer.cs:179

[nitpick] The constant 'CiCompiled' is now using PascalCase, which may be inconsistent with the previous naming style for constants. Consider adopting a consistent naming convention (such as all uppercase or a consistent prefix) for constants across the codebase.

private const RegexOptions CiCompiled = RegexOptions.IgnoreCase | RegexOptions.Compiled;

Ical.Net/Serialization/DataTypes/RecurrencePatternSerializer.cs:321

[nitpick] Consider trimming whitespace from the 'day' string in the AddWeekDays method to prevent potential issues when parsing day names.

byDay.Add(new WeekDay(day));

Copilot · 2025-03-30T18:52:08Z

Ical.Net/Serialization/DataTypes/RecurrencePatternSerializer.cs

+
+    private void ProcessKeyValuePair(string key, string value, RecurrencePattern r, ISerializerFactory factory)
+    {
+        switch (key)


[nitpick] The switch statement in ProcessKeyValuePair does not include a default case to handle unexpected keys. Adding a default branch (even if only for logging or graceful handling) would improve robustness.

minichma

Great, much better! Just a few minor comment.

minichma · 2025-03-30T19:17:04Z

Ical.Net/Serialization/DataTypes/RecurrencePatternSerializer.cs

-
+    private const RegexOptions CiCompiled = RegexOptions.IgnoreCase | RegexOptions.Compiled;
+
+    internal static readonly Regex FrequencyPattern =


make private?

minichma · 2025-03-30T19:21:10Z

Ical.Net/Serialization/DataTypes/RecurrencePatternSerializer.cs

+
+    internal static readonly Regex FrequencyPattern =
+        new Regex(@"FREQ=(SECONDLY|MINUTELY|HOURLY|DAILY|WEEKLY|MONTHLY|YEARLY);?(.*)", CiCompiled,
+            RegexDefaults.Timeout);


I know, this is legacy, but I think, we shouldn't specify a timeout in Regex. Timeouts in Regex are good if the pattern is user-specified, but in our case there is no reason to expect any bad behavior. On the other hand, the timeout could easily cause unexpected behavior. E.g. on a server running close to its limits, a timeout of 200 ms could easily be hit also on well-formed input.

Let's make the timeout 2000 ms, which is unlikely ever to be hit

minichma · 2025-03-30T19:35:41Z

Ical.Net/Serialization/DataTypes/RecurrencePatternSerializer.cs

-
+    private const RegexOptions CiCompiled = RegexOptions.IgnoreCase | RegexOptions.Compiled;
+
+    internal static readonly Regex FrequencyPattern =


Do we actually need this regex at all (anymore)? The only thing it really does now is validating that a RRULE contains a FREQ=xxx string (e.g. thisIsNotFREQ=DAILY! would work), but that's really not too helpful as either all parts of the string would have to be validated or nothing. The real validation happens throughout the parsing process.

minichma · 2025-03-30T19:36:43Z

Ical.Net/Serialization/DataTypes/RecurrencePatternSerializer.cs

-        {
-            // Parse the frequency type
-            r.Frequency = (FrequencyType) Enum.Parse(typeof(FrequencyType), match.Groups[1].Value, true);
+        return TryDeserializeFrequencyPattern(value, r, factory)


If the pattern can't be parsed, why would we return null? Shouldn't we raise an exception?

Will change to always throw on failure

minichma · 2025-03-30T19:44:09Z

Ical.Net/Serialization/DataTypes/RecurrencePatternSerializer.cs

        }
-        else
+
+        CheckMutuallyExclusive("COUNT", "UNTIL", r.Count, r.Until);


Check frequency is present here, rather than doing the regex upfront.

minichma · 2025-03-30T19:45:23Z

Ical.Net/Serialization/DataTypes/RecurrencePatternSerializer.cs

+            if (keywordPair.Length == 0)
            {
-                r.Interval = 1;
+                // ignore subsequent semi-colons


.. as well as preceding and duplicate ones

all kinds - will add a unit test

minichma · 2025-03-30T19:53:46Z

Ical.Net/Serialization/DataTypes/RecurrencePatternSerializer.cs

+                break;
+
+            case "bysecond":
+                AddInt32Values(r.BySecond, value);


By adding the entry, we accept BYSECOND to appear more than once (same for all 'BY' parts). Legacy though.

won't check

minichma · 2025-03-30T19:58:38Z

Ical.Net/Serialization/DataTypes/RecurrencePatternSerializer.cs

-            // Couldn't parse the object, return null!
-            r = null;
+            case "freq":
+                r.Frequency = (FrequencyType) Enum.Parse(typeof(FrequencyType), value, true);


I guess we should fail if None is specified.

minichma · 2025-03-30T20:10:37Z

Ical.Net/Serialization/DataTypes/RecurrencePatternSerializer.cs

+            case "wkst":
+                r.FirstDayOfWeek = GetDayOfWeek(value);
+                break;
        }


Shouldn't we add a default case for unknown/invalid rule parts and fail here?

axunonb · 2025-03-31T11:58:38Z

Agree with your comments on existing code. Starting with a bug fix and ending up with a rewrite, while limited time is running by...

sonarqubecloud · 2025-03-31T12:42:06Z

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

Copilot

Pull Request Overview

This PR enhances the RecurrencePatternSerializer by refactoring its deserialization logic to process RRULE parts in any order in accordance with RFC5545 and by removing natural language deserialization.

Refactored methods to reduce cognitive complexity and improve maintainability.
Removed unused regex patterns related to natural language inputs.
Updated regex timeout constant and added new tests for various RRULE orderings and error cases.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
Ical.Net/Serialization/DataTypes/RecurrencePatternSerializer.cs	Refactored deserialization logic, removed natural language regexes, and added helper methods.
Ical.Net/DataTypes/RecurrencePattern.cs	Added XML documentation for the BySetPosition property.
Ical.Net/Constants.cs	Increased the regex timeout from 200ms to 2000ms.
Ical.Net.Tests/RecurrenceTests.cs	Added tests validating RRULE property order, missing FREQ handling, and unsupported parts.

Comments suppressed due to low confidence (1)

Ical.Net/Constants.cs:345

Increasing the timeout from 200ms to 2000ms may have performance implications in parts of the system that rely on time-bound regex operations. Confirm that this change is acceptable for all affected components.

public const int TimeoutMilliseconds = 2000;

Copilot · 2025-03-31T12:46:31Z

Ical.Net/Serialization/DataTypes/RecurrencePatternSerializer.cs

+        System.Diagnostics.Debug.Assert(r != null);
+        System.Diagnostics.Debug.Assert(factory != null);



[nitpick] Using Debug.Assert for null checks may not enforce the condition in Release builds. Consider using an explicit exception or a proper null-check to ensure robust error handling in production.

Suggested change

System.Diagnostics.Debug.Assert(r != null);

System.Diagnostics.Debug.Assert(factory != null);

if (r == null)

{

throw new InvalidOperationException("Failed to create RecurrencePattern.");

}

if (factory == null)

{

throw new InvalidOperationException("Serializer factory service is not available.");

}

axunonb requested a review from Copilot March 30, 2025 16:10

Copilot AI reviewed Mar 30, 2025

View reviewed changes

axunonb force-pushed the axunonb/wip/pr/rec-pat-serializer branch 2 times, most recently from 5717fd6 to 191d839 Compare March 30, 2025 18:19

Enhance RecurrencePatternSerializer

1000abc

- RRULE parts are now processed in any order, which is compliant to RFC5545 section 3.3.10. - Refactor methods with high cognitive complexity - Split deserialization of RFC-compliant and natural language input

axunonb force-pushed the axunonb/wip/pr/rec-pat-serializer branch from 191d839 to 1000abc Compare March 30, 2025 18:20

Remove natural language syntax from RecurrencePatternSerializer

e030fe7

`

axunonb requested a review from Copilot March 30, 2025 18:51

Copilot AI reviewed Mar 30, 2025

View reviewed changes

axunonb marked this pull request as ready for review March 30, 2025 18:54

axunonb requested a review from minichma March 30, 2025 18:54

minichma reviewed Mar 30, 2025

View reviewed changes

minichma previously approved these changes Mar 30, 2025

View reviewed changes

Remarks from Review implemented

58895fa

axunonb dismissed minichma’s stale review via 58895fa March 31, 2025 12:41

axunonb requested review from Copilot and minichma March 31, 2025 12:46

Copilot AI reviewed Mar 31, 2025

View reviewed changes

minichma approved these changes Mar 31, 2025

View reviewed changes

axunonb merged commit 65dc5ef into main Mar 31, 2025
7 of 8 checks passed

axunonb deleted the axunonb/wip/pr/rec-pat-serializer branch March 31, 2025 12:53

axunonb restored the axunonb/wip/pr/rec-pat-serializer branch March 31, 2025 12:53

axunonb deleted the axunonb/wip/pr/rec-pat-serializer branch March 31, 2025 12:53

dependabot bot mentioned this pull request Jul 28, 2025

Bump the all-updates group with 4 updates mikkelkaas/HaandboldGrabber#13

Closed

This was referenced Jul 28, 2025

Bump Ical.Net from 4.3.1 to 5.1.0 Atrejoe/Inky-Calendar-Server#243

Closed

Bump Ical.Net and 5 others Atrejoe/Inky-Calendar-Server#245

Closed

dependabot bot mentioned this pull request Aug 18, 2025

Bump the all-updates group with 4 updates mikkelkaas/HaandboldGrabber#14

Merged

dependabot bot mentioned this pull request Aug 25, 2025

Bump Ical.Net from 4.3.1 to 5.1.0 Atrejoe/Inky-Calendar-Server#246

Closed

dependabot bot mentioned this pull request Sep 1, 2025

Bump Ical.Net from 4.3.1 to 5.1.0 Atrejoe/Inky-Calendar-Server#253

Closed

dependabot bot mentioned this pull request Oct 13, 2025

Bump Ical.Net from 4.3.1 to 5.1.1 Atrejoe/Inky-Calendar-Server#257

Open

This was referenced Oct 23, 2025

Bump Ical.Net from 4.3.1 to 5.1.1 joinrpg/joinrpg-net#3675

Closed

Bump Ical.Net from 4.3.1 to 5.1.1 loresoft/InstructorIQ#379

Merged

Bump Ical.Net from 4.3.1 to 5.1.1 joinrpg/joinrpg-net#3707

Closed


		private const RegexOptions CiCompiled = RegexOptions.IgnoreCase \| RegexOptions.Compiled;

		internal static readonly Regex FrequencyPattern =

		System.Diagnostics.Debug.Assert(r != null);
		System.Diagnostics.Debug.Assert(factory != null);

-        System.Diagnostics.Debug.Assert(r != null);
-        System.Diagnostics.Debug.Assert(factory != null);
+        if (r == null)
+        {
+            throw new InvalidOperationException("Failed to create RecurrencePattern.");
+        }
+        if (factory == null)
+        {
+            throw new InvalidOperationException("Serializer factory service is not available.");
+        }

Enhance RecurrencePatternSerializer #758

Enhance RecurrencePatternSerializer #758

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

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

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

minichma commented Mar 30, 2025

Uh oh!

axunonb commented Mar 30, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Mar 30, 2025

Choose a reason for hiding this comment

Uh oh!

minichma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axunonb commented Mar 31, 2025

Uh oh!

sonarqubecloud bot commented Mar 31, 2025

Quality Gate passed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Mar 31, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Enhance `RecurrencePatternSerializer` #758

Enhance `RecurrencePatternSerializer` #758

axunonb commented Mar 30, 2025 •

edited

Loading

codecov bot commented Mar 30, 2025 •

edited

Loading

axunonb commented Mar 30, 2025 •

edited

Loading