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

Rule #123 Null values should have their fields removed – still valid? #498

Closed
m99coder opened this issue May 8, 2019 · 12 comments · Fixed by #517
Closed

Rule #123 Null values should have their fields removed – still valid? #498

m99coder opened this issue May 8, 2019 · 12 comments · Fixed by #517

Comments

@m99coder
Copy link

m99coder commented May 8, 2019

I had a discussion in my team whether to omit or not null valued fields. Pretty clear is that optional fields can always be dropped, as there is no guarantee about their existence. But let’s imagine we are talking about a mandatory field.

In OpenAPI 2 there was no way to define null as a possible way, I learned. In OpenAPI 3, someone can define nullable: true to express null valued fields.

Most statically typed languages probably will treat a missing field and a null valued field the same (e.g. Scala encodes both with None in the case of an Option). For JavaScript someone would have to come up with a solution to check two things:

  1. Does the field exist at all (otherwise type is undefined)?
  2. Is the field null or something else?

By not dropping null valued, mandatory fields, the first check is unnecessary and the API spec can be treated as a real contract. Another option would be to make it explicit, that null is not a valid value. In this scenario it is to a certain extend unclear for the API consumer what happens with the field:

  • Will it be null in the response of a GET query for the same resource, as provided in the POST/PUT request to create it initially?
  • Will it even exist or omitted at all in the response of a GET query?

At the end I argue, the API should respect the provided input (e.g. null), if it is valid according to the JSON spec (which is the case for null) and explicitly handle it in some way expressing the business rules a/o logic. The latter can be propagating and keeping it or dropping it and therefore not ensuring that it returns the value null eventually.

Omitting a field is “lack of information”, while having it set to null does convey information… imho. Even the description of the rule states, that there are use cases where “using null” is important, e.g. JSON Merge Patch.

@whiskeysierra
Copy link
Contributor

Before we go into deeper discussions, can we agree on this?

required nullable {} {"example":null}
true true ✔️
false true ✔️ ✔️
true false
false false ✔️

@whiskeysierra
Copy link
Contributor

Link to the rule in question: https://opensource.zalando.com/restful-api-guidelines/#123

@m99coder
Copy link
Author

m99coder commented May 8, 2019

Almost 😉 The only exception is the second use case: Optional field, which can be null

required nullable {} {"example":null}
false true ✔️

Example 1

{
  "optional_field": null
}

Example 2

{}

Both examples would be possible and are treated semantically the same on API backend side. The API consumer has no guarantee about whether the provided null value will be kept or just not be there when querying the same resource. I would therefore rather vote for only one possibility. As required is “more important” than nullable, {} should be the only allowed case here.

@whiskeysierra
Copy link
Contributor

Both examples would be possible

That's what my ✔️ was symbolizing.

✔️ satisfied schema
❌ violates schema

@whiskeysierra
Copy link
Contributor

{} should be the only allowed case here

This is exactly what rule 123 is enforcing.

@m99coder
Copy link
Author

m99coder commented May 8, 2019

From my understanding it lacks the other part of even making {"optional_field": null} invalid, so that really only one variant is possible and not two (or more).

@whiskeysierra
Copy link
Contributor

whiskeysierra commented May 8, 2019

Ok, to be totally precise:

OpenAPI 3

required nullable {} {"example":null}
true true ✔️
false true ✔️ ✔️
true false
false false ✔️

RESTful API Guidelines

required nullable {} {"example":null}
true true
false true ✔️
true false
false false ✔️

Should: Null values should have their fields removed actually implies that nullable is completely useless, since API providers are required to omit those properties completely.

@m99coder
Copy link
Author

m99coder commented May 8, 2019

Thanks for clarifying. I think the second cross in first row of “RESTful API Guidelines” should be a checkmark, no? Apart from that I would say it’s clear to me now. We can close this issue, if you think it doesn’t serve any other purpose.

@whiskeysierra
Copy link
Contributor

If this issues doesn’t serve any other purpose, feel free to close it.

I feel that we should be more precise in the rule and state that

MUST: Not use nullable: true

@whiskeysierra
Copy link
Contributor

Conclusion: We changed the wording of the rule to something along the lines of

SHOULD treat absent and null values the same way

It basically leaves engineers some freedom. They can choose to use null or not, but there shouldn't be different semantics for both.

@zalando zalando deleted a comment from m99coder Jul 17, 2019
@zalando zalando deleted a comment from m99coder Jul 17, 2019
@zalando zalando deleted a comment from m99coder Jul 17, 2019
whiskeysierra pushed a commit that referenced this issue Jul 17, 2019
@whiskeysierra
Copy link
Contributor

@m99coder See #517

whiskeysierra pushed a commit that referenced this issue Jul 17, 2019
whiskeysierra pushed a commit that referenced this issue Jul 17, 2019
@m99coder
Copy link
Author

m99coder commented Jul 18, 2019

Thanks, @whiskeysierra . You can close this issue then, I guess.

tkrop added a commit that referenced this issue Jul 18, 2019
whiskeysierra pushed a commit that referenced this issue Jul 25, 2019
whiskeysierra pushed a commit that referenced this issue Jul 25, 2019
whiskeysierra pushed a commit that referenced this issue Jul 25, 2019
whiskeysierra pushed a commit that referenced this issue Jul 30, 2019
Rephrased rule about null/absent properties
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants