diff --git a/pkg/analysis/helpers/extractjsontags/analyzer.go b/pkg/analysis/helpers/extractjsontags/analyzer.go index 14e00311..221a0ee7 100644 --- a/pkg/analysis/helpers/extractjsontags/analyzer.go +++ b/pkg/analysis/helpers/extractjsontags/analyzer.go @@ -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 { @@ -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, @@ -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 diff --git a/pkg/analysis/optionalfields/analyzer.go b/pkg/analysis/optionalfields/analyzer.go index ae4da943..d5423600 100644 --- a/pkg/analysis/optionalfields/analyzer.go +++ b/pkg/analysis/optionalfields/analyzer.go @@ -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 } @@ -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. @@ -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. + 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) } } @@ -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) } @@ -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 { // 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 diff --git a/pkg/analysis/optionalfields/testdata/src/b/a.go b/pkg/analysis/optionalfields/testdata/src/b/a.go index 1915d2d4..1274e3f9 100644 --- a/pkg/analysis/optionalfields/testdata/src/b/a.go +++ b/pkg/analysis/optionalfields/testdata/src/b/a.go @@ -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" diff --git a/pkg/analysis/optionalfields/testdata/src/b/a.go.golden b/pkg/analysis/optionalfields/testdata/src/b/a.go.golden index de953890..5383bb9f 100644 --- a/pkg/analysis/optionalfields/testdata/src/b/a.go.golden +++ b/pkg/analysis/optionalfields/testdata/src/b/a.go.golden @@ -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" diff --git a/pkg/analysis/optionalfields/util.go b/pkg/analysis/optionalfields/util.go index 8190c14f..75e49592 100644 --- a/pkg/analysis/optionalfields/util.go +++ b/pkg/analysis/optionalfields/util.go @@ -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