globus/globus-sdk-python

Cannot make request to BaseClient base_url without appending a trailing slash

jimPruyne opened this issue · 5 comments

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.

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...)

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.

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.

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.

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.