Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix default parse server url #356

Merged
merged 1 commit into from
Jan 21, 2016
Merged

Conversation

wangmengyan95
Copy link
Contributor

We need to add additional / at the end of our default server URL address. Otherwise, when we use new URL(server, httpPath) to build the URL here, the /1 will be ignored.
cc @richardjrossiii

@facebook-github-bot
Copy link

By analyzing the blame information on this pull request, we identified @richardjrossiii, @grantland and @wangmengyan95 to be potential reviewers.

@grantland
Copy link
Contributor

Fixes #352

@grantland
Copy link
Contributor

@richardjrossiii Does this comply with the batch issue raised in #330?

@richardjrossiii
Copy link
Contributor

@grantland Yes, URLs are constructed identically in batch vs normal creation.

@wangmengyan95
Copy link
Contributor Author

@grantland @richardjrossiii any other opinions about this fix?

@grantland
Copy link
Contributor

As long as everything works, I think it's fine. The original concern with trailing slashes was that it would cause a double slash which would mean root:

https://api.parse.com/1//blah -> https://api.parse.com/blah

Are you sure this scenario won't be an issue?

@grantland grantland added this to the 1.13.0 milestone Jan 21, 2016
@wangmengyan95
Copy link
Contributor Author

@grantland, just tested

URL root = new URL("https://api.parse.com/");
String path = "/123";
URL url = new URL(server, path);

The finial url is https://api.parse.com/123, no double slash.

@grantland
Copy link
Contributor

Awesome, LGTM

@grantland grantland assigned wangmengyan95 and unassigned grantland Jan 21, 2016
wangmengyan95 added a commit that referenced this pull request Jan 21, 2016
@wangmengyan95 wangmengyan95 merged commit 0a05514 into master Jan 21, 2016
@wangmengyan95 wangmengyan95 deleted the wangmengyan.fix_server_url_bug branch January 21, 2016 19:43
@richardjrossiii
Copy link
Contributor

Hey guys, we may want to re-investigate this. I just tested (locally, and here), it definitely converts a double slash into a path for root.

@wangmengyan95
Copy link
Contributor Author

Yep, I do a more through test here.

@grantland
Copy link
Contributor

Discussed in person. The original concern comes from the fact that the paths in iOS/.NET are defined with a leading /, so passing in a server path with a trailing / will yield // which changes the URL to root. We don't define paths in Android with leading /, so this is not an issue.

@grantland grantland mentioned this pull request Jan 22, 2016
4 tasks
@facebook-github-bot
Copy link

@wangmengyan95 updated the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants