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

documented requestBody #878

Merged
merged 7 commits into from Feb 7, 2017
Merged

documented requestBody #878

merged 7 commits into from Feb 7, 2017

Conversation

fehguy
Copy link
Contributor

@fehguy fehguy commented Feb 1, 2017

This PR adds documentation for requestBody to describe inputs into operations.

This fixes #665, #303, #222, #418, #179, #874, #800, #303,

It clarifies #430

versions/3.0.md Outdated
type: string
format: binary

# mutliple files:
Copy link

@Danielku15 Danielku15 Feb 1, 2017

Choose a reason for hiding this comment

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

Should be multiple files (spelling)

versions/3.0.md Outdated
@@ -580,7 +578,7 @@ Field Name | Type | Description
<a name="operationExternalDocs"></a>externalDocs | [External Documentation Object](#externalDocumentationObject) | Additional external documentation for this operation.
<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="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>requestBody | [[Request Body Object](#requestBodyObject) <span>&#124;</span> [Reference Object](#referenceObject)] | The request body applicable for this operation.
<a name="operationRequestBody"></a>requestBody | [[Request Body Object](#requestBodyObject) <span>&#124;</span> [Reference Object](#referenceObject)] | The request body applicable for this operation. The `requestBody` is only supported in HTTP methods where the [HTTP 1.1 specification](https://tools.ietf.org/html/rfc7231#section-4.3.1) has explicitly defined semantics for request bodies.
Copy link
Member

Choose a reason for hiding this comment

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

What does "is only supported" mean? Does that mean you cannot specify a requestBody in the OAI document in these situations or that the consumer of the OAI document is not required to do anything with the requestBody in these situations? If this is the latter, what impact does this have on tooling authors?

Copy link
Contributor Author

@fehguy fehguy Feb 1, 2017

Choose a reason for hiding this comment

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

It means the spec does not allow, for example, requestBody on GET operations, and as a tooling vendor, you're outside the spec by supporting it.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, carry on.

Copy link
Member

Choose a reason for hiding this comment

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

The language needs to be stronger. Should say either that it MUST be one of the specific HTTP verbs (meaning using it with GET, DELETE...) will make the spec invalid - OR - that it SHALL be ignored if used with the wrong verbs (meaning, validation will allow it to exist there, but tools can safely ignore).

@whitlockjc
Copy link
Member

whitlockjc commented Feb 2, 2017

There are a few places where you use requestBody and I wonder what you think about using a link to the "Request Body Object" documentation itself instead? I see that in the "Content Object" section (https://github.com/OAI/OpenAPI-Specification/blob/OpenAPI.next/versions/3.0.md#content-object) we're already doing this and that's why it came to mind.

versions/3.0.md Outdated
@@ -580,7 +578,7 @@ Field Name | Type | Description
<a name="operationExternalDocs"></a>externalDocs | [External Documentation Object](#externalDocumentationObject) | Additional external documentation for this operation.
<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="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>requestBody | [[Request Body Object](#requestBodyObject) <span>&#124;</span> [Reference Object](#referenceObject)] | The request body applicable for this operation.
<a name="operationRequestBody"></a>requestBody | [[Request Body Object](#requestBodyObject) <span>&#124;</span> [Reference Object](#referenceObject)] | The request body applicable for this operation. The `requestBody` is only supported in HTTP methods where the [HTTP 1.1 specification](https://tools.ietf.org/html/rfc7231#section-4.3.1) has explicitly defined semantics for request bodies.
Copy link
Member

Choose a reason for hiding this comment

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

The language needs to be stronger. Should say either that it MUST be one of the specific HTTP verbs (meaning using it with GET, DELETE...) will make the spec invalid - OR - that it SHALL be ignored if used with the wrong verbs (meaning, validation will allow it to exist there, but tools can safely ignore).

# content transferred with base64 encoding
schema:
type: string
format: base64
Copy link
Member

Choose a reason for hiding this comment

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

This is byte today. We can change, but needs to be updated in the other section as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this will be a documentation concern

versions/3.0.md Outdated
items:
type: string
format: binary
```
Copy link
Member

Choose a reason for hiding this comment

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

This is incorrect. You can't have multiple file uploads with octet-stream, as they have no separator. For multiple files, the schema has to be named.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

That's multipart. This is octet-stream.

Copy link
Member

Choose a reason for hiding this comment

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

You are right. My bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ron just reminded me that we need to have a property with a name, holding the array property. Thus Ron is right, we can document that an array request body without schema property is meaningless and unsupported by all implementations of the tooling (thx @webron )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 @webron

versions/3.0.md Outdated
```

In scenarios where more control is needed over the Content-Type for `multipart` request bodies, an `encoding` attribute is introduced. This attribute is _only_ applicable to `mulitpart/*` request bodies.

Copy link
Member

Choose a reason for hiding this comment

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

Language is okay - this means that a use can choose to define it still, but it will be ignored if not multipart/*.

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 updated to say the encoding attribute are also applies to application/x-www-form-urlencoded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fine.

versions/3.0.md Outdated
##### Patterned Fields
Field Pattern | Type | Description
---|:---:|---
<a name="encodingObjectProperty"></a>The property to apply encoding to | [Encoding Property](#encodingProperty) <span>&#124;</span> [Encoding](#encoding) | The field name to apply special encoding attributes to. This field must exist in the schema as a property. To avoid collisions with specification extensions, properties may not begin with `x-`.
Copy link
Member

Choose a reason for hiding this comment

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

What's Encoding Property? Take a look at the type column.

Copy link
Member

Choose a reason for hiding this comment

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

Would also suggest to reverse the restriction. I think it's better to allow x- in properties and not allow extensions here, as you can add the extensions a level up covering this as well.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that not allowing vendor extensions here would be best.

versions/3.0.md Outdated
##### Fixed Fields
Field Name | Type | Description
---|:---:|---
<a name="contentType"></a>Content-Type | `string` | **Required.** The content-type to use for encoding a specific property.
Copy link
Member

Choose a reason for hiding this comment

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

media type

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 is wrong. Should stay contentType.

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

webron commented Feb 2, 2017

Thanks for putting this together, @fehguy - this addresses a lot of important issues.

Following our discussions, one thing is missing and that's addressing custom serialization format with x-www-form-urlencoded. The defaults are mentioned. IIRC, these should fall under the Encoding Object.

If we choose to skip that, then the Encoding Object structure can be simplified.

encoding:
historyMetadata:
# require XML content-type in utf-8 encoding
contentType: application/xml; charset=utf-8
Copy link
Contributor Author

Choose a reason for hiding this comment

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

what about Content-Disposition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Waiting for @darrelmiller for feedback on this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's add style here, great idea @webron ?

versions/3.0.md Outdated
Field Name | Type | Description
---|:---:|---
<a name="contentType"></a>Content-Type | `string` | **Required.** The content-type to use for encoding a specific property.
<a name="headers"></a>Headers | `object` | A string map allowing additional information to be provided as headers, for example `Content-Disposition`. Note `Content-Type` is described separately and will be ignored from this section.
Copy link
Member

Choose a reason for hiding this comment

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

Is this only applicable for multi-part support? If so, shouldn't we document it as such?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it is. I'll address in the PR

@webron
Copy link
Member

webron commented Feb 3, 2017

LGTM (though Content-Type should be contentType).

versions/3.0.md Outdated
<a name="headers"></a>Headers | `object` | A string map allowing additional information to be provided as headers, for example `Content-Disposition`. Note `Content-Type` is described separately and will be ignored from this section.
<a name="style"></a>Style | `string` | The content-type to use for encoding a specific property. See (#parameterContent) for details on the `style` property
<a name="explode"></a>explode | `boolean` | When this is true, property values of type `array` or `object` generate seperate parameters for each value of the array, or key-value-pair of the map. For other types of properties this property has no effect. The default value is false.
<a name="contentType"></a>Content-Type | `string` | **Required.** The content-type to use for encoding a specific property.
Copy link
Member

Choose a reason for hiding this comment

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

Content Type is here twice. Should it even be here or do we move it to headers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops. I'll remove one of them.

I left it out of the headers section intentionally. Since it describes the serialization strategy it's (a) required and (b) easier to predict in it's own attribute.

@darrelmiller
Copy link
Member

LGTM (though see comment about content-type, Ron might be wrong!)

@fehguy
Copy link
Contributor Author

fehguy commented Feb 7, 2017

@OAI/tdc shall we merge? (cha cha cha)

@whitlockjc
Copy link
Member

LGTM

@darrelmiller
Copy link
Member

:shipit:

@fehguy fehguy merged commit 17a389a into OpenAPI.next Feb 7, 2017
@fehguy fehguy deleted the issue-565 branch February 7, 2017 01:13
AndersDJohnson pushed a commit to AndersDJohnson/OpenAPI-Specification that referenced this pull request Apr 8, 2019
AndersDJohnson pushed a commit to AndersDJohnson/OpenAPI-Specification that referenced this pull request Apr 8, 2019
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

5 participants