Skip to content

Commit

Permalink
Updated regex pattern to allow all three valid bracket notations.
Browse files Browse the repository at this point in the history 
Updated regex pattern to allow all three valid bracket notations.

Prior to this, when supplied parameter name was not the same as that
called in the step, webhook raised an error when using the dot notation
as expected. However, it did not when using the bracket notation.

Updated the unit tests to also check for valid bracket notations.
  • Loading branch information
@chitrangpatel
chitrangpatel committed May 4, 2022
1 parent bceff42 commit 6161681
Show file tree
Hide file tree
Showing 2 changed files with 100 additions and 8 deletions.
15 changes: 10 additions & 5 deletions pkg/substitution/substitution.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,7 @@ import (
)

const parameterSubstitution = `[_a-zA-Z][_a-zA-Z0-9.-]*(\[\*\])?`

const braceMatchingRegex = "(\\$(\\(%s\\.(?P<var>%s)\\)))"
const braceMatchingRegex = "(\\$(\\(%s(\\.(?P<var1>%s)|\\[\"(?P<var2>%s)\"\\]|\\['(?P<var3>%s)'\\])\\)))"

// ValidateVariable makes sure all variables in the provided string are known
func ValidateVariable(name, value, prefix, locationName, path string, vars sets.String) *apis.FieldError {
Expand Down Expand Up @@ -137,7 +136,7 @@ func ValidateVariableIsolatedP(value, prefix string, vars sets.String) *apis.Fie
// Extract a the first full string expressions found (e.g "$(input.params.foo)"). Return
// "" and false if nothing is found.
func extractExpressionFromString(s, prefix string) (string, bool) {
pattern := fmt.Sprintf(braceMatchingRegex, prefix, parameterSubstitution)
pattern := fmt.Sprintf(braceMatchingRegex, prefix, parameterSubstitution, parameterSubstitution, parameterSubstitution)
re := regexp.MustCompile(pattern)
match := re.FindStringSubmatch(s)
if match == nil {
Expand All @@ -147,7 +146,7 @@ func extractExpressionFromString(s, prefix string) (string, bool) {
}

func extractVariablesFromString(s, prefix string) ([]string, bool) {
pattern := fmt.Sprintf(braceMatchingRegex, prefix, parameterSubstitution)
pattern := fmt.Sprintf(braceMatchingRegex, prefix, parameterSubstitution, parameterSubstitution, parameterSubstitution)
re := regexp.MustCompile(pattern)
matches := re.FindAllStringSubmatch(s, -1)
if len(matches) == 0 {
Expand All @@ -159,7 +158,13 @@ func extractVariablesFromString(s, prefix string) ([]string, bool) {
// foo -> foo
// foo.bar -> foo
// foo.bar.baz -> foo
vars[i] = strings.SplitN(groups["var"], ".", 2)[0]
for _, v := range []string{"var1", "var2", "var3"} {
val := strings.SplitN(groups[v], ".", 2)[0]
if strings.SplitN(groups[v], ".", 2)[0] != "" {
vars[i] = val
break
}
}
}
return vars, true
}
Expand Down
93 changes: 90 additions & 3 deletions pkg/substitution/substitution_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func TestValidateVariables(t *testing.T) {
}, {
name: "multiple variables",
args: args{
input: "--flag=$(inputs.params.baz) $(input.params.foo)",
input: "--flag=$(inputs.params.baz) $(inputs.params.foo)",
prefix: "inputs.params",
locationName: "step",
path: "taskspec.steps",
Expand Down Expand Up @@ -95,14 +95,14 @@ func TestValidateVariables(t *testing.T) {
}, {
name: "undefined variable and defined variable",
args: args{
input: "--flag=$(inputs.params.baz) $(input.params.foo)",
input: "--flag=$(inputs.params.baz) $(inputs.params.foo)",
prefix: "inputs.params",
locationName: "step",
path: "taskspec.steps",
vars: sets.NewString("foo"),
},
expectedError: &apis.FieldError{
Message: `non-existent variable in "--flag=$(inputs.params.baz) $(input.params.foo)" for step somefield`,
Message: `non-existent variable in "--flag=$(inputs.params.baz) $(inputs.params.foo)" for step somefield`,
Paths: []string{"taskspec.steps.somefield"},
},
}} {
Expand All @@ -116,6 +116,93 @@ func TestValidateVariables(t *testing.T) {
}
}

func TestValidateVariablePs(t *testing.T) {
type args struct {
input string
prefix string
vars sets.String
}
for _, tc := range []struct {
name string
args args
expectedError *apis.FieldError
}{{
name: "valid variable",
args: args{
input: "--flag=$(inputs.params.baz)",
prefix: "inputs.params",
vars: sets.NewString("baz"),
},
expectedError: nil,
}, {
name: "valid variable with double quote bracket",
args: args{
input: "--flag=$(inputs.params[\"baz\"])",
prefix: "inputs.params",
vars: sets.NewString("baz"),
},
expectedError: nil,
}, {
name: "valid variable with single quotebracket",
args: args{
input: "--flag=$(inputs.params['baz'])",
prefix: "inputs.params",
vars: sets.NewString("baz"),
},
expectedError: nil,
}, {
name: "valid variable contains diffetent chars",
args: args{
input: "--flag=$(inputs.params['ba-_9z'])",
prefix: "inputs.params",
vars: sets.NewString("ba-_9z"),
},
expectedError: nil,
}, {
name: "valid variable uid",
args: args{
input: "--flag=$(context.taskRun.uid)",
prefix: "context.taskRun",
vars: sets.NewString("uid"),
},
expectedError: nil,
}, {
name: "multiple variables",
args: args{
input: "--flag=$(inputs.params.baz) $(inputs.params.foo)",
prefix: "inputs.params",
vars: sets.NewString("baz", "foo"),
},
expectedError: nil,
}, {
name: "different context and prefix",
args: args{
input: "--flag=$(something.baz)",
prefix: "something",
vars: sets.NewString("baz"),
},
expectedError: nil,
}, {
name: "undefined variable",
args: args{
input: "--flag=$(inputs.params.baz)",
prefix: "inputs.params",
vars: sets.NewString("foo"),
},
expectedError: &apis.FieldError{
Message: `non-existent variable in "--flag=$(inputs.params.baz)"`,
Paths: []string{""},
},
}} {
t.Run(tc.name, func(t *testing.T) {
got := substitution.ValidateVariableP(tc.args.input, tc.args.prefix, tc.args.vars)

if d := cmp.Diff(got, tc.expectedError, cmp.AllowUnexported(apis.FieldError{})); d != "" {
t.Errorf("ValidateVariableP() error did not match expected error %s", diff.PrintWantGot(d))
}
})
}
}
func TestApplyReplacements(t *testing.T) {
type args struct {
input string
Expand Down

0 comments on commit 6161681

Please sign in to comment.