Document how to use URL params with DELETE method #220

tpltn · 2018-03-06T06:22:14Z

perlun · 2018-03-06T14:19:50Z

Thanks for the effort @tpltn, much appreciated!

Before we merge this, could you ensure that we also have a test for DELETE with payload body? (I'm not sure we have a the moment) DELETE is an ugly HTTP method in the sense that clients act differently; some put the data in the request parameters (i.e. the query string) whereas others put them in the request body. We need to make sure we properly support both scenarios.

Thanks in advance.

tpltn · 2018-03-07T20:18:41Z

@perlun you are right: we cannot get payload body in this way. I’ll look at the env_for(uri, env) method a little bit more and try add support for both scenarios.

perlun · 2018-03-08T12:59:58Z

@perlun you are right: we cannot get payload body in this way. I’ll look at the env_for(uri, env) method a little bit more and try add support for both scenarios.

Great, thanks! I also suggest:

Please add notes to the docs/README etc for both use cases.
If possible, also add tests for both scenarios (so we ensure we don't regress either one at some point)

tpltn · 2018-03-08T18:29:17Z

RFC 7231 says:

A payload within a DELETE request message has no defined semantics;
sending a payload body on a DELETE request might cause some existing
implementations to reject the request.

This means that DELETE method can have payload body.

rack-test sets url params or payload body with method(uri, params) syntax for now. So, I think it’s impossible to set both params and body with the same syntax.

I suggest:

use delete '/?foo=bar' to set url params
use delete '/', foo: 'bar' to set payload body

perlun · 2018-03-09T13:41:05Z

I suggest:

use delete '/?foo=bar' to set url params
use delete '/', foo: 'bar' to set payload body

I'm inclined to say I agree to this. jquery/jquery#3269 seemed to have made the same decision a while ago. (years, literally)

So:

BODY should be the default
Query options can still be used, either by hardwiring it like above or if we add a nice helper method or something.

Sorry for the messy state here, going a bit back and forth. Unless someone objects very strongly, this is the direction in which I intend to steer this.

perlun

We're getting there, thanks a lot for this @tpltn. With the suggested changes I think we are close to merging.

perlun · 2018-03-09T13:42:23Z

README.md

+
+  def delete_with_url_params_and_body
+    delete '/?foo=bar', JSON.generate('foo' => 'baz')
+  end


This example is a bit messy. Using foo both for the query parameter and the body value, that would be a bit messy on the receiving end.

Could you change it to be 'baz' => 'zot' instead? That would make the example more clear.

perlun · 2018-03-09T13:52:50Z

spec/rack/test_spec.rb

+    it 'uses the provided params hash' do
+      delete '/', foo: 'bar'
+      expect(last_request.GET).to eq({})
+      expect(last_request.POST).to eq('foo' => 'bar')


For the casual reader, it might not be entirely obvious that the body here is not JSON but instead multipart/form-encoded data. Would you mind adding a note about that, or a separate test that tests the content of last_request.body.read?

@malacalypse

...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.

tpltn · 2018-03-10T06:52:51Z

@perlun thanks for the review! I fixed all the remarks you listed.

perlun · 2018-03-10T13:01:06Z

Thanks, merging this now.

@malacalypse

...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.

* restore ability to set URL params for DELETE method * make DELETE method acts as POST * fix pr remarks

@malacalypse

...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.

tpltn and others added 2 commits March 6, 2018 09:24

restore ability to set URL params for DELETE method

d39b90e

Merge branch 'master' into master

a0f11dd

tpltn and others added 2 commits March 8, 2018 21:38

make DELETE method acts as POST

d043639

Merge branch 'master' into master

089c086

perlun requested changes Mar 9, 2018

View reviewed changes

perlun mentioned this pull request Mar 9, 2018

Always set CONTENT_TYPE for non-GET requests #223

Merged

fix pr remarks

f577e62

Merge branch 'master' into master

8bde2b1

perlun changed the title ~~Restore ability to set URL params for DELETE method~~ Document how to use URL params with DELETE method Mar 10, 2018

perlun approved these changes Mar 10, 2018

View reviewed changes

perlun merged commit 575a689 into rack:master Mar 10, 2018

perlun added a commit that referenced this pull request Mar 10, 2018

History.md: Added note about #220

00dcb0d

alex-damian-negru pushed a commit to alex-damian-negru/rack-test that referenced this pull request Apr 5, 2021

Document how to use URL params with DELETE method (rack#220)

aa3dc5e

* restore ability to set URL params for DELETE method * make DELETE method acts as POST * fix pr remarks

alex-damian-negru pushed a commit to alex-damian-negru/rack-test that referenced this pull request Apr 5, 2021

History.md: Added note about rack#220

81f1e7f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document how to use URL params with DELETE method #220

Document how to use URL params with DELETE method #220

tpltn commented Mar 6, 2018

perlun commented Mar 6, 2018

tpltn commented Mar 7, 2018

perlun commented Mar 8, 2018

tpltn commented Mar 8, 2018 •

edited

Loading

perlun commented Mar 9, 2018 •

edited

Loading

perlun left a comment

perlun Mar 9, 2018

perlun Mar 9, 2018

tpltn commented Mar 10, 2018

perlun commented Mar 10, 2018

Document how to use URL params with DELETE method #220

Document how to use URL params with DELETE method #220

Conversation

tpltn commented Mar 6, 2018

perlun commented Mar 6, 2018

tpltn commented Mar 7, 2018

perlun commented Mar 8, 2018

tpltn commented Mar 8, 2018 • edited Loading

perlun commented Mar 9, 2018 • edited Loading

perlun left a comment

Choose a reason for hiding this comment

perlun Mar 9, 2018

Choose a reason for hiding this comment

perlun Mar 9, 2018

Choose a reason for hiding this comment

tpltn commented Mar 10, 2018

perlun commented Mar 10, 2018

tpltn commented Mar 8, 2018 •

edited

Loading

perlun commented Mar 9, 2018 •

edited

Loading