Skip to content

Discourage usage of nullable on API types #8488

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+12 −0
Open

Discourage usage of nullable on API types #8488

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions contributors/devel/sig-architecture/api-conventions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ An introduction to using resources with kubectl can be found in [the object mana
- [Categories](#categories)
- [Idempotency](#idempotency)
- [Optional vs. Required](#optional-vs-required)
- [Nullable](#nullable)
- [Defaulting](#defaulting)
- [Static Defaults](#static-defaults)
- [Admission Controlled Defaults](#admission-controlled-defaults)
Expand Down Expand Up @@ -866,6 +867,17 @@ language client, and any other clients that use corresponding types
Therefore, we ask that pointers always be used with optional fields that do not
have a built-in `nil` value.

## Nullable

The `+nullable` comment tag allows the json `null` value to be a valid value
for a field. The `null` value is serialized only when a field is a pointer
in the Go definition, and does not have the `omitempty` json tag.

For example, a map field marked with `+nullable` would accept either `foo: null` or `foo: {}`.

Usage of `+nullable` is discouraged as it introduces several issues:
- It is not compatible with json merge patching.
Copy link
Member

Choose a reason for hiding this comment

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

(from the merge patch RFC: "Null values in the merge patch are given special meaning to indicate the removal of existing values in the target.")

- It is not compatible with generic protobuf.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- It is not compatible with generic protobuf.
- Explicit `null` values are not be persisted in proto serializations


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 it's also not compatible with server-side apply, but this should be verified (I have a vague memory of SSA using null values as a signal to remove a key, which would make a null value impossible to persist via SSA)

## Defaulting

Expand Down