-
Notifications
You must be signed in to change notification settings - Fork 422
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
Setting options for AkkaHttpServerInterpreter as explicit parameter #1321
Conversation
README.md
Outdated
@@ -54,7 +54,7 @@ val booksListing: Endpoint[(BooksFromYear, Limit, AuthToken), String, List[Book] | |||
import sttp.tapir.docs.openapi.OpenAPIDocsInterpreter | |||
import sttp.tapir.openapi.circe.yaml._ | |||
|
|||
val docs = OpenAPIDocsInterpreter.toOpenAPI(booksListing, "My Bookshop", "1.0") | |||
val docs = OpenAPIDocsInterpreter()().toOpenAPI(booksListing, "My Bookshop", "1.0") |
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.
hm this doesn't look nice - why do we need the double ()
?
@@ -1,3 +1,9 @@ | |||
package sttp.tapir.client | |||
|
|||
package object sttp extends TapirSttpClient | |||
package object sttp { | |||
def apply(clientOptions: SttpClientOptions = SttpClientOptions.default): TapirSttpClient = { |
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.
hm I think this might be confusing. Maybe it's time to remove the deprecated TapirSttpClient
simply
@@ -8,6 +8,8 @@ import scala.concurrent.Future | |||
|
|||
trait SttpClientInterpreterExtensions { | |||
|
|||
def sttpClientOptions: SttpClientOptions = SttpClientOptions.default |
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.
since this is always extended by SttpClientInterpreter
, we might use self-types here and avoid the additional definiton of sttpClientOptions
.
Sth like: trait SttpClientInterpreterExtensions { this: SttpClientInterpreter =>
eClassTag: ClassTag[E], | ||
timer: Timer[F] | ||
): HttpRoutes[F] = toRoutes(e.serverLogicRecoverErrors(logic)) | ||
trait Http4sServerInterpreter[F[_], G[_]] { |
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 think the toRoutes
variant is the one that is predominantly used. So maybe we could rename this one to Http4sServerToHttpInterpreter
, and the routes one would keep the simpler Http4sServerInterpreter
name?
|
||
package object vertx { | ||
|
||
object VertxZioServerInterpreter extends VertxZioServerInterpreter | ||
object VertxZioServerInterpreter { |
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 think the companion could be defined in the same file as the class?
One more thing (apart from merge conflicts :) ) - the various |
# Conflicts: # client/http4s-client/src/test/scala/sttp/tapir/client/http4s/Http4sClientTests.scala # docs/openapi-docs/src/test/scala/sttp/tapir/docs/openapi/VerifyYamlValidatorTest.scala # server/finatra-server/src/main/scala/sttp/tapir/server/finatra/FinatraServerInterpreter.scala # server/http4s-server/src/main/scala/sttp/tapir/server/http4s/Http4sServerInterpreter.scala # server/play-server/src/main/scala/sttp/tapir/server/play/PlayServerInterpreter.scala # server/vertx/src/main/scala/sttp/tapir/server/vertx/interpreters/VertxCatsServerInterpreter.scala # server/vertx/src/main/scala/sttp/tapir/server/vertx/interpreters/VertxFutureServerInterpreter.scala # server/vertx/src/test/scala/sttp/tapir/server/vertx/CatsVertxServerTest.scala # serverless/aws/lambda/src/main/scala/sttp/tapir/serverless/aws/lambda/AwsToResponseBody.scala
@@ -43,6 +42,7 @@ object MultipleEndpointsDocumentationHttp4sServer extends App { | |||
implicit val ec: ExecutionContext = scala.concurrent.ExecutionContext.Implicits.global | |||
implicit val contextShift: ContextShift[IO] = IO.contextShift(ec) | |||
implicit val timer: Timer[IO] = IO.timer(ec) | |||
implicit val concurrent: Concurrent[IO] = IO.ioConcurrentEffect |
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.
why do we need a concurrent instance now, as we didn't need one before?
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.
It is used implicitly by Http4sServerToHttpInterpreter's apply method
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.
But ... it wasn't needed before, so it shouldn't be needed now?
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.
You are right I am missing type parameter for Http4sServerInterpreter
doc/server/http4s.md
Outdated
@@ -60,10 +63,11 @@ implicit val cs: ContextShift[IO] = | |||
IO.contextShift(scala.concurrent.ExecutionContext.global) | |||
implicit val t: Timer[IO] = | |||
IO.timer(scala.concurrent.ExecutionContext.global) | |||
implicit val concurrent: Concurrent[IO] = IO.ioConcurrentEffect |
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.
so do we need this? :)
doc/server/vertx.md
Outdated
@@ -14,7 +14,7 @@ to use this interpreter with `Future`. | |||
|
|||
Then import the object: | |||
```scala mdoc:compile-only | |||
import sttp.tapir.server.vertx.VertxFutureServerInterpreter._ | |||
import sttp.tapir.server.vertx.interpreters.VertxFutureServerInterpreter._ |
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 think I'd keep the interpreters in the "root" package, that is sttp.tapir.server.vertx
. Easier for users :)
generated-doc/out/server/http4s.md
Outdated
val countCharactersRoutes: HttpRoutes[IO] = | ||
Http4sServerInterpreter.toRoutes(countCharactersEndpoint)(countCharacters _) | ||
val countCharactersRoutes: HttpRoutes[IO] = | ||
Http4sServerToHttpInterpreter.toRoutes(countCharactersEndpoint)(countCharacters _) |
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.
shouldn't this be the "normal" interpreter?
@@ -47,7 +47,7 @@ class Http4sServerTest[R >: Fs2Streams[IO] with WebSockets] extends TestSuite wi | |||
def additionalTests(): List[Test] = List( | |||
Test("should work with a router and routes in a context") { | |||
val e = endpoint.get.in("test" / "router").out(stringBody).serverLogic(_ => IO.pure("ok".asRight[Unit])) | |||
val routes = Http4sServerInterpreter.toRoutes(e) | |||
val routes = Http4sServerInterpreter()(interpreter.concurrent, cs, timer).toRoutes(e) |
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.
do we need to provide these implicits explicitly now?
): Router => Route = | ||
VertxCatsServerInterpreter.routeRecoverErrors(e)(fn) | ||
): Router => Route = { | ||
val fs: Sync[IO] = Sync[IO] |
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 should be inferred by the compiler?
val apiGateway: AwsTerraformApiGateway = AwsTerraformInterpreter.toTerraformConfig(helloEndpoint) | ||
val apiGateway: AwsTerraformApiGateway = AwsTerraformInterpreter(terraformOptions).toTerraformConfig(helloEndpoint) | ||
|
||
implicit val encoder: Encoder[AwsTerraformApiGateway] = encoderAwsTerraformApiGateway(terraformOptions) |
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.
we need an encoder now? :)
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.
yeah because options are no longer implicit and encoder requires them as implicit parameter.
I can change encoder to explicit def with explicit parameters what do you think ?
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.
hm and where do we use the encoder? Maybe at the point of use, we can import the encoder as an implicit value
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.
From what I see encoder is required by asJson method from AwsTerraformApiGatewa class
- Removed not needed implicit Concurrent[IO] - Moved Vertex based interpreters to root package - Replace usage of toHttp interpreter with normal interpreter
- Removed additional test which do not exists on master - Fixed calls to Http4sClientInterpreter - Removed unused imports
|
||
def toHttp( | ||
serverEndpoints: List[ServerEndpoint[_, _, _, Fs2Streams[F] with WebSockets, G]] | ||
)(fToG: F ~> G)(gToF: G ~> F): Http[OptionT[G, *], F] = { |
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.
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.
in Http4sBodyListener
, we need to know when the body has finished streaming - hence we need a way to translate the effect of the callback (in which the response is returned) to the effect in which the body is returned.
If you'd have a better idea how to model this, I'm open to suggestions :)
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.
There are a lot of changes was made. I'll try to see what you mean ASAP :)
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've seen that the ServerInterpriter
was generalized and as as result ServerInterpriter
should support transformation F ~> G
but unfortunately it didn't happen. It was an mistake to propagate G
in the Http4sBodyListener
and others because F
is an effect-full computation not G
. G
is just a monad needed only in the Http[OptionT[G, *], F]
. It would be better to remove G
and F ~> G
at all because it only confuse :)
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.
No description provided.