Extension typeclasses #85

jisantuc · 2020-05-14T22:01:23Z

Overview

This PR adds typeclasses for extending each kind of STAC object. It also updates the case classes for each type of STAC object to include extensionFields, since otherwise it was gonna be really hard to write the companion objects with instance constructors for the typeclasses. 🤷‍♂️ ~~It's also failing because I did something wrong with collections.~~ nevermind!

Checklist

New tests have been added or existing tests have been modified
Changelog updated

Notes

There's a bit of a tradeoff here between verbosity of encoders / decoders for the base types and the convenience of extending them. I think it's a good trade, since the base types shouldn't change much, but we want to add a lot of extensions, but someone could argue otherwise.

Testing

bloop console core-test
check this out as an example of how this works:

@ import org.scalacheck.Arbitrary.arbitrary
@ import com.azavea.stac4s._
@ import com.azavea.stac4s.Generators._
@ import io.circe.syntax._
@ val collection = arbitrary[StacCollection].sample.get
@ collection.extensionFields // if you got an empty json object, sample and reassign
@ colleciton.asJson.spaces2 // check out your assets as a normal top level field

Closes #82

Closes #81

Closes geotrellis/geotrellis-server#152

jisantuc

This wound up a lot bigger than it felt like, so here are some explanatory comments. Also I didn't test the label items extension because I'd have to write a lot of generators, but I'm happy to tack that on -- I'm at 6 hours on a 2 point card + a 1 point card, so there's time for sure

jisantuc · 2020-05-15T17:20:29Z

modules/core/src/main/scala/com/azavea/stac4s/ItemCollection.scala

+        )
+      )
+
+      baseEncoder(collection).deepMerge(collection.extensionFields.asJson)


This is the first big increase in verbosity-- everything with extensionFields has a baseEncoder that gets mashed together with whatever's in the extensionFields attribute

jisantuc · 2020-05-15T17:21:04Z

modules/core/src/main/scala/com/azavea/stac4s/ItemCollection.scala

+      c.get[Option[List[String]]]("stac_extensions"),
+      c.get[List[StacItem]]("features"),
+      c.get[Option[List[StacLink]]]("links"),
+      c.value.as[JsonObject]


Here's the second part of the big increase in verbosity -- derivation is no longer correct for any of these, since we need "root document without the vanilla fields"

jisantuc · 2020-05-15T17:23:24Z

modules/core/src/main/scala/com/azavea/stac4s/extensions/ItemExtension.scala

+import io.circe._
+import io.circe.syntax._
+
+// typeclass trait for anything that is an extension of item properties


This was the first one I wrote, so it's where the comments are, even though it's not first in the file order. It's pretty basic, since StacItems don't have extensionFields, they just toss stuff into properties, but it explains the general strategy.

jisantuc · 2020-05-15T17:24:25Z

modules/core/src/main/scala/com/azavea/stac4s/extensions/LinkExtension.scala

+  def extend(link: StacLink, extensionFields: T): StacLink
+}
+
+object LinkExtension {


This is the simplest example with extensionFields

jisantuc · 2020-05-15T17:25:56Z

modules/core/src/main/scala/com/azavea/stac4s/extensions/asset/AssetCollectionExtension.scala

+import io.circe._
+import io.circe.syntax._
+
+final case class AssetCollectionExtension(


It turns out we'd implemented the asset extension? I don't know why we did that. But I moved it to the appropriate zone. We could consider moving implemented extensions into another module, or a module per extension or something? I don't think the second one is great because there are tons of extensions but the first one might be reasonable

jisantuc · 2020-05-15T17:26:14Z

modules/core/src/main/scala/com/azavea/stac4s/extensions/asset/StacCollectionAsset.scala

@@ -1,4 +1,6 @@
-package com.azavea.stac4s
+package com.azavea.stac4s.extensions.asset


moved because it's an extension

modules/core/src/test/resources/catalogs/landsat-stac/landsat-8-l1/catalog.json

jisantuc · 2020-05-15T17:28:52Z

modules/core/src/test/scala/com/azavea/stac4s/Generators.scala

+  private def collectionExtensionFieldsGen: Gen[JsonObject] = Gen.oneOf(
+    Gen.const(().asJsonObject),
+    Gen
+      .mapOf(
+        (nonEmptyStringGen, stacCollectionAssetGen).tupled
+      )
+      .map(AssetCollectionExtension.apply)
+      .map(_.asJsonObject)
+  )
+
+  private def linkExtensionFields: Gen[JsonObject] = Gen.oneOf(
+    Gen.const(().asJsonObject),
+    Gen
+      .nonEmptyListOf(nonEmptyStringGen)
+      .map(NonEmptyList.fromListUnsafe)
+      .map(LabelLinkExtension.apply)
+      .map(_.asJsonObject)
+  )


by adding these generators, we can ensure that with and without the extension fields, we can still roundtrip

notthatbreezy

I think this looks great - the comments are helpful and it looks like a good strategy. There are a few minor comments but nothing major. The comment about changing asset roles to a set can be left for a new issue if there are downstream consequences that make it more than a simple switch.

I think if you still have time to write tests for the label extension(s) that would be good since we'll want to encourage writing tests for extensions as we go forward.

modules/core/src/main/scala/com/azavea/stac4s/ItemCollection.scala

notthatbreezy · 2020-05-18T19:25:01Z

modules/core/src/main/scala/com/azavea/stac4s/StacCatalog.scala

 )

 object StacCatalog {

+  val catalogFields = Set(


This definition of "fields" seems like the riskiest part of this strategy. Do you think there is a way to write tests that would catch incorrectly adding or removing fields?

It's not hard to ensure that no fields from the case class are also keys in the extensionFields json object -- we can use LabelledGeneric from shapeless to extract the field names, and I think we can just map _type => type in the names and end up with something that mostly works. I'm trying to think of whether this is a terrible idea for some reason I haven't thought of yet. I have end-of-day brain though so I'll think about it again tomorrow morning.

I actually ran into this issue while working on this PR -- I'd forgotten the filter in collection and had about a half hour of trouble figuring out what was wrong with my collection json round trips.

I like the idea of using LabelledGeneric a bunch - seems like something that will be useful to keep the strings in sync.

ade5ae9 -- I tried out the generic strategy in the linked blog post but had trouble with the implicits, so we have a less generic generics strategy here

@jisantuc what was decided btw?

They're used in the linked commit

modules/core/src/test/resources/catalogs/landsat-stac/landsat-8-l1/catalog.json

modules/core/src/main/scala/com/azavea/stac4s/extensions/layer/LayerItemExtension.scala

modules/core/src/main/scala/com/azavea/stac4s/StacItemAsset.scala

pomadchin

LGTM in general; I left a couple of minor comments. And I like the general idea 👍

I think that it would be really very useful to add some usage examples into CatalogLayer spec (it is the only "real" spec and also the only simple usage example of stac4s)
What do you think about adding the com.azavea.stac4s.syntax package? Without syntax it is not very obvious how to use it.

// looks really complicated
ItemExtension[LayerItemExtension].getProperties(item)

How about smth like

item.getProperties[LayerItemExtension]
item.addProperties[T]
// or smth like
item.getPropertyExtension[T]

modules/core/src/main/scala/com/azavea/stac4s/StacCatalog.scala

modules/core/src/test/scala/com/azavea/stac4s/CatalogSpec.scala

modules/core/src/main/scala/com/azavea/stac4s/extensions/CatalogExtension.scala

pomadchin · 2020-05-18T21:57:13Z

modules/core/src/main/scala/com/azavea/stac4s/extensions/ItemCollectionExtension.scala

+          itemCollection.extensionFields.asJson.hcursor
+        )
+
+      def extend(itemCollection: ItemCollection, extensionProperties: T) =


Missing return type in a public method here, can be nice to have it for users.

Adding, but since this is evidence for a typeclass defined like 12 lines up... is it really missing? I'm curious about the custom here. extend has a required return type from the trait

modules/core/src/main/scala/com/azavea/stac4s/extensions/layer/LayerItemExtension.scala

modules/core/src/test/scala/com/azavea/stac4s/CatalogLayerSpec.scala

modules/core/src/main/scala/com/azavea/stac4s/StacItemAsset.scala

jisantuc · 2020-05-19T15:51:18Z

syntax package added in 36721b3. I believe that's all the comments so I'm re-requesting reviews

pomadchin · 2020-05-19T16:00:33Z

modules/core/src/main/scala/com/azavea/stac4s/ItemCollection.scala

+  private val generic      = LabelledGeneric[ItemCollection]
+  private val keys         = Keys[generic.Repr].apply
+  val itemCollectionFields = keys.toList.flatMap(field => substituteFieldName(field.name)).toSet
+


Can we put it into a single function to reduce some boilerplate?

That's the thing I had trouble with implicits with -- it wasn't obvious to me how to get the implicit evidence required to have getFieldNames[T] cooperate. Specifically, the evidence provided in the example blog post:

implicit def toAttributes[T, Repr <: HList, KeysRepr <: HList]( implicit gen: LabelledGeneric.Aux[T, Repr], keys: Keys.Aux[Repr, KeysRepr], traversable: ToTraversable.Aux[KeysRepr, List, Symbol] ):

was insufficient for Keys[generic.Repr] when I tried making it generic.

notthatbreezy

This looks good to me - I like @pomadchin's suggestion to reduce boilerplate, but that change wouldn't require another review IMO.

jisantuc added 10 commits May 14, 2020 13:57

Add item extension typeclass and LabelProperties evidence

d4eea57

expunge superfluous label:assets on links

a542890

add linkextension typeclass, evidence for LabelLinkExtension

dbfed6d

add item extension evidence to layer extension, rename

a28eccd

add label link extension to serdespec for links

6228757

add catalog and asset extensions

d007214

add rest of extensions / tests (failing)

949f318

fix collection decoder

dba2160

update testing static json

c97c2cb

add itemcollection extension

c842be5

jisantuc changed the title ~~[wip] Extension typeclasses~~ Extension typeclasses May 15, 2020

jisantuc added 4 commits May 15, 2020 10:58

update pr template

02b2e5b

Update changelog

304f2a2

Move asset extension type to extensions

5b3aec8

test asset extension

3f9cfb4

jisantuc requested a review from notthatbreezy May 15, 2020 17:18

jisantuc assigned notthatbreezy May 15, 2020

jisantuc requested a review from pomadchin May 15, 2020 17:18

jisantuc assigned pomadchin May 15, 2020

Update comment

d95ba5e

jisantuc commented May 15, 2020

View reviewed changes

notthatbreezy suggested changes May 18, 2020

View reviewed changes

jisantuc added 2 commits May 18, 2020 17:13

layer ids string list -> non empty string list

5c1d81f

nonemptystrings and sets oh my

2e89e74

pomadchin requested changes May 18, 2020

View reviewed changes

layer extension requires nonemptylist

9a349f5

notthatbreezy reviewed May 19, 2020

View reviewed changes

modules/core/src/main/scala/com/azavea/stac4s/StacItemAsset.scala Show resolved Hide resolved

jisantuc added 3 commits May 19, 2020 08:33

provide default for extensionFields

aa8a265

s/getProperties/getExtensionFields

75a67f9

s/extend/addExtensionFields

680f399

jisantuc added 5 commits May 19, 2020 08:56

fix and test layer item extension decoder

6dcbdc1

test that item ser-de works with extensions

853d9b6

derive case class fields with shapeless

ade5ae9

add syntax package and test where possible

36721b3

lint

8f0ad3e

jisantuc requested review from notthatbreezy and pomadchin May 19, 2020 15:51

pomadchin reviewed May 19, 2020

View reviewed changes

notthatbreezy approved these changes May 19, 2020

View reviewed changes

pomadchin approved these changes May 19, 2020

View reviewed changes

jisantuc merged commit 7adf740 into master May 19, 2020

jisantuc deleted the feature/js/extension-typeclasses branch May 19, 2020 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extension typeclasses #85

Extension typeclasses #85

jisantuc commented May 14, 2020 •

edited

Loading

jisantuc left a comment •

edited

Loading

jisantuc May 15, 2020

jisantuc May 15, 2020

jisantuc May 15, 2020

jisantuc May 15, 2020

jisantuc May 15, 2020

jisantuc May 15, 2020

jisantuc May 15, 2020

notthatbreezy left a comment

notthatbreezy May 18, 2020

jisantuc May 18, 2020

notthatbreezy May 19, 2020

jisantuc May 19, 2020

pomadchin May 19, 2020

jisantuc May 19, 2020

pomadchin left a comment •

edited

Loading

pomadchin May 18, 2020

jisantuc May 19, 2020

jisantuc commented May 19, 2020

pomadchin May 19, 2020

jisantuc May 19, 2020

jisantuc May 19, 2020

notthatbreezy left a comment

		@@ -1,4 +1,6 @@
		package com.azavea.stac4s
		package com.azavea.stac4s.extensions.asset

Extension typeclasses #85

Extension typeclasses #85

Conversation

jisantuc commented May 14, 2020 • edited Loading

Overview

Checklist

Notes

Testing

jisantuc left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notthatbreezy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pomadchin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jisantuc commented May 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notthatbreezy left a comment

Choose a reason for hiding this comment

jisantuc commented May 14, 2020 •

edited

Loading

jisantuc left a comment •

edited

Loading

pomadchin left a comment •

edited

Loading