swagger-api / swagger-editor

Swagger Editor
https://editor.swagger.io
Apache License 2.0
8.89k stars 2.25k forks source link

`ERROR: DELETE operations cannot have a requestBody` contradicts RFC 7231 Section 4.3.5 #1929

Closed stephenc closed 5 years ago

stephenc commented 5 years ago

Q&A (please complete the following information)

Content & configuration

openapi: 3.0.2  
info:
  title: I can haz delete with request body
  version: 1.x
paths:
  /test:
    delete:
      requestBody:
        required: true
        content:
          application/json:
            schema:
              type: object 
      responses:
        '204':
          description: No Content

Describe the bug you're encountering

RFC 7231 Section 4.3.5 states:

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 other words, a DELETE request may have a request body, it is up to the server to define the semantics.

Given that OpenAPI is specifically designed to specify semantics, we should not get an error message such as this:

Semantic error at paths./test.delete.requestBody
DELETE operations cannot have a requestBody.

To reproduce...

Steps to reproduce the behavior:

  1. Go to https://editor.swagger.io/
  2. Replace the editor content with the sample yaml above

Expected behavior

It shall not be an error for a DELETE request to define the semantics with respect to request bodies.

Screenshots

screenshot 2019-01-09 at 10 42 40

Additional context or thoughts

From looking at the HTTP 1.1. spec, the only request type that is forbidden from having a request body is TRACE

A client MUST NOT send a message body in a TRACE request.

Thus if the spec authors wanted to exclude request bodies from DELETE requests they would have included a MUST NOT in the DELETE section, but they didn't therefore request bodies are allowed

shockey commented 5 years ago

Hi @stephenc,

We're following OpenAPI for this validator, justified by this:

The requestBody is only supported in HTTP methods where the HTTP 1.1 specification RFC7231 has explicitly defined semantics for request bodies.

As is clear in the RFC 7231 excerpt you shared, DELETE lacks defined semantics.

In order to have this changed in Swagger Editor, a modification to the OpenAPI Specification would have to happen. As luck would have it, a ticket for this was opened this week (https://github.com/OAI/OpenAPI-Specification/issues/1801) - consider chiming in there!

On a more practical note:

Far too many people don't seem to understand that "has no defined semantics" is standards-ese for "FFS don't do this!" and not "sure, do whatever!"

@handrews, https://github.com/OAI/OpenAPI-Specification/issues/1747#issuecomment-439146861

I don't know enough to argue for or against this opinion of what the standard intended, but it seems to be in line with the opinion of the OpenAPI TSC, since DELETE bodies went from being technically allowed in 2.0 to explicitly banned in 3.0.

For more context on this, see https://github.com/swagger-api/swagger-ui/issues/4425.

shockey commented 5 years ago

Closing due to inactivity.

This is simply to keep our issue tracker clean - feel free to comment if there are any further thoughts or concerns, and we'll be happy to reopen this issue.

n2ygk commented 5 years ago

I've noted that despite the error message, the "Try it out" actually still works for an API that in fact requires a request body with DELETE. I think a warning is a good idea for this practice, but, where one knows what one is doing (e.g. trying to document a {json:api} update of a to-many relationship it would be really useful to have an option to explicitly allow this functionality.

RFC 7231 does not explicitly disallow DELETE with request body -- It just says the semantics are undefined -- so neither should OAS nor swagger-editor; The spec is meant to be use for real-world APIs, many of which do use requestBody with DELETE.

javabrett commented 5 years ago

@n2ygk

RFC 7231 does not explicitly disallow DELETE with request body -- It just says the semantics are undefined

Correct - and the OpenAPI specification says:

The requestBody is only supported in HTTP methods where the HTTP 1.1 specification RFC7231 has explicitly defined semantics for request bodies. In other cases where the HTTP spec is vague, requestBody SHALL be ignored by consumers.

So I think the request body should be ignored here. Are you saying that try-it-out not only sent the request, but processed the DELETE request body? That should be a bug.

n2ygk commented 5 years ago

Your bug is my feature

On Mon, Apr 22, 2019 at 5:21 PM Brett Randall notifications@github.com wrote:

@n2ygk https://github.com/n2ygk

RFC 7231 does not explicitly disallow DELETE with request body -- It just says the semantics are undefined

Correct - and the OpenAPI specification says https://swagger.io/specification/#operationRequestBody:

The requestBody is only supported in HTTP methods where the HTTP 1.1 specification RFC7231 has explicitly defined semantics for request bodies. In other cases where the HTTP spec is vague, requestBody SHALL be ignored by consumers.

So I think the request body should be ignored here. Are you saying that try-it-out not only sent the request, but processed the DELETE request body? That should be a bug.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/swagger-api/swagger-editor/issues/1929#issuecomment-485557123, or mute the thread https://github.com/notifications/unsubscribe-auth/ABBHS55BMLFKDBLJMWVATULPRYT7HANCNFSM4GO45EJQ .

shockey commented 5 years ago

@n2ygk:

RFC 7231 does not explicitly disallow DELETE with request body [...] so neither should OAS nor swagger-editor

Please note that OAI and Swagger Editor are not one in the same - the OpenAPI Specification has its own open governance model unrelated to us.

I encourage you to lobby the OAI to support your use case, but until and unless the OpenAPI Specification's position on this changes we can't explicitly support it.

the "Try it out" actually still works for an API that in fact requires a request body with DELETE.

Indeed! This is more of a fluke than a feature.

The Fetch API standard doesn't cite RFC 7231's restrictions and our Fetch request builder doesn't enforce the restrictions, so they work because, essentially, nothing is stopping you at runtime.

Enjoy that freedom (personally, I have no plans to change the current behavior), but we can't endorse it or promise that it will continue to work indefinitely.

it would be really useful to have an option to explicitly allow this functionality.

I understand the value of such an option from a user's perspective, but we try to be conservative with non-standard features because the entire idea of OpenAPI is a portable document that behaves consistently across tools.

Each non-standard configuration option we add to Swagger UI or Swagger Editor fractures the entire tooling ecosystem, which is a high price to pay. We prefer support for new use cases to come from the OAI recognizing the use case and building support for it into the Specification.

(edit: cc https://github.com/OAI/OpenAPI-Specification/issues/1801)

handrews commented 5 years ago

@n2ygk

Your bug is my feature

The bug is that the {json:api} people decided to ignore the guidance of RFC 7231 and exploit an intentionally undefined area in a non-standard way. I'd file a bug on their spec, although I doubt they will be receptive given the attitude towards the RFC on display in their docs.

n2ygk commented 5 years ago

@n2ygk

Your bug is my feature

The bug is that the {json:api} people decided to ignore the guidance of RFC 7231 and exploit an intentionally undefined area in a non-standard way. I'd file a bug on their spec, although I doubt they will be receptive given the attitude towards the RFC on display in their docs.

@handrews One might argue that ember data and jsonapi.org history predates RFC 7231. I'm not sure Yehuda Katz intentionally ignored guidance that hadn't been given yet. The guidance was not present in the then-current RFC 2616 and the {json:api} DELETE method is idempotent. (The "no body rule" first appeared in draft 14, April 2011, well after SproutCore 2.0 which became Ember.js had been developed).

Just sayin' that perhaps DELETE with a request body is a bad idea but I wouldn't take the leap and say people intentionally ignored guidance.

n2ygk commented 5 years ago

@shockey Thanks for the explanation. I've chimed in on https://github.com/OAI/OpenAPI-Specification/issues/1801

adjenks commented 5 years ago

@shockey Very diplomatic answer. I like it.

yuvke commented 3 years ago

@shockey If I understand correctly the discussion under the OpenAPI-Specification issue, the decision was to update the spec to allow this. Can we then update it so that the error is not shown?

cleitondfl commented 2 years ago

just need to delete the line with this error