add support for duplicate query param keys

[acdupont]

• Store values for duplicate keys in array
• Add two tests for parsing duplicate query param keys
  • Tests pass with same behavior as prior to adding the tests (server.jl tests sometimes hang)

[codecov-io]

Codecov Report

Merging #433 into master will decrease coverage by 0.25%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #433      +/-   ##
==========================================
- Coverage   73.59%   73.34%   -0.26%     
==========================================
  Files          34       34              
  Lines        1909     1917       +8     
==========================================
+ Hits         1405     1406       +1     
- Misses        504      511       +7

Impacted Files	Coverage Δ
src/URIs.jl	89.51% <100%> (+0.62%)	⬆️
src/StreamRequest.jl	88.46% <0%> (-3.85%)	⬇️
src/IOExtras.jl	57.14% <0%> (-3.58%)	⬇️
src/Servers.jl	56.25% <0%> (-2.68%)	⬇️
src/Streams.jl	93.02% <0%> (-0.78%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c27eb0c...b3e54c6. Read the comment docs.

[quinnj]

@acdupont, sorry for the slow review on this. I do think we should handle the case of parsing duplicate keys. I just want to make sure we have a quick discussion on the best solution here, options include:

1. return a Vector{Tuple{String, String}}, which we support in several other cases in HTTP.jl via the Pairs module; this would probably be a bit more awkward for common use-cases, because you have to use the Pairs.getkv and Pairs.setkv methods instead of the normal getindex/setindex! for Dict. This would also be breaking.
2. we could make the return type of HTTP.queryparams Dict{String, Vector{String}}, which would mean always returning a Vector{String}, even if there was only a single value for each key (the most common case). The advantage of this case is consistency when using the results of queryparams (i.e. always iterating the results of each key-value pair), but would also be breaking.
3. This approach, i.e. only returning a Vector{String} if there are duplicate keys. This is non-breaking in the non-duplicate case, which is good. But also introduces the inconsistency of not knowing whether I'll get just a String or a Vector{String} when accessing a key.

@oxinabox @omus @ararslan @randyzwitch , do you all have any opinions on the best way forward here? We could also manage the breaking situations with various plans: introduce a new function that handles the duplicate keys while leaving queryparams as-is w/ a deprecation. etc.

[oxinabox]

We should not fear breaking.
Everyone has upper-bounded their compat now.
And this is why HTTP.jl has not yet tagged version 1.
I would remove that from the consideration.

I lean towards option two.
Consistency is important.
And one can just write:

x, = params["key"]

On the other hand, some frameworks deem this various kinds of illegal,
and everything handles it differently.
https://stackoverflow.com/questions/1746507/authoritative-position-of-duplicate-http-get-query-keys

On that basis, I don't hate 3.
And can always use dispatch there.
Take advantage of julia being a dynamic language

[acdupont]

I lean towards option 2 as well. I wrote the code for this PR to have as little impact as possible, but returning the same type no matter how many parameters there are is less surprising. I would expect Option 3 to cause frustrating problems in calling code because places where one queryparams is expected, but multiple are given, would most likely result in the code breaking. I like the simpler API in accessing the results of option 2 vs option 1. Lastly, I like the synatx @oxinabox showed above in that dealing with a Vector when you expect one result is simple.

[fredrikekre]

FWIW I kinda like 3 since that is how I specify duplicate keys when passing them to HTTP.jl. But maybe the other options also roundtrips?

[clarkevans]

This is a hard one. Perhaps option #4... you have two functions. One that always returns a vector (or should it be a tuple?), the other that always returns a string. The one that always returns a string raise an exception if there there is not exactly one value; if the parameter is missing or the parameter is specified twice, it becomes an error. Alternatively, if you always return a tuple, someone could write the one that asserts a single value in user-land. I guess option #2 as x, = params["key"] supports that singularity assertion via user pattern. I don't like #3, you're often not dispatching when just reading a parameter value.

[fredrikekre]

This code has now been moved to URIs.jl. See also URIs.jl#30.