447913 - CalDAV provider should avoid query-by-uid on after-PUT fetches

Status: RESOLVED FIXED

(Calendar :: Provider: CalDAV, defect)

Description

[Bruno Browning]

Attachment: try multiget before query-by-UID

The CalDAV provider refetches items after PUT in order to detect changes applied by the server. It currently does so with a query-by-UID REPORT to ensure interop with OGo/SOGo servers, which at one time relocated items on PUT. The relocate-on-PUT is not RFC4791-compliant and we arguably should not support it, and the query-by-UID puts unnecessary loads on RFC-compliant servers, where a simpler GET would do the trick. 

With this patch we first attempt the refetch with a single-item multiget on the URI to which the item was just PUT, and fall back to the query-by-UID only if that fails. I don't have access to a OGo server, so this has not been tested with OGo. I do have limited access to a SOGo server, and it does not seem to do the relocation any longer, at least for CalDAV purposes. It's possible that the fallback to query-by-UID is no longer necessary.

Comment 1

[Philipp Kewisch [:Fallen][📱📆]]

I'm not sure this is the right approach. I think we should determine the type of get that is required per server, my ideas as follows:

*update item, GET returns 200
==> Server supports query by GET (i.e doesn't move an item),
    save a bit and in future only query by GET, regardless of 404


* update item, GET returns 404
* try query by uid, returns 200
==> Server supports query by uid and moves the item,
    save a bit and in the future only query by uid

* update item, GET returns 404
* try query by uid, returns 400, or an 5xx code
==> Server doesn't support query by uid,
    save a bit and in future only query by GET, regardless of 404

This keeps down the needed number of requests. We could either save this bit transiently to make sure you don't need to re-add the calendar when the server is upgraded, or persist to avoid retesting.

Bruno, how does this sound for you?

Comment 2

[Bruno Browning]

(In reply to comment #1)
> 
> Bruno, how does this sound for you?
> 

I think you are nicer than I am. :)

RFC 4791 paragraph 7.2 says that REPORTs should only consider items that are in the calendar collection, not items that are in subcollections of the calendar collection. The move-after-PUT behavior is not compliant; to the extent that a server behaves this way it is not a CalDAV server and IMO support for it is out-of-scope for the Mozilla CalDAV provider. If a server wants to bill itself as a CalDAV server, it needs to comply with the RFC.

My suggested course of action (and I should have been more explicit about this in the bug report):
1) this patch
2) file bug(s) at OGo (and SOGo if necessary) saying that support for servers which behave this way will be removed from Sunbird/Lightning at some specified point in the future (my suggestion: shortly after our 0.9 release)
3) at that point remove getUpdatedItemByUid(), the consts, and 404-check; in other words only ever use GET, and that on the URL to which we just PUT.

That gives us cleaner, CalDAV-only code and also gives developers of problematic server impls time to cope with the change in whatever manner they see fit.

Comment 3

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 331253 [details] [diff] [review]
try multiget before query-by-UID

Argh it seems my review comments and review was lost...

>+        var itemUri = this.mCalendarUri.clone();
>+        try {
>+            // refetch after PUT of modified item
>+            var locationPath = this.mItemInfoCache[aItem.id].locationPath;
>+        } catch(ex) {
>+            // refetch after PUT of new item
>+            var locationPath = aItem.id + ".ics";
>+        }
I'd prefer using if() instead of lazily using try/catch. Also, I'd suggest to revive the old getLocationPath() function, fixing it to make it part of the prototype.

>+                if (elementStatus == 404 && aRefetchType == CALDAV_REFETCH_GET) {
>+                    // this is probably an OGo/SOo server that has improperly
>+                    // moved the calitem into a subcollection
>+                    thisCalendar.getUpdatedItemByUid(aItem, aListener);
>+                    return;
>+                }
It seems you only check for REFETCH_GET in this function, therefore I'd suggest to rather call the parameter aRefetchByUid and make it boolean.

r=philipp

Comment 4

[Philipp Kewisch [:Fallen][📱📆]]

Ah yes, the reason I +'d this instead of using my solution, I agree we shouldn't work around if its not really needed.

Since I'd like this in, I'm going to fix the comments and check in. Bruno if you disagree to my comments, please tell me and I'll change things.

Comment 5

[Philipp Kewisch [:Fallen][📱📆]]

Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED

Comment 6

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: [checked in] Additional fix - v1

I accidentally checked in the old patch as is, these are my suggested review comment fixes.

Comment 7

[Philipp Kewisch [:Fallen][📱📆]]

Thats what I get for not testing my patches. It should of course be getItemLocationPath() and  aItem.id in this.mItemInfoCache.

Please pretend its that way