-
Notifications
You must be signed in to change notification settings - Fork 47
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
Use collection relation for links to collection #404
Conversation
I think both should be used for maximum interoperability in clients. It's also how static STAC does it. I'm pretty sure the proposed change would break STAC Browser to some extent as it expects a complete set of parent links from item to root. |
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.
comments inline
Please add a CHANGELOG entry per the PR template checkboxes. |
@m-mohr - there could be a breaking change I haven't noticed, but I made this change to get STAC Browser working. When I used the |
That sounds like a bug in the impl-- the parent of /collection/{c_id}/items/{item_id} is /collection/{c_id}, but I've seen implementions incorrectly point If you're able to share either the endpoint or the Item json, it would help to validate that. |
Also, I have no idea why the CircleCI build is failing. as long as npm check passes, I'm fine merging without it. |
9c68256
to
272bfe6
Compare
@tschaub The navigation issue in STAC Browser sounds like an issue in the underlying data. As @philvarner mentioned, usually the links in the data are note correct then. The navigation using parent and/or collection work in valid implementations afaik. Feel free to send me link or JSON document and I can verify what's going wrong. |
I've opened radiantearth/stac-browser#286 with an example item that uses |
Summarizing the current state of things as I understand it. The STAC Item spec describes the The STAC Collection spec has a note that requires that items in a collection link back to the collection with the It looks to me like the STAC API spec adds the requirement that an item in a collection also has a stac-api-spec/ogcapi-features/README.md Lines 155 to 161 in f7c2fb1
So a |
ogcapi-features/README.md
Outdated
Note that the `parent` link for an Item should point to the containing Collection | ||
(e.g., `/collections/{collectionId}`), rather than the API sub-path | ||
of `/collections/{collectionId}/items/`. | ||
The `parent` link for an item may point to a Collection or a Catalog. A `collection` link for an Item will always point to the containing Collection. Links to a Collection must point to the `/collections/{collectionId}` endpoint, rather than the API sub-path of `/collections/{collectionId}/items/`. |
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.
The `parent` link for an item may point to a Collection or a Catalog. A `collection` link for an Item will always point to the containing Collection. Links to a Collection must point to the `/collections/{collectionId}` endpoint, rather than the API sub-path of `/collections/{collectionId}/items/`. | |
The `parent` link for an Item may point to a Collection or a Catalog. The `collection` link for an Item will always point to the containing Collection. Links to a Collection must point to the `/collections/{collectionId}` endpoint, rather than the API sub-path of `/collections/{collectionId}/items/`. |
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.
Sorry, I was bit unclear about this in the call. The / at the end for non-files is usually only required in self links, but there it's very important. I would say it's also a best practice to follow with all other links, but it may not be so much of an issue there as usually you always resolve against self if present.
Here's the corresponding stac-spec PR: radiantearth/stac-spec#1214 @philvarner
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.
Here's an example for why this is an issue:
Response for a specific item:
- self link is:
https://example.com/collections/abc/items/123
- asset href is
./file.tif
- URI classes resolve this to:
https://example.com/collections/abc/items/file.tif
but it should behttps://example.com/collections/abc/items/123/file.tif
Adding the / at the end solves this issue. We've faced this a couple of times in clients and many implementation don't provide the slash, so we should make this very clear and also update the implementations accordingly. Fortunately we don't face it too frequently as many decide to make most links absolute, but for relative links it's a pain.
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.
References for issues due to missing traliing slashes:
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.
@m-mohr - I think this is probably better for a dedicated ticket, but it looks to me like you are talking about a problem with incorrect relative links rather than a problem with trailing slashes.
If the self
link is https://example.com/some/resource
and the URL for a related resource is https://example.com/some/resource/related
, then a correct relative URL is ./resource/related
.
I feel like requiring that URLs end with a slash may be overdoing it. And this will lead to inconsistencies with OGC API specs (OGC API – Features uses URLs like collections/{collectionId}
, collections/{collectionId}/items
, collections/{collectionId}/items/{itemId}
etc.).
I know that it can be easy to get relative URLs wrong, and that resolving absolute URLs can be tricky. But both can be done correctly without requiring that URLs end with a slash.
My personal opinion is that the OGC APIs and perhaps STAC are a bit overspecified in requiring both specific links and specific URLs. And I think things would be improved if OGC APIs allowed trailing slashes (it can be easier to stand up static sites that work with trailing slashes). But I know it is too late for that.
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.
@tschaub Indeed, I just posted it here because we stumbled across it in this PR during the STAC community call.
Whether it's an invalid self link or relative link is not so important I think, the main issue is that it resolves incorrectly. But indeed we should maybe also update the stac-spec to mention that you can also solve this by changing the relative links. It's just simpler to add a slash then update 20 relative links in links and assets...
My personal opinion is that the OGC APIs and perhaps STAC are a bit overspecified in requiring both specific links and specific URLs. And I think things would be improved if OGC APIs allowed trailing slashes (it can be easier to stand up static sites that work with trailing slashes). But I know it is too late for that.
I'm not sure... is it really specified in OGC APIs that trailing slashes are not allowed?
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.
But indeed we should maybe also update the stac-spec to mention that you can also solve this by changing the relative links. It's just simpler to add a slash then update 20 relative links in links and assets
I think I'm missing context. It sounds like you are starting with a problem that I don't know about. Does the spec have language or examples in it that create this problem?
is it really specified in OGC APIs that trailing slashes are not allowed?
No, the OGC specs do not forbid the use of trailing slashes. But they do require that services respond (with a 200) to requests without trailing slashes. For example, in the OGC API – Features spec, requirements 18 and 19 essentially say "requests to /collections/{collectionId}
must respond with a 200 that includes collection metadata." That doesn't mean that they cannot also respond to /collections/{collectionId}/
(with a trailing slash) – but as you know the content of the response in those two cases needs to be different if (dot) relative URLs are used in links. This pattern of URLs without trailing slashes is repeated throughout the Features spec and other OGC API specs (e.g. Tiles).
Again, I might not be seeing the problem that you are starting with, but I think if the STAC specs say anything about all this, it should be advice about being careful with (dot) relative URLs. I don't see trailing slashes as magically solving all problems resolving relative URLs. And I also don't see this as just a problem in resolving relative URLs against a "self" link. Some implementations may use the "self" link as the base, but it also seems reasonable that a client might use a request URL as a base and ignore the "self" link when resolving relative URLs (assuming that links in the response will be relative to the request). So I think that any best practice advice given in the spec should say that care needs to be taken to ensure that all links (self and other related links) are correctly formatted.
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.
=> radiantearth/stac-spec#1217
@philvarner If we prominently point to this in STAC spec, we could probably leave the API spec as it is...
@tschaub I think CI wasn't running because you didn't have write access to the repo -- it's either that, or you don't have the CirceCI github app configured for your account. (Ideally, we'd move to use GH instead of CircleCI for this build, but I haven't made the time to do that) |
Co-authored-by: Phil Varner <pvarner@element84.com>
7004544
to
196816a
Compare
I left a similar comment above, but I'll ask again here in case it wasn't clear: Is it intentional that the STAC API spec adds a REQUIREMENT that and Item have a The STAC Item spec does not require that an Item must have a Currently, the STAC API spec uses this language: stac-api-spec/ogcapi-features/README.md Line 155 in f7c2fb1
And the following table includes a |
I filed this issue: #406 I added that section since 1.0.0-rc.2, following the pattern of all of the other link rels that require I was going to say that the |
Related Issue(s):
Proposed Changes:
collection
relation type for links from an item to a collectionThe STAC Collection spec includes this language:
This proposed change makes the STAC API spec consistent with that requirement.
PR Checklist:
stac-spec
directory (these are included as a subtree and should be updated directly in radiantearth/stac-spec)