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

Upgrade to STAC 0.9.0 #48

Merged
merged 11 commits into from
Mar 19, 2020
Merged

Upgrade to STAC 0.9.0 #48

merged 11 commits into from
Mar 19, 2020

Conversation

jisantuc
Copy link
Contributor

@jisantuc jisantuc commented Mar 16, 2020

Overview

This PR upgrades datatypes for compatibility with STAC 0.9.0. It makes no effort to maintain backwards compatibility, deferring that challenge to a future time.

Checklist

  • New tests have been added or existing tests have been modified

Notes

Testing with the home grown getPropTest for json serialization round trips has been replaced with CodecTests[T], which uses circe's JSON codec laws. This should minimize our exposure to surprising JSON behavior that lies just past the horizon of my understanding.

I don't think we can do much with the version extension here outside of add the link types, since the versioning logic will need to live in Franklin itself.

I parsed

Remove: [...] Asset Types (pre-defined values for the keys of individual assets, not media types) in Items. Use the asset's roles instead.

as "keys in the Assets dictionary in items are no longer semantic, but media types should still be around. I couldn't come up with another way to parse it that made sense. That also made it a no-op change for us.

Extension strategy

This also implements the first extension codec. The point of creating extension codecs without doing any checks on StacItems is to allow downstream consumers to decode the relevant extension fields when it's obvious a user wants them and not to freak out about them otherwise. Here's the diff necessary to implement and test case classes + codecs for the label extension:

$ git diff --stat 825ef10..HEAD
 modules/core/src/main/scala/com/extensions/label/LabelClass.scala               |  24 ++++++++++++++
 modules/core/src/main/scala/com/extensions/label/LabelClassClasses.scala        |  44 +++++++++++++++++++++++++
 modules/core/src/main/scala/com/extensions/label/LabelClassName.scala           |  30 +++++++++++++++++
 modules/core/src/main/scala/com/extensions/label/LabelCount.scala               |  24 ++++++++++++++
 modules/core/src/main/scala/com/extensions/label/LabelExtensionProperties.scala |  73 ++++++++++++++++++++++++++++++++++++++++
 modules/core/src/main/scala/com/extensions/label/LabelMethod.scala              |  26 +++++++++++++++
 modules/core/src/main/scala/com/extensions/label/LabelOverview.scala            |  38 +++++++++++++++++++++
 modules/core/src/main/scala/com/extensions/label/LabelProperties.scala          |  32 ++++++++++++++++++
 modules/core/src/main/scala/com/extensions/label/LabelStats.scala               |  21 ++++++++++++
 modules/core/src/main/scala/com/extensions/label/LabelTask.scala                |  31 +++++++++++++++++
 modules/core/src/main/scala/com/extensions/label/LabelType.scala                |  25 ++++++++++++++
 modules/core/src/test/scala/com/azavea/stac4s/core/Generators.scala             | 119 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 modules/core/src/test/scala/com/azavea/stac4s/core/SerDeSpec.scala              |  20 ++++++++++-
 13 files changed, 506 insertions(+), 1 deletion(-)

It was... a lot, but not that much, and it was pretty easy to keep track of, since I could add the case classes as necessary, add a checkAll("Codec.CaseClassName", ...) every time I added one, then follow the compiler around until it was finally happy. Total time on that work was about 2:30, so that should help us estimate the time cost of adding extensions by comparing them to the complexity of the label extension. I think it shows some promise and lets us keep up with the main STAC spec while adding extensions as we need them and deferring solving the hardest problem (decoding arbitrary mixes of extensions + common metadata fields) until probably never.

Testing

  • run tests

Closes #6

Closes #8

@jisantuc jisantuc changed the title [wip] Upgrade to STAC 0.9.0 Upgrade to STAC 0.9.0 Mar 16, 2020
case object ServiceDesc extends StacLinkType("service-desc")
case object ServiceDoc extends StacLinkType("service-doc")
case object Conformance extends StacLinkType("conformance")
case object Data extends StacLinkType("data")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These moved inside the object because Data is both a link type and an asset role and they need not to be in the same namespace

Copy link
Contributor

@aaronxsu aaronxsu left a comment

Choose a reason for hiding this comment

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

In addition to the minor things I commented below, I have a question about optional/not-required fields from spec in general: does it mean that if such key/field don't exist/appear in json, it should still be considered as a valid STAC object so that it should be decoded and encoded? Or is it like, the key needs to be there, the value can be null/None of an empty list/string/etc?

Another thing is about the type of the properties field in the STAC Item considering the datetime key in it. But this is out of scope of this issue and we may create a new issue to address it.

@jisantuc
Copy link
Contributor Author

Optionality has always been a little bit unclear to me. @lossyrob said a while ago that if something is null it shouldn't show up in the encoded json. That tells us less about decoding, but non-required lists are a bit of a weird case, since they have two empty states to think about, which is why a lot (all? I hope all) of them have decoders that check whether they're present at all and default to an empty list if they're not given (unless null has an explicit meaning in the spec, in which case I respected that meaning, like with null -> "it's raster data" in class names). Optionality means a lot of different things in difference places in the spec so it's a little tough to answer.

Copy link
Contributor

@notthatbreezy notthatbreezy 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 - do we need an issue in Franklin for the strategy of optional decoding?

@jisantuc jisantuc merged commit e7d6b2b into master Mar 19, 2020
@jisantuc jisantuc deleted the feature/js/stac-0.9-compliance branch March 19, 2020 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update for STAC version 0.9.0 Use discipline for serde testing
3 participants