Merge pull request #219 from jessicayuen/param-set-hyphens

Accurately read/write non-ASCII param identifiers

cmd/root.go

@@ -27,8 +27,6 @@ import (
 	"reflect"
 	"strings"
 
-	"github.com/ksonnet/ksonnet/metadata/params"
-
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 
 	log "github.com/sirupsen/logrus"
@@ -480,7 +478,7 @@ func constructBaseObj(componentPaths, componentNames []string) (string, error) {
 
 		// Emit object field. Sanitize the name to guarantee we generate valid
 		// Jsonnet.
-		componentName = params.SanitizeComponent(componentName)
+		componentName = utils.QuoteNonASCII(componentName)
 		fmt.Fprintf(&obj, "  %s: %s,\n", componentName, importExpr)
 	}
 

metadata/params/params.go

@@ -22,9 +22,10 @@ import (
 	"strconv"
 	"strings"
 
+	"github.com/ksonnet/ksonnet/utils"
+
 	"github.com/google/go-jsonnet/ast"
 	"github.com/google/go-jsonnet/parser"
-	"github.com/ksonnet/ksonnet/utils"
 )
 
 const (
@@ -66,15 +67,6 @@ func findComponentsObj(node ast.Node) (*ast.Object, error) {
 	return nil, fmt.Errorf("Invalid params schema -- did not expect node type: %T", node)
 }
 
-// SanitizeComponent puts quotes around a component name if it contains special
-// characters.
-func SanitizeComponent(component string) string {
-	if !utils.IsASCIIIdentifier(component) {
-		return fmt.Sprintf(`"%s"`, component)
-	}
-	return component
-}
-
 func getFieldID(field ast.ObjectField) (string, error) {
 	switch field.Kind {
 	case ast.ObjectFieldStr:
@@ -109,14 +101,15 @@ func visitParams(component ast.Node) (Params, *ast.LocationRange, error) {
 
 	loc = n.Loc()
 	for _, field := range n.Fields {
-		if field.Id != nil {
-			key := string(*field.Id)
-			val, err := visitParamValue(field.Expr2)
-			if err != nil {
-				return nil, nil, err
-			}
-			params[key] = val
+		key, err := getFieldID(field)
+		if err != nil {
+			return nil, nil, err
 		}
+		val, err := visitParamValue(field.Expr2)
+		if err != nil {
+			return nil, nil, err
+		}
+		params[key] = val
 	}
 
 	return params, loc, nil
@@ -179,9 +172,9 @@ func writeParams(indent int, params Params) string {
 	buffer.WriteString("\n")
 	for i, key := range keys {
 		param := params[key]
-		if strings.HasPrefix(param, "|||\n") {
-			println(param)
+		key := utils.QuoteNonASCII(key)
 
+		if strings.HasPrefix(param, "|||\n") {
 			// every line in a block string needs to be indented
 			lines := strings.Split(param, "\n")
 			buffer.WriteString(fmt.Sprintf("%s%s: %s\n", indentBuffer.String(), key, lines[0]))
@@ -225,7 +218,7 @@ func appendComponent(component, snippet string, params Params) (string, error) {
 
 	// Create the jsonnet resembling the component params
 	var buffer bytes.Buffer
-	buffer.WriteString("    " + SanitizeComponent(component) + ": {")
+	buffer.WriteString("    " + utils.QuoteNonASCII(component) + ": {")
 	buffer.WriteString(writeParams(6, params))
 	buffer.WriteString("    },")
 
@@ -254,7 +247,7 @@ func getComponentParams(component, snippet string) (Params, *ast.LocationRange,
 		}
 	}
 
-	return nil, nil, fmt.Errorf("Could not find component identifier '%s' when attempting to set params", component)
+	return nil, nil, fmt.Errorf("Could not find component identifier '%s'", component)
 }
 
 func getAllComponentParams(snippet string) (map[string]Params, error) {
@@ -342,7 +335,7 @@ func setEnvironmentParams(component, snippet string, params Params) (string, err
 	lines := strings.Split(snippet, "\n")
 	if !hasComponent {
 		var buffer bytes.Buffer
-		buffer.WriteString(fmt.Sprintf("\n    %s +: {", SanitizeComponent(component)))
+		buffer.WriteString(fmt.Sprintf("\n    %s +: {", utils.QuoteNonASCII(component)))
 		buffer.WriteString(writeParams(6, params))
 		buffer.WriteString("    },\n")
 		paramsSnippet = buffer.String()

metadata/params/params_test.go

@@ -275,11 +275,11 @@ func TestGetComponentParams(t *testing.T) {
     },
     foo: {
       name: "foo",
-      replicas: 1,
+      "replica-count": 1,
     },
   },
 }`,
-			Params{"name": `"foo"`, "replicas": "1"},
+			Params{"name": `"foo"`, "replica-count": "1"},
 		},
 		// Test getting the parameters for a component name with special characters
 		{
@@ -521,6 +521,30 @@ func TestSetComponentParams(t *testing.T) {
       replicas: 5,
     },
   },
+}`,
+		},
+		// Test adding a parameter with special characters in the param name
+		{
+			"foo",
+			`
+{
+  components: {
+    foo: {
+      "foo-bar": 5,
+      name: "foo",
+    },
+  },
+}`,
+			Params{"replica-count": "5"},
+			`
+{
+  components: {
+    foo: {
+      "foo-bar": 5,
+      name: "foo",
+      "replica-count": 5,
+    },
+  },
 }`,
 		},
 		// Test setting parameter for a component with a special character in name
@@ -700,13 +724,13 @@ params + {
     },
     "foo" +: {
       name: "foo",
-      replicas: 5,
+      "replica-count": 5,
     },
   },
 }`,
 			map[string]Params{
 				"bar": Params{"name": `"bar"`, "replicas": "1"},
-				"foo": Params{"name": `"foo"`, "replicas": "5"},
+				"foo": Params{"name": `"foo"`, "replica-count": "5"},
 			},
 		},
 	}
@@ -815,6 +839,38 @@ params + {
       replicas: 5,
     },
   },
+}`,
+		},
+		// Test special character in param identifier
+		{
+			"foo",
+			`
+local params = import "/fake/path";
+params + {
+  components +: {
+    bar +: {
+      name: "bar",
+      "replica-count": 1,
+    },
+    foo +: {
+      name: "foo",
+    },
+  },
+}`,
+			Params{"replica-count": "5"},
+			`
+local params = import "/fake/path";
+params + {
+  components +: {
+    bar +: {
+      name: "bar",
+      "replica-count": 1,
+    },
+    foo +: {
+      name: "foo",
+      "replica-count": 5,
+    },
+  },
 }`,
 		},
 		// Test setting environment param case where component isn't in the snippet

utils/strings.go

@@ -17,6 +17,7 @@ package utils
 
 import (
 	"bytes"
+	"fmt"
 	"strings"
 
 	"github.com/PuerkitoBio/purell"
@@ -34,6 +35,15 @@ func IsASCIIIdentifier(s string) bool {
 	return true
 }
 
+// QuoteNonASCII puts quotes around an identifier that contains non-ASCII
+// characters.
+func QuoteNonASCII(s string) string {
+	if !IsASCIIIdentifier(s) {
+		return fmt.Sprintf(`"%s"`, s)
+	}
+	return s
+}
+
 // NormalizeURL uses purell's "usually safe normalization" algorithm to
 // normalize URLs. This includes removing dot segments, removing trailing
 // slashes, removing unnecessary escapes, removing default ports, and setting

utils/strings_test.go

@@ -53,6 +53,37 @@ func TestIsASCIIIdentifier(t *testing.T) {
 	}
 }
 
+func TestQuoteNonASCII(t *testing.T) {
+	tests := []struct {
+		input    string
+		expected string
+	}{
+		{
+			input:    "HelloWorld",
+			expected: "HelloWorld",
+		},
+		{
+			input:    "Hello World",
+			expected: `"Hello World"`,
+		},
+		{
+			input:    "helloworld",
+			expected: "helloworld",
+		},
+		{
+			input:    "hello-world",
+			expected: `"hello-world"`,
+		},
+		{
+			input:    "hello世界",
+			expected: `"hello世界"`,
+		},
+	}
+	for _, test := range tests {
+		require.EqualValues(t, test.expected, QuoteNonASCII(test.input))
+	}
+}
+
 func TestNormalizeURL(t *testing.T) {
 	tests := []struct {
 		input    string