Skip to content

Commit 43a29a6

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #121 from yongruilin/fix-enum-on-type
fix(optionalfields): lookup types markers of fields when inspect fields
2 parents 4fab82d + d8e303d commit 43a29a6

File tree

8 files changed

+170
-12
lines changed

8 files changed

+170
-12
lines changed

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,20 @@ type A struct {
9898
// PointerMapAlias is a pointer map alias field.
9999
// +optional
100100
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."
101+
102+
// StringAliasWithEnum is a string alias field with enum validation.
103+
// With the "Always" pointer preference, optional fields should be pointers regardless of zero value validity.
104+
// +optional
105+
StringAliasWithEnum StringAliasWithEnum `json:"stringAliasWithEnum,omitempty"` // want "field StringAliasWithEnum is optional and should be a pointer"
106+
107+
// StringAliasWithEnumPointer is a pointer string alias field with enum validation.
108+
// This is correctly a pointer since the zero value is not valid.
109+
// +optional
110+
StringAliasWithEnumPointer *StringAliasWithEnum `json:"stringAliasWithEnumPointer,omitempty"`
111+
112+
// StringAliasWithEnumNoOmitEmpty is a string alias field with enum validation and no omitempty.
113+
// +optional
114+
StringAliasWithEnumNoOmitEmpty *StringAliasWithEnum `json:"stringAliasWithEnumNoOmitEmpty"` // want "field StringAliasWithEnumNoOmitEmpty is optional and should have the omitempty tag"
101115
}
102116

103117
type B struct {
@@ -129,3 +143,8 @@ type BoolAlias bool
129143
type SliceAlias []string
130144

131145
type MapAlias map[string]string
146+
147+
// StringAliasWithEnum is a string alias with enum validation.
148+
// The zero value ("") is not in the enum, making it invalid.
149+
// +kubebuilder:validation:Enum=value1;value2
150+
type StringAliasWithEnum string

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,20 @@ type A struct {
9898
// PointerMapAlias is a pointer map alias field.
9999
// +optional
100100
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."
101+
102+
// StringAliasWithEnum is a string alias field with enum validation.
103+
// With the "Always" pointer preference, optional fields should be pointers regardless of zero value validity.
104+
// +optional
105+
StringAliasWithEnum *StringAliasWithEnum `json:"stringAliasWithEnum,omitempty"` // want "field StringAliasWithEnum is optional and should be a pointer"
106+
107+
// StringAliasWithEnumPointer is a pointer string alias field with enum validation.
108+
// This is correctly a pointer since the zero value is not valid.
109+
// +optional
110+
StringAliasWithEnumPointer *StringAliasWithEnum `json:"stringAliasWithEnumPointer,omitempty"`
111+
112+
// StringAliasWithEnumNoOmitEmpty is a string alias field with enum validation and no omitempty.
113+
// +optional
114+
StringAliasWithEnumNoOmitEmpty *StringAliasWithEnum `json:"stringAliasWithEnumNoOmitEmpty,omitempty"` // want "field StringAliasWithEnumNoOmitEmpty is optional and should have the omitempty tag"
101115
}
102116

103117
type B struct {
@@ -129,3 +143,8 @@ type BoolAlias bool
129143
type SliceAlias []string
130144

131145
type MapAlias map[string]string
146+
147+
// StringAliasWithEnum is a string alias with enum validation.
148+
// The zero value ("") is not in the enum, making it invalid.
149+
// +kubebuilder:validation:Enum=value1;value2
150+
type StringAliasWithEnum string

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,24 @@ type A struct {
270270
// MapAlias is a map alias field.
271271
// +optional
272272
MapAlias MapAlias `json:"mapAlias,omitempty"`
273+
274+
// StringAliasWithEnum is a string alias field with enum validation.
275+
// The zero value ("") is not in the enum, so this should NOT be a pointer.
276+
// +optional
277+
StringAliasWithEnum StringAliasWithEnum `json:"stringAliasWithEnum,omitempty"`
278+
279+
// StringAliasWithEnumPointer is a pointer string alias field with enum validation.
280+
// This should NOT be a pointer since the zero value is not valid.
281+
// +optional
282+
StringAliasWithEnumPointer *StringAliasWithEnum `json:"stringAliasWithEnumPointer,omitempty"` // want "field StringAliasWithEnumPointer is optional and does not allow the zero value. The field does not need to be a pointer."
283+
284+
// StringAliasWithEnumNoOmitEmpty is a string alias field with enum validation and no omitempty.
285+
// +optional
286+
StringAliasWithEnumNoOmitEmpty StringAliasWithEnum `json:"stringAliasWithEnumNoOmitEmpty"` // want "field StringAliasWithEnumNoOmitEmpty is optional and should have the omitempty tag"
287+
288+
// StringAliasWithEnumEmptyValue is a string alias field with enum validation and empty value.
289+
// +optional
290+
StringAliasWithEnumEmptyValue *StringAliasWithEnumEmptyValue `json:"stringAliasWithEnumEmptyValue,omitempty"`
273291
}
274292

275293
type B struct {
@@ -308,3 +326,12 @@ type BoolAlias bool
308326
type SliceAlias []string
309327

310328
type MapAlias map[string]string
329+
330+
// StringAliasWithEnum is a string alias with enum validation.
331+
// The zero value ("") is not in the enum, making it invalid.
332+
// +kubebuilder:validation:Enum=value1;value2
333+
type StringAliasWithEnum string
334+
335+
// StringAliasWithEnumEmptyValue is a string alias with enum validation and empty value.
336+
// +kubebuilder:validation:Enum=value1;value2;""
337+
type StringAliasWithEnumEmptyValue string

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,24 @@ type A struct {
270270
// MapAlias is a map alias field.
271271
// +optional
272272
MapAlias MapAlias `json:"mapAlias,omitempty"`
273+
274+
// StringAliasWithEnum is a string alias field with enum validation.
275+
// The zero value ("") is not in the enum, so this should NOT be a pointer.
276+
// +optional
277+
StringAliasWithEnum StringAliasWithEnum `json:"stringAliasWithEnum,omitempty"`
278+
279+
// StringAliasWithEnumPointer is a pointer string alias field with enum validation.
280+
// This should NOT be a pointer since the zero value is not valid.
281+
// +optional
282+
StringAliasWithEnumPointer StringAliasWithEnum `json:"stringAliasWithEnumPointer,omitempty"` // want "field StringAliasWithEnumPointer is optional and does not allow the zero value. The field does not need to be a pointer."
283+
284+
// StringAliasWithEnumNoOmitEmpty is a string alias field with enum validation and no omitempty.
285+
// +optional
286+
StringAliasWithEnumNoOmitEmpty StringAliasWithEnum `json:"stringAliasWithEnumNoOmitEmpty,omitempty"` // want "field StringAliasWithEnumNoOmitEmpty is optional and should have the omitempty tag"
287+
288+
// StringAliasWithEnumEmptyValue is a string alias field with enum validation and empty value.
289+
// +optional
290+
StringAliasWithEnumEmptyValue *StringAliasWithEnumEmptyValue `json:"stringAliasWithEnumEmptyValue,omitempty"`
273291
}
274292

275293
type B struct {
@@ -308,3 +326,12 @@ type BoolAlias bool
308326
type SliceAlias []string
309327

310328
type MapAlias map[string]string
329+
330+
// StringAliasWithEnum is a string alias with enum validation.
331+
// The zero value ("") is not in the enum, making it invalid.
332+
// +kubebuilder:validation:Enum=value1;value2
333+
type StringAliasWithEnum string
334+
335+
// StringAliasWithEnumEmptyValue is a string alias with enum validation and empty value.
336+
// +kubebuilder:validation:Enum=value1;value2;""
337+
type StringAliasWithEnumEmptyValue string

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -421,6 +421,24 @@ type A struct {
421421
// PointerPointerString is a double pointer string field.
422422
// +optional
423423
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."
424+
425+
// StringAliasWithEnum is a string alias field with enum validation.
426+
// The zero value ("") is not in the enum, so this should NOT be a pointer.
427+
// +optional
428+
StringAliasWithEnum StringAliasWithEnum `json:"stringAliasWithEnum,omitempty"`
429+
430+
// StringAliasWithEnumPointer is a pointer string alias field with enum validation.
431+
// This should NOT be a pointer since the zero value is not valid.
432+
// +optional
433+
StringAliasWithEnumPointer *StringAliasWithEnum `json:"stringAliasWithEnumPointer,omitempty"` // want "field StringAliasWithEnumPointer is optional and does not allow the zero value. The field does not need to be a pointer."
434+
435+
// StringAliasWithEnumNoOmitEmpty is a string alias field with enum validation and no omitempty.
436+
// +optional
437+
StringAliasWithEnumNoOmitEmpty StringAliasWithEnum `json:"stringAliasWithEnumNoOmitEmpty"` // want "field StringAliasWithEnumNoOmitEmpty is optional and does not allow the zero value. It must have the omitempty tag."
438+
439+
// StringAliasWithEnumEmptyValue is a string alias field with enum validation and empty value.
440+
// +optional
441+
StringAliasWithEnumEmptyValue *StringAliasWithEnumEmptyValue `json:"stringAliasWithEnumEmptyValue,omitempty"`
424442
}
425443

426444
type B struct {
@@ -526,3 +544,12 @@ type G struct {
526544
// +kubebuilder:validation:Enum=foo;bar
527545
EnumWithoutOmitEmpty string `json:"enumWithoutOmitEmpty"` // want "field EnumWithoutOmitEmpty is optional and does not allow the zero value. It must have the omitempty tag."
528546
}
547+
548+
// StringAliasWithEnum is a string alias with enum validation.
549+
// The zero value ("") is not in the enum, making it invalid.
550+
// +kubebuilder:validation:Enum=value1;value2
551+
type StringAliasWithEnum string
552+
553+
// StringAliasWithEnumEmptyValue is a string alias with enum validation and empty value.
554+
// +kubebuilder:validation:Enum=value1;value2;""
555+
type StringAliasWithEnumEmptyValue string

‎pkg/analysis/optionalfields/testdata/src/c/a.go.golden‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -421,6 +421,24 @@ type A struct {
421421
// PointerPointerString is a double pointer string field.
422422
// +optional
423423
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."
424+
425+
// StringAliasWithEnum is a string alias field with enum validation.
426+
// The zero value ("") is not in the enum, so this should NOT be a pointer.
427+
// +optional
428+
StringAliasWithEnum StringAliasWithEnum `json:"stringAliasWithEnum,omitempty"`
429+
430+
// StringAliasWithEnumPointer is a pointer string alias field with enum validation.
431+
// This should NOT be a pointer since the zero value is not valid.
432+
// +optional
433+
StringAliasWithEnumPointer StringAliasWithEnum `json:"stringAliasWithEnumPointer,omitempty"` // want "field StringAliasWithEnumPointer is optional and does not allow the zero value. The field does not need to be a pointer."
434+
435+
// StringAliasWithEnumNoOmitEmpty is a string alias field with enum validation and no omitempty.
436+
// +optional
437+
StringAliasWithEnumNoOmitEmpty StringAliasWithEnum `json:"stringAliasWithEnumNoOmitEmpty,omitempty"` // want "field StringAliasWithEnumNoOmitEmpty is optional and does not allow the zero value. It must have the omitempty tag."
438+
439+
// StringAliasWithEnumEmptyValue is a string alias field with enum validation and empty value.
440+
// +optional
441+
StringAliasWithEnumEmptyValue *StringAliasWithEnumEmptyValue `json:"stringAliasWithEnumEmptyValue,omitempty"`
424442
}
425443

426444
type B struct {
@@ -526,3 +544,12 @@ type G struct {
526544
// +kubebuilder:validation:Enum=foo;bar
527545
EnumWithoutOmitEmpty string `json:"enumWithoutOmitEmpty,omitempty"` // want "field EnumWithoutOmitEmpty is optional and does not allow the zero value. It must have the omitempty tag."
528546
}
547+
548+
// StringAliasWithEnum is a string alias with enum validation.
549+
// The zero value ("") is not in the enum, making it invalid.
550+
// +kubebuilder:validation:Enum=value1;value2
551+
type StringAliasWithEnum string
552+
553+
// StringAliasWithEnumEmptyValue is a string alias with enum validation and empty value.
554+
// +kubebuilder:validation:Enum=value1;value2;""
555+
type StringAliasWithEnumEmptyValue string

‎pkg/analysis/optionalfields/util.go‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -165,10 +165,10 @@ func isZeroValueValid(pass *analysis.Pass, field *ast.Field, typeExpr ast.Expr,
165165
case *ast.Ident:
166166
return isIdentZeroValueValid(pass, field, t, markersAccess)
167167
case *ast.MapType:
168-
return isMapZeroValueValid(field, markersAccess)
168+
return isMapZeroValueValid(pass, field, markersAccess)
169169
case *ast.ArrayType:
170170
// For arrays, we can use a zero value if the array is not required to have a minimum number of items.
171-
return isArrayZeroValueValid(field, t, markersAccess)
171+
return isArrayZeroValueValid(pass, field, t, markersAccess)
172172
case *ast.StarExpr:
173173
return isZeroValueValid(pass, field, t.X, markersAccess)
174174
}
@@ -249,7 +249,7 @@ func isIdentZeroValueValid(pass *analysis.Pass, field *ast.Field, ident *ast.Ide
249249
// Check if the identifier is a known type that can have a zero value.
250250
switch {
251251
case isStringIdent(ident):
252-
return isStringZeroValueValid(field, markersAccess)
252+
return isStringZeroValueValid(pass, field, markersAccess)
253253
case isIntegerIdent(ident):
254254
return isNumericZeroValueValid[int](pass, field, markersAccess)
255255
case isFloatIdent(ident):
@@ -270,8 +270,8 @@ func isIdentZeroValueValid(pass *analysis.Pass, field *ast.Field, ident *ast.Ide
270270

271271
// isStringZeroValueValid checks if a string field can have a zero value.
272272
// This would be true when either there is no minimum length marker, or when the minimmum length marker is set to 0.
273-
func isStringZeroValueValid(field *ast.Field, markersAccess markershelper.Markers) (bool, bool) {
274-
fieldMarkers := markersAccess.FieldMarkers(field)
273+
func isStringZeroValueValid(pass *analysis.Pass, field *ast.Field, markersAccess markershelper.Markers) (bool, bool) {
274+
fieldMarkers := utils.TypeAwareMarkerCollectionForField(pass, markersAccess, field)
275275

276276
if stringFieldIsEnum(fieldMarkers) {
277277
return enumFieldAllowsEmpty(fieldMarkers), true
@@ -285,8 +285,8 @@ func isStringZeroValueValid(field *ast.Field, markersAccess markershelper.Marker
285285

286286
// isMapZeroValueValid checks if a map field can have a zero value.
287287
// For maps, this means there is no minProperties marker, or the minProperties marker is set to 0.
288-
func isMapZeroValueValid(field *ast.Field, markersAccess markershelper.Markers) (bool, bool) {
289-
fieldMarkers := markersAccess.FieldMarkers(field)
288+
func isMapZeroValueValid(pass *analysis.Pass, field *ast.Field, markersAccess markershelper.Markers) (bool, bool) {
289+
fieldMarkers := utils.TypeAwareMarkerCollectionForField(pass, markersAccess, field)
290290

291291
hasMinPropertiesMarker := fieldMarkers.Has(markers.KubebuilderMinPropertiesMarker)
292292
minPropertiesMarkerIsZero := fieldMarkers.HasWithValue(fmt.Sprintf("%s=0", markers.KubebuilderMinPropertiesMarker))
@@ -295,13 +295,13 @@ func isMapZeroValueValid(field *ast.Field, markersAccess markershelper.Markers)
295295
}
296296

297297
// isArrayZeroValueValid checks if an array field can have a zero value.
298-
func isArrayZeroValueValid(field *ast.Field, arrayType *ast.ArrayType, markersAccess markershelper.Markers) (bool, bool) {
298+
func isArrayZeroValueValid(pass *analysis.Pass, field *ast.Field, arrayType *ast.ArrayType, markersAccess markershelper.Markers) (bool, bool) {
299299
// Arrays of bytes are special cased and treated as strings.
300300
if ident, ok := arrayType.Elt.(*ast.Ident); ok && ident.Name == "byte" {
301-
return isStringZeroValueValid(field, markersAccess)
301+
return isStringZeroValueValid(pass, field, markersAccess)
302302
}
303303

304-
fieldMarkers := markersAccess.FieldMarkers(field)
304+
fieldMarkers := utils.TypeAwareMarkerCollectionForField(pass, markersAccess, field)
305305

306306
// For arrays, we can use a zero value if the array is not required to have a minimum number of items.
307307
minItems, err := getMarkerNumericValueByName[int](fieldMarkers, markers.KubebuilderMinItemsMarker)
@@ -341,7 +341,7 @@ type number interface {
341341
//
342342
//nolint:cyclop
343343
func isNumericZeroValueValid[N number](pass *analysis.Pass, field *ast.Field, markersAccess markershelper.Markers) (bool, bool) {
344-
fieldMarkers := markersAccess.FieldMarkers(field)
344+
fieldMarkers := utils.TypeAwareMarkerCollectionForField(pass, markersAccess, field)
345345

346346
minimum, err := getMarkerNumericValueByName[N](fieldMarkers, markers.KubebuilderMinimumMarker)
347347
if err != nil && !errors.Is(err, errMarkerMissingValue) {

‎pkg/analysis/utils/utils.go‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,11 +122,23 @@ func isInPassPackage(pass *analysis.Pass, namedType *types.Named) bool {
122122
// TypeAwareMarkerCollectionForField collects the markers for a given field into a single markers.MarkerSet.
123123
// If the field has a type that is not a basic type (i.e a custom type) then it will also gather any markers from
124124
// the type and include them in the markers.MarkerSet that is returned.
125+
// It will look through *ast.StarExpr to the underlying type.
125126
// Markers on the type will always come before markers on the field in the list of markers for an identifier.
126127
func TypeAwareMarkerCollectionForField(pass *analysis.Pass, markersAccess markers.Markers, field *ast.Field) markers.MarkerSet {
127128
markers := markersAccess.FieldMarkers(field)
128129

129-
ident, ok := field.Type.(*ast.Ident)
130+
var underlyingType ast.Expr
131+
132+
switch t := field.Type.(type) {
133+
case *ast.Ident:
134+
underlyingType = t
135+
case *ast.StarExpr:
136+
underlyingType = t.X
137+
default:
138+
return markers
139+
}
140+
141+
ident, ok := underlyingType.(*ast.Ident)
130142
if !ok {
131143
return markers
132144
}

0 commit comments

Comments
 (0)