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

Allow requestBody for the DELETE method. #1801

Closed
adjenks opened this issue Jan 8, 2019 · 23 comments
Closed

Allow requestBody for the DELETE method. #1801

adjenks opened this issue Jan 8, 2019 · 23 comments

Comments

@adjenks
Copy link

adjenks commented Jan 8, 2019

I would like the specification to allow a requestBody in the DELETE method and other methods without explicitly defined semantics.

One of the answers in this StackOverflow post states: "The spec does not explicitly forbid or discourage it, so I would tend to say it is allowed."
I would agree with that statement.

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

I think this should raise a notice, but not be unsupported and raise an error.

Addendum:

As I mentioned in a comment below, if you do choose to create a batch delete method on your API, whether or not anyone recommends it, make sure to check your cache settings and think about how these settings will interact with DELETE requests. Calling DELETE on /users with data [3,4] will not invalidate the cache for /users/3 or /users/4, so a GET request to either of them may return invalid data depending on cache settings. My comment below describes methods of mitigating this.

@keenle
Copy link

keenle commented Jan 11, 2019

The way I read the latest specification for Operation requestBody I think it does not strictly prohibit DELETE body:

The request body applicable for this operation. 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.

"... not supported ..." and "... SHALL ..." parts - no clear "cannot have" message.

It would be better to have more concrete language defining that it is allowed to put requestBody property under delete method in Open API v3 spec so existing UI tools stop showing errors when body encountered.

@handrews
Copy link
Member

@keenle UI tools are correct to show an error as the spec says that any such request SHALL be ignored. If your UI tool is trying to show you where you are doing things that don't work, it should highlight this as an error.

@adjenks regarding

"The spec does not explicitly forbid or discourage it, so I would tend to say it is allowed."

"no defined semantics" is standards-ese for "seriously, don't do this, literally anything can happen, but for some reason (probably historical weirdness or difficulty of reliable enforcement) we can't outright forbid it"

Anytime you see "no defined semantics" or "the behavior is undefined" in an RFC, you should stay far away from it.

@keenle
Copy link

keenle commented Jan 11, 2019

@handrews , agreed on UI behavior in response to the current definition of requestBody in Open API specification. My ask is rather to the specification to allow requestBody as RFC-7231 does not prohibit the payload with DELETE request:

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.

I see the point in your suggestion to stay away from undefined stuff, though there are live services out there which accept payload with DELETE. The approach does make sense in certain scenarios which are out of the scope of this issue.

@adjenks
Copy link
Author

adjenks commented Jan 12, 2019

Users may be documenting an existing legacy API that cannot change, so there may be a DELETE method requiring a body. I understand it's not recommended, but because behavior is "undefined" in the RFC, I think it should be declared as "undefined" in OpenAPI, not "unsupported". Other methods explicitly prohibit the use of a body such as TRACE:

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

So I believe the same distinction should be made in OpenAPI.

@darrelmiller
Copy link
Member

@adjenks While I understand the desire to be able to describe existing APIs that have less than ideal behavior, we have to balance that with opening the door for more people to create new APIs that make this same mistake because we allow it.

I don't know why the HTTP spec is explicit about the request body being not allowed on some methods and not on others.

@handrews
Copy link
Member

I don't know why the HTTP spec is explicit about the request body being not allowed on some methods and not on others.

RFC 2616 didn't talk about request bodies for GET, DELETE, or TRACE at all, so I assume when they were working on RFC 7231 they locked things down as much as they could get away with.

RFC 2616 doesn't specifically allow them, it was just pretty light on details of method semantics in general. Part of the reason for the 723x RFCs was to improve that situation.

@notEthan
Copy link
Contributor

@darrelmiller

While I understand the desire to be able to describe existing APIs that have less than ideal behavior, we have to balance that with opening the door for more people to create new APIs that make this same mistake because we allow it.

I wouldn't expect people to creating APIs to constrain themselves by OpenAPI. when I create an API, first comes the needed functionality, and documenting it with OpenAPI or whatever tool I am using, that comes after. I wouldn't worry about "opening the door" for unencouraged behavior as that's not really OpenAPI's door to hold.

I'm using OpenAPI to document an API that does use delete with a request body, and is not going to change. seems to me I should be able to describe this API with a valid OpenAPI document.

@n2ygk
Copy link
Contributor

n2ygk commented Apr 22, 2019

{json:api} explicitly defines a DELETE request body for updating to-many relationships and it has quite well-defined semantics: Remove only the supplied set of resource identifier objects from the (potentially larger) full list of related resource identifier objects.

I am unable to fully represent a {json:api} schema with OAS if it explicitly prohibits what RFC 7231 allows (as undefined but not prohibited). Please (re)consider allowing a request body for DELETE.

@adjenks
Copy link
Author

adjenks commented Apr 23, 2019

If one does want to support a request body in their API, they must consider the caching consequences. As stated in RFC7234:

A cache MUST invalidate the effective Request URI (Section 5.5 of
[RFC7230]) as well as the URI(s) in the Location and Content-Location
response header fields (if present) when a non-error status code is
received in response to an unsafe request method.

So, using the example in {json:api}:

DELETE /articles/1/relationships/comments HTTP/1.1
Content-Type: application/vnd.api+json
Accept: application/vnd.api+json

{
  "data": [
    { "type": "comments", "id": "12" },
    { "type": "comments", "id": "13" }
  ]
}

The cache for the resource /articles/1/relationships/comments will be cleared. Imagine this sequence of events:

  1. User-agent requests GET /articles/1/relationships/comments
  2. Server returns 1000 comments and tells the user-agent (hypothetically) to cache it.
  3. User-agent requests GET /articles/1/relationships/comments/12
  4. Server returns 1 comment and tells the user-agent (hypothetically) to cache it.
  5. The DELETE request from the above example is made, it deletes objects 12 and 13 and returns Status 200.
  6. The browser interprets this as having deleted the resource at /articles/1/relationships/comments and it clears the cache for this path.
  7. User-agent requests GET /articles/1/relationships/comments again.
  8. Browser fetches a fresh 998 comments because cache was cleared. This is a good thing.
  9. User-agent requests GET /articles/1/relationships/comments/12 again.
  10. Browser pulls cached resource from steps 3&4. This is not so great.

It's a good thing that the cache was cleared for the main resource, since you don't want it to use the cached 1000 comments when there are only 998. So having it invalidate the base resource is good. However, user-agents will not invalidate /articles/1/relationships/comments/12 and /articles/1/relationships/comments/13 when requesting the batch delete resource. Therefore fetching a single comment (step 9) may return invalid results (10) if they were previously requested (3), cached, deleted via a batch request, and requested again, since the cache may be used.

Overall, if you do choose to create a batch delete method on your API, whether or not anyone recommends it, make sure to check your cache settings and think about how these settings will interact with DELETE requests.

To mitigate this you could disable caching of individual resources, but the client would then have to fetch every time. Using standard HTTP/1 you can't invalidate arbitrary caches, so deleting 12, and 13 in a batch wouldn't clear their cache, as mentioned before, unless you HTTP/2 server push which allows you to send arbitrary other resources along with the response for another request, so your response to a batch delete could pack with it the invalidation of those two resources.

There's a lot to consider here. I do think that it would be great to support a batch delete operation in multiple standards and protocols, but it needs to be thoroughly discussed and everyone would have to cooperate on the appropriate course of action.

It is also interesting to consider that using a custom method, which is the commonly recommended way to perform a batch delete request, such as POST /comments/deleteComments, or /comments:deleteComments, will also not invalidate any caches associated with the resources that your request intended to delete. So a subsequent request to GET /comments/5 after it has been deleted via a batch method may fetch stale and invalid results.

@keenle
Copy link

keenle commented May 6, 2019

Tiny fact that is missing from this discussion and totally worth mentioning since it can be an obstacle on a path of upgrading from Swagger v2 to Open API v3 is that with Swagger v2 one can declare payload for DELETE method.

Swagger UI can render DELETE endpoint with payload.

@mikaeldanielsson
Copy link

This is a gentle bump to see if you are any closer to deciding on this issue?

We are in the process of migrating from Swagger v2 to Open API v3, we have some services that provides DELETE endpoints that currently rely on a request body. This is currently stopping us from fully adapting Open API v3 which has a lot of benefits over v2 for us.

Is there anything we can do from our end to help accelerate and resolve this issue?

@darrelmiller
Copy link
Member

@mikaeldanielsson I added it to the TSC agenda for Thursday.

@n2ygk
Copy link
Contributor

n2ygk commented May 28, 2019

@mikaeldanielsson According to my reading of https://www.openapis.org/participate/how-to-contribute this needs to become a "next proposal" which only the TSC members can label as such.

Am I correct @darrelmiller that this is the formal process for bringing issues to the TSC? (I "picked on" you since you appear to be the main committer of much of the TSC process documentation; Apologies if you are not the best contact for this and please do indicate the appropriate mechanism).

Does the TSC need a more formally-written request to consider this issue? At this point it appears to be:

  1. A requirement for backward-compatibility with Swagger 2 per @keenle
  2. A requirement for certain pre-existing API frameworks including https://jsonapi.org
  3. Not explicitly disallowed ("MUST NOT", "SHALL NOT" nor even "NOT RECOMMENDED" or "SHOULD NOT") by RFC 7231.
  4. Was not explicitly disallowed in RAML 1.0 which was also an input to OAS 3 (@usarid ?)

Language in the OAS could certainly be written to warn against use of the requestBody with DELETE.

I would be happy to collaborate on writing a more formal request or spec document PR.

@darrelmiller
Copy link
Member

As this is not a new feature, nor one that requires significant updates to the spec, I think a PR would be sufficient. We will discuss on Thursday's meeting and see if we think this is a fix that should be made. If we get consensus there then a PR would be gratefully accepted.

@n2ygk
Copy link
Contributor

n2ygk commented May 30, 2019

@darrelmiller per the TSC call, I'll begin working on a PR.

Is v3.0.3-dev the correct branch to base the PR on?

@n2ygk
Copy link
Contributor

n2ygk commented Jun 21, 2019

See #1929 (TSC Open Meeting of 2019-05-30) where this was discussed.

remmeier pushed a commit to crnk-project/crnk-framework that referenced this issue Oct 6, 2019
This introduces an **experimental** crnk-gen-openapi module for generating OpenApi v3 specification from crnk projects.

Generated openapi.yaml output can be inspected in the online swagger editor: http://editor.swagger.io/

The **experimental** label will come off as the generator reaches maturity. Currently the known remaining work includes:

- Support for more overrides during build (All Info / Servers)
- Ensuring that when starting with a template, all template data is merged onto the generated code (currently only has partial support)
- Support for openapi annotations to override generated code and add constraints/patterns on resource attributes
- Support for the Plain JSON flavour of crnk
- Support for Bulk Operations / JSON Patch
- Support for json output and other arguments to the output format / options
- csv parameters should set the `style` attribute to something like:
```
style:
	format: "form"
	explode: false
```
- fields[<associated_resource_type>] for every resource that has associated resources
- Support for Delete request bodies (Currently OpenAPI does not allow DELETE methods to define a RequestBody - OAI/OpenAPI-Specification#1801)
- Make use of re-usable parameter groups when https://github.com/OAI/OpenAPI-Specification/issues/445 lands
@gliderkite
Copy link

Is there any update for this? Thanks.

@webron
Copy link
Member

webron commented Jan 23, 2020

See #1937.

@handrews
Copy link
Member

handrews commented Feb 9, 2020

The issues/PRs mentioned here have been merged or are in progress and tracked in more current meeting agendas.

@handrews handrews closed this as completed Feb 9, 2020
@handrews
Copy link
Member

handrews commented Feb 9, 2020

Oops too many tabs open, closed the wrong one, sorry!

@handrews handrews reopened this Feb 9, 2020
@handrews
Copy link
Member

handrews commented Feb 9, 2020

Actually, looks like this issue's PR was also merged so I should have just left it closed 😆

@handrews handrews closed this as completed Feb 9, 2020
sambacha pushed a commit to manifoldfinance/open-sushi that referenced this issue Feb 28, 2021
github-actions bot pushed a commit to manifoldfinance/open-sushi that referenced this issue Feb 28, 2021
paulsturgess added a commit to apiaframework/apia-openapi that referenced this issue Nov 7, 2023
This gem allows us to generate an [OpenAPI schema](https://www.openapis.org/) of an [Apia API](https://github.com/krystal/apia).

## Why are we using v3.0.0 when the latest is v.3.1.0 ?

The [OpenAPI generator](https://openapi-generator.tech) does not support 3.1.0 (at least for Ruby yet).

So the specification is for version 3.0.0. Annoyingly in v3.0.0, having a request body against a DELETE is deemed to be an error. And this shows up in [swagger-editor](https://editor.swagger.io/). However, after [community pressure](OAI/OpenAPI-Specification#1801), this decision was reversed and in [version 3.1.0 DELETE requests are now allowed to have a request body](OAI/OpenAPI-Specification#1937). 

I have successfully used the Ruby client library to use a DELETE request with a v3.0.0 schema, so I don't think it's a big deal. We can bump to 3.1.0 when the tooling is ready.

## What is implemented?

- All endpoints are described by the spec.
- ArgumentSet lookups with multiple methods of supplying params are handled
- All the various "non-standard" Apia data types are mapped to OpenAPI ones (e.g. decimal, unix)
- If `include` is declared on a field for partial object properties, then the endpoint response will accurately reflect that
- Array params for get requests work in the "rails way". e.g. `user_ids[]=1,user_ids[]=2`
- [swagger-editor](https://editor.swagger.io/) works, so we can use the "try it out" feature (including bearer auth)
- Routes that exclude themselves from the Apia schema are excluded from the OpenAPI output
- Endpoints are converted into "nice" names so that the generated client code is more pleasant to use
- Apia types (enums, objects, argument sets, polymorphs) are implemented as re-usable component schemas
- The spec is good enough to generate [client libraries in various programming languages](https://github.com/krystal/katapult-openapi-clients)

## What isn't implemented?

- Only the "happy path" response is included in the spec, we need to add error responses
- There are places in the spec where we can explicitly mark things as "required" and this has not been implemented everywhere.
- Perhaps we can improve how ArgumentSet lookups are declared – currently [swagger-editor](https://editor.swagger.io/) allows both params (e.g. id and permalink) to be sent in the request which triggers an error.
- We can improve the accuracy of the [data types](https://swagger.io/docs/specification/data-models/data-types/#numbers) by declaring the `format`. This is not implemented.
- There's one specification test that simply asserts against a static json file generated from the example app. Perhaps we could try actually validating it with something like https://github.com/kevindew/openapi3_parser
- Might be nice to dynamically determine the API version
- The example app needs expanding to ensure all code-paths are triggered in the generation of the schema

## Any other known issues?
- We can't have deeply nested objects in GET request query params. This just isn't defined by the OpenAPI spec. [There's GitHub issue about it](OAI/OpenAPI-Specification#1706). I don't believe we can do much here and probably we don't need to.
- File uploads are not implemented, but I don't think we have a need for that.
- We do not try to be too 'clever' when the endpoint field uses include to customize the properties returned. e.g. `include: '*,target[id,name]'` in this case we could actually use a `$ref` for the object referenced by the `*`. But instead, if a field uses `include` we just declare an inline schema for that field for that endpoint.
- The example API has been copied and expanded from the apia repo. Some of the additional arguments and ways the objects have been expanded is nonsense, but they're there just to ensure we execute all the code paths in the schema generation. Maybe we could come up with a better example API or perhaps we just don't worry about it.
@dmyersturnbull
Copy link

RFC 9110 was proposed in 2018 and accepted in 2022. Its language is similar.

@adjenks
Copy link
Author

adjenks commented Oct 9, 2024

Thanks for the news @dmyersturnbull

This part seems to indicate that content is still okay for clients if the server specifies that content would be supported:

A client SHOULD NOT generate content in a DELETE request unless it is made directly to an origin server that has previously indicated, in or out of band, that such a request has a purpose and will be adequately supported.

, although this part seems to indicate that servers should avoid doing it because "intermediaries" might mess things up:

An origin server SHOULD NOT rely on private agreements to receive content, since participants in HTTP communication are often unaware of intermediaries along the request chain.

So, those two sentences seem to be slightly contradictory, but they use the words "SHOULD" instead of "MUST", so it's not recommended to put any content in a DELETE, or accept any content in a DELETE, but it's still permitted either way.

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

10 participants