fix(controller): JSON-unmarshal marshaled expression template before evaluating

docs/fields.md

@@ -124,6 +124,8 @@ Workflow is the definition of a workflow resource
 
 - [`expression-destructure-json.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-destructure-json.yaml)
 
+- [`expression-reusing-verbose-snippets.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-reusing-verbose-snippets.yaml)
+
 - [`expression-tag-template-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-tag-template-workflow.yaml)
 
 - [`fibonacci-seq-conditional-param.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/fibonacci-seq-conditional-param.yaml)
@@ -527,6 +529,8 @@ WorkflowSpec is the specification of a Workflow.
 
 - [`expression-destructure-json.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-destructure-json.yaml)
 
+- [`expression-reusing-verbose-snippets.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-reusing-verbose-snippets.yaml)
+
 - [`expression-tag-template-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-tag-template-workflow.yaml)
 
 - [`fibonacci-seq-conditional-param.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/fibonacci-seq-conditional-param.yaml)
@@ -947,6 +951,8 @@ CronWorkflowSpec is the specification of a CronWorkflow
 
 - [`expression-destructure-json.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-destructure-json.yaml)
 
+- [`expression-reusing-verbose-snippets.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-reusing-verbose-snippets.yaml)
+
 - [`expression-tag-template-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-tag-template-workflow.yaml)
 
 - [`fibonacci-seq-conditional-param.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/fibonacci-seq-conditional-param.yaml)
@@ -1324,6 +1330,8 @@ WorkflowTemplateSpec is a spec of WorkflowTemplate.
 
 - [`expression-destructure-json.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-destructure-json.yaml)
 
+- [`expression-reusing-verbose-snippets.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-reusing-verbose-snippets.yaml)
+
 - [`expression-tag-template-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-tag-template-workflow.yaml)
 
 - [`fibonacci-seq-conditional-param.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/fibonacci-seq-conditional-param.yaml)
@@ -1660,6 +1668,8 @@ Arguments to a template
 
 - [`expression-destructure-json.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-destructure-json.yaml)
 
+- [`expression-reusing-verbose-snippets.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-reusing-verbose-snippets.yaml)
+
 - [`expression-tag-template-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-tag-template-workflow.yaml)
 
 - [`fibonacci-seq-conditional-param.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/fibonacci-seq-conditional-param.yaml)
@@ -2387,6 +2397,8 @@ Parameter indicate a passed string parameter to a service template with an optio
 
 - [`expression-destructure-json.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-destructure-json.yaml)
 
+- [`expression-reusing-verbose-snippets.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-reusing-verbose-snippets.yaml)
+
 - [`expression-tag-template-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-tag-template-workflow.yaml)
 
 - [`fibonacci-seq-conditional-param.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/fibonacci-seq-conditional-param.yaml)
@@ -2901,6 +2913,8 @@ Inputs are the mechanism for passing parameters, artifacts, volumes from one tem
 
 - [`expression-destructure-json.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-destructure-json.yaml)
 
+- [`expression-reusing-verbose-snippets.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-reusing-verbose-snippets.yaml)
+
 - [`expression-tag-template-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-tag-template-workflow.yaml)
 
 - [`fibonacci-seq-conditional-param.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/fibonacci-seq-conditional-param.yaml)
@@ -3106,6 +3120,8 @@ ScriptTemplate is a template subtype to enable scripting through code steps
 
 - [`expression-destructure-json.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-destructure-json.yaml)
 
+- [`expression-reusing-verbose-snippets.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-reusing-verbose-snippets.yaml)
+
 - [`fibonacci-seq-conditional-param.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/fibonacci-seq-conditional-param.yaml)
 
 - [`loops-param-result.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/loops-param-result.yaml)
@@ -4077,6 +4093,8 @@ DataSource sources external data into a data template
 
 - [`expression-destructure-json.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-destructure-json.yaml)
 
+- [`expression-reusing-verbose-snippets.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-reusing-verbose-snippets.yaml)
+
 - [`fibonacci-seq-conditional-param.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/fibonacci-seq-conditional-param.yaml)
 
 - [`loops-param-result.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/loops-param-result.yaml)
@@ -4708,6 +4726,8 @@ ObjectMeta is metadata that all persisted resources must have, which includes al
 
 - [`expression-destructure-json.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-destructure-json.yaml)
 
+- [`expression-reusing-verbose-snippets.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-reusing-verbose-snippets.yaml)
+
 - [`expression-tag-template-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-tag-template-workflow.yaml)
 
 - [`fibonacci-seq-conditional-param.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/fibonacci-seq-conditional-param.yaml)
@@ -5584,6 +5604,8 @@ EnvVar represents an environment variable present in a Container.
 
 - [`expression-destructure-json.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-destructure-json.yaml)
 
+- [`expression-reusing-verbose-snippets.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-reusing-verbose-snippets.yaml)
+
 - [`secrets.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/secrets.yaml)
 
 - [`sidecar-dind.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/sidecar-dind.yaml)
@@ -5989,6 +6011,8 @@ PersistentVolumeClaimSpec describes the common attributes of storage devices and
 
 - [`expression-destructure-json.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-destructure-json.yaml)
 
+- [`expression-reusing-verbose-snippets.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-reusing-verbose-snippets.yaml)
+
 - [`expression-tag-template-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-tag-template-workflow.yaml)
 
 - [`fibonacci-seq-conditional-param.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/fibonacci-seq-conditional-param.yaml)

examples/expression-reusing-verbose-snippets.yaml

@@ -0,0 +1,44 @@
+# Expressions do not support variables. Rather than repeating verbose expressions to reuse output, you can map over the
+# expression output and use its value which is aliased as `#`. Then you can place your "variables" in a JSON object to
+# be used elsewhere.
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+  generateName: expression-reusing-verbose-snippets-
+spec:
+  arguments:
+    parameters:
+      - name: weather
+        # The base64 string is this JSON: {"temps": [34, 27, 15, 57, 46]}
+        value: '{"weekWeather": "eyJ0ZW1wcyI6IFszNCwgMjcsIDE1LCA1NywgNDZdfQo="}'
+  entrypoint: main
+  templates:
+    - name: main
+      inputs:
+        parameters:
+          - name: week-temps
+            # The line being mapped over is verbose. Rather than repeat it, we use `map` to alias its output as #.
+            value: >-
+              {{=
+                map([
+                    jsonpath(sprig.b64dec(jsonpath(workflow.parameters.weather, '$.weekWeather')), '$.temps')
+                  ], {
+                    toJson({
+                      avg: sprig.add(#[0], #[1], #[2], #[3], #[4]) / 5,
+                      min: sprig.min(#[0], #[1], #[2], #[3], #[4]),
+                      max: sprig.max(#[0], #[1], #[2], #[3], #[4])
+                    })
+                })[0]
+              }}
+      script:
+        env:
+          - name: AVG
+            value: "{{=jsonpath(inputs.parameters['week-temps'], '$.avg')}}"
+          - name: MIN
+            value: "{{=jsonpath(inputs.parameters['week-temps'], '$.min')}}"
+          - name: MAX
+            value: "{{=jsonpath(inputs.parameters['week-temps'], '$.max')}}"
+        image: debian:9.4
+        command: [bash]
+        source: |
+          echo "The week's average temperature was $AVG with a minimum of $MIN and a maximum of $MAX."

util/template/expression_template.go

@@ -1,6 +1,7 @@
 package template
 
 import (
+	"encoding/json"
 	"fmt"
 	"io"
 	"os"
@@ -17,12 +18,22 @@ func init() {
 }
 
 func expressionReplace(w io.Writer, expression string, env map[string]interface{}, allowUnresolved bool) (int, error) {
-	if _, ok := env["retries"]; !ok && hasRetries(expression) && allowUnresolved {
+	// The template is JSON-marshaled. This JSON-unmarshals the expression to undo any character escapes.
+	var unmarshalledExpression string
+	err := json.Unmarshal([]byte(fmt.Sprintf(`"%s"`, expression)), &unmarshalledExpression)
+	if err != nil && allowUnresolved {
+		return w.Write([]byte(fmt.Sprintf("{{%s%s}}", kindExpression, expression)))
+	}
+	if err != nil {
+		return 0, fmt.Errorf("failed to unmarshall JSON expression: %w", err)
+	}
+
+	if _, ok := env["retries"]; !ok && hasRetries(unmarshalledExpression) && allowUnresolved {
 		// this is to make sure expressions like `sprig.int(retries)` don't get resolved to 0 when `retries` don't exist in the env
 		// See https://github.com/argoproj/argo-workflows/issues/5388
 		return w.Write([]byte(fmt.Sprintf("{{%s%s}}", kindExpression, expression)))
 	}
-	result, err := expr.Eval(expression, env)
+	result, err := expr.Eval(unmarshalledExpression, env)
 	if (err != nil || result == nil) && allowUnresolved { //  <nil> result is also un-resolved, and any error can be unresolved
 		return w.Write([]byte(fmt.Sprintf("{{%s%s}}", kindExpression, expression)))
 	}
@@ -32,7 +43,19 @@ func expressionReplace(w io.Writer, expression string, env map[string]interface{
 	if result == nil {
 		return 0, fmt.Errorf("failed to evaluate expression %q", expression)
 	}
-	return w.Write([]byte(fmt.Sprintf("%v", result)))
+	resultMarshaled, err := json.Marshal(fmt.Sprintf("%v", result))
+	if (err != nil || resultMarshaled == nil) && allowUnresolved {
+		return w.Write([]byte(fmt.Sprintf("{{%s%s}}", kindExpression, expression)))
+	}
+	if err != nil {
+		return 0, fmt.Errorf("failed to marshal evaluated expression: %w", err)
+	}
+	if resultMarshaled == nil {
+		return 0, fmt.Errorf("failed to marshal evaluated marshaled expression %q", expression)
+	}
+	// Trim leading and trailing quotes. The value is being inserted into something that's already a string.
+	marshaledLength := len(resultMarshaled)
+	return w.Write(resultMarshaled[1 : marshaledLength-1])
 }
 
 func envMap(replaceMap map[string]string) map[string]interface{} {

util/template/replace.go

@@ -1,9 +1,29 @@
 package template
 
+import (
+	"encoding/json"
+	"errors"
+)
+
+// Replace takes a json-formatted string and performs variable replacement.
 func Replace(s string, replaceMap map[string]string, allowUnresolved bool) (string, error) {
+	if !json.Valid([]byte(s)) {
+		return "", errors.New("cannot do template replacements with invalid JSON")
+	}
+
 	t, err := NewTemplate(s)
 	if err != nil {
 		return "", err
 	}
-	return t.Replace(replaceMap, allowUnresolved)
+
+	replacedString, err := t.Replace(replaceMap, allowUnresolved)
+	if err != nil {
+		return s, err
+	}
+
+	if !json.Valid([]byte(replacedString)) {
+		return s, errors.New("cannot finish template replacement because the result was invalid JSON")
+	}
+
+	return replacedString, nil
 }

util/template/replace_test.go

@@ -1,56 +1,62 @@
 package template
 
 import (
+	"encoding/json"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 )
 
+func toJsonString(v interface{}) string {
+	jsonString, _ := json.Marshal(v)
+	return string(jsonString)
+}
+
 func Test_Replace(t *testing.T) {
-	t.Run("InvailedTemplate", func(t *testing.T) {
-		_, err := Replace("{{", nil, false)
+	t.Run("InvalidTemplate", func(t *testing.T) {
+		_, err := Replace(toJsonString("{{"), nil, false)
 		assert.Error(t, err)
 	})
 	t.Run("Simple", func(t *testing.T) {
 		t.Run("Valid", func(t *testing.T) {
-			r, err := Replace("{{foo}}", map[string]string{"foo": "bar"}, false)
+			r, err := Replace(toJsonString("{{foo}}"), map[string]string{"foo": "bar"}, false)
 			assert.NoError(t, err)
-			assert.Equal(t, "bar", r)
+			assert.Equal(t, toJsonString("bar"), r)
 		})
 		t.Run("Unresolved", func(t *testing.T) {
 			t.Run("Allowed", func(t *testing.T) {
-				_, err := Replace("{{foo}}", nil, true)
+				_, err := Replace(toJsonString("{{foo}}"), nil, true)
 				assert.NoError(t, err)
 			})
 			t.Run("Disallowed", func(t *testing.T) {
-				_, err := Replace("{{foo}}", nil, false)
+				_, err := Replace(toJsonString("{{foo}}"), nil, false)
 				assert.EqualError(t, err, "failed to resolve {{foo}}")
 			})
 		})
 	})
 	t.Run("Expression", func(t *testing.T) {
 		t.Run("Valid", func(t *testing.T) {
-			r, err := Replace("{{=foo}}", map[string]string{"foo": "bar"}, false)
+			r, err := Replace(toJsonString("{{=foo}}"), map[string]string{"foo": "bar"}, false)
 			assert.NoError(t, err)
-			assert.Equal(t, "bar", r)
+			assert.Equal(t, toJsonString("bar"), r)
 		})
 		t.Run("Unresolved", func(t *testing.T) {
 			t.Run("Allowed", func(t *testing.T) {
-				_, err := Replace("{{=foo}}", nil, true)
+				_, err := Replace(toJsonString("{{=foo}}"), nil, true)
 				assert.NoError(t, err)
 			})
 			t.Run("AllowedRetries", func(t *testing.T) {
-				replaced, err := Replace("{{=sprig.int(retries)}}", nil, true)
+				replaced, err := Replace(toJsonString("{{=sprig.int(retries)}}"), nil, true)
 				assert.NoError(t, err)
-				assert.Equal(t, replaced, "{{=sprig.int(retries)}}")
+				assert.Equal(t, replaced, toJsonString("{{=sprig.int(retries)}}"))
 			})
 			t.Run("Disallowed", func(t *testing.T) {
-				_, err := Replace("{{=foo}}", nil, false)
+				_, err := Replace(toJsonString("{{=foo}}"), nil, false)
 				assert.EqualError(t, err, "failed to evaluate expression \"foo\"")
 			})
 		})
 		t.Run("Error", func(t *testing.T) {
-			_, err := Replace("{{=!}}", nil, false)
+			_, err := Replace(toJsonString("{{=!}}"), nil, false)
 			if assert.Error(t, err) {
 				assert.Contains(t, err.Error(), "failed to evaluate expression")
 			}
@@ -61,53 +67,53 @@ func Test_Replace(t *testing.T) {
 func TestNestedReplaceString(t *testing.T) {
 	replaceMap := map[string]string{"inputs.parameters.message": "hello world"}
 
-	test := `{{- with secret "{{inputs.parameters.message}}" -}}
+	test := toJsonString(`{{- with secret "{{inputs.parameters.message}}" -}}
     {{ .Data.data.gitcreds }}
-  {{- end }}`
+  {{- end }}`)
 	replacement, err := Replace(test, replaceMap, true)
 	if assert.NoError(t, err) {
-		assert.Equal(t, "{{- with secret \"hello world\" -}}\n    {{ .Data.data.gitcreds }}\n  {{- end }}", replacement)
+		assert.Equal(t, toJsonString("{{- with secret \"hello world\" -}}\n    {{ .Data.data.gitcreds }}\n  {{- end }}"), replacement)
 	}
 
-	test = `{{- with {{ secret "{{inputs.parameters.message}}" -}}
+	test = toJsonString(`{{- with {{ secret "{{inputs.parameters.message}}" -}}
     {{ .Data.data.gitcreds }}
-  {{- end }}`
+  {{- end }}`)
 
 	replacement, err = Replace(test, replaceMap, true)
 	if assert.NoError(t, err) {
-		assert.Equal(t, "{{- with {{ secret \"hello world\" -}}\n    {{ .Data.data.gitcreds }}\n  {{- end }}", replacement)
+		assert.Equal(t, toJsonString("{{- with {{ secret \"hello world\" -}}\n    {{ .Data.data.gitcreds }}\n  {{- end }}"), replacement)
 	}
 
-	test = `{{- with {{ secret "{{inputs.parameters.message}}" -}} }}
+	test = toJsonString(`{{- with {{ secret "{{inputs.parameters.message}}" -}} }}
     {{ .Data.data.gitcreds }}
-  {{- end }}`
+  {{- end }}`)
 
 	replacement, err = Replace(test, replaceMap, true)
 	if assert.NoError(t, err) {
-		assert.Equal(t, "{{- with {{ secret \"hello world\" -}} }}\n    {{ .Data.data.gitcreds }}\n  {{- end }}", replacement)
+		assert.Equal(t, toJsonString("{{- with {{ secret \"hello world\" -}} }}\n    {{ .Data.data.gitcreds }}\n  {{- end }}"), replacement)
 	}
 
-	test = `{{- with secret "{{inputs.parameters.message}}" -}} }}
+	test = toJsonString(`{{- with secret "{{inputs.parameters.message}}" -}} }}
     {{ .Data.data.gitcreds }}
-  {{- end }}`
+  {{- end }}`)
 
 	replacement, err = Replace(test, replaceMap, true)
 	if assert.NoError(t, err) {
-		assert.Equal(t, "{{- with secret \"hello world\" -}} }}\n    {{ .Data.data.gitcreds }}\n  {{- end }}", replacement)
+		assert.Equal(t, toJsonString("{{- with secret \"hello world\" -}} }}\n    {{ .Data.data.gitcreds }}\n  {{- end }}"), replacement)
 	}
 
-	test = `{{- with {{ {{ }} secret "{{inputs.parameters.message}}" -}} }}
+	test = toJsonString(`{{- with {{ {{ }} secret "{{inputs.parameters.message}}" -}} }}
     {{ .Data.data.gitcreds }}
-  {{- end }}`
+  {{- end }}`)
 
 	replacement, err = Replace(test, replaceMap, true)
 	if assert.NoError(t, err) {
-		assert.Equal(t, "{{- with {{ {{ }} secret \"hello world\" -}} }}\n    {{ .Data.data.gitcreds }}\n  {{- end }}", replacement)
+		assert.Equal(t, toJsonString("{{- with {{ {{ }} secret \"hello world\" -}} }}\n    {{ .Data.data.gitcreds }}\n  {{- end }}"), replacement)
 	}
 
-	test = `{{- with {{ {{ }} secret "{{does-not-exist}}" -}} }}
+	test = toJsonString(`{{- with {{ {{ }} secret "{{does-not-exist}}" -}} }}
     {{ .Data.data.gitcreds }}
-  {{- end }}`
+  {{- end }}`)
 
 	replacement, err = Replace(test, replaceMap, true)
 	if assert.NoError(t, err) {
@@ -118,9 +124,9 @@ func TestNestedReplaceString(t *testing.T) {
 func TestReplaceStringWithWhiteSpace(t *testing.T) {
 	replaceMap := map[string]string{"inputs.parameters.message": "hello world"}
 
-	test := `{{ inputs.parameters.message }}`
+	test := toJsonString(`{{ inputs.parameters.message }}`)
 	replacement, err := Replace(test, replaceMap, true)
 	if assert.NoError(t, err) {
-		assert.Equal(t, "hello world", replacement)
+		assert.Equal(t, toJsonString("hello world"), replacement)
 	}
 }