1015717 - Support Schedule-Tag and If-Schedule-Tag-Match headers

Status: NEW

(Calendar :: Provider: CalDAV, defect)

Description

[Malintha Fernando [:marlio]]

Attachment: Shows schedule-tag property change in organizer object resource against several changes

Schedule-Tag property has been introduced in RFC 6638 and needs to be implemented in Lightning. http://tools.ietf.org/html/rfc6638#section-8.2

Schedule-tag property will be used to specify in if-schedule-tag-match request header, which will be mentioned in schedule requests sent out by attendees, instead of if-match Etag property to resolve conflicts within the server. There are several situations in which the schedule-tag will be updated by the server in organizer's and attendee's object resources, but almost both behave similarly.
We need to request for schedule-tag in multiget and sync request headers (any occasion which updates the object resource) to store any changes in the schedule-tag and store as a property of the scheduling object resource to be mentioned in if-schedule-tag-match property. 

If a GET request sent out, server must reply with the schedule-tag and no need to specify. But calendar is currently sending prop requests, therefore it will be needed to specify in the request header.

Comment 1

[Malintha Fernando [:marlio]]

Attachment: schedule-tag

This modifies the multiget request query in multiget request handler with schedule-tag and adds the returned value into mItemInfoCache. I added the schedule-tag query only to multiget request initially and other handlers also need to get updated. Just want to make sure the way I did here is correct.

Comment 2

[Malintha Fernando [:marlio]]

Attachment: schedule-tag v2

This may causes to add a null schedule-tag to the mItemInfoCache. It will be handled with If-Schedule-Tag-Match property.

Comment 3

[Malintha Fernando [:marlio]]

Attachment: schedule-tag v3

This may causes to add a null schedule-tag to the mItemInfoCache. It will be handled with If-Schedule-Tag-Match property.

Comment 4

[Mohit Kanwal [:redDragon]]

Comment on attachment 8432140 [details] [diff] [review]
schedule-tag v3

Review of attachment 8432140 [details] [diff] [review]:
-----------------------------------------------------------------

I think we might require some discussions regarding the Schedule-Tag implementation. Lets iron out some of the implementation details so that it developing the patch becomes easier.

::: calendar/providers/caldav/calDavCalendar.js
@@ +784,4 @@
>                  notifyListener(listenerStatus, listenerDetail, true);
>              }
>          };
> +        //RFC6638: TODO: change If-Match to If-Schedule-Tag-Match

This might be a breaking change, only a handful of servers support the Schedule-Tag implementation. If we change this a lot of things will break. We may need to provide a pluggable way of doing this so that things work even when the server does not support schedule-tag implementation.

@@ +978,5 @@
>       * @param aUri      Base URI of the request
>       * @param aListener Listener
>       */
> +     //RFC6638
> +    addTargetCalendarItem : function caldav_addTargetCalendarItem(path,calData,aUri, etag, scheduleTag ,aListener) {

Small knit: Usually for arguments we use names starting with `a`

::: calendar/providers/caldav/calDavRequestHandlers.js
@@ +928,5 @@
>                             r.href && r.href.length &&
>                             r.calendardata && r.calendardata.length) {
>                      let oldEtag;
> +                    //RFC6638
> +                    let oldScheduleTag;

I see you try to get the old schedule tag but no comparisons done with it.

Comment 5

[Malintha Fernando [:marlio]]

In the RFC they have not mentioned about any method to confirm whether the server supports schedule-tag or not. So it'll be like checking the multiget response and find for schedule-tag and add if it is available. As long as adding new values is done within addTargetCalendarItem, we have to pass the value into it. So still it changes the signature of addTargetCalendarItem. 

Guess I wouldn't have taken the old Schedule-tag in this scenario. It's not necessary to check because of the etag comparison.

Comment 6

[Malintha Fernando [:marlio]]

Attachment: schedule-tag_headers v4

Has both Schedule-Tag and If-Schedule-tag-Match headers.

Comment 7

[Malintha Fernando [:marlio]]

Attachment: schedule-tag_headers

Has both Schedule-Tag and If-Schedule-tag-Match headers.

Comment 8

[Mohit Kanwal [:redDragon]]

Comment on attachment 8436323 [details] [diff] [review]
schedule-tag_headers

Review of attachment 8436323 [details] [diff] [review]:
-----------------------------------------------------------------

Some minor adjustments needed, Some minor typing nits. Also putting up a preference for schedule tag support would be nice.

::: calendar/providers/caldav/calDavCalendar.js
@@ +787,3 @@
>  
>          this.sendHttpRequest(eventUri, modifiedItemICS, MIME_TEXT_CALENDAR, null, (channel) => {
>              if (!aIgnoreEtag) {

we might wanna change the names of the tags. I think also it would be good if we are able to turn on/turn off behavior using a calendar pref

@@ +787,4 @@
>  
>          this.sendHttpRequest(eventUri, modifiedItemICS, MIME_TEXT_CALENDAR, null, (channel) => {
>              if (!aIgnoreEtag) {
> +            	ScheduleTag = this.mItemInfoCache[aNewItem.id].ScheduleTag;

var names should begin with small case.

@@ +797,4 @@
>                                           false);
> +                }
> +                //try with etag property. Therefore the request header should be If-Match
> +                else{

I think !aIgnoreEtag makes more sense here, we cannot default to etag match unless required.

@@ +2863,5 @@
>          copyHeader("Recipient");
>          copyHeader("If-None-Match");
>          copyHeader("If-Match");
> +        copyHeader("If-Schedule-Tag-Match");
> +        

Extra Tabspace

::: calendar/providers/caldav/calDavRequestHandlers.js
@@ +926,4 @@
>                      } else {
>                          cal.LOG("CalDAV: skipping unfound deleted item : " + r.href);
>                      }
> +                

Tab space.

@@ +933,5 @@
> +                    cal.LOG("RFC6638: Calendar supports schedule-tag : "+this.isScheduleTagSupport+" : "+r.scheduleTag);
> +                    let oldscheduleTag;
> +                    let itemId = this.calendar.mHrefIndex[r.href];
> +                    if(itemId) {
> +                        oldscheduleTag = this.calendar.mItemInfoCache[itemId].scheduleTag;

this.mItemInfoCache[item.id].ScheduleTag is used previously, here scheduleTag is used. Is this the convention?

Comment 9

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

I will comment on this more tomorrow, but I just wanted to jump in quickly before you start hiding it behind a preference: I don't think we should hide Schedule-Tag behind a preference, but instead auto-detect if the server supports it. We made this mistake with the caldav.sched.enabled pref and the result is that its broken, probably noone uses it and the code is just dead.

Comment 10

[Malintha Fernando [:marlio]]

Philipp,Looking for the comment. Mohit, yes I have mixed up the names.  I'll clean it and submit. Also looking if there is a change needed for schedule-tag like if (!aIgnoreEtag)

Comment 11

[Malintha Fernando [:marlio]]

Attachment: schedule-tag_headers

Hope this works with Google calendar.

Comment 12

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

Comment on attachment 8437031 [details] [diff] [review]
schedule-tag_headers

Review of attachment 8437031 [details] [diff] [review]:
-----------------------------------------------------------------

::: calendar/providers/caldav/calDavCalendar.js
@@ +787,4 @@
>  
>          this.sendHttpRequest(eventUri, modifiedItemICS, MIME_TEXT_CALENDAR, null, (channel) => {
>              if (!aIgnoreEtag) {
> +            	scheduleTag = this.mItemInfoCache[aNewItem.id].scheduleTag;

Is this variable initialized somewhere? (same for etag, actually)

@@ +790,5 @@
> +            	scheduleTag = this.mItemInfoCache[aNewItem.id].scheduleTag;
> +                etag = this.mItemInfoCache[aNewItem.id].etag;
> +                //check if the server has returned ScheduleTag for this item. If yes, set the header to If-Schedule-Tag-Match
> +                if(scheduleTag){
> +                    cal.LOG("RFC6638: Schedule Tag to match "+ this.mItemInfoCache[aNewItem.id].scheduleTag);

Go ahead and use your variable here.

@@ +970,4 @@
>  
>          this.sendHttpRequest(eventUri, null, null, null, (channel) => {
>              if (!aIgnoreEtag) {
> +            	scheduleTag = this.mItemInfoCache[aItem.id].scheduleTag;

Same comment about initialization

@@ +975,5 @@
> +                //Check if the server has returned a ScheduleTag value for the item. If yes, set the 
> +                //request header to If-Schedule-Tag-Match
> +                if (scheduleTag) {
> +                    cal.LOG("RFC6638: ScheduleTag match uuid: "+aItem.id+" scheduleTag: "+scheduleTag);
> +                    cal.LOG("CalDAV: Will only delete if matches ScheduleTag " + scheduleTag);

Just for my understanding of rfc6638, does anything special have to be done when deleting scheduling objects? I am thinking about the ambiguity where "delete" could either mean really removing the item (you are the organizer or its not a scheduling object) or the result is that you as an attendee are marked as DECLINED.

I am not sure how rfc6638 handles this, please enlighten me :-)

::: calendar/providers/caldav/calDavRequestHandlers.js
@@ +896,5 @@
>                  this.currentResponse[this.tag ] = "";
>                  break;
> +            case "schedule-tag":
> +                this.isScheduleTagSupport = true;
> +                this.tag = aLocalName.replace("schedule-t","scheduleT");

While this makes the code easier to read, I don't think you have to change the tag name here, avoiding another string operation. using r.scheduletag later on sounds fine to me.

Comment 13

[Malintha Fernando [:marlio]]

Philipp, I'll look into the changes of the patch as soon as I'm done with the Fake server test. About the DELETE behaviour. It performs similar as the Etag. We just call the method with last known schedule-tag value specified under If-Schedule-Tag-Match value. This is quoted from RFC 6638 :
"Clients MAY use the If-Schedule-Tag-Match request header to do a
   DELETE, COPY, or MOVE request that ensures that "inconsequential"
   changes on the server do not result in a precondition error.  The
   value of the request header is set to the last Schedule-Tag value
   received for the resource being deleted.  If the value of the
   If-Schedule-Tag-Match header matches the current value of the CALDAV:
   schedule-tag property, the server performs the normal DELETE, COPY,
   or MOVE request processing for the resource.  Otherwise, the server
   MUST fail the request with a 412 Precondition Failed status code."

Comment 14

[Mohit Kanwal [:redDragon]]

Comment on attachment 8437031 [details] [diff] [review]
schedule-tag_headers

Review of attachment 8437031 [details] [diff] [review]:
-----------------------------------------------------------------

Setting - to get the patch cleaned up after the fake server tests are finished

Comment 15

[Malintha Fernando [:marlio]]

Attachment: schedule-tag_headers

Updated patch for schedule-tag and if-schedule-tag-match headers.

Comment 16

[Malintha Fernando [:marlio]]

Attachment: schedule-tag_headers

Updated patch for schedule-tag and if-schedule-tag-match request headers.

Comment 17

[Mohit Kanwal [:redDragon]]

Comment on attachment 8470201 [details] [diff] [review]
schedule-tag_headers

Review of attachment 8470201 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks fine, need to fix the style issues, and I think you can create a separate unit test for schedule tags using the fake server and attach it with this patch (or multiple patches) depending on which one gets checked in first.

::: calendar/providers/caldav/calDavCalendar.js
@@ +831,5 @@
> +                                             scheduleTag,
> +                                             false);
> +                }
> +                //try with etag property. Therefore the request header should be If-Match
> +                else{

style extra space needed between `else` & `{`

::: calendar/providers/caldav/calDavRequestHandlers.js
@@ +689,4 @@
>      unhandledErrors : 0,
>      itemsNeedFetching: null,
>      additionalSyncNeeded: false,
> +    isScheduleTagSupport: false,

isScheduleTagSupported or hasScheduleTagSupport or supportsScheduleTag is a better name :)

@@ +898,5 @@
> +            case "schedule-tag":
> +                this.isScheduleTagSupport = true;
> +                this.tag = aLocalName.replace(/-/g,'');
> +                this.currentResponse[this.tag] = "";
> +                cal.LOG("RFC6638: isScheduleTagSupport: "+this.isScheduleTagSupport);

you might want to turn off this log message after you are done debugging otherwise it will print a lot of log messages ;)

@@ +926,4 @@
>                      } else {
>                          cal.LOG("CalDAV: skipping unfound deleted item : " + r.href);
>                      }
> +                

style knits extra tab characters present

Comment 18

[Malintha Fernando [:marlio]]

Attachment: schedule-tag_headers

Comment 19

[Malintha Fernando [:marlio]]

Attachment: schedule-tag_headers

Comment 20

[Malintha Fernando [:marlio]]

Attachment: schedule-tag_headers v5

Comment 21

[Mohit Kanwal [:redDragon]]

Comment on attachment 8472178 [details] [diff] [review]
schedule-tag_headers v5

Review of attachment 8472178 [details] [diff] [review]:
-----------------------------------------------------------------

Patch is almost there, just some very minor styling issues. Also the patch is not the correct hg changeset format. To get the patch to a changeset format so that it applies cleanly you need to provide a username for the patch. Something like this: https://bug1049591.bugzilla.mozilla.org/attachment.cgi?id=8561122 Notice the username at the top of the patch. If you are using mercurial, you can refer to http://mercurial.selenic.com/wiki/QuickStart to set a username in your local ~/.hgrc file. That should fix it.

::: calendar/providers/caldav/calDavCalendar.js
@@ +1009,2 @@
>                  let etag = this.mItemInfoCache[aItem.id].etag;
> +                //Check if the server has returned a ScheduleTag value for the item. If yes, set the 

There is an extra space here.

@@ +1036,4 @@
>       * @param aUri      Base URI of the request
>       * @param aListener Listener
>       */
> +        addTargetCalendarItem : function caldav_addTargetCalendarItem(path,calData,aUri, etag, aScheduleTag ,aListener) {

I think the doc comment should be updated as well to reflect the change.

@@ +2880,5 @@
>          copyHeader("Recipient");
>          copyHeader("If-None-Match");
>          copyHeader("If-Match");
> +        copyHeader("If-Schedule-Tag-Match");
> +        

Extra space here.

::: calendar/providers/caldav/calDavRequestHandlers.js
@@ +576,1 @@
>                      }

What's the change here, why is this line highlighted?

@@ +933,5 @@
> +                    let itemId = this.calendar.mHrefIndex[r.href];
> +                    if(itemId) {
> +                        oldScheduleTag = this.calendar.mItemInfoCache[itemId].scheduletag;
> +                    }
> +                    else {

brace ordering is incorrect. should be 
if (itemId) {
///
} else {
///
}

@@ +946,5 @@
> +                                                            r.getetag,
> +                                                            r.scheduletag,
> +                                                            this.listener);
> +                    }
> +                    else {

Same here, the brace positioning is incorrect

@@ +951,5 @@
> +                        cal.LOG("CalDAV: skipping item with unmodified schedule-tag : " + oldScheduleTag);
> +                    }
> +                }
> +                // Created or Updated item, etag comparison
> +                else if (r.getetag && r.getetag.length &&

Same here. The comment can come after the braces.

Comment 22

[Mohit Kanwal [:redDragon]]

Ah splinter does not show up correctly in the comments, you can click on review button to get the actual patch line comments to see the style related knits.