diff --git a/pkg/cmd/template/schema_consumer_test.go b/pkg/cmd/template/schema_consumer_test.go index 25f28572..1db8c8a5 100644 --- a/pkg/cmd/template/schema_consumer_test.go +++ b/pkg/cmd/template/schema_consumer_test.go @@ -1546,7 +1546,7 @@ foo: assertFailsWithSchemaAndDataValues(t, schemaYAML, valuesYAML, expectedErrMsg) }) - t.Run("when @schema/nullable, skips if value is null (unless either not_null=True or when_null_skip=False)", func(t *testing.T) { + t.Run("when @schema/nullable, skips if value is null (unless not_null=True)", func(t *testing.T) { schemaYAML := `#@data/values-schema --- #@schema/nullable @@ -1555,15 +1555,11 @@ foo: 0 #@schema/nullable #@schema/validation ("bar > 2", lambda v: True if v == None else v > 2), not_null=True bar: 0 -#@schema/nullable -#@schema/validation ("qux > 2", lambda v: v > 2), when_null_skip=False -qux: 0 ` valuesYAML := `` expectedErrMsg := `One or more data values were invalid: - "bar" (schema.yaml:8) requires "not null"; fail: value is null (by schema.yaml:7) -- "qux" (schema.yaml:11) requires "qux > 2"; NoneType > int not implemented (by schema.yaml:10) ` assertFailsWithSchemaAndDataValues(t, schemaYAML, valuesYAML, expectedErrMsg) }) diff --git a/pkg/schema/schema.go b/pkg/schema/schema.go index 660ada1d..c8ddc7ac 100644 --- a/pkg/schema/schema.go +++ b/pkg/schema/schema.go @@ -24,7 +24,7 @@ func NewDocumentType(doc *yamlmeta.Document) (*DocumentType, error) { return nil, err } - v, err := getValidation(doc, typeOfValue) + v, err := getValidation(doc) if err != nil { return nil, err } @@ -59,7 +59,7 @@ func NewMapItemType(item *yamlmeta.MapItem) (*MapItemType, error) { return nil, err } - v, err := getValidation(item, typeOfValue) + v, err := getValidation(item) if err != nil { return nil, err } @@ -99,7 +99,7 @@ func NewArrayItemType(item *yamlmeta.ArrayItem) (*ArrayItemType, error) { return nil, err } - v, err := getValidation(item, typeOfValue) + v, err := getValidation(item) if err != nil { return nil, err } @@ -163,7 +163,7 @@ func getValue(node yamlmeta.Node, t Type) (interface{}, error) { return t.GetDefaultValue(), nil } -func getValidation(node yamlmeta.Node, nodeType Type) (*validations.NodeValidation, error) { +func getValidation(node yamlmeta.Node) (*validations.NodeValidation, error) { if !experiments.IsValidationsEnabled() { return nil, nil } @@ -174,9 +174,6 @@ func getValidation(node yamlmeta.Node, nodeType Type) (*validations.NodeValidati } if validationAnn != nil { - if _, ok := nodeType.(*NullType); ok { - validationAnn.GetValidation().DefaultNullSkipTrue() - } return validationAnn.GetValidation(), nil } return nil, nil diff --git a/pkg/validations/annotations.go b/pkg/validations/annotations.go index dfb5a8ec..2edc8bca 100644 --- a/pkg/validations/annotations.go +++ b/pkg/validations/annotations.go @@ -16,15 +16,14 @@ import ( const ( AnnotationAssertValidate template.AnnotationName = "assert/validate" - KwargWhen string = "when" - KwargWhenNullSkip string = "when_null_skip" - KwargMinLength string = "min_len" - KwargMaxLength string = "max_len" - KwargMin string = "min" - KwargMax string = "max" - KwargNotNull string = "not_null" - KwargOneNotNull string = "one_not_null" - KwargOneOf string = "one_of" + KwargWhen string = "when" + KwargMinLength string = "min_len" + KwargMaxLength string = "max_len" + KwargMin string = "min" + KwargMax string = "max" + KwargNotNull string = "not_null" + KwargOneNotNull string = "one_not_null" + KwargOneOf string = "one_of" ) // ProcessAssertValidateAnns checks Assert annotations on data values and stores them on a Node as Validations. @@ -138,13 +137,6 @@ func newValidationKwargs(kwargs []starlark.Tuple, annPos *filepos.Position) (val return validationKwargs{}, fmt.Errorf("expected keyword argument %q to be a function, but was %s (at %s)", KwargWhen, value[1].Type(), annPos.AsCompactString()) } processedKwargs.when = v - case KwargWhenNullSkip: - v, ok := value[1].(starlark.Bool) - if !ok { - return validationKwargs{}, fmt.Errorf("expected keyword argument %q to be a boolean, but was %s (at %s)", KwargWhenNullSkip, value[1].Type(), annPos.AsCompactString()) - } - b := bool(v) - processedKwargs.whenNullSkip = &b case KwargMinLength: v, err := starlark.NumberToInt(value[1]) if err != nil { diff --git a/pkg/validations/filetests/when_null_skip=/false--runs-rules.tpltest b/pkg/validations/filetests/when_null_skip=/false--runs-rules.tpltest deleted file mode 100644 index 7027802e..00000000 --- a/pkg/validations/filetests/when_null_skip=/false--runs-rules.tpltest +++ /dev/null @@ -1,7 +0,0 @@ -#@assert/validate ("", lambda v: fail("rules were run")), when_null_skip=False -foo: null - -+++ - -ERR: -- "foo" (stdin:2) requires ""; fail: rules were run (by stdin:1) diff --git a/pkg/validations/filetests/when_null_skip=/rejects-non-bool-arg.tpltest b/pkg/validations/filetests/when_null_skip=/rejects-non-bool-arg.tpltest deleted file mode 100644 index dd5a7dfb..00000000 --- a/pkg/validations/filetests/when_null_skip=/rejects-non-bool-arg.tpltest +++ /dev/null @@ -1,6 +0,0 @@ -#@assert/validate when_null_skip="True" -foo: null - -+++ - -ERR: Invalid @assert/validate annotation - expected keyword argument "when_null_skip" to be a boolean, but was string (at stdin:1) \ No newline at end of file diff --git a/pkg/validations/filetests/when_null_skip=/true--and-not_null--runs-rules.tpltest b/pkg/validations/filetests/when_null_skip=/true--and-not_null--runs-rules.tpltest index 6bfd4c08..fcb97330 100644 --- a/pkg/validations/filetests/when_null_skip=/true--and-not_null--runs-rules.tpltest +++ b/pkg/validations/filetests/when_null_skip=/true--and-not_null--runs-rules.tpltest @@ -1,4 +1,4 @@ -#@assert/validate when_null_skip=True, not_null=True +#@assert/validate not_null=True foo: null +++ diff --git a/pkg/validations/filetests/when_null_skip=/true--and-value-is-null--skips-rules.tpltest b/pkg/validations/filetests/when_null_skip=/true--and-value-is-null--skips-rules.tpltest index c7a81314..3482e8db 100644 --- a/pkg/validations/filetests/when_null_skip=/true--and-value-is-null--skips-rules.tpltest +++ b/pkg/validations/filetests/when_null_skip=/true--and-value-is-null--skips-rules.tpltest @@ -1,4 +1,4 @@ -#@assert/validate ("", lambda v: fail("fails")), when_null_skip=True +#@assert/validate ("", lambda v: fail("fails")) foo: null +++ diff --git a/pkg/validations/filetests/when_null_skip=/true--and-value-not-null--runs-rules.tpltest b/pkg/validations/filetests/when_null_skip=/true--and-value-not-null--runs-rules.tpltest index eba83dde..452e22c8 100644 --- a/pkg/validations/filetests/when_null_skip=/true--and-value-not-null--runs-rules.tpltest +++ b/pkg/validations/filetests/when_null_skip=/true--and-value-not-null--runs-rules.tpltest @@ -1,4 +1,4 @@ -#@assert/validate ("", lambda v: fail("rules were run")), when_null_skip=True +#@assert/validate ("", lambda v: fail("rules were run")) foo: "" +++ diff --git a/pkg/validations/validate.go b/pkg/validations/validate.go index 8323cd82..18cfb7bc 100644 --- a/pkg/validations/validate.go +++ b/pkg/validations/validate.go @@ -30,15 +30,14 @@ type rule struct { // validationKwargs represent the optional keyword arguments and their values in a validationRun annotation. type validationKwargs struct { - when starlark.Callable - whenNullSkip *bool // default: nil if kwarg is not set, True if value is Nullable - minLength *starlark.Int // 0 len("") == 0, this always passes - maxLength *starlark.Int - min starlark.Value - max starlark.Value - notNull bool - oneNotNull starlark.Value // valid values are either starlark.Sequence or starlark.Bool - oneOf starlark.Sequence + when starlark.Callable + minLength *starlark.Int // 0 len("") == 0, this always passes + maxLength *starlark.Int + min starlark.Value + max starlark.Value + notNull bool + oneNotNull starlark.Value // valid values are either starlark.Sequence or starlark.Bool + oneOf starlark.Sequence } // Run takes a root Node, and threadName, and validates each Node in the tree. @@ -120,22 +119,11 @@ func (v NodeValidation) Validate(node yamlmeta.Node, thread *starlark.Thread) [] return failures } -// DefaultNullSkipTrue sets the kwarg when_null_skip to true if not set explicitly. -func (v *NodeValidation) DefaultNullSkipTrue() { - if v.kwargs.whenNullSkip == nil { - t := true - v.kwargs.whenNullSkip = &t - } -} - // shouldValidate uses validationKwargs and the node's value to run checks on the value. If the value satisfies the checks, // then the NodeValidation's rules should execute, otherwise the rules will be skipped. func (v validationKwargs) shouldValidate(value starlark.Value, thread *starlark.Thread) (bool, error) { - // avoid nil pointer errors: handle `when_null_skip=`, first _, valueIsNull := value.(starlark.NoneType) - nullIsAllowed := !v.notNull - whenNullSkip := v.whenNullSkip != nil && *v.whenNullSkip - if valueIsNull && nullIsAllowed && whenNullSkip { + if valueIsNull && !v.notNull { return false, nil }