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