grape-swagger icon indicating copy to clipboard operation
grape-swagger copied to clipboard

properly handle DELETE endpoints

Open barunio opened this issue 4 years ago • 7 comments

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 avatar Jun 10 '20 19:06 barunio

This should address https://github.com/ruby-grape/grape-swagger/issues/711

barunio avatar Jun 10 '20 19:06 barunio

1 Warning
: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 :no_entry_sign: danger

grape-bot avatar Jun 10 '20 19:06 grape-bot

Coverage Status

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

coveralls avatar Jun 10 '20 19:06 coveralls

Coverage Status

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

coveralls avatar Jun 10 '20 19:06 coveralls

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

LeFnord avatar Jun 11 '20 14:06 LeFnord

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

barunio avatar Jun 11 '20 16:06 barunio

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 😉

LeFnord avatar Jun 12 '20 17:06 LeFnord