-
Notifications
You must be signed in to change notification settings - Fork 128
Fix for proper handling of relative links for assets of network-loaded STAC items #1599
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1599 +/- ##
==========================================
+ Coverage 92.29% 92.30% +0.01%
==========================================
Files 55 55
Lines 8395 8397 +2
Branches 971 971
==========================================
+ Hits 7748 7751 +3
Misses 458 458
+ Partials 189 188 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gadomski
left a comment
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.
This PR looks like it only fixes href resolution for assets. Any reason to not also consider href resolution for traversing links, e.g. resolving a "child" or "item" link?
Also, can you add a CHANGELOG entry? Thanks.
I was unsure it would be easy to make it work for the 'links', because it uses the |
|
I have added the CHANGELOG |
gadomski
left a comment
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.
I agree that implementing this for links will be much tricker, so I'm ok with just handling assets for now.
Can you add a test for get_absolute_href, since that's the API change that you're mention in the CHANGELOG? Thanks!
| for asset in self.assets.values(): | ||
| asset_href = asset.href | ||
| if not is_absolute_href(asset_href): | ||
| if not is_absolute_href(asset_href, prev_href): |
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.
Since we're only changing behavior for assets, do we need to change the set_self_href behavior on the item?
Co-authored-by: Pete Gadomski <[email protected]>
Co-authored-by: Pete Gadomski <[email protected]>
Related Issue(s):
Description:
when a STAC Item is loaded from a remote URL, pystac was not resolving correctly relative URL references (e.g.
/d/myfile) to absolute paths.This was due to the utils.is_absolute_href function checking always via
os.path.isabs, even if the STAC Item is from a non-filesystem URL.The patch proposed here applies the same principle of "utils.make_absolute_href", so to use the STAC item self value to determine if this is filesystem or network url and act accordingly (for network urls, the presence of the schema in front is considered absolute URL, instead a relative URL).
PR Checklist:
pre-commit run --all-files)pytest)