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

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

Closed
stephenc opened this issue Jan 9, 2019 · 12 comments

Comments

@stephenc
Copy link

stephenc commented Jan 9, 2019

Q&A (please complete the following information)

  • OS: maxOS
  • Browser: chrome
  • Version: 71
  • Method of installation: https://editor.swagger.io/
  • Swagger-Editor version: unclear (would be nice if the version was shown on the UI
  • Swagger/OpenAPI version: OpenAPI 3.0

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
Copy link
Contributor

shockey commented Jan 10, 2019

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 (OAI/OpenAPI-Specification#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, OAI/OpenAPI-Specification#1747 (comment)

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 swagger-api/swagger-ui#4425.

@shockey
Copy link
Contributor

shockey commented Jan 24, 2019

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.

@shockey shockey closed this as completed Jan 24, 2019
@n2ygk
Copy link

n2ygk commented Apr 22, 2019

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
Copy link

@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
Copy link

n2ygk commented Apr 22, 2019 via email

@shockey
Copy link
Contributor

shockey commented Apr 22, 2019

@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 OAI/OpenAPI-Specification#1801)

@handrews
Copy link

@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
Copy link

n2ygk commented Apr 23, 2019

@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
Copy link

n2ygk commented Apr 23, 2019

@shockey Thanks for the explanation. I've chimed in on OAI/OpenAPI-Specification#1801

@adjenks
Copy link

adjenks commented Apr 23, 2019

@shockey Very diplomatic answer. I like it.

@yuvke
Copy link

yuvke commented Dec 21, 2020

@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
Copy link

just need to delete the line with this error

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

No branches or pull requests

8 participants