Merge pull request #131 from JoelSpeed/extract-serialization-checker

Extract serialization checker into a separate utility

Author: k8s-ci-robot

pkg/analysis/optionalfields/analyzer.go

@@ -23,7 +23,7 @@ import (
 	"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/extractjsontags"
 	"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/inspector"
 	markershelper "sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/markers"
-	"sigs.k8s.io/kube-api-linter/pkg/analysis/utils"
+	"sigs.k8s.io/kube-api-linter/pkg/analysis/utils/serialization"
 	"sigs.k8s.io/kube-api-linter/pkg/markers"
 )
 
@@ -46,10 +46,7 @@ func init() {
 }
 
 type analyzer struct {
-	pointerPolicy     OptionalFieldsPointerPolicy
-	pointerPreference OptionalFieldsPointerPreference
-	omitEmptyPolicy   OptionalFieldsOmitEmptyPolicy
-	omitZeroPolicy    OptionalFieldsOmitZeroPolicy
+	serializationCheck serialization.SerializationCheck
 }
 
 // newAnalyzer creates a new analyzer.
@@ -60,11 +57,21 @@ func newAnalyzer(cfg *OptionalFieldsConfig) *analysis.Analyzer {
 
 	defaultConfig(cfg)
 
+	serializationCheck := serialization.New(&serialization.Config{
+		Pointers: serialization.PointersConfig{
+			Policy:     serialization.PointersPolicy(cfg.Pointers.Policy),
+			Preference: serialization.PointersPreference(cfg.Pointers.Preference),
+		},
+		OmitEmpty: serialization.OmitEmptyConfig{
+			Policy: serialization.OmitEmptyPolicy(cfg.OmitEmpty.Policy),
+		},
+		OmitZero: serialization.OmitZeroConfig{
+			Policy: serialization.OmitZeroPolicy(cfg.OmitZero.Policy),
+		},
+	})
+
 	a := &analyzer{
-		pointerPolicy:     cfg.Pointers.Policy,
-		pointerPreference: cfg.Pointers.Preference,
-		omitEmptyPolicy:   cfg.OmitEmpty.Policy,
-		omitZeroPolicy:    cfg.OmitZero.Policy,
+		serializationCheck: serializationCheck,
 	}
 
 	return &analysis.Analyzer{
@@ -99,9 +106,6 @@ func (a *analyzer) checkField(pass *analysis.Pass, field *ast.Field, markersAcce
 	}
 
 	fieldMarkers := markersAccess.FieldMarkers(field)
-
-	fieldName := field.Names[0].Name
-
 	if !isFieldOptional(fieldMarkers) {
 		// The field is not marked optional, so we don't need to check it.
 		return
@@ -112,7 +116,7 @@ func (a *analyzer) checkField(pass *analysis.Pass, field *ast.Field, markersAcce
 		return
 	}
 
-	a.checkFieldProperties(pass, field, fieldName, markersAccess, jsonTags)
+	a.serializationCheck.Check(pass, field, markersAccess, jsonTags)
 }
 
 func defaultConfig(cfg *OptionalFieldsConfig) {
@@ -133,158 +137,7 @@ func defaultConfig(cfg *OptionalFieldsConfig) {
 	}
 }
 
-func (a *analyzer) checkFieldProperties(pass *analysis.Pass, field *ast.Field, fieldName string, markersAccess markershelper.Markers, jsonTags extractjsontags.FieldTagInfo) {
-	hasValidZeroValue, completeValidation := utils.IsZeroValueValid(pass, field, field.Type, markersAccess, a.omitZeroPolicy != OptionalFieldsOmitZeroPolicyForbid)
-	hasOmitEmpty := jsonTags.OmitEmpty
-	hasOmitZero := jsonTags.OmitZero
-	isPointer, underlying := isStarExpr(field.Type)
-	isStruct := utils.IsStructType(pass, field.Type)
-
-	if a.pointerPreference == 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)
-
-		return
-	}
-
-	// The pointer preference is now when required.
-	// Validate for omitzero policy.
-	if a.omitZeroPolicy != OptionalFieldsOmitZeroPolicyForbid {
-		// If we require omitzero, we can check the field properties based on it being an omitzero field.
-		a.checkFieldPropertiesWithOmitZeroRequired(pass, field, fieldName, jsonTags, hasOmitZero, isPointer, isStruct, hasValidZeroValue)
-	} else {
-		// when the omitzero policy is set to forbid, we need to report removing omitzero if set on the struct fields.
-		a.checkFieldPropertiesWithOmitZeroForbidPolicy(pass, field, fieldName, isStruct, hasOmitZero, jsonTags)
-	}
-
-	// The pointer preference is now when required.
-	// Validate for omitempty policy.
-	if a.omitEmptyPolicy != 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)
-	} else {
-		// The field does not have omitempty, and does not require it.
-		a.checkFieldPropertiesWithoutOmitEmpty(pass, field, fieldName, jsonTags, underlying, hasValidZeroValue, completeValidation, isPointer, isStruct)
-	}
-}
-
-func (a *analyzer) checkFieldPropertiesWithOmitEmptyRequired(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, underlying ast.Expr, hasOmitEmpty, 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)
-
-	switch {
-	case isStruct && !hasValidZeroValue && a.omitZeroPolicy != OptionalFieldsOmitZeroPolicyForbid:
-		// The struct field need not be pointer if it does not have a valid zero value.
-		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.
-	case hasValidZeroValue, isStruct:
-		// The field validation infers that the zero value is valid, the field needs to be a pointer.
-		// Optional structs with omitempty should always be pointers, else they won't actually be omitted.
-		a.handleFieldShouldBePointer(pass, field, fieldName, isPointer, underlying)
-	case !hasValidZeroValue && completeValidation && !isStruct:
-		// The validation is fully complete, and the zero value is not valid, so we don't need a pointer.
-		a.handleFieldShouldNotBePointer(pass, field, fieldName, isPointer, "field %s is optional and does not allow the zero value. The field does not need to be a pointer.")
-	}
-}
-
-func (a *analyzer) checkFieldPropertiesWithoutOmitEmpty(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, underlying ast.Expr, hasValidZeroValue, completeValidation, isPointer, isStruct 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, 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, false, hasValidZeroValue, completeValidation, isPointer, isStruct)
-	}
-}
-
-func (a *analyzer) checkFieldPropertiesWithOmitZeroRequired(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, hasOmitZero, isPointer, isStruct, hasValidZeroValue bool) {
-	if !isStruct || hasValidZeroValue {
-		return
-	}
-
-	a.handleFieldShouldHaveOmitZero(pass, field, fieldName, hasOmitZero, jsonTags)
-	a.handleFieldShouldNotBePointer(pass, field, fieldName, isPointer, "field %s is optional and does not have a valid zero value. The field does not need to be a pointer.")
-}
-
-func (a *analyzer) checkFieldPropertiesWithOmitZeroForbidPolicy(pass *analysis.Pass, field *ast.Field, fieldName string, isStruct, hasOmitZero bool, jsonTags extractjsontags.FieldTagInfo) {
-	if !isStruct || !hasOmitZero {
-		// Handle omitzero only for struct field having omitZero tag.
-		return
-	}
-
-	reportShouldRemoveOmitZero(pass, field, fieldName, jsonTags)
-}
-
-func (a *analyzer) handleFieldShouldBePointer(pass *analysis.Pass, field *ast.Field, fieldName string, isPointer bool, underlying ast.Expr) {
-	if isPointerType(pass, underlying) {
-		if isPointer {
-			switch a.pointerPolicy {
-			case OptionalFieldsPointerPolicySuggestFix:
-				reportShouldRemovePointer(pass, field, OptionalFieldsPointerPolicySuggestFix, fieldName, "field %s is optional but the underlying type does not need to be a pointer. The pointer should be removed.")
-			case OptionalFieldsPointerPolicyWarn:
-				pass.Reportf(field.Pos(), "field %s is optional but the underlying type does not need to be a pointer. The pointer should be removed.", fieldName)
-			}
-		}
-
-		return
-	}
-
-	if isPointer {
-		return
-	}
-
-	switch a.pointerPolicy {
-	case OptionalFieldsPointerPolicySuggestFix:
-		reportShouldAddPointer(pass, field, OptionalFieldsPointerPolicySuggestFix, fieldName, "field %s is optional and should be a pointer")
-	case OptionalFieldsPointerPolicyWarn:
-		pass.Reportf(field.Pos(), "field %s is optional and should be a pointer", fieldName)
-	}
-}
-
-func (a *analyzer) handleFieldShouldNotBePointer(pass *analysis.Pass, field *ast.Field, fieldName string, isPointer bool, message string) {
-	if !isPointer {
-		return
-	}
-
-	reportShouldRemovePointer(pass, field, a.pointerPolicy, fieldName, message)
-}
-
-func (a *analyzer) handleFieldShouldHaveOmitEmpty(pass *analysis.Pass, field *ast.Field, fieldName string, hasOmitEmpty bool, jsonTags extractjsontags.FieldTagInfo) {
-	if hasOmitEmpty {
-		return
-	}
-
-	reportShouldAddOmitEmpty(pass, field, a.omitEmptyPolicy, fieldName, "field %s is optional and should have the omitempty tag", jsonTags)
-}
-
-func (a *analyzer) handleFieldShouldHaveOmitZero(pass *analysis.Pass, field *ast.Field, fieldName string, hasOmitZero bool, jsonTags extractjsontags.FieldTagInfo) {
-	if hasOmitZero {
-		return
-	}
-	// Currently, add omitzero tags to only struct fields.
-	reportShouldAddOmitZero(pass, field, a.omitZeroPolicy, fieldName, "field %s is optional and does not allow the zero value. It must have the omitzero tag.", jsonTags)
-}
-
-func (a *analyzer) handleIncompleteFieldValidation(pass *analysis.Pass, field *ast.Field, fieldName string, isPointer bool, underlying ast.Expr) {
-	if isPointer || isPointerType(pass, underlying) {
-		// Don't warn them if the field is already a pointer.
-		// If they change the validation then they'll fall into the correct logic for categorizing the field.
-		// When the field is a pointer type (e.g. map, array), we don't need to warn them either as they should not make these fields pointers.
-		return
-	}
-
-	zeroValue := utils.GetTypedZeroValue(pass, underlying)
-	validationHint := utils.GetTypedValidationHint(pass, underlying)
-
-	pass.Reportf(field.Pos(), "field %s is optional and has a valid zero value (%s), but the validation is not complete (e.g. %s). The field should be a pointer to allow the zero value to be set. If the zero value is not a valid use case, complete the validation and remove the pointer.", fieldName, zeroValue, validationHint)
+// isFieldOptional checks if a field has an optional marker.
+func isFieldOptional(fieldMarkers markershelper.MarkerSet) bool {
+	return fieldMarkers.Has(markers.OptionalMarker) || fieldMarkers.Has(markers.KubebuilderOptionalMarker)
 }

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

@@ -19,36 +19,36 @@ type A struct {
 
 	// string is a string field.
 	// +optional
-	String string `json:"string,omitempty"` // want "field String is optional and should be a pointer"
+	String string `json:"string,omitempty"` // want "field String should be a pointer."
 
 	// NonOmittedString is a string field without omitempty
 	// +optional
-	NonOmittedString string `json:"nonOmittedString"` // want "field NonOmittedString is optional and should be a pointer" "field NonOmittedString is optional and should have the omitempty tag"
+	NonOmittedString string `json:"nonOmittedString"` // want "field NonOmittedString should be a pointer." "field NonOmittedString should have the omitempty tag."
 
 	// int is an int field.
 	// +optional
-	Int int `json:"int,omitempty"` // want "field Int is optional and should be a pointer"
+	Int int `json:"int,omitempty"` // want "field Int should be a pointer."
 
 	// nonOmittedInt is an int field without omitempty
 	// +optional
-	NonOmittedInt int `json:"nonOmittedInt"` // want "field NonOmittedInt is optional and should be a pointer" "field NonOmittedInt is optional and should have the omitempty tag"
+	NonOmittedInt int `json:"nonOmittedInt"` // want "field NonOmittedInt should be a pointer." "field NonOmittedInt should have the omitempty tag."
 
 	// struct is a struct field.
 	// +optional
-	Struct B `json:"struct,omitempty"` // want "field Struct is optional and should be a pointer"
+	Struct B `json:"struct,omitempty"` // want "field Struct should be a pointer."
 
 	// nonOmittedStruct is a struct field without omitempty.
 	// +optional
-	NonOmittedStruct B `json:"nonOmittedStruct"` // want "field NonOmittedStruct is optional and should be a pointer" "field NonOmittedStruct is optional and should have the omitempty tag"
+	NonOmittedStruct B `json:"nonOmittedStruct"` // want "field NonOmittedStruct should be a pointer." "field NonOmittedStruct should have the omitempty tag."
 
 	// structWithMinProperties is a struct field with a minimum number of properties.
 	// +kubebuilder:validation:MinProperties=1
 	// +optional
-	StructWithMinProperties B `json:"structWithMinProperties,omitempty"` // want "field StructWithMinProperties is optional and should be a pointer"
+	StructWithMinProperties B `json:"structWithMinProperties,omitempty"` // want "field StructWithMinProperties should be a pointer."
 
 	// 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"
+	StructWithMinPropertiesOnStruct D `json:"structWithMinPropertiesOnStruct,omitempty"` // want "field StructWithMinPropertiesOnStruct should be a pointer."
 
 	// slice is a slice field.
 	// +optional
@@ -60,15 +60,15 @@ type A struct {
 
 	// PointerSlice is a pointer slice field.
 	// +optional
-	PointerSlice *[]string `json:"pointerSlice,omitempty"` // want "field PointerSlice is optional but the underlying type does not need to be a pointer. The pointer should be removed."
+	PointerSlice *[]string `json:"pointerSlice,omitempty"` // want "field PointerSlice underlying type does not need to be a pointer. The pointer should be removed."
 
 	// PointerMap is a pointer map field.
 	// +optional
-	PointerMap *map[string]string `json:"pointerMap,omitempty"` // want "field PointerMap is optional but the underlying type does not need to be a pointer. The pointer should be removed."
+	PointerMap *map[string]string `json:"pointerMap,omitempty"` // want "field PointerMap underlying type does not need to be a pointer. The pointer should be removed."
 
 	// PointerPointerString is a double pointer string field.
 	// +optional
-	DoublePointerString **string `json:"doublePointerString,omitempty"` // want "field DoublePointerString is optional but the underlying type does not need to be a pointer. The pointer should be removed."
+	DoublePointerString **string `json:"doublePointerString,omitempty"` // want "field DoublePointerString underlying type does not need to be a pointer. The pointer should be removed."
 
 	// PointerStringAlias is a pointer string alias field.
 	// +optional
@@ -93,16 +93,16 @@ type A struct {
 
 	// PointerSliceAlias is a pointer slice alias field.
 	// +optional
-	PointerSliceAlias *SliceAlias `json:"pointerSliceAlias,omitempty"` // want "field PointerSliceAlias is optional but the underlying type does not need to be a pointer. The pointer should be removed."
+	PointerSliceAlias *SliceAlias `json:"pointerSliceAlias,omitempty"` // want "field PointerSliceAlias underlying type does not need to be a pointer. The pointer should be removed."
 
 	// PointerMapAlias is a pointer map alias field.
 	// +optional
-	PointerMapAlias *MapAlias `json:"pointerMapAlias,omitempty"` // want "field PointerMapAlias is optional but the underlying type does not need to be a pointer. The pointer should be removed."
+	PointerMapAlias *MapAlias `json:"pointerMapAlias,omitempty"` // want "field PointerMapAlias underlying type does not need to be a pointer. The pointer should be removed."
 
 	// StringAliasWithEnum is a string alias field with enum validation.
 	// With the "Always" pointer preference, optional fields should be pointers regardless of zero value validity.
 	// +optional
-	StringAliasWithEnum StringAliasWithEnum `json:"stringAliasWithEnum,omitempty"` // want "field StringAliasWithEnum is optional and should be a pointer"
+	StringAliasWithEnum StringAliasWithEnum `json:"stringAliasWithEnum,omitempty"` // want "field StringAliasWithEnum should be a pointer."
 
 	// StringAliasWithEnumPointer is a pointer string alias field with enum validation.
 	// This is correctly a pointer since the zero value is not valid.
@@ -111,7 +111,7 @@ type A struct {
 
 	// StringAliasWithEnumNoOmitEmpty is a string alias field with enum validation and no omitempty.
 	// +optional
-	StringAliasWithEnumNoOmitEmpty *StringAliasWithEnum `json:"stringAliasWithEnumNoOmitEmpty"` // want "field StringAliasWithEnumNoOmitEmpty is optional and should have the omitempty tag"
+	StringAliasWithEnumNoOmitEmpty *StringAliasWithEnum `json:"stringAliasWithEnumNoOmitEmpty"` // want "field StringAliasWithEnumNoOmitEmpty should have the omitempty tag."
 }
 
 type B struct {