Absolute HREFs of assets #57
Conversation
|
One difficulty I ran into when developing this was that caching on |
|
|
|
#59 has the fix for the broken data links, thanks for catching that. I also had friction with the caching in development while testing that fix, we don't have a known-good workaround, but I think disabling the behavior altogether by default in dev is probably a good idea. |
moradology
force-pushed
the
feature/absolute-asset-hrefs
branch
from
3352e3c to
023a031
Compare
| ] | ||
| ] | ||
| }, | ||
| "links": [ |
There was a problem hiding this comment.
Just for clarity, a good test would be to add a new link here with a relative path to another item (collections/naip/items/{item_id}).
There was a problem hiding this comment.
It doesn't look like my original main review comment made it into the review - sorry if this test comment was confusing. I've re-added a comment about asset vs link hrefs in this follow-up review.
pcstac/pcstac/client.py
Outdated
| def absolute_asset_links(self, item: Item, base_url: str) -> Item: | ||
| """Convert relative asset links to absolute links""" | ||
| # Supporting the fields extension means skipping if assets aren't here | ||
| if "assets" in item and "image" in item["assets"]: |
There was a problem hiding this comment.
Rather than relative asset href links, I think the use case we're trying to support is for link hrefs to be relative and replaced with the current base_url. For example, we're expecting future collections that have a derivedfrom rel type, which is a link to another item in a different collection, accessed via the STAC API. The assets in all cases should always be absolute. For links, I think it's safe to assume any relative path should be made absolute, and we shouldn't need to check for specirfic rel types
There was a problem hiding this comment.
Updating this with the proposed tests, but it appears to me that stac-fastapi already does what is desired?(here: https://github.com/stac-utils/stac-fastapi/blob/c948dbfcd12fff0939d0bc3ecb907878ee217416/stac_fastapi/pgstac/stac_fastapi/pgstac/models/links.py#L105)
| ] | ||
| ] | ||
| }, | ||
| "links": [ |
There was a problem hiding this comment.
It doesn't look like my original main review comment made it into the review - sorry if this test comment was confusing. I've re-added a comment about asset vs link hrefs in this follow-up review.
moradology
force-pushed
the
feature/absolute-asset-hrefs
branch
from
541606d to
ba5cf15
Compare
pcstac/pcstac/client.py
Outdated
| @@ -72,7 +72,7 @@ def inject_collection_links(self, collection: Collection) -> Collection: | |||
|
|
|||
| return collection | |||
|
|
|||
| def inject_item_links(self, item: Item) -> Item: | |||
| def inject_item_links(self, item: Item, base_url: str) -> Item: | |||
There was a problem hiding this comment.
We can drop this base_url arg now
pcstac/pcstac/client.py
Outdated
| @@ -176,11 +176,13 @@ async def _fetch() -> ItemCollection: | |||
| # Remove context extension until we fully support it. | |||
| result.pop("context", None) | |||
|
|
|||
| base_url = str(kwargs["request"].base_url) | |||
There was a problem hiding this comment.
Same here, we don't need to pass in base_url to the inject method anymore
Description
This PR adds conditional modification of
Itemasset hrefs so that relative paths to image assets in the DB are rendered as absolute for users. While adding this feature, it was also clear thattilejsonandmetadataassets could be rendered as absolute on the assumption that the only change necessary is replacement of/stac/with/data/- I could use a second pair of eyes on this assumption as it is possible I'm not imagining a failure condition which makes this solution unworkable.Type of change
Please delete options that are not relevant.
How Has This Been Tested?
I've manually tested this behavior and added an item with ID
al_m_3008501_ne_16_060_20191109_20200114_relative_path_testthat should trigger relative->absolute path transformation. As pgstac 0.4.5 is necessary to run things locally, a couple of tests are commented out due to incompatibility with changes on this version of pgstac.Checklist:
Please delete options that are not relevant.