tapir icon indicating copy to clipboard operation
tapir copied to clipboard

happy flow statuscodes 3xx, 4xx, 5xx considered an error?

Open ThijsBroersen opened this issue 1 year ago • 9 comments

Tapir version: 1.2.12

Scala version: 2.13.10

A 3xx, 4xx and 5xx status codes are always considered an error when put into the happy flow (.out).

I think all happy flow definitions should be parsed as right-flow, not left. Consider:

..: PublicEndpoint[Unit, Unit, String, Any]
---
.out(statusCode(StatusCode.TemporaryRedirect))
.out(
  header[String]("Location").description("Redirect address")
)

I would expect this endpoint definition to do a call and get back a 307 response with a header. I expect the 307 response and everything else I want to consider an error. The openapi spec will correctly list the response as one-of the possible response (including all listed 4xx and 5xx response). So also:

..: PublicEndpoint[Unit, Unit, String, Any]
---
.out(statusCode(StatusCode.NotFound))
.out(
  header[String]("MyCustomHeader").description("Some value")
)

should follow a right-flow when used with tapir clients.

Only when the response does not fit within the right-flow it should go left and attempt decoding using the defined errorOut types.

What do you think?

ThijsBroersen avatar Apr 13 '23 19:04 ThijsBroersen

Yes I think this might make sense, though unless status codes mappings are explicitly defined, I'd keep the current 200/non-200 split.

Question is, what if an endpoint defines a catch-all status code (a statusCode output, without a fixed value). Would this mean that every response should be parsed as the "happy path"?

adamw avatar May 09 '23 17:05 adamw

I'm currently defining and endpoint and implementing a client using it. A particular endpoint always returns a 302 Found response and I need to retrieve the Location header in this case. But 302 is considered an error and I don't seem to be able to override it.

Maybe there is a way and I'm just missing it?

I have something like this currently:

 .out(
        oneOf(
          oneOfVariant(
            StatusCode.Found,
            header[String](HeaderNames.Location)
...

(3xx is not really an error in terms of http)

yurique avatar Jan 09 '24 18:01 yurique

Currently that's unfortunately hard-coded in the client interpreter: https://github.com/softwaremill/tapir/blob/7a9e8872a7e3faaf2f7d94bb7e4ab01beb72c80b/client/sttp-client/src/main/scala/sttp/tapir/client/sttp/EndpointToSttpClient.scala#L44

I guess we could traverse the outputs of a given endpoint, collect all known status codes, and modify the logic to use the outputs then.

What would be problematic, is when the status code is dynamically generated - then we don't really have a way to determine on the client side if it was generated by out or errorOut, so we'd have to use the current heuristic anyway.

adamw avatar Jan 10 '24 11:01 adamw

Yeah, I can see how this can be a tricky problem. For now my workaround is to register an errorOut for the 302 Found with a special "error" class that contains the extracted Location and then recover this particular error in the client code.

yurique avatar Jan 10 '24 11:01 yurique

There's another problem semi-related to this: AkkaHTTP backend follows redirects when created by one of the AkkaHttpBackend.apply methods (and the constructor is private - so it doesn't look like there's a way to make it NOT follow redirects). While, on the other hand, http4s client does NOT follow the redirects. So an http4s-based client built on top of an endpoint that expects redirects works, and with AkkaHTTP - it doesn't (took us a while to figure out what was going on).

yurique avatar Jan 16 '24 18:01 yurique

@yurique isn't that a configuration option of Akka, to automatically follow redirects or not? I think there is a way to provide custom configuration for akka

adamw avatar Jan 22 '24 16:01 adamw

@adamw I mean this one: https://github.com/softwaremill/sttp/blob/master/akka-http-backend/src/main/scala/sttp/client4/akkahttp/AkkaHttpBackend.scala#L187 - I'm assuming FollowRedirectsBackend will follow redirects no matter what :)

yurique avatar Jan 22 '24 17:01 yurique

@yurique Ah yes - unless the request has the followRedirects option set to false (or a backend delegate sets it). But the http4s backend does the same? https://github.com/softwaremill/sttp/blob/db80192a21707c00ec7f220289662c1c3d7111eb/http4s-backend/src/main/scala/sttp/client4/http4s/Http4sBackend.scala#L285

adamw avatar Jan 22 '24 17:01 adamw

@adamw Interesting! I don't think we're changing any defaults when constructing the request:

// http4s
val (req, parseResponse) = toSecureRequest(e, baseUri)(authIn)(input)

// akka
val req = toSecureRequest(e, baseUri.map(Uri.unsafeParse))(implicitly)(authIn)(input)

and I'm pretty sure the http4s version doesn't follow the 302 redirect. I'll take another look at it, though.

Regardless, this is super helpful! We can configure it and that's good :).

yurique avatar Jan 22 '24 17:01 yurique