Do not set Content-Type if params are explicitly set to nil #212
Conversation
|
Hi @perlun, as you suggested, I've created a PR. Please review at any time convenient. |
lib/rack/test.rb
Outdated
| # merge :params with the query string | ||
| if params = env[:params] | ||
| if params = params |
There was a problem hiding this comment.
Would an if params do the same thing as this?
There was a problem hiding this comment.
Yup, just copy-paste error. Fixed!
The breaking change was introduced with d016695 that disallows to send the payload with DELETE requests. Make the request method skip setting default Content-Type header if params are explicitly set to `nil`.
|
Thanks @barthez, a great PR! I made some minor adjustments. Merging now. |
|
New release out: https://github.com/rack-test/rack-test/releases/tag/v0.8.3 🎉 |
|
This broken compatibility with Rails in POST request. Rails always pass |
Do you have some more details about this - some example code, or a GitHub issue in another repo that I can look at to investigate it further?
I must say, I'm not 100% sure on this one. If you are passing in RFC 7231 is unfortunately a bit vague in this regard:
So it's a bit open to interpretation. Looking at other tools, curling with
I would argue that this is perhaps the simpler way, to always provide the content type regardless if we have a POST body or not. Will try to put in a PR for this asap. |
You may have the request body in the I have a test for rack-test, I'll commit and push to a branch, but in Rails case the code is: We can also fix this on the Rails said always passing the I agree that we should keep defaulting to |
Thanks. According to SemVer, it's actually fine for a 0.x release - "anything can change at any time". But I agree it's not great. We will also try to reach 1.0 as soon as we can, so you can have something to rely on in a different way than a 0.x release can provide.
Yes - working on a patch/PR right now to that avail. |
...even when no parameters are provided (or the provided parameters are `nil`) The long story: - #132 changed the behavior for HTTP DELETE requests to behave like GET: put the parameters in the query string, and don't set the Content-Type. This change was merged in d016695 - This broke `rack-test` for certain people, which was highlighted in #200. Arguably, the change incorporated in d016695 was too brutal. - #212 tried to sort it out, by not setting Content-Type if params are nil, and also reverting the change to put DELETE parameters in the query string. The first part of this (params being nil) caused issues for certain people. So this PR now tries to once and for all sort this out by: - Always setting `env['CONTENT_TYPE']`, even when params are `nil`. - Adding more tests for how `CONTENT_TYPE` and `CONTENT_LENGTH` should behave; some of this does not come from us, but from Rack upstream. I encourage everyone to give this a try before we merge, to ensure we don't have to revert once more.
...even when no parameters are provided (or the provided parameters are `nil`) The long story: - #132 changed the behavior for HTTP DELETE requests to behave like GET: put the parameters in the query string, and don't set the Content-Type. This change was merged in d016695 - This broke `rack-test` for certain people, which was highlighted in #200. Arguably, the change incorporated in d016695 was too brutal. - #212 tried to sort it out, by not setting Content-Type if params are nil, and also reverting the change to put DELETE parameters in the query string. The first part of this (params being nil) caused issues for certain people. So this PR now tries to once and for all sort this out by: - Always setting `env['CONTENT_TYPE']`, even when params are `nil`. - Adding more tests for how `CONTENT_TYPE` and `CONTENT_LENGTH` should behave; some of this does not come from us, but from Rack upstream. - Settles with the discussion in #220: if you are using `DELETE` and must use query params, put them manually in there like this: `delete '/foo?bar=baz'`. Arguably not very clean, but better than changing back and forth. `params` are overloaded in rack 0.x and will be so in 1.0 also. I am thinking that we should go with the excellent suggestion provided by @malacalypse in #200 to use dedicated `body` and `params` parameters in the long run (and probably use keyword parameters instead, but that's definitely a 2.x feature since it breaks all existing usage.) For now, I think we'll live with the ugliness. I encourage everyone to give this a try before we merge, to ensure we don't have to revert once more.
|
See #223. Let's move the discussion there. |
The breaking change was introduced with d016695 that disallows to send the payload with DELETE requests.
Make the request method skip setting default Content-Type header if params are explicitly set to
nil.It solves the problem from the #200, keeping a way to achieve goal requested in #132.