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

properly handle DELETE endpoints #795

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

barunio
Copy link

@barunio barunio commented Jun 10, 2020

Prior to this commit, DELETE endpoints were not being documented
appropriately. Parameters for these endpoints should be documented as
part of the body, not the path.

Prior to this commit, DELETE endpoints were not being documented
appropriately. Parameters for these endpoints should be documented as
part of the body, not the path.
@barunio
Copy link
Author

barunio commented Jun 10, 2020

This should address #711

@grape-bot
Copy link

grape-bot commented Jun 10, 2020

1 Warning
⚠️ Unless you’re refactoring existing code, please update CHANGELOG.md.

Here's an example of a CHANGELOG.md entry:

* [#795](https://github.com/ruby-grape/grape-swagger/pull/795): Properly handle delete endpoints - [@barunio](https://github.com/barunio).

Generated by 🚫 danger

@coveralls
Copy link

coveralls commented Jun 10, 2020

Coverage Status

Coverage remained the same at 99.469% when pulling 0be0514 on barunio:fix_delete_endpoints into 961e7ca on ruby-grape:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 99.456% when pulling 1ed84a1 on barunio:fix_delete_endpoints into 17fe8a6 on ruby-grape:master.

@LeFnord
Copy link
Member

LeFnord commented Jun 11, 2020

Hi @barunio … nice thesis 😉 …

according to the http specification (RFC 2616)

"The DELETE method requests that the origin server delete the resource identified by the Request-URI."

is the actual behaviour the correct one, but it can be changed to "let the user choose" which behaviour she wants, also according to the spec

"This method MAY be overridden by human intervention (or other means) on the origin server."

@barunio
Copy link
Author

barunio commented Jun 11, 2020

@LeFnord I don't think the portion of the spec you're referencing is relevant to the change I'm proposing here.

The issue I am trying to address is that the json created by grape-swagger currently sets in: query for parameters passed to delete requests, and I contend that these parameters should instead be considered as part of the request body. The latest spec, RFC 7231, is ambiguous about the idea of request bodies in DELETE requests:

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.

In reality, though, all modern browsers support a request body for DELETE requests. And the common practice for modern web applications is to utilize a request body when there are parameters for these requests. So in reality parameters for DELETE requests could either be in the URL or request body, but I believe it is more sensible to assume the latter, not the former.

@LeFnord
Copy link
Member

LeFnord commented Jun 12, 2020

the target of my comment wasn't the change itself, it was the wording of your PR description, please think about it

so please implement it that way, that the default is in the path (as part of the URI, as the RFC it describes), but the user can set it to the body

besides that, the browswer ins't the point, the server is it, which have to accept it, from this point, grape don't differ between path, query or body params, that's all params 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants