Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: Change string list representation so we can distinguish empty, single element lists #2504

Merged
merged 4 commits into from
Jun 26, 2015
Merged

core: Change string list representation so we can distinguish empty, single element lists #2504

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
10b3abf
config: introduce StringList to abstract over list hack
phinze Jun 10, 2015
7238b3b
core: encapsulate representation of StringList
phinze Jun 25, 2015
e88aeed
core: allow distinguishing between empty lists and strings
phinze Jun 25, 2015
c95f21c
config: make formatlist work on lists of length 1
phinze Jun 25, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 25 additions & 20 deletions config/interpolate_funcs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,13 @@ func interpolationFuncConcat() ast.Function {
continue
}

if strings.Contains(argument, InterpSplitDelim) {
if IsStringList(argument) {
isDeprecated = false
finalList = append(finalList, StringList(argument).Slice()...)
} else {
finalList = append(finalList, argument)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was implicitly relying on the fact that the old representation of StringLists allowed them to be arbitrarily combined with bare elements.

For example, previously: ["foo", "barSLDbaz", "qux"] would make it through the strings.Join as a valid list. Now we need to respect the encapsulation of the type and handle it explicitly.

}

finalList = append(finalList, argument)

// Deprecated concat behaviour
b.WriteString(argument)
}
Expand All @@ -68,7 +69,7 @@ func interpolationFuncConcat() ast.Function {
return b.String(), nil
}

return strings.Join(finalList, InterpSplitDelim), nil
return NewStringList(finalList).String(), nil
},
}
}
Expand Down Expand Up @@ -131,11 +132,16 @@ func interpolationFuncFormatList() ast.Function {
if !ok {
continue
}
parts := strings.Split(s, InterpSplitDelim)
if len(parts) == 1 {
if !IsStringList(s) {
continue
}

parts := StringList(s).Slice()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was previously relying on the fact that a bare string would have no problem being split. Need to handle it explicitly now.


// otherwise the list is sent down to be indexed
varargs[i-1] = parts

// Check length
if n == 0 {
// first list we've seen
n = len(parts)
Expand Down Expand Up @@ -167,7 +173,7 @@ func interpolationFuncFormatList() ast.Function {
}
list[i] = fmt.Sprintf(format, fmtargs...)
}
return strings.Join(list, InterpSplitDelim), nil
return NewStringList(list).String(), nil
},
}
}
Expand All @@ -181,7 +187,7 @@ func interpolationFuncJoin() ast.Function {
Callback: func(args []interface{}) (interface{}, error) {
var list []string
for _, arg := range args[1:] {
parts := strings.Split(arg.(string), InterpSplitDelim)
parts := StringList(arg.(string)).Slice()
list = append(list, parts...)
}

Expand Down Expand Up @@ -223,18 +229,15 @@ func interpolationFuncLength() ast.Function {
ReturnType: ast.TypeInt,
Variadic: false,
Callback: func(args []interface{}) (interface{}, error) {
if !strings.Contains(args[0].(string), InterpSplitDelim) {
if !IsStringList(args[0].(string)) {
return len(args[0].(string)), nil
}

var list []string
length := 0
for _, arg := range args {
parts := strings.Split(arg.(string), InterpSplitDelim)
for _, part := range parts {
list = append(list, part)
}
length += StringList(arg.(string)).Length()
}
return len(list), nil
return length, nil
},
}
}
Expand All @@ -246,7 +249,9 @@ func interpolationFuncSplit() ast.Function {
ArgTypes: []ast.Type{ast.TypeString, ast.TypeString},
ReturnType: ast.TypeString,
Callback: func(args []interface{}) (interface{}, error) {
return strings.Replace(args[1].(string), args[0].(string), InterpSplitDelim, -1), nil
sep := args[0].(string)
s := args[1].(string)
return NewStringList(strings.Split(s, sep)).String(), nil
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't cheat with a Replace here anymore. 😀

},
}
}
Expand Down Expand Up @@ -284,15 +289,15 @@ func interpolationFuncElement() ast.Function {
ArgTypes: []ast.Type{ast.TypeString, ast.TypeString},
ReturnType: ast.TypeString,
Callback: func(args []interface{}) (interface{}, error) {
list := strings.Split(args[0].(string), InterpSplitDelim)
list := StringList(args[0].(string))

index, err := strconv.Atoi(args[1].(string))
if err != nil {
return "", fmt.Errorf(
"invalid number for index, got %s", args[1])
}

v := list[index%len(list)]
v := list.Element(index)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Easiest to just push this down to the StringList type - with the mod behavior living down there.

return v, nil
},
}
Expand Down Expand Up @@ -323,7 +328,7 @@ func interpolationFuncKeys(vs map[string]ast.Variable) ast.Function {

sort.Strings(keys)

return strings.Join(keys, InterpSplitDelim), nil
return NewStringList(keys).String(), nil
},
}
}
Expand Down Expand Up @@ -363,7 +368,7 @@ func interpolationFuncValues(vs map[string]ast.Variable) ast.Function {
vals = append(vals, vs[k].Value.(string))
}

return strings.Join(vals, InterpSplitDelim), nil
return NewStringList(vals).String(), nil
},
}
}
117 changes: 35 additions & 82 deletions config/interpolate_funcs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"io/ioutil"
"os"
"reflect"
"strings"
"testing"

"github.com/hashicorp/terraform/config/lang"
Expand Down Expand Up @@ -42,74 +41,46 @@ func TestInterpolateFuncConcat(t *testing.T) {
// String + list
{
`${concat("a", split(",", "b,c"))}`,
fmt.Sprintf(
"%s%s%s%s%s",
"a",
InterpSplitDelim,
"b",
InterpSplitDelim,
"c"),
NewStringList([]string{"a", "b", "c"}).String(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feelsgoodman.gif 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:D

false,
},

// List + string
{
`${concat(split(",", "a,b"), "c")}`,
fmt.Sprintf(
"%s%s%s%s%s",
"a",
InterpSplitDelim,
"b",
InterpSplitDelim,
"c"),
NewStringList([]string{"a", "b", "c"}).String(),
false,
},

// Single list
{
`${concat(split(",", ",foo,"))}`,
fmt.Sprintf(
"%s%s%s",
InterpSplitDelim,
"foo",
InterpSplitDelim),
NewStringList([]string{"", "foo", ""}).String(),
false,
},
{
`${concat(split(",", "a,b,c"))}`,
fmt.Sprintf(
"%s%s%s%s%s",
"a",
InterpSplitDelim,
"b",
InterpSplitDelim,
"c"),
NewStringList([]string{"a", "b", "c"}).String(),
false,
},

// Two lists
{
`${concat(split(",", "a,b,c"), split(",", "d,e"))}`,
strings.Join([]string{
"a", "b", "c", "d", "e",
}, InterpSplitDelim),
NewStringList([]string{"a", "b", "c", "d", "e"}).String(),
false,
},
// Two lists with different separators
{
`${concat(split(",", "a,b,c"), split(" ", "d e"))}`,
strings.Join([]string{
"a", "b", "c", "d", "e",
}, InterpSplitDelim),
NewStringList([]string{"a", "b", "c", "d", "e"}).String(),
false,
},

// More lists
{
`${concat(split(",", "a,b"), split(",", "c,d"), split(",", "e,f"), split(",", "0,1"))}`,
strings.Join([]string{
"a", "b", "c", "d", "e", "f", "0", "1",
}, InterpSplitDelim),
NewStringList([]string{"a", "b", "c", "d", "e", "f", "0", "1"}).String(),
false,
},
},
Expand Down Expand Up @@ -204,7 +175,7 @@ func TestInterpolateFuncFormatList(t *testing.T) {
// formatlist applies to each list element in turn
{
`${formatlist("<%s>", split(",", "A,B"))}`,
"<A>" + InterpSplitDelim + "<B>",
NewStringList([]string{"<A>", "<B>"}).String(),
false,
},
// formatlist repeats scalar elements
Expand All @@ -219,23 +190,18 @@ func TestInterpolateFuncFormatList(t *testing.T) {
"A=1, B=2, C=3",
false,
},
// formatlist of lists of length zero/one are repeated, just as scalars are
{
`${join(", ", formatlist("%s=%s", split(",", ""), split(",", "1,2,3")))}`,
"=1, =2, =3",
false,
},
{
`${join(", ", formatlist("%s=%s", split(",", "A"), split(",", "1,2,3")))}`,
"A=1, A=2, A=3",
false,
},
// Mismatched list lengths generate an error
{
`${formatlist("%s=%2s", split(",", "A,B,C,D"), split(",", "1,2,3"))}`,
nil,
true,
},
// Works with lists of length 1 [GH-2240]
{
`${formatlist("%s.id", split(",", "demo-rest-elb"))}`,
NewStringList([]string{"demo-rest-elb.id"}).String(),
false,
},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tiny detail was the instigator of this whole rabbit hole, heh.

},
})
}
Expand All @@ -250,7 +216,8 @@ func TestInterpolateFuncJoin(t *testing.T) {
},

{
`${join(",", "foo")}`,
fmt.Sprintf(`${join(",", "%s")}`,
NewStringList([]string{"foo"}).String()),
"foo",
false,
},
Expand All @@ -266,10 +233,7 @@ func TestInterpolateFuncJoin(t *testing.T) {

{
fmt.Sprintf(`${join(".", "%s")}`,
fmt.Sprintf(
"foo%sbar%sbaz",
InterpSplitDelim,
InterpSplitDelim)),
NewStringList([]string{"foo", "bar", "baz"}).String()),
"foo.bar.baz",
false,
},
Expand Down Expand Up @@ -386,47 +350,39 @@ func TestInterpolateFuncSplit(t *testing.T) {
true,
},

{
`${split(",", "")}`,
NewStringList([]string{""}).String(),
false,
},

{
`${split(",", "foo")}`,
"foo",
NewStringList([]string{"foo"}).String(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important detail that this is definitely output as a list now.

false,
},

{
`${split(",", ",,,")}`,
fmt.Sprintf(
"%s%s%s",
InterpSplitDelim,
InterpSplitDelim,
InterpSplitDelim),
NewStringList([]string{"", "", "", ""}).String(),
false,
},

{
`${split(",", "foo,")}`,
fmt.Sprintf(
"%s%s",
"foo",
InterpSplitDelim),
NewStringList([]string{"foo", ""}).String(),
false,
},

{
`${split(",", ",foo,")}`,
fmt.Sprintf(
"%s%s%s",
InterpSplitDelim,
"foo",
InterpSplitDelim),
NewStringList([]string{"", "foo", ""}).String(),
false,
},

{
`${split(".", "foo.bar.baz")}`,
fmt.Sprintf(
"foo%sbar%sbaz",
InterpSplitDelim,
InterpSplitDelim),
NewStringList([]string{"foo", "bar", "baz"}).String(),
false,
},
},
Expand Down Expand Up @@ -484,9 +440,7 @@ func TestInterpolateFuncKeys(t *testing.T) {
Cases: []testFunctionCase{
{
`${keys("foo")}`,
fmt.Sprintf(
"bar%squx",
InterpSplitDelim),
NewStringList([]string{"bar", "qux"}).String(),
false,
},

Expand Down Expand Up @@ -533,9 +487,7 @@ func TestInterpolateFuncValues(t *testing.T) {
Cases: []testFunctionCase{
{
`${values("foo")}`,
fmt.Sprintf(
"quack%sbaz",
InterpSplitDelim),
NewStringList([]string{"quack", "baz"}).String(),
false,
},

Expand Down Expand Up @@ -568,29 +520,30 @@ func TestInterpolateFuncElement(t *testing.T) {
Cases: []testFunctionCase{
{
fmt.Sprintf(`${element("%s", "1")}`,
"foo"+InterpSplitDelim+"baz"),
NewStringList([]string{"foo", "baz"}).String()),
"baz",
false,
},

{
`${element("foo", "0")}`,
fmt.Sprintf(`${element("%s", "0")}`,
NewStringList([]string{"foo"}).String()),
"foo",
false,
},

// Invalid index should wrap vs. out-of-bounds
{
fmt.Sprintf(`${element("%s", "2")}`,
"foo"+InterpSplitDelim+"baz"),
NewStringList([]string{"foo", "baz"}).String()),
"foo",
false,
},

// Too many args
{
fmt.Sprintf(`${element("%s", "0", "2")}`,
"foo"+InterpSplitDelim+"baz"),
NewStringList([]string{"foo", "baz"}).String()),
nil,
true,
},
Expand Down
Loading