Last Comment Bug 734245 - Thunderbird/Lightning ignores EXDATEs of type DATE when the DTSTART is of type DATE-TIME
: Thunderbird/Lightning ignores EXDATEs of type DATE when the DTSTART is of typ...
Status: RESOLVED FIXED
[calconnect25]
:
Product: Calendar
Classification: Client Software
Component: Internal Components (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: 2.2
Assigned To: Manuel Schneider
:
:
Mentors:
Depends on: 828754
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-08 14:02 PST by Manuel Schneider
Modified: 2013-01-09 18:10 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (1.67 KB, patch)
2012-03-08 14:04 PST, Manuel Schneider
philipp: review+
Details | Diff | Splinter Review
korganizer generated ics (2.48 KB, text/plain)
2012-03-13 16:17 PDT, Manuel Schneider
no flags Details
the new patch (3.91 KB, patch)
2012-04-03 05:30 PDT, Manuel Schneider
philipp: review+
Details | Diff | Splinter Review
Fix typo (1.64 KB, patch)
2012-12-06 20:08 PST, Matthew Mecca [:mmecca]
philipp: review+
Details | Diff | Splinter Review

Description Manuel Schneider 2012-03-08 14:02:27 PST
Hi,


I think the following behaviour is not intended:
When parsing Ical, Thunderbird/Lightning ignores EXDATEs
of type DATE when the DTSTART is of type DATE-TIME.

The RFC states that EXDATE may be DATE or DATE-TIME. My
interpretation is: Whenever an EXDATE is of type DATE,
for deciding if the EXDATE matches only the DATE-part
should be compared.

I've created a patch to fix this, it's attached.

Unfortunately I cannot think of an implementation that is
more performant without changing the occurrenceMap itself,
due to the current representation of occurrenceMap.

I'd really appreciate to see this bug fixed :o)


Yours,
Manuel
Comment 1 Manuel Schneider 2012-03-08 14:04:23 PST
Created attachment 604196 [details] [diff] [review]
patch
Comment 2 Matthew Mecca [:mmecca] 2012-03-08 21:00:34 PST
(In reply to Manuel Schneider from comment #0)
> The RFC states that EXDATE may be DATE or DATE-TIME. My
> interpretation is: Whenever an EXDATE is of type DATE,
> for deciding if the EXDATE matches only the DATE-part
> should be compared.

From http://tools.ietf.org/html/rfc5545#section-3.8.5.1

      The final recurrence set is generated by gathering all of the
      start DATE-TIME values generated by any of the specified "RRULE"
      and "RDATE" properties, and then excluding any start DATE-TIME
      values specified by "EXDATE" properties.  This implies that start
      DATE-TIME values specified by "EXDATE" properties take precedence
      over those specified by inclusion properties (i.e., "RDATE" and
      "RRULE").

My interpretation is that an occurrence should match the EXDATE exactly if it is to be excluded, not just the date part. DATE is allowed as a type for EXDATE for the case of items where the DTSTART property is of also of the type DATE, which propagates to the calculated recurrence set.
Comment 3 Manuel Schneider 2012-03-09 06:52:52 PST
The problem is that the RFC simply does not specify how DATE should be handled,
if we're not overseeing anything.

> My interpretation is that an occurrence should match the EXDATE exactly
> if it is to be excluded, not just the date part.

I think semantically an EXDATE specified as DATE could match an occurrence
specified as DATE-TIME exactly, the only missing link is a type-casting
from DATE to multiple DATE-TIME values. The other way round won't work of
course. 

My point is it *can* happen and will be perfectly valid as it is not properly
defined in the RFC (as far as we're not overseeing some passage). In fact,
afaik eg horde and korganizer read and write EXDATES in ics the way I proposed.
The logical conclusion for me is that the parser has to accept it. Although
the way you write ICS may of course be more specific.


Yours,
Manuel
Comment 4 Matthew Mecca [:mmecca] 2012-03-11 13:55:10 PDT
Can you detail the problem that this is designed to fix, maybe post an example of an event created by the client software you mentioned, and explain how that application handles it differently?

I think one of the risks in inferring too much from a vague part of the spec is that we may inadvertently break compatibility with one client or server while trying to be more compatible with another. I think it would be a good idea to test how a few others handle this type of EXDATE, if it turns out that it's widely supported then I would agree that we should make the change.
Comment 5 Manuel Schneider 2012-03-13 16:15:55 PDT
> Can you detail the problem that this is designed to fix,
> maybe post an example of an event created by the client
> software you mentioned, and explain how that application
> handles it differently?

Of course :) I've attached a file written by korganizer:
It contains a DATE EXDATE within an event defined by a
DATE-TIME DTSTART.
I've put it in Horde, and it displays the event with the
excluded occurence missing.
Horde's Caldav-Service returns (with slight changes
concerning the VALUE= representation) again a DATE for
the EXDATE -> put it back in korganizer and it works.

> I think one of the risks in inferring too much from a
> vague part of the spec is that we may inadvertently break
> compatibility with one client or server while trying to be
> more compatible with another.

I fully agree with you - we should not break something. But
the only case I could think of breaking is EXDATEs specified
to be ignored intentionally - which is ****. At least
I cannot think of a case in which anyone would like to do this.
You don't specify something that should be ignored...

> I think it would be a good idea to test how a few others
> handle this type of EXDATE, if it turns out that it's widely
> supported then I would agree that we should make the change.

I don't think it has to widely supported, because I don't think it'll 
break anything.


Yours,
Manuel
Comment 6 Manuel Schneider 2012-03-13 16:17:00 PDT
Created attachment 605585 [details]
korganizer generated ics
Comment 7 Matthew Mecca [:mmecca] 2012-03-15 07:01:11 PDT
(In reply to Manuel Schneider from comment #5)
> I fully agree with you - we should not break something. But
> the only case I could think of breaking is EXDATEs specified
> to be ignored intentionally - which is ****. At least
> I cannot think of a case in which anyone would like to do this.
> You don't specify something that should be ignored...

I see your point.

The only ambiguous case I can think of would involve a combination of an RRULE in an item with a DTSTART of type DATE, and an RDATE of type DATE-TIME that would create multiple occurrences on a given day. An EXDATE with type DATE could be interpreted to apply only to the occurrence created by the RRULE, with this patch both would be excluded.

That said, I think a case like that is pretty remote, so this should be a fairly safe fix. As far as I can tell this shouldn't affect any items as currently generated by Lightning.
Comment 8 Matthew Mecca [:mmecca] 2012-03-15 20:00:27 PDT
Comment on attachment 604196 [details] [diff] [review]
patch

Review of attachment 604196 [details] [diff] [review]:
-----------------------------------------------------------------

A few comments on the patch, I'll pass review to Philipp.

I think the getNextOccurrence function should be updated as well - see 
http://mxr.mozilla.org/comm-central/source/calendar/base/src/calRecurrenceInfo.js#389

A few minor nits:

::: calendar/base/src/calRecurrenceInfo.js
@@ +586,5 @@
>              // check exceptions here
>              for each (var dateToRemove in cur_dates) {
>                  var dateToRemoveKey = getRidKey(dateToRemove);
> +                if (dateToRemove.isDate) {
> +                    var toRemove = [];

Please use let instead of var, here and elsewhere in the patch.

@@ +593,5 @@
> +                            dates = dates.filter(function (d) { return d.id.compare(dateToRemove) != 0; });
> +                            toRemove.push(occurenceKey);
> +                        }
> +                    }
> +                    for (var i=0;i<toRemove.length;i++) {

Please use blanks around operators and after semi-colon
 (see https://wiki.mozilla.org/Calendar:Style_Guide)
Comment 9 Philipp Kewisch [:Fallen] 2012-03-16 02:01:27 PDT
Comment on attachment 604196 [details] [diff] [review]
patch

Review of attachment 604196 [details] [diff] [review]:
-----------------------------------------------------------------

Aside from what Matthew mentioned I'm fine with the patch. I think you should add a comment to this part of the code, explaining that the rfc doesn't clearly specify how to handle EXDATE with VALUE=DATE and to be compatible to certain servers we chose to handle them the way you have mentioned.

I'd appreciate if you could upload a new patch with the comment added and nits fixed. Added bonus for formatting the commit message as "Fix bug 734245 - <title of bug>. r=philipp,mmecca", this way I can commit it without changing anything :-)
Comment 10 Stefan Sitter 2012-03-16 02:20:20 PDT
I'd appreciate a patch that adds unit test cases too.
Comment 11 Manuel Schneider 2012-04-03 05:30:08 PDT
Created attachment 611783 [details] [diff] [review]
the new patch
Comment 12 Manuel Schneider 2012-04-03 05:32:50 PDT
Hi,


I've attached a new patch, which additionally includes a fix for getNextOccurrence and 
a unit-test.

I hope I catched all nits you found.


Yours,
Manuel
Comment 13 Philipp Kewisch [:Fallen] 2012-09-29 12:31:12 PDT
I'm going to discuss this at calconnect to see what the rfc gurus say.
Comment 14 Martin Hochreiter 2012-10-01 02:24:14 PDT
Hi, is my case the same issue?


I have this appointment in the ics and the exdates are ignored by lightning:
BEGIN:VEVENT
DTSTART:20111011T160000Z
DTEND:20111011T160100Z
DTSTAMP:20120930T125054Z
UID:d9378f03-a639-466f-9e36-fa598e15b702
CREATED:20110827T181238Z
LAST-MODIFIED:20120926T083910Z
SUMMARY:NAW-Dienst
CATEGORIES:RK
LOCATION:NAW
CLASS:PUBLIC
STATUS:CONFIRMED
TRANSP:OPAQUE
RRULE:FREQ=WEEKLY;INTERVAL=2;BYDAY=TU
EXDATE:20121022T220000Z,20121008T220000Z,20120813T220000Z,20120521T220000Z,
20120507T220000Z,20120312T230000Z,20120326T220000Z,20120326T220000Z,2012031
2T230000Z,20120312T230000Z,20120326T220000Z,20120326T220000Z,20120521T22000
0Z,20120521T220000Z,20121008T220000Z,20121022T220000Z
END:VEVENT
Comment 15 Manuel Schneider 2012-10-01 15:12:35 PDT
Hi Martin,

(In reply to Martin Hochreiter from comment #14)
> Hi, is my case the same issue?

Unfortunately I don't think so:

> DTSTART:20111011T160000Z
..
> EXDATE:20121022T220000Z,20121008T220000Z,20120813T220000Z,20120521T220000Z,
> 20120507T220000Z,20120312T230000Z,20120326T220000Z,20120326T220000Z,2012031
> 2T230000Z,20120312T230000Z,20120326T220000Z,20120326T220000Z,20120521T22000
> 0Z,20120521T220000Z,20121008T220000Z,20121022T220000Z

I think ignoring the exdates is correct here, as your date is time-associated
(*16..Z) and your Exdates are also time-associated - but with another time 
(*22..Z) -> It's simply not matching (also with the 'type-casting' I proposed).


Yours,
Manuel
Comment 16 Martin Hochreiter 2012-10-01 22:23:05 PDT
The .ics is from the horde-kronolith calendar and in the horde-webmail the dates are correctly excluded - only lightning still shows the excluded appointments.

ok, if i understand your statement correctly, i should address to the kronolith guys to resolve that?

regards
Martin
Comment 17 Philipp Kewisch [:Fallen] 2012-11-29 13:48:09 PST
Comment on attachment 611783 [details] [diff] [review]
the new patch

Review of attachment 611783 [details] [diff] [review]:
-----------------------------------------------------------------

r=philipp, I will take care of the review comments myself.

::: calendar/base/src/calRecurrenceInfo.js
@@ +387,5 @@
>                  }
> +                
> +                // As decided in bug 734245, an EXDATE of type DATE shall also match a DTSTART of type DATE-TIME
> +                if ((getRidKey(nextOccurrences[i]) && (negMap[getRidKey(nextOccurrences[i]).substring(0,8)] || this.mExceptionMap[getRidKey(nextOccurrences[i]).substring(0,8)])) ||
> +                    negMap[getRidKey(nextOccurrences[i])] || this.mExceptionMap[getRidKey(nextOccurrences[i])]) {

It would be nice to split this into multiple lines.
Comment 18 Philipp Kewisch [:Fallen] 2012-11-29 13:50:12 PST
Pushed to comm-central changeset b8dcd1c378b4
Comment 19 Matthew Mecca [:mmecca] 2012-12-06 20:06:56 PST
Checked in patch causes the following error in some cases:

Error: nextOccurences is not defined
Source file: resource://calendar/modules/calUtils.jsm -> file:///.../calendar-js/calRecurrenceInfo.js
Line: 355
Comment 20 Matthew Mecca [:mmecca] 2012-12-06 20:08:24 PST
Created attachment 689547 [details] [diff] [review]
Fix typo

Fix typo in nextOccurrences
Comment 21 Philipp Kewisch [:Fallen] 2012-12-06 23:28:27 PST
Comment on attachment 689547 [details] [diff] [review]
Fix typo

Review of attachment 689547 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for catching this, r=philipp
Comment 22 Matthew Mecca [:mmecca] 2012-12-08 09:48:38 PST
Pushed to comm-central - https://hg.mozilla.org/comm-central/rev/76bb339cdf63

Note You need to log in before you can comment on or make changes to this bug.