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

[BUG] Tapir oneOf generates unexpected behavior of generated sttp client #2354

Closed
majk-p opened this issue Aug 5, 2022 · 3 comments · Fixed by #2357
Closed

[BUG] Tapir oneOf generates unexpected behavior of generated sttp client #2354

majk-p opened this issue Aug 5, 2022 · 3 comments · Fixed by #2357

Comments

@majk-p
Copy link
Contributor

majk-p commented Aug 5, 2022

TL;DR I think I've found a bug in how Tapir generates client behavior based on OneOfVariant used. The following repository reproduces the issue https://github.com/majk-p/oneof-tapir-demo

Tapir versions tested: 1.0.1, 0.19.x
Scala version: 2.13.8

Background

2 teams, one provides a service, and the other uses it over https. Both use tapir. The client models the server endpoint on its own and generates a client out of the sources (each team has its own code repository).

Goal

On the server side create an endpoint GET /order/:id: that

  • Requires bearer auth
  • Returns 404 Not found when order doesn't exist
  • Returns order object when it exists

The client wants to model that endpoint regardless of the way it's implemented on the server. Especially the client team doesn't need to know anything more than the error code in terms of error modeling.

The problem

Let's say the server implements the endpoint following way:

val findOrder: Endpoint[String, String, Error, Order, Any] =
  endpoint.get
    .securityIn(auth.bearer[String]())
    .in("order")
    .in(path[String]("id"))
    .out(jsonBody[Order])
    .errorOut(
      oneOf(notFoundJson, unauthorized)
    )

where

val notFoundJson =
  oneOfVariantValueMatcher(
    StatusCode.NotFound,
    jsonBody[Unit].map(_ => NotFound)(_ => ())
  ) { case NotFound => true }

and the client goes with:

val findOrder: Endpoint[String, String, Error, Order, Any] =
  endpoint.get
    .securityIn(auth.bearer[String]())
    .in("order")
    .in(path[String]("id"))
    .out(jsonBody[Order])
    .errorOut(
      oneOf(
        notFoundStatus, // This seems to be wha'ts causing the issue
        unauthorized
      )
    )

where

val notFoundStatus =
  oneOfVariantValueMatcher(
    // If the status matches it's good enough, we don't care about the body and headers
    statusCode(StatusCode.NotFound).map(_ => NotFound)(_ => ())
  ) { case NotFound => true }

So the difference is clearly only about how we define the matcher.

When we generate the client out of the client findOrder and try to execute it against the server, in the case when we ask for non-existing Order. The request ends with DecodeResult.Failure of Mismatch(403,404). This seems like the client was expecting 403 but received 404.

With the notFoundStatus definition I'd expect that the client would check if the status code matches, and if it does - it should match.

I've prepared the full example available at https://github.com/majk-p/oneof-tapir-demo. If you run that code you'll get the following output:

...
====================
basic checks
Response = {"Unauthorized":{"message":"Failed"}} Code = 403
Successfully verified invalid token
Response = {} Code = 404
Successfully verified invalid user
Response = {"orderId":"1"} Code = 200
Successfully verified valid user
====================
client checks where client uses server endpoint definition
Result: Left(java.lang.Exception: Error Unauthorized(Failed))
Successfully verified invalid token
Result: Right(None)
Successfully verified invalid user
Result: Right(Some(Order(1)))
Successfully verified valid user
====================
client checks where client uses client specific endpoint definition
Result: Left(java.lang.Exception: Error Unauthorized(Failed))
Successfully verified invalid token
Result: Left(java.lang.Exception: failed to decode response: Mismatch(403,404))
[error] java.lang.AssertionError: assertion failed
[error]         at scala.Predef$.assert(Predef.scala:264)
[error]         at net.michalp.oneoftapirdemo.ClientChecks.$anonfun$verifyValidTokenInvalidOrder$1(ClientChecks.scala:52)
[error]         at net.michalp.oneoftapirdemo.ClientChecks.$anonfun$verifyValidTokenInvalidOrder$1$adapted(ClientChecks.scala:50)
...

I would expect this would work that this third example works exactly like the second one.

My questions are:

  1. Is my approach correct? Is it the right thing to expect this matcher to work that way?
  2. If not, is modeling client 1:1 with the server the only option? what if I don't have access to the server-side implementation?
  3. If it's correct - any ideas on how to approach fixing this? I'd be happy to contribute if that's not a huge change
@matwojcik
Copy link
Contributor

What's interesting if client models only single error like that:

 val findOrderWithSingleError: Endpoint[String, String, NotFound.type, Order, Any] =
      endpoint.get
        .securityIn(auth.bearer[String]())
        .in("order")
        .in(path[String]("id"))
        .out(jsonBody[Order])
        .errorOut(
          oneOf(
            notFoundStatus
          )
        )

then 404 would be handled well by the client. Only adding another oneOf brings this problem.

@majk-p
Copy link
Contributor Author

majk-p commented Aug 8, 2022

That's right! The example repository is now updated to reflect that situation. As you can see in the run output below, the client properly handles the NotFound error in the case where it's defined as .errorOut(oneOf(notFoundStatus)), it only fails on 403 which is expected with such definition.

image

@adamw
Copy link
Member

adamw commented Aug 8, 2022

The problem was due to content negotiation implementation - if some of the variants specified a body (with a content-type), and some didn't there was a pre-filtering step which removed the variants without the body. Now these variants are preserved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants