Repository: compose-spec/compose-go
Pull request #626 (2024-05-14 07:48:58)

Fix variable extraction when string uses variables concatenation
by using `(.*)` to capture default value, variable regex can't distinguish multiple variables being concatenated
trying to fix the regex doesn't seem trivial (at least for me) and introduces a risk to break variable interpolation, so I preferred to replicate the interpolate login in variable extraction to recursively search for variables in the default value

also moved variable extraction logic to a dedicated go file for clarity

fixes https://github.com/docker/compose/issues/11785


Patch:
diff --git a/template/template.go b/template/template.go
index 0d9c17e..d9483cb 100644
--- a/template/template.go
+++ b/template/template.go
@@ -271,102 +271,6 @@ func Substitute(template string, mapping Mapping) (string, error) {
 	return SubstituteWith(template, mapping, DefaultPattern)
 }
 
-// ExtractVariables returns a map of all the variables defined in the specified
-// composefile (dict representation) and their default value if any.
-func ExtractVariables(configDict map[string]interface{}, pattern *regexp.Regexp) map[string]Variable {
-	if pattern == nil {
-		pattern = DefaultPattern
-	}
-	return recurseExtract(configDict, pattern)
-}
-
-func recurseExtract(value interface{}, pattern *regexp.Regexp) map[string]Variable {
-	m := map[string]Variable{}
-
-	switch value := value.(type) {
-	case string:
-		if values, is := extractVariable(value, pattern); is {
-			for _, v := range values {
-				m[v.Name] = v
-			}
-		}
-	case map[string]interface{}:
-		for _, elem := range value {
-			submap := recurseExtract(elem, pattern)
-			for key, value := range submap {
-				m[key] = value
-			}
-		}
-
-	case []interface{}:
-		for _, elem := range value {
-			if values, is := extractVariable(elem, pattern); is {
-				for _, v := range values {
-					m[v.Name] = v
-				}
-			}
-		}
-	}
-
-	return m
-}
-
-type Variable struct {
-	Name          string
-	DefaultValue  string
-	PresenceValue string
-	Required      bool
-}
-
-func extractVariable(value interface{}, pattern *regexp.Regexp) ([]Variable, bool) {
-	sValue, ok := value.(string)
-	if !ok {
-		return []Variable{}, false
-	}
-	matches := pattern.FindAllStringSubmatch(sValue, -1)
-	if len(matches) == 0 {
-		return []Variable{}, false
-	}
-	values := []Variable{}
-	for _, match := range matches {
-		groups := matchGroups(match, pattern)
-		if escaped := groups[groupEscaped]; escaped != "" {
-			continue
-		}
-		val := groups[groupNamed]
-		if val == "" {
-			val = groups[groupBraced]
-		}
-		name := val
-		var defaultValue string
-		var presenceValue string
-		var required bool
-		switch {
-		case strings.Contains(val, ":?"):
-			name, _ = partition(val, ":?")
-			required = true
-		case strings.Contains(val, "?"):
-			name, _ = partition(val, "?")
-			required = true
-		case strings.Contains(val, ":-"):
-			name, defaultValue = partition(val, ":-")
-		case strings.Contains(val, "-"):
-			name, defaultValue = partition(val, "-")
-		case strings.Contains(val, ":+"):
-			name, presenceValue = partition(val, ":+")
-		case strings.Contains(val, "+"):
-			name, presenceValue = partition(val, "+")
-		}
-		values = append(values, Variable{
-			Name:          name,
-			DefaultValue:  defaultValue,
-			PresenceValue: presenceValue,
-			Required:      required,
-		})
-	}
-	return values, len(values) > 0
-}
-
 // Soft default (fall back if unset or empty)
 func defaultWhenEmptyOrUnset(substitution string, mapping Mapping) (string, bool, error) {
 	return withDefaultWhenAbsence(substitution, mapping, true)
diff --git a/template/variables.go b/template/variables.go
new file mode 100644
index 0000000..fb32f9c
--- /dev/null
+++ b/template/variables.go
@@ -0,0 +1,155 @@
+/*
+   Copyright 2020 The Compose Specification Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package template
+
+import (
+	"regexp"
+	"strings"
+)
+
+type Variable struct {
+	Name          string
+	DefaultValue  string
+	PresenceValue string
+	Required      bool
+}
+
+// ExtractVariables returns a map of all the variables defined in the specified
+// compose file (dict representation) and their default value if any.
+func ExtractVariables(configDict map[string]interface{}, pattern *regexp.Regexp) map[string]Variable {
+	if pattern == nil {
+		pattern = DefaultPattern
+	}
+	return recurseExtract(configDict, pattern)
+}
+
+func recurseExtract(value interface{}, pattern *regexp.Regexp) map[string]Variable {
+	m := map[string]Variable{}
+
+	switch value := value.(type) {
+	case string:
+		if values, is := extractVariable(value, pattern); is {
+			for _, v := range values {
+				m[v.Name] = v
+			}
+		}
+	case map[string]interface{}:
+		for _, elem := range value {
+			submap := recurseExtract(elem, pattern)
+			for key, value := range submap {
+				m[key] = value
+			}
+		}
+
+	case []interface{}:
+		for _, elem := range value {
+			if values, is := extractVariable(elem, pattern); is {
+				for _, v := range values {
+					m[v.Name] = v
+				}
+			}
+		}
+	}
+
+	return m
+}
+
+func extractVariable(value interface{}, pattern *regexp.Regexp) ([]Variable, bool) {
+	sValue, ok := value.(string)
+	if !ok {
+		return []Variable{}, false
+	}
+	matches := pattern.FindAllStringSubmatch(sValue, -1)
+	if len(matches) == 0 {
+		return []Variable{}, false
+	}
+	values := []Variable{}
+	for _, match := range matches {
+		groups := matchGroups(match, pattern)
+		if escaped := groups[groupEscaped]; escaped != "" {
+			continue
+		}
+		val := groups[groupNamed]
+		if val == "" {
+			val = groups[groupBraced]
+			s := match[0]
+			i := getFirstBraceClosingIndex(s)
+			if i > 0 {
+				val = s[2:i]
+				if len(s) > i {
+					if v, b := extractVariable(s[i+1:], pattern); b {
+						values = append(values, v...)
+					}
+				}
+			}
+		}
+		name := val
+		var defaultValue string
+		var presenceValue string
+		var required bool
+		i := strings.IndexFunc(val, func(r rune) bool {
+			if r >= 'a' && r <= 'z' {
+				return false
+			}
+			if r >= 'A' && r <= 'Z' {
+				return false
+			}
+			if r == '_' {
+				return false
+			}
+			return true
+		})
+
+		if i > 0 {
+			name = val[:i]
+			rest := val[i:]
+			switch {
+			case strings.HasPrefix(rest, ":?"):
+				required = true
+			case strings.HasPrefix(rest, "?"):
+				required = true
+			case strings.HasPrefix(rest, ":-"):
+				defaultValue = rest[2:]
+			case strings.HasPrefix(rest, "-"):
+				defaultValue = rest[1:]
+			case strings.HasPrefix(rest, ":+"):
+				presenceValue = rest[2:]
+			case strings.HasPrefix(rest, "+"):
+				presenceValue = rest[1:]
+			}
+		}
+
+		values = append(values, Variable{
+			Name:          name,
+			DefaultValue:  defaultValue,
+			PresenceValue: presenceValue,
+			Required:      required,
+		})
+
+		if defaultValue != "" {
+			if v, b := extractVariable(defaultValue, pattern); b {
+				values = append(values, v...)
+			}
+		}
+		if presenceValue != "" {
+			if v, b := extractVariable(presenceValue, pattern); b {
+				values = append(values, v...)
+			}
+		}
+	}
+	return values, len(values) > 0
+}


Test patch:
diff --git a/template/template_test.go b/template/template_test.go
index b1afa21..c52fdcd 100644
--- a/template/template_test.go
+++ b/template/template_test.go
@@ -405,6 +405,10 @@ func TestSubstituteWithReplacementFunc(t *testing.T) {
 
 	_, err = SubstituteWithOptions("ok ${NOTHERE}", defaultMapping, options...)
 	assert.Check(t, is.ErrorContains(err, "bad choice"))
+
+	result, err = SubstituteWith("ok ${SUBDOMAIN:-redis}.${FOO:?}", defaultMapping, DefaultPattern)
+	assert.NilError(t, err)
+	assert.Check(t, is.Equal("ok redis.first", result))
 }
 
 func TestSubstituteWithReplacementAppliedFunc(t *testing.T) {
@@ -478,128 +482,6 @@ func TestPrecedence(t *testing.T) {
 	}
 }
 
-func TestExtractVariables(t *testing.T) {
-	testCases := []struct {
-		name     string
-		dict     map[string]interface{}
-		expected map[string]Variable
-	}{
-		{
-			name:     "empty",
-			dict:     map[string]interface{}{},
-			expected: map[string]Variable{},
-		},
-		{
-			name: "no-variables",
-			dict: map[string]interface{}{
-				"foo": "bar",
-			},
-			expected: map[string]Variable{},
-		},
-		{
-			name: "variable-without-curly-braces",
-			dict: map[string]interface{}{
-				"foo": "$bar",
-			},
-			expected: map[string]Variable{
-				"bar": {Name: "bar"},
-			},
-		},
-		{
-			name: "variable",
-			dict: map[string]interface{}{
-				"foo": "${bar}",
-			},
-			expected: map[string]Variable{
-				"bar": {Name: "bar", DefaultValue: ""},
-			},
-		},
-		{
-			name: "required-variable",
-			dict: map[string]interface{}{
-				"foo": "${bar?:foo}",
-			},
-			expected: map[string]Variable{
-				"bar": {Name: "bar", DefaultValue: "", Required: true},
-			},
-		},
-		{
-			name: "required-variable2",
-			dict: map[string]interface{}{
-				"foo": "${bar?foo}",
-			},
-			expected: map[string]Variable{
-				"bar": {Name: "bar", DefaultValue: "", Required: true},
-			},
-		},
-		{
-			name: "default-variable",
-			dict: map[string]interface{}{
-				"foo": "${bar:-foo}",
-			},
-			expected: map[string]Variable{
-				"bar": {Name: "bar", DefaultValue: "foo"},
-			},
-		},
-		{
-			name: "default-variable2",
-			dict: map[string]interface{}{
-				"foo": "${bar-foo}",
-			},
-			expected: map[string]Variable{
-				"bar": {Name: "bar", DefaultValue: "foo"},
-			},
-		},
-		{
-			name: "multiple-values",
-			dict: map[string]interface{}{
-				"foo": "${bar:-foo}",
-				"bar": map[string]interface{}{
-					"foo": "${fruit:-banana}",
-					"bar": "vegetable",
-				},
-				"baz": []interface{}{
-					"foo",
-					"$docker:${project:-cli}",
-					"$toto",
-				},
-			},
-			expected: map[string]Variable{
-				"bar":     {Name: "bar", DefaultValue: "foo"},
-				"fruit":   {Name: "fruit", DefaultValue: "banana"},
-				"toto":    {Name: "toto", DefaultValue: ""},
-				"docker":  {Name: "docker", DefaultValue: ""},
-				"project": {Name: "project", DefaultValue: "cli"},
-			},
-		},
-		{
-			name: "presence-value-nonEmpty",
-			dict: map[string]interface{}{
-				"foo": "${bar:+foo}",
-			},
-			expected: map[string]Variable{
-				"bar": {Name: "bar", PresenceValue: "foo"},
-			},
-		},
-		{
-			name: "presence-value",
-			dict: map[string]interface{}{
-				"foo": "${bar+foo}",
-			},
-			expected: map[string]Variable{
-				"bar": {Name: "bar", PresenceValue: "foo"},
-			},
-		},
-	}
-	for _, tc := range testCases {
-		tc := tc
-		t.Run(tc.name, func(t *testing.T) {
-			actual := ExtractVariables(tc.dict, DefaultPattern)
-			assert.Check(t, is.DeepEqual(actual, tc.expected))
-		})
-	}
-}
-
 func TestSubstitutionFunctionChoice(t *testing.T) {
 	testcases := []struct {
 		name   string
diff --git a/template/variables_test.go b/template/variables_test.go
new file mode 100644
index 0000000..00133d5
--- /dev/null
+++ b/template/variables_test.go
@@ -0,0 +1,166 @@
+/*
+   Copyright 2020 The Compose Specification Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package template
+
+import (
+	"testing"
+
+	"gotest.tools/v3/assert"
+	is "gotest.tools/v3/assert/cmp"
+)
+
+func TestExtractVariables(t *testing.T) {
+	testCases := []struct {
+		name     string
+		dict     map[string]interface{}
+		expected map[string]Variable
+	}{
+		{
+			name:     "empty",
+			dict:     map[string]interface{}{},
+			expected: map[string]Variable{},
+		},
+		{
+			name: "no-variables",
+			dict: map[string]interface{}{
+				"foo": "bar",
+			},
+			expected: map[string]Variable{},
+		},
+		{
+			name: "variable-without-curly-braces",
+			dict: map[string]interface{}{
+				"foo": "$bar",
+			},
+			expected: map[string]Variable{
+				"bar": {Name: "bar"},
+			},
+		},
+		{
+			name: "variable",
+			dict: map[string]interface{}{
+				"foo": "${bar}",
+			},
+			expected: map[string]Variable{
+				"bar": {Name: "bar", DefaultValue: ""},
+			},
+		},
+		{
+			name: "required-variable",
+			dict: map[string]interface{}{
+				"foo": "${bar?:foo}",
+			},
+			expected: map[string]Variable{
+				"bar": {Name: "bar", DefaultValue: "", Required: true},
+			},
+		},
+		{
+			name: "required-variable2",
+			dict: map[string]interface{}{
+				"foo": "${bar?foo}",
+			},
+			expected: map[string]Variable{
+				"bar": {Name: "bar", DefaultValue: "", Required: true},
+			},
+		},
+		{
+			name: "default-variable",
+			dict: map[string]interface{}{
+				"foo": "${bar:-foo}",
+			},
+			expected: map[string]Variable{
+				"bar": {Name: "bar", DefaultValue: "foo"},
+			},
+		},
+		{
+			name: "default-variable2",
+			dict: map[string]interface{}{
+				"foo": "${bar-foo}",
+			},
+			expected: map[string]Variable{
+				"bar": {Name: "bar", DefaultValue: "foo"},
+			},
+		},
+		{
+			name: "multiple-values",
+			dict: map[string]interface{}{
+				"foo": "${bar:-foo}",
+				"bar": map[string]interface{}{
+					"foo": "${fruit:-banana}",
+					"bar": "vegetable",
+				},
+				"baz": []interface{}{
+					"foo",
+					"$docker:${project:-cli}",
+					"$toto",
+				},
+			},
+			expected: map[string]Variable{
+				"bar":     {Name: "bar", DefaultValue: "foo"},
+				"fruit":   {Name: "fruit", DefaultValue: "banana"},
+				"toto":    {Name: "toto", DefaultValue: ""},
+				"docker":  {Name: "docker", DefaultValue: ""},
+				"project": {Name: "project", DefaultValue: "cli"},
+			},
+		},
+		{
+			name: "presence-value-nonEmpty",
+			dict: map[string]interface{}{
+				"foo": "${bar:+foo}",
+			},
+			expected: map[string]Variable{
+				"bar": {Name: "bar", PresenceValue: "foo"},
+			},
+		},
+		{
+			name: "presence-value",
+			dict: map[string]interface{}{
+				"foo": "${bar+foo}",
+			},
+			expected: map[string]Variable{
+				"bar": {Name: "bar", PresenceValue: "foo"},
+			},
+		},
+		{
+			name: "concat",
+			dict: map[string]interface{}{
+				"domainname": "${SUBDOMAIN:-redis}.${ROOTDOMAIN:?}",
+			},
+			expected: map[string]Variable{
+				"ROOTDOMAIN": {Name: "ROOTDOMAIN", Required: true},
+				"SUBDOMAIN":  {Name: "SUBDOMAIN", DefaultValue: "redis"},
+			},
+		},
+		{
+			name: "nested",
+			dict: map[string]interface{}{
+				"domainname": "${SUBDOMAIN:-$ROOTDOMAIN}",
+			},
+			expected: map[string]Variable{
+				"ROOTDOMAIN": {Name: "ROOTDOMAIN"},
+				"SUBDOMAIN":  {Name: "SUBDOMAIN", DefaultValue: "$ROOTDOMAIN"},
+			},
+		},
+	}
+	for _, tc := range testCases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			actual := ExtractVariables(tc.dict, DefaultPattern)
+			assert.Check(t, is.DeepEqual(actual, tc.expected))
+		})
+	}
+}