Server location needs a slash at the end

VERIFIED FIXED in mozilla20

Status

Cloud Services
Firefox Sync: Backend
--
minor
VERIFIED FIXED
9 years ago
4 years ago

People

(Reporter: Vincent Robert, Assigned: Michal Jaskurzynski)

Tracking

unspecified
mozilla20
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

9 years ago
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

9 years ago
Fixed in 0.1.27.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

8 years ago
Component: Weave → General
Product: Mozilla Labs → Weave
Target Milestone: -- → ---
QA Contact: weave → general
(Assignee)

Comment 2

5 years ago
This issue appears again. I tested it with Firefox 15.0.1 and last trunk build.
(Assignee)

Comment 3

5 years ago
Created attachment 666222 [details] [diff] [review]
patch
Attachment #666222 - Flags: review?(thunder)

Comment 4

5 years ago
I'm really out of the loop on this, but reopening so someone else will look at it.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: FIXED → ---
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?
Attachment #666222 - Flags: review?(thunder) → feedback?(gps)
Component: General → Firefox Sync: UI
OS: Mac OS X → All
Hardware: PowerPC → All

Comment 6

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

Comment 7

5 years ago
Created attachment 666665 [details] [diff] [review]
serverURL setter patch

Ok, I moved slash adding to serverURL setter.
Attachment #666665 - Flags: review?(gps)
Comment on attachment 666665 [details] [diff] [review]
serverURL setter patch

Check should be done prior to equality check. 

(sorry for brevity, on phone)
Attachment #666665 - Flags: review?(gps) → review-
(Assignee)

Comment 9

5 years ago
Created attachment 667074 [details] [diff] [review]
revised patch
Attachment #666222 - Attachment is obsolete: true
Attachment #666665 - Attachment is obsolete: true
Attachment #667074 - Flags: review?(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("/").
Attachment #667074 - Flags: review?(gps) → feedback+
(Assignee)

Comment 11

5 years ago
Created attachment 679329 [details] [diff] [review]
patch with test
Attachment #667074 - Attachment is obsolete: true
Attachment #679329 - Flags: review?(gps)
Comment on attachment 679329 [details] [diff] [review]
patch with test

Stealing this.
Attachment #679329 - Flags: review?(gps) → review?(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.
Attachment #679329 - Flags: review?(rnewman) → feedback+
Assignee: nobody → mjaskurzynski
Status: REOPENED → ASSIGNED
Component: Firefox Sync: UI → Firefox Sync: Backend
(Assignee)

Comment 14

5 years ago
Created attachment 680376 [details] [diff] [review]
patch with test
Attachment #679329 - Attachment is obsolete: true
Attachment #680376 - Flags: review?(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.
Attachment #680376 - Flags: review?(rnewman) → review+
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.
Whiteboard: [fixed in services][qa+]
https://hg.mozilla.org/mozilla-central/rev/a090e3ede2e7
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services][qa+] → [qa+]
Target Milestone: --- → mozilla20
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.
Status: RESOLVED → VERIFIED
Whiteboard: [qa+]
You need to log in before you can comment on or make changes to this bug.