Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions pkg/analysis/utils/serialization/serialization_check.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,19 @@ func (s *serializationCheck) Check(pass *analysis.Pass, field *ast.Field, marker
isPointer, underlying := utils.IsStarExpr(field.Type)
isStruct := utils.IsStructType(pass, field.Type)

// Check if this struct should be treated as a non-struct type (e.g., Type=string marker).
// This handles structs with custom marshalling that serialize as other types.
if isStruct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to set these on non struct types? Could other types have custom marshalling and custom types?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, type markers can technically be set on non-struct custom types (e.g., type MyInt int), and non-struct types can have custom marshalling.

For non-struct types, validation markers are already collected via TypeAwareMarkerCollectionForField and handled correctly by IsZeroValueValid without needing special treatment in the serialization check.

typeValue := utils.GetTypeMarkerValue(pass, field, markersAccess)
// If the type marker indicates this is not a struct, treat it accordingly.
// Type "object" means it's still a struct/object type in the OpenAPI sense.
// Other types (string, number, integer, boolean, array) indicate custom marshalling
// that changes the serialization format from a struct to that type.
if typeValue != "" && typeValue != "object" {
isStruct = false
}
}

switch s.pointerPreference {
case PointersPreferenceAlways:
// The field must always be a pointer, pointers require omitempty, so enforce that too.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,19 @@
package a

// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=23
type BootstrapTokenString string

// BootstrapTokenStruct is a struct with Type=string marker (issue #138).
// This struct has custom JSON marshalling that serializes it as a string, not an object.
// +kubebuilder:validation:Type=string
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=23
type BootstrapTokenStruct struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this would typically have a custom unmarshal function right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I initially tried to add actual (un)marshal methods to the testdata file to make this more explicit, but the test framework expects the data to remain in a specific state.

ID string `json:"-"`
Secret string `json:"-"`
}

type TestStrings struct {
String string `json:"string"` // want "field String should have the omitempty tag." "field String has a valid zero value \\(\"\"\\), but the validation is not complete \\(e.g. minimum length\\). 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."

Expand Down Expand Up @@ -56,4 +70,22 @@ type TestStrings struct {

// +kubebuilder:validation:Enum=a;b;c;""
EnumValidEmptyStringPtrWithOmitEmpty *string `json:"enumValidEmptyStringPtrWithOmitEmpty,omitempty"`

// Test for issue #138: custom string type with validation markers on the type definition
Token BootstrapTokenString `json:"token"` // want "field Token should have the omitempty tag."

TokenWithOmitEmpty BootstrapTokenString `json:"tokenWithOmitEmpty,omitempty"`

TokenPtr *BootstrapTokenString `json:"tokenPtr"` // want "field TokenPtr should have the omitempty tag." "field TokenPtr does not allow the zero value. The field does not need to be a pointer."

TokenPtrWithOmitEmpty *BootstrapTokenString `json:"tokenPtrWithOmitEmpty,omitempty"` // want "field TokenPtrWithOmitEmpty does not allow the zero value. The field does not need to be a pointer."

// Test for issue #138: struct with Type=string marker
TokenStruct BootstrapTokenStruct `json:"tokenStruct"` // want "field TokenStruct should have the omitempty tag."

TokenStructWithOmitEmpty BootstrapTokenStruct `json:"tokenStructWithOmitEmpty,omitempty"`

TokenStructPtr *BootstrapTokenStruct `json:"tokenStructPtr"` // want "field TokenStructPtr should have the omitempty tag." "field TokenStructPtr does not allow the zero value. The field does not need to be a pointer."

TokenStructPtrWithOmitEmpty *BootstrapTokenStruct `json:"tokenStructPtrWithOmitEmpty,omitempty"` // want "field TokenStructPtrWithOmitEmpty does not allow the zero value. The field does not need to be a pointer."
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,19 @@
package a

// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=23
type BootstrapTokenString string

// BootstrapTokenStruct is a struct with Type=string marker (issue #138).
// This struct has custom JSON marshalling that serializes it as a string, not an object.
// +kubebuilder:validation:Type=string
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=23
type BootstrapTokenStruct struct {
ID string `json:"-"`
Secret string `json:"-"`
}

type TestStrings struct {
String *string `json:"string,omitempty"` // want "field String should have the omitempty tag." "field String has a valid zero value \\(\"\"\\), but the validation is not complete \\(e.g. minimum length\\). 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."

Expand Down Expand Up @@ -56,4 +70,22 @@ type TestStrings struct {

// +kubebuilder:validation:Enum=a;b;c;""
EnumValidEmptyStringPtrWithOmitEmpty *string `json:"enumValidEmptyStringPtrWithOmitEmpty,omitempty"`

// Test for issue #138: custom string type with validation markers on the type definition
Token BootstrapTokenString `json:"token,omitempty"` // want "field Token should have the omitempty tag."

TokenWithOmitEmpty BootstrapTokenString `json:"tokenWithOmitEmpty,omitempty"`

TokenPtr BootstrapTokenString `json:"tokenPtr,omitempty"` // want "field TokenPtr should have the omitempty tag." "field TokenPtr does not allow the zero value. The field does not need to be a pointer."

TokenPtrWithOmitEmpty BootstrapTokenString `json:"tokenPtrWithOmitEmpty,omitempty"` // want "field TokenPtrWithOmitEmpty does not allow the zero value. The field does not need to be a pointer."

// Test for issue #138: struct with Type=string marker
TokenStruct BootstrapTokenStruct `json:"tokenStruct,omitempty"` // want "field TokenStruct should have the omitempty tag."

TokenStructWithOmitEmpty BootstrapTokenStruct `json:"tokenStructWithOmitEmpty,omitempty"`

TokenStructPtr BootstrapTokenStruct `json:"tokenStructPtr,omitempty"` // want "field TokenStructPtr should have the omitempty tag." "field TokenStructPtr does not allow the zero value. The field does not need to be a pointer."

TokenStructPtrWithOmitEmpty BootstrapTokenStruct `json:"tokenStructPtrWithOmitEmpty,omitempty"` // want "field TokenStructPtrWithOmitEmpty does not allow the zero value. The field does not need to be a pointer."
}
29 changes: 29 additions & 0 deletions pkg/analysis/utils/zero_value.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,14 +73,43 @@ func getUnderlyingType(expr ast.Expr) ast.Expr {
return expr
}

// GetTypeMarkerValue returns the value of the kubebuilder Type marker for a field.
// Returns empty string if no Type marker is present.
// The Type marker indicates how the field serializes (e.g., "string", "number", "object").
func GetTypeMarkerValue(pass *analysis.Pass, field *ast.Field, markersAccess markershelper.Markers) string {
fieldMarkers := TypeAwareMarkerCollectionForField(pass, markersAccess, field)
typeMarkers := fieldMarkers.Get(markers.KubebuilderTypeMarker)

for _, typeMarker := range typeMarkers {
// The value might be "string" (with quotes) or string (without quotes)
typeValue := strings.Trim(typeMarker.Payload.Value, `"`)
if typeValue != "" {
return typeValue
}
}

return ""
}

// isStructZeroValueValid checks if the zero value of a struct is valid.
// It checks if all non-omitted fields within the struct accept their zero values.
// It also checks if the struct has a minProperties marker, and if so, whether the number of non-omitted fields is greater than or equal to the minProperties value.
// Special case: If the struct has Type=string marker with string validation markers (MinLength/MaxLength),
// treat it as a string for validation purposes (e.g., for structs with custom marshalling).
func isStructZeroValueValid(pass *analysis.Pass, field *ast.Field, structType *ast.StructType, markersAccess markershelper.Markers, considerOmitzero bool) (bool, bool) {
if structType == nil {
return false, false
}

// Check if this struct should be validated as a string (Type=string marker).
// This handles structs with custom marshalling that serialize as strings.
if GetTypeMarkerValue(pass, field, markersAccess) == "string" {
// Use string validation logic instead of struct validation logic.
// This ensures that string-specific validation markers (MinLength, MaxLength, Pattern)
// are properly evaluated for structs that marshal as strings.
return isStringZeroValueValid(pass, field, markersAccess)
}

jsonTagInfo, ok := pass.ResultOf[extractjsontags.Analyzer].(extractjsontags.StructFieldTags)
if !ok {
panic("could not get struct field tags from pass result")
Expand Down