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

ASSIGNED
Assigned to

Status

defect
ASSIGNED
5 years ago
4 years ago

People

(Reporter: marlio, Assigned: marlio)

Tracking

(Blocks 1 bug)

Details

(Whiteboard: [calconnect31])

Attachments

(1 attachment, 11 obsolete attachments)

Assignee

Description

5 years ago
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.
Assignee

Updated

5 years ago
Assignee: nobody → malinthak2
Attachment #8428353 - Attachment mime type: text/xml → text/plain
Assignee

Comment 1

5 years ago
Posted patch schedule-tag (obsolete) β€” β€” Splinter Review
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.
Attachment #8430421 - Flags: feedback?(philipp)
Attachment #8430421 - Flags: feedback?(mohit.kanwal)
Assignee

Updated

5 years ago
Attachment #8430421 - Attachment is patch: true
Assignee

Comment 2

5 years ago
Posted patch schedule-tag v2 (obsolete) β€” β€” Splinter Review
This may causes to add a null schedule-tag to the mItemInfoCache. It will be handled with If-Schedule-Tag-Match property.
Attachment #8430421 - Attachment is obsolete: true
Attachment #8430421 - Flags: feedback?(philipp)
Attachment #8430421 - Flags: feedback?(mohit.kanwal)
Attachment #8432139 - Flags: feedback?(philipp)
Assignee

Comment 3

5 years ago
Posted patch schedule-tag v3 (obsolete) β€” β€” Splinter Review
This may causes to add a null schedule-tag to the mItemInfoCache. It will be handled with If-Schedule-Tag-Match property.
Attachment #8432139 - Attachment is obsolete: true
Attachment #8432139 - Flags: feedback?(philipp)
Assignee

Updated

5 years ago
Attachment #8432140 - Flags: feedback?(philipp)
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.
Assignee

Comment 5

5 years ago
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.
Assignee

Updated

5 years ago
Summary: Support Schedule-Tag response header in auto scheduling → Support Schedule-Tag and If-Schedule-Tag-Match headers
Assignee

Comment 6

5 years ago
Posted patch schedule-tag_headers v4 (obsolete) β€” β€” Splinter Review
Has both Schedule-Tag and If-Schedule-tag-Match headers.
Attachment #8432140 - Attachment is obsolete: true
Attachment #8432140 - Flags: feedback?(philipp)
Attachment #8436321 - Flags: feedback?(mohit.kanwal)
Assignee

Comment 7

5 years ago
Posted patch schedule-tag_headers (obsolete) β€” β€” Splinter Review
Has both Schedule-Tag and If-Schedule-tag-Match headers.
Attachment #8436321 - Attachment is obsolete: true
Attachment #8436321 - Flags: feedback?(mohit.kanwal)
Attachment #8436323 - Flags: feedback?(mohit.kanwal)
Status: NEW → ASSIGNED
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?
Attachment #8436323 - Flags: feedback?(mohit.kanwal) → feedback-
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.
Assignee

Comment 10

5 years ago
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)
Assignee

Comment 11

5 years ago
Posted patch schedule-tag_headers (obsolete) β€” β€” Splinter Review
Hope this works with Google calendar.
Attachment #8436323 - Attachment is obsolete: true
Attachment #8437031 - Flags: feedback?(mohit.kanwal)
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.
Assignee

Comment 13

5 years ago
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 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
Attachment #8437031 - Flags: feedback?(mohit.kanwal) → feedback-
Assignee

Comment 15

5 years ago
Posted patch schedule-tag_headers (obsolete) β€” β€” Splinter Review
Updated patch for schedule-tag and if-schedule-tag-match headers.
Attachment #8428353 - Attachment is obsolete: true
Attachment #8437031 - Attachment is obsolete: true
Attachment #8470194 - Flags: review?(philipp)
Assignee

Comment 16

5 years ago
Posted patch schedule-tag_headers (obsolete) β€” β€” Splinter Review
Updated patch for schedule-tag and if-schedule-tag-match request headers.
Attachment #8470194 - Attachment is obsolete: true
Attachment #8470194 - Flags: review?(philipp)
Attachment #8470201 - Flags: review?(philipp)
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
Attachment #8470201 - Flags: review+
Assignee

Comment 18

5 years ago
Posted file schedule-tag_headers (obsolete) β€”
Attachment #8470201 - Attachment is obsolete: true
Attachment #8470201 - Flags: review?(philipp)
Attachment #8472083 - Flags: review?(mohit.kanwal)
Assignee

Comment 19

5 years ago
Posted patch schedule-tag_headers (obsolete) β€” β€” Splinter Review
Attachment #8472083 - Attachment is obsolete: true
Attachment #8472083 - Flags: review?(mohit.kanwal)
Attachment #8472085 - Flags: review?(mohit.kanwal)
Assignee

Comment 20

5 years ago
Attachment #8472085 - Attachment is obsolete: true
Attachment #8472085 - Flags: review?(mohit.kanwal)
Attachment #8472178 - Flags: review?(mohit.kanwal)
Whiteboard: [calconnect31]
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.
Attachment #8472178 - Flags: review?(mohit.kanwal) → review-
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.
You need to log in before you can comment on or make changes to this bug.