[BUG] Option of CommaSeparated has a strange behavior when no value is provided

[fmeriaux]

Tapir version: *** 1.10.10

Scala version: *** 2.13

Describe the bug

I need to have an optional list of parameters. This list comes from a query parameter that is either not present in the query at all, defined without a value, or defined with a comma-separated list of values.

Use case:

For example, let's take the parameter "sort" and "desc", which controls the sorting of an API result. Sort indicates the list of fields to be sorted, in order, and desc indicates the direction. If desc is:

1. absent from the query, then sorting is ascending.
2. present without value, then the sort is descending on all fields.
3. present with value, then sorting is ascending except on fields listed in desc.

This use case is in fact an implementation of a design proposed by Octo on the sorting of a REST API.

I then expect to have to declare something like:

 query[Option[CommaSeparated[String]]]("desc")
          .map(_.map(_.values))(values => values.map(Delimited.apply))

In reality I have to do:

 query[Option[CommaSeparated[String]]]("desc")
          .map(_.map(_.values.filterNot(_.isBlank)))(values => values.map(Delimited.apply))

What is the problem?

The problem is that in the case where the parameter is supplied without a value, I should have Some(Nil) as the decoded value, but I actually have Some(List("")). And if the codec type is not a string but an enumeration or something else, then the decoding return an error for invalid value.

For my opinion, this result is not what we can expect when we declare a query input as "option of list".

What do you think about that ?

[adamw]

I think the current design is consistent with how non-comma-separated parameters are represented. That is, if you have a query[String]("x"), and the server receives ?x=, I think it's quite natural that the value here is the empty string. I don't think there could be another design choice, in fact.

If "empty value" is a valid input it should be properly handled by the server logic - in your case, it's discarding/using a default value, in others it might be quite different.

[fmeriaux]

I agree with your example of a non-list value. It's actually quite simple, I imagine, with Tapir, to validate the String and set a minimum length if necessary.

However, in the case of an argument with a list, I don't think it's the responsibility of the logic, but of the codec. When we declare as input, via Tapir, that we want a List Option, we don't expect to have Some(List("")) as real input. If we take the case of CommaSeparated with an enum, we don't get the empty string, but a parsing error (it tried to parse the empty string as an enum).

That's why I think that managing non-blank in a codec option list string, in the comma separated, or a little earlier (I don't know tapir well enough), seems to me to make sense and be of value to the library.

This would make it possible to manage my case, but also the case with an enum, because you won't get a parsing error if you supply ?x=. If you want an error in this case, you can do it cleanly, by declaring a validator that checks that at least one value is supplied.

That's my opinion, but after all it's up to you :)

Thank you for your reply.

[adamw]

I'd say that if I declare that I want a list of comma-separated values parsed as an enum, and the user passes ?x=, that's an invalid parameter on their part? The default-value-if-empty-parameter-provided seems to be quite a special case

[fmeriaux]

This is quite a special case I agree, my opinion is that the behavior should be:

1. Option's codec should manage the presence or absence of the parameter (none if key absent, parsing attempt otherwise).

2. The list codec (comma separated in particular) manages values from a key (if present). So as it's a list, if I have an empty entry then I should have an empty list. The arguments are that for an entry "1,2,3" if the expected result is List[Int] with List(1,2,3), that for an entry "1", I have List(1), then for an empty entry "" I must have an empty List. Otherwise I have no way of materialising the empty list as input. You can see that this limits the possibilities of the logic you want to implement.

As a result, with this reasoning, if we declare Option[List[T]], I must have None if I don't fill in my parameter, Some(Nil), if my parameter is present with no entry, and Some(List(T)) if my parameter is present with at least one parseable character.

Note that this approach gives the final choice to the user: if I want the list to be empty, I have the possibility (unlike now); if I don't want the list to be empty, I add a validator to check that I have at least one occurrence.

I hope I've convinced you :)

NB: In terms of implementation, this could be, if I'm not mistaken, a modification to the codec delimited function which, before parsing the type T, checks that the string is not blank.

[adamw]

I agree with your reasoning, and indeed there's currently no way of representing empty lists. On the other hand, if we deserialise "" as Nil, then there won't be a way to represent empty entries. After all, you could have ",," which would deserialise to List("", "", ""). So "" should deserialise to List("") ;). Maybe we simply need two different codecs, for both cases?

[fmeriaux]

Yes, why not, it could also be a parameter to the current codec, the most important thing is to have control over the behaviour