Address review comments

Author: Karthik-K-N

pkg/analysis/helpers/extractjsontags/analyzer.go

@@ -25,7 +25,7 @@ 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"
 )
 
@@ -142,15 +142,15 @@ 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) || (len(tagValues) == 3 && (tagValues[1] == omitEmpty || tagValues[2] == omitEmpty)),
-		// Considering, omitzero will always coexist with omitemtpy and it can be either second or third tag.
-		OmitZero: len(tagValues) == 3 && (tagValues[1] == omitZero || tagValues[2] == omitZero),
-		RawValue: tagValue,
-		Pos:      pos,
-		End:      end,
+		OmitEmpty: tagSet.Has(omitEmpty),
+		OmitZero:  tagSet.Has(omitZero),
+		RawValue:  tagValue,
+		Pos:       pos,
+		End:       end,
 	}
 }
 

pkg/analysis/optionalfields/analyzer.go

@@ -141,7 +141,7 @@ func (a *analyzer) checkFieldProperties(pass *analysis.Pass, field *ast.Field, f
 	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
 	}
@@ -153,17 +153,17 @@ func (a *analyzer) checkFieldProperties(pass *analysis.Pass, field *ast.Field, f
 		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, 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 valid zero value and has omitzero tag.
+		// 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)
@@ -178,16 +178,21 @@ 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.
+			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.
@@ -229,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)
 }
 

pkg/analysis/optionalfields/testdata/src/b/a.go

@@ -171,10 +171,15 @@ type A struct {
 	// +optional
 	StructWithMinProperties B `json:"structWithMinProperties,omitempty"` // want "field StructWithMinProperties is optional and should be a pointer"
 
-	// StructWithMinPropertiesAndOmitEmptyTag is a struct field with a minimum number of properties.
+	// structWithMinPropertiesAndOmitEmptyAndOmitZero is a struct field with a minimum number of properties and has both omitzero and omitemtpy tag.
 	// +kubebuilder:validation:MinProperties=1
 	// +optional
-	StructWithMinPropertiesAndOmitEmptyTag B `json:"structWithMinPropertiesAndOmitEmptyTag,omitempty,omitzero"`
+	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

pkg/analysis/optionalfields/testdata/src/b/a.go.golden

@@ -171,10 +171,15 @@ type A struct {
 	// +optional
 	StructWithMinProperties *B `json:"structWithMinProperties,omitempty"` // want "field StructWithMinProperties is optional and should be a pointer"
 
-	// StructWithMinPropertiesAndOmitEmptyTag is a struct field with a minimum number of properties.
+	// structWithMinPropertiesAndOmitEmptyAndOmitZero is a struct field with a minimum number of properties and has both omitzero and omitemtpy tag.
 	// +kubebuilder:validation:MinProperties=1
 	// +optional
-	StructWithMinPropertiesAndOmitEmptyTag B `json:"structWithMinPropertiesAndOmitEmptyTag,omitempty,omitzero"`
+	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