Closed Bug 357756 Opened 13 years ago Closed 13 years ago

CalDav URL doesn't like parameters

Categories

(Calendar :: Provider: CalDAV, defect)

x86
Windows XP
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: randy, Assigned: browning)

References

()

Details

Attachments

(1 file, 3 obsolete files)

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 '/')
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

Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee: nobody → browning
Status: ASSIGNED → NEW
initial support for Cosmo tickets
Attachment #243411 - Flags: second-review?(dmose)
Attachment #243411 - Flags: first-review?(lilmatt)
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.
Attachment #243411 - Flags: second-review?(dmose)
Attachment #243411 - Flags: first-review?(lilmatt)
Attachment #243411 - Flags: first-review-
Attached patch changes per r1 (obsolete) — Splinter Review
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.
Attachment #243419 - Flags: first-review?(lilmatt)
Comment on attachment 243419 [details] [diff] [review]
changes per r1

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

r=lilmatt
Attachment #243419 - Flags: first-review?(lilmatt) → first-review+
Attachment #243419 - Flags: second-review?(dmose)
Comment on attachment 243419 [details] [diff] [review]
changes per r1

Over to jminta in dmose's absence
Attachment #243419 - Flags: second-review?(dmose) → second-review?(jminta)
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;
},
Attached patch changes per r2 (obsolete) — Splinter Review
thanks. changes made as suggested.
Attachment #243411 - Attachment is obsolete: true
Attachment #243419 - Attachment is obsolete: true
Attachment #244531 - Flags: second-review?(jminta)
Attachment #243419 - Flags: second-review?(jminta)
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.
Attachment #244531 - Flags: second-review?(jminta) → second-review+
final change per r2
Attachment #244531 - Attachment is obsolete: true
Whiteboard: [checkin needed]
Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
You need to log in before you can comment on or make changes to this bug.