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

Do not set Content-Type for DELETE requests #132

Closed
wants to merge 1 commit into from

Conversation

davidcelis
Copy link

DELETE requests, like GET requests, should not have a Content-Type
header as there will not be a request body. However, rack-test attempts
to set a Content-Type of application/x-www-form-urlencoded which messes
with some web frameworks' test suites (including my own). Instead of
trying to set a Content-Type header and request body on DELETEs, treat
them more like GET requests.

Signed-off-by: David Celis me@davidcel.is

DELETE requests, like GET requests, should not have a Content-Type
header as there will not be a request body. However, rack-test attempts
to set a Content-Type of application/x-www-form-urlencoded which messes
with some web frameworks' test suites (including my own). Instead of
trying to set a Content-Type header and request body on DELETEs, treat
them more like GET requests.

Signed-off-by: David Celis <me@davidcel.is>
@davidcelis
Copy link
Author

Hey @brynary, just a friendly bump on this. It would be nice to get this fixed merged upstream. Thanks!

perlun pushed a commit that referenced this pull request Apr 21, 2017
Thanks to davidcelis.
Implements: [PR 132](#132)
@junaruga
Copy link
Contributor

@davidcelis thank you for your PR. finally it was merged as d016695 .

@junaruga junaruga closed this Apr 26, 2017
@davidcelis davidcelis deleted the no-delete-content-type branch April 26, 2017 18:30
@davidcelis
Copy link
Author

@junaruga Thanks for letting me know! I hope this doesn't come off the wrong way, but I feel like I have to ask: why was my PR committed by two other people instead of just being merged into the project? It's not that I care too much about my profile stats, but it feels weird and a little unsavory to see my exact commit authored by people who weren't me. It makes me feel not recognized for my work/contribution, and it also makes me feel much less likely to contribute in the future.

@junaruga
Copy link
Contributor

junaruga commented Apr 26, 2017

@davidcelis I apologize to you about it. The reason is because this rack-test project had been suspended more than 2 years. People like you and me, sent PR or tried to contacted to original author. But we could not contact long time.

However recently the situation was changed.

2 leaders were appeared in this project.
1st leader had kept own folk merging many PRs like this PR to expand rack-test's life time.
2nd leader was able to contact original author, and did stand to move original repository to rack-test/rack-test. and added members. You can see this repo's URL is changed to rack-test/rack-test.

They were the light of this project. They recovered this dead project.
They are working for this project as maintainer.
I also joined from this month as maintainer.

Last time only 1 maintainer for rack-test.
But now we have at least 3 maintainers for rack-test.

We have to admit that we missed merging many PRs to master this week.
There are around 10 or 20 people like you, who disappointed for that.

I definitely write every people's name like you to History.txt file, though this is not best way.
https://github.com/rack-test/rack-test/blob/master/History.txt

I really thank you all very much.

@davidcelis
Copy link
Author

@junaruga Thank you very much for your explanation, I really appreciate it ❤️. And thank you for picking up the maintainership of this project!

@blambeau
Copy link

blambeau commented Jul 26, 2017

@davidcelis @scepticulous this pull request (well, the actual commit, in practice) breaks some of my projets when upgrading rack-test from 0.6.3 to 0.7.0. When a body is provided (which is not recommended, but actually allowed by the HTTP spec), it's not a good idea to strip the content type completely.

Any idea about how we could find a solution that works for everyone?

blambeau added a commit to enspirit/webspicy that referenced this pull request Jul 26, 2017
rack-test 0.7.0 introduces controversial changes, in particular about
body provided on DELETE requests:

rack/rack-test#132

Waiting for a solution there, we force the version of rack-test to
a version that is known for working on Enspirit projets.
@davidcelis
Copy link
Author

@blambeau Correct me if I'm wrong, but there's nothing stopping you from manually setting a Content-Type… Right? My PR should only prevent rack-test from setting it automatically

twe4ked added a commit to twe4ked/rspec-api-docs that referenced this pull request Aug 22, 2017
@barthez
Copy link
Contributor

barthez commented Nov 29, 2017

@davidcelis I have similar troubles to @blambeau. I cannot set JSON payload to DELETE requests. :params parameters are being transformed to QUERY_STRING not to input. I cannot pass :input directly because of my web framework test suit.

I'm wondering why it was implemented that way? WHy we could not just skip content type setting if the method is delete and params are empty.

@perlun
Copy link
Contributor

perlun commented Nov 30, 2017

@barthez See the discussion in #200, please add to that discussion and/or submit PRs if you feel the current behavior (which was merged in d016695) is wrong, or would like to improve on it.

I will lock this thread now, since discussions going on in closed PRs are really hard to find. Thanks!

@rack rack locked and limited conversation to collaborators Nov 30, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants