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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Discourage usage of nullable on API types #8488

wants to merge 1 commit into from
+21 −0

Conversation

JoelSpeed
Copy link
Contributor

From conversation around #8486, we realised that the usage of nullable is problematic and would like to discourage new usage of it.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/developer-guide Issues or PRs related to the developer guide labels Jun 13, 2025
@k8s-ci-robot k8s-ci-robot requested a review from dims June 13, 2025 15:56
@k8s-ci-robot k8s-ci-robot added the sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. label Jun 13, 2025
@k8s-ci-robot k8s-ci-robot added sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 13, 2025
liggitt
liggitt reviewed Jun 13, 2025
View reviewed changes
contributors/devel/sig-architecture/api-conventions.md
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.")

contributors/devel/sig-architecture/api-conventions.md Outdated

Usage of `+nullable` is discouraged as it introduces several issues:
- It is not compatible with json merge patching.
- 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

contributors/devel/sig-architecture/api-conventions.md

Usage of `+nullable` is discouraged as it introduces several issues:
- It is not compatible with json merge patching.
- 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.

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

@fabriziopandini
Copy link
Member

Thanks for this clarification!

@JoelSpeed JoelSpeed force-pushed the discourage-nullable branch from 709c963 to b650843 Compare July 2, 2025 13:45
liggitt
liggitt reviewed Jul 2, 2025
View reviewed changes
Copy link
Member

@liggitt liggitt left a comment

Choose a reason for hiding this comment

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

a couple typos, then lgtm

contributors/devel/sig-architecture/api-conventions.md Outdated
- It is not compatible with json merge patching.
- 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 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.

Suggested change
- Explicit `null` values are not be persisted in proto serializations.
- Explicit `null` values are not persisted in proto serializations.

contributors/devel/sig-architecture/api-conventions.md Outdated
- A persisted `null` value would not round-trip through the applyconfiguration type
encode/decode cycle.

Avoid designing APIs that require the distinction bewteen 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.

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

@liggitt
Copy link
Member

liggitt commented Jul 2, 2025

(and squash down to a single commit)

@JoelSpeed JoelSpeed force-pushed the discourage-nullable branch from b650843 to aae2996 Compare July 2, 2025 15:12
@liggitt
Copy link
Member

liggitt commented Jul 2, 2025

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 2, 2025
@JoelSpeed JoelSpeed force-pushed the discourage-nullable branch from aae2996 to 33b481e Compare July 3, 2025 09:15
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 3, 2025
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 3, 2025
@JoelSpeed
Copy link
Contributor Author

Had to rebase to fix conflicts after my updates around optional/required serialization merged

@liggitt
Copy link
Member

liggitt commented Jul 3, 2025

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: JoelSpeed, liggitt
Once this PR has been reviewed and has the lgtm label, please assign derekwaynecarr for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@dims dims Awaiting requested review from dims

@johnbelamaric johnbelamaric Awaiting requested review from johnbelamaric

Assignees

@liggitt liggitt

Labels
area/developer-guide Issues or PRs related to the developer guide cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JoelSpeed @fabriziopandini @liggitt @k8s-ci-robot