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

Sample http4s REST client/server with client macro derivation #552

Merged
merged 39 commits into from Mar 8, 2019

Conversation

Projects
None yet
5 participants
@L-Lavigne
Copy link
Contributor

commented Jan 29, 2019

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 the http 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.

@codecov

This comment has been minimized.

Copy link

commented Feb 18, 2019

Codecov Report

Merging #552 into master will increase coverage by 0.09%.
The diff coverage is 81.81%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
...c/main/scala/higherkindness/mu/http/protocol.scala 100% <100%> (ø)
.../main/scala/higherkindness/mu/http/implicits.scala 81.25% <81.25%> (ø)
...herkindness/mu/rpc/channel/cache/ClientCache.scala 85.71% <0%> (+3.57%) ⬆️
...c/main/scala/higherkindness/mu/MonixAdapters.scala 92.3% <0%> (+3.84%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc51c6f...bf6e853. Read the comment docs.

@L-Lavigne

This comment has been minimized.

@rafaparadela

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

This PR is ready for review. Below a brief summary about how it works:

HTTP Protocol

Given 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 @http we would be letting the macro know that an HTTP Server should be built, which must be able to serve responses to call from the paths: sayHellos, sayHelloAll, and sayHellosAll.

Run the HTTP Server

Assuming 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 Greeter[F] with the actual implementation of every operation.

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 Client

Firstly, 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 IO[HelloResponse] as expected.

Show resolved Hide resolved build.sbt Outdated
@pepegar
Copy link
Member

left a comment

This looks great! thank you @rafaparadela !

I've added a couple of minor comments

@rafaparadela

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

Addressed all the comments by @juanpedromoreno and @pepegar

val scheduler: List[Tree] = operations
.find(_.operation.isMonixObservable)
.map(_ => q"import _root_.monix.execution.Scheduler.Implicits.global")

This comment has been minimized.

Copy link
@juanpedromoreno

juanpedromoreno Mar 6, 2019

Member

The execution context needs to be asked as implicit parameter. We cannot add the global here.

This comment has been minimized.

Copy link
@rafaparadela

rafaparadela Mar 7, 2019

Contributor

Moved at fb91483

Show resolved Hide resolved project/ProjectPlugin.scala Outdated
Show resolved Hide resolved project/ProjectPlugin.scala Outdated

juanpedromoreno and others added some commits Mar 7, 2019

Merge remote-tracking branch 'origin/feature/182-http-support-from-pr…
…otocols' into feature/182-http-support-from-protocols

# Conflicts:
#	project/ProjectPlugin.scala
@juanpedromoreno
Copy link
Member

left a comment

This is great, thanks so much @L-Lavigne and @rafaparadela !

@fedefernandez
Copy link
Collaborator

left a comment

Left some comments. Thanks @rafaparadela @L-Lavigne

l or r
}

implicit private val throwableDecoder: Decoder[Throwable] =

This comment has been minimized.

Copy link
@fedefernandez

fedefernandez Mar 8, 2019

Collaborator

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?

This comment has been minimized.

Copy link
@rafaparadela

rafaparadela Mar 8, 2019

Contributor

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]
}

This comment has been minimized.

Copy link
@fedefernandez

fedefernandez Mar 8, 2019

Collaborator

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

This comment has been minimized.

Copy link
@fedefernandez

fedefernandez Mar 8, 2019

Collaborator

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
}

This comment has been minimized.

Copy link
@fedefernandez

fedefernandez Mar 8, 2019

Collaborator
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
}

This comment has been minimized.

Copy link
@fedefernandez

fedefernandez Mar 8, 2019

Collaborator

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)

This comment has been minimized.

Copy link
@fedefernandez

fedefernandez Mar 8, 2019

Collaborator
def asJsonEither(implicit encoder: Encoder[A]): Stream[F, Json] = 
  stream.attempt.bimap(e => UnexpectedError(e.getClass.getName, Option(e.getMessage)), _.asJson)

This comment has been minimized.

Copy link
@rafaparadela

rafaparadela Mar 8, 2019

Contributor

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)
@rafaparadela

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

@fedefernandez Please, have a look at implicits.scala again and let me know if now makes more sense to you. Thanks

@fedefernandez
Copy link
Collaborator

left a comment

LGTM!

@L-Lavigne
Copy link
Contributor Author

left a comment

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 {

This comment has been minimized.

Copy link
@L-Lavigne

L-Lavigne Mar 8, 2019

Author Contributor

Just curious, were there any issues with the ScalaCheck tests?

This comment has been minimized.

Copy link
@L-Lavigne

L-Lavigne Mar 8, 2019

Author Contributor

(resolved offline)

}

final case class UnexpectedError(className: String, msg: Option[String])

This comment has been minimized.

Copy link
@L-Lavigne

L-Lavigne Mar 8, 2019

Author Contributor

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 ResponseExceptions. Can we combine those error types or rename this one?

This comment has been minimized.

Copy link
@L-Lavigne

L-Lavigne Mar 8, 2019

Author Contributor

(resolved offline)

@L-Lavigne

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

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.
(@rafaparadela please let me know if you don't have merge permissions and then I'll do it)

@rafaparadela rafaparadela merged commit cdf7e99 into master Mar 8, 2019

3 of 4 checks passed

codecov/patch 81.81% of diff hit (target 84.61%)
Details
codecov/project 84.71% (+0.09%) compared to bc51c6f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@L-Lavigne L-Lavigne referenced this pull request Mar 18, 2019

Closed

WIP: derive http client from service #368

3 of 4 tasks complete

@diesalbla diesalbla deleted the feature/182-http-support-from-protocols branch May 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.