Simplify, document and add more tests to var replacement.

api/filters/refvar/doc.go

@@ -1,3 +1,3 @@
 // Package refvar contains a kio.Filter implementation of the kustomize
-// refvar transformer.
+// refvar transformer (find and replace $(FOO) style variables in strings).
 package refvar

api/filters/refvar/expand.go

@@ -1,12 +1,12 @@
 // Copyright 2019 The Kubernetes Authors.
 // SPDX-License-Identifier: Apache-2.0
 
-// Package expansion provides functions find and replace $(FOO) style variables in strings.
 package refvar
 
 import (
-	"bytes"
 	"fmt"
+	"log"
+	"strings"
 )
 
 const (
@@ -17,38 +17,64 @@ const (
 
 // syntaxWrap returns the input string wrapped by the expansion syntax.
 func syntaxWrap(input string) string {
-	return string(operator) + string(referenceOpener) + input + string(referenceCloser)
+	var sb strings.Builder
+	sb.WriteByte(operator)
+	sb.WriteByte(referenceOpener)
+	sb.WriteString(input)
+	sb.WriteByte(referenceCloser)
+	return sb.String()
 }
 
-// MappingFuncFor returns a mapping function for use with Expand that
-// implements the expansion semantics defined in the expansion spec; it
-// returns the input string wrapped in the expansion syntax if no mapping
-// for the input is found.
-func MappingFuncFor(
-	counts map[string]int,
-	context ...map[string]interface{}) func(string) interface{} {
-	return func(input string) interface{} {
-		for _, vars := range context {
-			val, ok := vars[input]
-			if ok {
-				counts[input]++
-				switch typedV := val.(type) {
-				case string, int64, float64, bool:
-					return typedV
-				default:
-					return syntaxWrap(input)
-				}
+// MappingFunc maps a string to anything.
+type MappingFunc func(string) interface{}
+
+// MakePrimitiveReplacer returns a MappingFunc that uses a map to do
+// replacements, and a histogram to count map hits.
+//
+// Func behavior:
+//
+// If the input key is NOT found in the map, the key is wrapped up as
+// as a variable declaration string and returned, e.g. key FOO becomes $(FOO).
+// This string is presumably put back where it was found, and might get replaced
+// later.
+//
+// If the key is found in the map, the value is returned if it is a primitive
+// type (string, bool, number), and the hit is counted.
+//
+// If it's not a primitive type (e.g. a map, struct, func, etc.) then this
+// function doesn't know what to do with it and it returns the key wrapped up
+// again as if it had not been replaced.  This should probably be an error.
+func MakePrimitiveReplacer(
+	counts map[string]int, someMap map[string]interface{}) MappingFunc {
+	return func(key string) interface{} {
+		if value, ok := someMap[key]; ok {
+			switch typedV := value.(type) {
+			case string, int, int32, int64, float32, float64, bool:
+				counts[key]++
+				return typedV
+			default:
+				// If the value is some complicated type (e.g. a map or struct),
+				// this function doesn't know how to jam it into a string,
+				// so just pretend it was a cache miss.
+				// Likely this should be an error instead of a silent failure,
+				// since the programmer passed an impossible value.
+				log.Printf(
+					"MakePrimitiveReplacer: bad replacement type=%T val=%v",
+					typedV, typedV)
+				return syntaxWrap(key)
 			}
 		}
-		return syntaxWrap(input)
+		// If unable to return the mapped variable, return it
+		// as it was found, and a later mapping might be able to
+		// replace it.
+		return syntaxWrap(key)
 	}
 }
 
-// Expand replaces variable references in the input string according to
-// the expansion spec using the given mapping function to resolve the
-// values of variables.
-func Expand(input string, mapping func(string) interface{}) interface{} {
-	var buf bytes.Buffer
+// DoReplacements replaces variable references in the input string
+// using the mapping function.
+func DoReplacements(input string, mapping MappingFunc) interface{} {
+	var buf strings.Builder
 	checkpoint := 0
 	for cursor := 0; cursor < len(input); cursor++ {
 		if input[cursor] == operator && cursor+1 < len(input) {

api/filters/refvar/expand_test.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	. "sigs.k8s.io/kustomize/api/filters/refvar"
 )
 
@@ -15,6 +16,48 @@ type expected struct {
 	edited string
 }
 
+func TestPrimitiveReplacer(t *testing.T) {
+	varCounts := make(map[string]int)
+	f := MakePrimitiveReplacer(
+		varCounts,
+		map[string]interface{}{
+			"FOO":        "bar",
+			"ZOO":        "$(FOO)-1",
+			"BLU":        "$(ZOO)-2",
+			"EIGHT":      8,
+			"PI":         3.14159,
+			"ZINT":       "$(INT)",
+			"BOOL":       "true",
+			"HUGENUMBER": int64(9223372036854775807),
+			"CRAZYMAP":   map[string]int{"crazy": 200},
+			"ZBOOL":      "$(BOOL)",
+		})
+	assert.Equal(t, "$()", f(""))
+	assert.Equal(t, "$(      )", f("      "))
+	assert.Equal(t, "$(florida)", f("florida"))
+	assert.Equal(t, "$(0)", f("0"))
+	assert.Equal(t, "bar", f("FOO"))
+	assert.Equal(t, "bar", f("FOO"))
+	assert.Equal(t, "bar", f("FOO"))
+	assert.Equal(t, 8, f("EIGHT"))
+	assert.Equal(t, 8, f("EIGHT"))
+	assert.Equal(t, 3.14159, f("PI"))
+	assert.Equal(t, "true", f("BOOL"))
+	assert.Equal(t, int64(9223372036854775807), f("HUGENUMBER"))
+	assert.Equal(t, "$(FOO)-1", f("ZOO"))
+	assert.Equal(t, "$(CRAZYMAP)", f("CRAZYMAP"))
+	assert.Equal(t,
+		map[string]int{
+			"FOO":        3,
+			"EIGHT":      2,
+			"BOOL":       1,
+			"PI":         1,
+			"ZOO":        1,
+			"HUGENUMBER": 1,
+		},
+		varCounts)
+}
+
 func TestMapReference(t *testing.T) {
 	type env struct {
 		Name  string
@@ -51,7 +94,7 @@ func TestMapReference(t *testing.T) {
 		},
 	}
 
-	declaredEnv := map[string]interface{}{
+	varMap := map[string]interface{}{
 		"FOO":   "bar",
 		"ZOO":   "$(FOO)-1",
 		"BLU":   "$(ZOO)-2",
@@ -61,11 +104,11 @@ func TestMapReference(t *testing.T) {
 		"ZBOOL": "$(BOOL)",
 	}
 
-	counts := make(map[string]int)
-	mapping := MappingFuncFor(counts, declaredEnv)
-
+	varCounts := make(map[string]int)
 	for _, env := range envs {
-		declaredEnv[env.Name] = Expand(fmt.Sprintf("%v", env.Value), mapping)
+		varMap[env.Name] = DoReplacements(
+			fmt.Sprintf("%v", env.Value),
+			MakePrimitiveReplacer(varCounts, varMap))
 	}
 
 	expectedEnv := map[string]expected{
@@ -79,45 +122,20 @@ func TestMapReference(t *testing.T) {
 	}
 
 	for k, v := range expectedEnv {
-		if e, a := v, declaredEnv[k]; e.edited != a || e.count != counts[k] {
+		if e, a := v, varMap[k]; e.edited != a || e.count != varCounts[k] {
 			t.Errorf("Expected %v count=%d, got %v count=%d",
-				e.edited, e.count, a, counts[k])
+				e.edited, e.count, a, varCounts[k])
 		} else {
-			delete(declaredEnv, k)
+			delete(varMap, k)
 		}
 	}
 
-	if len(declaredEnv) != 0 {
-		t.Errorf("Unexpected keys in declared env: %v", declaredEnv)
+	if len(varMap) != 0 {
+		t.Errorf("Unexpected keys in declared env: %v", varMap)
 	}
 }
 
 func TestMapping(t *testing.T) {
-	context := map[string]interface{}{
-		"VAR_A":     "A",
-		"VAR_B":     "B",
-		"VAR_C":     "C",
-		"VAR_REF":   "$(VAR_A)",
-		"VAR_EMPTY": "",
-	}
-	doExpansionTest(t, context)
-}
-
-func TestMappingDual(t *testing.T) {
-	context := map[string]interface{}{
-		"VAR_A":     "A",
-		"VAR_EMPTY": "",
-	}
-	context2 := map[string]interface{}{
-		"VAR_B":   "B",
-		"VAR_C":   "C",
-		"VAR_REF": "$(VAR_A)",
-	}
-
-	doExpansionTest(t, context, context2)
-}
-
-func doExpansionTest(t *testing.T, context ...map[string]interface{}) {
 	cases := []struct {
 		name     string
 		input    string
@@ -333,11 +351,17 @@ func doExpansionTest(t *testing.T, context ...map[string]interface{}) {
 			expected: "\n",
 		},
 	}
-
 	for _, tc := range cases {
 		counts := make(map[string]int)
-		mapping := MappingFuncFor(counts, context...)
-		expanded := Expand(fmt.Sprintf("%v", tc.input), mapping)
+		expanded := DoReplacements(
+			fmt.Sprintf("%v", tc.input),
+			MakePrimitiveReplacer(counts, map[string]interface{}{
+				"VAR_A":     "A",
+				"VAR_B":     "B",
+				"VAR_C":     "C",
+				"VAR_REF":   "$(VAR_A)",
+				"VAR_EMPTY": "",
+			}))
 		if e, a := tc.expected, expanded; e != a {
 			t.Errorf("%v: expected %q, got %q", tc.name, e, a)
 		}
@@ -347,8 +371,7 @@ func doExpansionTest(t *testing.T, context ...map[string]interface{}) {
 		}
 		if len(tc.counts) > 0 {
 			for k, expectedCount := range tc.counts {
-				c, ok := counts[k]
-				if ok {
+				if c, ok := counts[k]; ok {
 					if c != expectedCount {
 						t.Errorf(
 							"%v: k=%s, expected count %d, got %d",

api/filters/refvar/refvar.go

@@ -13,8 +13,8 @@ import (
 // Filter updates $(VAR) style variables with values.
 // The fieldSpecs are the places to look for occurrences of $(VAR).
 type Filter struct {
-	MappingFunc func(string) interface{} `json:"mappingFunc,omitempty" yaml:"mappingFunc,omitempty"`
-	FieldSpec   types.FieldSpec          `json:"fieldSpec,omitempty" yaml:"fieldSpec,omitempty"`
+	MappingFunc MappingFunc     `json:"mappingFunc,omitempty" yaml:"mappingFunc,omitempty"`
+	FieldSpec   types.FieldSpec `json:"fieldSpec,omitempty" yaml:"fieldSpec,omitempty"`
 }
 
 func (f Filter) Filter(nodes []*yaml.RNode) ([]*yaml.RNode, error) {
@@ -67,7 +67,7 @@ func (f Filter) setScalar(node *yaml.RNode) error {
 	if !yaml.IsYNodeString(node.YNode()) {
 		return nil
 	}
-	v := Expand(node.YNode().Value, f.MappingFunc)
+	v := DoReplacements(node.YNode().Value, f.MappingFunc)
 	updateNodeValue(node.YNode(), v)
 	return nil
 }
@@ -83,7 +83,7 @@ func (f Filter) setMap(node *yaml.RNode) error {
 		if !yaml.IsYNodeString(contents[i+1]) {
 			continue
 		}
-		newValue := Expand(contents[i+1].Value, f.MappingFunc)
+		newValue := DoReplacements(contents[i+1].Value, f.MappingFunc)
 		updateNodeValue(contents[i+1], newValue)
 	}
 	return nil
@@ -94,7 +94,7 @@ func (f Filter) setSeq(node *yaml.RNode) error {
 		if !yaml.IsYNodeString(item) {
 			return fmt.Errorf("invalid value type expect a string")
 		}
-		newValue := Expand(item.Value, f.MappingFunc)
+		newValue := DoReplacements(item.Value, f.MappingFunc)
 		updateNodeValue(item, newValue)
 	}
 	return nil

api/filters/refvar/refvar_test.go

@@ -11,8 +11,12 @@ import (
 	"sigs.k8s.io/kustomize/kyaml/yaml"
 )
 
+var makeMf = func(theMap map[string]interface{}) MappingFunc {
+	ignored := make(map[string]int)
+	return MakePrimitiveReplacer(ignored, theMap)
+}
+
 func TestFilter(t *testing.T) {
-	replacementCounts := make(map[string]int)
 
 	testCases := map[string]struct {
 		input    string
@@ -35,7 +39,7 @@ metadata:
 spec:
   replicas: 5`,
 			filter: Filter{
-				MappingFunc: MappingFuncFor(replacementCounts, map[string]interface{}{
+				MappingFunc: makeMf(map[string]interface{}{
 					"VAR": int64(5),
 				}),
 				FieldSpec: types.FieldSpec{Path: "spec/replicas"},
@@ -57,7 +61,7 @@ metadata:
 spec:
   replicas: 1`,
 			filter: Filter{
-				MappingFunc: MappingFuncFor(replacementCounts, map[string]interface{}{
+				MappingFunc: makeMf(map[string]interface{}{
 					"VAR": int64(5),
 				}),
 				FieldSpec: types.FieldSpec{Path: "spec/replicas"},
@@ -79,7 +83,7 @@ metadata:
 spec:
   replicas: 1`,
 			filter: Filter{
-				MappingFunc: MappingFuncFor(replacementCounts, map[string]interface{}{
+				MappingFunc: makeMf(map[string]interface{}{
 					"VAR": int64(5),
 				}),
 				FieldSpec: types.FieldSpec{Path: "a/b/c"},
@@ -111,7 +115,7 @@ data:
 - false
 - 1.23`,
 			filter: Filter{
-				MappingFunc: MappingFuncFor(replacementCounts, map[string]interface{}{
+				MappingFunc: makeMf(map[string]interface{}{
 					"FOO":   "foo",
 					"BAR":   "bar",
 					"BOOL":  false,
@@ -142,7 +146,7 @@ data:
   BAZ: $(BAZ)
   PLUS: foo+bar`,
 			filter: Filter{
-				MappingFunc: MappingFuncFor(replacementCounts, map[string]interface{}{
+				MappingFunc: makeMf(map[string]interface{}{
 					"FOO": "foo",
 					"BAR": "bar",
 				}),
@@ -181,7 +185,7 @@ data:
     SLICE:
     - $(FOO)`,
 			filter: Filter{
-				MappingFunc: MappingFuncFor(replacementCounts, map[string]interface{}{
+				MappingFunc: makeMf(map[string]interface{}{
 					"FOO": "foo",
 					"BAR": "bar",
 				}),
@@ -204,8 +208,10 @@ metadata:
 data:
   FOO: null`,
 			filter: Filter{
-				MappingFunc: MappingFuncFor(replacementCounts, map[string]interface{}{}),
-				FieldSpec:   types.FieldSpec{Path: "data/FOO"},
+				MappingFunc: makeMf(map[string]interface{}{
+					// no replacements!
+				}),
+				FieldSpec: types.FieldSpec{Path: "data/FOO"},
 			},
 		},
 	}
@@ -223,8 +229,6 @@ data:
 }
 
 func TestFilterUnhappy(t *testing.T) {
-	replacementCounts := make(map[string]int)
-
 	testCases := map[string]struct {
 		input         string
 		expectedError string
@@ -250,7 +254,7 @@ data:
   - false
 ' at path 'data/slice': invalid value type expect a string`,
 			filter: Filter{
-				MappingFunc: MappingFuncFor(replacementCounts, map[string]interface{}{
+				MappingFunc: makeMf(map[string]interface{}{
 					"VAR": int64(5),
 				}),
 				FieldSpec: types.FieldSpec{Path: "data/slice"},
@@ -274,7 +278,7 @@ data:
   1: str
 ' at path 'data': invalid map key: value='1', tag='` + yaml.NodeTagInt + `'`,
 			filter: Filter{
-				MappingFunc: MappingFuncFor(replacementCounts, map[string]interface{}{
+				MappingFunc: makeMf(map[string]interface{}{
 					"VAR": int64(5),
 				}),
 				FieldSpec: types.FieldSpec{Path: "data"},

api/go.sum

@@ -599,8 +599,6 @@ mvdan.cc/lint v0.0.0-20170908181259-adc824a0674b h1:DxJ5nJdkhDlLok9K6qO+5290kphD
 mvdan.cc/lint v0.0.0-20170908181259-adc824a0674b/go.mod h1:2odslEg/xrtNQqCYg2/jCoyKnw3vv5biOc3JnIcYfL4=
 mvdan.cc/unparam v0.0.0-20190720180237-d51796306d8f h1:Cq7MalBHYACRd6EesksG1Q8EoIAKOsiZviGKbOLIej4=
 mvdan.cc/unparam v0.0.0-20190720180237-d51796306d8f/go.mod h1:4G1h5nDURzA3bwVMZIVpwbkw+04kSxk3rAtzlimaUJw=
-sigs.k8s.io/kustomize v1.0.11 h1:Yb+6DDt9+aR2AvQApvUaKS/ugteeG4MPyoFeUHiPOjk=
-sigs.k8s.io/kustomize v2.0.3+incompatible h1:JUufWFNlI44MdtnjUqVnvh29rR37PQFzPbLXqhyOyX0=
 sigs.k8s.io/structured-merge-diff v0.0.0-20190525122527-15d366b2352e/go.mod h1:wWxsB5ozmmv/SG7nM11ayaAW51xMvak/t1r0CSlcokI=
 sigs.k8s.io/yaml v1.1.0/go.mod h1:UJmg0vDUVViEyp3mgSv9WPwZCDxu4rQW1olrI1uml+o=
 sigs.k8s.io/yaml v1.2.0 h1:kr/MCeFWJWTwyaHoR9c8EjH9OumOmoF9YGiZd7lFm/Q=

api/internal/accumulator/refvartransformer.go

@@ -13,7 +13,6 @@ type refVarTransformer struct {
 	varMap            map[string]interface{}
 	replacementCounts map[string]int
 	fieldSpecs        []types.FieldSpec
-	mappingFunc       func(string) interface{}
 }
 
 // newRefVarTransformer returns a new refVarTransformer
@@ -32,8 +31,7 @@ func newRefVarTransformer(
 func (rv *refVarTransformer) UnusedVars() []string {
 	var unused []string
 	for k := range rv.varMap {
-		_, ok := rv.replacementCounts[k]
-		if !ok {
+		if _, ok := rv.replacementCounts[k]; !ok {
 			unused = append(unused, k)
 		}
 	}
@@ -43,12 +41,11 @@ func (rv *refVarTransformer) UnusedVars() []string {
 // Transform replaces $(VAR) style variables with values.
 func (rv *refVarTransformer) Transform(m resmap.ResMap) error {
 	rv.replacementCounts = make(map[string]int)
-	rv.mappingFunc = refvar.MappingFuncFor(
-		rv.replacementCounts, rv.varMap)
+	mf := refvar.MakePrimitiveReplacer(rv.replacementCounts, rv.varMap)
 	for _, res := range m.Resources() {
 		for _, fieldSpec := range rv.fieldSpecs {
 			err := res.ApplyFilter(refvar.Filter{
-				MappingFunc: rv.mappingFunc,
+				MappingFunc: mf,
 				FieldSpec:   fieldSpec,
 			})
 			if err != nil {