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

Nullable properties with a default of null are not possible according to the 3.0 spec #2057

Closed
johncmunson opened this issue Nov 17, 2019 · 12 comments

Comments

@johncmunson
Copy link

johncmunson commented Nov 17, 2019

From the spec...

default - The default value represents what would be assumed by the consumer of the input as the value of the schema if one is not provided. Unlike JSON Schema, the value MUST conform to the defined type for the Schema Object defined at the same level. For example, if type is string, then default can be "foo" but cannot be 1.

This, combined with the fact that properties can be nullable, means that it's not possible to have a property with a default value of null.

You can see below that the folks over at Swagger have interpreted the spec literally, as they should. It's the spec that seems to be wrong.

Here's a sample response body on my /todos/{id} route in the Swagger UI...

{
  "id": 0,
  "title": "string",
  "complete": false,
  "note": "null",
  "dueDate": "null",
  "categoryId": 0
}

And here's the OpenAPI schema for my todo objects...

Todo:
  description: A todo item helps you track what you need to get done
  type: object
  required:
    - title
    - complete
  properties:
    id:
      type: integer
    title:
      type: string
    complete:
      type: boolean
      default: false
    note:
      type: string
      nullable: true
      default: null
    dueDate:
      type: string
      nullable: true
      default: null
    categoryId:
      type: integer
      nullable: true
      default: null

Looking at the note property, we can see that the default value of null gets interpreted as the string "null".

Looking at categoryId, it seems that 0 is the closest integer to null they could think of.

@tedepstein
Copy link
Contributor

tedepstein commented Nov 17, 2019

@johncmunson , as I read the spec, it is only saying that the default value must conform to the type specified in the schema.

Unlike JSON Schema, the value MUST conform to the defined type for the Schema Object defined at the same level.

This is just to contrast the requirement with JSON Schema's less stringent guidance:

It is RECOMMENDED that a default value be valid against the associated schema.

As we're attempting to clarify in this proposal, nullable should function as a type modifier. Under that interpretation, your note Schema Object would translate to a JSON Schema with "type" : ["string", "null"]. In that case, "default" : null should be interpreted as a literal null value, not the string "null", and it should be allowed.

I'm not sure which Swagger component you were referring to. I would say that the behavior of this Swagger component is wrong; it should allow null as a default value. But honestly, the current spec is very unclear as to how nullable should be interpreted, and how it should interact with other keywords. This is yet another case of that, so I'll add it to the proposal for completeness.

Until the proposal is accepted and the v3.0.3 patch of the specification is released, we're kind of in a gray area here. But you might raise the issue in the appropriate Swagger project, and see if they agree that they should change the default validation logic. They might want to make this change even before the proposed clarification is official.

@johncmunson
Copy link
Author

johncmunson commented Nov 17, 2019

@tedepstein thanks for some of the background details surrounding this. I'm glad that it's on the team's radar to clear up some of the ambiguity surrounding null values. After your comments, I now see it from your perspective that the spec is a little ambiguous in this regard, rather than contradicting or wrong. I will raise this issue in the appropriate Swagger repo and see what their perspective is on this. Thanks!

Update:
Here is the related issue I created for Swagger.

darrelmiller pushed a commit that referenced this issue 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.
@handrews
Copy link
Member

If we need to further clarify the language in OAS 3.1, we should track this in #2099. Otherwise it sounds like maybe this can be closed?

@tedepstein
Copy link
Contributor

I would like to clarify the language in 3.1. In fact, #1977 doesn't update the nullable definition to the final language that we agreed to, so we need a PR to cover that anyway. I can propose some other minor clarifications in the same PR.

@darrelmiller
Copy link
Member

@tedepstein Could that be because we updated the nullable language in 3.0.3 and it hasn't been ported over to 3.1 yet?

@handrews
Copy link
Member

@tedepstein I did the language for 3.1 did it not get merged into the PR? I don't understand how GitHub's "suggestions" work. https://github.com/OAI/OpenAPI-Specification/pull/1977/files#diff-b83206d953b4be5d312b068f82ba0217R2295

@webron
Copy link
Member

webron commented Jan 22, 2020

@handrews suggestions need to be accepted by the original author of the PR, which possibly did not happen in this case. Would be easier to just submit another PR for it.

@philsturgeon
Copy link
Contributor

@tedepstein I added that language from @handrews to #2124 so if there is more improvements to come lets do it over there.

@tedepstein
Copy link
Contributor

Thanks, @philsturgeon , @darrelmiller & @handrews .

Aside from the nullable definition, which is now complete, I would still like to review the other parts of the spec that mention nullable or relate to it, just to see if any part of it needs to be clarified or realigned with the updated definition. It's on my to-do list, and once I get to it I'll open a separate PR.

@philsturgeon , you might want to add this to your punch list in #2099 as @handrews suggests.

@philsturgeon
Copy link
Contributor

@tedepstein it's on! what needs to be done? We gotta merge #2115 then we're done, or is there more?

@tedepstein
Copy link
Contributor

@philsturgeon , I just had a look and I think #2115 is ready to go. No need for further clarifications about nullable in the spec, unless we want to provide examples somewhere. And given that it's deprecated, I'd rather provide examples elsewhere instead of adding weight to the spec.

@philsturgeon
Copy link
Contributor

Dope! That PR is on the agenda so I'm gonna close this one on down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants