softwaremill / tapir

Rapid development of self-documenting APIs
https://tapir.softwaremill.com
Apache License 2.0
1.32k stars 403 forks source link

[BUG] All optional query parameters are required #3756

Closed pk1982r closed 2 months ago

pk1982r commented 2 months ago

Tapir version: 1.10.6

Scala version: 2.13.11

Query parameters declared as optional are required by the endpoint. If any parameter is omitted the endpoint returns "Invalid value for: query parameter..." error. Problematic parameters are lists of entities. Akka interpreter is used. Akka version 2.6.21, Akka http/spray/ modules in 10.2.10 version. When lists of entities are replaced by Strings issue does not occur.

What is the problem?

If you skip any "optional" parameter HTTP 400 error is returned.

curl 'https://aaaa/names?surname=111' Invalid value for: query parameter car_name (missing)

curl 'https://aaaa/names?surname=111&bike_name=123' Invalid value for: query parameter car_name (missing)

curl 'https://aaaa/names?car_name=112313&surname=111' Invalid value for: query parameter bike_name (missing)

Only query with all parameters works.

curl 'https://aaaa/names?car_name=123&surname=111&bike_name=123' carName: CarName(123)

Maybe you can provide code to reproduce the problem?

import akka.http.scaladsl.server.Route
import cats.effect.IO
import cats.effect.unsafe.implicits.global
import cats.implicits.catsSyntaxApplyOps
import org.typelevel.log4cats.slf4j.Slf4jLogger
import sttp.tapir.CodecFormat.TextPlain
import sttp.tapir.server.akkahttp.AkkaHttpServerInterpreter
import sttp.tapir.{Codec, DecodeResult, EndpointInput, endpoint, query, stringBody}

import scala.concurrent.{ExecutionContext, Future}
import scala.language.postfixOps

class NamesRoute private (
)(implicit executionContext: ExecutionContext) {

  private lazy val logger = Slf4jLogger.getLogger[IO]

  def decode[T](s: String, fromString: String => T): DecodeResult[Option[List[T]]] = s.split(",", -1).toList match {
    case Nil => DecodeResult.Value(None)
    case l   => DecodeResult.Value(Some(l.map(fromString)))
  }

  def encode[T](list: Option[List[T]], asString: T => String): String =
    list.fold("")(l => l.map(asString).mkString(","))

  case class CarName(name: String)
  case class BikeName(name: String)

  implicit val carNameCodec: Codec[String, Option[List[CarName]], TextPlain] =
    Codec.string.mapDecode(h => decode[CarName](h, CarName))(encode[CarName](_, _.name))

  implicit val stringCodec: Codec[String, Option[List[String]], TextPlain] =
    Codec.string.mapDecode(h => decode[String](h, identity))(encode[String](_, identity))

  implicit val bikeNameCodec: Codec[String, Option[List[BikeName]], TextPlain] =
    Codec.string.mapDecode(h => decode[BikeName](h, BikeName))(encode[BikeName](_, _.name))

  val identifiers: EndpointInput[
    (Option[List[CarName]], Option[List[String]], Option[List[BikeName]])
  ] =
    query[Option[List[CarName]]]("car_name")
      .and(query[Option[List[String]]]("surname"))
      .and(query[Option[List[BikeName]]]("bike_name"))

  def route: Route = {

    val names =
      endpoint.get
        .in("names")
        .in(identifiers)
        .out(stringBody)

    AkkaHttpServerInterpreter().toRoute(names.serverLogicSuccess {
      case (Some(carName), _, _)        => Future.successful(s"carName: ${carName.mkString(",")}")
      case (None, Some(surname), _)     => Future.successful(s"surname: ${surname.mkString(",")}")
      case (None, None, Some(bikeName)) => Future.successful(s"bike_name: ${bikeName.mkString(",")}")
      case params =>
        logger.error(s"improper request parameters: $params").unsafeToFuture() *>
          Future.failed(GenericError(s"improper request parameters: $params"))
    })
  }
}
adamw commented 2 months ago

This happens because you are defining custom codecs from String to your type (Option[List[CarName]] in your case, but from tapir's point of view this is just a type T):

  implicit val carNameCodec: Codec[String, Option[List[CarName]], TextPlain] =
    Codec.string.mapDecode(h => decode[CarName](h, CarName))(encode[CarName](_, _.name))

So we have a String <-> T conversion, where the base type is required (a String). If you then create a query[T] input, tapir it's trying to construct a List[String] <-> T codec, and to do that, it's using the following chain: List[String] -> get the only element (or fail) -> T.

I think defining your codec as String <-> List[CarName], and then creating the input as an optional one: query[Option[List[CarName]]] should work.

adamw commented 2 months ago

This seems to work:

package sttp.tapir.examples

import org.apache.pekko.actor.ActorSystem
import org.apache.pekko.http.scaladsl.Http
import org.apache.pekko.http.scaladsl.server.Route
import sttp.tapir.*
import sttp.tapir.CodecFormat.TextPlain
import sttp.tapir.server.pekkohttp.PekkoHttpServerInterpreter

import scala.concurrent.duration.*
import scala.concurrent.{Await, Future}
import scala.io.StdIn

object HelloWorldPekkoServer extends App {
  implicit val actorSystem: ActorSystem = ActorSystem()
  import actorSystem.dispatcher

  case class CarName(name: String)

  def decode[T](s: String, fromString: String => T): DecodeResult[List[T]] = DecodeResult.Value(s.split(",", -1).toList.map(fromString))
  def encode[T](list: List[T], asString: T => String): String = list.map(asString).mkString(",")

  implicit val carNameCodec: Codec[String, List[CarName], TextPlain] =
    Codec.string.mapDecode(h => decode[CarName](h, CarName))(encode[CarName](_, _.name))

  val helloWorld: PublicEndpoint[Option[List[CarName]], Unit, String, Any] =
    endpoint.get.in("hello").in(query[Option[List[CarName]]]("name")).out(stringBody)

  val helloWorldRoute: Route =
    PekkoHttpServerInterpreter().toRoute(helloWorld.serverLogicSuccess(cars => Future.successful(s"Got: $cars!")))

  Await.result(Http().newServerAt("localhost", 8080).bindFlow(helloWorldRoute), 1.minute)

  StdIn.readLine()
}

Test:

[~]% curl "http://localhost:8080/hello?name=x"
Got: Some(List(CarName(x)))!%
[~]% curl "http://localhost:8080/hello?name=x,y"
Got: Some(List(CarName(x), CarName(y)))!%
[~]% curl "http://localhost:8080/hello"
Got: None!%
pk1982r commented 2 months ago

First - thank you for such a fast answer. Second - works like a charm. Third - I am aware of type erasure in JVM, but not understand how removing Option from a codec helps to make a parameter optional. It's counterintuitive. Still, I can live with that as long as it works. Best regards, PK

adamw commented 2 months ago

This doesn't reach type erasure, everything is done at compiler level (where we have full type info). There are no run-time checks if something is an instance of an option:

For a query[T] input we need a Codec[List[String], T] (the parameter might appear multiple times in the url). So the compiler tries to find the appropriate instance in the implicit scope.

In your case, if you have query[Option[List[CarName]]], the compiler will try to find Codec[List[String], Option[List[CarName]]]. So it tries to look that up - it will try several available options, among them this one, which is a rule on how to construct a Codec[List[T], Option[U]] given a Codec[T, U]. Substituting our types for T and U, we reduce the problem to looking up Codec[String, List[CarName]] - which you provide in your own code base. Problem solved!

Now in your original example, you provided a Codec[String, Option[List[CarName]]. So this can only be used, if we have the String to decode (tapir can't, and doesn't inspect the exact shape of the right-hand side - it can be whatever; here it's an option, but that's completely opaque to tapir). While constructing the Codec[List[String], Option[List[CarName]]] required for the query paramter, another rule was used, to get the single element from the list, and if it's not there - report a missing element.

pk1982r commented 1 month ago

Thank you very much for such a detailed and clear response. Now I understand the Option should be only in endpoint input declaration (and why input is List not single String)!