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

Client multi responses #152

Merged
merged 9 commits into from Jan 14, 2019

Conversation

Projects
None yet
2 participants
@blast-hardcheese
Copy link
Collaborator

blast-hardcheese commented Jan 6, 2019

Depends on #150
Resolves #151

This extends the akka-http clients to behave similarly to the http4s clients, generating a unique sealed abstract class for each operation and status code, permitting mapping and deserializing all known status codes.

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

blast-hardcheese commented Jan 6, 2019

Two major issues:
first, jsonEntityUnmarshaller has been marked deprecated, but it is still used in the multipart form-data unmarshaller.
second, I'd like to improve the user experience of consuming the response types.

Currently, a user needs to import getFooBarResponse as a sibling to the Client, then match on the result of calling getFooBar, matching on the different possible status codes.

What would be preferable would be either a .fold[A], representing each status as a partial function from each decoded type to A, or some other way to make it easier for consumers to work with potentially unwieldy names.

Additionally, to ease the transition to the new behavior, .best: Either[getFooBarResponse, FooBar] could return the "best" status code, based on the current heuristic.

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

blast-hardcheese commented Jan 6, 2019

After playing with the fold implementation for a bit, I still feel that it's a compelling direction:

  • Discoverability, as the type only offers one top-level method
  • Implementation details hidden, reducing burden on the consumer

The tradeoffs are:

  • Named parameters are almost a necessity for clarity
  • As all parameters are peers, explicitly specifying the result type is likely necessary to prevent divergence
  import akka.http.scaladsl.model.HttpResponse
  def handleError: Either[Throwable, HttpResponse] => Future[String] = _.fold(ex => Future.successful(ex.getMessage()), { resp =>
    import scala.concurrent.duration._
    resp.entity.toStrict(5.seconds).map { entity =>
      s"${resp.status}: ${entity}"
    }
  })
  client.getPet("foo")
    .leftSemiflatMap(handleError)
    .flatMap(
      _.fold[EitherT[Future, String, IndexedSeq[_root_.client.definitions.Pet]]](
        handleOK = pet => EitherT.pure(IndexedSeq(pet)),     // Pet => A
        handleCreated = EitherT.pure(_),                     // IndexedSeq[Pet] => A
        handleAccepted = EitherT.pure(IndexedSeq.empty),     // => A
        handleNonAuthoritativeInformation = EitherT.pure(_)  // IndexedSeq[Pet] => A
      )
    ).value.onComplete(println)

@blast-hardcheese blast-hardcheese force-pushed the blast-hardcheese:akka-client-multi-responses branch from 6127366 to 160b5f3 Jan 7, 2019

@kelnos

This comment has been minimized.

Copy link
Member

kelnos commented Jan 8, 2019

I like the fold implementation, and I think sorta "forcing" use of named parameters is a strength, not a tradeoff. However, what if an endpoint only has a single success status? What would the return type (after handling errors) look like?

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

blast-hardcheese commented Jan 8, 2019

@kelnos https://github.com/twilio/guardrail/pull/152/files#diff-c99036f0b5ed1567f3f308983e33aa9dR196

This is a single response type that has no response body:

          def fold[A](handleOk: => A): A = this match {
            case GetBarResponse.Ok => handleOk
          }

If the response body had been specified, it'd look like this:

          def fold[A](handleOk: Foo => A): A = this match {
            case GetBarResponse.Ok(x) => handleOk(x)
          }
@kelnos

This comment has been minimized.

Copy link
Member

kelnos commented Jan 8, 2019

Having fold with a single 'branch' feels odd, but I guess is fine.

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

blast-hardcheese commented Jan 8, 2019

Yeah -- I was thinking about special-casing it, but altering the spec to have other response types would create unpleasant compiler errors, plus having to remember special cases makes additional burdens for consumers

@blast-hardcheese blast-hardcheese force-pushed the blast-hardcheese:akka-client-multi-responses branch from 160b5f3 to 0f1f3d3 Jan 8, 2019

@blast-hardcheese blast-hardcheese force-pushed the blast-hardcheese:akka-client-multi-responses branch from 0f1f3d3 to 547d592 Jan 12, 2019

@blast-hardcheese blast-hardcheese force-pushed the blast-hardcheese:akka-client-multi-responses branch from 547d592 to 4869fe1 Jan 12, 2019

@blast-hardcheese blast-hardcheese changed the title Akka client multi responses Client multi responses Jan 13, 2019

@blast-hardcheese blast-hardcheese merged commit 6c36341 into twilio:master Jan 14, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@blast-hardcheese blast-hardcheese deleted the blast-hardcheese:akka-client-multi-responses branch Jan 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment