Add exception to pointer guidance for structs that must be omitted #1411
Conversation
|
Saw this before openshift/api#1310 (comment) and then again today working with the OpenStack team on their failure domain implementation |
|
LGTM |
|
|
||
| The JSON tag `omitempty` is not compatible with struct references. Meaning any struct will always, when empty, be marshalled to `{}`. If the struct **must** genuinely be omitted, it must be a pointer. | ||
|
|
||
| When used in combination with a validation such as `minProperties` on the parent, or a required field within the struct, the struct itself must be |
There was a problem hiding this comment.
Always useful to provide a simple example. I prefer to first check examples and then read in more detail all the limitations/constraints.
|
|
||
| An exception to this rule is when using a pointer to a struct in combination with an API validation that requires the field to be unset. | ||
|
|
||
| The JSON tag `omitempty` is not compatible with struct references. Meaning any struct will always, when empty, be marshalled to `{}`. If the struct **must** genuinely be omitted, it must be a pointer. |
There was a problem hiding this comment.
under what condition must the struct be omitted? Discriminated unions, yes. But anywhere else? I would expect the zero value to be exactly the same as nil and missing.
There was a problem hiding this comment.
A concrete example is the API we are adding for the OpenStack failure domains to control plane machine set.
We don't want to allow an empty failure domain in the list, it must have at least one non-empty field.
- {} // invalid, no fields set in the failure domain
- rootVolume: {} // rootVolume is a struct, this entry in the list is still invalid
- availabilityZone: foo // valid, one field is non-empty
- rootVolume:
availabilityZone: foo // valid, one field is non-empty
- availabilityZone: foo
rootVolume:
availabilityZone: foo // Also valid, more than one field is non-empty
To achieve this set of requirements, we use
// +kubebuilder:validation:MinProperties:=1
type OpenStackFailureDomain struct {
// +optional
AvailabilityZone string
// +optional
RootVolume *RootVolume
}
// +kubebuilder:validation:MinProperties:=1
type RootVolume struct {
// +optional
AvailabilityZone string
}
If RootVolume is not a pointer, when serialized from Go, the JSON library will serialize it as {}, so an empty OpenStackFailureDomain struct becomes {"rootVolume":{}} in JSON. Because the field is then "present", the validation runs and it is rejected because rootVolume does not have 1 property. That case is ok.
But, the issue comes if I want to create:
foo := OpenStackFailureDomain{
AvailabilityZone: "foo",
}
With a pointer it marshals to {"availabilityZone": "foo"}, without a pointer it marshals to {"availabilityZone": "foo","rootVolume":{}}, which then fails validation because rootVolume does not have 1 property.
Is there some alternative way to achieve the validation we want?
A GoPlayground to demonstrate the behaviour of json.Marshal.
And the PR.
There was a problem hiding this comment.
This looks great. Can you mention all of it in the document?
There was a problem hiding this comment.
Is there a specific part you think is particularly useful? I worry adding too many in depth examples will lead to the document being overwhelmingly long and complicated to read
Perhaps a link to this thread would make that easier?
There was a problem hiding this comment.
The link works as well (assuming the thread will not get removed by accident).
There was a problem hiding this comment.
So while I understand that it makes the validation slightly easier, for a configuration resource, why shouldn't we expect a zero-value of rootVolume to behave the same as a nil rootVolume?
For a workload resource, I definitely agree it should be a pointer?
There was a problem hiding this comment.
One example would be a discriminated union. We use those on configuration resources regularly. The DU needs a required discriminator, but, you cannot have a required field in a non-pointer struct. So any DU must be a pointer to struct independent of configuration or not.
In other examples where we want a required field in a struct, say for example an optional reference to another object with a namespace. If you specify the reference, we want the name and namespace field within to be required.
Is it better here to use CEL to check for "all required values are empty" and allow that case? Does that become a little confusing to the user?
|
LGTM, thanks for the addition. This will help. |
|
Inactive enhancement proposals go stale after 28d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
/remove-lifecycle stale |
openshift-ci
bot
removed
the
lifecycle/stale
|
Inactive enhancement proposals go stale after 28d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
/remove-lifecycle stale. |
|
/remove-lifecycle stale |
|
Stale enhancement proposals rot after 7d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
/remove-lifecycle rotten |
openshift-ci
bot
removed
the
lifecycle/rotten
|
Inactive enhancement proposals go stale after 28d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
/remove-lifecycle stale |
openshift-ci
bot
removed
the
lifecycle/stale
|
Inactive enhancement proposals go stale after 28d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
Still need to come back to this and update the guidance |
openshift-ci
bot
added
the
lifecycle/stale
|
Stale enhancement proposals rot after 7d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
/remove-lifecycle rotten |
openshift-ci
bot
removed
the
lifecycle/rotten
|
Inactive enhancement proposals go stale after 28d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
Stale enhancement proposals rot after 7d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
/remove-lifecycle rotten |
openshift-ci
bot
removed
the
lifecycle/rotten
|
Inactive enhancement proposals go stale after 28d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
/remove-lifecycle stale Still relevant |
openshift-ci
bot
removed
the
lifecycle/stale
|
Inactive enhancement proposals go stale after 28d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
Stale enhancement proposals rot after 7d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
/remove-lifecycle rotten |
openshift-ci
bot
removed
the
lifecycle/rotten
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@JoelSpeed: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
A couple of times now I've needed to use a pointer with a struct to achieve the desired API behaviour, for example when the empty struct must genuinely be omitted.
An example of this is when the field has a required field within it. API validations will not execute if the serialised version does not include the field. (ie
{}vs{blah: {}})A secondary example is when you use the
minPropertiesvalidation on the parent, in this case we need the field to be genuinely omitted else the parent cannot useminPropertieseffectively. ({blah: {}}would passminProperties: 1even though the intention is to have at least one non-empty field within the struct)CC @deads2k @ingvagabund