allow, but discourage, requestBody for GET, HEAD, DELETE #1937
Conversation
|
This is a good change. |
|
@philsturgeon Thanks. I suppose I need to forward-port this against |
|
@n2ygk I expect there are periodic merges from master to 3.1.0-dev. |
@philsturgeon Yeah I was thinking that, except the filename is |
|
|
I'd remember the following from RFC7230 too, as HTTP intermediaries could rejejct the request.
|
OK, so I'll await for direction from the TSC on whether I should submit this against 3.1.0 instead.
D'oh! Thanks. commit coming. |
@ioggstream: I believe that's in RFC 7231 4.3.1, not 7230. I prefer not to quote too much from other references rather than just citing them as is already done in the prior sentence. |
True ;)
Agree. I think the caveat I cited is easier to understand respect I generally agree on the content. |
|
👍 latest wording looks good. Again we don't want to ban this, but we can suggest folks avoid it because spooky confusion my occur. SHOULD is the way to go. |
- We'll probably revert this eventually - Check this out: OAI/OpenAPI-Specification#1937
Input type doc - Ignore mime-types accept for GET, HEAD and DELETE operation docs - We'll probably revert this eventually - Check this out: OAI/OpenAPI-Specification#1937 - Add accepted mime-types to operation doc - Add request type filtering to readme - Add default permissive request body description to doc generation - Fix unknown type "as-is" on cli options - Move accepts type parsing to own file - Update test suite to listen based on a constant instead of explicit 'localhost' See merge request GCSBOSS/nodecaf!20
|
@darrelmiller is there a plan to merge this at some point? Just trying to clean up my various open PRs.... |
There was a problem hiding this comment.
I agree with @MikeRalphson that as this is actually changing allowed behaviour we should defer this change to 3.1
|
Hello, |
@darrelmiller Do you want me to rebase this PR against 3.1? BTW, I would make the counter-argument that it fixes a mistake in 3.0 vs. adding new functionality, as 3.0 breaks functionality from Swagger 2. So it could go in 3.0.3, thereby unblocking @rcgroup-areuin and others who are documenting pre-existing APIs -- not inventing new ones. Anyone "doing it right" would never run across this bug/feature in the spec anyway. Is there a need for another TSC meeting agenda item to move this forward to a conclusion? I see a note in the June TSC #1941 needing to follow up on this. I imagine the September TSC is coming up shortly. Also, are there meeting minutes posted somewhere? |
|
@darrelmiller @MikeRalphson should this (or the corresponding issue) be added to #2025 for 3.1 tracking? |
|
As discussed in today's TSC call, this relaxes our stance on an issue that, as Ron observed, is something v2 was more relaxed about. General consensus was to accept this PR, thank you. |
|
Bah! If I had remembered which branch this was targeted against, I would have rattled my semver stick. |
|
@MikeRalphson - you're right. This should be reverted and reapplied to 3.1 - it can't be changed in 3.0. |
#87914) ## Summary Changes `DELETE` to `POST` for _bulk_delete on the client only for a variety of reasons. According to the RFC, not all servers and proxies need to honor DELETE having a body. From: https://tools.ietf.org/html/rfc7231 ``` 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. ``` Within at least one proxy, h2o2, we have found that it does indeed change request headers which will cause NodeJS to not attach the body of a `DELETE`: hapijs/h2o2#124 Also from other communities such as OpenAPI where they debated this, they allow it but discourage it for reasons outlined there that I will not repeat here: OAI/OpenAPI-Specification#1937 Elastic Search API's and other Kibana API's use `POST` rather than `DELETE` for their bodies that are attached to `DELETE`: https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-delete-by-query.html We still support bodies in `DELETE` and `POST` but are just changing the web client to utilize `POST` moving forward. ### Checklist Reviewed and we already have unit tests and end to end tests for these use cases so we are good with just updating them. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
elastic#87914) ## Summary Changes `DELETE` to `POST` for _bulk_delete on the client only for a variety of reasons. According to the RFC, not all servers and proxies need to honor DELETE having a body. From: https://tools.ietf.org/html/rfc7231 ``` 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. ``` Within at least one proxy, h2o2, we have found that it does indeed change request headers which will cause NodeJS to not attach the body of a `DELETE`: hapijs/h2o2#124 Also from other communities such as OpenAPI where they debated this, they allow it but discourage it for reasons outlined there that I will not repeat here: OAI/OpenAPI-Specification#1937 Elastic Search API's and other Kibana API's use `POST` rather than `DELETE` for their bodies that are attached to `DELETE`: https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-delete-by-query.html We still support bodies in `DELETE` and `POST` but are just changing the web client to utilize `POST` moving forward. ### Checklist Reviewed and we already have unit tests and end to end tests for these use cases so we are good with just updating them. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
#87914) (#88509) ## Summary Changes `DELETE` to `POST` for _bulk_delete on the client only for a variety of reasons. According to the RFC, not all servers and proxies need to honor DELETE having a body. From: https://tools.ietf.org/html/rfc7231 ``` 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. ``` Within at least one proxy, h2o2, we have found that it does indeed change request headers which will cause NodeJS to not attach the body of a `DELETE`: hapijs/h2o2#124 Also from other communities such as OpenAPI where they debated this, they allow it but discourage it for reasons outlined there that I will not repeat here: OAI/OpenAPI-Specification#1937 Elastic Search API's and other Kibana API's use `POST` rather than `DELETE` for their bodies that are attached to `DELETE`: https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-delete-by-query.html We still support bodies in `DELETE` and `POST` but are just changing the web client to utilize `POST` moving forward. ### Checklist Reviewed and we already have unit tests and end to end tests for these use cases so we are good with just updating them. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
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.
Fixes #1801 per TSC discussion of 2019-05-30 which requested a PR for review.