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

Refactor: Expand recurring events until end date is reached #48

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Refactor: Expand recurring events until end date is reached #48

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
9806624
fix(parser): expand recurring events until end date is reached
antosan May 20, 2021
f83a58b
chore(core): update dependencies
antosan May 27, 2021
347a51f
fix(parser): allow user to specify upper iteration bound
antosan Jul 9, 2021
9dee7b0
chore(core): update dependencies
antosan Jul 9, 2021
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
47 changes: 29 additions & 18 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,24 +11,34 @@ Specify basic configuration:
config = {
auth: {
user: "username",
pass: "password"
pass: "password",
},
// example using baikal as CalDAV server
uri: "http://example.com/cal.php/calendars/<user name>/<calendar name>"
uri: "http://example.com/cal.php/calendars/<user name>/<calendar name>",
events: {
maxExpandCount: 10,
},
};
```

The request will timeout if it gets no reponse from the CalDav server after 10 seconds.
`config.events.maxExpandCount` is a required parameter that receives a positive number which defines how many upcoming
occurrences of each recurring event should be expanded. This prevents the possibility of an almost infinite loop.
However, if you don't care about running into an infinite loop, you can set it to `Infinity`.

The request will timeout if it gets no response from the CalDav server after 10 seconds.
An optional `timeout` parameter can be provided to override this default by passing an integer containing the number of milliseconds to wait for the server to send the response before aborting the request.

```javascript
config = {
auth: {
user: "username",
pass: "password"
pass: "password",
},
uri: "http://example.com/cal.php/calendars/<user name>/<calendar name>",
timeout: 20000
events: {
maxExpandCount: 10,
},
timeout: 20000,
};
```

Expand Down Expand Up @@ -70,12 +80,12 @@ You'll get an array of objects, which looks like this:
[
{
ics: "/cal.php/calendars/test/holidays/6151613161614616.ics",
etag: "fc46dd304e83f572688c68ab63816c8f"
etag: "fc46dd304e83f572688c68ab63816c8f",
},
{
ics: "/cal.php/calendars/test/holidays/6816189165131651.ics",
etag: "8d59671ba294af1de0e0b154a8ea64c2"
}
etag: "8d59671ba294af1de0e0b154a8ea64c2",
},
];
```

Expand Down Expand Up @@ -112,12 +122,12 @@ Output should be something like this:
hours: 0,
minutes: 0,
seconds: 0,
isNegative: false
isNegative: false,
},
type: { recurring: false, edited: false },
createdAt: "2017-01-24T15:33:04.000Z"
}
}
createdAt: "2017-01-24T15:33:04.000Z",
},
},
];
```

Expand All @@ -132,17 +142,18 @@ If you leave `start` and `end` out, you'll get all upcoming events from today.
Passing only one date as a parameter returns all upcoming events from that date.
The end-date must be larger that the start-date.

NOTE: The provided `config.events.maxExpandCount` config property takes precedence over the `end` date when expanding
recurring events. The event expansion will be terminated once `config.events.maxExpandCount` is reached. If you wish to
expand all events until the end date you can set `config.events.maxExpandCount` to a really high number, like `3650`
(this would expand an event that recurs every day for 10 years) or set it to `Infinity`.

Example using [moment.js](http://momentjs.com/) for date formatting:

```javascript
const moment = require("moment");

const start = moment()
.startOf("month")
.format("YYYYMMDD[T]HHmmss[Z]");
const end = moment()
.endOf("month")
.format("YYYYMMDD[T]HHmmss[Z]");
const start = moment().startOf("month").format("YYYYMMDD[T]HHmmss[Z]");
const end = moment().endOf("month").format("YYYYMMDD[T]HHmmss[Z]");

scrapegoat.getEventsByTime(start, end).then(console.log);
```
Expand Down
27 changes: 15 additions & 12 deletions lib/Calendar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,15 @@ function createCalendar(request) {
if (!config) {
throw new Error("Missing config object");
}

if (!config.events || config.events.maxExpandCount === undefined) {
throw new Error("Missing config.events.maxExpandCount");
}

if (typeof config.events.maxExpandCount !== "number" || config.events.maxExpandCount <= 0) {
throw new TypeError("config.events.maxExpandCount should be a positive number or Infinity");
}

this.config = config;
}

Expand Down Expand Up @@ -50,7 +59,7 @@ function createCalendar(request) {
const filteredEvents = events.filter((event) => event.ics && event.ics.endsWith(".ics"));
const multiget = multigetTemplate({ gets: filteredEvents });

return request(this.config, "REPORT", 1, multiget).then(xmlParser.parseEvents);
return request(this.config, "REPORT", 1, multiget).then((xml) => xmlParser.parseEvents(xml, this.config.events.maxExpandCount));
}

/**
Expand All @@ -59,7 +68,7 @@ function createCalendar(request) {
* @returns {Promise}
*/
getAllEvents() {
return request(this.config, "REPORT", 1, xml.calendarQuery).then(xmlParser.parseEvents);
return request(this.config, "REPORT", 1, xml.calendarQuery).then((xml) => xmlParser.parseEvents(xml, this.config.events.maxExpandCount));
}

/**
Expand All @@ -82,21 +91,15 @@ function createCalendar(request) {

if (Boolean(end) && moment(end).isSameOrBefore(start)) {
// CalDAV requires end-date to be larger than start-date
end = null;
throw new Error("End date must be after start date");
}

// The returned events from `byTimeTemplate` are already filtered out by start and end date (if provided)
// and only include recurring events that start (or have an occurrence) between the start and end date
const xmlRequest = byTimeTemplate({ start, end });

return request(this.config, "REPORT", 1, xmlRequest)
.then(xmlParser.parseEvents)
.then(events => {
return events.filter(event => {
const isNotRecurring = !event.data.type.recurring;
const isSameDayOrAfter = moment(event.data.start).isSameOrAfter(start, "day");

return isNotRecurring || isSameDayOrAfter;
});
});
.then((xml) => xmlParser.parseEvents(xml, this.config.events.maxExpandCount, start, end));
}
}

Expand Down
88 changes: 44 additions & 44 deletions lib/xml/parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ function getNormalizedEndDate(endDate, duration) {
endDate.toISOString();
}

function getNormalOccurenceEventData(nextEvent, eventData, vevent) {
function getNormalOccurenceEventData(nextEvent, vevent) {
const dtstart = nextEvent.startDate;
const dtend = nextEvent.endDate;
const { summary: title, uid, location, description } = nextEvent.item;
Expand All @@ -118,7 +118,7 @@ function getNormalOccurenceEventData(nextEvent, eventData, vevent) {
};
}

function getModifiedOccurenceEventData(vevent, eventData) {
function getModifiedOccurenceEventData(vevent) {
const dtstart = vevent.getFirstPropertyValue("dtstart");
const dtend = vevent.getFirstPropertyValue("dtend");
const duration = getNormalizedDuration(dtstart, dtend);
Expand All @@ -136,7 +136,7 @@ function getModifiedOccurenceEventData(vevent, eventData) {
};
}

function parseEvents(xml) {
function parseEvents(xml, maxExpandCount, startDate = null, endDate = null) {
let parsed;
const formatted = [];

Expand All @@ -152,8 +152,6 @@ function parseEvents(xml) {

etag = stripDoubleQuotes(etag);

let eventData = {};

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for cleaning this up :)

if (!event.propstat[0].prop[0]["calendar-data"]) {
return;
}
Expand All @@ -178,48 +176,51 @@ function parseEvents(xml) {
const icalEvent = new ICAL.Event(vevent);

if (icalEvent.isRecurring()) {
// This is a recurring event, expand the next few occurances
const expand = new ICAL.RecurExpansion({
component: vevent,
dtstart: vevent.getFirstPropertyValue("dtstart"),
});
const startRange = startDate ? ICAL.Time.fromDateTimeString(moment(startDate).toISOString()) : null;
const endRange = endDate ? ICAL.Time.fromDateTimeString(moment(endDate).toISOString()) : null;
let occurrencesCount = 0;

for (let next = expand.next(); next; next = expand.next()) {
// If max iteration count is reached: break
if (occurrencesCount >= maxExpandCount) {
break;
}

// If date is after end date: break
if (endRange && next.compare(endRange) > 0) {
break;
}

// Since there are infinite rules, its a good idea to limit the scope
// of the iteration then resume later on
// Expand upto a maximum of 10 upcoming occurances
for (let i = 0; i < 10; i++) {
const nextOccuranceTime = expand.next();

if (!expand.complete) {
// Handle this next expanded occurence
const nextEvent = icalEvent.getOccurrenceDetails(nextOccuranceTime);

eventData = {};

if (modifiedOccurences.length === 0) {
// No events have been modified
formatted.push({
ics: event.href[0],
etag,
data: getNormalOccurenceEventData(nextEvent, eventData, vevent),
});
} else if (isModified(nextOccuranceTime, modifiedOccurences)) {
// This is the event that has been modied
const key = getModifiedOccuranceKey(nextOccuranceTime, modifiedOccurences) || 0;

formatted.push({
ics: event.href[0],
etag,
data: getModifiedOccurenceEventData(vevents[key], eventData),
});
} else {
// Expand this event normally
formatted.push({
ics: event.href[0],
etag,
data: getNormalOccurenceEventData(nextEvent, eventData, vevent),
});
}
// If date is before start date: continue
if (startRange && next.compare(startRange) < 0) {
continue;
}
Copy link
Member

Choose a reason for hiding this comment

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

I like how the break/continue conditions are at the beginning of the for loop 👍

Personally, I would've reached for a while loop here:

while (next = expand.next()) {
  // do something with next
}

but that's just a minor stylistic preference.

Logic-wise I'm not 100% sure if it's a good idea to include the possibility of an almost infinite loop. The problem here is that the library user of scrapegoat has no way to limit the iteration count if bad data is passed to the parser. Personally I'd prefer the library user to specify the upper iteration bound, like maxExpandCount. If they don't care about this issue, they still can pass maxExpandCount: Infinity. I also think that this upper iteration bound should be applied to all events. It also would be a required parameter which forces you to think about this possibility.

Furthermore I think there's an opportunity for improvement here: we could remove all recurring events that start after the specified end date. That would save us some iterations.

So my preferred logic would be:

  • Remove all events that start after endRange (if specified)
  • Then iterate:
    • If max iteration count is reached: break
    • If date is after end date: break
    • If date is before start date: continue

Maybe you can also add an error if startRange is after endRange.

What do you think? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for raising the almost infinite loop possibility 👍 The only case where I see an almost infinite loop occurring is when an event is finite but has an end date (or count) that is way too far in the future (for infinite events we are already capping the iterations to 10). Are we seeing this the same way?

I like the idea of the library user specifying the upper iteration bound 👍 If I understand you well, I will include maxExpandCount in the config options provided by the user as a required parameter and pass it to all events:

const config = {
    auth: {
        user: BAIKAL_USER,
        pass: BAIKAL_PASSWORD,
    },
    uri: BAIKAL_URI,
    maxExpandCount: 10,
};

Furthermore I think there's an opportunity for improvement here: we could remove all recurring events that start after the specified end date. That would save us some iterations.

This situation does not exist. When we query CalDav with an end date specified, the response we get only includes recurring events that start (or have an occurrence) between the startRange and endRange.

If they don't care about this issue, they still can pass maxExpandCount: Infinity.

Here is where It gets blurry again: (going with the logic you suggested)

CASE 1: startRange and endRange provided and maxExpandCount: Infinity

  • If the endRange is near the startRange, then all is well. However, if the endRange is way into the future, we have the same possibility of an almost infinite loop, only that the user opted into it - this is OK

CASE 2: startRange provided, endRange not provided and maxExpandCount: Infinity

  • What happens here? This will loop indefinitely... (at least until the year 10000 breaks the JS Date 😄) I do not see a use case of maxExpandCount being set to Infinity, it should be a finite number always.
    What do you think?

Maybe you can also add an error if startRange is after endRange.

Right now we silently change the endRange to null if this case happens but I agree with throwing an error 👍

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for raising the almost infinite loop possibility 👍 The only case where I see an almost infinite loop occurring is when an event is finite but has an end date (or count) that is way too far in the future (for infinite events we are already capping the iterations to 10). Are we seeing this the same way?

Yes, I was referring to this case. I don't know how common that is. But I know that some tools limit the max iteration count or the end date when creating new re-occurring events.

I think I would scope the config option under events:

const config = {
    auth: {
        user: BAIKAL_USER,
        pass: BAIKAL_PASSWORD,
    },
    uri: BAIKAL_URI,
    events: {
		maxExpandCount: 10,
    }
};

This situation does not exist. When we query CalDav with an end date specified, the response we get only includes recurring events that start (or have an occurrence) between the startRange and endRange.

Ok, I didn't know that. Thanks for clarifying. You could add a comment in the code that explains this.

About the logic behind maxExpandCount:

I think maxExpandCount should be a finite number by default and should be applied to all situations, even when there's a start and an end date. Specifying maxExpandCount gives you the guarantee that scrapegoat won't expand events above a given limit, even when the end date is far in the future. Personally I think using Infinity for "Don't apply any limit" is ok, but I'm also ok with using undefined. In both cases, it needs to be explained in the README. I would use a finite number by default because often developers don't think about these edge cases. But I think the number can be really high, like 5.000 (this would include a reoccurring event every day for 10 years).

Regarding your concern when an endDate is missing: I think it's ok to have the possibility for an infinite loop. Since the default for maxExpandCount is a finite number, you need disable this safety net explicitly. If you do that, you're on your own. Again, I think there should be a warning in the README.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, thanks for the clarification. This is now implemented and ready for review.


occurrencesCount += 1;

// Handle this next expanded occurence
const nextEvent = icalEvent.getOccurrenceDetails(next);

if (modifiedOccurences.length && isModified(next, modifiedOccurences)) {
// This event has been modified
const key = getModifiedOccuranceKey(next, modifiedOccurences);

formatted.push({
ics: event.href[0],
etag,
data: getModifiedOccurenceEventData(vevents[key]),
});
} else {
// Expand this event normally
formatted.push({
ics: event.href[0],
etag,
data: getNormalOccurenceEventData(nextEvent, vevent),
});
}
}
} else {
Expand All @@ -228,8 +229,7 @@ function parseEvents(xml) {
const dtstart = vevent.getFirstPropertyValue("dtstart");
const dtend = vevent.getFirstPropertyValue("dtend");
const duration = getNormalizedDuration(dtstart, dtend);

eventData = {
const eventData = {
title: vevent.getFirstPropertyValue("summary"),
uid: vevent.getFirstPropertyValue("uid"),
location: vevent.getFirstPropertyValue("location"),
Expand Down
Loading