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 <kartiksuryavanshi@users.noreply.github.com>
Signed-off-by: Kartik Suryavanshi <158498247+KartikSuryavanshi@users.noreply.github.com>
pull/32363/head
Kartik Suryavanshi 3 months ago
parent 65c3fc7581
commit e58ce34aa1

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

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

Loading…
Cancel
Save