-
Notifications
You must be signed in to change notification settings - Fork 48
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
Ensure client self hrefs don't end in '/' #373
Conversation
Co-authored-by: Tom Augspurger <[email protected]>
Codecov ReportBase: 85.30% // Head: 85.31% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #373 +/- ##
==========================================
+ Coverage 85.30% 85.31% +0.01%
==========================================
Files 11 11
Lines 796 797 +1
==========================================
+ Hits 679 680 +1
Misses 117 117
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@gadomski I don't think this is a good idea. We recently had a similar case in STAC Browser:
Not sure why this API implementation works this way, but removing the slash from the "self URL" was also my first instinct, but quickly I got complaints. So I think the right fix for this can only be to construct the URLs correctly when merging URI parts. |
Hm, it's a good point -- this PR makes the assumption that we know what's best w.r.t. trailing slashes on the root endpoint, but that's not always the case. I'll cook up a more robust solution in a new PR. |
Some servers require it (per #373 (comment)).
Some servers require it (per #373 (comment)).
Some servers require it (per #373 (comment)).
Some servers require it (per #373 (comment)).
Related Issue(s):
Description:
Our url construction is a bit naïve -- we use basic string formatting, e.g.
pystac-client/pystac_client/client.py
Line 257 in 3f305e2
As raised in #369, this can lead to request urls containing double
//
, which breaks some APIs (e.g. out-of-the-box stac-fastapi). This PR does the simplest-possible fix by ensuring that aClient
's self href doesn't end in a slash.PR Checklist: