Skip to content
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

Fail when there's an unexpected response body #3095

Merged
merged 10 commits into from
Aug 24, 2023
2 changes: 1 addition & 1 deletion build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,7 @@ lazy val testing: ProjectMatrix = (projectMatrix in file("testing"))
.jvmPlatform(scalaVersions = scala2And3Versions)
.jsPlatform(scalaVersions = scala2And3Versions, settings = commonJsSettings)
.nativePlatform(scalaVersions = List(scala3), settings = commonNativeSettings)
.dependsOn(core)
.dependsOn(core, circeJson % Test)

lazy val tests: ProjectMatrix = (projectMatrix in file("tests"))
.settings(commonSettings)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,9 +220,10 @@ class ServerInterpreter[R, F[_], B, S](
val statusCode = outputValues.statusCode.getOrElse(defaultStatusCode)

val headers = outputValues.headers
outputValues.body match {
case Some(bodyFromHeaders) => ServerResponse(statusCode, headers, Some(bodyFromHeaders(Headers(headers))), Some(output)).unit
case None => ServerResponse(statusCode, headers, None: Option[B], Some(output)).unit
(statusCode, outputValues.body) match {
case (StatusCode.NoContent | StatusCode.NotModified, Some(_)) => monad.error(new IllegalStateException(s"Unexpected response body when status code == $statusCode"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be better to say: The status code XYZ doesn't allow a body

Also ... what if the response body is defined as an Option? Or if the body is returned as an empty string "" - shouldn't we special case on this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be hard to check in this general function. The body is of type B which is backend-dependent (like akka.http.scaladsl.model.HttpEntity).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, forget it then :)

case (_, Some(bodyFromHeaders)) => ServerResponse(statusCode, headers, Some(bodyFromHeaders(Headers(headers))), Some(output)).unit
case (_, None) => ServerResponse(statusCode, headers, None: Option[B], Some(output)).unit
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -769,6 +769,34 @@ class ServerBasicTests[F[_], OPTIONS, ROUTE](
r.code shouldBe StatusCode.InternalServerError
r.body shouldBe Symbol("left")
}
},
testServer(
"fail when status is 204 or 304, but there's a body",
NonEmptyList.of(
route(List(
endpoint.in("no_content").out(jsonBody[Unit]).out(statusCode(StatusCode.NoContent)).serverLogicSuccess[F](_ => pureResult(())),
endpoint.in("not_modified").out(jsonBody[Unit]).out(statusCode(StatusCode.NotModified)).serverLogicSuccess[F](_ => pureResult(())),
endpoint
.in("one_of")
.in(query[String]("select_err"))
.errorOut(
sttp.tapir.oneOf[ErrorInfo](
oneOfVariant(statusCode(StatusCode.NotFound).and(jsonBody[NotFound])),
oneOfVariant(statusCode(StatusCode.NoContent).and(jsonBody[NoContentData])),
)
)
.serverLogic[F] { selectErr =>
if (selectErr == "no_content")
pureResult[F, Either[ErrorInfo, Unit]](Left(NoContentData("error")))
else
pureResult[F, Either[ErrorInfo, Unit]](Left(NotFound("error")))
}
)))
) { (backend, baseUri) =>
basicRequest.get(uri"$baseUri/no_content").send(backend).map(_.code shouldBe StatusCode.InternalServerError) >>
basicRequest.get(uri"$baseUri/not_modified").send(backend).map(_.code shouldBe StatusCode.InternalServerError) >>
basicRequest.get(uri"$baseUri/one_of?select_err=no_content").send(backend).map(_.code shouldBe StatusCode.InternalServerError) >>
basicRequest.get(uri"$baseUri/one_of?select_err=not_found").send(backend).map(_.code shouldBe StatusCode.NotFound)
}
)

Expand All @@ -787,3 +815,7 @@ object Animal extends Enum[Animal] with TapirCodecEnumeratum {

override def values = findValues
}

sealed trait ErrorInfo
case class NotFound(what: String) extends ErrorInfo
case class NoContentData(msg: String) extends ErrorInfo
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package sttp.tapir.testing

import sttp.model.Method
import sttp.tapir.AnyEndpoint
import sttp.model.StatusCode

sealed trait EndpointVerificationError

Expand Down Expand Up @@ -52,3 +53,17 @@ case class IncorrectPathsError(e: AnyEndpoint, at: Int) extends EndpointVerifica
case class DuplicatedMethodDefinitionError(e: AnyEndpoint, methods: List[Method]) extends EndpointVerificationError {
override def toString: String = s"An endpoint ${e.show} have multiple method definitions: $methods"
}

/**
* Endpoint `e` defines outputs where status code indicates no body, but at the same time a body output is specified. For status codes 204 and 304 it's forbidden by specification.
*
* Example of incorrectly defined endpoint:
*
* {{{
* endpoint.get.in("x").out(jsonBody[Unit]).out(statusCode(StatusCode.NoContent))
* }}}
*
*/
case class UnexpectedBodyError(e: AnyEndpoint, statusCode: StatusCode) extends EndpointVerificationError {
override def toString: String = s"An endpoint ${e.show} may return status code ${statusCode} with body, which is not allowed by specificiation."
}
21 changes: 18 additions & 3 deletions testing/src/main/scala/sttp/tapir/testing/EndpointVerifier.scala
Original file line number Diff line number Diff line change
@@ -1,16 +1,18 @@
package sttp.tapir.testing

import sttp.model.Method
import sttp.tapir.internal.{RichEndpointInput, UrlencodedData}
import sttp.tapir.{AnyEndpoint, EndpointInput, testing}
import sttp.model.StatusCode.{NoContent, NotModified}
import sttp.tapir.internal.{RichEndpointInput, RichEndpointOutput, UrlencodedData}
import sttp.tapir.{AnyEndpoint, EndpointIO, EndpointInput, EndpointOutput, testing}

import scala.annotation.tailrec

object EndpointVerifier {
def apply(endpoints: List[AnyEndpoint]): Set[EndpointVerificationError] = {
findShadowedEndpoints(endpoints, List()).groupBy(_.e).map(_._2.head).toSet ++
findIncorrectPaths(endpoints).toSet ++
findDuplicatedMethodDefinitions(endpoints).toSet
findDuplicatedMethodDefinitions(endpoints).toSet ++
findIncorrectStatusWithBody(endpoints).toSet
}

private def findIncorrectPaths(endpoints: List[AnyEndpoint]): List[IncorrectPathsError] = {
Expand All @@ -35,6 +37,19 @@ object EndpointVerifier {
in.filter(e => checkIfShadows(endpoint, e)).map(e => testing.ShadowedEndpointError(e, endpoint))
}

private def findIncorrectStatusWithBody(endpoints: List[AnyEndpoint]): List[UnexpectedBodyError] =
endpoints.flatMap { e =>
val outputs = (e.output.asBasicOutputsList ++ e.errorOutput.asBasicOutputsList)
outputs.flatMap { outputElems =>
val hasBody = outputElems.collectFirst { case b: EndpointIO.Body[_, _] => b }.isDefined
val noBodyStatusCodes = outputElems.collect {
case EndpointOutput.FixedStatusCode(NoContent, _, _) => NoContent
case EndpointOutput.FixedStatusCode(NotModified, _, _) => NotModified
}
if (hasBody) noBodyStatusCodes.map(UnexpectedBodyError(e, _)) else Nil
}
}

private def checkIfShadows(e1: AnyEndpoint, e2: AnyEndpoint): Boolean =
checkMethods(e1, e2) && checkPaths(e1, e2)

Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
package sttp.tapir.testing

import io.circe.generic.auto._
import org.scalatest.flatspec.AnyFlatSpecLike
import org.scalatest.matchers.should.Matchers
import sttp.model.Method
import sttp.model.{Method, StatusCode}
import sttp.tapir._
import sttp.tapir.generic.auto._
import sttp.tapir.json.circe._

class EndpointVerifierTest extends AnyFlatSpecLike with Matchers {

Expand Down Expand Up @@ -286,4 +289,42 @@ class EndpointVerifierTest extends AnyFlatSpecLike with Matchers {

result shouldBe Set()
}

it should "detect endpoints with body where status code doesn't allow a body" in {

val e1 = endpoint.in("endpoint1_Err").out(stringBody).out(statusCode(StatusCode.NoContent))
val e2 = endpoint.in("endpoint2_Ok").out(stringBody).out(statusCode(StatusCode.BadRequest))
val e3 = endpoint.in("endpoint3_Err").out(stringBody).out(statusCode(StatusCode.NotModified))
val e4 = endpoint.in("endpoint4_ok").out(emptyOutputAs(NoContent)).out(statusCode(StatusCode.NoContent))
val e5 = endpoint
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the checker pass for an endpoint such as:

val e6 = endpoint
      .in("endpoint6_ok")
       .errorOut(
         sttp.tapir.oneOf[ErrorInfo](
           oneOfVariant(statusCode(StatusCode.NotFound).and(jsonBody[NotFound])),
           oneOfVariant(statusCode(StatusCode.NoContent).and(emptyOutputAs[NoContent]))
         )
       )

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea to check this, I have added a test.

.in("endpoint5_err")
.out(stringBody)
.errorOut(
sttp.tapir.oneOf[ErrorInfo](
oneOfVariant(statusCode(StatusCode.NotFound).and(jsonBody[NotFound])),
oneOfVariant(statusCode(StatusCode.NoContent).and(jsonBody[NoContentData]))
)
)
val e6 = endpoint
.in("endpoint6_ok")
.errorOut(
sttp.tapir.oneOf[ErrorInfo](
oneOfVariant(statusCode(StatusCode.NotFound).and(jsonBody[NotFound])),
oneOfVariant(statusCode(StatusCode.NoContent).and(emptyOutputAs(NoContent)))
)
)

val result = EndpointVerifier(List(e1, e2, e3, e4, e5, e6))

result shouldBe Set(
UnexpectedBodyError(e1, StatusCode.NoContent),
UnexpectedBodyError(e3, StatusCode.NotModified),
UnexpectedBodyError(e5, StatusCode.NoContent)
)
}
}

sealed trait ErrorInfo
case class NotFound(what: String) extends ErrorInfo
case object NoContent extends ErrorInfo
case class NoContentData(msg: String) extends ErrorInfo