From 35180a99b1d65652ce45735509cc9b070a046afd Mon Sep 17 00:00:00 2001 From: John Ryan Date: Tue, 30 Aug 2022 15:07:07 -0700 Subject: [PATCH 1/2] Give validation rules priority and severity - the higher the priority, the earlier a rule runs - if a rule is "fatal" it stops further rule checking on that validation. --- ...is-null-short-circuits-other-rules.tpltest | 7 +++++ pkg/validations/validate.go | 27 +++++++++++++++++++ 2 files changed, 34 insertions(+) create mode 100644 pkg/validations/filetests/not_null=/when-value-is-null-short-circuits-other-rules.tpltest diff --git a/pkg/validations/filetests/not_null=/when-value-is-null-short-circuits-other-rules.tpltest b/pkg/validations/filetests/not_null=/when-value-is-null-short-circuits-other-rules.tpltest new file mode 100644 index 00000000..cad59998 --- /dev/null +++ b/pkg/validations/filetests/not_null=/when-value-is-null-short-circuits-other-rules.tpltest @@ -0,0 +1,7 @@ +#@assert/validate min=1, not_null=True +value: null + ++++ + +ERR: +- "value" (stdin:2) requires "not null"; fail: value is null (by stdin:1) diff --git a/pkg/validations/validate.go b/pkg/validations/validate.go index 8323cd82..66ca7b21 100644 --- a/pkg/validations/validate.go +++ b/pkg/validations/validate.go @@ -6,6 +6,7 @@ package validations import ( "fmt" "reflect" + "sort" "github.com/k14s/starlark-go/starlark" "github.com/vmware-tanzu/carvel-ytt/pkg/filepos" @@ -26,6 +27,23 @@ type NodeValidation struct { type rule struct { msg string assertion starlark.Callable + priority int // how early to run this rule. 0 = order it appears; more positive: earlier, more negative: later. + isFatal bool // whether not satisfying this rule prevents others rules from running. +} + +// byPriority is a sort.Interface that orders rules based on priority +type byPriority []rule + +func (r byPriority) Len() int { + return len(r) +} +func (r byPriority) Swap(idx, jdx int) { + r[idx], r[jdx] = r[jdx], r[idx] +} + +// Less reports whether the rule at "idx" should run _later_ than the rule at "jdx". +func (r byPriority) Less(idx, jdx int) bool { + return r[idx].priority > r[jdx].priority } // validationKwargs represent the optional keyword arguments and their values in a validationRun annotation. @@ -106,14 +124,21 @@ func (v NodeValidation) Validate(node yamlmeta.Node, thread *starlark.Thread) [] return nil } + sort.Sort(byPriority(v.rules)) var failures []error for _, r := range v.rules { result, err := starlark.Call(thread, r.assertion, starlark.Tuple{nodeValue}, []starlark.Tuple{}) if err != nil { failures = append(failures, fmt.Errorf("%s (%s) requires %q; %s (by %s)", key, node.GetPosition().AsCompactString(), r.msg, err.Error(), v.position.AsCompactString())) + if r.isFatal { + break + } } else { if !(result == starlark.True) { failures = append(failures, fmt.Errorf("%s (%s) requires %q (by %s)", key, node.GetPosition().AsCompactString(), r.msg, v.position.AsCompactString())) + if r.isFatal { + break + } } } } @@ -187,6 +212,8 @@ func (v validationKwargs) asRules() []rule { rules = append(rules, rule{ msg: fmt.Sprintf("not null"), assertion: yttlibrary.NewAssertNotNull().CheckFunc(), + isFatal: true, + priority: 100, }) } if v.oneNotNull != nil { From 04f9ab6860b5091f2d050558300c1f87773941d8 Mon Sep 17 00:00:00 2001 From: John Ryan Date: Wed, 31 Aug 2022 04:55:15 -0700 Subject: [PATCH 2/2] Preserve original order of rules as we sort - and use a less violent synonym of "fatal" without losing clarity. In response to feedback: https://kubernetes.slack.com/archives/CH8KCCKA5/p1661901649424219?thread_ts=1661901631.192439&cid=CH8KCCKA5 --- pkg/validations/validate.go | 44 ++++++++++++++++--------------------- 1 file changed, 19 insertions(+), 25 deletions(-) diff --git a/pkg/validations/validate.go b/pkg/validations/validate.go index 66ca7b21..a846afaa 100644 --- a/pkg/validations/validate.go +++ b/pkg/validations/validate.go @@ -25,25 +25,20 @@ type NodeValidation struct { // A rule contains a string description of what constitutes a valid value, // and a function that asserts the rule against an actual value. type rule struct { - msg string - assertion starlark.Callable - priority int // how early to run this rule. 0 = order it appears; more positive: earlier, more negative: later. - isFatal bool // whether not satisfying this rule prevents others rules from running. + msg string + assertion starlark.Callable + priority int // how early to run this rule. 0 = order it appears; more positive: earlier, more negative: later. + isCritical bool // whether not satisfying this rule prevents others rules from running. } -// byPriority is a sort.Interface that orders rules based on priority -type byPriority []rule - -func (r byPriority) Len() int { - return len(r) -} -func (r byPriority) Swap(idx, jdx int) { - r[idx], r[jdx] = r[jdx], r[idx] -} - -// Less reports whether the rule at "idx" should run _later_ than the rule at "jdx". -func (r byPriority) Less(idx, jdx int) bool { - return r[idx].priority > r[jdx].priority +// byPriority sorts (a copy) of "rules" by priority in descending order (i.e. the order in which the rules should run) +func byPriority(rules []rule) []rule { + sorted := make([]rule, len(rules)) + copy(sorted, rules) + sort.SliceStable(sorted, func(idx, jdx int) bool { + return sorted[idx].priority > sorted[jdx].priority + }) + return sorted } // validationKwargs represent the optional keyword arguments and their values in a validationRun annotation. @@ -124,19 +119,18 @@ func (v NodeValidation) Validate(node yamlmeta.Node, thread *starlark.Thread) [] return nil } - sort.Sort(byPriority(v.rules)) var failures []error - for _, r := range v.rules { + for _, r := range byPriority(v.rules) { result, err := starlark.Call(thread, r.assertion, starlark.Tuple{nodeValue}, []starlark.Tuple{}) if err != nil { failures = append(failures, fmt.Errorf("%s (%s) requires %q; %s (by %s)", key, node.GetPosition().AsCompactString(), r.msg, err.Error(), v.position.AsCompactString())) - if r.isFatal { + if r.isCritical { break } } else { if !(result == starlark.True) { failures = append(failures, fmt.Errorf("%s (%s) requires %q (by %s)", key, node.GetPosition().AsCompactString(), r.msg, v.position.AsCompactString())) - if r.isFatal { + if r.isCritical { break } } @@ -210,10 +204,10 @@ func (v validationKwargs) asRules() []rule { } if v.notNull { rules = append(rules, rule{ - msg: fmt.Sprintf("not null"), - assertion: yttlibrary.NewAssertNotNull().CheckFunc(), - isFatal: true, - priority: 100, + msg: fmt.Sprintf("not null"), + assertion: yttlibrary.NewAssertNotNull().CheckFunc(), + isCritical: true, + priority: 100, }) } if v.oneNotNull != nil {