diff --git a/CHANGELOG.md b/CHANGELOG.md index 61e644bb3..1a217ff6f 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/modules/core/src/smithy4s/http/HttpContractError.scala b/modules/core/src/smithy4s/http/HttpContractError.scala index 2b381a507..6f67229cf 100644 --- a/modules/core/src/smithy4s/http/HttpContractError.scala +++ b/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._ diff --git a/modules/core/src/smithy4s/http/HttpResponse.scala b/modules/core/src/smithy4s/http/HttpResponse.scala index c134453f6..770eb9425 100644 --- a/modules/core/src/smithy4s/http/HttpResponse.scala +++ b/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 + ) ) ) } diff --git a/modules/core/src/smithy4s/http/HttpUnaryServerCodecs.scala b/modules/core/src/smithy4s/http/HttpUnaryServerCodecs.scala index 8ec911a96..71c468884 100644 --- a/modules/core/src/smithy4s/http/HttpUnaryServerCodecs.scala +++ b/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) diff --git a/modules/core/src/smithy4s/http/RawErrorResponse.scala b/modules/core/src/smithy4s/http/RawErrorResponse.scala new file mode 100644 index 000000000..93fe9b872 --- /dev/null +++ b/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) + } +} diff --git a/modules/core/src/smithy4s/http/UnknownErrorResponse.scala b/modules/core/src/smithy4s/http/UnknownErrorResponse.scala deleted file mode 100644 index 6f0152798..000000000 --- a/modules/core/src/smithy4s/http/UnknownErrorResponse.scala +++ /dev/null @@ -1,26 +0,0 @@ -/* - * 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 - -case class UnknownErrorResponse( - code: Int, - headers: Map[CaseInsensitive, Seq[String]], - body: String -) extends Throwable { - override def getMessage(): String = - s"status $code, headers: $headers, body:\n$body" -} diff --git a/modules/docs/markdown/03-protocols/04-simple-rest-json/03-client.md b/modules/docs/markdown/03-protocols/04-simple-rest-json/03-client.md index 9e3b02ea9..6cae4901c 100644 --- a/modules/docs/markdown/03-protocols/04-simple-rest-json/03-client.md +++ b/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. diff --git a/modules/http4s/test/src/smithy4s/http4s/Http4sPizzaSpec.scala b/modules/http4s/test/src/smithy4s/http4s/Http4sPizzaSpec.scala index 687d1b7c0..e76d08b23 100644 --- a/modules/http4s/test/src/smithy4s/http4s/Http4sPizzaSpec.scala +++ b/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) + } + } + } diff --git a/modules/tests/src/smithy4s/tests/PizzaAdminServiceImpl.scala b/modules/tests/src/smithy4s/tests/PizzaAdminServiceImpl.scala index d738736c8..12787f5be 100644 --- a/modules/tests/src/smithy4s/tests/PizzaAdminServiceImpl.scala +++ b/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]) diff --git a/modules/tests/src/smithy4s/tests/PizzaClientSpec.scala b/modules/tests/src/smithy4s/tests/PizzaClientSpec.scala index 8108acf94..406c7d229 100644 --- a/modules/tests/src/smithy4s/tests/PizzaClientSpec.scala +++ b/modules/tests/src/smithy4s/tests/PizzaClientSpec.scala @@ -31,7 +31,9 @@ import smithy4s.example._ import smithy4s.Timestamp import weaver._ import smithy4s.http.CaseInsensitive -import smithy4s.http.UnknownErrorResponse +import smithy4s.http.RawErrorResponse +import smithy4s.http.HttpDiscriminator +import smithy4s.http.FailedDecodeAttempt.DecodingFailure abstract class PizzaClientSpec extends IOSuite { @@ -111,29 +113,49 @@ abstract class PizzaClientSpec extends IOSuite { GenericClientError("generic error message for 418") ) - clientTestForError( - "Handle error w/o a discriminator header nor a unique status code", - Response(status = Status.ProxyAuthenticationRequired) - .withEntity( - Json.obj("message" -> Json.fromString("generic client error message")) - ), - unknownResponse( - 407, - Map("Content-Length" -> "42", "Content-Type" -> "application/json"), - """{"message":"generic client error message"}""" - ) + clientAssertError[RawErrorResponse]( + "Handle error with a discriminator but can't be decoded", + Response[IO](status = Status.NotFound) + .withEntity("malformed body") + .withHeaders(Header.Raw(CIString("X-Error-Type"), "NotFoundError")), + error => { + expect(error.code == 404) && + expect( + error.headers == Map( + CaseInsensitive("X-Error-Type") -> List("NotFoundError") + ) + ) && + expect(error.body == "malformed body") && + expect( + error.failedDecodeAttempt.discriminator == HttpDiscriminator.NameOnly( + "NotFoundError" + ) + ) && + expect(error.failedDecodeAttempt.isInstanceOf[DecodingFailure]) + } ) - private def unknownResponse( - code: Int, - headers: Map[String, String], - body: String - ): UnknownErrorResponse = - UnknownErrorResponse( - code, - headers.map { case (k, v) => CaseInsensitive(k) -> List(v) }, - body - ) + clientAssertError[RawErrorResponse]( + "Handle malformed error response with no discriminator", + Response(status = Status.InternalServerError) + .withEntity("goodbye world"), + error => { + expect(error.code == 500) && + expect( + error.headers == Map( + CaseInsensitive("Content-Length") -> List("13"), + CaseInsensitive("Content-Type") -> List( + "text/plain; charset=UTF-8" + ) + ) + ) && + expect(error.body == "goodbye world") && + expect( + error.failedDecodeAttempt.discriminator == HttpDiscriminator + .StatusCode(500) + ) + } + ) clientTest("Headers are case insensitive") { (client, backend, log) => for { @@ -208,6 +230,30 @@ abstract class PizzaClientSpec extends IOSuite { } } + def clientAssertError[E]( + name: String, + response: Response[IO], + assert: E => Expectations + )(implicit + loc: SourceLocation, + ct: scala.reflect.ClassTag[E] + ) = { + clientTest(name) { (client, backend, log) => + for { + _ <- backend.prepResponse(name, response) + maybeResult <- client.getMenu(name).attempt + + } yield maybeResult match { + case Right(_) => failure("expected failure") + case Left(error: E) => + assert(error) + case Left(error) => + failure(s"Error of unexpected type: $error") + } + } + + } + def clientTest(name: TestName)( f: ( PizzaAdminService[IO], diff --git a/modules/tests/src/smithy4s/tests/PizzaSpec.scala b/modules/tests/src/smithy4s/tests/PizzaSpec.scala index b17982521..4cf954c08 100644 --- a/modules/tests/src/smithy4s/tests/PizzaSpec.scala +++ b/modules/tests/src/smithy4s/tests/PizzaSpec.scala @@ -19,17 +19,23 @@ package smithy4s.tests import cats.data.NonEmptyList import cats.effect._ import cats.syntax.all._ + import io.circe._ -import org.http4s.Request +import org.http4s._ import org.http4s.Uri import org.http4s.circe._ import org.http4s.client.Client +import org.typelevel.ci.CIString import org.http4s.client.dsl.Http4sClientDsl import org.http4s.dsl.Http4sDsl +import org.http4s.Response import smithy4s.http.HttpPayloadError import smithy4s.example.PizzaAdminService import smithy4s.http.CaseInsensitive import smithy4s.http.HttpContractError +import smithy4s.http.HttpDiscriminator +import smithy4s.http.FailedDecodeAttempt +import smithy4s.http.RawErrorResponse import weaver._ import cats.Show import org.http4s.EntityDecoder @@ -44,6 +50,11 @@ abstract class PizzaSpec errorAdapter: PartialFunction[Throwable, Throwable] ): Resource[IO, Res] + def runServerWithClient( + pizzaService: PizzaAdminService[IO], + clientResponse: Response[IO] + ): Resource[IO, Res] + val pizzaItem = Json.obj( "pizza" -> Json.obj( "name" -> Json.fromString("margharita"), @@ -463,6 +474,113 @@ abstract class PizzaSpec } } + routerTest("Negative: handle error without discriminator") { + (client, uri, log) => + val badResponse = org.http4s.Response[IO]( + status = Status.InternalServerError, + body = fs2.Stream.emits("malformed body".getBytes), + headers = Headers( + Header.Raw(CIString("Content-Length"), "14"), + Header.Raw(CIString("Content-Type"), "text/plain") + ) + ) + + for { + stateRef <- IO.ref(PizzaAdminServiceImpl.State(Map.empty)) + impl = new PizzaAdminServiceImpl(stateRef) + response <- runServerWithClient( + impl, + badResponse + ).use { case (client, uri) => + val request = POST( + menuItem, + uri / "restaurant" / "boom" / "menu" / "item" + ) + client + .run(request) + .use { response => + IO.pure(response.status.code) + } + .attempt + } + + } yield { + val expectHeaders = Map( + CaseInsensitive("Content-Type") -> List("text/plain"), + CaseInsensitive("Content-Length") -> List("14") + ) + response match { + case Left(e: RawErrorResponse) => + expect(e.code == 500) && + expect(e.headers == expectHeaders) && + expect(e.body.contains("malformed body")) && + expect( + e.failedDecodeAttempt == FailedDecodeAttempt + .UnrecognisedDiscriminator( + HttpDiscriminator.StatusCode(500) + ) + ) + case _ => + failure("Expected RawErrorResponse with status 500") + } + } + } + + routerTest( + "Negative: handle decoder exception for known error with discriminator" + ) { (client, uri, log) => + val badResponse = org.http4s.Response[IO]( + status = Status.InternalServerError, + body = fs2.Stream.emits("malformed body".getBytes), + headers = Headers( + Header.Raw(CIString("Content-Length"), "14"), + Header.Raw(CIString("Content-Type"), "text/plain"), + Header.Raw(CIString("X-Error-Type"), "GenericServerError") + ) + ) + + for { + stateRef <- IO.ref(PizzaAdminServiceImpl.State(Map.empty)) + impl = new PizzaAdminServiceImpl(stateRef) + response <- runServerWithClient( + impl, + badResponse + ).use { case (client, uri) => + val request = POST( + menuItem, + uri / "restaurant" / "boom" / "menu" / "item" + ) + client + .run(request) + .use { response => + IO.pure(response.status.code) + } + .attempt + + } + + } yield { + val expectHeaders = Map( + CaseInsensitive("Content-Length") -> List("14"), + CaseInsensitive("Content-Type") -> List("text/plain"), + CaseInsensitive("X-Error-Type") -> List("GenericServerError") + ) + response match { + case Left(e: RawErrorResponse) => + expect(e.code == 500) && + expect(e.headers == expectHeaders) && + expect(e.body.contains("malformed body")) && + expect( + e.failedDecodeAttempt.discriminator == HttpDiscriminator.NameOnly( + "GenericServerError" + ) + ) + case _ => + failure("Expected RawErrorResponse with status 500") + } + } + } + type Res = (Client[IO], Uri) def sharedResource: Resource[IO, (Client[IO], Uri)] = for { stateRef <- Resource.eval(