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

feat: extend the colection format rule (154) to support OpenAPI 3 (#439) #440

Merged
merged 3 commits into from
Aug 23, 2018

Conversation

maxim-tschumak
Copy link
Contributor

Closes #439

@ghost
Copy link

ghost commented Aug 21, 2018

👍

@@ -229,24 +229,28 @@ for further hints on how to support the different HTTP methods on
resources.

[#154]
== {SHOULD} Explicitly define the Collection Format of Query Parameters
== {SHOULD} Use and Specify Explicitly the Form-Style Query Format for Collection Parameters
Copy link
Member

Choose a reason for hiding this comment

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

I would stick to

== {SHOULD} Define Collection Format of Query Parameters

as minimal precise description.

@@ -229,24 +229,28 @@ for further hints on how to support the different HTTP methods on
resources.

[#154]
== {SHOULD} Explicitly define the Collection Format of Query Parameters
== {SHOULD} Use and Specify Explicitly the Form-Style Query Format for Collection Parameters

There are different ways of supplying a set of values as a query
Copy link
Member

Choose a reason for hiding this comment

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

To simplify language I suggest the following text:

Sometimes, query parameters allow to provide a list of values, either by providing a comma-separated list (csv) or by repeating the parameter multiple times with different values (multi). The API specification should explicitly define one type as follows:

I think the following two sentences are optional:

In the Open API 3.0, the https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.1.md#style-values[`style: form] in combination with explode: true | false` is used.

In the Open API 2.0 the https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#parameter-object[`collectionFormat: csv | multi`] is used.


|`multi` |Multiple parameter instances
|`?parameter=value1&parameter=value2&parameter=value3`
|Description |OpenAPI 3 |OpenAPI 2 |Example
Copy link
Member

Choose a reason for hiding this comment

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

May be better

  1. Open API 3.0 | Open API 2.0
  2. The example can use key or param instead of parameter.
  3. Multiple parameter instances => Multiple parameters.

@maxim-tschumak maxim-tschumak force-pushed the 439-adopt-collection-format-rule-for-openapi3 branch from 3c7b0a9 to 7c651cc Compare August 23, 2018 07:41
@@ -229,24 +229,18 @@ for further hints on how to support the different HTTP methods on
resources.

[#154]
== {SHOULD} Explicitly define the Collection Format of Query Parameters
== {SHOULD} Define Collection Format of Query Parameters
Copy link
Member

Choose a reason for hiding this comment

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

I'm still not sure whether we should restrict this to query parameters only. Shouldn't path or header parameters follow the same rule if used?

Copy link
Contributor

Choose a reason for hiding this comment

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

Headers maybe, but arrays in path segments sounds incredibly wrong to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also wouldn't do this for path parameters, but it sounds reasonable for headers.

Copy link
Member

Choose a reason for hiding this comment

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

@whiskeysierra I agree, but the URI format is open to this, and if we cannot predict use cases here, my approach would to not restrict the application of the rule. The only result would be, that readers heaving the use case would not specify it, making it even harder to detect in a review.

|`?parameter=value1&parameter=value2&parameter=value3`
|Description |OpenAPI 3.0 |OpenAPI 2.0 |Example
|Comma separated values |`style: form, explode: false` |`collectionFormat: csv` |`?param=value1,value2`
|Multiple parameters |`style: form, explode: true` |`collectionFormat: multi` |`?param=value1&param=value2`
|=======================================================================
Copy link
Member

@tkrop tkrop Aug 23, 2018

Choose a reason for hiding this comment

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

In reviews I have experienced, that at least some readers cannot apply this concept in Open API. So may be we should give them at least one very concrete example:

For Open API 3.0 a csv query parameter will look as follows:

parameters:
    - name: customer_ids
      in: query
      type: array
      style: form
      explode: false
      items:
        type: string

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's explode: false shouldn't it be customer_ids then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tkrop I would not include an example. It's clearly stated in the table which properties have to be set.
And if we decide to include an OpenAPI 3 example, we also have to include an OpenAPI 2 example. This will take too much space.

@maxim-tschumak maxim-tschumak force-pushed the 439-adopt-collection-format-rule-for-openapi3 branch from 7c651cc to 04a2150 Compare August 23, 2018 09:29
@whiskeysierra
Copy link
Contributor

👍

2 similar comments
@maxim-tschumak
Copy link
Contributor Author

👍

@tkrop
Copy link
Member

tkrop commented Aug 23, 2018

👍

@tkrop tkrop merged commit 26637a9 into master Aug 23, 2018
@maxim-tschumak maxim-tschumak deleted the 439-adopt-collection-format-rule-for-openapi3 branch August 23, 2018 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants