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.