From f3d205f526d547f45ec379c69897e85f67a831a6 Mon Sep 17 00:00:00 2001 From: Feng Shao Date: Tue, 21 Apr 2026 19:32:06 +0800 Subject: [PATCH] missing path and non bool as err Signed-off-by: Feng Shao --- pkg/chart/common/util/condition_expression.go | 63 ++++-- .../common/util/condition_expression_test.go | 211 ++++++++++++++++++ 2 files changed, 257 insertions(+), 17 deletions(-) create mode 100644 pkg/chart/common/util/condition_expression_test.go diff --git a/pkg/chart/common/util/condition_expression.go b/pkg/chart/common/util/condition_expression.go index fc238a284..76e3cba47 100644 --- a/pkg/chart/common/util/condition_expression.go +++ b/pkg/chart/common/util/condition_expression.go @@ -17,7 +17,7 @@ package util import ( "fmt" - "log" + "log/slog" "strings" "unicode" @@ -25,7 +25,7 @@ import ( ) type conditionExpression interface { - eval(conditionEvalContext) bool + eval(conditionEvalContext) (bool, error) } type conditionEvalContext struct { @@ -38,29 +38,33 @@ type conditionPath struct { path string } -func (n conditionPath) eval(ctx conditionEvalContext) bool { +func (n conditionPath) eval(ctx conditionEvalContext) (bool, error) { v, err := ctx.values.PathValue(ctx.chartPath + n.path) if err != nil { if _, ok := err.(common.ErrNoValue); !ok { - log.Printf("Warning: PathValue returned error %v", err) + slog.Warn("PathValue returned error", "error", err) } - return false + return false, err } b, ok := v.(bool) if !ok { - log.Printf("Warning: Condition path '%s' for chart %s returned non-bool value", n.path, ctx.chartName) - return false + slog.Warn("condition path returned non-bool value", "path", n.path, "chart", ctx.chartName) + return false, fmt.Errorf("condition path returned non-bool value") } - return b + return b, nil } type conditionNot struct { expr conditionExpression } -func (n conditionNot) eval(ctx conditionEvalContext) bool { - return !n.expr.eval(ctx) +func (n conditionNot) eval(ctx conditionEvalContext) (bool, error) { + result, err := n.expr.eval(ctx) + if err != nil { + return false, err + } + return !result, nil } type conditionAnd struct { @@ -68,9 +72,13 @@ type conditionAnd struct { right conditionExpression } -func (n conditionAnd) eval(ctx conditionEvalContext) bool { - if !n.left.eval(ctx) { - return false +func (n conditionAnd) eval(ctx conditionEvalContext) (bool, error) { + left, err := n.left.eval(ctx) + if err != nil { + return false, err + } + if !left { + return false, nil } return n.right.eval(ctx) } @@ -80,9 +88,13 @@ type conditionOr struct { right conditionExpression } -func (n conditionOr) eval(ctx conditionEvalContext) bool { - if n.left.eval(ctx) { - return true +func (n conditionOr) eval(ctx conditionEvalContext) (bool, error) { + left, err := n.left.eval(ctx) + if err != nil { + return false, err + } + if left { + return true, nil } return n.right.eval(ctx) } @@ -282,11 +294,28 @@ func (p *conditionParser) parsePrimary() (conditionExpression, error) { } } +// IsConditionExpression reports whether condition uses the boolean-expression +// syntax understood by EvaluateConditionExpression. +// +// A condition expression must be wrapped in a single outer pair of +// parentheses after trimming whitespace, for example `(subchart.enabled && +// !global.disabled)`. Inside the outer parentheses, operands are value paths +// made of letters, digits, `_`, `-`, and `.`, and they may be combined with +// `!`, `&&`, `||`, and nested parentheses. func IsConditionExpression(condition string) bool { trimmed := strings.TrimSpace(condition) return strings.HasPrefix(trimmed, "(") && strings.HasSuffix(trimmed, ")") } +// EvaluateConditionExpression parses and evaluates a condition expression +// against the provided values. +// +// The condition must follow the syntax recognized by IsConditionExpression: +// after trimming whitespace, the full expression is expected to be enclosed in +// outer parentheses, operands are value paths containing only letters, +// digits, `_`, `-`, and `.`, and operators are limited to `!`, `&&`, `||`, +// and nested parentheses. Evaluation returns an error if any referenced path +// is missing or resolves to a non-bool value. func EvaluateConditionExpression(condition string, cvals common.Values, cpath, chartName string) (bool, error) { expr, err := parseConditionExpression(condition) if err != nil { @@ -298,5 +327,5 @@ func EvaluateConditionExpression(condition string, cvals common.Values, cpath, c chartName: chartName, chartPath: cpath, } - return expr.eval(ctx), nil + return expr.eval(ctx) } diff --git a/pkg/chart/common/util/condition_expression_test.go b/pkg/chart/common/util/condition_expression_test.go new file mode 100644 index 000000000..86790a8a0 --- /dev/null +++ b/pkg/chart/common/util/condition_expression_test.go @@ -0,0 +1,211 @@ +/* +Copyright The Helm 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 util + +import ( + "strings" + "testing" + + "helm.sh/helm/v4/pkg/chart/common" +) + +func TestIsConditionExpression(t *testing.T) { + tests := []struct { + name string + condition string + want bool + }{ + {name: "trimmed outer parentheses", condition: " (enabled && !disabled) ", want: true}, + {name: "missing leading parenthesis", condition: "enabled && !disabled)", want: false}, + {name: "missing trailing parenthesis", condition: "(enabled && !disabled", want: false}, + {name: "plain path", condition: "enabled", want: false}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if got := IsConditionExpression(test.condition); got != test.want { + t.Fatalf("IsConditionExpression(%q) = %t, want %t", test.condition, got, test.want) + } + }) + } +} + +func TestParseConditionExpression(t *testing.T) { + values := common.Values{ + "enabled": true, + "disabled": false, + "other": false, + "nested": map[string]any{ + "enabled": true, + "disabled": false, + }, + } + + tests := []struct { + name string + expression string + want bool + }{ + { + name: "and binds tighter than or", + expression: "enabled || other && disabled", + want: true, + }, + { + name: "parentheses override precedence", + expression: "(enabled || other) && disabled", + want: false, + }, + { + name: "whitespace and unary not", + expression: " !nested.disabled && nested.enabled ", + want: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + expr, err := parseConditionExpression(test.expression) + if err != nil { + t.Fatalf("parseConditionExpression(%q) returned error: %v", test.expression, err) + } + + got, err := expr.eval(conditionEvalContext{values: values, chartName: "chart"}) + if err != nil { + t.Fatalf("expr.eval() returned error: %v", err) + } + if got != test.want { + t.Fatalf("parseConditionExpression(%q) evaluated to %t, want %t", test.expression, got, test.want) + } + }) + } +} + +func TestEvaluateConditionExpression(t *testing.T) { + values := common.Values{ + "parent": map[string]any{ + "enabled": true, + "guard": false, + "name": "demo", + }, + "fallback": true, + } + + tests := []struct { + name string + condition string + chartPath string + want bool + wantErr string + }{ + { + name: "uses chart path prefix", + condition: "(enabled && !guard)", + chartPath: "parent.", + want: true, + }, + { + name: "short circuit or avoids missing path error", + condition: "(enabled || missing)", + chartPath: "parent.", + want: true, + }, + { + name: "short circuit and avoids missing path error", + condition: "(guard && missing)", + chartPath: "parent.", + want: false, + }, + { + name: "missing values return error", + condition: "(missing || enabled)", + chartPath: "parent.", + wantErr: "\"missing\" is not a value", + }, + { + name: "non boolean values return error", + condition: "(name || guard)", + chartPath: "parent.", + wantErr: "condition path returned non-bool value", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got, err := EvaluateConditionExpression(test.condition, values, test.chartPath, "chart") + if test.wantErr != "" { + if err == nil { + t.Fatalf("EvaluateConditionExpression(%q) returned nil error, want %q", test.condition, test.wantErr) + } + if !strings.Contains(err.Error(), test.wantErr) { + t.Fatalf("EvaluateConditionExpression(%q) error = %q, want substring %q", test.condition, err.Error(), test.wantErr) + } + return + } + + if err != nil { + t.Fatalf("EvaluateConditionExpression(%q) returned error: %v", test.condition, err) + } + + if got != test.want { + t.Fatalf("EvaluateConditionExpression(%q) = %t, want %t", test.condition, got, test.want) + } + }) + } +} + +func TestParseConditionExpressionErrors(t *testing.T) { + tests := []struct { + name string + expression string + wantErr string + }{ + { + name: "missing closing parenthesis", + expression: "(enabled && (other || disabled)", + wantErr: "missing closing ')'", + }, + { + name: "single ampersand", + expression: "enabled & other", + wantErr: "unexpected token '&'", + }, + { + name: "invalid character", + expression: "enabled || $other", + wantErr: "unexpected token '$'", + }, + { + name: "operator without right operand", + expression: "enabled &&", + wantErr: "unexpected token \"\"", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + _, err := parseConditionExpression(test.expression) + if err == nil { + t.Fatalf("parseConditionExpression(%q) returned nil error", test.expression) + } + + if !strings.Contains(err.Error(), test.wantErr) { + t.Fatalf("parseConditionExpression(%q) error = %q, want substring %q", test.expression, err.Error(), test.wantErr) + } + }) + } +}