357756 - CalDav URL doesn't like parameters

Status: RESOLVED FIXED

(Calendar :: Provider: CalDAV, defect)

Description

[Randy Letness]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7

I'm trying to subscribe to a CalDAV calendar with a URL that contains a parameter.  For example:

http://server:port:/home/calendar/?ticket=abc

Sunbird seems to be adding a '/' to the end of the URL making it:

http://server:port:/home/calendar/?ticket=abc/

which results in the server getting the parameter value 'abc/'.

Reproducible: Always

Steps to Reproduce:
1. File-->New Calendar
2. choose "On the Network"
3. choose "CalDAV"
4. Enter URL with parameter like "http://server:port:/home/calendar/?ticket=abc"


Actual Results:  
HTTP request URL is "http://server:port:/home/calendar/?ticket=abc/" (extra '/' at the end)

Expected Results:  
HTTP request URL is "http://server:port:/home/calendar/?ticket=abc" (no extra '/')

Comment 1

[Bruno Browning]

This needs to be fixed to support Cosmo tickets, which for the purposes of this bug are http://www.sharemation.com/%7Emilele/public/dav/draft-ito-dav-ticket-00.txt

Comment 2

[Bruno Browning]

Attachment: parse out ?params and append to URI as needed

initial support for Cosmo tickets

Comment 3

[Matthew (lilmatt) Willis]

Comment on attachment 243411 [details] [diff] [review]
parse out ?params and append to URI as needed

>+    mTicket: null,

Please add a comment explaining what this variable is to be used for. We're need to improve at that in general.

I don't think "mTicket" is the right variable name. Correct me if I'm wrong, but "ticket" is a strictly Cosmo-centric thing. Anywhere else, and these are simply query parameters. Maybe mParams or mUriParams?

>     get mCalendarUri() { 
>         calUri = this.mUri.clone();
>+        var paramBegin = calUri.spec.indexOf("?");
>+        if (paramBegin >= 0) {
I think this should be > (rather than >=). If the ? is in position 0, the URI is bogus, isn't it?


>+           this.mTicket = calUri.spec.substring(paramBegin);
>+           calUri.spec = calUri.spec.substring(0, paramBegin);
>+        }
>         if (calUri.spec.charAt(calUri.spec.length-1) != '/') {
>             calUri.spec += "/";
>         }
I also don't think this is the right behaviour.  What if my CalDAV server's url is caldav.html?user=lilmatt or caldav.jsp?calendar=foo

I think (and I am open to discussion) the Right Thing to do is not append the / unless we get a 404 and then try again with the slash.


Minusing based on all that.

Comment 4

[Bruno Browning]

Attachment: changes per r1

Renamed variable to mUriParams and added comment, as suggested. "Ticket" is not strictly speaking a Cosmo thing (see comment #2), but I've certainly never seen them used otherwise.

Also changed the >= to >

I think I disagree about appending the slash. This was added back in April (bug 311268) so that Sunbird could deal with user-supplied calendar URIs either with or without the trailing slash. CalDAV calendars are DAV collections, so I don't think the trailing slash is incorrect even if a calendar is named caldav.html or .jsp. I certainly do think that we have badness lurking here wrt that slash, X-MOZ-LOCATIONPATH, CalDAV calendar trees and the UI needed for them, how to determine a CalDAV principal's "home" calendar, etc - but that's a whole passel of other bugs, not this one.

Comment 5

[Matthew (lilmatt) Willis]

Comment on attachment 243419 [details] [diff] [review]
changes per r1

> I think I disagree about appending the slash. 
Ok. You sold me.

r=lilmatt

Comment 6

[Matthew (lilmatt) Willis]

Comment on attachment 243419 [details] [diff] [review]
changes per r1

Over to jminta in dmose's absence

Comment 7

[Joey Minta]

Comment on attachment 243419 [details] [diff] [review]
changes per r1

+        var paramBegin = calUri.spec.indexOf("?");
+        if (paramBegin > 0) {
+           this.mUriParams = calUri.spec.substring(paramBegin);
+           calUri.spec = calUri.spec.substring(0, paramBegin);
+        }

Just for reference, I'd do this as:
var parts = calUri.spec.split('?');
if (parts.length > 1) {
    calUri.spec = parts.shift();
    this.mUriParams = '?'+parts.join('?');
}
It feels like a little less object manipulation, but who knows, maybe the join() is inefficient.  It's late and I'm rambling...

+        if (this.mUriParams != null) {itemUri.spec += this.mUriParams;}
Two slightly more legitimate nits here:
1.) just do if (this.mUriParams)
2.) Mozilla style forbids conditional blocks on the same line, no matter how short.

More significantly, this seems to be begging for a helper function.  All the cases I see can be reduced to something like
this.mMakeUri: function caldav_makeUri(aInsertString) {
  var spec = this.mCalendarUri + aInsertString+this.mUriParams;
  if (this.mUriParams) {
    return spec + this.mUriParms;
  }
  return spec;
},

Comment 8

[Bruno Browning]

Attachment: changes per r2

thanks. changes made as suggested.

Comment 9

[Joey Minta]

Comment on attachment 244531 [details] [diff] [review]
changes per r2

+    mUriParams: null,
+
It's possible for the mUriParams of one calendar to leak into another calendar unless you explicitly set this to null in the calDavCalendar constructor too.  See bug 337058 for a similar problem. r2=jminta with that.

Comment 10

[Bruno Browning]

Attachment: include mUriParams in constructor

final change per r2

Comment 11

[Matthew (lilmatt) Willis]

Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED