Issue-1438: Fix upstream service error represented as 400 (#1570)

* issue-1438- new error type for upstream service error

* Issue-1438: handle new error type and return 500

* issue-1438-formatted files

* issue-1438: undo formatting errors

* issue-1438-add missing import

* issue-1438: implemented RawErrorResponse error object

* issue-1438: add more test cases

* issue-1438: update tests and code clean up

* issue-1438: code clean up

* issue-1438: add missing headers

* issue-1438: format files

* refactor FailedDecodeAttempt

* make the RawErrorResponse case class more binary compatibility-friendly

* issue-1438: make all field private

* issue-1438: unapply method for RawResponse

* issue-1438: make unapply private

* issue-1438: fixed tests

* issue-1438: fixed tests

* issue-1438: make field public

* Add changelog entry

* Expose fields directly

* Redo the docs a bit, add more private  constructors

---------

Co-authored-by: Jakub Kozłowski <kubukoz@gmail.com>

CHANGELOG.md

@@ -25,6 +25,10 @@ Previously they'd be named after the **member target**, now they will use the na
 
 There's usually only one instance of `EncoderK[F, A]` for a particular `F[_]`, and interpreters don't need to know what `A` is. For convenience, the type parameter has been moved to a type member.
 
+# Removed `UnknownErrorResponse` in [#1570](https://github.com/disneystreaming/smithy4s/pull/1570)
+
+The error type `smithy4s.http.UnknownErrorResponse` has been replaced with `smithy4s.http.RawErrorResponse`, which provides a more accurate description of an error response that failed to decode, including a full representation of the response code, headers, body and the discriminator if one was found.
+
 # 0.18.24
 
 * Adds missing nanoseconds in Document encoding of EPOCH_SECOND timestamps

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

@@ -18,8 +18,7 @@ package smithy4s
 package http
 
 import smithy4s.capability.MonadThrowLike
-import smithy4s.codecs.PayloadError
-import smithy4s.codecs.PayloadPath
+import smithy4s.codecs.{PayloadError, PayloadPath}
 import smithy4s.kinds.PolyFunction
 import smithy4s.schema.Schema._
 import smithy4s.schema._

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

@@ -228,9 +228,9 @@ object HttpResponse {
     * Creates a response decoder that dispatches  the response
     * to a given decoder, based on some discriminator.
     */
-    private def discriminating[F[_], Body, Discriminator, E](
-        discriminate: HttpResponse[Body] => F[Discriminator],
-        select: Discriminator => Option[Decoder[F, Body, E]],
+    private def discriminating[F[_], Body, E](
+        discriminate: HttpResponse[Body] => F[HttpDiscriminator],
+        select: HttpDiscriminator => Option[Decoder[F, Body, E]],
         toStrict: Body => F[(Body, Blob)]
     )(implicit F: MonadThrowLike[F]): Decoder[F, Body, E] =
       new Decoder[F, Body, E] {
@@ -239,13 +239,32 @@ object HttpResponse {
             val strictResponse = response.copy(body = strictBody)
             F.flatMap(discriminate(strictResponse)) { discriminator =>
               select(discriminator) match {
-                case Some(decoder) => decoder.decode(strictResponse)
+                case Some(decoder) =>
+                  F.handleErrorWith(decoder.decode(strictResponse)) {
+                    case error: HttpContractError =>
+                      F.raiseError(
+                        RawErrorResponse(
+                          response.statusCode,
+                          response.headers,
+                          bodyBlob.toUTF8String,
+                          FailedDecodeAttempt.DecodingFailure(
+                            discriminator = discriminator,
+                            contractError = error
+                          )
+                        )
+                      )
+                    case otherError => F.raiseError(otherError)
+                  }
                 case None =>
                   F.raiseError(
-                    smithy4s.http.UnknownErrorResponse(
-                      response.statusCode,
-                      response.headers,
-                      bodyBlob.toUTF8String
+                    smithy4s.http.RawErrorResponse(
+                      code = response.statusCode,
+                      headers = response.headers,
+                      body = bodyBlob.toUTF8String,
+                      failedDecodeAttempt =
+                        FailedDecodeAttempt.UnrecognisedDiscriminator(
+                          discriminator
+                        )
                     )
                   )
               }

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

@@ -197,6 +197,7 @@ object HttpUnaryServerCodecs {
           def encodeError(e: E) = F.map(base)(errorW.write(_, e))
           def httpContractErrorEncoder(e: HttpContractError) =
             F.map(base)(httpContractErrorWriters.write(_, e).withStatusCode(400))
+
           def throwableEncoders(throwable: Throwable): F[HttpResponse[Blob]] =
             throwable match {
               case e: HttpContractError => httpContractErrorEncoder(e)

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

@@ -0,0 +1,126 @@
+/*
+ *  Copyright 2021-2024 Disney Streaming
+ *
+ *  Licensed under the Tomorrow Open Source Technology License, Version 1.0 (the "License");
+ *  you may not use this file except in compliance with the License.
+ *  You may obtain a copy of the License at
+ *
+ *     https://disneystreaming.github.io/TOST-1.0.txt
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License.
+ */
+
+package smithy4s.http
+
+import scala.annotation.nowarn
+
+final case class RawErrorResponse private (
+    code: Int,
+    headers: Map[CaseInsensitive, Seq[String]],
+    body: String,
+    failedDecodeAttempt: FailedDecodeAttempt
+) extends Throwable {
+
+  def withCode(code: Int): RawErrorResponse =
+    copy(code = code)
+
+  def withHeaders(
+      headers: Map[CaseInsensitive, Seq[String]]
+  ): RawErrorResponse =
+    copy(headers = headers)
+
+  def withBody(body: String): RawErrorResponse =
+    copy(body = body)
+
+  def withFailedDecodeAttempt(
+      failedDecodeAttempt: FailedDecodeAttempt
+  ): RawErrorResponse =
+    copy(failedDecodeAttempt = failedDecodeAttempt)
+
+  override def getMessage(): String = {
+    val baseMessage = s"status $code, headers: $headers, body:\n$body"
+    baseMessage + s"""
+                     |FailedDecodeAttempt:
+                     |  ${failedDecodeAttempt.getMessage}
+       """.stripMargin
+  }
+
+  override def getCause: Throwable = failedDecodeAttempt
+}
+
+object RawErrorResponse {
+  def apply(
+      code: Int,
+      headers: Map[CaseInsensitive, Seq[String]],
+      body: String,
+      failedDecodeAttempt: FailedDecodeAttempt
+  ): RawErrorResponse =
+    new RawErrorResponse(code, headers, body, failedDecodeAttempt)
+
+  @nowarn
+  private def unapply(response: RawErrorResponse): Option[
+    (Int, Map[CaseInsensitive, Seq[String]], String, FailedDecodeAttempt)
+  ] =
+    Some(
+      (
+        response.code,
+        response.headers,
+        response.body,
+        response.failedDecodeAttempt
+      )
+    )
+
+}
+
+sealed trait FailedDecodeAttempt extends Throwable {
+  def discriminator: HttpDiscriminator
+  def getMessage: String
+}
+
+object FailedDecodeAttempt {
+
+  final case class UnrecognisedDiscriminator private (
+      discriminator: HttpDiscriminator
+  ) extends FailedDecodeAttempt {
+
+    def withDiscriminator(
+        discriminator: HttpDiscriminator
+    ): UnrecognisedDiscriminator =
+      copy(discriminator = discriminator)
+
+    override def getMessage: String =
+      s"Unrecognised discriminator: $discriminator"
+  }
+
+  object UnrecognisedDiscriminator {
+    def apply(discriminator: HttpDiscriminator): UnrecognisedDiscriminator =
+      new UnrecognisedDiscriminator(discriminator)
+  }
+
+  final case class DecodingFailure private (
+      discriminator: HttpDiscriminator,
+      contractError: HttpContractError
+  ) extends FailedDecodeAttempt {
+
+    def withDiscriminator(discriminator: HttpDiscriminator): DecodingFailure =
+      copy(discriminator = discriminator)
+
+    def withContractError(contractError: HttpContractError): DecodingFailure =
+      copy(contractError = contractError)
+
+    override def getMessage: String =
+      s"Decoding failed for discriminator: $discriminator with error: ${contractError.getMessage}"
+  }
+
+  object DecodingFailure {
+    def apply(
+        discriminator: HttpDiscriminator,
+        contractError: HttpContractError
+    ): DecodingFailure =
+      new DecodingFailure(discriminator, contractError)
+  }
+}

modules/docs/markdown/03-protocols/04-simple-rest-json/03-client.md

@@ -57,10 +57,21 @@ All smithy4s services provide an `X-Error-Type` in responses by default.
 
 #### Status Code
 
-If the `X-Error-Type` header is not defined, smithy4s clients will use the status code to attempt to decide which error type to utilize. It does so as follows:
+If the `X-Error-Type` header is provided, smithy4s clients will attempt to use the status code to decide which error type to utilize. They do so as follows:
 
-1. If there is a single Error type that contains the correct status code in the `httpError` trait, this type will be used. If there are two error types with the same status code, an `UnknownErrorResponse` will be surfaced to the client.
-2. If there is NOT a matching status code, but there is a single error marked with the `error` trait, this error type will be used as long as the returned status code is in the range for either a client or server error. In other words if a single error shape has no status code, but is annotated with `@error("client")` and the returned status code is 404 then this error type will be used. If there are multiple error types with no status code and a matching error type (client/server), then an `UnknownErrorResponse` will be surfaced to the client.
+1. First, we look for an exact match. The response code is compared against the value of the `@httpError` trait on the operation's error types. If there's more than one match, a `RawErrorResponse` is raised. If there were no exact matches, we move on to step 2.
+2. Now, we check the category of the response code: if it's a `4xx` (e.g. 404 or 401), we look for **exactly one** error marked with `@error("client")`. If it's a `5xx`, we look for `@error("server")` instead. Again, in case of more than one match, we raise a `RawErrorResponse`.
+
+#### Total failure of decoding
+
+If all the above methods fail to find a suitable error type to decode the response as, OR if one is found but the response doesn't match its Smithy schema, a `RawErrorResponse` is raised.
+
+The `RawErrorResponse` class carries information about the response that produced it. It also holds information about _why_ it was raised (in a `FailedDecodeAttempt`), e.g.
+
+- the decoding couldn't figure out which error type to use (`FailedDecodeAttempt.UnrecognisedDiscriminator`)
+- an error type was found but it failed to decode (`FailedDecodeAttempt.DecodingFailure`).
+
+#### Examples
 
 Here are some examples to show more what this looks like.
 
@@ -115,12 +126,12 @@ structure AnotherError {
 
 Would result in the following:
 
-| Status Code | Error Selected           |
-| ----------- | ------------------------ |
-| 404         | NotFoundError            |
-| 400         | **UnknownErrorResponse** |
-| 503         | ServiceUnavailableError  |
-| 500         | CatchAllServerError      |
+| Status Code | Error Selected          |
+| ----------- | ----------------------- |
+| 404         | NotFoundError           |
+| 400         | **RawErrorResponse**    |
+| 503         | ServiceUnavailableError |
+| 500         | CatchAllServerError     |
 
 Notice that the 400 status code cannot be properly mapped. This is because there is no exact match AND there are two errors that are labeled with `@error("client")` which also do not have an associated `httpError` trait containing a status code.
 
@@ -136,12 +147,12 @@ structure AnotherNotFoundError {
 
 Will result in the following:
 
-| Status Code | Error Selected           |
-| ----------- | ------------------------ |
-| 404         | **UnknownErrorResponse** |
-| 400         | UnknownErrorResponse     |
-| 503         | ServiceUnavailableError  |
-| 500         | CatchAllServerError      |
+| Status Code | Error Selected          |
+| ----------- | ----------------------- |
+| 404         | **RawErrorResponse**    |
+| 400         | RawErrorResponse        |
+| 503         | ServiceUnavailableError |
+| 500         | CatchAllServerError     |
 
 Now the 404 status code cannot be mapped. This is due to the fact that two different error types are annotated with a 404 `httpError` trait. This means that the smithy4s
 client is not able to decide which of these errors is correct.

modules/http4s/test/src/smithy4s/http4s/Http4sPizzaSpec.scala

@@ -19,8 +19,9 @@ package smithy4s.http4s
 import cats.effect.IO
 import cats.effect.Resource
 import org.http4s.Uri
-import org.http4s.client.Client
+import org.http4s.Response
 import org.http4s.implicits._
+import org.http4s.client.Client
 import smithy4s.example.PizzaAdminService
 
 object Http4sPizzaSpec extends smithy4s.tests.PizzaSpec {
@@ -40,4 +41,24 @@ object Http4sPizzaSpec extends smithy4s.tests.PizzaSpec {
 
   }
 
+  def runServerWithClient(
+      pizzaService: PizzaAdminService[IO],
+      clientResponse: Response[IO]
+  ): Resource[IO, Res] = {
+    SimpleRestJsonBuilder
+      .routes(
+        SimpleRestJsonBuilder(PizzaAdminService)
+          .client[IO](Client(_ => Resource.pure(clientResponse)))
+          .make
+          .toTry
+          .get
+      )
+      .resource
+      .map { httpRoutes =>
+        val client = Client.fromHttpApp(httpRoutes.orNotFound)
+        val uri = Uri.unsafeFromString("http://localhost")
+        (client, uri)
+      }
+  }
+
 }

modules/tests/src/smithy4s/tests/PizzaAdminServiceImpl.scala

@@ -17,15 +17,14 @@
 package smithy4s.tests
 
 import cats.effect._
+import cats.effect.std.UUIDGen
 import cats.implicits._
 import smithy4s.Timestamp
 import smithy4s.example._
+import smithy4s.tests.PizzaAdminServiceImpl._
 
 import java.util.UUID
 
-import PizzaAdminServiceImpl._
-import cats.effect.std.UUIDGen
-
 object PizzaAdminServiceImpl {
   case class Item(food: Food, price: Float, addedAt: Timestamp)
   case class State(restaurants: Map[String, Restaurant])