324849 - need to check etag before uploading an ics file

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

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

We should check etags, before uploading an ics file, to be sure that we don't overwrite changes made by others.

Comment 1

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

Attachment: patch v1

Patch makes us check etags.
As bonus, we now finally really use webdav :)

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 209746 [details] [diff] [review]
patch v1

The overall design looks good. Some thoughts...

I notice that there is an onBeforeGet(), but we never actually call it.  How about removing the hook until we actually find a need for it or else adding a call?

>@@ -158,18 +170,20 @@ calICSCalendar.prototype = {
>         "PRODID": true,
>         "VERSION": true
>     },
> 
>     // nsIStreamLoaderObserver impl
>     // Listener for download. Parse the downloaded file
> 
>     onStreamComplete: function(loader, ctxt, status, resultLength, result)
>     {
>+        this.mHooks.onAfterGet(loader.request.QueryInterface(Components.interfaces.nsIChannel));
>+

How about a comment above this line, explaining that the hook might be used to, for example, save off the ETag value of the retrieved resource.  If you could add similar commentary above the other hook calls as well, it would make reading through the code easier for folks who are new to it.

>+/***************************
>+ * Transport Abstraction
>+ */

How about making the above commentary more detailed, and also include the rationale for the design of the hooks?

>+httpHooks.prototype = {
>+    
>+    onAfterGet: function(aChannel) {
>+        var httpchannel = aChannel.QueryInterface(Components.interfaces.nsIHttpChannel);
>+        this.mEtag = httpchannel.getResponseHeader("ETag");
>+        dump("old etag: "+this.mEtag+"\n");
>+        return true;
>+    },

This needs a try/catch block, since getResponseHeader will throw if the server hasn't set an ETag header.

>+    onAfterPut: function(aChannel) {
>+        var httpchannel = aChannel.QueryInterface(Components.interfaces.nsIHttpChannel);
>+        try {
>+            this.mEtag = httpchannel.getResponseHeader("ETag");
>+            dump("new etag: "+this.mEtag+"\n");
>+        } catch(e) {
>+            // There was no ETag header on the response. This means that
>+            // putting is not atomic. This is bad.
>+            // Try to do the best we can, by immediatly getting the etag.

It would probably be good to point out in the comment that since this is async, there is some period of time where we may not know the ETag, even though it exists.  Some argument could be made that we should keep this provider locked until the onOperationComplete returns, since it might make us slightly less likely to lose the race.

>+            var res = new WebDavResource(aChannel.URI);
>+            var webSvc = Components.classes['@mozilla.org/webdav/service;1']
>+                                   .getService(Components.interfaces.nsIWebDAVService);
>+            webSvc.getResourceProperties(res, 1, ['DAV: getetag'], false,
>+                                         this, null);
>+        }
>+        return true;
>+    },
>+
>+    onOperationComplete: function(aStatusCode, aResource, aOperation, aClosure) {
>+         dump("onOperationComplete\n");
>+    },
>+
>+    onOperationDetail: function(aStatusCode, aResource, aOperation, aDetail, aClosure) {
>+        dump("onOperationDetail\n");
>+        dump(aStatusCode+" "+aOperation+" "+aDetail+"\n");
>+        var props = aDetail.QueryInterface(Components.interfaces.nsIProperties);
>+        this.mEtag = props.get('DAV: getetag', Components.interfaces.nsISupportsString).toString();

Note that props.get() will throw, if an etag wasn't found.  A try/catch block isn't entirely necessary here, but it would be good style.

>+WebDavResource.prototype = {
>+    mResourceURL: {},
>+    get resourceURL() { 
>+        return this.mResourceURL;}  ,

Can you put the entire getter on a single line and get rid of the whitespace before the comma?

Comment 3

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

Attachment: patch v2

Updated patch. Now with better and more comments.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 210476 [details] [diff] [review]
patch v2

The patch itself looks great.  The one thing that worries me is that we're just going to break on servers that support HTTP PUT but not WebDAV.  Given that we support FTP, which is even lower fidelity than that, this seems odd.  

In the longer term, I suspect we may want some UI that informs the user how suitable a given connection is for use sharing calendars.

In the shorter term, I'm not sure what the right thing to do is.

Thoughts?

Comment 5

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

I can't really test, because the only server i have is apache. But if the server doesn't support etags, nothing should break. (As in: not break more then without the patch). Everything is in try/catch blocks, and if there is no etag, we don't set the if-header.
Given that apache break on headers like if-unmodified-since, I don't think we should use those headers. (breaks means that it always returns a pre-condition-failed response)

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 210476 [details] [diff] [review]
patch v2

According to the specs, unknown headers should be ignored, so this shouldn't cause the problem I was wondering about.  r=dmose

Comment 7

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

patch checked in