Merge pull request #737 from vmware-tanzu/732-remove-when_null_skip

Remove unnecessary when_null_skip= keyword argument

pkg/cmd/template/schema_consumer_test.go

@@ -1573,7 +1573,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
@@ -1582,15 +1582,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)
 	})

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
 	}
@@ -159,7 +159,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
 	}
@@ -170,9 +170,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

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 {

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
 
 +++

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
 
 +++

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: ""
 
 +++

pkg/validations/validate.go

@@ -43,15 +43,14 @@ func byPriority(rules []rule) []rule {
 
 // 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.
@@ -139,22 +138,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
 	}