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

Сlient module #140

Merged
merged 27 commits into from
Jan 4, 2021
Merged

Сlient module #140

merged 27 commits into from
Jan 4, 2021

Conversation

pomadchin
Copy link
Collaborator

@pomadchin pomadchin commented Aug 20, 2020

Overview

This PR adds a stac4s api client module.

Checklist

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

Notes

It is a WIP pr, any suggestnions are appreciated.

Closes geotrellis/geotrellis-server#288

@pomadchin pomadchin force-pushed the feature/client branch 2 times, most recently from 38d1dbb to 87f7b51 Compare August 20, 2020 22:25
@pomadchin pomadchin force-pushed the feature/client branch 2 times, most recently from b3ee32c to bb2e57e Compare December 23, 2020 17:03
build.sbt Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
@pomadchin pomadchin force-pushed the feature/client branch 2 times, most recently from 542f684 to a0e5b64 Compare December 24, 2020 21:42
Copy link
Contributor

@jisantuc jisantuc left a comment

Choose a reason for hiding this comment

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

I'm very excited to have an API client and I might borrow this in my R&D instead of hand rolling a the few endpoints I need methods for

baseUri: Uri
) extends StacClient[F] {

type Filter
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little confused -- what's the purpose of a dependent Filter type?

Copy link
Collaborator Author

@pomadchin pomadchin Dec 24, 2020

Choose a reason for hiding this comment

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

@jisantuc the trick here is to have it in the shared package. To put STACClient into the shared package we only need to have SearchFilters compiling for both JS and JVM backends.

SearchFilters should countain jts.Geometry type that is why we need to maintain a spearate implementaion for the JS backend.

I wanted to have an abstraction of the Filter type (since it is very backend and implementation dependent). Having this Filter type hidden helps to simplify the STACClient signature.

StacClient[F] signatures makes sense, but StacClient[F, SearchFilter] looks extremely confusing.

The alternative to that can be found here 0647db7 (maintaining two 95% equal STACClient implementations but with a different SearchFilters type)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Btw we can have a quick call if you want me to walk you through this decision


import java.time.Instant

final case class PaginationToken(timestampAtLeast: Instant, serialIdGreaterThan: PosInt)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is correct for Franklin's implementation of pagination, but only for Franklin's -- the particular pagination strategy is left open to implementers. So there are a few things that are a bit weird as a result --

  • client side search filters can't assume that the value in next will be a json PaginationToken, since it will just be an opaque string by the time it reaches the client. This means this is probably wrong for any other STAC API implementation.
  • the thing Franklin actually sends back in this field is a base64 encoding of the pagination token, so decoding it probably won't work client side (not sure if you had a chance to test that)
  • the thing Franklin expects is for the client to have a hold of the opaque token, so it does a base64 decode to get the json out

https://github.com/azavea/franklin/blob/f5be8ddf48661c5bc43cbd22cb7277e961641803/application/src/main/scala/com/azavea/franklin/api/schemas/package.scala#L84-L85

So I think this shouldn't work

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd say this is fine; since it is a default implementation (i.e. if smth doesn't work user can always define theirs own STACClient with a redefined search filters: see https://github.com/azavea/stac4s/blob/7f6abceeebea59dfc9a605891c73fc121116610b/modules/client/jvm/src/main/scala/com/azavea/stac4s/api/client/SttpStacClient.scala#L9-L13 - they only need to define their own version of SearchFilters), if smth won't work in the future with other STAC Services I think we should handle it later (?).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Opened an issue for that #198

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not really talking about streaming here though -- I don't think this pagination encoding will work, and the tests included here don't exercise pagination. Since the pagination implementation here isn't exercised and I think won't work, I think it should just be omitted until someone is working on #198

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops forgot to push not tested codecs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jisantuc added it into the issue description #198

@jisantuc jisantuc mentioned this pull request Dec 29, 2020
@jisantuc
Copy link
Contributor

confirmed, build tooling is not smart and doesn't know about scalajs decorators in shared/ 😢

I think users who want the JS version will need to create a client and export the methods they want from it.

Copy link
Contributor

@jisantuc jisantuc left a comment

Choose a reason for hiding this comment

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

🎉 Is #198 prioritized / anyone's responsibility? It would be good to tackle that while we still have context pretty well loaded

@pomadchin
Copy link
Collaborator Author

pomadchin commented Jan 4, 2021

@jisantuc we don't have it on the radar right now, but we'll have some time in terms of the other project to handle that. I'll assign it on myself in order not to forget about it.

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.

Move STAC API Client out of the geotrellis-server project
3 participants