Skip to content

Commit

Permalink
Clean up Regexes
Browse files Browse the repository at this point in the history 
Prior to this commit, the regexes in substitution.go file were hard to
understand and hard to reuse because of the complexity of the regexes.
For example, we have to consider
  - all the three notations (dot notation, bracket with single quote,
  and bracket with double quote)
  - the operator suffix including star `[*]` operator and
  int indexing that was not considerd before.
Also the old regex coupled each notation part with the operator
part, which leads to extra step to be done in the extract-related functions
i.e. have to trim the [*] every time.

In this PR, we
  - clean up the regexes and make it as much reable and reuseble as possible
  - make the method `extractVariablesFromString` be capable of extracting
  both variable name and the operator (indexing operator or star operator)
  examples:
    - $(params.myString)       - name: "myString", operator: ""
    - $(params['myArray'][2])  - name: "myArray", operator: "1"
    - $(params.myObject[*])    - name: "myObject", operator: "*"

Enabling `extractVariablesFromString` to extract both name and operator
is particularly useful for array indexing validation. There will be more
PRs to come to clean up considering the complexity of the all regexes.
Also results-related regexes in resultref.go file need cleanup too.
  • Loading branch information
@chuangw6
chuangw6 committed Aug 15, 2022
1 parent 06a2887 commit 8551303
Show file tree
Hide file tree
Showing 9 changed files with 312 additions and 107 deletions.
11 changes: 2 additions & 9 deletions pkg/apis/pipeline/v1/param_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"context"
"encoding/json"
"fmt"
"regexp"
"strings"

resource "github.com/tektoncd/pipeline/pkg/apis/resource/v1alpha1"
Expand All @@ -29,12 +28,6 @@ import (
"knative.dev/pkg/apis"
)

// exactVariableSubstitutionFormat matches strings that only contain a single reference to result or param variables, but nothing else
// i.e. `$(result.resultname)` is a match, but `foo $(result.resultname)` is not.
const exactVariableSubstitutionFormat = `^\$\([_a-zA-Z0-9.-]+(\.[_a-zA-Z0-9.-]+)*(\[([0-9]+|\*)\])?\)$`

var exactVariableSubstitutionRegex = regexp.MustCompile(exactVariableSubstitutionFormat)

// ParamsPrefix is the prefix used in $(...) expressions referring to parameters
const ParamsPrefix = "params"

Expand Down Expand Up @@ -228,7 +221,7 @@ func (arrayOrString *ArrayOrString) applyOrCorrect(stringReplacements map[string

// if the stringVal is a string literal or a string that mixed with var references
// just do the normal string replacement
if !exactVariableSubstitutionRegex.MatchString(stringVal) {
if !isolatedResultVariableSubstitutionRegex.MatchString(stringVal) && !substitution.IsIsolatedArrayOrObjectParamRef(stringVal) {
arrayOrString.StringVal = substitution.ApplyReplacements(arrayOrString.StringVal, stringReplacements)
return
}
Expand Down Expand Up @@ -350,7 +343,7 @@ func validateParamStringValue(param Param, prefix string, paramNames sets.String
stringValue := param.Value.StringVal

// if the provided param value is an isolated reference to the whole array/object, we just check if the param name exists.
isIsolated, errs := substitution.ValidateWholeArrayOrObjectRefInStringVariable(param.Name, stringValue, prefix, paramNames)
isIsolated, errs := substitution.ValidateIsolatedArrayOrObjectRefInStringVariable(param.Name, stringValue, prefix, paramNames)
if isIsolated {
return errs
}
Expand Down
77 changes: 75 additions & 2 deletions pkg/apis/pipeline/v1/pipeline_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -996,7 +996,7 @@ func TestValidatePipelineParameterVariables_Success(t *testing.T) {
}},
}},
}, {
name: "array param - using the whole variable as a param's value that is intended to be array type",
name: "array param - using the whole variable (dot reference) as the value of an array param",
params: []ParamSpec{{
Name: "myArray",
Type: ParamTypeArray,
Expand All @@ -1008,6 +1008,45 @@ func TestValidatePipelineParameterVariables_Success(t *testing.T) {
Name: "a-param-intended-to-be-array", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(params.myArray[*])"},
}},
}},
}, {
name: "array param - using the whole variable (bracket with single quote) as the value of an array param",
params: []ParamSpec{{
Name: "myArray",
Type: ParamTypeArray,
}},
tasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "a-param-intended-to-be-array", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(params['myArray'][*])"},
}},
}},
}, {
name: "array param - using the whole variable (bracket with double quote) as the value of an array param",
params: []ParamSpec{{
Name: "myArray",
Type: ParamTypeArray,
}},
tasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "a-param-intended-to-be-array", Value: ArrayOrString{Type: ParamTypeString, StringVal: `$(params["myArray"][*])`},
}},
}},
}, {
name: "array index param - using the value of an array index as a string param",
params: []ParamSpec{{
Name: "myArray",
Type: ParamTypeArray,
}},
tasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "a-param-intended-to-be-from-an-array-index", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(params.myArray[0])"},
}},
}},
}, {
name: "object param - using single individual variable in string param",
params: []ParamSpec{{
Expand Down Expand Up @@ -1083,7 +1122,7 @@ func TestValidatePipelineParameterVariables_Success(t *testing.T) {
}},
}},
}, {
name: "object param - using the whole variable as a param's value that is intended to be object type",
name: "object param - using the whole variable (dot notation) as the value of an object param",
params: []ParamSpec{{
Name: "myObject",
Type: ParamTypeObject,
Expand All @@ -1099,6 +1138,40 @@ func TestValidatePipelineParameterVariables_Success(t *testing.T) {
Name: "a-param-intended-to-be-object", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(params.myObject[*])"},
}},
}},
}, {
name: "object param - using the whole variable (bracket notation with single quote) as the value of an object param",
params: []ParamSpec{{
Name: "myObject",
Type: ParamTypeObject,
Properties: map[string]PropertySpec{
"key1": {Type: "string"},
"key2": {Type: "string"},
},
}},
tasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "a-param-intended-to-be-object", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(params['myObject'][*])"},
}},
}},
}, {
name: "object param - using the whole variable (bracket notation with double quote) as the value of an object param",
params: []ParamSpec{{
Name: "myObject",
Type: ParamTypeObject,
Properties: map[string]PropertySpec{
"key1": {Type: "string"},
"key2": {Type: "string"},
},
}},
tasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "a-param-intended-to-be-object", Value: ArrayOrString{Type: ParamTypeString, StringVal: `$(params["myObject"][*])`},
}},
}},
}, {
name: "object param - using individual variable in input of when expression, and using both object individual variable and array reference in values of when expression",
params: []ParamSpec{{
Expand Down
16 changes: 11 additions & 5 deletions pkg/apis/pipeline/v1/resultref.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,18 +42,24 @@ const (
// ResultResultPart Constant used to define the "results" part of a pipeline result reference
ResultResultPart = "results"
// TODO(#2462) use one regex across all substitutions
// variableSubstitutionFormat matches format like $result.resultname, $result.resultname[int] and $result.resultname[*]
variableSubstitutionFormat = `\$\([_a-zA-Z0-9.-]+(\.[_a-zA-Z0-9.-]+)*(\[([0-9]+|\*)\])?\)`
// resultVariableSubstitutionFormat matches format like $result.resultname, $result.resultname[int] and $result.resultname[*]
resultVariableSubstitutionFormat = `\$\([_a-zA-Z0-9.-]+(\.[_a-zA-Z0-9.-]+)*(\[([0-9]+|\*)\])?\)`
// isolatedResultVariableSubstitutionFormat matches strings that only contain a single reference to result or param variables, but nothing else
// i.e. `$(result.resultname)` is a match, but `foo $(result.resultname)` is not.
isolatedResultVariableSubstitutionFormat = `^\$\([_a-zA-Z0-9.-]+(\.[_a-zA-Z0-9.-]+)*(\[([0-9]+|\*)\])?\)$`
// arrayIndexing will match all `[int]` and `[*]` for parseExpression
arrayIndexing = `\[([0-9])*\*?\]`
)

// VariableSubstitutionRegex is a regex to find all result matching substitutions
var VariableSubstitutionRegex = regexp.MustCompile(variableSubstitutionFormat)
// ResultVariableSubstitutionRegex is a regex to find all result matching substitutions
var ResultVariableSubstitutionRegex = regexp.MustCompile(resultVariableSubstitutionFormat)

// arrayIndexingRegex is used to match `[int]` and `[*]`
var arrayIndexingRegex = regexp.MustCompile(arrayIndexing)

// isolatedResultVariableSubstitutionRegex is used to check if a reference is an isolated result reference
var isolatedResultVariableSubstitutionRegex = regexp.MustCompile(isolatedResultVariableSubstitutionFormat)

// NewResultRefs extracts all ResultReferences from a param or a pipeline result.
// If the ResultReference can be extracted, they are returned. Expressions which are not
// results are ignored.
Expand Down Expand Up @@ -131,7 +137,7 @@ func GetVarSubstitutionExpressionsForPipelineResult(result PipelineResult) ([]st
}

func validateString(value string) []string {
expressions := VariableSubstitutionRegex.FindAllString(value, -1)
expressions := ResultVariableSubstitutionRegex.FindAllString(value, -1)
if expressions == nil {
return nil
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/apis/pipeline/v1beta1/param_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ func (arrayOrString *ArrayOrString) applyOrCorrect(stringReplacements map[string

// if the stringVal is a string literal or a string that mixed with var references
// just do the normal string replacement
if !exactVariableSubstitutionRegex.MatchString(stringVal) {
if !isolatedResultVariableSubstitutionRegex.MatchString(stringVal) && !substitution.IsIsolatedArrayOrObjectParamRef(stringVal) {
arrayOrString.StringVal = substitution.ApplyReplacements(arrayOrString.StringVal, stringReplacements)
return
}
Expand Down Expand Up @@ -338,7 +338,7 @@ func validateParamStringValue(param Param, prefix string, paramNames sets.String
stringValue := param.Value.StringVal

// if the provided param value is an isolated reference to the whole array/object, we just check if the param name exists.
isIsolated, errs := substitution.ValidateWholeArrayOrObjectRefInStringVariable(param.Name, stringValue, prefix, paramNames)
isIsolated, errs := substitution.ValidateIsolatedArrayOrObjectRefInStringVariable(param.Name, stringValue, prefix, paramNames)
if isIsolated {
return errs
}
Expand Down
77 changes: 75 additions & 2 deletions pkg/apis/pipeline/v1beta1/pipeline_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1517,7 +1517,7 @@ func TestValidatePipelineParameterVariables_Success(t *testing.T) {
}},
}},
}, {
name: "array param - using the whole variable as a param's value that is intended to be array type",
name: "array param - using the whole variable (dot reference) as the value of an array param",
params: []ParamSpec{{
Name: "myArray",
Type: ParamTypeArray,
Expand All @@ -1529,6 +1529,45 @@ func TestValidatePipelineParameterVariables_Success(t *testing.T) {
Name: "a-param-intended-to-be-array", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(params.myArray[*])"},
}},
}},
}, {
name: "array param - using the whole variable (bracket with single quote) as the value of an array param",
params: []ParamSpec{{
Name: "myArray",
Type: ParamTypeArray,
}},
tasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "a-param-intended-to-be-array", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(params['myArray'][*])"},
}},
}},
}, {
name: "array param - using the whole variable (bracket with double quote) as the value of an array param",
params: []ParamSpec{{
Name: "myArray",
Type: ParamTypeArray,
}},
tasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "a-param-intended-to-be-array", Value: ArrayOrString{Type: ParamTypeString, StringVal: `$(params["myArray"][*])`},
}},
}},
}, {
name: "array index param - using the value of an array index as a string param",
params: []ParamSpec{{
Name: "myArray",
Type: ParamTypeArray,
}},
tasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "a-param-intended-to-be-from-an-array-index", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(params.myArray[0])"},
}},
}},
}, {
name: "object param - using single individual variable in string param",
params: []ParamSpec{{
Expand Down Expand Up @@ -1621,7 +1660,7 @@ func TestValidatePipelineParameterVariables_Success(t *testing.T) {
}},
}},
}, {
name: "object param - using the whole variable as a param's value that is intended to be object type",
name: "object param - using the whole variable (dot notation) as the value of an object param",
params: []ParamSpec{{
Name: "myObject",
Type: ParamTypeObject,
Expand All @@ -1637,6 +1676,40 @@ func TestValidatePipelineParameterVariables_Success(t *testing.T) {
Name: "a-param-intended-to-be-object", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(params.myObject[*])"},
}},
}},
}, {
name: "object param - using the whole variable (bracket notation with single quote) as the value of an object param",
params: []ParamSpec{{
Name: "myObject",
Type: ParamTypeObject,
Properties: map[string]PropertySpec{
"key1": {Type: "string"},
"key2": {Type: "string"},
},
}},
tasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "a-param-intended-to-be-object", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(params['myObject'][*])"},
}},
}},
}, {
name: "object param - using the whole variable (bracket notation with double quote) as the value of an object param",
params: []ParamSpec{{
Name: "myObject",
Type: ParamTypeObject,
Properties: map[string]PropertySpec{
"key1": {Type: "string"},
"key2": {Type: "string"},
},
}},
tasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "a-param-intended-to-be-object", Value: ArrayOrString{Type: ParamTypeString, StringVal: `$(params["myObject"][*])`},
}},
}},
}, {
name: "object param - using individual variable in input of when expression, and using both object individual variable and array reference in values of when expression",
params: []ParamSpec{{
Expand Down
16 changes: 8 additions & 8 deletions pkg/apis/pipeline/v1beta1/resultref.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,20 +44,20 @@ const (
// ResultResultPart Constant used to define the "results" part of a pipeline result reference
ResultResultPart = "results"
// TODO(#2462) use one regex across all substitutions
// variableSubstitutionFormat matches format like $result.resultname, $result.resultname[int] and $result.resultname[*]
variableSubstitutionFormat = `\$\([_a-zA-Z0-9.-]+(\.[_a-zA-Z0-9.-]+)*(\[([0-9]+|\*)\])?\)`
// exactVariableSubstitutionFormat matches strings that only contain a single reference to result or param variables, but nothing else
// resultVariableSubstitutionFormat matches format like $result.resultname, $result.resultname[int] and $result.resultname[*]
resultVariableSubstitutionFormat = `\$\([_a-zA-Z0-9.-]+(\.[_a-zA-Z0-9.-]+)*(\[([0-9]+|\*)\])?\)`
// isolatedResultVariableSubstitutionFormat matches strings that only contain a single reference to result or param variables, but nothing else
// i.e. `$(result.resultname)` is a match, but `foo $(result.resultname)` is not.
exactVariableSubstitutionFormat = `^\$\([_a-zA-Z0-9.-]+(\.[_a-zA-Z0-9.-]+)*(\[([0-9]+|\*)\])?\)$`
isolatedResultVariableSubstitutionFormat = `^\$\([_a-zA-Z0-9.-]+(\.[_a-zA-Z0-9.-]+)*(\[([0-9]+|\*)\])?\)$`
// arrayIndexing will match all `[int]` and `[*]` for parseExpression
arrayIndexing = `\[([0-9])*\*?\]`
// ResultNameFormat Constant used to define the the regex Result.Name should follow
ResultNameFormat = `^([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]$`
)

// VariableSubstitutionRegex is a regex to find all result matching substitutions
var VariableSubstitutionRegex = regexp.MustCompile(variableSubstitutionFormat)
var exactVariableSubstitutionRegex = regexp.MustCompile(exactVariableSubstitutionFormat)
// ResultVariableSubstitutionRegex is a regex to find all result matching substitutions
var ResultVariableSubstitutionRegex = regexp.MustCompile(resultVariableSubstitutionFormat)
var isolatedResultVariableSubstitutionRegex = regexp.MustCompile(isolatedResultVariableSubstitutionFormat)
var resultNameFormatRegex = regexp.MustCompile(ResultNameFormat)

// arrayIndexingRegex is used to match `[int]` and `[*]`
Expand Down Expand Up @@ -140,7 +140,7 @@ func GetVarSubstitutionExpressionsForPipelineResult(result PipelineResult) ([]st
}

func validateString(value string) []string {
expressions := VariableSubstitutionRegex.FindAllString(value, -1)
expressions := ResultVariableSubstitutionRegex.FindAllString(value, -1)
if expressions == nil {
return nil
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciler/pipelinerun/resources/validate_params.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ func findInvalidParamArrayReference(paramReference string, arrayParams map[strin
for _, val := range list {
indexString := substitution.ExtractIndexString(paramReference)
idx, _ := substitution.ExtractIndex(indexString)
v := substitution.TrimArrayIndex(val)
v := substitution.TrimTailOperator(val)
if paramLength, ok := arrayParams[v]; ok {
if idx >= paramLength {
outofBoundParams.Insert(val)
Expand Down
4 changes: 2 additions & 2 deletions pkg/reconciler/taskrun/validate_resources.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ func wrongTypeParamsNames(params []v1beta1.Param, matrix []v1beta1.Param, needed
// to pass array result to array param, yet in yaml format this will be
// unmarshalled to string for ArrayOrString. So we need to check and skip this validation.
// Please refer issue #4879 for more details and examples.
if param.Value.Type == v1beta1.ParamTypeString && (neededParamsTypes[param.Name] == v1beta1.ParamTypeArray || neededParamsTypes[param.Name] == v1beta1.ParamTypeObject) && v1beta1.VariableSubstitutionRegex.MatchString(param.Value.StringVal) {
if param.Value.Type == v1beta1.ParamTypeString && (neededParamsTypes[param.Name] == v1beta1.ParamTypeArray || neededParamsTypes[param.Name] == v1beta1.ParamTypeObject) && v1beta1.ResultVariableSubstitutionRegex.MatchString(param.Value.StringVal) {
continue
}
if param.Value.Type != neededParamsTypes[param.Name] {
Expand Down Expand Up @@ -428,7 +428,7 @@ func extractParamIndex(paramReference string, arrayParams map[string]int, outofB
for _, val := range list {
indexString := substitution.ExtractIndexString(paramReference)
idx, _ := substitution.ExtractIndex(indexString)
v := substitution.TrimArrayIndex(val)
v := substitution.TrimTailOperator(val)
if paramLength, ok := arrayParams[v]; ok {
if idx >= paramLength {
outofBoundParams.Insert(val)
Expand Down
Loading

0 comments on commit 8551303

Please sign in to comment.