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

Cannot make request to BaseClient base_url without appending a trailing slash #363

Closed
jimPruyne opened this issue Jul 23, 2019 · 5 comments · Fixed by #364
Closed

Cannot make request to BaseClient base_url without appending a trailing slash #363

jimPruyne opened this issue Jul 23, 2019 · 5 comments · Fixed by #364

Comments

@jimPruyne
Copy link
Contributor

If I create a new BaseClient and set the base_url property there is no way to make an HTTP request off of the BaseClient to the base_url without a trailing slash (/) appended to the URL.

This is due to the manner in which the input url to the http method (e.g. get()) is appended to the base_url in slash_join(). No value of input url (one might naturally try just "") will result in the trailing slash not being appended.

@jaswilli
Copy link
Contributor

What's the use case (keeping in mind that neither BaseClient nor its base_url member are exactly public)?

Also, in general I would consider a server that responded to http://example.com but not http://example.com/ to be broken (perhaps on purpose in order to attempt to prove some point, but...)

@jimPruyne
Copy link
Contributor Author

So, on the former, BaseClient is really useful for building other tools on top of the SDK for working with other Globus Auth enabled services, but I'm no spot to request it be "public" if the SDK devs don't want it to be. If it is going to be of any use, though, base_url is pretty much required.

On the latter, there seems to be a technical distinction on existence of the trailing slash. I personally would (cough do cough) consider it broken, but at least Flask differentiates so one mis-using Flask may not get the desired "irrelevant trailing slash" behavior.

So, potentially easy fix of limited value. I won't argue against a quick close / no action on this.

@jaswilli
Copy link
Contributor

jaswilli commented Jul 24, 2019

So, on the former, BaseClient is really useful for building other tools on top of the SDK for working with other Globus Auth enabled services

Yeah, in the beginning the BaseClient constructor didn't even take base_url but that was added as a nod to allowing people to leverage the SDK without getting their service URL in the config file. I don't think anyone has any issue with that expansion. At its core though BaseClient exists to support the SDK proper so I'm always hesitant to make changes.

That said, I'm not against a change to make URLs generated from a base_url with a base_path of None not have a trailing slash if others think it's a good (or at least reasonable) change and we determine it's not likely to cause issues.

@jimPruyne
Copy link
Contributor Author

if others think it's a good (or at least reasonable) change and we determine it's not likely to cause issues.

I guess this is the key: it seemed simple enough but has this situation that it doesn't handle (probably outside the scope of initial implementation concerns, but a case in any event), and I wouldn't want introducing a different approach, perhaps more complicated, to foul up the important cases it already handles. I guess would require more thoughtful review than may initially be obvious.

@jaswilli
Copy link
Contributor

After thinking about this for a while I think I've moved from neutral to pro on this change:

  • If we're ever in a position where we just can't send trailing slashes we can do it.
  • More importantly, the SDK no longer needs to hold or defend any position on trailing slashes.

sirosen added a commit to sirosen/globus-sdk-python that referenced this issue Jul 26, 2019
BaseClient.get("") should call the base URL without appending a slash if
none was present. Makes it possible to call out to servers which care
about this sort of thing.

closes globus#363
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 a pull request may close this issue.

2 participants