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.

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

Get Occurrences fails on a RRULE FREQ=SECONDLY #622

Closed
axunonb opened this issue Oct 22, 2024 Discussed in #615 · 6 comments · Fixed by #627
Closed

Get Occurrences fails on a RRULE FREQ=SECONDLY #622

axunonb opened this issue Oct 22, 2024 Discussed in #615 · 6 comments · Fixed by #627
Labels

Comments

@axunonb
Copy link
Collaborator

axunonb commented Oct 22, 2024

Discussed in #615

Originally posted by DaleyKD October 18, 2024
Does this library support getting occurrences if we use a secondly recurrence pattern?

Our project doesn't use this for calendar events, but more to help us determine when to run the next job (as part of our job scheduler). And SOME jobs need to run every 45 seconds or so.

I've been trying to get it to work, but have had no luck. It seems to work for FREQ=MINUTELY.

@axunonb axunonb added the bug label Oct 22, 2024
@axunonb
Copy link
Collaborator Author

axunonb commented Oct 23, 2024

@DaleyKD, @minichma

Here is temporary workaround for RRULE FREQ=SECONDLY

[Test]
public void SecondlyRecurrence()
{
    var dateTime = DateTime.UtcNow;

    // Create a new calendar
    var calendar = new Calendar
    {
        RecurrenceRestriction = RecurrenceRestrictionType.NoRestriction
    };

    var calendarEvent = new CalendarEvent
    {
        Summary = "Recurring Event Every 45 Seconds",
        DtStart = new CalDateTime(dateTime),
        DtEnd = new CalDateTime(dateTime.AddHours(1)),
        RecurrenceRules = new List<RecurrencePattern>
        {
            new RecurrencePattern(FrequencyType.Secondly, 45)
        }
    };

    // This does not seem to add any reference to the parent `Calendar`,
    // Services inside `CalendarEvent` and its children should be updated
    calendar.Events.Add(calendarEvent);

    // Serialize the calendar to an iCalendar string
    var serializer = new CalendarSerializer();
    var serializedCalendar = serializer.SerializeToString(calendar);

    /*************************************************************
     *  This is a temporary workaround to make GetOccurrences work
     *  with the RecurrenceRestriction set to NoRestriction.
     *************************************************************/
    calendar = Calendar.Load(serializedCalendar);
    // This setting does not deserialize, so we have to set again
    calendar.RecurrenceRestriction = RecurrenceRestrictionType.NoRestriction;

    // Evaluate the recurrences
    var occurrences = calendar.GetOccurrences(dateTime, dateTime.AddHours(1));

    Assert.That(occurrences, Has.Count.EqualTo(80));
}

@DaleyKD
Copy link

DaleyKD commented Oct 23, 2024

Wow! I didn't expect a response so quickly.

I'm currently only using the RecurrencePatternEvaluator to get the occurrences. I'll try to use the pattern's restriction policy (I didn't even realize there was one!) when I get back to writing unit tests.

You guys are absolutely knocking it out of the park. Thank you so very much.

@minichma
Copy link
Collaborator

Btw, there's one more limitation that could play a role with SECONDLY freq:

private const int _maxIncrementCount = 1000;

The number of increments between occurrences must be < (or <=) 1000, so something like this might not work as expected:

FREQ=SECONDLY;BYHOUR=1,2,3

because after 3:59:59 the number of second increments until 1:00:00 is > 1000. Shouldn't play a role in case of 45s intervals though.

@axunonb
Copy link
Collaborator Author

axunonb commented Oct 23, 2024

@minichma The recurrence related properties on Calender tend to complicate things and are not fully implemented. When using them, CalendarEvent can't be used standalone. Also, could we get rid of RecurrenceRestrictionType when _maxIncrementCount was configurable? We should discuss/evaluate how to handle that.

@DaleyKD
Copy link

DaleyKD commented Oct 23, 2024

FWIW, for me, this can be a lower priority than whatever else you have in the pipeline. The specific product I'm on right now is only days+. However, when we migrate other products to this, we'll need seconds.

@minichma
Copy link
Collaborator

The recurrence related properties on Calender tend to complicate things and are not fully implemented. When using them, CalendarEvent can't be used standalone. Also, could we get rid of RecurrenceRestrictionType when _maxIncrementCount was configurable? We should discuss/evaluate how to handle that.

Fully agree. I tend to think that its not the lib's task to define which kind of restrictions on performance are acceptable for the user. So I would probably remove the RecurrenceRestrictionType altogether. Its also not quite easy to understand what it is doing. The maxIncrement probably makes sense but in my opinion should default to indefinite.

axunonb added a commit to axunonb/ical.net that referenced this issue Oct 27, 2024
Modified Constants.cs:
- Deprecated `RecurrenceRestrictionType` and `RecurrenceEvaluationModeType` enums.

Updated RecurrencePattern.cs:
- Deprecated `RestrictionType` and `EvaluationMode` properties.
- Set default for `RestrictionType`.

Enhanced RecurrenceUtil:
- Added new using directives.
- Introduced `DefaultTimeout`.
- Updated `GetOccurrences` with timeout logic.

Refined RecurrencePatternEvaluator:
- Removed `_maxIncrementCount` with connected logic in favor of `RecurrenceUtil.DefaultTimeout`
- Improved XML documentation comments.

Updated Calendar.cs:
- Simplified `Load` method.
- Deprecated certain properties and methods associated with `RestrictionType` and `EvaluationMode`
- Removed obsolete `Evaluate` method.

Enhanced RecurrenceTests.cs with more robust test cases:
- Replaced `Assert.That` with `Assert.Multiple`.
- Changed assertion for occurrences count.
- Renamed test methods for clarity.
- Added timeout settings and updated expected exceptions.
- Adjusted test data and added comments.

Corrected typos and improved formatting for better readability.

Fixes ical-org#622
axunonb added a commit to axunonb/ical.net that referenced this issue Oct 28, 2024
- Deprecated `RecurrenceRestrictionType` and `RecurrenceEvaluationModeType` enums.

Updated RecurrencePattern.cs:
- Deprecated `RestrictionType` and `EvaluationMode` properties.
- Set default for `RestrictionType`.

Enhanced RecurrenceUtil:
- Added new using directives.
- Introduced `DefaultTimeout`.
- Updated `GetOccurrences` with timeout logic.

Refined RecurrencePatternEvaluator:
- Removed `_maxIncrementCount` with connected logic in favor of `RecurrenceUtil.DefaultTimeout`
- Improved XML documentation comments.

Updated Calendar.cs:
- Simplified `Load` method.
- Deprecated certain properties and methods associated with `RestrictionType` and `EvaluationMode`
- Removed obsolete `Evaluate` method.

Enhanced RecurrenceTests.cs with more robust test cases:
- Replaced `Assert.That` with `Assert.Multiple`.
- Changed assertion for occurrences count.
- Renamed test methods for clarity.
- Added timeout settings and updated expected exceptions.
- Adjusted test data and added comments.

Corrected typos and improved formatting for better readability.

Fixes ical-org#622
@axunonb axunonb closed this as completed in 533aeeb Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants