-
Couldn't load subscription status.
- Fork 11
Сlient module #140
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
Conversation
modules/client/src/main/scala/com/azavea/stac4s/api/client/Http4sStacClient.scala
Outdated
Show resolved
Hide resolved
38d1dbb to
87f7b51
Compare
modules/client/src/main/scala/com/azavea/stac4s/api/client/PaginationToken.scala
Outdated
Show resolved
Hide resolved
modules/client/src/main/scala/com/azavea/stac4s/api/client/Http4sStacClient.scala
Outdated
Show resolved
Hide resolved
b3ee32c to
bb2e57e
Compare
bb2e57e to
7630412
Compare
b193ddb to
31a0bf0
Compare
modules/client/jvm/src/test/scala/com/azavea/stac4s/api/client/StacClientSpec.scala
Outdated
Show resolved
Hide resolved
modules/client/jvm/src/test/scala/com/azavea/stac4s/api/client/StacClientSpec.scala
Outdated
Show resolved
Hide resolved
8cb9db6 to
8215679
Compare
modules/client/shared/src/main/scala/com/azavea/stac4s/api/client/SttpStacClient.scala
Outdated
Show resolved
Hide resolved
8215679 to
932452a
Compare
932452a to
c732bae
Compare
337811c to
eb6b449
Compare
eb6b449 to
739912d
Compare
eee15ea to
7a275a2
Compare
8cf725a to
f483fe2
Compare
a11281a to
fdee543
Compare
modules/client/js/src/test/scala/com/azavea/stac4s/api/client/StacClientSpec.scala
Outdated
Show resolved
Hide resolved
modules/core/js/src/main/scala/com/azavea/stac4s/geometry/Geometry.scala
Outdated
Show resolved
Hide resolved
modules/client/jvm/src/main/scala/com/azavea/stac4s/api/client/SttpStacClient.scala
Outdated
Show resolved
Hide resolved
542f684 to
a0e5b64
Compare
a0e5b64 to
7f6abce
Compare
There was a problem hiding this 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
modules/client/js/src/main/scala/com/azavea/stac4s/api/client/SearchFilters.scala
Show resolved
Hide resolved
| baseUri: Uri | ||
| ) extends StacClient[F] { | ||
|
|
||
| type Filter |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
modules/client/shared/src/main/scala/com/azavea/stac4s/api/client/StacClient.scala
Outdated
Show resolved
Hide resolved
|
|
||
| import java.time.Instant | ||
|
|
||
| final case class PaginationToken(timestampAtLeast: Instant, serialIdGreaterThan: PosInt) |
There was a problem hiding this comment.
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
nextwill be a jsonPaginationToken, 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
So I think this shouldn't work
There was a problem hiding this comment.
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 (?).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modules/client/jvm/src/test/scala/com/azavea/stac4s/api/client/StacClientSpec.scala
Show resolved
Hide resolved
modules/client/shared/src/main/scala/com/azavea/stac4s/api/client/StacClient.scala
Outdated
Show resolved
Hide resolved
modules/client/jvm/src/test/scala/com/azavea/stac4s/api/client/StacClientSpec.scala
Outdated
Show resolved
Hide resolved
ad67f2a to
6d2ab01
Compare
modules/client/shared/src/main/scala/com/azavea/stac4s/api/client/BaseSttpStacClient.scala
Outdated
Show resolved
Hide resolved
|
confirmed, build tooling is not smart and doesn't know about scalajs decorators in I think users who want the JS version will need to create a client and export the methods they want from it. |
140b105 to
ea9b35c
Compare
ea9b35c to
c708009
Compare
There was a problem hiding this 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
|
@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. |
Overview
This PR adds a stac4s api client module.
Checklist
Notes
It is a WIP pr, any suggestnions are appreciated.
Closes geotrellis/geotrellis-server#288