-
Notifications
You must be signed in to change notification settings - Fork 9.2k
v3.3: edits to parameter sections #5122
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
base: v3.3-dev
Are you sure you want to change the base?
v3.3: edits to parameter sections #5122
Conversation
4f1638a to
490500a
Compare
this was added in commit 9739dfe, but now that the leading "?" has been removed, this is no longer needed
490500a to
2eed149
Compare
handrews
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karenetheridge thanks for continuing to work through all of these details! Some good catches here. I hope you don't mind me commenting on the draft as it sounds like you are adding more rather than likely to change what you have already posted. Let me know if you would prefer me to wait in the future.
I have a few questions and possible suggestions, and one very reluctant "it's not what I would do but it's what was decided" objection.
|
|
||
| * path - Used together with [Path Templating](#path-templating), 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`; MUST NOT appear in the same operation (or in the operation's path-item) as an `in: "querystring"` parameter. | ||
| * query - Parameters that are appended to the URL with the `?` character (or to existing query parameters with the `&` character), as described by [RFC3986 section 3.4](https://datatracker.ietf.org/doc/html/rfc3986#section-3.4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the RFC3986 reference? That section does define the query component, but only vaguely alludes to key-value pairs and does not mention the & character at all. Technically RFC6570 is the most relevant specification defining the query string as produced by in: query, although not as produces by in: querystring, for which WHATWG URL is more relevant.
| #### Fixed Fields | ||
|
|
||
| The rules for serialization of the parameter are specified in one of two ways. | ||
| The rules for serialization and deserialization of the parameter are specified in one of two ways. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in other places we have used "serialization and parsing" or "parsing and serialization." Do you think "deserialization" is a better term here? Is there a distinction between that and "parsing"? I have no idea what the answer is, I either copied "parsing" or picked it without much thought, so I'm open to changing it as long as we're consistent.
| | form | false | <span style="white-space: nowrap;">color=</span> | <span style="white-space: nowrap;">color=blue</span> | <span style="white-space: nowrap;">color=blue,black,brown</span> | <span style="white-space: nowrap;">color=R,100,G,200,B,150</span> | | ||
| | form | true | <span style="white-space: nowrap;">color=</span> | <span style="white-space: nowrap;">color=blue</span> | <span style="white-space: nowrap;">color=blue&color=black&color=brown</span> | <span style="white-space: nowrap;">R=100&G=200&B=150</span> | | ||
| | spaceDelimited</span> | false | _n/a_ | _n/a_ | <span style="white-space: nowrap;">color=blue%20black%20brown</span> | <span style="white-space: nowrap;">color=R%20100%20G%20200%20B%20150</span> | | ||
| | form | false | color= | color=blue | color=blue,black,brown | color=R,100,G,200,B,150 | | ||
| | form | true | color= | color=blue | color=blue&color=black&color=brown | R=100&G=200&B=150 | | ||
| | spaceDelimited | false | _n/a_ | _n/a_ | color=blue%20black%20brown | color=R%20100%20G%20200%20B%20150 | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My recollection is that we need these because otherwise the text will wrap at = and/or & without this, but I'm not at all certain. I tried to build locally to check but I'm having problems with that. Have you verified that it won't wrap without this?
| When inspecting schemas, given a starting point schema, implementations MUST examine that schema and all schemas that can be reached from it by following only `$ref` and `allOf` keywords. | ||
| These schemas are guaranteed to apply to any instance. | ||
| When searching schemas for `type`, if the `type` keyword's value is a list of types and the serialized value can be successfully parsed as more than one of the types in the list, and no other findable `type` keyword disambiguates the actual required type, the behavior is implementation-defined. | ||
| When searching schemas for `type`, if the `type` keyword's value is a list of types and the serialized value can be successfully parsed as more than one of the types in the list, and no other findable `type` keyword disambiguates the actual required type, the behavior is implementation-defined and must be documented. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a debate in the Thursday meeting over documentation requirements like this, which I had put in a lot of places, and the consensus was not to include it. So I had to take it out, and we can only have language like this if we want to re-open that discussion in the Thursday call.
| type: object | ||
| additionalProperties: | ||
| type: string | ||
| explode: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I was trying to be unambiguous about the explode value rather than leaving it as the default, which can be confusing. I don't know that we need to keep what I was doing, I'm just explaining why it was there.
| explode: true | ||
| allowReserved: true | ||
| - name: words | ||
| in: query | ||
| style: spaceDelimited | ||
| explode: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise here I was trying to show the explicit lack of difference between explode values that was clarified in 3.2. Likewise this is more of an explanation than a real objection.
| ``` | ||
|
|
||
| This example is equivalent to RFC6570's `{?foo*,bar}`, and **NOT** `{?foo*}{&bar}`. The latter is problematic because if `foo` is not defined, the result will be an invalid URI. | ||
| This example is equivalent to RFC6570's `{?foo*,bar}`, and **NOT** `{?foo*}{&bar}`. The latter is problematic because if `foo` is not defined (or the object has no properties), the result will be an invalid URI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember wrestling with whether to get into this when I wrote this part. RFC6570's actual requirements are even more complex:
A variable defined as a list value is considered undefined if the
list contains zero members. A variable defined as an associative
array of (name, value) pairs is considered undefined if the array
contains zero members or if all member names in the array are
associated with undefined values.
I think I decided to not get into it here because of this normative reference in the Parameter Object section:
The
undefinedcolumn replaces theemptycolumn in previous versions of this specification in order to better align with [RFC6570] Section 2.3 terminology, which describes certain values including but not limited tonullas “undefined” values with special handling; notably, the empty string is not undefined.
Perhaps a more general "see RFC6570 §3.2 for details on what is considered undefined" rather than trying to enumerate the conditions would be better? I do like the idea of making some note here that "undefined" is a complex concept.
| then: | ||
| properties: | ||
| name: | ||
| pattern: '^[^{}]+$' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch on the braces. I don't think we actually forbid the empty string, though, so * instead of +? (We probably should forbid it, but I don't think we do).
draft, as I may be adding more commits.