Issue #2321: Accept multiple query parameter values

[eshu]

/claim #2321

Also please check this PR: it could be a good addition to current one: eshu#1

[CLAassistant]

All committers have signed the CLA.

[codecov-commenter]

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (f2ce5cf) 64.35% compared to head (286f6de) 64.62%.

Files	Patch %	Lines
...src/main/scala/zio/http/codec/TextChunkCodec.scala	90.62%	3 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2472      +/-   ##
==========================================
+ Coverage   64.35%   64.62%   +0.27%     
==========================================
  Files         140      141       +1     
  Lines        8326     8359      +33     
  Branches     1517     1495      -22     
==========================================
+ Hits         5358     5402      +44     
+ Misses       2968     2957      -11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[eshu]

/claim #2321

[987Nabil]

I am pretty sure, that this is not necessary. We have already HttpCodec#optional that should cover this case.
Please try to replace QueryCodec.queryOpt[X](y) in the tests with QueryCodec.query[X](y).optional.
If it works, I think you can remove this constructor and TextChunkCodec.optional.
If not, please report back why it does not work.

[eshu]

Yes, but it would be better to solve all cardinality cases in the same class and do not spread it over all code base.

[987Nabil]

I like your idea, of having more detailed errors. But I still think the whole TextChunkCodec is not necessary.
I think you could just fit it in TextCodec. You could add two new methods to TextCodec, def decodeAll(values: Chunk[String]): Chunk[A] and def encodeAll(values: Chunk[A]): Chunk[String].
For Query, add a parameter cardinality: Cardinality

sealed trait Cardinality
object Cardinality {
  case object ExactyOne extends Cardinality
  case object AtLeastOne extends Cardinality
  case object Many extends Cardinality
}

And set this according to the constructors.
Then just use the two new methods in EncoderDecoder and throw an exception if cardinality and the size of the chunk do not match.

This has imho the following benefits:

• no extra class
• every query codec is just one object instead of 2
• still having better error messages, without wrapping each result of decoding a query in DecodeResult

This seems to me better in terms of maintainability and allocations during decoding.

[eshu]

You ask me to drop one class and replace with another one. :)
It won't decrease the number of classes in code and definitely increase complexity because we put more functionality in TextCodec unrelated to its main purpose.

TextCodec is good for encoding/decoding scalar values, this is it's speciality. TextChunkCodec do not have an intersection with it, and have another specialization. I believe it is good to follow to the single responcibility principle and do not pull all functionality in one class.

[987Nabil]

You are right, that my idea still have one new data structure. The difference is, there are only singletons and it is analysable. What you have right now, can't be translated into docs. I can't take a look at an instance of TextChunkCodec and know the cardinality.
Also I don't see any problem with extending TextCodc by these methods, since they can be added to the trait and would be the same for all implementations.
As well I'd argue that it is still single responsibility. The responsibility to transform text to values and values to text.
@vigoo @jdegoes do you have some opinion / other idea(s)?

[eshu]

For documentation it is possible to add a method "description" like in TextCodec or add another way of tagging. This is not an issue.

[eshu]

@987Nabil I added the method cardinality to TextChunkCodec.

I can't agree that TextChunkCodec and TextCodec have the same responsibility. TextCodec solves the scalar transformation. One string - one value. TextChunkCodec solves the issue of cardinality: check amount of values and coerce it to expected cardinality. It uses TextCodec to encode each string scalar to a typed value.

[987Nabil]

I offered my opinion. And I don't feel like this is my field of expertise for zio-http (yet). I'll neither ask you to change nor approve this PR but rather summon my fellow zio-http team members ༼ つ ◕◕ ༽つ

[jdegoes]

@eshu @987Nabil

I really do not like the idea of TextChunkCodec. Rather, I feel like every query parameter may have zero or more values (according to spec), so the base primitive for QueryCodec should in fact generate Chunk[A] rather than A.

Essentially, HttpCodec.Query should be changed from this:

private[http] final case class Query[A](name: String, textCodec: TextCodec[A], index: Int = 0)
      extends Atom[HttpCodecType.Query, A]  {

to this:

private[http] final case class Query[A](name: String, textCodec: TextCodec[A], index: Int = 0)
      extends Atom[HttpCodecType.Query, Chunk[A]]  {

and all types updated. Now, we shouldn't break backward compatibility: and, indeed, most people using query parameters will just be looking for one value, rather than zero to many. So we should expose the same set of QueryCodecs (QueryCodecs.scala) that we do today, but we can expose a more foundational one that provides access to 0 to many query parameter values.

How does this approach sound?

Please re-open if you get a chance to simplify this solution, and I will be happy to review and merge quickly.

[987Nabil]

Sounds good to me.