Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Minor clean-up and more intro #58
Minor clean-up and more intro #58
Changes from 3 commits
13381aa
a4cc645
ae4b0f1
d223e44
19f0627
1dbdece
fe80830
cf46f0f
12f67c4
858caeb
ab3cc73
c6b586b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
"for any relevant image" -> "for any matching Items."
"not yet specified by OAFeat" suggests this is an upcoming capability, we should avoid making assumptions about the future.
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.
They have identified it as future work (in opengeospatial/ogcapi-features#451), and reference it in Filter/CQL - http://docs.opengeospatial.org/DRAFTS/19-079.html#filter-param-multiple-collections
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.
How to expose only STAC Collections in an API? This would make 10+ implementations in openEO not STAC API compliant any more!
In general, I changed my mind on this since commenting in #42. I think we should only require the landing page and from there just look what has been provided in the links. The issue is that clients will now expect that all those endpoints are accessible, but for (partly) private APIs this is not the case. For example, Radiant Earth has only /, /collections and /collections/:id publicly available and then the others need authentication. So by just pointing a client without credentials to this API, it will fail on all requests that require authentication, which is the same as just not having them implemented (like in openEO). If clients follow the RESTful approach, they just follow links and don't necessarily need any pre-defined endpoint to exist (except for a landing page).
Also, I think there's a mismatch in this PR: Either its RESTful as specified in the introduction (and thus only links are required to navigate the API, no specific endpoints required other than a landing page) OR we require certain endpoints, but then it's not strictly RESTful any longer IMHO.
If we stick with this requirement, it must be mentioned in the Changelog as this is a major change compared to 0.9.
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'm open on what exactly we do here, was just trying to capture what was in the PR.
My thinking was that a Collection only endpoint is actually not a STAC API. In my mind a STAC API is when you provide 'search'. OpenEO is a compliant STAC catalog, as is https://storage.googleapis.com/pdd-stac/disasters-0.9.0/catalog.json And I'd see implementations that are servers (RESTful API's) that only implement STAC core, and don't offer search/ or collections/items/ endpoints.
I think I agree with your point about RESTful, with no specific endpoints required, but we should be able to still specify functionality required in some way? Is OGC Features API RESTful?
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.
Unfortunately, it is not. We use the data rel type instead of child as it is specified in STAC API. So it's actually not a valid STAC catalog... And then /collections is also not valid STA, it's only valid in STAC API.
Why would that be required? If we have conformance classes anyway, couldn't we just specify what is implemented there or just follow links? I'm not sure myself yet, but I don't like that it's obviously getting towards just STAC API for Items and it seems I need to come up with my own STAC API spec for Collections. I mean it's a bit an issue of course that OGC API Features requires the item endpoints, too, and as such I'm basically only compliant to OGC API Commons, but then have STAC Collections of course. Anyway, we can't represent all static STACs in the API it seems...
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.
Ah, that's interesting on using data rel type instead of child, I'd thought they were child. I do think that's a confusing little area.
I'm not set on not having STAC API just for items, just saying that was more of my mental model. That everyone should implement STAC Core, and that you layer on Features API / Search endpoint for filtering items. And the latter is 'STAC API'.
But for beta.1 I prefer to not rock the boat - mostly just want to align with CQL / Transactions, clean things up, and get something for people to work with. I definitely see a beta.2, and I think the recommendations for how to implement STAC catalogs / collections is core to figure out, and this to me fits in. I think the conformance classes are probably the way to go. And I'm not set on my mental model, I can see value in having 'STAC API' specify how to implement Collections / Catalogs without search. But I also like that STAC Core can be implemented by a dynamic server, but that we don't need to specify that.
What does this mean?
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.
It makes sense, because it returns you a different JSON. Not a collection/catalog, but the response of /collections, which contains collections and links. /collections then lists the "children" instead of adding (in our case) 450+ links to the links in the landing page. It's just that this is not specified in STAC itself, but only in API (originating from Features).
Agreed, see #27.
Actually, I'd rather say we should have collection search at some point and then there might be no "without search" anylonger.
If we merge this PR, you need Items in collections/catalogs, otherwise the API is not compliant although it's a valid STAC.
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.
Hmm, my statement about adding child links to root is a bit biased by openEO and is also weakened by the discussion on Monday. I don't think there's a major issue with your implementation. child links at the landing page are fine, BUT I'd recommend to put them in /collections and link to it with rel type data if OAFeat is implemented anyway. Also, in an API that is not necessarily only STAC (as openEO is), it would bloat the initial request with data that is not required at that stage and thus connection times can slow down. In an API just covering STAC, I'd argue it's not a major problem as it's the primary goal/content of the API.
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 think I'd just remove the whole new chapter for now, merge this and then write-up the conformance classes. Those will basically change the whole chapter anyway.
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.
My main interest is in having the API browsable by STAC browser, so I think that will need an update to respect the
data
rel type link.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.
Yes, that was my plan - remove the whole chapter on compliance, and then redo it with the conformance PR.
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.
@matthewhanson I implemented the data rel type some days ago into STAC Browser: radiantearth/stac-browser#59 Already live on stacindex.org...