Closed Bug 348796 Opened 14 years ago Closed 14 years ago

Unable to view events on calDAV calendars (2)

Categories

(Calendar :: Provider: CalDAV, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: browning, Assigned: browning)

Details

Attachments

(1 file, 4 obsolete files)

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.
Attached patch don't use braces in filesnames (obsolete) — Splinter Review
Attachment #233903 - Flags: first-review?(jminta)
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?
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?
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?
(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?
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.
Attachment #233903 - Attachment is obsolete: true
Attachment #233903 - Flags: first-review?(jminta)
Attachment #234436 - Flags: first-review?(jminta)
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.
Attachment #234436 - Flags: second-review?(dmose)
Attachment #234436 - Flags: first-review?(jminta)
Attachment #234436 - Flags: first-review+
Assignee: nobody → jminta
Assignee: jminta → browning
(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.     
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking0.3-
Attachment #234436 - Flags: second-review?(dmose) → second-review+
Flags: blocking0.3- → blocking0.3?
OS: Linux → All
Hardware: PC → All
Attached patch address reviewer's concerns (obsolete) — Splinter Review
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.
Bruno,
Do you have cvs commit rights, or should one of us check this in for you?
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.
Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Using Lightning 2006082806 and Thunderbird version 2 alpha 1 (20060827) (WindowsXP) the problem still exists.

Events are still created with braces around UUIDs.
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!
I also verify the bug is fixed.
Status: RESOLVED → VERIFIED
Flags: blocking0.3?
Attachment #234436 - Attachment is obsolete: true
Reopening for further tweaking.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
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.
Attachment #236582 - Flags: second-review?(dmose)
Attachment #236582 - Flags: first-review?(jminta)
Attachment #235592 - Attachment is obsolete: true
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?
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 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?
Attachment #236582 - Flags: second-review?(dmose)
Attachment #236582 - Flags: second-review-
Attachment #236582 - Flags: first-review?(jminta)
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. ;-)
Given the utter lack of outcry here, I'd accept a patch for that.  :-)
Attachment #236582 - Attachment is obsolete: true
patch rev 5, tested w/ sunbird/trunk, caldav/storage/ics providers
Attachment #238897 - Flags: second-review?(dmose)
Attachment #238897 - Flags: first-review?(jminta)
Comment on attachment 238897 [details] [diff] [review]
nip braces in the bud

sigh... it's not pretty, but inter-op rarely is. r=jminta
Attachment #238897 - Flags: first-review?(jminta) → first-review+
Comment on attachment 238897 [details] [diff] [review]
nip braces in the bud

Looks good; thanks for the patch!
Attachment #238897 - Flags: second-review?(dmose) → second-review+
Whiteboard: [needs checkin]
Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Whiteboard: [needs checkin]
You need to log in before you can comment on or make changes to this bug.