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

Request bodies moved out of parameters #670

Merged
merged 2 commits into from
Jun 6, 2016
Merged

Conversation

jharmn
Copy link
Contributor

@jharmn jharmn commented Apr 25, 2016

Problem description

Request bodies are currently specified in parameters as in: body. This has been addressed as an incongruence with other parameters throughout a variety of issues in meta #565, as well as discussions with tooling vendors, as well as many exceptions made for request bodies throughout the spec.

A number of issues arise from treating request body like other parameter locations (path, query, header) :

  • name is meaningless in body, and is required. To underscore this, from the current spec (I would also argue this serves no documentation purpose):

The name of the body parameter has no effect on the parameter itself and is used for documentation purposes only

  • Body parameters must be unique in the list of parameters. This currently has to be enforced/validated by tooling, as the array structure does not currently create any natural constraint. From Path Item Object

There can be one "body" parameter at most.

  • In trying to add serialization schemes for complex non-body parameters (in added schema for all parameters #654), some trouble arises.
    • Request body is usually a media type like application/json, application/xml
    • Other parameters are typically either some sort of collection, and infrequently json, xml, etc.
  • In adding examples to the body parameter (Add examples object #636), the examples aren’t really applicable to other parameter types.
  • Throughout the spec, there are exceptions made for the inconsistency in the body parameter vs the other parameter types.

Proposed solution

  • Move body parameter into it’s own key, peer-level to parameters.
  • Simplify the request body object (vs parameter object), to remove name, in, required, and deprecated.
  • Eliminate the use of examples in parameters (as media types are largely irrelevant). This was already unclear in the spec, and probably needs revision in another PR (as mentioned in Add examples object #636).

@ePaul
Copy link
Contributor

ePaul commented Apr 25, 2016

  • Eliminate the use of examples in parameters (as media types are largely irrelevant). This was already unclear in the spec, and probably needs revision in another PR (as mentioned in Add examples object #636).

While I think media types need separate considerations (see #146), I think the plural examples is still useful, i.e. the ability to give multiple examples. (Though this could live inside the schema object, not at the parameter level.)

@IvanGoncharov
Copy link
Contributor

IvanGoncharov commented Apr 25, 2016

@jasonh-n-austin Cool PR 👍 definitely make spec less confusing.
But I disagree with removing required and deprecated.

You can make POST requests without body parameter, so you should have the ability to write required: false. For details, see this disscussion.

If you use the same schema to describe multiple body parameters, how you will deprecate only one of them? So body need it's own deprecate property.

@@ -346,7 +346,8 @@ Field Name | Type | Description
<a name="pathItemHost"></a>host | `string` | The host (name or ip) serving the path. This optional value will override the top-level [host](#oasHost) if present. This MUST be the host only and does not include the scheme nor sub-paths. It MAY include a port. If the `host` is not included, the host serving the documentation is to be used (including the port). The `host` does not support [path templating](#pathTemplating).
<a name="pathItemBasePath"></a>basePath | `string` | The base path on which the API is served, which is relative to the [`host`](#pathItemHost). This optional value will override the top-level [basePath](#oasBasePath) if present. If it is not included, the API is served directly under the `host`. The value MUST start with a leading slash (`/`). The `basePath` does not support [path templating](#pathTemplating).
<a name="pathItemSchemes"></a>schemes | [`string`] | The transfer protocol of the API. Values MUST be from the list: `"http"`, `"https"`, `"ws"`, `"wss"`. This optional value will override the top-level [schemes](#oasSchemes) if present. If the `schemes` is not included, the default scheme to be used is the one used to access the OpenAPI definition itself.
<a name="pathItemParameters"></a>parameters | [[Parameter Object](#parameterObject) <span>&#124;</span> [Reference Object](#referenceObject)] | A list of parameters that are applicable for all the operations described under this path. These parameters can be overridden at the operation level, but cannot be removed there. The list MUST NOT include duplicated parameters. A unique parameter is defined by a combination of a [name](#parameterName) and [location](#parameterIn). The list can use the [Reference Object](#referenceObject) to link to parameters that are defined at the [OpenAPI Object's parameters](#oasParameters). There can be one "body" parameter at most.
<a name="pathItemParameters"></a>parameters | [[Parameter Object](#parameterObject) <span>&#124;</span> [Reference Object](#referenceObject)] | A list of parameters that are applicable for all the operations described under this path. These parameters can be overridden at the operation level, but cannot be removed there. The list MUST NOT include duplicated parameters. A unique parameter is defined by a combination of a [name](#parameterName) and [location](#parameterIn). The list can use the [Reference Object](#referenceObject) to link to parameters that are defined at the [OpenAPI Object's parameters](#oasParameters).
<a name="pathItemRequestBody"></a>requestBody | [[Request Body Object](#requestBodyObject) <span>&#124;</span> [Reference Object](#referenceObject)] | The request body applicable for all the operations described under this path. This request body can be overriden at the operation level, but cannot be removed there.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I would omit it at the path item level, and have it only at the operation level.

I can't think of a single request body definition which would be plausibly the same for multiple, or even all, operations on the same path.

Copy link
Contributor Author

@jharmn jharmn Apr 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's super common, but I could see a case where the same type is used on multiple operations in the same path, with some exceptions. I'm not super tied to it, but since v2.0 has the concept for parameters, it seemed fair to keep it in splitting out requestBody.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding to the people above and @whitlockjc's comment on the main thread - this just doesn't seem like a real case we need to handle. The idea behind parameters being at the POI was that people won't need to repeat common parameters - this makes sense for path parameters, query parameters and obviously header parameters. This doesn't make sense as much for payloads (form and body). You could argue there's a case where a single body can be used for both POST and PUT, and that a specific path doesn't have a GET/DELETE/... calls as well. So yes, but it feels like the edge of the edge.

Does adding support to it cost much? Probably not. Will it be used by enough APIs to justify it? Probably even less. So in favor of simplifying the tooling and considering there's a very easy alternative, I'd suggest we drop this from the POI.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I'll drop it.

@ePaul
Copy link
Contributor

ePaul commented Apr 25, 2016

Deprecating a request body is only sensible when the operation previously had a body parameter, but in the future should be called without one. (Otherwise you would deprecate the whole operation.)

I guess this happens, but not that often, so I'm ambivalent if it is necessary.

Which might be happening more often would be passing a different body parameter (maybe with a different Content-Type), and then the old one will be deprecated. Enabling this at all belongs to #146, though. I guess a solution to that will also cover the case of "no body parameter".


Describes a single request body.

If the `requestBody` attribute is not present, or `schema` is not defined, the request body is not required.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@IvanGoncharov this concept is meant to preclude the need for required. If no request body is needed, then one should not be specified. You could also specify a schema with nothing specified and get close the same net effect.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get it. If a requestBody is defined, how can it not have schema? Seems like schema needs to be required in this case.

Not sure we can avoid required here. To me, not stating a requestBody doesn't make it optional, it makes it 'ignored', 'unaccepted'.

We can argue whether there are cases that a payload can be optional, question is do we really want to not allow specifying it. We can make it implicitly required allowing people to explicitly set it as not-required if needed, covering most cases and reducing bloat.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding required back in.

I'm specifying that it defaults to true, to avoid having to specify required: true for every request body (when that is probably the 90+% case).

@jharmn
Copy link
Contributor Author

jharmn commented Apr 28, 2016

@IvanGoncharov I think @ePaul got it right here. In the case of an operation with a request body, the entire operation would be deprecated, but a request body would be very unlikely to be deprecated.

I intentionally didn't tackle multiple request bodies in this PR, to try and take a bite sized chunk of the complexity. Perhaps when we consider content-type-specific requests the deprecated issue comes back into focus.

On multiple request bodies (to be addressed in a future PR), to emphasize why it's not in here:
The additional complexity here is that the current structure uses one schema (the OpenAPI subset of JSON Schema) to describe all media types (which is, in itself, an issue), and examples are delineated by content-type.
If multiple request bodies (by content-type) are specified, then examples must be modified to unlink them from content-type.

@@ -445,7 +446,8 @@ Field Name | Type | Description
<a name="operationId"></a>operationId | `string` | Unique string used to identify the operation. The id MUST be unique among all operations described in the API. Tools and libraries MAY use the operationId to uniquely identify an operation, therefore, it is recommended to follow common programming naming conventions.
<a name="operationConsumes"></a>consumes | [`string`] | A list of MIME types the operation can consume. This overrides the [`consumes`](#oasConsumes) definition at the OpenAPI Object. An empty value MAY be used to clear the global definition. Value MUST be as described under [Mime Types](#mimeTypes).
<a name="operationProduces"></a>produces | [`string`] | A list of MIME types the operation can produce. This overrides the [`produces`](#oasProduces) definition at the OpenAPI Object. An empty value MAY be used to clear the global definition. Value MUST be as described under [Mime Types](#mimeTypes).
<a name="operationParameters"></a>parameters | [[Parameter Object](#parameterObject) <span>&#124;</span> [Reference Object](#referenceObject)] | A list of parameters that are applicable for this operation. If a parameter is already defined at the [Path Item](#pathItemParameters), the new definition will override it, but can never remove it. The list MUST NOT include duplicated parameters. A unique parameter is defined by a combination of a [name](#parameterName) and [location](#parameterIn). The list can use the [Reference Object](#referenceObject) to link to parameters that are defined at the [OpenAPI Object's parameters](#oasParameters). There can be one "body" parameter at most.
<a name="operationParameters"></a>parameters | [[Parameter Object](#parameterObject) <span>&#124;</span> [Reference Object](#referenceObject)] | A list of parameters that are applicable for this operation. If a parameter is already defined at the [Path Item](#pathItemParameters), the new definition will override it, but can never remove it. The list MUST NOT include duplicated parameters. A unique parameter is defined by a combination of a [name](#parameterName) and [location](#parameterIn). The list can use the [Reference Object](#referenceObject) to link to parameters that are defined at the [OpenAPI Object's parameters](#oasParameters).
<a name="operationRequestBody"></a>body | [[Request Body Object](#requestBodyObject) <span>&#124;</span> [Reference Object](#referenceObject)] | The request body applicable for this operation. If a request body is already defined at the [Path Item](#pathItemParameters), the new definition will override it, but can never remove it.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be requestBody instead of body.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

body should be requestBody here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we move formData parameters to the requestBody as well (and maybe even if not), perhaps a more suitable name would be payload (not a big fan of mixed-case names, body may be too obscure to some).

@whitlockjc
Copy link
Member

My personal opinion is that having a path-level requestBody is not all that useful. Sure, there are some cases where the request body will have the same schema across all operations with a body but it could easily be confusing for operations where that isn't the case. I would much rather reuse things using references much like we do elsewhere using a $ref and so specifying a request body would be opt-in instead of opt-out in this case. I realize the thinking behind this was in the same vein around the ability now to set body parameters at the path-level but I'm not sure how useful even that is.

Would this require a global requests or requestBodies object to store the reusable schemas?

* Path - Used together with [Path Templating](#pathTemplating), where the parameter value is actually part of the operation's URL. This does not include the host or base path of the API. For example, in `/items/{itemId}`, the path parameter is `itemId`.
* Query - Parameters that are appended to the URL. For example, in `/items?id=###`, the query parameter is `id`.
* Header - Custom headers that are expected as part of the request.
* Body - The payload that's appended to the HTTP request. Since there can only be one payload, there can only be *one* body parameter. The name of the body parameter has no effect on the parameter itself and is used for documentation purposes only. Since Form parameters are also in the payload, body and form parameters cannot exist together for the same operation.
* Form - Used to describe the payload of an HTTP request when either `application/x-www-form-urlencoded`, `multipart/form-data` or both are used as the content type of the request (in the OpenAPI Specification's definition, the [`consumes`](#operationConsumes) property of an operation). This is the only parameter type that can be used to send files, thus supporting the `file` type. Since form parameters are sent in the payload, they cannot be declared together with a body parameter for the same operation. Form parameters have a different format based on the content-type used (for further details, consult http://www.w3.org/TR/html401/interact/forms.html#h-17.13.4):
* `application/x-www-form-urlencoded` - Similar to the format of Query parameters but as a payload. For example, `foo=1&bar=swagger` - both `foo` and `bar` are form parameters. This is normally used for simple parameters that are being transferred.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@webron to look at how to deal with form urlencoded parameters.

@webron
Copy link
Member

webron commented Apr 29, 2016

This is a good proposal. I'll add my comments to specific sections tackling the proposal as-is, then go over the formData parameters.

* Path - Used together with [Path Templating](#pathTemplating), where the parameter value is actually part of the operation's URL. This does not include the host or base path of the API. For example, in `/items/{itemId}`, the path parameter is `itemId`.
* Query - Parameters that are appended to the URL. For example, in `/items?id=###`, the query parameter is `id`.
* Header - Custom headers that are expected as part of the request.
* Body - The payload that's appended to the HTTP request. Since there can only be one payload, there can only be *one* body parameter. The name of the body parameter has no effect on the parameter itself and is used for documentation purposes only. Since Form parameters are also in the payload, body and form parameters cannot exist together for the same operation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this description removes the restriction of using body parameters alongside formData parameters (which just can't happen). We either need to add it (probably in both places) or discuss moving formData parameters to the requestBody as well.

@webron
Copy link
Member

webron commented Apr 29, 2016

Re examples - we may need to revisit it when we handle the content negotiation ticket.
Not sure how/why this changes the example for regular parameters though.

Re required - added my comments. Not sure we can avoid it.

Re deprecated - I tend to agree that if the body is deprecated, then the operation is deprecated. An alternative (and followup to above) to change it as marked to not being required - that should cover it.

@whitlockjc raises an important issue regarding reusable components - since requestBody is no longer a parameter, do we need to allow it under the new components construct? Not sure we can avoid it.

@jharmn
Copy link
Contributor Author

jharmn commented May 6, 2016

  • Fixed a typo and refined some language.
  • Added required
  • Removed path-level body

Now to look at formData.

@webron
Copy link
Member

webron commented May 20, 2016

I'm trying to collect my thoughts around formData - my take is that we should try eliminating it and converging it with the requestBody - there are a few gotchas there but combining this PR with #654 allows us to solve many issues regarding formData (validation in conjunction with requestBody, allowing multiple file uploads, setting types for files and so on).

Hate bringing it up but regarding the name, are we set on requestBody? Was thinking of just payload, unless we're set on having the word request as part of it.

@webron
Copy link
Member

webron commented May 20, 2016

Ugh, forgot to say, I think this can be merged, and I'll tackle the formData in a separate PR.

@wparad
Copy link

wparad commented May 20, 2016

2 cents, please help rid the whole of formdata, either body or payload.

@webron
Copy link
Member

webron commented Jun 6, 2016

This also resolved #278.

Field Name | Type | Description
---|:---:|---
<a name="requestBodyDescription"></a>description | `string` | A brief description of the request body. This could contain examples of use. [GFM syntax](https://help.github.com/articles/github-flavored-markdown) can be used for rich text representation.
<a name="requestBodySchema"></a>schema | [Schema Object](#schemaObject) | The schema defining the type used for the request body.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed this - shouldn't the schema field be required? What does a request body mean without a schema?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of our APIs use AtomPub-style "media resources", see https://tools.ietf.org/html/rfc5023#section-9.6. In this case the request body of a POST can have any media type, e.g. a picture or PDF document, so there's no schema.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many media types full describe the semantics of a body.

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

Successfully merging this pull request may close these issues.

None yet

9 participants