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
Browseable recommendations and Children conformance class #229
Browseable recommendations and Children conformance class #229
Changes from 6 commits
67bfa4ba7cfb0884bc36cf12c2ad7981ce35beb369969d125369ae529abae23b568773a40b9dec0cda8bd7fbd6df8d3d107b6bd6f788be5e3bcf17bbe01abbc128863966d433491af9cb6d43790c8829a5a694d55bd2d2ecaa5ef103200a54443078d6581b4f112c012a54b6a435fb5a8cc16425353b53File 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.
Are catalogs returned by this endpoint allowed to implement item search (i.e. can they have a
/searchendpoint)?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 landing page is just a catalog, and itself implements
/search, so I'm guessing this is allowed. And the/searchresponse would only return items that are contained by the catalog (potentially across many 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.
Yes, any (sub) Catalog can implement a search endpoint.
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.
Thanks for bringing this up. I thought I'd explicitly mentioned this, but I can't find it, so I'll make sure to add something.
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 confused if the server is expected to return just the first generation of children, the last generation of children, or the entire ancestry. For example, if I had Landsat catalogs organized as
/catalogs/landsat_8_c1/{path}_{row}_{date}, the/childrenendpoint could:{path}catalogs (immediate children).{path}_{row}_{date}(last generation of children).{path},{path}_{row}, and{path}_{row}_{date}(entire ancestry).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... is this a conformance class a "simple" way for providers to express their "preferred list of catalogs and collections to show on a frontpage"?
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.
To answer @geospatial-jeff 's question, it's up to the implementer. I think typically it will return exactly the same set of children that have
childrelations from the root. The benefit to having it at this endpoint is that the title, desc, etc can all be returned, so a client doesn't have to retrieve every single child link just to find out the title to display -- this was Rob's use case.Maybe "simple" isn't the right word. I'll think about this.
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 that returning both Catalogs and Collections in the
/childrenendpoint is confusing. My understanding after reading this PR is that collections are for searching while catalogs are for browsing. It even says in theBrowseable Catalogsbest practices that collections should not be included as part of the browseable tree of catalogs. Allowing collections to be used for browsing is poor separation of concerns, and instead of just recommending against their use the spec should forbid using collections in this way.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.
Yeah, this is a bit confusing. Like do I not need to retrieve /collections anymore if /children is implemented? How should clients handle all the duplication to show a list of unique catalogs/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.
I can see the benefit of having
childrenreturn both Catalogs and Collections - it more fits with the STAC core specification which usesrel=childfor both of those cases. It also allows for all of the catalogs and collections to be retrieved in a single paginated call - useful for rendering the content of the API.One addition we could make is that a query parameter can determine which type to return - e.g.
Where is the best practices text you referenced located?
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's included as part of https://github.com/philvarner/stac-api-spec/blob/catalogs-and-browseable/core/README.md#browseable-catalogs introduced in this PR. The exact language is:
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'll see if I explicitly stated it anywhere, but the intention was that
/childrenwould return the same list of entities that are linked to viarel=childfrom the root. The benefit (as Rob has articulated before) is that a client can get all of the entities with one call instead of having to make one HTTP request for each one to, say, only get the title of the collection or catalog.