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

Comma separated values only works on scala 2.13+ #3343

Open
andreazube opened this issue Nov 27, 2023 · 4 comments
Open

Comma separated values only works on scala 2.13+ #3343

andreazube opened this issue Nov 27, 2023 · 4 comments

Comments

@andreazube
Copy link
Contributor

Tapir version: 1.9.2

Hi peps!

Context

A very common use case when defining an endpoint is adding an (optional) filter of "categories".
One way of doing so is as a list of comma-separated values, like

GET /tapirs?cute=true&breed=malayan,baird

Tapir, after this discussion, provides the handy CommaSeparatedValues.
This work perfectly, and the openapi documentation is generated as one would expect in the case of enumerations (with allowedValues).

The problem

The issue with this is that CommaSeparated is defined as

type CommaSeparated[T] = Delimited[",", T]

Unfortunately, type literals are only supported in Scala 2.13+. Which means, people using previous versions of Scala can't use this feature :(

One possible solution

The most trivial solution is just to hard-code the comma in CommaSeparated, of course

  final case class CommaSeparated[T](values: List[T])

  /** Creates a codec which handles values delimited using `,`. The implicit `T`-codec is used for handling each individual value.
    *
    * Upon decoding, the string is split using the delimiter, and then decoded using the `T`-codec. Upon encoding, the values are first
    * encoded using the `T`-codec, and then combined using the delimiter.
    *
    * The codec's schema has the `explode` attribute set to `false`.
    */
  implicit def commaSeparated[T](
    implicit
    codec: Codec[String, T, CodecFormat.TextPlain]
  ): Codec[String, CommaSeparated[T], CodecFormat.TextPlain] =
    Codec.string
      .map(_.split(",").toList)(_.mkString(","))
      .mapDecode(ls => DecodeResult.sequence(ls.map(codec.decode)).map(_.toList))(_.map(codec.encode))
      .schema(codec.schema.asIterable[List].attribute(Schema.Explode.Attribute, Schema.Explode(false)))
      .map(CommaSeparated[T](_))(_.values)

  implicit def schemaForCommaSeparated[D <: String, T](
    implicit
    tSchema: Schema[T]
  ): Schema[CommaSeparated[T]] =
    tSchema.asIterable[List].map(l => Some(CommaSeparated[T](l)))(_.values).attribute(Explode.Attribute, Explode(false))

The disadvantage is that we'd not be reusing the exact same code we use for Delimited.

What to do next?

I personally see a few options

  • Do nothing code-wise, but specify that the feature is only available in Scala 2.13 in the documentation
  • Duplicate the code so that CommaSeparated is available for everyone and Delimited only for Scala 2.13+
  • Remove Delimited altogether and only keep CommaSeparated

The third option is quite extreme and causes a breaking change of course.
On the other hand, I don't see a real use case for anything other than comma (though clearly you do, so perhaps it's just inexperience on my part), and by making this more generic we're sacrificing the functionality for an interesting portion of Scala users (according to this 2022 survery, about 40% of Scala users still use 2.12 in some projects).
Do we have any kind of metric about how needed this functionality is?

Thanks for reading!

@adamw
Copy link
Member

adamw commented Nov 27, 2023

I'm aware that this is a Scala 2.13/3-only feature, and this was a conscious decision (Scala 2.12 users are about 9% of tapir's downloads).

Unfortunately breaking changes are not possible - we could only do that when bumping the major version of tapir-core. I think unless there's demand (please vote on this issue if it affects you!), the best course of action would be to define a comma-separated codec locally (probably copying much of the existing code, but hard-coding ,).

@andreazube
Copy link
Contributor Author

I'm aware that this is a Scala 2.13/3-only feature, and this was a conscious decision (Scala 2.12 users are about 9% of tapir's downloads).

Unfortunately breaking changes are not possible - we could only do that when bumping the major version of tapir-core. I think unless there's demand (please vote on this issue if it affects you!), the best course of action would be to define a comma-separated codec locally (probably copying much of the existing code, but hard-coding ,).

Yep, that's what I did indeed.

Thanks for the quick answer, then unless this really gets traction I guess it is what it is. What do you think of adding a quick note in the docs ? I could open a PR later today if you think it's a good idea

@adamw
Copy link
Member

adamw commented Nov 27, 2023

Ah yes sure, updating the docs is always good :)

@adamw
Copy link
Member

adamw commented Nov 27, 2023

thanks!

adamw pushed a commit that referenced this issue Nov 27, 2023
As mentioned on #3343, Tapir
only supports comma-separated values (out of the box, that is) on Scala
2.13+.

This small PR makes that explicit in the main documentation, as it
would've personally saved me a few minutes.

For good measure, I also added a complete runnable example. 

To be fair, the existing doc page already includes clear code, but I
thought a complete example could still be useful. Wdyt?

p.s. It's my first contribution to Tapir. It's a very small one and only
touches documentation, but still, I'd be extra careful in case I missed
something :)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants