missing path and non bool as err

Signed-off-by: Feng Shao <shaof777@gmail.com>

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)
 }

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