Skip to content
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

refactor link relation tables, make parent not a required field #407

Merged
merged 7 commits into from
Mar 20, 2023

Conversation

philvarner
Copy link
Collaborator

Related Issue(s):

Proposed Changes:

  1. parent link rel is not required for Collection or Item. In <= 1.0.0-rc.2, it was required for /collections/{collectionId}, and the requirement on /collections/{collectionId}/items/{itemId} was added since 1.0.0-rc.2. This aligns with it not being required in STAC Item and Collection definitions.
  2. Link relation tables are refactored to align with the style in stac-spec of using REQUIRED for required relations.

PR Checklist:

  • This PR has no breaking changes.
  • This PR does not make any changes to the core spec in the stac-spec directory (these are included as a subtree and should be updated directly in radiantearth/stac-spec)
  • I have added my changes to the CHANGELOG or a CHANGELOG entry is not required.

@philvarner philvarner added this to the 1.0.0-rc.3 milestone Mar 14, 2023
@philvarner philvarner requested review from tschaub, cholmes, m-mohr and matthewhanson and removed request for tschaub March 14, 2023 00:30
ogcapi-features/README.md Outdated Show resolved Hide resolved
Co-authored-by: Matthias Mohr <[email protected]>
@philvarner philvarner requested a review from m-mohr March 14, 2023 12:31
ogcapi-features/README.md Outdated Show resolved Hide resolved
ogcapi-features/README.md Outdated Show resolved Hide resolved
@tschaub
Copy link
Collaborator

tschaub commented Mar 14, 2023

Looks great, @philvarner. Thanks for fixing this up.

@m-mohr
Copy link
Collaborator

m-mohr commented Mar 14, 2023

I'm actually not sure whether we should drop the requirement. I feel like requiring the parent links (and potentially even more) would help in the long run. I think adding links in API implementations is much "cheaper" than in static catalogs so I'd think we could require more in the API compared to static catalogs.

@tschaub
Copy link
Collaborator

tschaub commented Mar 14, 2023

Maybe conditional requirements are too sloppy, but how about only requiring parent if an Item is not part of a Collection? I've only worked with Items whose "parent" is a Collection, so I'm not sure how common Collection-less Items are in STAC API implementations. It just struck me as odd to require that /collections/{collectionId}/items/{itemId} link to /collections/{collectionId} twice (once as parent and once as collection).

@m-mohr
Copy link
Collaborator

m-mohr commented Mar 14, 2023

That's weird as you'd need a condition in implementation, but I also don't think having them twice is an issue.
The same may occur in the collection where root and parent are pointing to the same thing.
It's pretty common in static STAC (although not required) so I feel we could at least recommend all of the links (self, root, parent, collection)?

Phil Varner and others added 2 commits March 14, 2023 19:47
@philvarner
Copy link
Collaborator Author

I think if we want to keep Collection parent as required and have Item parent as required (since it was inadvertently added by me without discussion), we can do that, but I'd prefer we have that as a future discussion for 1.0.0-rc.4. I prefer aligning them with STAC definitions for now. I really just want to get 1.0.0-rc.3 out.

@philvarner philvarner merged commit 13d3dac into radiantearth:main Mar 20, 2023
@philvarner philvarner deleted the pv/parent-link-rel branch March 20, 2023 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants