From 09e8c44965d7568e45aaf8b45ccd05136a07613f Mon Sep 17 00:00:00 2001
From: Chitrang Patel <chitrang@google.com>
Date: Mon, 25 Apr 2022 14:40:48 -0400
Subject: [PATCH] Add validation for duplicated param names in `TaskSpec`

Prior to this, when duplicated names for parameters were assigned in `taskSpec`, the param value would be overwritten by the last value.

To avoid this, a validation check was added to ensure that no param name is duplicated in TaskSpec.
---
 pkg/apis/pipeline/v1beta1/task_validation.go  | 14 +++++++++----
 .../pipeline/v1beta1/task_validation_test.go  | 20 +++++++++++++++++++
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/pkg/apis/pipeline/v1beta1/task_validation.go b/pkg/apis/pipeline/v1beta1/task_validation.go
index e6baaed04eb..57a53a210b9 100644
--- a/pkg/apis/pipeline/v1beta1/task_validation.go
+++ b/pkg/apis/pipeline/v1beta1/task_validation.go
@@ -62,7 +62,7 @@ func (ts *TaskSpec) Validate(ctx context.Context) (errs *apis.FieldError) {
 
 	errs = errs.Also(validateSteps(ctx, mergedSteps).ViaField("steps"))
 	errs = errs.Also(ts.Resources.Validate(ctx).ViaField("resources"))
-	errs = errs.Also(ValidateParameterTypes(ts.Params).ViaField("params"))
+	errs = errs.Also(validateParameterSpecs(ts.Params).ViaField("params"))
 	errs = errs.Also(ValidateParameterVariables(ts.Steps, ts.Params))
 	errs = errs.Also(ValidateResourcesVariables(ts.Steps, ts.Resources))
 	errs = errs.Also(validateTaskContextVariables(ts.Steps))
@@ -244,12 +244,18 @@ func validateStep(ctx context.Context, s Step, names sets.String) (errs *apis.Fi
 	return errs
 }
 
-// ValidateParameterTypes validates all the types within a slice of ParamSpecs
-func ValidateParameterTypes(params []ParamSpec) (errs *apis.FieldError) {
+// ValidateParameterSpecs validates that there are no duplicate names in the TaskSpec and all the types within a slice of ParamSpecs.
+func validateParameterSpecs(params []ParamSpec) (errs *apis.FieldError) {
+	// validate type
 	for _, p := range params {
 		errs = errs.Also(p.ValidateType())
 	}
-	return errs
+	// validate no duplicate names
+	var names []string
+	for _, p := range params {
+		names = append(names, p.Name)
+	}
+	return errs.Also(validateNoDuplicateNames(names, false))
 }
 
 // ValidateType checks that the type of a ParamSpec is allowed and its default value matches that type
diff --git a/pkg/apis/pipeline/v1beta1/task_validation_test.go b/pkg/apis/pipeline/v1beta1/task_validation_test.go
index c860b927ded..7af80db8118 100644
--- a/pkg/apis/pipeline/v1beta1/task_validation_test.go
+++ b/pkg/apis/pipeline/v1beta1/task_validation_test.go
@@ -487,6 +487,26 @@ func TestTaskSpecValidateError(t *testing.T) {
 			Message: `expected exactly one, got both`,
 			Paths:   []string{"resources.outputs.name"},
 		},
+	}, {
+		name: "duplicated param names",
+		fields: fields{
+			Params: []v1beta1.ParamSpec{{
+				Name:        "foo",
+				Type:        v1beta1.ParamTypeString,
+				Description: "parameter",
+				Default:     v1beta1.NewArrayOrString("value1"),
+			}, {
+				Name:        "foo",
+				Type:        v1beta1.ParamTypeString,
+				Description: "parameter",
+				Default:     v1beta1.NewArrayOrString("value2"),
+			}},
+			Steps: validSteps,
+		},
+		expectedError: apis.FieldError{
+			Message: `expected exactly one, got both`,
+			Paths:   []string{"params[foo].name"},
+		},
 	}, {
 		name: "invalid param type",
 		fields: fields{