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

Adds support to HTTP::Params.encode encode an arrays of values for a key. #7862

Merged

Conversation

rodrigopinto
Copy link
Contributor

What is this PR for?

This PR is for adding support to HTTP::Params.encode encode a key with an array of values. Like:

data = {"key" => ["a", "b"], "foo" => "bar"}

puts HTTP::Params.encode(data) # => "key=a&key=b&foo=bar"

This closes #7826

@rodrigopinto rodrigopinto changed the title [#7826] [WIP] Adds support to HTTP::Params.encode encode an arrays of values for a key. [WIP] Adds support to HTTP::Params.encode encode an arrays of values for a key. Jun 6, 2019
@rodrigopinto rodrigopinto force-pushed the suport-array-on-params-encode branch from 6947af1 to 6f1e7b8 Compare June 6, 2019 07:24
@jwoertink
Copy link
Contributor

How would slightly more complex structures look?

data = {"key" => {"subkey" => ["a", "b"], "foo" => "bar", "baz" => [["c"], ["d"]]}
puts HTTP::Params.encode(data) # => ??

Or would just say that the default lib only supports very simple structures, and all complex matters need to be handled by individual frameworks?

@oprypin
Copy link
Member

oprypin commented Jun 6, 2019

application/x-www-form-urlencoded doesn't define any such complex structures

@oprypin
Copy link
Member

oprypin commented Jun 6, 2019

So the thing is,
this already exists

data = {"key" => ["a", "b"], "foo" => ["bar"]}

puts HTTP::Params.new(data) # => "key=a&key=b&foo=bar"

https://carc.in/#/r/71jb

so why introduce another slightly different and inconsistent approach?

@oprypin
Copy link
Member

oprypin commented Jun 7, 2019

The implementation here looks good though, and actually doesn't add much complexity.

@rodrigopinto
Copy link
Contributor Author

@oprypin The problem on extending Params.new is that it expects HTTP::Params.new(raw_params : Hash(String, Array(String))) so the parameters should always be String => Array(String) and would never accept String => String.

So instead of extending the Params.new I added the behavior mentioned on the issue.

From my point of view, be too strict on only accepting values as Array, is like passing the problem to the clients that will use Params.new. I don't mind to have another implementation of the Params.new accepting HTTP::Params.new(raw_params : Hash(String, String)), but then I would change the current implementation. WDYT?

@rodrigopinto rodrigopinto force-pushed the suport-array-on-params-encode branch from 6f1e7b8 to d9a7319 Compare June 14, 2019 21:51
@rodrigopinto rodrigopinto changed the title [WIP] Adds support to HTTP::Params.encode encode an arrays of values for a key. Adds support to HTTP::Params.encode encode an arrays of values for a key. Jun 14, 2019
src/http/params.cr Outdated Show resolved Hide resolved
Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor changes, but after that I'll approve and merge.

src/http/params.cr Outdated Show resolved Hide resolved
src/http/params.cr Show resolved Hide resolved
src/http/params.cr Outdated Show resolved Hide resolved
@rodrigopinto rodrigopinto force-pushed the suport-array-on-params-encode branch from d9a7319 to c6a2b1f Compare June 24, 2019 10:56
@yxhuvud
Copy link
Contributor

yxhuvud commented Jun 24, 2019

In practice, one problem here is that there is no standardized way to do it, so people have invented a whole bunch of ways. See https://swagger.io/docs/specification/describing-parameters/#query-parameters for a list of a bunch of different options.

@rodrigopinto
Copy link
Contributor Author

@asterite Applied the CR suggestions and pushed. The red on CI is something not related.

@rodrigopinto rodrigopinto force-pushed the suport-array-on-params-encode branch from c6a2b1f to 6dcc9e6 Compare June 27, 2019 07:47
@RX14 RX14 added this to the 0.30.0 milestone Jun 27, 2019
@RX14 RX14 merged commit 3df0fd5 into crystal-lang:master Jun 27, 2019
@rodrigopinto rodrigopinto deleted the suport-array-on-params-encode branch September 5, 2019 20:49
dnamsons pushed a commit to dnamsons/crystal that referenced this pull request Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request: HTTP::Params.encode Array value support
6 participants