Provide a way to opt-in to a full request payload on a DELETE request #200
Comments
request '/', method: :delete, input: "{}"works. Found here: https://github.com/rack-test/rack-test/blob/0047c07/spec/rack/test_spec.rb#L95 |
|
Ah, that certainly does the trick in the meantime. Thanks @lanej! |
|
Seems as if this issue is already solved. If not, please reopen with the missing behaviour. |
|
@scepticulous I don't think non-collaborators can reopen issues. @mwpastore did not seem happy about closing this, so I'm reopening now. |
😄 Sorry if i over-interpreted you. I just wanted to avoid us sending out the message that we close people's issues without they feeling that the issues are really resolved.
Please suggest how the API could look to get it working and I think we can solve it. Maybe something like: delete '/foo', '{}', payload: :bodyOr perhaps someone can come up with something better? |
|
Well, for now, if env['CONTENT_TYPE'] is "application/vnd.api+json" we should definitely be putting the params in the body, since that's a spec for that content type. This is a breaking change that has really caused a lot of trouble for us as well testing JSONAPI endpoints. My suggestion for the long term is that there are "params" and there is a "body" and that if the body is set we do nothing with it other than pass it on, and if the params are set we put them either in the query string or in the body based on the method. |
|
This also caused regression in Rails apps rails/rails#30570 |
I think that makes sense. Would you like to submit a patch that makes it behave that way?
Also makes sense. Or we could just KISS and say that
Sorry for that. Bear in mind that |
|
Is |
|
Fixed in #215, closing this. (New release out: https://github.com/rack-test/rack-test/releases/tag/v0.8.3 🎉) |
|
@mwpastore You're right, I mixed things up in the rush. 😢 Sorry for that! The proper PR number is #212, it should be the proper fix here. |
...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.
...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.
...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.
As far as I can tell from reading RFCs, it is completely valid to send a full request payload (with a content-type) on a
DELETErequest. In fact, for example, the {json:api} spec depends on this functionality. The recent change to remove this capability from Rack::Test has crippled my ability to test {json:api}-conformant backends.I can almost understand making the new default
DELETEbehavior send query params without a content-type, but there should be a way to opt-in to the old behavior.The text was updated successfully, but these errors were encountered: