363441 - Same item from different calendars

Status: VERIFIED FIXED

(Calendar :: General, defect)

Description

[Daniel Boelzle [:dbo]]

There may be multiple items having the same id coming from different calendar instances, e.g. in an invitation scenario:
1. Organizer schedules an event, inviting an attendee.
2. Organizer subscribes to the calendar of that attendee.
=> The same items occurs twice in the view (which is ok): the organizer's and the attendee's copy.
The views are having problems, e.g. switching from one week to another, an exception is thrown

"dayHeaderBox.mItemBoxes[i] has no properties" {file: 
"chrome://calendar/content/calendar-multiday-view.xml" line: 
2398}]' when calling method: 
[calIOperationListener::onOperationComplete]" nsresult: "0x80570021 
(NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "JS frame

Comment 1

[Daniel Boelzle [:dbo]]

Attachment: testing whether two items have same id and belong to same calendar instance

I assume that all calendar code currently calling hasSameIds strictly wants to test whether items also belong to the same calendar instance.
Preserving hasSameIds() (we may need it for e.g. sync'ing items from different calendar instances), I have added isSameInstance(), though I still don't like that name... refersToSame()?

Comment 2

[Joey Minta]

Comment on attachment 248251 [details] [diff] [review]
testing whether two items have same id and belong to same calendar instance

This is a pretty significant removal of a previously enforced architectural rule.  Since I don't trust myself to think through all the consequences, I'd like to see a quick newsgroup discussion on the problem/solution.

Initially, I'm concerned about the impact allowing multiple events with identical ids will have on other, id-dependent areas of the code.  off-hand, I know the alarm code could break, the proposed storage-caching will break, storage calendars itself may break, and a previous guard against importing duplicate events will be removed.  It's by no means a trivial amount of work to correct these problems, so I want to make sure that removing the requirement that id's be unique is really what we want.

Comment 3

[Michiel van Leeuwen (email: mvl+moz@)]

Yes, we do want to be able to have the same item in multiple calendars. So I guess we have to do the work, and fix other places that make wrong assumptions.
(The storage cache won't break, as it is per-calendar. The guard for importing need to be rewritten anyway, since the error messages are not at all user friendly)

Comment 6

[Daniel Boelzle [:dbo]]

Requires an UI decision about how e.g. invitation copies should be presented (or merged?).

Comment 7

[Daniel Boelzle [:dbo]]

Attachment: calIItemBase::hashId

calIItemBase::hashId piece, work in progress patch

Comment 8

[Daniel Boelzle [:dbo]]

Attachment: calIItemBase::hashId

Similar approach that philipp has suggested some time ago, but saves the hashed id and computes it a little different.

Comment 9

[Daniel Boelzle [:dbo]]

Attachment: views, todo-list, unifinder

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 274026 [details] [diff] [review]
calIItemBase::hashId

>diff -x CVS -a -U 8 -pN -r /cygdrive/d/pim/mozilla/mozilla_ref/calendar/base/public/calIItemBase.idl mozilla/calendar/base/public/calIItemBase.idl
>+   * Setting either id, recurrenceId or the calendar attribute leads to
>+   * a recomputation of hashId.
>+  readonly attribute AUTF8String hashId;
Is it really that intense to calculate the hashId that we need to store it? I guess it improves performance at least to some extent though, so thats fine with me.

>    * Checks whether the argument object refers the same calendar item as
>    * this one, by testing both the id and recurrenceId property.  This 
>   boolean hasSameIds(in calIItemBase aItem);
I see the comments don't note that this method also checks the calendar, but should it? If so, we can compare hashId's there. Are there any other places in the code where we could use hashId's? At least in the patch I tried to introduce this in it could also be changed. Please either do a quick search for places that use something similar or file a follow-up bug to address the issue.

>+        if (this.mHashId === null) {
>+            var id = encodeURIComponent(this.id);
>+            var rid = this.recurrenceId;
>+            if (rid) {
>+                id += "#"; // some unused delim character
>+                id += rid.getInTimezone("UTC").icalString;
>+            }
>+            id += "#"; // some unused delim character
>+            var cal = this.calendar;
>+            if (cal) {
>+                id += encodeURIComponent(cal.id);
>+            }
>+            this.mHashId = id;
>+        }
>+        return this.mHashId;
>+    },
You could shorten all this to [encodeURIComponent(this.id),rid,encodeURIComponent(cal.id)].join("#"); I don't think it matters if we have an extra # or two in there.

>+        this.mHashId = null; // recompute hashId
I'm not sure if this belongs to calendar style, but all files I have seen have had their comments above the line, no matter how short the comment was.


r+ with the above comments fixed.

Comment 11

[Daniel Boelzle [:dbo]]

(In reply to comment #10)
> >    * Checks whether the argument object refers the same calendar item as
> >    * this one, by testing both the id and recurrenceId property.  This 
> >   boolean hasSameIds(in calIItemBase aItem);
> I see the comments don't note that this method also checks the calendar, but
> should it? If so, we can compare hashId's there. Are there any other places in
> the code where we could use hashId's? At least in the patch I tried to
> introduce this in it could also be changed. Please either do a quick search 
I proposed to change all code that currently uses hasSameIds to use hashId (patches for mickey), because I think hasSameIds is not correctly used there: all those places aggregate items from different calendars. The hashId attribute fixes that and moreover provides a key for associative lookup (advantage over hasSameIds). However hasSameIds may make sense for code to come; so I left it. But we could also remove it, at least until it's needed again.

> >+        if (this.mHashId === null) {
> >+            var id = encodeURIComponent(this.id);
> >+            var rid = this.recurrenceId;
> >+            if (rid) {
> >+                id += "#"; // some unused delim character
> >+                id += rid.getInTimezone("UTC").icalString;
> >+            }
> >+            id += "#"; // some unused delim character
> >+            var cal = this.calendar;
> >+            if (cal) {
> >+                id += encodeURIComponent(cal.id);
> >+            }
> >+            this.mHashId = id;
> >+        }
> >+        return this.mHashId;
> >+    },
> You could shorten all this to
> [encodeURIComponent(this.id),rid,encodeURIComponent(cal.id)].join("#"); I don't
> think it matters if we have an extra # or two in there.
- strictly spoken, those are needed
- we could either go with rid.getInTimezone("UTC").icalString or use encodeURIComponent(rid): both have no # in, what would you prefer?

> >+        this.mHashId = null; // recompute hashId
> I'm not sure if this belongs to calendar style, but all files I have seen have
> had their comments above the line, no matter how short the comment was.
I've seen this style a lot and like it for short comments, because it's saving Y-space :)

Comment 12

[Michael Büttner (no reviews TFN)]

Comment on attachment 274028 [details] [diff] [review]
views, todo-list, unifinder

>         function getSavedItem(aItemToDelete) {
>             // Get the parent item, saving it in our recurringItems object for
>-            // later use. Use one string twice to resolve "ab" + "cd" vs. "a" +
>-            // "bcd" ambiguity.
>-            var hashVal = aItemToDelete.parentItem.calendar.id +
>-                          aItemToDelete.parentItem.id +
>-                          aItemToDelete.parentItem.calendar.id;
>+            // later use.
>+            var hashVal = aItemToDelete.hashId;
>             if (!recurringItems[hashVal]) {
>                 recurringItems[hashVal] = {
>                     oldItem: aItemToDelete.parentItem,
>                     newItem: aItemToDelete.parentItem.clone()
I'm wondering why you're taking the hashId of the occurrence instead of the parentItem. Basically this code has been introduced with Bug #376167 and takes care to collect all parent items that need to be modified. We call removeOccurrenceAt() and issue a single modifyItem() at the end of the call. That's why I think it's correct to use the parentItem.id here.

>             itemToDelete = this.finalizePendingModification(itemToDelete);
>-            if (!itemToDelete.parentItem.hasSameIds(itemToDelete) &&
>-                (itemToDelete.calendar.type != "wcap")) {
>+            if (itemToDelete.parentItem.hashId != itemToDelete.hashId) {
The != "wcap" line is not part of the source tree, did you mix this patch with something else?

The rest of the patch looks just fine for me, but I'm waiting for the answer on the issue raised above before giving r+.

Comment 13

[Daniel Boelzle [:dbo]]

(In reply to comment #12)
> >+            var hashVal = aItemToDelete.hashId;
> I'm wondering why you're taking the hashId of the occurrence instead of the
> parentItem. Basically this code has been introduced with Bug #376167 and takes
right, has to be parentItem of course. good catch!

> >             itemToDelete = this.finalizePendingModification(itemToDelete);
> >-            if (!itemToDelete.parentItem.hasSameIds(itemToDelete) &&
> >-                (itemToDelete.calendar.type != "wcap")) {
> >+            if (itemToDelete.parentItem.hashId != itemToDelete.hashId) {
> The != "wcap" line is not part of the source tree, did you mix this patch with
> something else?
yes, just ignore that wcap condition; it's a leftover.

Comment 14

[Michael Büttner (no reviews TFN)]

Comment on attachment 274028 [details] [diff] [review]
views, todo-list, unifinder

r=mickey with the previous comment being addressed.

Comment 15

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to comment #11)
> But we could also remove it, at least until it's needed again.
If its not used anywhere else, I think it should be removed.
> - we could either go with rid.getInTimezone("UTC").icalString or use
> encodeURIComponent(rid): both have no # in, what would you prefer?
Yes, I meant that. I would prefer encodeURIComponent(rid), since that saves an extra timezone conversion.

Comment 16

[Daniel Boelzle [:dbo]]

(In reply to comment #15)
> (In reply to comment #11)
> > But we could also remove it, at least until it's needed again.
> If its not used anywhere else, I think it should be removed.
> > - we could either go with rid.getInTimezone("UTC").icalString or use
> > encodeURIComponent(rid): both have no # in, what would you prefer?
> Yes, I meant that. I would prefer encodeURIComponent(rid), since that saves an
> extra timezone conversion.

Thinking about this, my paranoid mind thinks about that toString need not necessarily be implemented to return some unique string, doesn't it? I know this sounds weird (because calDatetime.cpp actually implements it), but providers may implement it by themselves for their published items, e.g. return "not impl" as first shot. So I would go with the UTC canonical form...

Comment 17

[Daniel Boelzle [:dbo]]

Checked in on HEAD and MOZILLA_1_8_BRANCH.

Comment 18

[Damian Szczepanik]

this seems to be major issue and may cause some problem in the future hovewer I have no idea how to prepare simple test case

Philipp, is it possible to create another calendar (or rather two calendars) similar to http://mozilla.kewis.ch/dav/testday-zones.ics where we can prepare events with defined ID so we can use them in litmus?

at least we would have possibility to verify this bug

Comment 19

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Sure that should be no problem. Just create a such calendar and upload it to my server. I'll set it readonly when I see it.

Comment 20

[Damian Szczepanik]

verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.7pre) Gecko/20070908 Calendar/0.7pre

Comment 21

[Omar Bajraszewski]

For me it doesn't work in following scenario (Mozilla/5.0 (Windows; U; Windows NT 5.1; pl; rv:1.8.1.8pre) Gecko/20070930 Sunbird/0.7):
1) start a new profile
2) Subscribe to http://www.google.com/calendar/ical/u7pke58sc91ksbm4h9pf9ofe8c%40group.calendar.google.com/public/basic.ics
3) Copy i.e.Calendar QA Chat (4th of October) to your local calendar
4)Restart Sunbird

Result-> The new event disappeared

Is it a new bug?

Comment 22

[Stefan Sitter [:ssitter]]

(In reply to comment #21)
No. See Bug 345607 / Bug 393084.