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

Always set CONTENT_TYPE for non-GET requests #223

Merged
merged 3 commits into from
Mar 27, 2018

Conversation

perlun
Copy link
Contributor

@perlun perlun commented Mar 9, 2018

...even when no parameters are provided (or the provided parameters are nil)

The long story:

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 Document how to use URL params with DELETE method #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 Provide a way to opt-in to a full request payload on a DELETE request #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. I plan on waiting with the merge until sometime next week because of this.

Ping @mwpastore @junaruga @scepticulous @barthez @rafaelfranca @tpltn @JoshAshby et al.


Oh, and one more thing: if you need the behavior in #132, to not have the Content-Type set for DELETE requests - please change your code to check if Content-Length is zero. If it is, don't try to parse the request body. Because of the slight pain these changes have inflicted on the community, I will refuse further PRs that tries to reintroduce that behavior; if this causes problems for you, it's unfortunately your code or app that has to change. Each version of rack-test released gets downloaded hundreds of thousands of times, so it is actually good thing if it doesn't move too fast. (but, as implied above, I'm hoping we can release a clearly-marked-as-such version 2.0 at some point with some nice improvements that do break a bit of b/c)

...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.
@rafaelfranca
Copy link

I confirm this PR fix my issue.

@JoshAshby
Copy link

This fixes things for me as well, although I originally came across these issues as a result of encountering these issues through the rspec_api_documentation gem. I am unable to get this to play nice with the latest rspec_api_docuementation (5.1.0), although version 0.7.0 works in case anyone else encounters this (otherwise follow this issue for it zipmark/rspec_api_documentation#342).

@perlun perlun merged commit 07a57b6 into master Mar 27, 2018
@perlun perlun deleted the fix/always-set-content-type branch March 27, 2018 17:55
@perlun perlun mentioned this pull request Mar 27, 2018
alex-damian-negru pushed a commit to alex-damian-negru/rack-test that referenced this pull request Apr 5, 2021
...even when no parameters are provided (or the provided parameters are `nil`)

The long story:

- rack#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 rack@d016695
- This broke `rack-test` for certain people, which was highlighted in rack#200. Arguably, the change incorporated in rack@d016695 was too brutal.
- rack#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 rack#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 rack#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants