Skip to content

Commit 8ade98c

Browse files
JoelSpeedJoelSpeed
committed
Rebase required fields linter on serialization checker
1 parent bdb40c6 commit 8ade98c

File tree

19 files changed

+1508
-193
lines changed

19 files changed

+1508
-193
lines changed

‎pkg/analysis/requiredfields/analyzer.go‎

Lines changed: 55 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -16,16 +16,14 @@ limitations under the License.
1616
package requiredfields
1717

1818
import (
19-
"fmt"
2019
"go/ast"
21-
"strings"
2220

2321
"golang.org/x/tools/go/analysis"
2422
kalerrors "sigs.k8s.io/kube-api-linter/pkg/analysis/errors"
2523
"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/extractjsontags"
2624
"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/inspector"
2725
markershelper "sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/markers"
28-
"sigs.k8s.io/kube-api-linter/pkg/analysis/utils"
26+
"sigs.k8s.io/kube-api-linter/pkg/analysis/utils/serialization"
2927
"sigs.k8s.io/kube-api-linter/pkg/markers"
3028
)
3129

@@ -34,11 +32,19 @@ const (
3432
)
3533

3634
func init() {
37-
markershelper.DefaultRegistry().Register(markers.RequiredMarker, markers.KubebuilderRequiredMarker)
35+
markershelper.DefaultRegistry().Register(
36+
markers.RequiredMarker,
37+
markers.KubebuilderRequiredMarker,
38+
markers.KubebuilderMinItemsMarker,
39+
markers.KubebuilderMinLengthMarker,
40+
markers.KubebuilderMinPropertiesMarker,
41+
markers.KubebuilderMinimumMarker,
42+
markers.KubebuilderEnumMarker,
43+
)
3844
}
3945

4046
type analyzer struct {
41-
pointerPolicy RequiredFieldPointerPolicy
47+
serializationCheck serialization.SerializationCheck
4248
}
4349

4450
// newAnalyzer creates a new analyzer.
@@ -49,15 +55,34 @@ func newAnalyzer(cfg *RequiredFieldsConfig) *analysis.Analyzer {
4955

5056
defaultConfig(cfg)
5157

58+
serializationCheck := serialization.New(&serialization.Config{
59+
Pointers: serialization.PointersConfig{
60+
Policy: serialization.PointersPolicy(cfg.Pointers.Policy),
61+
// We only allow the WhenRequired preference for required fields.
62+
// This works for both built-in types and custom resources, and
63+
// avoids pointers unless absolutely necessary.
64+
Preference: serialization.PointersPreferenceWhenRequired,
65+
},
66+
OmitEmpty: serialization.OmitEmptyConfig{
67+
Policy: serialization.OmitEmptyPolicy(cfg.OmitEmpty.Policy),
68+
},
69+
OmitZero: serialization.OmitZeroConfig{
70+
Policy: serialization.OmitZeroPolicy(cfg.OmitZero.Policy),
71+
},
72+
})
73+
5274
a := &analyzer{
53-
pointerPolicy: cfg.PointerPolicy,
75+
serializationCheck: serializationCheck,
5476
}
5577

5678
return &analysis.Analyzer{
57-
Name: name,
58-
Doc: "Checks that all required fields are not pointers, and do not have the omitempty tag.",
79+
Name: name,
80+
Doc: `Checks that all required fields are serialized correctly.
81+
Where the zero value is not valid, this means the field should not be a pointer, and should have the omitempty tag.
82+
Where the zero value is valid, this means the field should be a pointer and should not have the omitempty tag.
83+
`,
5984
Run: a.run,
60-
Requires: []*analysis.Analyzer{inspector.Analyzer},
85+
Requires: []*analysis.Analyzer{inspector.Analyzer, extractjsontags.Analyzer},
6186
}
6287
}
6388

@@ -68,76 +93,46 @@ func (a *analyzer) run(pass *analysis.Pass) (any, error) {
6893
}
6994

7095
inspect.InspectFields(func(field *ast.Field, stack []ast.Node, jsonTagInfo extractjsontags.FieldTagInfo, markersAccess markershelper.Markers) {
71-
a.checkField(pass, field, markersAccess.FieldMarkers(field), jsonTagInfo)
96+
a.checkField(pass, field, markersAccess, jsonTagInfo)
7297
})
7398

7499
return nil, nil //nolint:nilnil
75100
}
76101

77-
func (a *analyzer) checkField(pass *analysis.Pass, field *ast.Field, fieldMarkers markershelper.MarkerSet, fieldTagInfo extractjsontags.FieldTagInfo) {
78-
fieldName := utils.FieldName(field)
79-
if fieldName == "" {
102+
func (a *analyzer) checkField(pass *analysis.Pass, field *ast.Field, markersAccess markershelper.Markers, jsonTags extractjsontags.FieldTagInfo) {
103+
if field == nil || len(field.Names) == 0 {
80104
return
81105
}
82106

83-
if !fieldMarkers.Has(markers.RequiredMarker) && !fieldMarkers.Has(markers.KubebuilderRequiredMarker) {
107+
fieldMarkers := markersAccess.FieldMarkers(field)
108+
if !isFieldRequired(fieldMarkers) {
84109
// The field is not marked required, so we don't need to check it.
85110
return
86111
}
87112

88-
if fieldTagInfo.OmitEmpty {
89-
pass.Report(analysis.Diagnostic{
90-
Pos: field.Pos(),
91-
Message: fmt.Sprintf("field %s is marked as required, but has the omitempty tag", fieldName),
92-
SuggestedFixes: []analysis.SuggestedFix{
93-
{
94-
Message: "should remove the omitempty tag",
95-
TextEdits: []analysis.TextEdit{
96-
{
97-
Pos: fieldTagInfo.Pos,
98-
End: fieldTagInfo.End,
99-
NewText: []byte(strings.Replace(fieldTagInfo.RawValue, ",omitempty", "", 1)),
100-
},
101-
},
102-
},
103-
},
104-
})
105-
}
106-
107113
if field.Type == nil {
108114
// The field has no type? We can't check if it's a pointer.
109115
return
110116
}
111117

112-
if starExpr, ok := field.Type.(*ast.StarExpr); ok {
113-
var suggestedFixes []analysis.SuggestedFix
114-
115-
switch a.pointerPolicy {
116-
case RequiredFieldPointerWarn:
117-
// Do not suggest a fix.
118-
case RequiredFieldPointerSuggestFix:
119-
suggestedFixes = append(suggestedFixes, analysis.SuggestedFix{
120-
Message: "should remove the pointer",
121-
TextEdits: []analysis.TextEdit{
122-
{
123-
Pos: starExpr.Pos(),
124-
End: starExpr.X.Pos(),
125-
NewText: nil,
126-
},
127-
},
128-
})
129-
}
130-
131-
pass.Report(analysis.Diagnostic{
132-
Pos: field.Pos(),
133-
Message: fmt.Sprintf("field %s is marked as required, should not be a pointer", fieldName),
134-
SuggestedFixes: suggestedFixes,
135-
})
136-
}
118+
a.serializationCheck.Check(pass, field, markersAccess, jsonTags)
137119
}
138120

139121
func defaultConfig(cfg *RequiredFieldsConfig) {
140-
if cfg.PointerPolicy == "" {
141-
cfg.PointerPolicy = RequiredFieldPointerSuggestFix
122+
if cfg.Pointers.Policy == "" {
123+
cfg.Pointers.Policy = RequiredFieldsPointerPolicySuggestFix
142124
}
125+
126+
if cfg.OmitEmpty.Policy == "" {
127+
cfg.OmitEmpty.Policy = RequiredFieldsOmitEmptyPolicySuggestFix
128+
}
129+
130+
if cfg.OmitZero.Policy == "" {
131+
cfg.OmitZero.Policy = RequiredFieldsOmitZeroPolicySuggestFix
132+
}
133+
}
134+
135+
// isFieldRequired checks if a field has an required marker.
136+
func isFieldRequired(fieldMarkers markershelper.MarkerSet) bool {
137+
return fieldMarkers.Has(markers.RequiredMarker) || fieldMarkers.Has(markers.KubebuilderRequiredMarker)
143138
}

‎pkg/analysis/requiredfields/analyzer_test.go‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -32,16 +32,3 @@ func TestDefaultConfiguration(t *testing.T) {
3232

3333
analysistest.RunWithSuggestedFixes(t, testdata, a, "a")
3434
}
35-
36-
func TestWithPointerPolicyWarn(t *testing.T) {
37-
testdata := analysistest.TestData()
38-
39-
a, err := requiredfields.Initializer().Init(&requiredfields.RequiredFieldsConfig{
40-
PointerPolicy: requiredfields.RequiredFieldPointerWarn,
41-
})
42-
if err != nil {
43-
t.Fatal(err)
44-
}
45-
46-
analysistest.RunWithSuggestedFixes(t, testdata, a, "b")
47-
}

‎pkg/analysis/requiredfields/config.go‎

Lines changed: 90 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -15,23 +15,98 @@ limitations under the License.
1515
*/
1616
package requiredfields
1717

18-
// RequiredFieldPointerPolicy is the policy for pointers in required fields.
19-
type RequiredFieldPointerPolicy string
18+
// RequiredFieldsConfig contains configuration for the requiredfields linter.
19+
type RequiredFieldsConfig struct {
20+
// pointers is the policy for pointers in required fields.
21+
// This will allow configuration of whether or not to suggest fixes for pointers in required fields,
22+
// or just warn about their absence.
23+
// Pointers on required fields are only recommended when the zero value is a valid user choice.
24+
Pointers RequiredFieldsPointers `json:"pointers"`
2025

21-
const (
22-
// RequiredFieldPointerWarn indicates that the linter will emit a warning if a required field is a pointer.
23-
RequiredFieldPointerWarn RequiredFieldPointerPolicy = "Warn"
26+
// omitempty is the policy for the `omitempty` tag on required fields.
27+
// This will allow configuration of whether or not to suggest fixes for the `omitempty` tag on required fields,
28+
// or just warn about their absence.
29+
// Required fields should have the `omitempty` tag to allow structured clients to make an explicit choice
30+
// to include the field.
31+
OmitEmpty RequiredFieldsOmitEmpty `json:"omitempty"`
2432

25-
// RequiredFieldPointerSuggestFix indicates that the linter will emit a warning if a required field is a pointer and suggest a fix.
26-
RequiredFieldPointerSuggestFix RequiredFieldPointerPolicy = "SuggestFix"
27-
)
33+
// omitzero is the policy for the `omitzero` tag within the json tag for fields.
34+
// This defines how the linter should handle optional fields, and whether they should have the omitzero tag or not.
35+
// By default, all the struct fields will be expected to have the `omitzero` tag when their zero value is not an acceptable user choice.
36+
// Note, `omitzero` tag is supported in go version starting from go 1.24.
37+
// Note, Configure omitzero policy to 'Forbid', if using with go version less than go 1.24.
38+
OmitZero RequiredFieldsOmitZero `json:"omitzero"`
39+
}
2840

29-
// RequiredFieldsConfig contains configuration for the requiredfields linter.
30-
type RequiredFieldsConfig struct {
31-
// pointerPolicy is the policy for pointers in required fields.
32-
// Valid values are "Warn" and "SuggestFix".
33-
// When set to "Warn", the linter will emit a warning if a required field is a pointer.
34-
// When set to "SuggestFix", the linter will emit a warning if a required field is a pointer and suggest a fix.
41+
// RequiredFieldsPointers is the configuration for pointers in required fields.
42+
type RequiredFieldsPointers struct {
43+
// policy is the policy for the pointer preferences for required fields.
44+
// Valid values are "SuggestFix" and "Warn".
45+
// When set to "SuggestFix", the linter will emit a warning and suggest a fix if the field is a pointer and doesn't need to be, or, where it needs to be a pointer, but isn't.
46+
// When set to "Warn", the linter will emit a warning per the above, but without suggesting a fix.
3547
// When otherwise not specified, the default value is "SuggestFix".
36-
PointerPolicy RequiredFieldPointerPolicy `json:"pointerPolicy"`
48+
Policy RequiredFieldsPointerPolicy `json:"policy"`
3749
}
50+
51+
// RequiredFieldsOmitEmpty is the configuration for the `omitempty` tag on required fields.
52+
type RequiredFieldsOmitEmpty struct {
53+
// policy is the policy for the `omitempty` tag on required fields.
54+
// Valid values are "SuggestFix", "Warn" and "Ignore".
55+
// When set to "SuggestFix", the linter will suggest adding the `omitempty` tag when an required field does not have it.
56+
// When set to "Warn", the linter will emit a warning if the field does not have the `omitempty` tag.
57+
// When set to "Ignore", a required field missing the `omitempty` tag will be ignored.
58+
// Note, when set to "Ignore", and a field does not have the `omitempty` tag, this may affect whether the field should be a pointer or not.
59+
Policy RequiredFieldsOmitEmptyPolicy `json:"policy"`
60+
}
61+
62+
// RequiredFieldsOmitZero is the configuration for the `omitzero` tag on required fields.
63+
type RequiredFieldsOmitZero struct {
64+
// policy is the policy for the `omitzero` tag on required fields.
65+
// Valid values are "SuggestFix", "Warn" and "Forbid".
66+
// When set to "SuggestFix", the linter will suggest adding the `omitzero` tag when an required field does not have it.
67+
// When set to "Warn", the linter will emit a warning if the field does not have the `omitzero` tag.
68+
// When set to "Forbid", 'omitzero' tags wont be considered.
69+
// Note, when set to "Forbid", and a field have the `omitzero` tag, the linter will suggest to remove the `omitzero` tag.
70+
// Note, `omitzero` tag is supported in go version starting from go 1.24.
71+
// Note, Configure omitzero policy to 'Forbid', if using with go version less than go 1.24.
72+
Policy RequiredFieldsOmitZeroPolicy `json:"policy"`
73+
}
74+
75+
// RequiredFieldsPointerPolicy is the policy for pointers in required fields.
76+
type RequiredFieldsPointerPolicy string
77+
78+
const (
79+
// RequiredFieldsPointerPolicyWarn indicates that the linter will emit a warning if a required field is a pointer.
80+
RequiredFieldsPointerPolicyWarn RequiredFieldsPointerPolicy = "Warn"
81+
82+
// RequiredFieldsPointerPolicySuggestFix indicates that the linter will emit a warning if a required field is a pointer and suggest a fix.
83+
RequiredFieldsPointerPolicySuggestFix RequiredFieldsPointerPolicy = "SuggestFix"
84+
)
85+
86+
// RequiredFieldsOmitEmptyPolicy is the policy for the `omitempty` tag on required fields.
87+
type RequiredFieldsOmitEmptyPolicy string
88+
89+
const (
90+
// RequiredFieldsOmitEmptyPolicySuggestFix indicates that the linter will emit a warning if the field does not have omitempty, and suggest a fix.
91+
RequiredFieldsOmitEmptyPolicySuggestFix RequiredFieldsOmitEmptyPolicy = "SuggestFix"
92+
93+
// RequiredFieldsOmitEmptyPolicyWarn indicates that the linter will emit a warning if the field does not have omitempty.
94+
RequiredFieldsOmitEmptyPolicyWarn RequiredFieldsOmitEmptyPolicy = "Warn"
95+
96+
// RequiredFieldsOmitEmptyPolicyIgnore indicates that a required field missing the `omitempty` tag will be ignored.
97+
RequiredFieldsOmitEmptyPolicyIgnore RequiredFieldsOmitEmptyPolicy = "Ignore"
98+
)
99+
100+
// RequiredFieldsOmitZeroPolicy is the policy for the `omitzero` tag on required fields.
101+
type RequiredFieldsOmitZeroPolicy string
102+
103+
const (
104+
// RequiredFieldsOmitZeroPolicySuggestFix indicates that the linter will suggest adding the `omitzero` tag when an required field does not have it.
105+
RequiredFieldsOmitZeroPolicySuggestFix RequiredFieldsOmitZeroPolicy = "SuggestFix"
106+
107+
// RequiredFieldsOmitZeroPolicyWarn indicates that the linter will emit a warning if the field does not have omitzero.
108+
RequiredFieldsOmitZeroPolicyWarn RequiredFieldsOmitZeroPolicy = "Warn"
109+
110+
// RequiredFieldsOmitZeroPolicyForbid indicates that the linter will not consider `omitzero` tags, and will suggest to remove the `omitzero` tag if a field has it.
111+
RequiredFieldsOmitZeroPolicyForbid RequiredFieldsOmitZeroPolicy = "Forbid"
112+
)

‎pkg/analysis/requiredfields/initializer.go‎

Lines changed: 41 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -43,18 +43,55 @@ func initAnalyzer(rfc *RequiredFieldsConfig) (*analysis.Analyzer, error) {
4343
return newAnalyzer(rfc), nil
4444
}
4545

46-
// validateConfig is used to validate the configuration in the config.RequiredFieldsConfig struct.
46+
// validateConfig validates the configuration in the config.RequiredFieldsConfig struct.
4747
func validateConfig(rfc *RequiredFieldsConfig, fldPath *field.Path) field.ErrorList {
4848
if rfc == nil {
4949
return field.ErrorList{}
5050
}
5151

5252
fieldErrors := field.ErrorList{}
5353

54-
switch rfc.PointerPolicy {
55-
case "", RequiredFieldPointerWarn, RequiredFieldPointerSuggestFix:
54+
fieldErrors = append(fieldErrors, validateRequiredFieldsPointers(rfc.Pointers, fldPath.Child("pointers"))...)
55+
fieldErrors = append(fieldErrors, validateRequiredFieldsOmitEmpty(rfc.OmitEmpty, fldPath.Child("omitempty"))...)
56+
fieldErrors = append(fieldErrors, validateRequiredFieldsOmitZero(rfc.OmitZero, fldPath.Child("omitzero"))...)
57+
58+
return fieldErrors
59+
}
60+
61+
// validateRequiredFieldsPointers is used to validate the configuration in the config.RequiredFieldsPointers struct.
62+
func validateRequiredFieldsPointers(rfc RequiredFieldsPointers, fldPath *field.Path) field.ErrorList {
63+
fieldErrors := field.ErrorList{}
64+
65+
switch rfc.Policy {
66+
case "", RequiredFieldsPointerPolicySuggestFix, RequiredFieldsPointerPolicyWarn:
67+
default:
68+
fieldErrors = append(fieldErrors, field.Invalid(fldPath.Child("policy"), rfc.Policy, fmt.Sprintf("invalid value, must be one of %q, %q or omitted", RequiredFieldsPointerPolicySuggestFix, RequiredFieldsPointerPolicyWarn)))
69+
}
70+
71+
return fieldErrors
72+
}
73+
74+
// validateOptionFieldsOmitEmpty is used to validate the configuration in the config.OptionalFieldsOmitEmpty struct.
75+
func validateRequiredFieldsOmitEmpty(rfc RequiredFieldsOmitEmpty, fldPath *field.Path) field.ErrorList {
76+
fieldErrors := field.ErrorList{}
77+
78+
switch rfc.Policy {
79+
case "", RequiredFieldsOmitEmptyPolicyIgnore, RequiredFieldsOmitEmptyPolicyWarn, RequiredFieldsOmitEmptyPolicySuggestFix:
80+
default:
81+
fieldErrors = append(fieldErrors, field.Invalid(fldPath.Child("policy"), rfc.Policy, fmt.Sprintf("invalid value, must be one of %q, %q, %q or omitted", RequiredFieldsOmitEmptyPolicyIgnore, RequiredFieldsOmitEmptyPolicyWarn, RequiredFieldsOmitEmptyPolicySuggestFix)))
82+
}
83+
84+
return fieldErrors
85+
}
86+
87+
// validateRequiredFieldsOmitZero is used to validate the configuration in the config.RequiredFieldsOmitZero struct.
88+
func validateRequiredFieldsOmitZero(rfc RequiredFieldsOmitZero, fldPath *field.Path) field.ErrorList {
89+
fieldErrors := field.ErrorList{}
90+
91+
switch rfc.Policy {
92+
case "", RequiredFieldsOmitZeroPolicySuggestFix, RequiredFieldsOmitZeroPolicyWarn, RequiredFieldsOmitZeroPolicyForbid:
5693
default:
57-
fieldErrors = append(fieldErrors, field.Invalid(fldPath.Child("pointerPolicy"), rfc.PointerPolicy, fmt.Sprintf("invalid value, must be one of %q, %q or omitted", RequiredFieldPointerWarn, RequiredFieldPointerSuggestFix)))
94+
fieldErrors = append(fieldErrors, field.Invalid(fldPath.Child("policy"), rfc.Policy, fmt.Sprintf("invalid value, must be one of %q, %q, %q or omitted", RequiredFieldsOmitZeroPolicySuggestFix, RequiredFieldsOmitZeroPolicyWarn, RequiredFieldsOmitZeroPolicyForbid)))
5895
}
5996

6097
return fieldErrors

0 commit comments

Comments
 (0)