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

post request params broken #257

Closed
kostya opened this Issue Nov 23, 2016 · 16 comments

Comments

Projects
None yet
5 participants
@kostya

kostya commented Nov 23, 2016

seems this is after crystal update.

require "kemal"

post "/bla" do |env|
  p env.params.body["x"]?
end

Kemal.run

curl -s http://127.0.0.1:3000/bla -X POST --data 'x%3D1'
curl -s http://127.0.0.1:3000/bla -X POST --data 'x=1'

nil
nil

works in previous crystal, kemal

@sdogruyol

This comment has been minimized.

Show comment
Hide comment
@sdogruyol

sdogruyol Nov 23, 2016

Member

Now this is strange, all the specs are passing 😢

Member

sdogruyol commented Nov 23, 2016

Now this is strange, all the specs are passing 😢

@airinfection

This comment has been minimized.

Show comment
Hide comment
@airinfection

airinfection Nov 23, 2016

My mother always says "don't trust the specs".

airinfection commented Nov 23, 2016

My mother always says "don't trust the specs".

@Fusion

This comment has been minimized.

Show comment
Hide comment
@Fusion

Fusion Nov 24, 2016

I can confirm this is broken.
The only way I can retrieve these params is:

puts env.request.body.as(IO).gets_to_end

I suspect this has to do with Crystal now supporting streaming...

Fusion commented Nov 24, 2016

I can confirm this is broken.
The only way I can retrieve these params is:

puts env.request.body.as(IO).gets_to_end

I suspect this has to do with Crystal now supporting streaming...

@Fusion

This comment has been minimized.

Show comment
Hide comment
@Fusion

Fusion Nov 24, 2016

(note: for now, one can use this but really I hope we get a cleaner fix ;) )

env.request.body.as(IO).gets_to_end.split("&").each { |r| k,v=r.split("="); env.params.body[k] ||= v }

Fusion commented Nov 24, 2016

(note: for now, one can use this but really I hope we get a cleaner fix ;) )

env.request.body.as(IO).gets_to_end.split("&").each { |r| k,v=r.split("="); env.params.body[k] ||= v }

@sdogruyol sdogruyol added the bug label Nov 24, 2016

@sdogruyol sdogruyol closed this in 3b9a3f8 Nov 24, 2016

@sdogruyol

This comment has been minimized.

Show comment
Hide comment
@sdogruyol

sdogruyol Nov 24, 2016

Member

This is now fixed 👍 Thanks everyone for reporting ❤️

Member

sdogruyol commented Nov 24, 2016

This is now fixed 👍 Thanks everyone for reporting ❤️

@airinfection

This comment has been minimized.

Show comment
Hide comment
@airinfection

airinfection Nov 24, 2016

❤️S❤️E❤️R❤️D❤️A❤️R❤️ ❤️D❤️O❤️G❤️R❤️U❤️Y❤️O❤️L❤️

airinfection commented Nov 24, 2016

❤️S❤️E❤️R❤️D❤️A❤️R❤️ ❤️D❤️O❤️G❤️R❤️U❤️Y❤️O❤️L❤️

@kostya

This comment has been minimized.

Show comment
Hide comment
@kostya

kostya Nov 24, 2016

what about spec?

kostya commented Nov 24, 2016

what about spec?

@sdogruyol

This comment has been minimized.

Show comment
Hide comment
@sdogruyol

sdogruyol Nov 24, 2016

Member

@kostya the specs are correct, this was due to a behaviour change in HTTP::Request.body in Crystal std

Member

sdogruyol commented Nov 24, 2016

@kostya the specs are correct, this was due to a behaviour change in HTTP::Request.body in Crystal std

@Fusion

This comment has been minimized.

Show comment
Hide comment
@Fusion

Fusion Nov 24, 2016

I think Kostya was asking whether you are going to add a test case for params.body to the specs.

Fusion commented Nov 24, 2016

I think Kostya was asking whether you are going to add a test case for params.body to the specs.

@sdogruyol

This comment has been minimized.

Show comment
Hide comment
@Fusion

This comment has been minimized.

Show comment
Hide comment
@Fusion

Fusion Nov 24, 2016

Ooh, nice.
Question: are we definitely comfortable using gets_to_end? I mean, depending on the change in Crystal, you may, down the road, get something other than a FixedLengthContent object.

Fusion commented Nov 24, 2016

Ooh, nice.
Question: are we definitely comfortable using gets_to_end? I mean, depending on the change in Crystal, you may, down the road, get something other than a FixedLengthContent object.

@kostya

This comment has been minimized.

Show comment
Hide comment
@kostya

kostya Nov 24, 2016

this is unit test, i think better to add integration test also, run http server, and run requests. in crystal its quite easy. i do it in many projects. example in msgpack: https://github.com/benoist/msgpack-crystal/blob/master/spec/socket_spec.cr#L15

kostya commented Nov 24, 2016

this is unit test, i think better to add integration test also, run http server, and run requests. in crystal its quite easy. i do it in many projects. example in msgpack: https://github.com/benoist/msgpack-crystal/blob/master/spec/socket_spec.cr#L15

@sdogruyol

This comment has been minimized.

Show comment
Hide comment
@sdogruyol

sdogruyol Nov 24, 2016

Member

@Fusion i don't think that HTTP::Server will get another rewrite like this.

@kostya you are right. I've thought of that but not sure.

Member

sdogruyol commented Nov 24, 2016

@Fusion i don't think that HTTP::Server will get another rewrite like this.

@kostya you are right. I've thought of that but not sure.

@Fusion

This comment has been minimized.

Show comment
Hide comment
@Fusion

Fusion Nov 24, 2016

Thanks, Serdar. That was definitely a nitpick 👍

Fusion commented Nov 24, 2016

Thanks, Serdar. That was definitely a nitpick 👍

@rdp

This comment has been minimized.

Show comment
Hide comment
@rdp

rdp Oct 4, 2018

FWIW this should also work:

curl -X POST -H "Content-Type: multipart/form-data; boundary=----------------------------4ebf00fbcf09" -d $'------------------------------4ebf00fbcf09\r\nContent-Disposition: form-data; name="x"\r\n\r\ntest\r\n------------------------------4ebf00fbcf09--\r\n' http://localhost:3000/bla

However the first example and this one both fail currently...it gives empty env.params.body second example works though :)

rdp commented Oct 4, 2018

FWIW this should also work:

curl -X POST -H "Content-Type: multipart/form-data; boundary=----------------------------4ebf00fbcf09" -d $'------------------------------4ebf00fbcf09\r\nContent-Disposition: form-data; name="x"\r\n\r\ntest\r\n------------------------------4ebf00fbcf09--\r\n' http://localhost:3000/bla

However the first example and this one both fail currently...it gives empty env.params.body second example works though :)

@rdp

This comment has been minimized.

Show comment
Hide comment
@rdp

rdp Oct 6, 2018

OK after some research it appears that --data 'x%3D1' should be nil (that's a very long named key, named x=1, with no value) https://stackoverflow.com/questions/52641356/can-you-escape-the-equals-sign-in-post-data for the form-data one filed a new issue, seems unrelated.

rdp commented Oct 6, 2018

OK after some research it appears that --data 'x%3D1' should be nil (that's a very long named key, named x=1, with no value) https://stackoverflow.com/questions/52641356/can-you-escape-the-equals-sign-in-post-data for the form-data one filed a new issue, seems unrelated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment