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

The naive root + path might cause cases of double slashes e.g. http://path//relative-path. #109

Merged
merged 6 commits into from
Dec 10, 2013

Conversation

erlichmen
Copy link
Contributor

This indeed happened with the out of the box Google cloud endpoints GA discovery documents.
Switch to url.resolve() to solve those cases.

…/path//relative-path.

This indeed happened with the out of the box google cloud endpoints GA discovery documents.
Switch to url.resolve to solve this cases.
* @param {string} rootUrl
* @param {object} params
*/
var generateUploadPath = function(rootUrl, params) {

This comment was marked as spam.

2. fix the unit test to pass semi valid uri (one that begins with http://), non valid uris will not work
3. rename tests => test, this is the default conversation for mocha
@erlichmen
Copy link
Contributor Author

PR updated, I moved all the tests/* to be under test/* this is the default setup for mocha and will save time in when wanting to debug the project under pychram. npm test was modified as well.

I fixed the tests so that custom uri will begin with http:// and not just an arbitrary string, there is no real need (nor its valid) to pass arbitrary string as rootUri.

var root = root || '',
fullPath = '';

for (var i=0;i<paths.length;i++) {

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@erlichmen
Copy link
Contributor Author

Update again, less code for path normalization.

@tjwebb
Copy link

tjwebb commented Dec 9, 2013

I filed a similar issue: #114

@@ -100,6 +100,40 @@ BaseRequest.prototype.handleResponse = function(opt_fn) {
};

/**
* private: create the upload url for the method

This comment was marked as spam.

@rakyll
Copy link
Contributor

rakyll commented Dec 9, 2013

Added a few more comments.

Btw, I cant merge automatically, could you pull from upstream?

# By Burcu Dogan
# Via Burcu Dogan
* commit '3a421bb16fa5669c3578f7db8ada208d0ad75c7f':
  Testing request replays on auth failures for OAuth 2 client.
  Discovery should honor top level methods.
@erlichmen
Copy link
Contributor Author

I added a few more syntax fixes (thank you jshint and WebStorm) + merge with the latest.

rakyll pushed a commit that referenced this pull request Dec 10, 2013
The naive root + path might cause cases of double slashes e.g. http://path//relative-path.
@rakyll rakyll merged commit c025d3c into googleapis:master Dec 10, 2013
@rakyll
Copy link
Contributor

rakyll commented Dec 10, 2013

Thanks!

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.

3 participants