From 030d72959e8911acfebc37a365d31d367ebc5e9b Mon Sep 17 00:00:00 2001 From: yongruilin Date: Wed, 5 Nov 2025 01:55:52 +0000 Subject: [PATCH] feat(validation-gen): Refactor testscheme and add ratcheting bug tests Refactors the ValidationTester in testscheme to make update validation implicit. Calling .OldValue() now automatically triggers update validation, removing the need to pass an explicit operation type and simplifying the test-writing API. This is applied to new tests that demonstrate a validation ratcheting bug where validation is incorrectly skipped when comparing a nil old value with a new value. The tests cover list items and struct fields to ensure the fix will be verified. --- .../ratcheting/default_behavior/doc.go | 9 ++++ .../ratcheting/default_behavior/doc_test.go | 9 ++++ .../zz_generated.validations.go | 29 +++++++++++ .../output_tests/ratcheting/list/doc.go | 15 ++++++ .../output_tests/ratcheting/list/doc_test.go | 16 ++++++ .../list/zz_generated.validations.go | 51 +++++++++++++++++++ .../cmd/validation-gen/testscheme/doc.go | 4 +- .../validation-gen/testscheme/testscheme.go | 16 +++--- 8 files changed, 140 insertions(+), 9 deletions(-) diff --git a/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/default_behavior/doc.go b/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/default_behavior/doc.go index f8bcd0b1959..f20a4b553e5 100644 --- a/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/default_behavior/doc.go +++ b/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/default_behavior/doc.go @@ -142,3 +142,12 @@ type NestedNonDirectComparableStruct struct { // +k8s:validateFalse="field nonDirectComparableStructField" NonDirectComparableStructField NonDirectComparableStruct `json:"nonDirectComparableStructField"` } + +type MixComparableStruct struct { + TypeMeta int + + Primitive string `json:"Primitive"` + + // +k8s:validateFalse="field NonComparable" + NonComparable []string `json:"NonComparable"` +} diff --git a/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/default_behavior/doc_test.go b/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/default_behavior/doc_test.go index 9d840c97d13..eedb927f26a 100644 --- a/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/default_behavior/doc_test.go +++ b/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/default_behavior/doc_test.go @@ -175,3 +175,12 @@ func Test_StructEmbedded(t *testing.T) { st.Value(mkTest()).OldValue(mkTest()).ExpectValid() } + +// This test is to prove the bug of ratcheting behavior mistakenly skip validation on nil vs not found. +// TODO: update this test once the ratcheting behavior is fixed. +func Test_Mix(t *testing.T) { + st := localSchemeBuilder.Test(t) + st.Value(&MixComparableStruct{ + Primitive: "a", + }).OldValue(nil).ExpectValid() +} diff --git a/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/default_behavior/zz_generated.validations.go b/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/default_behavior/zz_generated.validations.go index 9dff33bad34..82fb34499b6 100644 --- a/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/default_behavior/zz_generated.validations.go +++ b/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/default_behavior/zz_generated.validations.go @@ -38,6 +38,14 @@ func init() { localSchemeBuilder.Register(RegisterValidations) } // RegisterValidations adds validation functions to the given scheme. // Public to allow building arbitrary schemes. func RegisterValidations(scheme *testscheme.Scheme) error { + // type MixComparableStruct + scheme.AddValidationFunc((*MixComparableStruct)(nil), func(ctx context.Context, op operation.Operation, obj, oldObj interface{}) field.ErrorList { + switch op.Request.SubresourcePath() { + case "/": + return Validate_MixComparableStruct(ctx, op, nil /* fldPath */, obj.(*MixComparableStruct), safe.Cast[*MixComparableStruct](oldObj)) + } + return field.ErrorList{field.InternalError(nil, fmt.Errorf("no validation found for %T, subresource: %v", obj, op.Request.SubresourcePath()))} + }) // type StructEmbedded scheme.AddValidationFunc((*StructEmbedded)(nil), func(ctx context.Context, op operation.Operation, obj, oldObj interface{}) field.ErrorList { switch op.Request.SubresourcePath() { @@ -121,6 +129,27 @@ func Validate_DirectComparableStruct(ctx context.Context, op operation.Operation return errs } +// Validate_MixComparableStruct validates an instance of MixComparableStruct according +// to declarative validation rules in the API schema. +func Validate_MixComparableStruct(ctx context.Context, op operation.Operation, fldPath *field.Path, obj, oldObj *MixComparableStruct) (errs field.ErrorList) { + // field MixComparableStruct.TypeMeta has no validation + // field MixComparableStruct.Primitive has no validation + + // field MixComparableStruct.NonComparable + errs = append(errs, + func(fldPath *field.Path, obj, oldObj []string) (errs field.ErrorList) { + // don't revalidate unchanged data + if op.Type == operation.Update && equality.Semantic.DeepEqual(obj, oldObj) { + return nil + } + // call field-attached validations + errs = append(errs, validate.FixedResult(ctx, op, fldPath, obj, oldObj, false, "field NonComparable")...) + return + }(fldPath.Child("NonComparable"), obj.NonComparable, safe.Field(oldObj, func(oldObj *MixComparableStruct) []string { return oldObj.NonComparable }))...) + + return errs +} + // Validate_MySlice validates an instance of MySlice according // to declarative validation rules in the API schema. func Validate_MySlice(ctx context.Context, op operation.Operation, fldPath *field.Path, obj, oldObj MySlice) (errs field.ErrorList) { diff --git a/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/list/doc.go b/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/list/doc.go index a46f406b160..edd0dcca9fe 100644 --- a/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/list/doc.go +++ b/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/list/doc.go @@ -104,3 +104,18 @@ type MixedKeyStruct struct { Key2 string `json:"key2"` Data string `json:"data"` } + +type Item struct { + Key string `json:"key"` + + // +k8s:validateFalse="field Data" + Data map[string]string `json:"data"` +} + +type ItemList struct { + TypeMeta int + + // +k8s:listType=map + // +k8s:listMapKey=key + Items []Item `json:"items"` +} diff --git a/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/list/doc_test.go b/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/list/doc_test.go index 08e3b9c9d8c..bb5aaeeaead 100644 --- a/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/list/doc_test.go +++ b/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/list/doc_test.go @@ -99,3 +99,19 @@ func Test_StructSlice(t *testing.T) { MapSliceMixedKeyField: []MixedKeyStruct{{Key1: ptr.To("a"), Key2: "1", Data: "A"}, {Key1: ptr.To("b"), Key2: "2", Data: "B"}}, }).ExpectValid() } + +// This test is to prove the bug of ratcheting behavior mistakenly skip validation on nil vs not found. +// TODO: update this test once the ratcheting behavior is fixed. +func Test_Items(t *testing.T) { + st := localSchemeBuilder.Test(t) + + st.Value(&ItemList{ + Items: []Item{ + {Key: "valid2"}, + }, + }).OldValue(&ItemList{ + Items: []Item{ + {Key: "valid1"}, + }, + }).ExpectValid() +} diff --git a/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/list/zz_generated.validations.go b/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/list/zz_generated.validations.go index 08c0f731d7f..e740378a902 100644 --- a/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/list/zz_generated.validations.go +++ b/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/list/zz_generated.validations.go @@ -38,6 +38,14 @@ func init() { localSchemeBuilder.Register(RegisterValidations) } // RegisterValidations adds validation functions to the given scheme. // Public to allow building arbitrary schemes. func RegisterValidations(scheme *testscheme.Scheme) error { + // type ItemList + scheme.AddValidationFunc((*ItemList)(nil), func(ctx context.Context, op operation.Operation, obj, oldObj interface{}) field.ErrorList { + switch op.Request.SubresourcePath() { + case "/": + return Validate_ItemList(ctx, op, nil /* fldPath */, obj.(*ItemList), safe.Cast[*ItemList](oldObj)) + } + return field.ErrorList{field.InternalError(nil, fmt.Errorf("no validation found for %T, subresource: %v", obj, op.Request.SubresourcePath()))} + }) // type StructSlice scheme.AddValidationFunc((*StructSlice)(nil), func(ctx context.Context, op operation.Operation, obj, oldObj interface{}) field.ErrorList { switch op.Request.SubresourcePath() { @@ -49,6 +57,49 @@ func RegisterValidations(scheme *testscheme.Scheme) error { return nil } +// Validate_Item validates an instance of Item according +// to declarative validation rules in the API schema. +func Validate_Item(ctx context.Context, op operation.Operation, fldPath *field.Path, obj, oldObj *Item) (errs field.ErrorList) { + // field Item.Key has no validation + + // field Item.Data + errs = append(errs, + func(fldPath *field.Path, obj, oldObj map[string]string) (errs field.ErrorList) { + // don't revalidate unchanged data + if op.Type == operation.Update && equality.Semantic.DeepEqual(obj, oldObj) { + return nil + } + // call field-attached validations + errs = append(errs, validate.FixedResult(ctx, op, fldPath, obj, oldObj, false, "field Data")...) + return + }(fldPath.Child("data"), obj.Data, safe.Field(oldObj, func(oldObj *Item) map[string]string { return oldObj.Data }))...) + + return errs +} + +// Validate_ItemList validates an instance of ItemList according +// to declarative validation rules in the API schema. +func Validate_ItemList(ctx context.Context, op operation.Operation, fldPath *field.Path, obj, oldObj *ItemList) (errs field.ErrorList) { + // field ItemList.TypeMeta has no validation + + // field ItemList.Items + errs = append(errs, + func(fldPath *field.Path, obj, oldObj []Item) (errs field.ErrorList) { + // don't revalidate unchanged data + if op.Type == operation.Update && equality.Semantic.DeepEqual(obj, oldObj) { + return nil + } + // call field-attached validations + // lists with map semantics require unique keys + errs = append(errs, validate.Unique(ctx, op, fldPath, obj, oldObj, func(a Item, b Item) bool { return a.Key == b.Key })...) + // iterate the list and call the type's validation function + errs = append(errs, validate.EachSliceVal(ctx, op, fldPath, obj, oldObj, func(a Item, b Item) bool { return a.Key == b.Key }, validate.SemanticDeepEqual, Validate_Item)...) + return + }(fldPath.Child("items"), obj.Items, safe.Field(oldObj, func(oldObj *ItemList) []Item { return oldObj.Items }))...) + + return errs +} + // Validate_MixedKeyStruct validates an instance of MixedKeyStruct according // to declarative validation rules in the API schema. func Validate_MixedKeyStruct(ctx context.Context, op operation.Operation, fldPath *field.Path, obj, oldObj *MixedKeyStruct) (errs field.ErrorList) { diff --git a/staging/src/k8s.io/code-generator/cmd/validation-gen/testscheme/doc.go b/staging/src/k8s.io/code-generator/cmd/validation-gen/testscheme/doc.go index 5d0f21e1672..618fb7f72cd 100644 --- a/staging/src/k8s.io/code-generator/cmd/validation-gen/testscheme/doc.go +++ b/staging/src/k8s.io/code-generator/cmd/validation-gen/testscheme/doc.go @@ -34,7 +34,9 @@ limitations under the License. // by generated test fixtures. // // For example, to test by hand. The testschema provides utilities to create a value and assert -// that the expected errors are returned when the value is validated: +// that the expected errors are returned when the value is validated. +// Note that if `OldValue()` or `OldValueFuzzed()` is called on the `ValidationTester`, subsequent +// validation calls will implicitly use update validation. // // func Test(t *testing.T) { // st := localSchemeBuilder.Test(t) diff --git a/staging/src/k8s.io/code-generator/cmd/validation-gen/testscheme/testscheme.go b/staging/src/k8s.io/code-generator/cmd/validation-gen/testscheme/testscheme.go index 0f4fe1849b4..5261c73dfe9 100644 --- a/staging/src/k8s.io/code-generator/cmd/validation-gen/testscheme/testscheme.go +++ b/staging/src/k8s.io/code-generator/cmd/validation-gen/testscheme/testscheme.go @@ -245,16 +245,18 @@ type ValidationTester struct { *ValidationTestBuilder value any oldValue any + isUpdate bool options []string subresources []string } -// OldValue sets the oldValue for this ValidationTester. When oldValue is set to -// a non-nil value, update validation will be used to test validation. +// OldValue sets the oldValue for this ValidationTester. When oldValue is set, +// update validation will be used to test validation. // oldValue must be the same type as value. // Returns ValidationTester to support call chaining. func (v *ValidationTester) OldValue(oldValue any) *ValidationTester { v.oldValue = oldValue + v.isUpdate = true return v } @@ -263,6 +265,7 @@ func (v *ValidationTester) OldValue(oldValue any) *ValidationTester { func (v *ValidationTester) OldValueFuzzed(oldValue any) *ValidationTester { randfiller().Fill(oldValue) v.oldValue = oldValue + v.isUpdate = true return v } @@ -370,11 +373,8 @@ func (v *ValidationTester) ExpectMatches(matcher field.ErrorMatcher, expected fi } func (v *ValidationTester) validate() field.ErrorList { - var errs field.ErrorList - if v.oldValue == nil { - errs = v.s.Validate(context.Background(), v.options, v.value, v.subresources...) - } else { - errs = v.s.ValidateUpdate(context.Background(), v.options, v.value, v.oldValue, v.subresources...) + if v.isUpdate { + return v.s.ValidateUpdate(context.Background(), v.options, v.value, v.oldValue, v.subresources...) } - return errs + return v.s.Validate(context.Background(), v.options, v.value, v.subresources...) }