-
Notifications
You must be signed in to change notification settings - Fork 34
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
Sample http4s REST client/server with client macro derivation #552
Conversation
This is basically a conversion to/from the internal fs2.Stream.
Note: Update of Monix/FS2 conversions is in progress with some outdated implementations removed, which fails several tests.
Codecov Report
@@ Coverage Diff @@
## master #552 +/- ##
==========================================
+ Coverage 84.61% 84.71% +0.09%
==========================================
Files 69 71 +2
Lines 1066 1099 +33
Branches 15 16 +1
==========================================
+ Hits 902 931 +29
- Misses 164 168 +4
Continue to review full report at Codecov.
|
This PR is ready for review. Below a brief summary about how it works: HTTP ProtocolGiven this example protocol below: import monix.reactive.Observable
@service(Avro) trait Greeter[F[_]] {
@http def sayHellos(requests: Observable[HelloRequest]): F[HelloResponse]
@http def sayHelloAll(request: HelloRequest): Observable[HelloResponse]
@http def sayHellosAll(requests: Observable[HelloRequest]): Observable[HelloResponse]
} With the annotation Run the HTTP ServerAssuming that implicits ExecutionContext and ContextShift are already in the scope: implicit val ec = monix.execution.Scheduler.Implicits.global
implicit val cs: ContextShift[IO] = IO.contextShift(ec) We need to provide an implicit instance of the interpreter of the service, that is, the implicit val greeterHndlerIO = new GreeterHandler[IO] Then we need a server builder by: val server = HttpServer.bind(80, "localhost", Greeter.route[IO]) And finally, we can run the server: server.resource.use(_ => IO.never).start.unsafeRunSync() Run the HTTP ClientFirstly, we should set the Uri to reach the server: val serviceUri: Uri = Uri.unsafeFromString(s"http://localhost:80") Then, we can instantiate the derived client: val client = Greeter.httpClient[IO](serviceUri) And now we can perform rest calls. For instance, by: val requests = Observable(HelloRequest("hey"), HelloRequest("there"))
val response = BlazeClientBuilder[IO](ec).resource.use(client.sayHellos(requests)(_)) where response is |
@rafaparadela this looks pretty cool. I didn't take a look at the code yet, but I have some questions/doubts:
|
modules/http/src/test/scala/higherkindness/mu/rpc/http/GreeterRestTests.scala
Outdated
Show resolved
Hide resolved
...c/test/scala/higherkindness/mu/rpc/prometheus/client/BaseMonitorClientInterceptorTests.scala
Show resolved
Hide resolved
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 looks great! thank you @rafaparadela !
I've added a couple of minor comments
Addressed all the comments by @juanpedromoreno and @pepegar |
modules/internal/src/main/scala/higherkindness/mu/rpc/internal/serviceImpl.scala
Outdated
Show resolved
Hide resolved
modules/internal/src/main/scala/higherkindness/mu/rpc/internal/serviceImpl.scala
Outdated
Show resolved
Hide resolved
modules/internal/src/main/scala/higherkindness/mu/rpc/internal/serviceImpl.scala
Outdated
Show resolved
Hide resolved
|
||
val scheduler: List[Tree] = operations | ||
.find(_.operation.isMonixObservable) | ||
.map(_ => q"import _root_.monix.execution.Scheduler.Implicits.global") |
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.
The execution context needs to be asked as implicit parameter. We cannot add the global here.
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.
Moved at fb91483
modules/internal/src/main/scala/higherkindness/mu/rpc/internal/serviceImpl.scala
Outdated
Show resolved
Hide resolved
…otocols' into feature/182-http-support-from-protocols # Conflicts: # project/ProjectPlugin.scala
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 great, thanks so much @L-Lavigne and @rafaparadela !
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.
Left some comments. Thanks @rafaparadela @L-Lavigne
l or r | ||
} | ||
|
||
implicit private val throwableDecoder: Decoder[Throwable] = |
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.
Please, help me to understand this. This is only used for streams and observable, right? The server generates an Either that is parsed in the client. Am I right?
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.
The implicits.scala
was enterally created by @L-Lavigne, so I'll let him answer all your questions.
.getConstructor(classOf[String]) | ||
.newInstance(msg) | ||
.asInstanceOf[Throwable] | ||
} |
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 we're losing the stacktrace here, I'd use our custom case class
:
final case class UnexpectedError(status: Status, msg: Option[String] = None)
extends RuntimeException(status + msg.fold("")(": " + _))
with NoStackTrace
//...
implicit val unexpectedErrorDecoder: Decoder[UnexpectedError] = deriveDecoder
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.
IMO we need to avoid custom logic in the encoders/decoders
final def apply(a: Either[A, B]): Json = a match { | ||
case Left(a) => a.asJson | ||
case Right(b) => b.asJson | ||
} |
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.
new Encoder[Either[A, B]] {
final def apply(a: Either[A, B]): Json = a.fold(_.asJson, _.asJson)
}
|
||
implicit val throwableEncoder: Encoder[Throwable] = new Encoder[Throwable] { | ||
def apply(ex: Throwable): Json = (ex.getClass.getName, ex.getMessage).asJson | ||
} |
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.
Based on the above:
implicit val unexpectedErrorEncoder: Encoder[UnexpectedError] = deriveEncoder
def apply(ex: Throwable): Json = (ex.getClass.getName, ex.getMessage).asJson | ||
} | ||
|
||
def asJsonEither(implicit encoder: Encoder[A]): Stream[F, Json] = stream.attempt.map(_.asJson) |
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.
def asJsonEither(implicit encoder: Encoder[A]): Stream[F, Json] =
stream.attempt.bimap(e => UnexpectedError(e.getClass.getName, Option(e.getMessage)), _.asJson)
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 sorry @fedefernandez, I don't really understand how you expect to instantiate the UnexpectedError
without having the Status
.
Maybe you're proposing
case class UnexpectedError(status: String, msg: Option[String] = None)
instead of
case class UnexpectedError(status: Status, msg: Option[String] = None)
@fedefernandez Please, have a look at implicits.scala again and let me know if now makes more sense to you. Thanks |
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.
LGTM!
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.
Good work! A few comments.
@@ -254,15 +250,5 @@ class GreeterRestTests | |||
.getOrElse(sys.error("Stuck!")) shouldBe Nil | |||
} | |||
|
|||
"serve ScalaCheck-generated POST requests with bidirectional Observable streaming" in { |
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.
Just curious, were there any issues with the ScalaCheck tests?
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.
(resolved offline)
} | ||
|
||
final case class UnexpectedError(className: String, msg: Option[String]) |
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.
The name is a bit generic - most errors are unexpected. The way we're using those, they're closer to "RequestException"s, or 4XX-status ResponseException
s. Can we combine those error types or rename this one?
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.
(resolved offline)
As discussed offline, I approve PR (can't set it to that status as I'm the owner), and I'll make a separate PR to adjust the naming of the exceptions. |
Adds sample http4s REST client/server implementations, which we will eventually derive from the same
@service
-annotated classes as we do for RPC. The roadmap for this is described in #182.Also implements HTTP client-side derivation using
@http
annotations, based on #368. Those are tested with unary clients, but support for streaming exists in our http4s samples and can be auto-derived as well. The server-side remains to be done.Note that this client derivation is implemented inside the
serviceImpl
class that processes the@service
annotation, due to difficulties in separating the macro processing between this class and another one for thehttp
module. It still needs to be moved there.Warning: do not merge in current state as this does not pass all unit tests, because auto-derived HTTP servers are not yet implemented.