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

Clarify allowEmptyValue further #3798

Closed
wants to merge 1 commit into from

Conversation

handrews
Copy link
Member

@handrews handrews commented May 11, 2024

[EDIT: Closed in favor of #3812]

[EDIT: See #3803 for an alternative based on discussion below.]

Make it clear that the default value does not do anything, and the non-default value (true) changes the normal expected behavior of the "schema" or "content" fields. This also explains why the keyword is safe to deprecate, as "schema" and "content" can be used to allow empty values in a more typical way.

@handrews handrews added clarification requests to clarify, but not change, part of the spec param serialization Issues related to parameter and/or header serialization labels May 11, 2024
@handrews handrews added this to the v3.0.4 milestone May 11, 2024
@handrews handrews requested a review from a team May 11, 2024 16:35
@handrews handrews marked this pull request as draft May 14, 2024 15:18
Make it clear that the default value does not do anything, and the
non-default value (true) changes the normal expected behavior of
the "schema" or "content" fields.  This also explains why the
keyword is safe to deprecate, as "schema" and "content" can be
used to allow empty values in a more typical way.
@ralfhandl
Copy link
Contributor

I think this PR is now ready for review.

@handrews
Copy link
Member Author

@ralfhandl Everyone is encouraged to review/approve this and other draft PRs. But with this much confusion on what is supposed to happen, I want to make sure that we do not merge it yet. See policy issue #3802 which I filed because at least 3 different people have merged PRs that I thought needed more attention, but we don't have any well-defined way to express that (discussions in TDC calls and comments in the PRs did not work).

@@ -1044,7 +1044,7 @@ Field Name | Type | Description
<a name="parameterDescription"></a>description | `string` | A brief description of the parameter. This could contain examples of use. [CommonMark syntax](https://spec.commonmark.org/) MAY be used for rich text representation.
<a name="parameterRequired"></a>required | `boolean` | Determines whether this parameter is mandatory. If the [parameter location](#parameterIn) is `"path"`, this property is **REQUIRED** and its value MUST be `true`. Otherwise, the property MAY be included and its default value is `false`.
<a name="parameterDeprecated"></a> deprecated | `boolean` | Specifies that a parameter is deprecated and SHOULD be transitioned out of usage. Default value is `false`.
<a name="parameterAllowEmptyValue"></a> allowEmptyValue | `boolean` | Sets the ability to pass empty-valued parameters. This is valid only for `query` parameters and allows sending a parameter with an empty value. Default value is `false`. If [`style`](#parameterStyle) is used, and if behavior is `n/a` (cannot be serialized), the value of `allowEmptyValue` SHALL be ignored. Use of this property is NOT RECOMMENDED, as it is likely to be removed in a later revision.
<a name="parameterAllowEmptyValue"></a> allowEmptyValue | `boolean` | If `true`, allows passing empty-valued parameters regardless of `schema` or `content`. This is valid only for `query` parameters. Default value is `false`. If [`style`](#parameterStyle) is used, and if behavior is `n/a` (cannot be serialized), the value of `allowEmptyValue` SHALL be ignored. Use of this property is NOT RECOMMENDED, as it is likely to be removed in a later revision; instead, ensure that the parameter's schema accepts any desired empty value(s).
Copy link
Contributor

Choose a reason for hiding this comment

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

Are ?foo and ?foo= both considered to have empty values? Should we say anything about whether one or the other is correct or preferred?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say both have empty values because if I run both variants through the standard URL parser of my current favorite programming language or build a service with my favorite web app framework both forms result in

{ 'foo' => '' }

so my server-side code would not be able to spot the difference.

Which of course is only my very personal opinion 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

@mikekistler it's not possible to produce ?foo (without an =). This is unfortunate, but it was discussed extensively in issue #1573 (the previous attempt to sort this out).

Copy link
Contributor

Choose a reason for hiding this comment

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

it's not possible to produce ?foo (without an =)

Just did with

http.get("http://localhost:3000/?foo", ...

My minimalistic server tells me that it received the URL /?foo. No = anywhere.

Copy link
Member

Choose a reason for hiding this comment

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

I think Henry means it's not possible to produce using an openapi tool to do the serialization? (Originally it was thought that allowEmptyValue would do this, but then in #1573 it was observed that this doesn't fit in with the URI Template spec.)

Copy link
Member Author

Choose a reason for hiding this comment

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

@ralfhandl – @karenetheridge is correct, and per #1573 whatever tool did that is non-compliant.

Copy link
Contributor

Choose a reason for hiding this comment

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

Consensus on the TSC is that this feature has ambiguous meanings, and interacts in awkward ways with Schema Object and Parameter style.

Definitely agree with that 😁

@handrews handrews closed this May 16, 2024
@handrews handrews deleted the allow-empty-304 branch May 20, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification requests to clarify, but not change, part of the spec param serialization Issues related to parameter and/or header serialization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants