Skip to content

Support omitzero tags in optionalfields linter #115

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 2 commits into
base: main
Choose a base branch
from
+60 −16
Open

Support omitzero tags in optionalfields linter #115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d5f8bf2
Support omitzero tags in optionalfields linter
Karthik-K-N Jul 3, 2025
8191661
Address review comments
Karthik-K-N Jul 4, 2025
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
14 changes: 12 additions & 2 deletions pkg/analysis/helpers/extractjsontags/analyzer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,15 @@ import (
"golang.org/x/tools/go/analysis"
"golang.org/x/tools/go/analysis/passes/inspect"
"golang.org/x/tools/go/ast/inspector"

"k8s.io/utils/set"
kalerrors "sigs.k8s.io/kube-api-linter/pkg/analysis/errors"
)

const (
omitEmpty = "omitempty"
omitZero = "omitzero"
)

// StructFieldTags is used to find information about
// json tags on fields within struct.
type StructFieldTags interface {
Expand Down Expand Up @@ -137,10 +142,12 @@ func extractTagInfo(tag *ast.BasicLit) FieldTagInfo {
}

tagName := tagValues[0]
tagSet := set.New[string](tagValues...)

return FieldTagInfo{
Name: tagName,
OmitEmpty: len(tagValues) == 2 && tagValues[1] == "omitempty",
OmitEmpty: tagSet.Has(omitEmpty),
OmitZero: tagSet.Has(omitZero),
RawValue: tagValue,
Pos: pos,
End: end,
Expand All @@ -159,6 +166,9 @@ type FieldTagInfo struct {
// OmitEmpty is true if the field has the omitempty option in the json tag.
OmitEmpty bool

// OmitZero is true if the field has the omitzero option in the json tag.
OmitZero bool

// Inline is true if the field has the inline option in the json tag.
Inline bool

Expand Down
40 changes: 27 additions & 13 deletions pkg/analysis/optionalfields/analyzer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,13 +134,14 @@ func defaultConfig(cfg *config.OptionalFieldsConfig) {
func (a *analyzer) checkFieldProperties(pass *analysis.Pass, field *ast.Field, fieldName string, markersAccess markershelper.Markers, jsonTags extractjsontags.FieldTagInfo) {
hasValidZeroValue, completeValidation := isZeroValueValid(pass, field, field.Type, markersAccess)
hasOmitEmpty := jsonTags.OmitEmpty
hasOmitZero := jsonTags.OmitZero
isPointer, underlying := isStarExpr(field.Type)
isStruct := isStructType(pass, field.Type)

if a.pointerPreference == config.OptionalFieldsPointerPreferenceAlways {
// The field must always be a pointer, pointers require omitempty, so enforce that too.
a.handleFieldShouldBePointer(pass, field, fieldName, isPointer, underlying)
a.handleFieldShouldHaveOmitEmpty(pass, field, fieldName, hasOmitEmpty, jsonTags)
a.handleFieldShouldHaveOmitEmpty(pass, field, fieldName, hasOmitEmpty, hasOmitZero, hasValidZeroValue, isStruct, jsonTags)

return
}
Expand All @@ -149,18 +150,21 @@ func (a *analyzer) checkFieldProperties(pass *analysis.Pass, field *ast.Field, f

if a.omitEmptyPolicy != config.OptionalFieldsOmitEmptyPolicyIgnore || hasOmitEmpty {
// If we require omitempty, or the field has omitempty, we can check the field properties based on it being an omitempty field.
a.checkFieldPropertiesWithOmitEmptyRequired(pass, field, fieldName, jsonTags, underlying, hasOmitEmpty, hasValidZeroValue, completeValidation, isPointer, isStruct)
a.checkFieldPropertiesWithOmitEmptyRequired(pass, field, fieldName, jsonTags, underlying, hasOmitEmpty, hasOmitZero, hasValidZeroValue, completeValidation, isPointer, isStruct)
} else {
// The field does not have omitempty, and does not require it.
a.checkFieldPropertiesWithoutOmitEmpty(pass, field, fieldName, jsonTags, underlying, hasValidZeroValue, completeValidation, isPointer, isStruct)
a.checkFieldPropertiesWithoutOmitEmpty(pass, field, fieldName, jsonTags, underlying, hasValidZeroValue, completeValidation, isPointer, isStruct, hasOmitZero)
}
}

func (a *analyzer) checkFieldPropertiesWithOmitEmptyRequired(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, underlying ast.Expr, hasOmitEmpty, hasValidZeroValue, completeValidation, isPointer, isStruct bool) {
func (a *analyzer) checkFieldPropertiesWithOmitEmptyRequired(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, underlying ast.Expr, hasOmitEmpty, hasOmitZero, hasValidZeroValue, completeValidation, isPointer, isStruct bool) {
// In this case, we should always add the omitempty if it isn't present.
a.handleFieldShouldHaveOmitEmpty(pass, field, fieldName, hasOmitEmpty, jsonTags)
a.handleFieldShouldHaveOmitEmpty(pass, field, fieldName, hasOmitEmpty, hasOmitZero, hasValidZeroValue, isStruct, jsonTags)

switch {
case !hasValidZeroValue && isStruct && hasOmitZero:
// The struct field need not be pointer if it does not have a valid zero value and has omitzero tag.
return
case hasValidZeroValue && !completeValidation:
a.handleIncompleteFieldValidation(pass, field, fieldName, isPointer, underlying)
fallthrough // Since it's a valid zero value, we should still enforce the pointer.
Expand All @@ -174,20 +178,25 @@ func (a *analyzer) checkFieldPropertiesWithOmitEmptyRequired(pass *analysis.Pass
}
}

func (a *analyzer) checkFieldPropertiesWithoutOmitEmpty(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, underlying ast.Expr, hasValidZeroValue, completeValidation, isPointer, isStruct bool) {
func (a *analyzer) checkFieldPropertiesWithoutOmitEmpty(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, underlying ast.Expr, hasValidZeroValue, completeValidation, isPointer, isStruct, hasOmitZero bool) {
switch {
case hasValidZeroValue:
// The field is not omitempty, and the zero value is valid, the field does not need to be a pointer.
a.handleFieldShouldNotBePointer(pass, field, fieldName, isPointer, "field %s is optional, without omitempty and allows the zero value. The field does not need to be a pointer.")
case !hasValidZeroValue:
// The zero value would not be accepted, so the field needs to have omitempty.
// Force the omitempty policy to suggest a fix. We can only get to this function when the policy is configured to Ignore.
// Since we absolutely have to add the omitempty tag, we can report it as a suggestion.
reportShouldAddOmitEmpty(pass, field, config.OptionalFieldsOmitEmptyPolicySuggestFix, fieldName, "field %s is optional and does not allow the zero value. It must have the omitempty tag.", jsonTags)
if isStruct && hasOmitZero {
// The struct field need not have omitempty tag if it does not have a valid zero value and has omitzero tag.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we are going to need similar omitzero behaviour/configuration as to what we have for omitempty. The logic below in the else clause is important for when the object is a struct, but does not have omitzero, we would need to suggest it.

At the moment, I would only recommend the omitzero checks/recommendations to look at structs

return
} else {
// The zero value would not be accepted, so the field needs to have omitempty.
// Force the omitempty policy to suggest a fix. We can only get to this function when the policy is configured to Ignore.
// Since we absolutely have to add the omitempty tag, we can report it as a suggestion.
reportShouldAddOmitEmpty(pass, field, config.OptionalFieldsOmitEmptyPolicySuggestFix, fieldName, "field %s is optional and does not allow the zero value. It must have the omitempty tag.", jsonTags)
}
// Once it has the omitempty tag, it will also need to be a pointer in some cases.
// Now handle it as if it had the omitempty already.
// We already handle the omitempty tag above, so force the `hasOmitEmpty` to true.
a.checkFieldPropertiesWithOmitEmptyRequired(pass, field, fieldName, jsonTags, underlying, true, hasValidZeroValue, completeValidation, isPointer, isStruct)
a.checkFieldPropertiesWithOmitEmptyRequired(pass, field, fieldName, jsonTags, underlying, true, false, hasValidZeroValue, completeValidation, isPointer, isStruct)
}
}

Expand Down Expand Up @@ -225,11 +234,16 @@ func (a *analyzer) handleFieldShouldNotBePointer(pass *analysis.Pass, field *ast
reportShouldRemovePointer(pass, field, a.pointerPolicy, fieldName, message)
}

func (a *analyzer) handleFieldShouldHaveOmitEmpty(pass *analysis.Pass, field *ast.Field, fieldName string, hasOmitEmpty bool, jsonTags extractjsontags.FieldTagInfo) {
func (a *analyzer) handleFieldShouldHaveOmitEmpty(pass *analysis.Pass, field *ast.Field, fieldName string, hasOmitEmpty, hasOmitZero, hasValidZeroValue, isStruct bool, jsonTags extractjsontags.FieldTagInfo) {
if hasOmitEmpty {
return
}

if !hasValidZeroValue && isStruct && hasOmitZero {
// The struct field need not have omitempty tag if it does not have a valid zero value and has omitzero tag.
return
}

reportShouldAddOmitEmpty(pass, field, a.omitEmptyPolicy, fieldName, "field %s is optional and should have the omitempty tag", jsonTags)
}

Expand Down Expand Up @@ -300,7 +314,7 @@ func getStructZeroValue(pass *analysis.Pass, structType *ast.StructType) string
for _, field := range structType.Fields.List {
fieldTagInfo := jsonTagInfo.FieldTags(field)

if fieldTagInfo.OmitEmpty {
if fieldTagInfo.OmitEmpty || fieldTagInfo.OmitZero {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can only rely on omitzero when the field isn't a pointer, can we check that here?

// If the field is omitted, we can use a zero value.
// For structs, if they aren't a pointer another error will be raised.
continue
Expand Down
10 changes: 10 additions & 0 deletions pkg/analysis/optionalfields/testdata/src/b/a.go
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

Please add the field with only omitzero tag.

Copy link
Author

Choose a reason for hiding this comment

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

Added, but whats the expectation, Currently linter suggests to add omitempty tag, Should that be honored or should we make changes to avoid that suggestion when omitzero present on optional tags?

Copy link
Contributor

Choose a reason for hiding this comment

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

Upstream guidance is still that omitempty should be everywhere, but it's useful to have a case where it is omitzero only, as I would expect that in the case where omitempty is being ignored, this change should still work

Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,16 @@ type A struct {
// +optional
StructWithMinProperties B `json:"structWithMinProperties,omitempty"` // want "field StructWithMinProperties is optional and should be a pointer"

// structWithMinPropertiesAndOmitEmptyAndOmitZero is a struct field with a minimum number of properties and has both omitzero and omitemtpy tag.
// +kubebuilder:validation:MinProperties=1
// +optional
StructWithMinPropertiesAndOmitEmptyAndOmitZero B `json:"structWithMinPropertiesAndOmitEmptyAndOmitZero,omitempty,omitzero"`

// structWithOnlyOmitZeroTag is a struct field with a minimum number of properties and has only omitzero tag.
// +kubebuilder:validation:MinProperties=1
// +optional
StructWithOnlyOmitZeroTag B `json:"structWithOnlyOmitZeroTag,omitzero"`
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, if we aren't using the ignore policy on omitempty, we should add omitempty as well as omitzero to be consistent with other types


// structWithMinPropertiesOnStruct is a struct field with a minimum number of properties on the struct.
// +optional
StructWithMinPropertiesOnStruct D `json:"structWithMinPropertiesOnStruct,omitempty"` // want "field StructWithMinPropertiesOnStruct is optional and should be a pointer"
Expand Down
10 changes: 10 additions & 0 deletions pkg/analysis/optionalfields/testdata/src/b/a.go.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,16 @@ type A struct {
// +optional
StructWithMinProperties *B `json:"structWithMinProperties,omitempty"` // want "field StructWithMinProperties is optional and should be a pointer"

// structWithMinPropertiesAndOmitEmptyAndOmitZero is a struct field with a minimum number of properties and has both omitzero and omitemtpy tag.
// +kubebuilder:validation:MinProperties=1
// +optional
StructWithMinPropertiesAndOmitEmptyAndOmitZero B `json:"structWithMinPropertiesAndOmitEmptyAndOmitZero,omitempty,omitzero"`

// structWithOnlyOmitZeroTag is a struct field with a minimum number of properties and has only omitzero tag.
// +kubebuilder:validation:MinProperties=1
// +optional
StructWithOnlyOmitZeroTag B `json:"structWithOnlyOmitZeroTag,omitzero"`

// structWithMinPropertiesOnStruct is a struct field with a minimum number of properties on the struct.
// +optional
StructWithMinPropertiesOnStruct *D `json:"structWithMinPropertiesOnStruct,omitempty"` // want "field StructWithMinPropertiesOnStruct is optional and should be a pointer"
Expand Down
2 changes: 1 addition & 1 deletion pkg/analysis/optionalfields/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ func areStructFieldZeroValuesValid(pass *analysis.Pass, structType *ast.StructTy
for _, field := range structType.Fields.List {
fieldTagInfo := jsonTagInfo.FieldTags(field)

if fieldTagInfo.OmitEmpty {
if fieldTagInfo.OmitEmpty || fieldTagInfo.OmitZero {
// If the field is omitted, we can use a zero value.
// For structs, if they aren't a pointer another error will be raised.
continue
Expand Down