424712 - Server location needs a slash at the end

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Vincent Robert]

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]]

Fixed in 0.1.27.

Comment 2

[Michal Jaskurzynski]

This issue appears again. I tested it with Firefox 15.0.1 and last trunk build.

Comment 3

[Michal Jaskurzynski]

Attachment: patch

Comment 4

[Dan Mills [:thunder]]

I'm really out of the loop on this, but reopening so someone else will look at it.

Comment 5

[Richard Newman [:rnewman]]

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]]

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]

Attachment: serverURL setter patch

Ok, I moved slash adding to serverURL setter.

Comment 8

[Richard Newman [:rnewman]]

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]

Attachment: revised patch

Comment 10

[Gregory Szorc [:gps]]

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]

Attachment: patch with test

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 679329 [details] [diff] [review]
patch with test

Stealing this.

Comment 13

[Richard Newman [:rnewman]]

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]

Attachment: patch with test

Comment 15

[Richard Newman [:rnewman]]

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]]

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]]

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

Comment 18

[Tracy Walker [:tracy]]

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.