From c15c0eb0cbac2158d442b31985890d3e9cf6dafa Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 1 Aug 2016 15:06:20 -0400 Subject: [PATCH 1/2] Disallow strings as arguments to concat The concat interpolation function now only accepts list arguments. Strings are no longer supported, for concatenation or appending to lists. All arguments must be a list, and single elements can be promoted with the `list` interpolation function. --- config/interpolate_funcs.go | 4 +--- config/interpolate_funcs_test.go | 25 ++++++++++++++++++------- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/config/interpolate_funcs.go b/config/interpolate_funcs.go index 87e9de778..8470b38e2 100644 --- a/config/interpolate_funcs.go +++ b/config/interpolate_funcs.go @@ -320,8 +320,6 @@ func interpolationFuncConcat() ast.Function { for _, arg := range args { switch arg := arg.(type) { - case string: - outputList = append(outputList, ast.Variable{Type: ast.TypeString, Value: arg}) case []ast.Variable: for _, v := range arg { switch v.Type { @@ -337,7 +335,7 @@ func interpolationFuncConcat() ast.Function { } default: - return nil, fmt.Errorf("concat() does not support %T", arg) + return nil, fmt.Errorf("concat() does not support type %T", arg) } } diff --git a/config/interpolate_funcs_test.go b/config/interpolate_funcs_test.go index 11450f668..0d4f38d9c 100644 --- a/config/interpolate_funcs_test.go +++ b/config/interpolate_funcs_test.go @@ -362,17 +362,19 @@ func TestInterpolateFuncConcat(t *testing.T) { testFunction(t, testFunctionConfig{ Cases: []testFunctionCase{ // String + list + // no longer supported, now returns an error { `${concat("a", split(",", "b,c"))}`, - []interface{}{"a", "b", "c"}, - false, + nil, + true, }, // List + string + // no longer supported, now returns an error { `${concat(split(",", "a,b"), "c")}`, - []interface{}{"a", "b", "c"}, - false, + nil, + true, }, // Single list @@ -427,6 +429,14 @@ func TestInterpolateFuncConcat(t *testing.T) { false, }, + // multiple strings + // no longer supported, now returns an error + { + `${concat("string1", "string2")}`, + nil, + true, + }, + // mismatched types { `${concat("${var.lists}", "${var.maps}")}`, @@ -1532,15 +1542,16 @@ type testFunctionCase struct { func testFunction(t *testing.T, config testFunctionConfig) { for i, tc := range config.Cases { + fmt.Println("running", i) ast, err := hil.Parse(tc.Input) if err != nil { - t.Fatalf("Case #%d: input: %#v\nerr: %s", i, tc.Input, err) + t.Fatalf("Case #%d: input: %#v\nerr: %v", i, tc.Input, err) } result, err := hil.Eval(ast, langEvalConfig(config.Vars)) - t.Logf("err: %s", err) + t.Logf("err: %v", err) if err != nil != tc.Error { - t.Fatalf("Case #%d:\ninput: %#v\nerr: %s", i, tc.Input, err) + t.Fatalf("Case #%d:\ninput: %#v\nerr: %v", i, tc.Input, err) } if !reflect.DeepEqual(result.Value, tc.Result) { From c9e15221035670f75fd094696ebe704e42eb3aae Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 1 Aug 2016 15:24:18 -0400 Subject: [PATCH 2/2] Use HIL to limit concat to ast.TypeList we can remove some type checks in the concat function --- config/interpolate_funcs.go | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/config/interpolate_funcs.go b/config/interpolate_funcs.go index 8470b38e2..099369064 100644 --- a/config/interpolate_funcs.go +++ b/config/interpolate_funcs.go @@ -311,31 +311,25 @@ func interpolationFuncCoalesce() ast.Function { // multiple lists. func interpolationFuncConcat() ast.Function { return ast.Function{ - ArgTypes: []ast.Type{ast.TypeAny}, + ArgTypes: []ast.Type{ast.TypeList}, ReturnType: ast.TypeList, Variadic: true, - VariadicType: ast.TypeAny, + VariadicType: ast.TypeList, Callback: func(args []interface{}) (interface{}, error) { var outputList []ast.Variable for _, arg := range args { - switch arg := arg.(type) { - case []ast.Variable: - for _, v := range arg { - switch v.Type { - case ast.TypeString: - outputList = append(outputList, v) - case ast.TypeList: - outputList = append(outputList, v) - case ast.TypeMap: - outputList = append(outputList, v) - default: - return nil, fmt.Errorf("concat() does not support lists of %s", v.Type.Printable()) - } + for _, v := range arg.([]ast.Variable) { + switch v.Type { + case ast.TypeString: + outputList = append(outputList, v) + case ast.TypeList: + outputList = append(outputList, v) + case ast.TypeMap: + outputList = append(outputList, v) + default: + return nil, fmt.Errorf("concat() does not support lists of %s", v.Type.Printable()) } - - default: - return nil, fmt.Errorf("concat() does not support type %T", arg) } }