Skip to content

Commit 7b3008b

Browse files
JoelSpeedJoelSpeed
committed
Ensure structs with invalid zero values must be marked omitempty
1 parent 3cc898b commit 7b3008b

File tree

5 files changed

+462
-23
lines changed

5 files changed

+462
-23
lines changed

‎pkg/analysis/optionalfields/analyzer.go

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,8 @@ const (
4747

4848
minimumMarker = markers.KubebuilderMinimumMarker
4949
maximumMarker = markers.KubebuilderMaximumMarker
50+
51+
enumMarker = markers.KubebuilderEnumMarker
5052
)
5153

5254
func init() {
@@ -92,7 +94,7 @@ func newAnalyzer(cfg config.OptionalFieldsConfig) *analysis.Analyzer {
9294
Where structs include required fields, they must be a pointer when they themselves are optional.
9395
`,
9496
Run: a.run,
95-
Requires: []*analysis.Analyzer{inspector.Analyzer},
97+
Requires: []*analysis.Analyzer{inspector.Analyzer, extractjsontags.Analyzer},
9698
}
9799
}
98100

@@ -272,9 +274,13 @@ func (a *analyzer) checkFieldPointersPreferenceWhenRequiredIdentObj(pass *analys
272274
// Any struct that has a minimum number of properties, or has required fields, should be a pointer.
273275
// Without a pointer, the JSON library cannot omit the field, and will always render a `{}`.
274276
// A rendered empty object would then violate the minimum number of properties/required field checks.
277+
//
278+
//nolint:cyclop
275279
func (a *analyzer) checkFieldPointersPreferenceWhenRequiredStructType(pass *analysis.Pass, field *ast.Field, fieldName string, isStarExpr bool, typeExpr *ast.StructType, markersAccess markershelper.Markers, jsonTags extractjsontags.FieldTagInfo) {
276280
hasRequiredFields := structContainsRequiredFields(typeExpr, markersAccess)
277281

282+
validZeroValueStruct := isStructZeroValueValid(pass, typeExpr, markersAccess)
283+
278284
hasMinimumProperties, err := structHasGreaterThanZeroMinProperties(typeExpr, markersAccess.StructMarkers(typeExpr))
279285
if err != nil {
280286
pass.Reportf(field.Pos(), "error checking struct for min properties: %v", err)
@@ -287,9 +293,13 @@ func (a *analyzer) checkFieldPointersPreferenceWhenRequiredStructType(pass *anal
287293
return
288294
}
289295

290-
if a.omitEmptyPolicy == config.OptionalFieldsOmitEmptyPolicyIgnore && !jsonTags.OmitEmpty {
296+
switch {
297+
case a.omitEmptyPolicy == config.OptionalFieldsOmitEmptyPolicyIgnore && !jsonTags.OmitEmpty && validZeroValueStruct:
291298
a.checkFieldPointersPreferenceWhenRequiredStructTypeWithoutOmitEmpty(pass, field, fieldName, isStarExpr, hasMinimumProperties, fieldHasMinimumProperties, markersAccess)
292-
} else {
299+
case a.omitEmptyPolicy == config.OptionalFieldsOmitEmptyPolicyIgnore && !jsonTags.OmitEmpty && !validZeroValueStruct:
300+
reportShouldAddOmitEmpty(pass, field, fieldName, "field %s is an optional struct without omitempty, but the zero value is not valid. Omitempty must be added.", jsonTags)
301+
fallthrough // Check as if it did have omitempty, since it requires the omitempty.
302+
default:
293303
a.checkFieldPointersPreferenceWhenRequiredStructTypeWithOmitEmpty(pass, field, fieldName, isStarExpr, hasRequiredFields, hasMinimumProperties || fieldHasMinimumProperties)
294304
}
295305
}
@@ -327,13 +337,26 @@ func (a *analyzer) checkFieldPointersPreferenceWhenRequiredStructTypeWithoutOmit
327337
// Where the minimum allowable length is 0, the field should be a pointer.
328338
// Where the minimum length is greater than 0, the field should not be a pointer.
329339
// When the field does not have omitempty, it should not be a pointer, and should not have a minimum length marker.
340+
//
341+
//nolint:cyclop
330342
func (a *analyzer) checkFieldPointersPreferenceWhenRequiredString(pass *analysis.Pass, field *ast.Field, fieldName string, isStarExpr bool, markersAccess markershelper.Markers, jsonTags extractjsontags.FieldTagInfo) {
331343
if a.omitEmptyPolicy == config.OptionalFieldsOmitEmptyPolicyIgnore && !jsonTags.OmitEmpty {
332344
a.checkFieldPointersPreferenceWhenRequiredStringWithoutOmitEmpty(pass, field, fieldName, isStarExpr, markersAccess)
333345
return
334346
}
335347

336348
fieldMarkers := markersAccess.FieldMarkers(field)
349+
if stringFieldIsEnum(fieldMarkers) {
350+
switch {
351+
case enumFieldAllowsEmpty(fieldMarkers) && !isStarExpr:
352+
reportShouldAddPointer(pass, field, a.pointerPolicy, fieldName, "field %s is an optional string enum allowing the empty value and should be a pointer")
353+
case !enumFieldAllowsEmpty(fieldMarkers) && isStarExpr:
354+
reportShouldRemovePointer(pass, field, a.pointerPolicy, fieldName, "field %s is an optional string enum not allowing the empty value and should not be a pointer")
355+
}
356+
357+
return
358+
}
359+
337360
if !fieldMarkers.Has(minLengthMarker) {
338361
if isStarExpr {
339362
pass.Reportf(field.Pos(), "field %s is an optional string and does not have a minimum length. Where the difference between omitted and the empty string is significant, set the minmum length to 0", fieldName)
@@ -365,6 +388,11 @@ func (a *analyzer) checkFieldPointersPreferenceWhenRequiredString(pass *analysis
365388
func (a *analyzer) checkFieldPointersPreferenceWhenRequiredStringWithoutOmitEmpty(pass *analysis.Pass, field *ast.Field, fieldName string, isStarExpr bool, markersAccess markershelper.Markers) {
366389
reportShouldRemoveAllInstancesOfIntegerMarker(pass, field, markersAccess, minLengthMarker, fieldName, "field %s has a greater than zero minimum length without omitempty. The minimum length should be removed.")
367390

391+
fieldMarkers := markersAccess.FieldMarkers(field)
392+
if stringFieldIsEnum(fieldMarkers) && !enumFieldAllowsEmpty(fieldMarkers) {
393+
pass.Reportf(field.Pos(), "field %s is an optional string enum not allowing the empty value without omitempty. Either allow the empty string or add omitempty.", fieldName)
394+
}
395+
368396
// When non-omitempty, the string field should not be a pointer.
369397
// The empty string should be a valid/acceptable value.
370398
if isStarExpr {

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

Lines changed: 106 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,16 @@ type A struct {
129129
// +optional
130130
StringWithMinLength0 string `json:"stringWithMinLength0,omitempty"` // want "field StringWithMinLength0 has a minimum length of 0. The empty string is a valid value and therefore the field should be a pointer"
131131

132+
// EnumString is a string field with an enum.
133+
// +kubebuilder:validation:Enum=foo;bar;baz
134+
// +optional
135+
EnumString string `json:"enumString,omitempty"`
136+
137+
// EnumStringWithEmptyValue is a string field with an enum, also allowing the empty string.
138+
// +kubebuilder:validation:Enum=foo;bar;baz;""
139+
// +optional
140+
EnumStringWithEmptyValue string `json:"enumStringWithEmptyValue,omitempty"` // want "field EnumStringWithEmptyValue is an optional string enum allowing the empty value and should be a pointer"
141+
132142
// stringWithMinLength0WithoutOmitEmpty with minimum length is a string field without omitempty.
133143
// +kubebuilder:validation:MinLength=0
134144
// +optional
@@ -322,7 +332,19 @@ type A struct {
322332

323333
// structWithRequiredFieldsWithoutOmitEmpty is a struct field without omitempty.
324334
// +optional
325-
StructWithRequiredFieldsWithoutOmitEmpty C `json:"structWithRequiredFieldsWithoutOmitEmpty"`
335+
StructWithRequiredFieldsWithoutOmitEmpty E `json:"structWithRequiredFieldsWithoutOmitEmpty"`
336+
337+
// structWithRequiredFieldsWithNonZeroAllowedValuesWithoutOmitEmpty is a struct field with required fields but where the zero values are not valid.
338+
// +optional
339+
StructWithRequiredFieldsWithNonZeroAllowedValuesWithoutOmitEmpty C `json:"structWithRequiredFieldsWithNonZeroAllowedValuesWithoutOmitEmpty"` // want "field StructWithRequiredFieldsWithNonZeroAllowedValuesWithoutOmitEmpty is optional, but contains required field\\(s\\) and should be a pointer" "field StructWithRequiredFieldsWithNonZeroAllowedValuesWithoutOmitEmpty is an optional struct without omitempty, but the zero value is not valid. Omitempty must be added."
340+
341+
// structWithNonZeroByteArray is a struct field with a non-zero byte array.
342+
// +optional
343+
StructWithNonZeroByteArray F `json:"structWithNonZeroByteArray"` // want "field StructWithNonZeroByteArray is an optional struct without omitempty, but the zero value is not valid. Omitempty must be added."
344+
345+
// structWithInvalidEmptyEnum is a struct field with an invalid empty enum.
346+
// +optional
347+
StructWithInvalidEmptyEnum G `json:"structWithInvalidEmptyEnum"` // want "field StructWithInvalidEmptyEnum is an optional struct without omitempty, but the zero value is not valid. Omitempty must be added"
326348

327349
// pointerStructWithOptionalFields is a pointer struct field.
328350
// +optional
@@ -338,7 +360,7 @@ type A struct {
338360

339361
// pointerStructWithRequiredFieldsWithoutOmitEmpty is a pointer struct field without omitempty.
340362
// +optional
341-
PointerStructWithRequiredFieldsWithoutOmitEmpty *C `json:"pointerStructWithRequiredFieldsWithoutOmitEmpty"` // want "field PointerStructWithRequiredFieldsWithoutOmitEmpty is an optional struct without omitempty. It should not be a pointer"
363+
PointerStructWithRequiredFieldsWithoutOmitEmpty *C `json:"pointerStructWithRequiredFieldsWithoutOmitEmpty"` // want "field PointerStructWithRequiredFieldsWithoutOmitEmpty is an optional struct without omitempty, but the zero value is not valid. Omitempty must be added"
342364

343365
// bool is a boolean field.
344366
// +optional
@@ -407,6 +429,7 @@ type B struct {
407429
type C struct {
408430
// string is a string field.
409431
// +required
432+
// +kubebuilder:validation:MinLength=1
410433
String string `json:"string"`
411434
}
412435

@@ -421,3 +444,84 @@ type D struct {
421444
// +optional
422445
StringWithMinLength1 string `json:"stringWithMinLength1,omitempty"`
423446
}
447+
448+
// E is a struct with required fields but where the zero values are valid.
449+
// In this case the struct does not need to be a pointer when there is no omitempty.
450+
type E struct {
451+
// string is a string field.
452+
// +required
453+
// +kubebuilder:validation:MinLength=0
454+
String string `json:"string"`
455+
456+
// stringWithMinLength1 with minimum length is a string field.
457+
// +optional
458+
// +kubebuilder:validation:MinLength=1
459+
StringWithMinLength1 string `json:"stringWithMinLength1,omitempty"`
460+
461+
// enumWithOmitEmpty is an enum field with omitempty.
462+
// It does not need to allow the zero value.
463+
// +optional
464+
// +kubebuilder:validation:Enum=foo;bar
465+
EnumWithOmitEmpty string `json:"enumWithOmitEmpty,omitempty"`
466+
467+
// enumWithoutOmitEmpty is an enum field without omitempty.
468+
// It does need to allow the zero value.
469+
// +optional
470+
// +kubebuilder:validation:Enum=foo;bar;""
471+
EnumWithoutOmitEmpty string `json:"enumWithoutOmitEmpty"`
472+
473+
// int is an int field.
474+
// +required
475+
// +kubebuilder:validation:Minimum=0
476+
Int int `json:"int"`
477+
478+
// intWithMinValue1 with minimum value is an int field.
479+
// +optional
480+
// +kubebuilder:validation:Minimum=1
481+
IntWithMinValue1 int `json:"intWithMinValue1,omitempty"`
482+
483+
// intWithMinValue1WithoutOmitEmpty with minimum value is an int field without omitempty.
484+
// +optional
485+
// +kubebuilder:validation:Maximum=1
486+
IntWithNoMinimum int `json:"intWithNoMinimum"`
487+
488+
// intWithNoMaximum with no maximum value is an int field.
489+
// +optional
490+
// +kubebuilder:validation:Minimum=-1
491+
IntWithNoMaximum int `json:"intWithNoMaximum"`
492+
493+
// float is a float field.
494+
// +required
495+
// +kubebuilder:validation:Minimum=0.0
496+
Float float64 `json:"float"`
497+
498+
// floatWithMinValue1 with minimum value is a float field.
499+
// +optional
500+
// +kubebuilder:validation:Minimum=1.0
501+
FloatWithMinValue1 float64 `json:"floatWithMinValue1,omitempty"`
502+
503+
// B is a struct with optional fields.
504+
B B `json:"b"`
505+
506+
// D is a struct that's optional, but does not have any required fields.
507+
D D `json:"d"`
508+
509+
// ByteArry is a byte array field.
510+
// +optional
511+
ByteArray []byte `json:"byteArray"`
512+
}
513+
514+
type F struct {
515+
// nonZeroByteArray is a byte array field that does not allow the zero value.
516+
// +kubebuilder:validation:MinLength=1
517+
// +optional
518+
NonZeroByteArray []byte `json:"nonZeroByteArray"`
519+
}
520+
521+
type G struct {
522+
// enumWithoutOmitEmpty is an enum field without omitempty.
523+
// It does need to allow the zero value, else the parent zero value is not valid.
524+
// +optional
525+
// +kubebuilder:validation:Enum=foo;bar
526+
EnumWithoutOmitEmpty string `json:"enumWithoutOmitEmpty"` // want "field EnumWithoutOmitEmpty is an optional string enum not allowing the empty value without omitempty. Either allow the empty string or add omitempty."
527+
}

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

Lines changed: 111 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -98,8 +98,8 @@ type A struct {
9898
// pointerStruct is a pointer struct field.
9999
// +optional
100100
PointerStruct B `json:"pointerStruct,omitempty"` // want "field PointerStruct is optional, and contains no required field\\(s\\) and does not need to be a pointer"
101-
102-
// pointerStructWithoutOmitEmpty is a pointer struct field without omitempty.
101+
102+
// pointerStructWithoutOmitEmpty is a pointer struct field without omitempty.
103103
// +optional
104104
PointerStructWithoutOmitEmpty B `json:"pointerStructWithoutOmitEmpty"` // want "field PointerStructWithoutOmitEmpty is an optional struct without omitempty. It should not be a pointer"
105105

@@ -125,6 +125,16 @@ type A struct {
125125
// +optional
126126
StringWithMinLength0 *string `json:"stringWithMinLength0,omitempty"` // want "field StringWithMinLength0 has a minimum length of 0. The empty string is a valid value and therefore the field should be a pointer"
127127

128+
// EnumString is a string field with an enum.
129+
// +kubebuilder:validation:Enum=foo;bar;baz
130+
// +optional
131+
EnumString string `json:"enumString,omitempty"`
132+
133+
// EnumStringWithEmptyValue is a string field with an enum, also allowing the empty string.
134+
// +kubebuilder:validation:Enum=foo;bar;baz;""
135+
// +optional
136+
EnumStringWithEmptyValue *string `json:"enumStringWithEmptyValue,omitempty"` // want "field EnumStringWithEmptyValue is an optional string enum allowing the empty value and should be a pointer"
137+
128138
// stringWithMinLength0WithoutOmitEmpty with minimum length is a string field without omitempty.
129139
// +kubebuilder:validation:MinLength=0
130140
// +optional
@@ -271,8 +281,8 @@ type A struct {
271281
// +kubebuilder:validation:Maximum=10.0
272282
// +optional
273283
FloatWithRangeWithoutOmitEmpty float64 `json:"floatWithRangeWithoutOmitEmpty"`
274-
275-
// floatWithInvalidMinimumValue with invalid minimum value is a float field.
284+
285+
// floatWithInvalidMinimumValue with invalid minimum value is a float field.
276286
// +kubebuilder:validation:Minimum=foo
277287
// +optional
278288
FloatWithInvalidMinimumValue float64 `json:"floatWithInvalidMinimumValue,omitempty"` // want "field FloatWithInvalidMinimumValue has a minimum value of foo, but it is not a float"
@@ -311,9 +321,21 @@ type A struct {
311321
// +optional
312322
StructWithRequiredFields *C `json:"structWithRequiredFields,omitempty"` // want "field StructWithRequiredFields is optional, but contains required field\\(s\\) and should be a pointer"
313323

314-
// structWithRequiredFieldsWithoutOmitEmpty is a struct field without omitempty.
324+
// structWithRequiredFieldsWithoutOmitEmpty is a struct field without omitempty.
325+
// +optional
326+
StructWithRequiredFieldsWithoutOmitEmpty E `json:"structWithRequiredFieldsWithoutOmitEmpty"`
327+
328+
// structWithRequiredFieldsWithNonZeroAllowedValuesWithoutOmitEmpty is a struct field with required fields but where the zero values are not valid.
315329
// +optional
316-
StructWithRequiredFieldsWithoutOmitEmpty C `json:"structWithRequiredFieldsWithoutOmitEmpty"`
330+
StructWithRequiredFieldsWithNonZeroAllowedValuesWithoutOmitEmpty *C `json:"structWithRequiredFieldsWithNonZeroAllowedValuesWithoutOmitEmpty,omitempty"` // want "field StructWithRequiredFieldsWithNonZeroAllowedValuesWithoutOmitEmpty is optional, but contains required field\\(s\\) and should be a pointer" "field StructWithRequiredFieldsWithNonZeroAllowedValuesWithoutOmitEmpty is an optional struct without omitempty, but the zero value is not valid. Omitempty must be added."
331+
332+
// structWithNonZeroByteArray is a struct field with a non-zero byte array.
333+
// +optional
334+
StructWithNonZeroByteArray F `json:"structWithNonZeroByteArray,omitempty"` // want "field StructWithNonZeroByteArray is an optional struct without omitempty, but the zero value is not valid. Omitempty must be added."
335+
336+
// structWithInvalidEmptyEnum is a struct field with an invalid empty enum.
337+
// +optional
338+
StructWithInvalidEmptyEnum G `json:"structWithInvalidEmptyEnum,omitempty"` // want "field StructWithInvalidEmptyEnum is an optional struct without omitempty, but the zero value is not valid. Omitempty must be added"
317339

318340
// pointerStructWithOptionalFields is a pointer struct field.
319341
// +optional
@@ -329,7 +351,7 @@ type A struct {
329351

330352
// pointerStructWithRequiredFieldsWithoutOmitEmpty is a pointer struct field without omitempty.
331353
// +optional
332-
PointerStructWithRequiredFieldsWithoutOmitEmpty C `json:"pointerStructWithRequiredFieldsWithoutOmitEmpty"` // want "field PointerStructWithRequiredFieldsWithoutOmitEmpty is an optional struct without omitempty. It should not be a pointer"
354+
PointerStructWithRequiredFieldsWithoutOmitEmpty *C `json:"pointerStructWithRequiredFieldsWithoutOmitEmpty,omitempty"` // want "field PointerStructWithRequiredFieldsWithoutOmitEmpty is an optional struct without omitempty, but the zero value is not valid. Omitempty must be added"
333355

334356
// bool is a boolean field.
335357
// +optional
@@ -396,6 +418,7 @@ type B struct {
396418
type C struct {
397419
// string is a string field.
398420
// +required
421+
// +kubebuilder:validation:MinLength=1
399422
String string `json:"string"`
400423
}
401424

@@ -410,3 +433,84 @@ type D struct {
410433
// +optional
411434
StringWithMinLength1 string `json:"stringWithMinLength1,omitempty"`
412435
}
436+
437+
// E is a struct with required fields but where the zero values are valid.
438+
// In this case the struct does not need to be a pointer when there is no omitempty.
439+
type E struct {
440+
// string is a string field.
441+
// +required
442+
// +kubebuilder:validation:MinLength=0
443+
String string `json:"string"`
444+
445+
// stringWithMinLength1 with minimum length is a string field.
446+
// +optional
447+
// +kubebuilder:validation:MinLength=1
448+
StringWithMinLength1 string `json:"stringWithMinLength1,omitempty"`
449+
450+
// enumWithOmitEmpty is an enum field with omitempty.
451+
// It does not need to allow the zero value.
452+
// +optional
453+
// +kubebuilder:validation:Enum=foo;bar
454+
EnumWithOmitEmpty string `json:"enumWithOmitEmpty,omitempty"`
455+
456+
// enumWithoutOmitEmpty is an enum field without omitempty.
457+
// It does need to allow the zero value.
458+
// +optional
459+
// +kubebuilder:validation:Enum=foo;bar;""
460+
EnumWithoutOmitEmpty string `json:"enumWithoutOmitEmpty"`
461+
462+
// int is an int field.
463+
// +required
464+
// +kubebuilder:validation:Minimum=0
465+
Int int `json:"int"`
466+
467+
// intWithMinValue1 with minimum value is an int field.
468+
// +optional
469+
// +kubebuilder:validation:Minimum=1
470+
IntWithMinValue1 int `json:"intWithMinValue1,omitempty"`
471+
472+
// intWithMinValue1WithoutOmitEmpty with minimum value is an int field without omitempty.
473+
// +optional
474+
// +kubebuilder:validation:Maximum=1
475+
IntWithNoMinimum int `json:"intWithNoMinimum"`
476+
477+
// intWithNoMaximum with no maximum value is an int field.
478+
// +optional
479+
// +kubebuilder:validation:Minimum=-1
480+
IntWithNoMaximum int `json:"intWithNoMaximum"`
481+
482+
// float is a float field.
483+
// +required
484+
// +kubebuilder:validation:Minimum=0.0
485+
Float float64 `json:"float"`
486+
487+
// floatWithMinValue1 with minimum value is a float field.
488+
// +optional
489+
// +kubebuilder:validation:Minimum=1.0
490+
FloatWithMinValue1 float64 `json:"floatWithMinValue1,omitempty"`
491+
492+
// B is a struct with optional fields.
493+
B B `json:"b"`
494+
495+
// D is a struct that's optional, but does not have any required fields.
496+
D D `json:"d"`
497+
498+
// ByteArry is a byte array field.
499+
// +optional
500+
ByteArray []byte `json:"byteArray"`
501+
}
502+
503+
type F struct {
504+
// nonZeroByteArray is a byte array field that does not allow the zero value.
505+
// +kubebuilder:validation:MinLength=1
506+
// +optional
507+
NonZeroByteArray []byte `json:"nonZeroByteArray"`
508+
}
509+
510+
type G struct {
511+
// enumWithoutOmitEmpty is an enum field without omitempty.
512+
// It does need to allow the zero value, else the parent zero value is not valid.
513+
// +optional
514+
// +kubebuilder:validation:Enum=foo;bar
515+
EnumWithoutOmitEmpty string `json:"enumWithoutOmitEmpty"` // want "field EnumWithoutOmitEmpty is an optional string enum not allowing the empty value without omitempty. Either allow the empty string or add omitempty."
516+
}

0 commit comments

Comments
 (0)