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

Link "type" field required for all link relations, clarify media types in examples #368

Merged
merged 17 commits into from
Mar 13, 2023

Conversation

philvarner
Copy link
Collaborator

@philvarner philvarner commented Jan 13, 2023

Related Issue(s):

Proposed Changes:

  1. Require Link type field to conform with OAFeat for STAC API - Features endpoints (but not Item Search)
  2. clarify media types in a many places.

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 marked this pull request as draft January 13, 2023 15:42
@philvarner philvarner marked this pull request as ready for review January 16, 2023 18:11
@m-mohr
Copy link
Collaborator

m-mohr commented Jan 16, 2023

Should we also required media types in assets? I mean basically they are also just links and I don't see a good reason why links should have them but assets not.

@philvarner
Copy link
Collaborator Author

Should we also required media types in assets? I mean basically they are also just links and I don't see a good reason why links should have them but assets not.

I agree, but that has to be an upstream change in stac-spec, since we don't re-define Asset in stac-api-spec. I think we should also make type required for Link in stac-spec, not just the objects in stac-api that use it.

@m-mohr
Copy link
Collaborator

m-mohr commented Jan 27, 2023

Requiring link media types is also an upstream change: radiantearth/stac-spec#1202

I don't really see a difference between assets and links, in both case we would require more than what we require in stac-spec.
The only difference is that in case an implementor implements the OGC API conformance classes (which is not required), he has also to add types because they are required by OGC APIs.

Having second thoughts right now... While I'm generally +1 to require type because it just helps clients a lot, it could become an issue that if you "harvest" static STAC catalogs into an API and they don't have media types, you can't simply make them available in the API anymore if the type field is required. So maybe we shouldn't enforce this in RC phase without a bit more consultation of implementers?

@m-mohr
Copy link
Collaborator

m-mohr commented Jan 27, 2023

I randonly stumbled across another use-case where the type is not clear and as such having it required may lead to some confusion: https://github.com/stac-extensions/web-map-links

In the web-map-links extension we use templates URIs to a service. It doesn't really have a unique media type... like what's the media type of a WMTS URL? XML? PNG? JPEG? ...

@philvarner
Copy link
Collaborator Author

There are several different cases:

  1. A template isn't a valid href, so this is somewhat of a misuse of a Link -- but it's done all the time. If it's an xyz tile layer with a template like /tiles/{z}/{x}/{y}.png, it should be It should be image/png, or if it's /tiles/{z}/{x}/{y} and returns jpeg by default, image/jpeg. If you don't know what it returns or the endpoint might return different types for different data, then the default MIME type of application/octet-stream should be used.

  2. For the WMTS capabilities doc, it's the root, which I assumes returns something even without the parameter. In that case, it should be application/xml. In the general case where there's a text doc of some sort, it should default to text/plain.

I'd also say that maybe the url template itself shouldn't be a Link, but rather the Link should be to a description of the tile service with the template defined. This is what EOD does:

https://eod-catalog-svc-prod.astraea.earth/collections/sentinel2_l2a/items/S2A_OPER_MSI_L2A_TL_EPAE_20190527T094026_A020508_T46VCQ_N02.12

There is a link with a valid URL:

{
"href": "https://eod-catalog-svc-prod.astraea.earth/collections/sentinel2_l2a/items/S2A_OPER_MSI_L2A_TL_EPAE_20190527T094026_A020508_T46VCQ_N02.12/maplayer?format=zxy",
"rel": "maplayer-zxy",
"title": "TMS Map Layer without Ratio",
"type": "application/json"
},

And then that link returns a body with the template for the TMS layer:

{
"bounds": [
89.03962362089236,
62.0909077211768,
91.27017682914958,
63.11825012810462
],
"links": [],
"maxZoom": 16,
"minZoom": 8,
"tmsFlag": false,
"tmsLayer": "[https://ifw0t8uxa4.execute-api.eu-central-1.amazonaws.com/prod/sentinel2_l2a/tiles/{z}/{x}/{y}.png?item_id=S2A_OPER_MSI_L2A_TL_EPAE_20190527T094026_A020508_T46VCQ_N02.12&assets=B04_10m,B03_10m,B02_10m&nodata=0&color_formula=Gamma+RGB+3.5+Saturation+1.7+Sigmoidal+RGB+15+0.35](https://ifw0t8uxa4.execute-api.eu-central-1.amazonaws.com/prod/sentinel2_l2a/tiles/%7Bz%7D/%7Bx%7D/%7By%7D.png?item_id=S2A_OPER_MSI_L2A_TL_EPAE_20190527T094026_A020508_T46VCQ_N02.12&assets=B04_10m,B03_10m,B02_10m&nodata=0&color_formula=Gamma+RGB+3.5+Saturation+1.7+Sigmoidal+RGB+15+0.35)"
}

I can't remember if that body is a standardized format or not, but I think it is.

@tschaub
Copy link
Collaborator

tschaub commented Jan 30, 2023

The OGC API – Tiles spec is an example where a link with a templated URL is required (for Tileset Conformance). As @philvarner points out, these still include the relevant type of the content to be returned. This precludes the use of a "format" (or other) template variable - but I think the idea is that people would provide multiple templated links in the case where users might be able to request multiple content types.

@philvarner
Copy link
Collaborator Author

The OGC API – Tiles spec is an example where a link with a templated URL is required (for Tileset Conformance). As @philvarner points out, these still include the relevant type of the content to be returned. This precludes the use of a "format" (or other) template variable - but I think the idea is that people would provide multiple templated links in the case where users might be able to request multiple content types.

I don't think it would preclude the use of a format template var -- it just means you have to use application/octet-stream instead of an accurate type.

@m-mohr
Copy link
Collaborator

m-mohr commented Jan 30, 2023

Yeah, I'm following @philvarner's suggestion now. I use templated URIs as href, but set the media type to the expected output file format.

cholmes
cholmes previously approved these changes Jan 31, 2023
Copy link
Collaborator

@cholmes cholmes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. There's quite a few formatting changes in here - might be nice to just do all the relevant formatting changes (remove trailing spaces, additional spacing) in one PR that's just those. And also could be good to have formatting in CI, update it all, and then enforce it.

m-mohr
m-mohr previously requested changes Jan 31, 2023
Copy link
Collaborator

@m-mohr m-mohr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we drifted a bit away with my questions regarding the web-map-links / templated URIs etc, I think it's still worth discussing the type requirement for links and assets in a STAC call or so. It feels weird to require them in links but not in assets although it feels they are much more valuable for assets. Not directly requesting a change here, but more a discussion :-) If this can't wait for the next STAC community meeting, also happy to bring it up in the PSC.

@philvarner
Copy link
Collaborator Author

As we drifted a bit away with my questions regarding the web-map-links / templated URIs etc, I think it's still worth discussing the type requirement for links and assets in a STAC call or so. It feels weird to require them in links but not in assets although it feels they are much more valuable for assets. Not directly requesting a change here, but more a discussion :-) If this can't wait for the next STAC community meeting, also happy to bring it up in the PSC.

I created #387 to track the discussion.

In this case, requiring the Link type aligns us with OAFeat, so I feel we should merge this PR to do that. I'm in favor of also requiring it for Asset, but that's a separate decision.

@philvarner philvarner dismissed cholmes’s stale review February 14, 2023 02:09

removed restriction that 'type' is required

@philvarner
Copy link
Collaborator Author

Updated text to reflect discussion at 2023-Feb-13 STAC meetup.

@philvarner
Copy link
Collaborator Author

@m-mohr please re-review.

@m-mohr
Copy link
Collaborator

m-mohr commented Feb 14, 2023

Hmm, this is a bit different to what I tried to express yesterday, but maybe that's not even possible. Let me try to explain it again:

Assuming I'm implementing STAC API - Features, there are a couple of conformance classes involved, which includes http://www.opengis.net/spec/ogcapi-features-1/1.0/req/core (in the following "OAF") and https://api.stacspec.org/v1.0.0-rc.2/ogcapi-features (in the following "STAC"). My proposal yesterday was:

  • If you have only OAF in the conformance classes: Types in links are required
  • If you have only STAC in the conformance classes: Types in links are NOT required
  • If you have both listed: Types in links are required

I wasn't aware yesterday that OAF seems to be required for the STAC conformance class, which makes this a bit akward. Possible solutions would be to not require OAF or to use the proposal from this PR. I think I'd prefer the former, but I'm not sure what others think. I prefer this because it leaves the option to omit the type in the use case described yesterday: ingest non-typed links from a static catalog.

@gadomski As you were also involved yesterday, what did you understood? Do you have a preference?

@gadomski
Copy link

I wasn't aware yesterday that OAF seems to be required for the STAC conformance class, which makes this a bit akward.

Agreed ... I was under the mistaken impression that an API with STAC API - Features could also implement OGC-API - Features, but did not have to. If all (compliant) STAC API - Features APIs must implement OGC-API - Features, and OCG-API - Features requires type in links, then there's really no choice, right? type is required.

How hard-and-fast is the depends on relationship between STAC and OAF? To @m-mohr's point, it'd be nice to give users a spec-compliant way to not have type. If possible, I'd be in favor of removing that depends on relationship, but I don't have a good sense of the ramifications.

I am 👍🏽 to providing type in the spec tables for links -- when I was fixing up stac-fastapi against stac-api-validator, I just let stac-api-validator tell me what the correct type should be; it would have been nice to have that information in the spec itself for quick reference.

@m-mohr
Copy link
Collaborator

m-mohr commented Feb 14, 2023

Yeah, I think my take would be to not require OAFeat for STAC API Features? We can recommend it with the note that this would include other requirements such as a "type" for links. On the other hand, STAC API - Features would be relatively loosely defined because we inherit many other requirements that we don't explicitly describe. Hmmm...

@philvarner
Copy link
Collaborator Author

I've always read this such that STAC API - Features defined a superset of behavior over OAFeat. I think this is more explicit in the changes in 1.0.0-rc.2 and this PR, but I looked at beta.5 and I think it's defined this way there, too.

If we decide to break compatibility, I think we'll need to more fully define STAC API - Features, as we can't defer any absent or imprecisely defined behavior to the OAFeat spec.

I'm strongly in favor of keeping our dependence on OAFeat.

Comment on lines 103 to 105
While the STAC definition does not require the existence of the `type` field in
all Link objects, this is required by OAFeat and is thereby required by the
*STAC API - Features* conformance class.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
While the STAC definition does not require the existence of the `type` field in
all Link objects, this is required by OAFeat and is thereby required by the
*STAC API - Features* conformance class.
All Link objects SHALL have a `type` field, as this is required by OAFeat and is thereby required by the
*STAC API - Features* conformance class.
If the target of a Link's `type` is unknown, `type` SHOULD be set to `application/octet-stream` or `text/plain`.

If we're keeping the direct dependency on OGC-API - Features (#368 (comment)), this language should be made stronger IMO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use must instead of shall, but I agree with the strenth of the wording and will make a change like that.

@philvarner
Copy link
Collaborator Author

I've thought about this more, and my belief that STAC API - Features should extend OAFeat is stronger now. I think there are two main reasons, one conceptual and one practical:

Conceptually, I think the STAC specs have always taken the approach of extending other specs when possible. The most obvious of this that Item and ItemCollection extend GeoJSON Feature and FeatureCollection.

Practically, as Matthias has stated with "On the other hand, STAC API - Features would be relatively loosely defined because we inherit many other requirements that we don't explicitly describe." We could just state that any behavior not otherwise defined defers to whatever OAFeat says except in this one case, which is reasonable.

I also think it's okay to define the spec tightly, but also recognize that there will be implementations that don't precisely conform to it. For example, we're not going to write pystac-client so that it crashes if any asset is ever missing a type field, but the validator might complain if you don't, in spirit of Postel's law. In practice, if you leave out type there's no real consequences, you're just non-compliant with the spec -- not a big deal!

@gadomski
Copy link

gadomski commented Feb 19, 2023

In practice, if you leave out type there's no real consequences, you're just non-compliant with the spec -- not a big deal!

If we create a certain requirement in a spec, but we expect a lot of folks to not follow that requirement, is that a smell? I.e. is that an indication that our spec is mis-aligned with the real world and how people would like to use our spec? OR, is this the sort of thing were we expect lots of non-compliance early, but anticipate a slow glide towards compliance as the main implementations pick up this new piece of the spec?

Mostly a curiosity question -- I'm new to this spec work, so I don't have great instincts about how horses lead carts, etc.

@philvarner
Copy link
Collaborator Author

I think the difference is that implementations would be accidentally doing it wrong, rather than intentionally doing it differently. I think this is just one of many things that people do wrong, of the hundreds of things they have to do to conform to STAC and STAC API specs.

@philvarner
Copy link
Collaborator Author

One more bit of evidence that the intention was for STAC API - Feature to extend OAFeat -- the conformance class is named ogcapi-features.

@philvarner philvarner changed the title clarify media types in examples Link "type" field required for all link relations, clarify media types in examples Feb 28, 2023
@philvarner
Copy link
Collaborator Author

I updated the language so that all conformance classes require Link type field now.

Ready for re-review.

@philvarner philvarner requested review from cholmes and gadomski March 6, 2023 13:26
@philvarner
Copy link
Collaborator Author

Please re-review @m-mohr @matthewhanson @cholmes

Co-authored-by: Tim Schaub <tschaub@users.noreply.github.com>
Copy link
Collaborator

@m-mohr m-mohr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I won't have time to look at it in March, sorry. Hope this removes/resets my "block" from the requested changes?!

Did we came to a conclustion regarding a type requirement in assets? If we can require it in links, we can likely also require it there.

@m-mohr m-mohr dismissed their stale review March 6, 2023 14:28

Likely solved

Copy link

@gadomski gadomski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you added a media type to some example assets as well -- do we want to include any text around the type field in assets? Or is that a separate / non issue?

@philvarner
Copy link
Collaborator Author

I see you added a media type to some example assets as well -- do we want to include any text around the type field in assets? Or is that a separate / non issue?

the specs that are referenced say that it's not required, so those are just examples of best practice. I want to avoid adding more text.

@philvarner philvarner merged commit 194e525 into radiantearth:main Mar 13, 2023
@philvarner philvarner deleted the pv/clarify-media-types-2 branch March 13, 2023 13:05
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.

5 participants