domkm / silk

Routing for Clojure & ClojureScript
222 stars 13 forks source link

Multi-valued query parameter only picks up the last value #29

Open bendlas opened 6 years ago

bendlas commented 6 years ago
> (domkm.silk/decode-query "foo=Greg&bar=Linda&foo=John")
{"foo" "John", "bar" "Linda"}

Such query-strings are allowed and widely supported [1] [2]. Even ring has some (admittedly dodgy) support for this, where it would return a vector, when there are multiple values to a query key.

[1] https://developer.mozilla.org/en/docs/Web/API/URLSearchParams/getAll [2] https://google.github.io/closure-library/api/goog.Uri.QueryData.html#getValues

domkm commented 6 years ago

Interesting. Out of curiousity, what is the use-case for this? Why not use foo=Greg,John?

I'm open to this functionality but I think returning a string or a vector, depending on input, is a misfeature as it will complicate all user code. Perhaps there could be a setting to always return vectors.

bendlas commented 6 years ago

Browsers send it like this, when a form has multiple inputs with the same name, so the meta - use-case are all progressively-enhanced web-apps. A concrete use case would be a form where you can add dynamically add input rows. It's just awkward to generate names like input_1, input_2, ... when browsers support this use case perfectly well. jQuery's input[] design doesn't help much.

Admittedly, due to ring's design, we are in somewhat of a pickle here. Even pedestal seems to have copied it. And a lot of web devs seem to successfully ignore this corner. I don't think that there is an easy way out, at this point. Maybe the best way would be to have an option to expect a multi-valued parameter and to raise a condition for unexpectedly multi-valued parameters.

bendlas commented 6 years ago

Why not use foo=Greg,John?

Browsers have built-in escaping (by percent-encoding) so that we don't have to reinvent it. Amongst the thousands of implementations of your suggestion, that are bound to exist in the wild, how many do you think will correctly handle a parameter map of {:foo "Wayne, John Wayne"}? How many of those will handle {:foo "Wayne\\, John Wayne"}? How many do actually agree on an escaping/encoding scheme? Aren't we just inviting injection attacks, when we encourage users to implement this stuff by themselves?

au-phiware commented 6 years ago

FWIW cljs-ajax has a concept of vec-strategy for handling vectors in query parameters https://github.com/JulianBirch/cljs-ajax#get-specific-settings

The default is similar behaviour to the browser, and called :java, e.g.

{"foo" ["Greg" "John"], "bar" "Linda"} ;=> "foo=Greg&foo=John&bar=Linda"

There is also a :rails option, e.g.

{"foo" ["Greg" "John"], "bar" "Linda"} ;=> "foo[]=Greg&foo[]=John&bar=Linda"

(I find the names a bit odd.)

Perhaps silk could have a vec-strategy too, e.g.

(domkm.silk/decode-query "foo=Greg&bar=Linda&foo=John" :vec-strategy :dynamic) ;=> {"foo" ["Greg" "John"], "bar" "Linda"}
(domkm.silk/decode-query "foo=Greg&bar=Linda&foo=John" :vec-strategy :always) ;=> {"foo" ["Greg" "John"], "bar" ["Linda"]}
(domkm.silk/decode-query "foo=Greg&bar=Linda&foo=John" :vec-strategy :last-wins) ;=> {"foo" "John", "bar" "Linda"}
(domkm.silk/decode-query "foo=Greg&bar=Linda&foo=John" :vec-strategy :first-wins) ;=> {"foo" "Greg", "bar" "Linda"}

I think, the big question is: which do you choose as the default? :last-wins for legacy reasons, or :dynamic for sensible reasons (aligns with browser behaviour).

Alternatively, silk could adopt so-called :rails behaviour, e.g.

(domkm.silk/decode-query "foo[]=Greg&bar=Linda&foo[]=John") ;=> {"foo" ["Greg" "John"], "bar" "Linda"}
(domkm.silk/decode-query "foo[]=Greg&bar[]=Linda&foo[]=John") ;=> {"foo" ["Greg" "John"], "bar" ["Linda"]}
(domkm.silk/decode-query "foo=Greg&bar=Linda&foo=John") ;=> {"foo" "John", "bar" "Linda"}

Of course, both could be combined and the :rails behaviour could be its own :vec-strategy (but please call it something different... I'm pretty sure the rails people got the idea from somewhere else, maybe :php would be a better name, but not by much).

bendlas commented 6 years ago

Good find! How about allowing the parser to distinguish parameters, that are expected to be multi-valued? Something like:

> (domkm.silk/decode-query "foo=Greg&bar=Linda&foo=John&moo=Mary" :multi #{"foo" "moo"})
{"foo" ["Greg" "John"],
 "bar" "Linda"
 "moo" ["Mary"]}

I don't think we should attempt to support any of the conventions as part of this ticket. This is about a fundamental restriction. If it's lifted, you can do any strategy that you like, on top of it.

bendlas commented 6 years ago

I've pushed an implementation of the idea from my last comment https://github.com/bendlas/silk/commit/bfd371c230655358ebb5beb88a9544bfa867edec

I could make a PR out of this, if people like this idea

domkm commented 6 years ago

Thanks @bendlas. I'm open to a PR for this. Could you add tests and documentation?

bendlas commented 6 years ago

Sure, I'll have to do another pass, because two tests are failing. I'll get back.