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

Resolved undocumented nullable behavior #2046

Merged
merged 2 commits into from Nov 26, 2019

Conversation

@philsturgeon
Copy link
Contributor

philsturgeon commented Oct 28, 2019

OpenAPI Schema Objects and JSON Schema have a few fundamental differences, and this clears up a bit of confusion about one of them.

Basically, in JSON Schema, a schema of {} is totally valid, and when type is missing it essentially means "type can be anything", which would allow null.

With OpenAPI Schema Objects, {} should be allowed, but nullable confuses that with its default of false. You'd need to write { nullable: true } at the very least to let a null squeak through.

This seems like its breaking, but I dug around and most tooling will fall over if type is missing.

image

Seeing as some tooling prevents {} and some tooling supports it, we have a bit of a nasty situation that needs resolving. This change basically says "nullable adds null to the type that exist and missing type essentially means type: string|integer|array|etc... so adding null to that list is fine.

OpenAPI Schema Objects and JSON Schema have a few fundamental differences, and this clears up a bit of confusion about one of them.
versions/3.0.3.md Outdated Show resolved Hide resolved
tedepstein added a commit to RepreZen/OpenAPI-Specification that referenced this pull request Oct 31, 2019
…. This adds a formal proposal to clarify the semantics of nullable, providing the necessary background and links to related resources.
@tedepstein tedepstein mentioned this pull request Nov 1, 2019
1 of 2 tasks complete
Co-Authored-By: Ted Epstein <ted.epstein@reprezen.com>
@@ -2326,7 +2326,7 @@ Other than the JSON Schema subset fields, the following fields MAY be used for f
##### Fixed Fields
Field Name | Type | Description
---|:---:|---
<a name="schemaNullable"></a>nullable | `boolean` | Allows sending a `null` value for the defined schema. Default value is `false`.
<a name="schemaNullable"></a>nullable | `boolean` | A `true` value adds `"null"` to the allowed type specified by the `type` keyword, only if `type` is explicitly defined within the same Schema Object. Other Schema Object constraints retain their defined behavior, and therefore may disallow the use of `null` as a value. A `false` value leaves the specified or default `type` unmodified. The default value is `false`.

This comment has been minimized.

Copy link
@philsturgeon

philsturgeon Nov 1, 2019

Author Contributor

Do we also need to mention and clear up that type being missing is string, integer, null, array, etc?

This comment has been minimized.

Copy link
@tedepstein

tedepstein Nov 1, 2019

Contributor

Do we also need to mention and clear up that type being missing is string, integer, null, array, etc?

I think that's covered by JSON Schema. type is a constraint, and in the absence of a type constraint, values of any type are allowed.

I would not mind stating it explicitly, but TSC folks on the call were pushing to keep the spec minimal and concise. Paraphrasing what was said on the call, people will inevitably get confused and ask questions, and no amount of explanatory verbiage in the spec will prevent that from happening.

This comment has been minimized.

Copy link
@handrews

handrews Nov 1, 2019

Contributor

@philsturgeon we covered this in the call. We want to keep the text minimal, and defer to JSON Schema as much as possible.

darrelmiller added a commit that referenced this pull request Nov 12, 2019
* Addressing #1389, and clearing the way for PRs #2046 and #1977. This adds a formal proposal to clarify the semantics of nullable, providing the necessary background and links to related resources.

* Corrected table formatting.

* Minor tweaks and corrections.

* Correct Change Log heading.

* Cleaned up notes about translation to JSON Schema and *Of inheritance semantics.

* Fix Change Log heading in the proposal template.

* Snappy answers to stupid questions.

* Change single-quote 'null' to double-quote "null"

Thanks, @handrews for the review.

* Clarified the proposed definition of nullable

Somehow in our collaborative editing, we neglected to state that `nullable` adds `"null"` to the set of allowed types. We just said that it "expands" the `type` value, but didn't state the obvious (to us) manner of said expansion. Correcting that oversight in this commit.

* Corrected the alternative, heavy-handed interpretation of nullable.

* Added more explicit detail about the primary use case.

* Added a more complete explanation of the problems created by disallowing nulls by default.
darrelmiller added a commit that referenced this pull request Nov 21, 2019
* Addressing #1389, and clearing the way for PRs #2046 and #1977. This adds a formal proposal to clarify the semantics of nullable, providing the necessary background and links to related resources.

* Corrected table formatting.

* Minor tweaks and corrections.

* Correct Change Log heading.

* Cleaned up notes about translation to JSON Schema and *Of inheritance semantics.

* Fix Change Log heading in the proposal template.

* Snappy answers to stupid questions.

* Change single-quote 'null' to double-quote "null"

Thanks, @handrews for the review.

* Clarified the proposed definition of nullable

Somehow in our collaborative editing, we neglected to state that `nullable` adds `"null"` to the set of allowed types. We just said that it "expands" the `type` value, but didn't state the obvious (to us) manner of said expansion. Correcting that oversight in this commit.

* Corrected the alternative, heavy-handed interpretation of nullable.

* Added more explicit detail about the primary use case.

* Added a more complete explanation of the problems created by disallowing nulls by default.

* Added issue #2057 - interactions between nullable and default value.
@tedepstein

This comment has been minimized.

Copy link
Contributor

tedepstein commented Nov 21, 2019

@darrelmiller , this is the PR that we would like to merge for the 3.0.3 patch release.

I believe we have discussed this exhaustively on TSC calls. The new nullable definition language is what we composed in a real-time editing session with @webron, @handrews, and others on a TSC call a few weeks ago. The complete background, motivation, and analysis are documented in the proposal here.

As discussed, please herd the TSC cats to approve and merge. Thank you! :-)

@handrews

This comment has been minimized.

Copy link
Contributor

handrews commented Nov 21, 2019

@darrelmiller my recollection matches @tedepstein's.

I also put wording as close to exactly matching this as possible given type arrays in #1977, in case anyone is worried about wether/how this would fit with possible OAS 3.1 changes.

@philsturgeon

This comment has been minimized.

Copy link
Contributor Author

philsturgeon commented Nov 21, 2019

giphy

@darrelmiller

This comment has been minimized.

Copy link
Member

darrelmiller commented Nov 21, 2019

I think it is fair to say we have done our due diligence on this particular issue. Hey @OAI/tsc please review and and sign off so we can get this merged into 3.0.3 so we can clear the path to updating JSON Schema in 3.1. Ted's proposal provides all the background to this conversation and the reasoning as to why this solution was chosen.

@philsturgeon

This comment has been minimized.

Copy link
Contributor Author

philsturgeon commented Nov 26, 2019

Yaaaaay, @darrelmiller could you click that button?

@darrelmiller

This comment has been minimized.

Copy link
Member

darrelmiller commented Nov 26, 2019

I was going to say that we a majority TSC vote, but I re-read the governance rules and we only need that for merging the patch branch to master. Sooooooo. Time to click the big green button.

@darrelmiller darrelmiller merged commit dd48da1 into OAI:v3.0.3-dev Nov 26, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@philsturgeon

This comment has been minimized.

Copy link
Contributor Author

philsturgeon commented Nov 26, 2019

Thank you Darrel!!

@darrelmiller

This comment has been minimized.

Copy link
Member

darrelmiller commented Nov 26, 2019

@philsturgeon No, thank you and all the other community folks who put the effort into solving this tricky problem /cc @tedepstein @handrews

@webron

This comment has been minimized.

Copy link
Member

webron commented Nov 26, 2019

@philsturgeon, @tedepstein, @handrews - indeed, thank you all for pushing this forward. I love this came to be an elegant solution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.