diff --git a/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/each.go b/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/each.go index 9e7fc7e459a..c3e20de6fbb 100644 --- a/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/each.go +++ b/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/each.go @@ -116,6 +116,8 @@ func (lttv listTypeTagValidator) Docs() TagDoc { Description: "", Docs: "map | atomic", }}, + PayloadsType: codetags.ValueTypeString, + PayloadsRequired: true, } return doc } @@ -174,6 +176,8 @@ func (lmktv listMapKeyTagValidator) Docs() TagDoc { Description: "", Docs: "The name of the field.", }}, + PayloadsType: codetags.ValueTypeString, + PayloadsRequired: true, } return doc } @@ -318,6 +322,8 @@ func (evtv eachValTagValidator) Docs() TagDoc { Description: "", Docs: "The tag to evaluate for each value.", }}, + PayloadsType: codetags.ValueTypeTag, + PayloadsRequired: true, } return doc } @@ -355,9 +361,6 @@ func (ektv eachKeyTagValidator) GetValidations(context Context, tag codetags.Tag Parent: t, Path: context.Path.Child("(keys)"), } - if tag.ValueTag == nil { - return Validations{}, fmt.Errorf("missing validation tag") - } if validations, err := ektv.validator.ExtractValidations(elemContext, *tag.ValueTag); err != nil { return Validations{}, err } else { @@ -394,6 +397,8 @@ func (ektv eachKeyTagValidator) Docs() TagDoc { Description: "", Docs: "The tag to evaluate for each value.", }}, + PayloadsType: codetags.ValueTypeTag, + PayloadsRequired: true, } return doc } diff --git a/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/limits.go b/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/limits.go index c4753a27e67..47f52aa2473 100644 --- a/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/limits.go +++ b/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/limits.go @@ -79,5 +79,7 @@ func (mtv minimumTagValidator) Docs() TagDoc { Description: "", Docs: "This field must be greater than or equal to x.", }}, + PayloadsType: codetags.ValueTypeInt, + PayloadsRequired: true, } } diff --git a/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/registry.go b/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/registry.go index 34d660b6c13..228e3b3174e 100644 --- a/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/registry.go +++ b/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/registry.go @@ -134,6 +134,9 @@ func (reg *registry) ExtractValidations(context Context, tags ...codetags.Tag) ( if scopes := tv.ValidScopes(); !scopes.Has(context.Scope) && !scopes.Has(ScopeAny) { return Validations{}, fmt.Errorf("tag %q cannot be specified on %s", tv.TagName(), context.Scope) } + if err := typeCheck(tag, tv.Docs()); err != nil { + return Validations{}, fmt.Errorf("tag %q: %w", tv.TagName(), err) + } if theseValidations, err := tv.GetValidations(context, tag); err != nil { return Validations{}, fmt.Errorf("tag %q: %w", tv.TagName(), err) } else { diff --git a/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/subfield.go b/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/subfield.go index de1d51d0236..7643dd364d2 100644 --- a/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/subfield.go +++ b/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/subfield.go @@ -62,26 +62,18 @@ func (stv subfieldTagValidator) GetValidations(context Context, tag codetags.Tag if t.Kind != types.Struct { return Validations{}, fmt.Errorf("can only be used on struct types") } - if len(args) != 1 || len(args[0].Name) != 0 || args[0].Type != codetags.ArgTypeString { - return Validations{}, fmt.Errorf("requires exactly 1 positional string argument") - } subname := args[0].Value submemb := getMemberByJSON(t, subname) if submemb == nil { return Validations{}, fmt.Errorf("no field for json name %q", subname) } - result := Validations{} - subContext := Context{ Scope: ScopeField, Type: submemb.Type, Parent: t, Path: context.Path.Child(subname), } - if tag.ValueTag == nil { - return Validations{}, fmt.Errorf("missing validation tag") - } if validations, err := stv.validator.ExtractValidations(subContext, *tag.ValueTag); err != nil { return Validations{}, err } else { @@ -121,12 +113,16 @@ func (stv subfieldTagValidator) Docs() TagDoc { Description: "Declares a validation for a subfield of a struct.", Args: []TagArgDoc{{ Description: "", + Type: codetags.ArgTypeString, + Required: true, }}, Docs: "The named subfield must be a direct field of the struct, or of an embedded struct.", Payloads: []TagPayloadDoc{{ Description: "", Docs: "The tag to evaluate for the subfield.", }}, + PayloadsType: codetags.ValueTypeTag, + PayloadsRequired: true, } return doc } diff --git a/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/testing.go b/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/testing.go index 81d28fc4fb9..adc873ef38b 100644 --- a/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/testing.go +++ b/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/testing.go @@ -124,6 +124,7 @@ func (frtv fixedResultTagValidator) Docs() TagDoc { Tag: frtv.TagName(), Scopes: frtv.ValidScopes().UnsortedList(), } + doc.PayloadsType = codetags.ValueTypeString if frtv.error { doc.Description = "Always fails code generation (useful for testing)." doc.Payloads = []TagPayloadDoc{{ @@ -135,21 +136,19 @@ func (frtv fixedResultTagValidator) Docs() TagDoc { doc.Payloads = []TagPayloadDoc{{ Description: "", }, { - Description: "", + Description: "", Docs: "The generated code will include this string.", }} doc.Args = []TagArgDoc{{ Name: "flags", - Description: "", + Description: "", Docs: `values: ShortCircuit, NonError`, - }, { - Name: "msg", - Description: "", - Docs: "The generated code will include this string.", + Type: codetags.ArgTypeString, }, { Name: "typeArg", Description: "", Docs: "The type arg in generated code (must be the value-type, not pointer).", + Type: codetags.ArgTypeString, }} if frtv.result { doc.Description = "Always passes validation (useful for testing)." diff --git a/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/validators.go b/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/validators.go index f3641b040ea..012926ec178 100644 --- a/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/validators.go +++ b/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/validators.go @@ -17,6 +17,8 @@ limitations under the License. package validators import ( + "fmt" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/gengo/v2/codetags" @@ -176,6 +178,19 @@ type TagDoc struct { // never has a payload, this list should be empty, but if the payload is // optional, this list should include an entry for "". Payloads []TagPayloadDoc + // PayloadsType is the type of the payloads. + PayloadsType codetags.ValueType + // PayloadsRequired is true if a payload is required. + PayloadsRequired bool +} + +func (td TagDoc) Arg(name string) (TagArgDoc, bool) { + for _, arg := range td.Args { + if arg.Name == name { + return arg, true + } + } + return TagArgDoc{}, false } // TagArgDoc describes an argument for a tag. @@ -189,9 +204,13 @@ type TagArgDoc struct { Name string // Description is a short description of this arg (e.g. ``). Description string + // Type is the type of the arg. + Type codetags.ArgType + // Required is true if the argument is required. + Required bool // Default is the effective value if no value is provided. Default string - // Docs is a human-orientd string explaining this payload. + // Docs is a human-oriented string explaining this arg. Docs string } @@ -202,16 +221,6 @@ type TagPayloadDoc struct { Description string // Docs is a human-oriented string explaining this payload. Docs string - // Schema details a JSON payload's contents. - Schema []TagPayloadSchema -} - -// TagPayloadSchema describes a JSON tag payload. -type TagPayloadSchema struct { - Key string - Value string - Docs string - Default string } // Validations define the function calls and variables to generate to perform @@ -440,3 +449,42 @@ type ParamResult struct { Name string Type *types.Type } + +// typeCheck checks that the argument and value types of the tag match the types +// declared in the doc. +func typeCheck(tag codetags.Tag, doc TagDoc) error { + for _, docArg := range doc.Args { + hasArg := false + for _, tagArg := range tag.Args { + if tagArg.Name == docArg.Name { + hasArg = true + if docArg.Type != tagArg.Type { + return fmt.Errorf("argument %q has wrong type: got %s, want %s", + tagArg, tagArg.Type, docArg.Type) + } + break + } + } + if !hasArg && docArg.Required { + if docArg.Name == "" { + return fmt.Errorf("missing required positional argument of type %s", docArg.Type) + } else { + return fmt.Errorf("missing named argument %q of type %s", docArg.Name, docArg.Type) + } + } + } + + for _, tagArg := range tag.Args { + if _, ok := doc.Arg(tagArg.Name); !ok { + return fmt.Errorf("unrecognized named argument %q", tagArg) + } + } + if tag.ValueType == codetags.ValueTypeNone { + if doc.PayloadsRequired { + return fmt.Errorf("missing required tag value of type %s", doc.PayloadsType) + } + } else if tag.ValueType != doc.PayloadsType { + return fmt.Errorf("tag value has wrong type: got %s, want %s", tag.ValueType, doc.PayloadsType) + } + return nil +} diff --git a/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/validators_test.go b/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/validators_test.go new file mode 100644 index 00000000000..5586168b467 --- /dev/null +++ b/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/validators_test.go @@ -0,0 +1,219 @@ +/* +Copyright 2024 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validators + +import ( + "strings" + "testing" + + "k8s.io/gengo/v2/codetags" +) + +func TestTypeCheck(t *testing.T) { + + mkt := func(args []codetags.Arg, valueType codetags.ValueType) codetags.Tag { + return codetags.Tag{ + Args: args, + ValueType: valueType, + } + } + + mkta := func(name string, value string, argType codetags.ArgType) codetags.Arg { + return codetags.Arg{ + Name: name, + Value: value, + Type: argType, + } + } + + mkd := func(args []TagArgDoc, payloadsRequired bool, payloadsType codetags.ValueType) TagDoc { + return TagDoc{ + Args: args, + PayloadsRequired: payloadsRequired, + PayloadsType: payloadsType, + } + } + + mkda := func(name string, argType codetags.ArgType, required bool) TagArgDoc { + return TagArgDoc{ + Name: name, + Type: argType, + Required: required, + } + } + tests := []struct { + name string + tag codetags.Tag + doc TagDoc + wantErr string + }{ + { + name: "valid tag with no args", + tag: mkt(nil, codetags.ValueTypeNone), + doc: mkd(nil, false, codetags.ValueTypeNone), + }, + + // named args + { + name: "valid tag with required named args", + tag: mkt([]codetags.Arg{ + mkta("arg1", "value1", codetags.ArgTypeString), + mkta("arg2", "value2", codetags.ArgTypeString), + }, codetags.ValueTypeNone), + doc: mkd([]TagArgDoc{ + mkda("arg1", codetags.ArgTypeString, true), + mkda("arg2", codetags.ArgTypeString, true), + }, false, codetags.ValueTypeNone), + }, + { + name: "valid tag with optional named args", + tag: mkt([]codetags.Arg{ + mkta("arg1", "value1", codetags.ArgTypeString), + mkta("arg2", "value2", codetags.ArgTypeString), + }, codetags.ValueTypeNone), + doc: mkd([]TagArgDoc{ + mkda("arg1", codetags.ArgTypeString, false), + mkda("arg2", codetags.ArgTypeString, false), + }, false, codetags.ValueTypeNone), + }, + { + name: "valid tag without optional named args", + tag: mkt([]codetags.Arg{}, codetags.ValueTypeNone), + doc: mkd([]TagArgDoc{ + mkda("arg1", codetags.ArgTypeString, false), + mkda("arg2", codetags.ArgTypeString, false), + }, false, codetags.ValueTypeNone), + }, + { + name: "missing required named argument", + tag: mkt(nil, codetags.ValueTypeNone), + doc: mkd([]TagArgDoc{ + mkda("arg1", codetags.ArgTypeString, true), + }, false, codetags.ValueTypeNone), + wantErr: "missing named argument", + }, + { + name: "named argument with wrong type", + tag: mkt([]codetags.Arg{ + mkta("arg1", "value1", codetags.ArgTypeString), + }, codetags.ValueTypeNone), + doc: mkd([]TagArgDoc{ + mkda("arg1", codetags.ArgTypeInt, true), + }, false, codetags.ValueTypeNone), + wantErr: "has wrong type: got string, want int", + }, + { + name: "unrecognized named argument", + tag: mkt([]codetags.Arg{ + mkta("arg1", "value1", codetags.ArgTypeString), + mkta("arg2", "value2", codetags.ArgTypeString), + }, codetags.ValueTypeNone), + doc: mkd([]TagArgDoc{ + mkda("arg1", codetags.ArgTypeString, true), + }, false, codetags.ValueTypeNone), + wantErr: "unrecognized named argument", + }, + + // positional arg + { + name: "valid tag with required positional arg", + tag: mkt([]codetags.Arg{ + mkta("", "value1", codetags.ArgTypeString), + }, codetags.ValueTypeNone), + doc: mkd([]TagArgDoc{ + mkda("", codetags.ArgTypeString, true), + }, false, codetags.ValueTypeNone), + }, + { + name: "valid tag with optional positional arg", + tag: mkt([]codetags.Arg{ + mkta("", "value1", codetags.ArgTypeString), + }, codetags.ValueTypeNone), + doc: mkd([]TagArgDoc{ + mkda("", codetags.ArgTypeString, false), + }, false, codetags.ValueTypeNone), + }, + { + name: "valid tag without optional positional arg", + tag: mkt([]codetags.Arg{}, codetags.ValueTypeNone), + doc: mkd([]TagArgDoc{ + mkda("", codetags.ArgTypeString, false), + }, false, codetags.ValueTypeNone), + }, + { + name: "missing required positional argument", + tag: mkt(nil, codetags.ValueTypeNone), + doc: mkd([]TagArgDoc{ + mkda("", codetags.ArgTypeString, true), + }, false, codetags.ValueTypeNone), + wantErr: "missing required positional argument", + }, + { + name: "positional argument with wrong type", + tag: mkt([]codetags.Arg{ + mkta("", "value1", codetags.ArgTypeString), + }, codetags.ValueTypeNone), + doc: mkd([]TagArgDoc{ + mkda("", codetags.ArgTypeInt, true), + }, false, codetags.ValueTypeNone), + wantErr: "has wrong type: got string, want int", + }, + + // values + { + name: "valid required tag value", + tag: mkt(nil, codetags.ValueTypeString), + doc: mkd(nil, true, codetags.ValueTypeString), + }, + { + name: "valid with optional tag value", + tag: mkt(nil, codetags.ValueTypeString), + doc: mkd(nil, false, codetags.ValueTypeString), + }, + { + name: "valid without optional tag value", + tag: mkt(nil, codetags.ValueTypeNone), + doc: mkd(nil, false, codetags.ValueTypeString), + }, + { + name: "missing required tag value", + tag: mkt(nil, codetags.ValueTypeNone), + doc: mkd(nil, true, codetags.ValueTypeString), + wantErr: "missing required tag value", + }, + { + name: "tag value with wrong type", + tag: mkt(nil, codetags.ValueTypeString), + doc: mkd(nil, false, codetags.ValueTypeInt), + wantErr: "tag value has wrong type", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := typeCheck(tt.tag, tt.doc) + if (len(tt.wantErr) == 0) != (err == nil) { + t.Errorf("typeCheck() error = %v, wantErr = %v", err, tt.wantErr) + } + if err != nil && tt.wantErr != "" { + if !strings.Contains(err.Error(), tt.wantErr) { + t.Errorf("typeCheck() error = %v, wantErr = %v", err, tt.wantErr) + } + } + }) + } +}