-
Notifications
You must be signed in to change notification settings - Fork 10
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
С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
next
will 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
Outdated
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