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

Partial alternative attempt at allowEmptyValue clarification #3803

Closed
wants to merge 1 commit into from

Conversation

handrews
Copy link
Member

@handrews handrews commented May 14, 2024

[EDIT: Closed in favor of #3812]

Alternative to:

This attempts to clarify based on objections to the previous attempt to clarify. However, it is incomplete as the concept of "empty value" is not well-defined, which is why the following is embedded in the PR text and (obviously) not intended to be merged:

TODO: What values are empty? RFC6570 defines null, [], {}, and objects with only null properties (e.g. {"foo": null, "bar": null} to be "undefined" and serialize as empty values, but it does not consider the empty string to be undefined even though it also serializes as an empty value. Also, how does this interact with content, which is not RFC6570-based and may result in empty values through any number of different circumstances.

@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 14, 2024
@handrews handrews added this to the v3.0.4 milestone May 14, 2024
@handrews handrews requested a review from a team May 14, 2024 17:54
This attempts to clarify based on objections to the previous attempt
to clarify.  However, it is incomplete as the concept of "empty value"
is not well-defined.
@ralfhandl
Copy link
Contributor

Independent of what "empty" means, this proposal seems to fall into the "API description versus API definition" trap by adding text that sounds like a command to server-side implementors:

empty values MUST NOT be accepted

I prefer the alternative in #3798.

@LasneF
Copy link
Member

LasneF commented May 15, 2024

I agree with @ralfhandl between the 2 PR, one trying to clarify , and one pushing for deprecation, i would rather go to deprecation. empty value does not make too munch sense ever you send the parameter with a value that must be compliant to the schema , either you do not send it.

@handrews
Copy link
Member Author

@ralfhandl @LasneF this is not about changing anything. This keyword does... something? It is not at all clear what.

Whether this PR or #3798 is accepted should not be about what we want, it should be about what was intended. If necessary, that can be balanced with what a representative set of multiple tooling vendors have done with it.

But this is a patch release. It's not supposed to change anything. If @ralfhandl 's previous assertion that this keyword, when false (the default) disallows empty values is correct, then yes, there is a MUST NOT here. Not because I'm adding it, but because it was always there. That's what we need to figure out. That's why I submitted this despite not thinking it's a good approach, and indeed not being able to even properly define the implications.

This is the sort of ambiguity our tool developers and end users struggle with. And if we can't figure it out, how can we expect them to do so?

@handrews handrews closed this May 16, 2024
@handrews handrews deleted the alt-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.

3 participants