HttpPayloadError in server logic gets turned into a 400

[kubukoz]

Similar to #1256, but not a regression from 0.17: it appears to have been here all along.

Consider this case: a smithy4s server wrapping a smithy4s client (doesn't have to be the same Service, but it can be, e.g. in a proxy scenario).

Now here's what may happen:

1. The upstream service fails with a non-parseable response. This is normal for uncaught exceptions, or services that aren't using smithy4s.
2. The smithy4s client will rightfully fail its effect with a HttpPayloadError.
3. The smithy4s server, instead of re-throwing said error, will return a 400 with a JSON body (the serialized form of HttpPayloadError).

This appears incorrect: the error is clearly not a problem of the ultimate caller of the service: ideally, the smithy4s server here would return a 500 (the serialization of which is up for debate). At the moment, it makes it seem like the client has made a mistake in its request. This may be very difficult to debug.

Reproduction: this prints a 400 response with a JSON body. I would expect it to fail on IO level.

//> using dep "com.disneystreaming.smithy4s::smithy4s-http4s:0.18.12"
//> using dep "com.disneystreaming.smithy4s::smithy4s-tests:0.18.12"
//> using option "-Wunused:imports"
import cats.effect.IO
import cats.effect.IOApp
import cats.implicits._
import org.http4s.Request
import org.http4s.implicits._
import smithy4s.example.PizzaAdminService
import smithy4s.http4s.SimpleRestJsonBuilder
import org.http4s.client.Client
import cats.effect.kernel.Resource
import org.http4s.Response
import org.http4s.Status

object Main extends IOApp.Simple {

  val route =
    SimpleRestJsonBuilder
      .routes(
        SimpleRestJsonBuilder(PizzaAdminService)
          .client[IO](
            Client(_ =>
              Resource.pure(
                Response(
                  status = Status.InternalServerError,
                  body = fs2.Stream.emits("goodbye world".getBytes),
                )
              )
            )
          )
          .make
          .toTry
          .get
      )
      .make
      .toTry
      .get

  override def run: IO[Unit] = route
    .run(Request(uri = uri"/health"))
    .value
    .debug()
    .flatMap(_.traverse_(resp => resp.bodyText.compile.string.debug()))

}

I believe this is due to this line:

smithy4s/modules/core/src/smithy4s/http/HttpUnaryServerCodecs.scala

Line 201 in 3233108

case e: HttpContractError => httpContractErrorEncoder(e)

and this function is later called here:

smithy4s/modules/core/src/smithy4s/server/UnaryServerEndpoint.scala

Line 37 in 3233108

codecs.throwableEncoder(e)

HPEs are special-cased in the first snippet, as far as I understand it's because they're normally raised in the request decoding logic. However, this is clearly not a request decoding problem, so we should probably circumvent that code path.

I'm not sure if this is a server problem or a client problem:

• on one hand, clients could wrap HPE instead of raising it directly, or use another type entirely (ClientHPE?)
• OTOH, this could be considered a server problem: the server interpreter could be more aware of where the HPE is coming from, and if it doesn't come from request decoding, treat it as any other throwable.

[Baccata]

Not characterising it as a bug, but it is an unfortunate consequence of the combinations of two features that are working well individually.

I think we're gonna have to target 0.19 for it : whatever road we take to solve this is gonna bring a behavioural change that will impact users.

I think I can agree that distinguishing ServerRequestDecodingError from ClientResponseDecodingError is gonna be required, and wrapping both cases in an bespoke and indicative throwable is likely to be the best course of action (putting aside the unavoidable behavioural change it'll produce).

[kubukoz]

eh, fair enough. I can't think of a case where you'd want this behavior by default, so to me that's still a bug :P

0.19 is fine by me - there's a workaround that users can apply in the meantime: adapt the client throwables by catching HPE and wrapping it in something else / replacing it with something else entirely.

[kubukoz]

cc @ghostbuster91

[GaryAghedo]

Hey @kubukoz @Baccata

I've picked up this issue and I'm working on a solution.

[kubukoz]

hmm after some thinking, and seeing what we built as a "workaround" for this internally (or rather extra feature, because this isn't considered a bug) I'm starting to think this is... not something that needs fixing.

Hear me out: the immediate problem was that the client is returning some sort of PayloadError for an unparseable response, even though it's not a known error response, and not even JSON:

Response(
  status = Status.InternalServerError,
  body = fs2.Stream.emits("goodbye world".getBytes)
)

Some(Response(status=400, httpVersion=HTTP/1.1, headers=Headers(Content-Length: 518, Content-Type: application/json)))
{"payload":{"path":".","expected":"object","message":"Expected JSON object, offset: 0x00000000, buf:\n+----------+-------------------------------------------------+------------------+\n|          |  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f | 0123456789abcdef |\n+----------+-------------------------------------------------+------------------+\n| 00000000 | 67 6f 6f 64 62 79 65 20 77 6f 72 6c 64          | goodbye world    |\n+----------+-------------------------------------------------+------------------+"}}

❓ But why is it doing that?

❗ Well, it appears to be the "best effort" logic of decoding errors based on a discriminator or lack of it.

Proof:

Health has a documented 500 response, and if you return something else (e.g. a 502 Bad Gateway), that gets propagated as smithy4s.http.UnknownErrorResponse: status 502:

//> using dep "com.disneystreaming.smithy4s::smithy4s-http4s:0.18.23"
//> using dep "com.disneystreaming.smithy4s::smithy4s-tests:0.18.23"
//> using option "-Wunused:imports"
import cats.effect.IO
import cats.effect.IOApp
import cats.implicits._
import org.http4s.Request
import org.http4s.implicits._
import smithy4s.example.PizzaAdminService
import smithy4s.http4s.SimpleRestJsonBuilder
import org.http4s.client.Client
import cats.effect.kernel.Resource
import org.http4s.Response
import org.http4s.Status

object Main extends IOApp.Simple {

  val route =
    SimpleRestJsonBuilder
      .routes(
        SimpleRestJsonBuilder(PizzaAdminService)
          .client[IO](
            Client(_ =>
              Resource.pure(
                Response(
                  status = Status.BadGateway,
                  body = fs2.Stream.emits("goodbye world".getBytes)
                )
              )
            )
          )
          .make
          .toTry
          .get
      )
      .make
      .toTry
      .get

  override def run: IO[Unit] = route
    .run(Request(uri = uri"/health"))
    .value
    .debug()
    .flatMap(_.traverse_(resp => resp.bodyText.compile.string.debug()))

}

This fails:

DEBUG: Errored: smithy4s.http.UnknownErrorResponse: status 502, headers: Map(), body:
goodbye world
smithy4s.http.UnknownErrorResponse: status 502, headers: Map(), body:
goodbye world
        at smithy4s.http.UnknownErrorResponse$.apply(UnknownErrorResponse.scala:19)
        at smithy4s.http.HttpResponse$Decoder$$anon$4.decode$$anonfun$1$$anonfun$1(HttpResponse.scala:248)
...

Internally, we've redefined our protocol to ignore the status code as a discriminator, so any error response that doesn't have a discriminator header results in an UnknownErrorResponse. This looks like:

.withErrorDiscriminator(resp =>
  MonadThrowLike[F].pure {
    val discriminator = HttpDiscriminator.fromResponse(errorHeaders, resp)
    discriminator match {
      case HttpDiscriminator.StatusCode(_) => HttpDiscriminator.Undetermined
      case other                           => other
    }
  }
)

and indeed, when an unparseable 500 is raised, the client doesn't even try, but instead it fails with UnknownErrorResponse: status 500.

Regardless of the status code, an UnknownErrorResponse raised in the client results in a failed IO, as was my original expectation. In default http4s behavior, that becomes a 500, which makes sense IMO.

Does this make sense?

[ghostbuster91]

hmm after some thinking, and seeing what we built as a "workaround" for this internally (or rather extra feature, because this isn't considered a bug) I'm starting to think this is... not something that needs fixing.

Hear me out: the immediate problem was that the client is returning some sort of PayloadError for an unparseable response, even though it's not a known error response, and not even JSON:

I was actually concerned about another thing.

The remaining issue, is when the external service returns a known (to the discriminator) but malformed response. In your example that would be 500 but with incorrect body payload. This would fail with PayloadError that gets then "fast-tracked" by smithy internals and converted to 400 BadRequest before returning to the most outer client:

//> using dep "com.disneystreaming.smithy4s::smithy4s-http4s:0.18.23"
//> using dep "com.disneystreaming.smithy4s::smithy4s-tests:0.18.23"
//> using option "-Wunused:imports"
import cats.effect.IO
import cats.effect.IOApp
import cats.implicits._
import org.http4s.Request
import org.http4s.implicits._
import smithy4s.example.PizzaAdminService
import smithy4s.http4s.SimpleRestJsonBuilder
import org.http4s.client.Client
import cats.effect.kernel.Resource
import org.http4s.Response
import org.http4s.Status

object Main extends IOApp.Simple {

  val route =
    SimpleRestJsonBuilder
      .routes(
        SimpleRestJsonBuilder(PizzaAdminService)
          .client[IO](
            Client(_ =>
              Resource.pure(
                Response(
                  status = Status.InternalServerError,
                  body = fs2.Stream.emits("goodbye world".getBytes)
                )
              )
            )
          )
          .make
          .toTry
          .get
      )
      .make
      .toTry
      .get

  override def run: IO[Unit] = route
    .run(Request(uri = uri"/health"))
    .value
    .debug()
    .flatMap(_.traverse_(resp => resp.bodyText.compile.string.debug()))

}

which returns:

DEBUG: Succeeded: Some(Response(status=400, httpVersion=HTTP/1.1, headers=Headers(Content-Type: application/json, Content-Length: 518)))
DEBUG: Succeeded: {"payload":{"path":".","expected":"object","message":"Expected JSON object, offset: 0x00000000, buf:\n+----------+-------------------------------------------------+------------------+\n|          |  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f | 0123456789abcdef |\n+----------+-------------------------------------------------+------------------+\n| 00000000 | 67 6f 6f 64 62 79 65 20 77 6f 72 6c 64          | goodbye world    |\n+----------+-------------------------------------------------+------------------+"}}

[Baccata]

Well I'm not sure whether your reasoning makes sense as a justification to not address the problem. As a matter of fact, it reinforces the justification to fix it, as in a situation where the server would issue a response that is not parseable by the client (which seems to happen often), the client would lose the original payload, which is detrimental to the user.

I think the correct solution would be that, upon failing to parse, always surface the same information we surface in UnknownErrorResponse but additionally carrying the HttpContractError if parsing was attempted.

[kubukoz]

So we just enhance UnknownErrorResponse by giving it an extra field for the underlying HttpContractError? Perhaps with a rename (like InvalidErrorResponse to account for "known but not valid") and maybe a field for the discriminator, if one was found?

[Baccata]

Yeah, I'm thinking we should get rid of the UnknownErrorResponse in favour of something like :

RawErrorResponse(status, headers, body, failedDecodeAttempt: Option[FailedDecodeAttempt])
FailedDecodeAttempt(discriminator, contractError) 

[kubukoz]

Sounds good to me. @ghostbuster91 @GaryAghedo any objections?

[ghostbuster91]

LGTM 👍

[kubukoz]

Closed in #1570