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
+21 −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
21 changes: 21 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 @@ -909,6 +910,26 @@ In these cases, pointers are not needed, as the zero value indicates to the stru

This can be beneficial to API authors, as it reduces the complexity of the API, and reduces the risk of nil pointer exceptions in controllers.

## 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, or sometimes where a
custom marshal function is implemented.

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.")

- From the [JSON Merge Patch RFC](https://tools.ietf.org/html/rfc7386#section-1):
> Null values in the merge patch are given special meaning to indicate the removal of existing values in the target.
- Explicit `null` values are not persisted in proto serializations.
- `null` values are not supported by Server-Side Apply applyconfiguration types.
- A persisted `null` value would not round-trip through the applyconfiguration type
encode/decode cycle.

Avoid designing APIs that require the distinction between unset and `null`.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reached out to folks in sig-apimachinery for a response for this one, hoping they will have seen this and can give me a quick answer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at an openshift type that has Nullable, the generated applyconfiguration looks like

type ClusterResourceQuotaSelectorApplyConfiguration struct {
	LabelSelector      *metav1.LabelSelectorApplyConfiguration `json:"labels,omitempty"`
	AnnotationSelector map[string]string                       `json:"annotations,omitempty"`
}

If I were to explicitly set a null, Idon't think this could round trip through this type given the omitempty.
My understanding of the generated applyconfiguration types is that they are always pointer+omitempty (apart from maps/slices which are just omitempty), and omitempty is not compatible with +nullable right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some conversation with Stefan in https://kubernetes.slack.com/archives/C0EG7JC6T/p1751462820154829?thread_ts=1750672509.687029&cid=C0EG7JC6T

## Defaulting

In general we want default values to be explicitly represented in our APIs,
Expand Down