348796 - Unable to view events on calDAV calendars (2)

Status: RESOLVED FIXED

(Calendar :: Provider: CalDAV, defect)

Description

[Bruno Browning]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.6) Gecko/20060808 Fedora/1.5.0.6-2.fc5 Firefox/1.5.0.6 pango-text
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060814 Calendar/0.3a2+

Sunbird stores data on CalDAV servers in files with names based on item.id, which often contain brace characters. Cosmo, at least, does not REPORT data in files with such names.

Reproducible: Always

Steps to Reproduce:
1. Create event on CalDAV (Cosmo) calendar with Sunbird
2. Exit/restart Sunbird
3. Note event missing in UI

Actual Results:  
Event does not appear in Sunbird UI

Expected Results:  
Event should appear in Sunbird UI

4. Access CalDAV server with e.g. cadaver
5. Note event has filename like {12911bd2-de20-47e1-874e-33da0d4874bd}.ics
6. Rename event to, say, 12911bd2-de20-47e1-874e-33da0d4874bd.ics 
7. Exit/restart Sunbird
8. Observe event in UI

I see this only with Cosmo; currently no access to oracle caldav server.
Using Sunbird with fix for bug 337790 applied.

Comment 1

[Bruno Browning]

Attachment: don't use braces in filesnames

Comment 2

[Joey Minta]

Comment on attachment 233903 [details] [diff] [review]
don't use braces in filesnames

-            eventUri.spec = eventUri.spec + aNewItem.id + ".ics";
+            eventUri.spec = eventUri.spec + aNewItem.id.replace(/[{}]/g, '').replace(/[{}]/g, '')
Why 2 .replace() calls?

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Do we know that this is a Mozilla bug and not a Cosmo bug?  If we do, is getting rid of braces the correct solution, or is it possible that this is a URL-encoding issue?

Comment 4

[Bruno Browning]

I think this *is* a Cosmo bug, and have filed a bug about it on the osaf bugzilla (#6518, fwiw). I don't see how it can be a urlencoding issue (at least Mozilla-side) since the REPORT request XML doesn't reference the filename, and the problem is reflected already in the REPORT response. 

While this is not a Mozilla bug, per se, it would be good to find a way to mitigate it if it's possible to do so reasonably. The CalDAV provider has been effectively broken for some time now, with all that means for interest/bugreports/contributions, and this bug means it's likely to remain so in 0.3 - at least wrt Cosmo. 

On further thought, though, simply omitting the braces is not a solution (even ignoring my ham-handed use of ^V while creating the patch) at least as long as they are still present in the UUID. Is there a reason to have braces in the UUID, other that that's how they come out of the UUID generator?

Comment 5

[Joey Minta]

(In reply to comment #4)
> On further thought, though, simply omitting the braces is not a solution (even
> ignoring my ham-handed use of ^V while creating the patch) at least as long as
> they are still present in the UUID. Is there a reason to have braces in the
> UUID, other that that's how they come out of the UUID generator?
> 
No, there's no compelling reason to use the braces.  Perhaps for calDAV we should strip them out after we call getUUID() with a note about cosmo-interop?

Comment 6

[Bruno Browning]

Attachment: strip braces from CalDAV UIDs for now

I think stripping braces from CalDAV UIDs is exactly what's needed for now. I also don't think we should special-case indefinitely: once Cosmo supports the UID style we use elsewhere this patch should be reverted. Suggest we apply the patch but leave the bug open until we can revert this.

Comment 7

[Joey Minta]

Comment on attachment 234436 [details] [diff] [review]
strip braces from CalDAV UIDs for now

r=jminta with a quick note in the comment explaining what the bug is (Cosmo doesn't like {} in UUIDs.)

Also, need dmose's sign-off on calDAV stuff.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #7)
> (From update of attachment 234436 [details] [diff] [review] [edit])
> r=jminta with a quick note in the comment explaining what the bug is (Cosmo
> doesn't like {} in UUIDs.)
> 
> Also, need dmose's sign-off on calDAV stuff.
> 

In general, I think we're probably best to just always strip the braces in the spirit of "be conservative in what you send and liberal in what you can receive".  2r=dmose with the comment tweaked to make that clear.     

Comment 9

[Bruno Browning]

Attachment: address reviewer's concerns

I think this version addresses reviewers concerns. The comment explains the server-side issue, and since we're not looking at this as a temporary thing I went ahead and just stripped braces as the UIDs are being generated.

Comment 10

[Matthew (lilmatt) Willis]

Bruno,
Do you have cvs commit rights, or should one of us check this in for you?

Comment 11

[Bruno Browning]

lilmatt--
I don't have cvs commit rights and would appreciate it if someone could check this in. I think 348891 has a patch ready to go too. Thanks.

Comment 12

[Matthew (lilmatt) Willis]

Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED

Comment 13

[Sebastian Schwieger]

Using Lightning 2006082806 and Thunderbird version 2 alpha 1 (20060827) (WindowsXP) the problem still exists.

Events are still created with braces around UUIDs.

Comment 14

[Sebastian Schwieger]

I can confirm this bug fixed using a *clean* profile and Lightning 2006082806/Thunderbird version 2 alpha 1 (20060827)(WindowsXP). 
The previous comment was referring to a non-clean profile!

Comment 15

[cmtalbert]

I also verify the bug is fixed.

Comment 16

[Bruno Browning]

Reopening for further tweaking.

Comment 17

[Bruno Browning]

Attachment: allow events to be adopted from non-CalDAV cals

previous version of patch allowed events to be created/modified on CalDAV calendars, but when an event created on a non-CalDAV calendar was moved onto a CalDAV calendar, the braces in the UID were preserved, triggering the bug. This version strips braces from UIDs regardless of origin, allowing move-event to work.

Comment 18

[Joey Minta]

Comment on attachment 236582 [details] [diff] [review]
allow events to be adopted from non-CalDAV cals

+        // CalDAV UIDs are used to construct filenames, so we strip braces to avoid
+        // problems with servers which do not support names with {}
+        aItem.id = aItem.id.replace(/[{}]/g, '')
+
Are we always assured that an item will be mutable here?

Comment 19

[Bruno Browning]

I would think that mutability would be ensured by onAccept in  calendar-event-dialog.js. Can add a check for mutability if it would be useful. Assume it should throw, since the idl says that adoptItem does not clone.

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 236582 [details] [diff] [review]
allow events to be adopted from non-CalDAV cals

It strikes me that mutating an existing UUID is dataloss, and I think it would be preferable to just wait for Cosmo to fix their bug.  

However, I'd be amenable to always stripping out the braces in getUUID() for all providers rather than doing it just specially in CalDAV, which would work around that bug in a different way.  Thoughts?

Comment 21

[Bruno Browning]

Sounds sensible to me: those braces strike me as both redundant and as problems still waiting to happen. Then again, I'm new enough to this project that most anyone's opinion ought to be more interesting than mine. ;-)

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

Given the utter lack of outcry here, I'd accept a patch for that.  :-)

Comment 23

[Bruno Browning]

Attachment: nip braces in the bud

patch rev 5, tested w/ sunbird/trunk, caldav/storage/ics providers

Comment 24

[Joey Minta]

Comment on attachment 238897 [details] [diff] [review]
nip braces in the bud

sigh... it's not pretty, but inter-op rarely is. r=jminta

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 238897 [details] [diff] [review]
nip braces in the bud

Looks good; thanks for the patch!

Comment 26

[Matthew (lilmatt) Willis]

Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED