Last Comment Bug 424712 - Server location needs a slash at the end
: Server location needs a slash at the end
Status: VERIFIED FIXED
:
Product: Cloud Services
Classification: Client Software
Component: Firefox Sync: Backend (show other bugs)
: unspecified
: All All
: -- minor (vote)
: mozilla20
Assigned To: Michal Jaskurzynski
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-03-23 18:40 PDT by Vincent Robert
Modified: 2013-01-08 10:21 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (489 bytes, patch)
2012-09-29 10:04 PDT, Michal Jaskurzynski
no flags Details | Diff | Splinter Review
serverURL setter patch (623 bytes, patch)
2012-10-01 13:05 PDT, Michal Jaskurzynski
rnewman: review-
Details | Diff | Splinter Review
revised patch (579 bytes, patch)
2012-10-02 11:25 PDT, Michal Jaskurzynski
gps: feedback+
Details | Diff | Splinter Review
patch with test (1.69 KB, patch)
2012-11-07 12:51 PST, Michal Jaskurzynski
rnewman: feedback+
Details | Diff | Splinter Review
patch with test (2.33 KB, patch)
2012-11-10 10:09 PST, Michal Jaskurzynski
rnewman: review+
Details | Diff | Splinter Review

Description Vincent Robert 2008-03-23 18:40:21 PDT
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; fr; rv:1.9b4) Gecko/2008030317 Firefox/3.0b4
Build Identifier: 0.1.22

I tried to use a personal server instead of services.mozilla.org to sync my profile using weave.

I discovered that what Weave 0.1.22 do is concatenate its pathes with the URL configured in the preferences. If this URL does not end with a slash, Weave just use the last part of the URL as a prefix for created folders.

Ex: 
Using https://example.com/weave/ creates https://example.com/weave/user/...
Using https://example.com/weave  creates https://example.com/weaveuser/...

Reproducible: Always

Steps to Reproduce:
1.Configure the server location as something like https://example.com/weave
2.Synchronize

Actual Results:  
The created folder will be https://example.com/weaveuser/

Expected Results:  
The created folder should be https://example.com/weave/user/
Comment 1 Dan Mills [:thunder] 2008-03-31 15:22:43 PDT
Fixed in 0.1.27.
Comment 2 Michal Jaskurzynski 2012-09-29 09:58:02 PDT
This issue appears again. I tested it with Firefox 15.0.1 and last trunk build.
Comment 3 Michal Jaskurzynski 2012-09-29 10:04:12 PDT
Created attachment 666222 [details] [diff] [review]
patch
Comment 4 Dan Mills [:thunder] 2012-09-29 10:10:14 PDT
I'm really out of the loop on this, but reopening so someone else will look at it.
Comment 5 Richard Newman [:rnewman] 2012-09-29 11:27:29 PDT
Comment on attachment 666222 [details] [diff] [review]
patch

I'd rather this logic live in services/sync/service.js, in the setter for serverURL. That will catch every case of assignment, and IMO is the right place to enforce this part of the rule. gps, do you agree?
Comment 6 Gregory Szorc [:gps] 2012-10-01 11:34:18 PDT
Comment on attachment 666222 [details] [diff] [review]
patch

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

I agree that this logic should be in service.js, closer to the core API.
Comment 7 Michal Jaskurzynski 2012-10-01 13:05:20 PDT
Created attachment 666665 [details] [diff] [review]
serverURL setter patch

Ok, I moved slash adding to serverURL setter.
Comment 8 Richard Newman [:rnewman] 2012-10-01 18:14:05 PDT
Comment on attachment 666665 [details] [diff] [review]
serverURL setter patch

Check should be done prior to equality check. 

(sorry for brevity, on phone)
Comment 9 Michal Jaskurzynski 2012-10-02 11:25:40 PDT
Created attachment 667074 [details] [diff] [review]
revised patch
Comment 10 Gregory Szorc [:gps] 2012-11-05 10:23:58 PST
Comment on attachment 667074 [details] [diff] [review]
revised patch

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

Could you please add a test for this?

Sorry the review took so long. It got lost in my review queue.

::: services/sync/modules/service.js
@@ +65,5 @@
>    get serverURL() Svc.Prefs.get("serverURL"),
>    set serverURL(value) {
> +    // Add slash at the end
> +    // See Bug 424712
> +    if (value.charAt(value.length - 1) != "/")

Use value.endsWith("/").
Comment 11 Michal Jaskurzynski 2012-11-07 12:51:11 PST
Created attachment 679329 [details] [diff] [review]
patch with test
Comment 12 Richard Newman [:rnewman] 2012-11-07 13:20:41 PST
Comment on attachment 679329 [details] [diff] [review]
patch with test

Stealing this.
Comment 13 Richard Newman [:rnewman] 2012-11-09 13:14:30 PST
Comment on attachment 679329 [details] [diff] [review]
patch with test

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

Thanks for the patch!

"test_service_serverUrl.js" should be "test_service_set_serverURL.js".

Please set the User etc. fields in your patch according to:

<https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F>

::: services/sync/modules/service.js
@@ +74,5 @@
>  
>    get serverURL() Svc.Prefs.get("serverURL"),
>    set serverURL(value) {
> +    // Add slash at the end
> +    // See Bug 424712

No need for these comments.

@@ +76,5 @@
>    set serverURL(value) {
> +    // Add slash at the end
> +    // See Bug 424712
> +    if (!value.endsWith("/"))
> +      value += "/";

Always use {} in conditional expressions.

::: services/sync/tests/unit/test_service_serverUrl.js
@@ +2,5 @@
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +Cu.import("resource://services-sync/service.js");
> +
> +// Bug 424712: Server location needs a slash at the end

Trailing period.

@@ +4,5 @@
> +Cu.import("resource://services-sync/service.js");
> +
> +// Bug 424712: Server location needs a slash at the end
> +function run_test() {
> +  Service.serverURL = "http://example.com/weave";

s/weave/sync/g

"Weave" died a death in ~2009; we shouldn't be introducing new instances of that string.

@@ +6,5 @@
> +// Bug 424712: Server location needs a slash at the end
> +function run_test() {
> +  Service.serverURL = "http://example.com/weave";
> +  do_check_eq(Service.serverURL,
> +              "http://example.com/weave/");

These do_check_eq calls shouldn't span multiple lines -- total length is 61 columns.
Comment 14 Michal Jaskurzynski 2012-11-10 10:09:57 PST
Created attachment 680376 [details] [diff] [review]
patch with test
Comment 15 Richard Newman [:rnewman] 2012-11-12 09:47:02 PST
Comment on attachment 680376 [details] [diff] [review]
patch with test

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

::: services/sync/modules/service.js
@@ +76,5 @@
>    set serverURL(value) {
> +    if (!value.endsWith("/")) {
> +      value += "/";
> +    }
> +    

Nit: trailing whitespace.

::: services/sync/tests/unit/xpcshell.ini
@@ +66,5 @@
>  [test_service_login.js]
>  [test_service_migratePrefs.js]
>  [test_service_passwordUTF8.js]
>  [test_service_persistLogin.js]
> +[test_service_set_serverUrl.js]

Should be serverURL.
Comment 16 Richard Newman [:rnewman] 2012-11-12 09:50:01 PST
Pushed with nits addressed:

https://hg.mozilla.org/services/services-central/rev/a090e3ede2e7

STR: custom server URL should work with or without a single trailing slash. Not an urgent verification.
Comment 17 Richard Newman [:rnewman] 2013-01-02 19:43:40 PST
https://hg.mozilla.org/mozilla-central/rev/a090e3ede2e7
Comment 18 Tracy Walker [:tracy] 2013-01-08 10:21:54 PST
If I leave off the slash, it is automatically added a brief time after .com is complete.  That appears to me what the code is meant to do.

Note You need to log in before you can comment on or make changes to this bug.