From e58ce34aa1a4b763a2075025c75f3906b7a6050a Mon Sep 17 00:00:00 2001 From: Kartik Suryavanshi <158498247+KartikSuryavanshi@users.noreply.github.com> Date: Wed, 15 Jul 2026 16:54:11 +0530 Subject: [PATCH] fix: use errors.As with content check for duplicate key detection Replace fragile string matching in isDuplicateKeyError with a type-based check using errors.As and *yamlv2.TypeError, combined with content checking for "already set in map". The error from sigs.k8s.io/yaml wraps the underlying yaml.TypeError, and errors.As properly unwraps it. This eliminates the dead code branches for "already defined" and "duplicate" strings that were never produced by the yaml parser, while still being precise enough to not misclassify other yaml.TypeError variants as duplicate key errors. Signed-off-by: Kartik Suryavanshi Signed-off-by: Kartik Suryavanshi <158498247+KartikSuryavanshi@users.noreply.github.com> --- internal/chart/v3/lint/rules/values.go | 18 +++++-- internal/chart/v3/lint/rules/values_test.go | 58 +++++++++++++++++++++ 2 files changed, 71 insertions(+), 5 deletions(-) diff --git a/internal/chart/v3/lint/rules/values.go b/internal/chart/v3/lint/rules/values.go index a91e70363..9ebf405c4 100644 --- a/internal/chart/v3/lint/rules/values.go +++ b/internal/chart/v3/lint/rules/values.go @@ -23,6 +23,8 @@ import ( "path/filepath" "strings" + yamlv2 "go.yaml.in/yaml/v2" + "helm.sh/helm/v4/internal/chart/v3/lint/support" "helm.sh/helm/v4/pkg/chart/common" "helm.sh/helm/v4/pkg/chart/common/util" @@ -89,10 +91,16 @@ func validateValuesFile(valuesPath string, overrides map[string]any, skipSchemaV return nil } -// isDuplicateKeyError checks if an error is related to duplicate YAML keys +// isDuplicateKeyError checks if an error is related to duplicate YAML keys. func isDuplicateKeyError(err error) bool { - errStr := err.Error() - return strings.Contains(errStr, "already set in map") || - strings.Contains(errStr, "already defined") || - strings.Contains(errStr, "duplicate") + var typeErr *yamlv2.TypeError + if !errors.As(err, &typeErr) { + return false + } + for _, e := range typeErr.Errors { + if strings.Contains(e, "already set in map") { + return true + } + } + return false } diff --git a/internal/chart/v3/lint/rules/values_test.go b/internal/chart/v3/lint/rules/values_test.go index 54c7e6457..0324930ed 100644 --- a/internal/chart/v3/lint/rules/values_test.go +++ b/internal/chart/v3/lint/rules/values_test.go @@ -17,11 +17,14 @@ limitations under the License. package rules import ( + "errors" + "fmt" "os" "path/filepath" "testing" "github.com/stretchr/testify/assert" + yamlv2 "go.yaml.in/yaml/v2" "helm.sh/helm/v4/internal/test/ensure" ) @@ -181,3 +184,58 @@ func createTestingSchema(t *testing.T, dir string) string { } return schemafile } + +func TestIsDuplicateKeyError(t *testing.T) { + tests := []struct { + name string + err error + expected bool + }{ + { + name: "duplicate key error", + err: &yamlv2.TypeError{Errors: []string{`line 2: key "key" already set in map`}}, + expected: true, + }, + { + name: "wrapped duplicate key error", + err: fmt.Errorf("error converting YAML to JSON: %w", &yamlv2.TypeError{Errors: []string{`line 2: key "key" already set in map`}}), + expected: true, + }, + { + name: "non-duplicate key error", + err: errors.New("some other error"), + expected: false, + }, + { + name: "type error but not duplicate-key related", + err: &yamlv2.TypeError{Errors: []string{"cannot unmarshal !!seq into map[string]interface {}"}}, + expected: false, + }, + { + name: "nil error", + err: nil, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := isDuplicateKeyError(tt.err) + assert.Equal(t, tt.expected, result) + }) + } +} + +func TestValidateValuesFileDuplicateKeys(t *testing.T) { + duplicateYaml := `key: value1 +key: value2 +` + tmpdir := ensure.TempFile(t, "values.yaml", []byte(duplicateYaml)) + valfile := filepath.Join(tmpdir, "values.yaml") + + err := validateValuesFile(valfile, map[string]any{}, false) + if err == nil { + t.Fatal("expected values file with duplicate keys to fail parsing") + } + assert.Contains(t, err.Error(), "contains duplicate keys") +}