From 3460b2238eb8430df36e202d4195043e3cc4c913 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Wed, 5 Mar 2025 20:04:56 -0800 Subject: [PATCH] Disallow optional/required on non-pointer structs --- .../output_tests/tags/optional/doc.go | 4 +--- .../output_tests/tags/optional/doc_test.go | 6 +---- .../tags/optional/zz_generated.validations.go | 9 -------- .../cmd/validation-gen/validators/required.go | 23 +++++++++++-------- 4 files changed, 15 insertions(+), 27 deletions(-) diff --git a/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/tags/optional/doc.go b/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/tags/optional/doc.go index 677d6ea21f3..2071dd7a953 100644 --- a/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/tags/optional/doc.go +++ b/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/tags/optional/doc.go @@ -35,9 +35,7 @@ type Struct struct { // +k8s:validateFalse="field Struct.StringPtrField" StringPtrField *string `json:"stringPtrField"` - // +k8s:optional - // +k8s:validateFalse="field Struct.OtherStructField" - OtherStructField OtherStruct `json:"otherStructField"` + // non-pointer struct fields cannot be optional // +k8s:optional // +k8s:validateFalse="field Struct.OtherStructPtrField" diff --git a/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/tags/optional/doc_test.go b/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/tags/optional/doc_test.go index a7e1b181d7b..f6d296dd4f5 100644 --- a/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/tags/optional/doc_test.go +++ b/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/tags/optional/doc_test.go @@ -27,21 +27,17 @@ func Test(t *testing.T) { st.Value(&Struct{ // All zero-values. - }).ExpectValidateFalseByPath(map[string][]string{ - "otherStructField": {"type OtherStruct", "field Struct.OtherStructField"}, // optional for structs is just documentation - }) + }).ExpectValid() st.Value(&Struct{ StringField: "abc", StringPtrField: ptr.To("xyz"), - OtherStructField: OtherStruct{}, OtherStructPtrField: &OtherStruct{}, SliceField: []string{"a", "b"}, MapField: map[string]string{"a": "b", "c": "d"}, }).ExpectValidateFalseByPath(map[string][]string{ "stringField": {"field Struct.StringField"}, "stringPtrField": {"field Struct.StringPtrField"}, - "otherStructField": {"type OtherStruct", "field Struct.OtherStructField"}, "otherStructPtrField": {"type OtherStruct", "field Struct.OtherStructPtrField"}, "sliceField": {"field Struct.SliceField"}, "mapField": {"field Struct.MapField"}, diff --git a/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/tags/optional/zz_generated.validations.go b/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/tags/optional/zz_generated.validations.go index e2454664c6b..4f0da3bcb47 100644 --- a/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/tags/optional/zz_generated.validations.go +++ b/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/tags/optional/zz_generated.validations.go @@ -76,15 +76,6 @@ func Validate_Struct(ctx context.Context, op operation.Operation, fldPath *field return }(fldPath.Child("stringPtrField"), obj.StringPtrField, safe.Field(oldObj, func(oldObj *Struct) *string { return oldObj.StringPtrField }))...) - // field Struct.OtherStructField - errs = append(errs, - func(fldPath *field.Path, obj, oldObj *OtherStruct) (errs field.ErrorList) { - errs = append(errs, validate.FixedResult(ctx, op, fldPath, obj, oldObj, false, "field Struct.OtherStructField")...) - // optional non-pointer structs are purely documentation - errs = append(errs, Validate_OtherStruct(ctx, op, fldPath, obj, oldObj)...) - return - }(fldPath.Child("otherStructField"), &obj.OtherStructField, safe.Field(oldObj, func(oldObj *Struct) *OtherStruct { return &oldObj.OtherStructField }))...) - // field Struct.OtherStructPtrField errs = append(errs, func(fldPath *field.Path, obj, oldObj *OtherStruct) (errs field.ErrorList) { diff --git a/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/required.go b/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/required.go index 7b3d42e30ad..866960c0901 100644 --- a/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/required.go +++ b/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/required.go @@ -97,10 +97,12 @@ func (requirednessTagValidator) doRequired(context Context) (Validations, error) case types.Pointer: return Validations{Functions: []FunctionGen{Function(requiredTagName, ShortCircuit, requiredPointerValidator)}}, nil case types.Struct: - // The +required tag on a non-pointer struct is only for documentation. - // We don't perform validation here and defer the validation to - // the struct's fields. - return Validations{Comments: []string{"required non-pointer structs are purely documentation"}}, nil + // The +k8s:required tag on a non-pointer struct is not supported. + // If you encounter this error and believe you have a valid use case + // for forbiddening a non-pointer struct, please let us know! We need + // to understand your scenario to determine if we need to adjust + // this behavior or provide alternative validation mechanisms. + return Validations{}, fmt.Errorf("non-pointer structs cannot use the %q tag", requiredTagName) } return Validations{Functions: []FunctionGen{Function(requiredTagName, ShortCircuit, requiredValueValidator)}}, nil } @@ -125,11 +127,12 @@ func (requirednessTagValidator) doOptional(context Context) (Validations, error) case types.Pointer: return Validations{Functions: []FunctionGen{Function(optionalTagName, ShortCircuit|NonError, optionalPointerValidator)}}, nil case types.Struct: - // Specifying that a non-pointer struct is optional doesn't actually - // make sense technically almost ever, and is better described as a - // union inside the struct. It does, however, make sense as - // documentation. - return Validations{Comments: []string{"optional non-pointer structs are purely documentation"}}, nil + // The +k8s:optional tag on a non-pointer struct is not supported. + // If you encounter this error and believe you have a valid use case + // for forbiddening a non-pointer struct, please let us know! We need + // to understand your scenario to determine if we need to adjust + // this behavior or provide alternative validation mechanisms. + return Validations{}, fmt.Errorf("non-pointer structs cannot use the %q tag", optionalTagName) } return Validations{Functions: []FunctionGen{Function(optionalTagName, ShortCircuit|NonError, optionalValueValidator)}}, nil } @@ -174,7 +177,7 @@ func (requirednessTagValidator) doForbidden(context Context) (Validations, error }, }, nil case types.Struct: - // The +forbidden tag on a non-pointer struct is not supported. + // The +k8s:forbidden tag on a non-pointer struct is not supported. // If you encounter this error and believe you have a valid use case // for forbiddening a non-pointer struct, please let us know! We need // to understand your scenario to determine if we need to adjust