Allow disabling fix for required fields pointer

JoelSpeed · JoelSpeed · commit 2fd663251e1b · 2024-12-06T20:29:41.000+07:00
diff --git a/pkg/analysis/requiredfields/analyzer.go b/pkg/analysis/requiredfields/analyzer.go
@@ -8,6 +8,7 @@ import (
 
 	"github.com/JoelSpeed/kal/pkg/analysis/helpers/extractjsontags"
 	"github.com/JoelSpeed/kal/pkg/analysis/helpers/markers"
+	"github.com/JoelSpeed/kal/pkg/config"
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/passes/inspect"
 	"golang.org/x/tools/go/ast/inspector"
@@ -26,11 +27,17 @@ var (
 	errCouldNotGetJSONTags  = errors.New("could not get json tags")
 )
 
-type analyzer struct{}
+type analyzer struct {
+	pointerPolicy config.RequiredFieldPointerPolicy
+}
 
 // newAnalyzer creates a new analyzer.
-func newAnalyzer() *analysis.Analyzer {
-	a := &analyzer{}
+func newAnalyzer(cfg config.RequiredFieldsConfig) *analysis.Analyzer {
+	defaultConfig(&cfg)
+
+	a := &analyzer{
+		pointerPolicy: cfg.PointerPolicy,
+	}
 
 	return &analysis.Analyzer{
 		Name:     name,
@@ -124,21 +131,34 @@ func (a *analyzer) checkField(pass *analysis.Pass, field *ast.Field, fieldMarker
 	}
 
 	if starExpr, ok := field.Type.(*ast.StarExpr); ok {
-		pass.Report(analysis.Diagnostic{
-			Pos:     field.Pos(),
-			Message: fmt.Sprintf("field %s is marked as required, should not be a pointer", fieldName),
-			SuggestedFixes: []analysis.SuggestedFix{
-				{
-					Message: "should remove the pointer",
-					TextEdits: []analysis.TextEdit{
-						{
-							Pos:     starExpr.Pos(),
-							End:     starExpr.X.Pos(),
-							NewText: nil,
-						},
+		var suggestedFixes []analysis.SuggestedFix
+
+		switch a.pointerPolicy {
+		case config.RequiredFieldPointerWarn:
+			// Do not suggest a fix.
+		case config.RequiredFieldPointerSuggestFix:
+			suggestedFixes = append(suggestedFixes, analysis.SuggestedFix{
+				Message: "should remove the pointer",
+				TextEdits: []analysis.TextEdit{
+					{
+						Pos:     starExpr.Pos(),
+						End:     starExpr.X.Pos(),
+						NewText: nil,
 					},
 				},
-			},
+			})
+		}
+
+		pass.Report(analysis.Diagnostic{
+			Pos:            field.Pos(),
+			Message:        fmt.Sprintf("field %s is marked as required, should not be a pointer", fieldName),
+			SuggestedFixes: suggestedFixes,
 		})
 	}
 }
+
+func defaultConfig(cfg *config.RequiredFieldsConfig) {
+	if cfg.PointerPolicy == "" {
+		cfg.PointerPolicy = config.RequiredFieldPointerSuggestFix
+	}
+}
diff --git a/pkg/analysis/requiredfields/analyzer_test.go b/pkg/analysis/requiredfields/analyzer_test.go
@@ -18,3 +18,18 @@ func TestDefaultConfiguration(t *testing.T) {
 
 	analysistest.RunWithSuggestedFixes(t, testdata, a, "a")
 }
+
+func TestWithPointerPolicyWarn(t *testing.T) {
+	testdata := analysistest.TestData()
+
+	a, err := requiredfields.Initializer().Init(config.LintersConfig{
+		RequiredFields: config.RequiredFieldsConfig{
+			PointerPolicy: config.RequiredFieldPointerWarn,
+		},
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	analysistest.RunWithSuggestedFixes(t, testdata, a, "b")
+}
diff --git a/pkg/analysis/requiredfields/initializer.go b/pkg/analysis/requiredfields/initializer.go
@@ -21,7 +21,7 @@ func (initializer) Name() string {
 
 // Init returns the intialized Analyzer.
 func (initializer) Init(cfg config.LintersConfig) (*analysis.Analyzer, error) {
-	return newAnalyzer(), nil
+	return newAnalyzer(cfg.RequiredFields), nil
 }
 
 // Default determines whether this Analyzer is on by default, or not.
diff --git a/pkg/analysis/requiredfields/testdata/src/b/a.go b/pkg/analysis/requiredfields/testdata/src/b/a.go
@@ -0,0 +1,39 @@
+package a
+
+type A struct {
+	// optional field should not be picked up.
+	// +optional
+	OptionalField *string `json:"optionalField,omitempty"`
+
+	// requiredCorrectField should not be picked up.
+	// +required
+	RequiredCorrectField string `json:"requiredCorrectField"`
+
+	// requiredOmitEmptyField field should be picked up.
+	// +required
+	RequiredOmitEmptyField string `json:"requiredOmitEmptyField,omitempty"` // want "field RequiredOmitEmptyField is marked as required, but has the omitempty tag"
+
+	// requiredPointerField should be picked up.
+	// +required
+	RequiredPointerField *string `json:"requiredPointerField"` // want "field RequiredPointerField is marked as required, should not be a pointer"
+
+	// requiredPointerOmitEmptyField should be picked up.
+	// +required
+	RequiredPointerOmitEmptyField *string `json:"requiredPointerOmitEmptyField,omitempty"` // want "field RequiredPointerOmitEmptyField is marked as required, but has the omitempty tag" "field RequiredPointerOmitEmptyField is marked as required, should not be a pointer"
+
+	// requiredKubebuilderMarkerField should not be picked up.
+	// +kubebuilder:validation:Required
+	RequiredKubebuilderMarkerField string `json:"requiredKubebuilderMarkerField"`
+
+	// requiredKubebuilderMarkerOmitEmptyField should be picked up.
+	// +kubebuilder:validation:Required
+	RequiredKubebuilderMarkerOmitEmptyField string `json:"requiredKubebuilderMarkerOmitEmptyField,omitempty"` // want "field RequiredKubebuilderMarkerOmitEmptyField is marked as required, but has the omitempty tag"
+
+	// requiredKubebuilderMarkerPointerField should be picked up.
+	// +kubebuilder:validation:Required
+	RequiredKubebuilderMarkerPointerField *string `json:"requiredKubebuilderMarkerPointerField"` // want "field RequiredKubebuilderMarkerPointerField is marked as required, should not be a pointer"
+
+	// requiredKubebuilderMarkerPointerOmitEmptyField should be picked up.
+	// +kubebuilder:validation:Required
+	RequiredKubebuilderMarkerPointerOmitEmptyField *string `json:"requiredKubebuilderMarkerPointerOmitEmptyField,omitempty"` // want "field RequiredKubebuilderMarkerPointerOmitEmptyField is marked as required, but has the omitempty tag" "field RequiredKubebuilderMarkerPointerOmitEmptyField is marked as required, should not be a pointer"
+}
diff --git a/pkg/analysis/requiredfields/testdata/src/b/a.go.golden b/pkg/analysis/requiredfields/testdata/src/b/a.go.golden
@@ -0,0 +1,39 @@
+package a
+
+type A struct {
+	// optional field should not be picked up.
+	// +optional
+	OptionalField *string `json:"optionalField,omitempty"`
+
+	// requiredCorrectField should not be picked up.
+	// +required
+	RequiredCorrectField string `json:"requiredCorrectField"`
+
+	// requiredOmitEmptyField field should be picked up.
+	// +required
+	RequiredOmitEmptyField string `json:"requiredOmitEmptyField"` // want "field RequiredOmitEmptyField is marked as required, but has the omitempty tag"
+
+	// requiredPointerField should be picked up.
+	// +required
+	RequiredPointerField *string `json:"requiredPointerField"` // want "field RequiredPointerField is marked as required, should not be a pointer"
+
+	// requiredPointerOmitEmptyField should be picked up.
+	// +required
+	RequiredPointerOmitEmptyField *string `json:"requiredPointerOmitEmptyField"` // want "field RequiredPointerOmitEmptyField is marked as required, but has the omitempty tag" "field RequiredPointerOmitEmptyField is marked as required, should not be a pointer"
+
+	// requiredKubebuilderMarkerField should not be picked up.
+	// +kubebuilder:validation:Required
+	RequiredKubebuilderMarkerField string `json:"requiredKubebuilderMarkerField"`
+
+	// requiredKubebuilderMarkerOmitEmptyField should be picked up.
+	// +kubebuilder:validation:Required
+	RequiredKubebuilderMarkerOmitEmptyField string `json:"requiredKubebuilderMarkerOmitEmptyField"` // want "field RequiredKubebuilderMarkerOmitEmptyField is marked as required, but has the omitempty tag"
+
+	// requiredKubebuilderMarkerPointerField should be picked up.
+	// +kubebuilder:validation:Required
+	RequiredKubebuilderMarkerPointerField *string `json:"requiredKubebuilderMarkerPointerField"` // want "field RequiredKubebuilderMarkerPointerField is marked as required, should not be a pointer"
+
+	// requiredKubebuilderMarkerPointerOmitEmptyField should be picked up.
+	// +kubebuilder:validation:Required
+	RequiredKubebuilderMarkerPointerOmitEmptyField *string `json:"requiredKubebuilderMarkerPointerOmitEmptyField"` // want "field RequiredKubebuilderMarkerPointerOmitEmptyField is marked as required, but has the omitempty tag" "field RequiredKubebuilderMarkerPointerOmitEmptyField is marked as required, should not be a pointer"
+}
diff --git a/pkg/config/linters_config.go b/pkg/config/linters_config.go
@@ -7,6 +7,9 @@ type LintersConfig struct {
 
 	// optionalOrRequired contains configuration for the optionalorrequired linter.
 	OptionalOrRequired OptionalOrRequiredConfig `json:"optionalOrRequired"`
+
+	// requiredFields contains configuration for the requiredfields linter.
+	RequiredFields RequiredFieldsConfig `json:"requiredFields"`
 }
 
 // JSONTagsConfig contains configuration for the jsontags linter.
@@ -29,3 +32,24 @@ type OptionalOrRequiredConfig struct {
 	// Valid values are "required" and "kubebuilder:validation:Required".
 	PreferredRequiredMarker string `json:"preferredRequiredMarker"`
 }
+
+// RequiredFieldPointerPolicy is the policy for pointers in required fields.
+type RequiredFieldPointerPolicy string
+
+const (
+	// RequiredFieldPointerWarn indicates that the linter will emit a warning if a required field is a pointer.
+	RequiredFieldPointerWarn RequiredFieldPointerPolicy = "Warn"
+
+	// RequiredFieldPointerSuggestFix indicates that the linter will emit a warning if a required field is a pointer and suggest a fix.
+	RequiredFieldPointerSuggestFix RequiredFieldPointerPolicy = "SuggestFix"
+)
+
+// RequiredFieldsConfig contains configuration for the requiredfields linter.
+type RequiredFieldsConfig struct {
+	// pointerPolicy is the policy for pointers in required fields.
+	// Valid values are "Warn" and "SuggestFix".
+	// When set to "Warn", the linter will emit a warning if a required field is a pointer.
+	// When set to "SuggestFix", the linter will emit a warning if a required field is a pointer and suggest a fix.
+	// When otherwise not specified, the default value is "SuggestFix".
+	PointerPolicy RequiredFieldPointerPolicy `json:"pointerPolicy"`
+}
diff --git a/pkg/validation/linters_config.go b/pkg/validation/linters_config.go
@@ -16,6 +16,7 @@ func ValidateLintersConfig(lc config.LintersConfig, fldPath *field.Path) field.E
 
 	fieldErrors = append(fieldErrors, validateJSONTagsConfig(lc.JSONTags, fldPath.Child("jsonTags"))...)
 	fieldErrors = append(fieldErrors, validateOptionalOrRequiredConfig(lc.OptionalOrRequired, fldPath.Child("optionalOrRequired"))...)
+	fieldErrors = append(fieldErrors, validateRequiredFieldsConfig(lc.RequiredFields, fldPath.Child("requiredFields"))...)
 
 	return fieldErrors
 }
@@ -51,3 +52,16 @@ func validateOptionalOrRequiredConfig(oorc config.OptionalOrRequiredConfig, fldP
 
 	return fieldErrors
 }
+
+// validateRequiredFieldsConfig is used to validate the configuration in the config.RequiredFieldsConfig struct.
+func validateRequiredFieldsConfig(rfc config.RequiredFieldsConfig, fldPath *field.Path) field.ErrorList {
+	fieldErrors := field.ErrorList{}
+
+	switch rfc.PointerPolicy {
+	case "", config.RequiredFieldPointerWarn, config.RequiredFieldPointerSuggestFix:
+	default:
+		fieldErrors = append(fieldErrors, field.Invalid(fldPath.Child("pointerPolicy"), rfc.PointerPolicy, fmt.Sprintf("invalid value, must be one of %q, %q or omitted", config.RequiredFieldPointerWarn, config.RequiredFieldPointerSuggestFix)))
+	}
+
+	return fieldErrors
+}
diff --git a/pkg/validation/linters_config_test.go b/pkg/validation/linters_config_test.go
@@ -83,5 +83,39 @@ var _ = Describe("LintersConfig", func() {
 			},
 			expectedErr: "lintersConfig.optionalOrRequired.preferredRequiredMarker: Invalid value: \"invalid\": invalid value, must be one of \"required\", \"kubebuilder:validation:Required\" or omitted",
 		}),
+
+		// RequiredFieldsConfig validation
+		Entry("With a valid RequiredFieldsConfig: omitted", validateLintersConfigTableInput{
+			config: config.LintersConfig{
+				RequiredFields: config.RequiredFieldsConfig{
+					PointerPolicy: "",
+				},
+			},
+			expectedErr: "",
+		}),
+		Entry("With a valid RequiredFieldsConfig: SuggestFix", validateLintersConfigTableInput{
+			config: config.LintersConfig{
+				RequiredFields: config.RequiredFieldsConfig{
+					PointerPolicy: config.RequiredFieldPointerSuggestFix,
+				},
+			},
+			expectedErr: "",
+		}),
+		Entry("With a valid RequiredFieldsConfig: Warn", validateLintersConfigTableInput{
+			config: config.LintersConfig{
+				RequiredFields: config.RequiredFieldsConfig{
+					PointerPolicy: config.RequiredFieldPointerWarn,
+				},
+			},
+			expectedErr: "",
+		}),
+		Entry("With an invalid RequiredFieldsConfig", validateLintersConfigTableInput{
+			config: config.LintersConfig{
+				RequiredFields: config.RequiredFieldsConfig{
+					PointerPolicy: "invalid",
+				},
+			},
+			expectedErr: "lintersConfig.requiredFields.pointerPolicy: Invalid value: \"invalid\": invalid value, must be one of \"Warn\", \"SuggestFix\" or omitted",
+		}),
 	)
 })

Original file line number	Diff line number	Diff line change
`@@ -21,7 +21,7 @@ func (initializer) Name() string {`
`21`	`21`
`22`	`22`	`// Init returns the intialized Analyzer.`
`23`	`23`	`func (initializer) Init(cfg config.LintersConfig) (*analysis.Analyzer, error) {`
`24`		`- return newAnalyzer(), nil`
	`24`	`+ return newAnalyzer(cfg.RequiredFields), nil`
`25`	`25`	`}`
`26`	`26`
`27`	`27`	`// Default determines whether this Analyzer is on by default, or not.`