From 99ecf57c6c2ecbe34e108a555c0541d6e2f8405f Mon Sep 17 00:00:00 2001 From: Arash Deshmeh Date: Wed, 21 Feb 2018 06:29:08 -0500 Subject: [PATCH 1/4] added support for mandatory variables to cli/compose Signed-off-by: Arash Deshmeh --- cli/compose/template/template.go | 22 +++++++++- cli/compose/template/template_test.go | 60 +++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 1 deletion(-) diff --git a/cli/compose/template/template.go b/cli/compose/template/template.go index 8426a23522fb..9bfc7cec5c79 100644 --- a/cli/compose/template/template.go +++ b/cli/compose/template/template.go @@ -7,7 +7,7 @@ import ( ) var delimiter = "\\$" -var substitution = "[_a-z][_a-z0-9]*(?::?-[^}]+)?" +var substitution = "[_a-z][_a-z0-9]*(?::?[-?][^}]+)?" var patternString = fmt.Sprintf( "%s(?i:(?P%s)|(?P%s)|{(?P%s)}|(?P))", @@ -69,6 +69,26 @@ func Substitute(template string, mapping Mapping) (string, error) { return value } + if strings.Contains(substitution, ":?") { + name, errorMessage := partition(substitution, ":?") + value, ok := mapping(name) + if !ok || value == "" { + err = &InvalidTemplateError{Template: errorMessage} + return "" + } + return value + } + + if strings.Contains(substitution, "?") { + name, errorMessage := partition(substitution, "?") + value, ok := mapping(name) + if !ok { + err = &InvalidTemplateError{Template: errorMessage} + return "" + } + return value + } + // No default (fall back to empty string) value, ok := mapping(substitution) if !ok { diff --git a/cli/compose/template/template_test.go b/cli/compose/template/template_test.go index 4ad3f252ac71..d11b0a8ec36b 100644 --- a/cli/compose/template/template_test.go +++ b/cli/compose/template/template_test.go @@ -81,3 +81,63 @@ func TestNonAlphanumericDefault(t *testing.T) { assert.Nil(t, err) assert.Equal(t, "ok /non:-alphanumeric", result) } + +func TestMandatoryVariableErrors(t *testing.T) { + testCases := []struct { + template string + expectedError string + }{ + { + template: "not ok ${UNSET_VAR:?Mandatory Variable Unset}", + expectedError: "Mandatory Variable Unset", + }, + { + template: "not ok ${BAR:?Mandatory Variable Empty}", + expectedError: "Mandatory Variable Empty", + }, + { + template: "not ok ${UNSET_VAR:?}", + expectedError: "", + }, + { + template: "not ok ${UNSET_VAR?Mandatory Variable Unset", + expectedError: "Mandatory Variable Unset", + }, + { + template: "not ok ${UNSET_VAR?}", + expectedError: "", + }, + } + + for _, tc := range testCases { + _, err := Substitute(tc.template, defaultMapping) + assert.Error(t, err) + assert.IsType(t, &InvalidTemplateError{tc.expectedError}, err) + } +} + +func TestDefaultsForMandatoryVariables(t *testing.T) { + testCases := []struct { + template string + expected string + }{ + { + template: "ok ${FOO:?err}", + expected: "ok first", + }, + { + template: "ok ${FOO?err}", + expected: "ok first", + }, + { + template: "ok ${BAR?err}", + expected: "ok ", + }, + } + + for _, tc := range testCases { + result, err := Substitute(tc.template, defaultMapping) + assert.Nil(t, err) + assert.Equal(t, tc.expected, result) + } +} From ce544823b61b856fad8483e05796a2f2831116ca Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 21 Feb 2018 15:16:12 -0500 Subject: [PATCH 2/4] Refactor substitute to reduce cyclo complexity Signed-off-by: Daniel Nephin --- cli/compose/template/template.go | 91 +++++++++++++-------------- cli/compose/template/template_test.go | 7 +++ 2 files changed, 51 insertions(+), 47 deletions(-) diff --git a/cli/compose/template/template.go b/cli/compose/template/template.go index 9bfc7cec5c79..2792a09ba539 100644 --- a/cli/compose/template/template.go +++ b/cli/compose/template/template.go @@ -37,77 +37,74 @@ func Substitute(template string, mapping Mapping) (string, error) { var err error result := pattern.ReplaceAllStringFunc(template, func(substring string) string { matches := pattern.FindStringSubmatch(substring) - groups := make(map[string]string) - for i, name := range pattern.SubexpNames() { - if i != 0 { - groups[name] = matches[i] - } + groups := matchGroups(matches) + if escaped := groups["escaped"]; escaped != "" { + return escaped } substitution := groups["named"] if substitution == "" { substitution = groups["braced"] } - if substitution != "" { - // Soft default (fall back if unset or empty) - if strings.Contains(substitution, ":-") { - name, defaultValue := partition(substitution, ":-") - value, ok := mapping(name) - if !ok || value == "" { - return defaultValue - } - return value - } - // Hard default (fall back if-and-only-if empty) - if strings.Contains(substitution, "-") { - name, defaultValue := partition(substitution, "-") - value, ok := mapping(name) - if !ok { - return defaultValue - } - return value + switch { + + case substitution == "": + err = &InvalidTemplateError{Template: template} + return "" + + // Soft default (fall back if unset or empty) + case strings.Contains(substitution, ":-"): + name, defaultValue := partition(substitution, ":-") + value, ok := mapping(name) + if !ok || value == "" { + return defaultValue } + return value - if strings.Contains(substitution, ":?") { - name, errorMessage := partition(substitution, ":?") - value, ok := mapping(name) - if !ok || value == "" { - err = &InvalidTemplateError{Template: errorMessage} - return "" - } - return value + // Hard default (fall back if-and-only-if empty) + case strings.Contains(substitution, "-"): + name, defaultValue := partition(substitution, "-") + value, ok := mapping(name) + if !ok { + return defaultValue } + return value - if strings.Contains(substitution, "?") { - name, errorMessage := partition(substitution, "?") - value, ok := mapping(name) - if !ok { - err = &InvalidTemplateError{Template: errorMessage} - return "" - } - return value + case strings.Contains(substitution, ":?"): + name, errorMessage := partition(substitution, ":?") + value, ok := mapping(name) + if !ok || value == "" { + err = &InvalidTemplateError{Template: errorMessage} + return "" } + return value - // No default (fall back to empty string) - value, ok := mapping(substitution) + case strings.Contains(substitution, "?"): + name, errorMessage := partition(substitution, "?") + value, ok := mapping(name) if !ok { + err = &InvalidTemplateError{Template: errorMessage} return "" } return value } - if escaped := groups["escaped"]; escaped != "" { - return escaped - } - - err = &InvalidTemplateError{Template: template} - return "" + value, _ := mapping(substitution) + return value }) return result, err } +func matchGroups(matches []string) map[string]string { + groups := make(map[string]string) + for i, name := range pattern.SubexpNames()[1:] { + groups[name] = matches[i+1] + } + return groups +} + // Split the string at the first occurrence of sep, and return the part before the separator, // and the part after the separator. // diff --git a/cli/compose/template/template_test.go b/cli/compose/template/template_test.go index d11b0a8ec36b..ce75216dde8c 100644 --- a/cli/compose/template/template_test.go +++ b/cli/compose/template/template_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) var defaults = map[string]string{ @@ -22,6 +23,12 @@ func TestEscaped(t *testing.T) { assert.Equal(t, "${foo}", result) } +func TestSubstituteNoMatch(t *testing.T) { + result, err := Substitute("foo", defaultMapping) + require.NoError(t, err) + require.Equal(t, "foo", result) +} + func TestInvalid(t *testing.T) { invalidTemplates := []string{ "${", From e33bc48752c35bf3ac4a86e904732aac4527887b Mon Sep 17 00:00:00 2001 From: Arash Deshmeh Date: Wed, 21 Feb 2018 17:02:38 -0500 Subject: [PATCH 3/4] Added error message check to TestMandatoryVariableErrors test Signed-off-by: Arash Deshmeh --- cli/compose/template/template_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cli/compose/template/template_test.go b/cli/compose/template/template_test.go index ce75216dde8c..66e50124edfd 100644 --- a/cli/compose/template/template_test.go +++ b/cli/compose/template/template_test.go @@ -3,6 +3,7 @@ package template import ( "testing" + "github.com/docker/cli/internal/test/testutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -119,7 +120,8 @@ func TestMandatoryVariableErrors(t *testing.T) { for _, tc := range testCases { _, err := Substitute(tc.template, defaultMapping) assert.Error(t, err) - assert.IsType(t, &InvalidTemplateError{tc.expectedError}, err) + assert.IsType(t, &InvalidTemplateError{}, err) + testutil.ErrorContains(t, err, tc.expectedError) } } From 5d8ce59a2578d7c145c33461727aed6391aa22bf Mon Sep 17 00:00:00 2001 From: Arash Deshmeh Date: Mon, 26 Feb 2018 15:23:52 -0500 Subject: [PATCH 4/4] fix the error message in Substitute function Signed-off-by: Arash Deshmeh --- cli/compose/template/template.go | 10 +++++++--- cli/compose/template/template_test.go | 12 ++++++------ 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/cli/compose/template/template.go b/cli/compose/template/template.go index 2792a09ba539..086194b8fe84 100644 --- a/cli/compose/template/template.go +++ b/cli/compose/template/template.go @@ -7,7 +7,7 @@ import ( ) var delimiter = "\\$" -var substitution = "[_a-z][_a-z0-9]*(?::?[-?][^}]+)?" +var substitution = "[_a-z][_a-z0-9]*(?::?[-?][^}]*)?" var patternString = fmt.Sprintf( "%s(?i:(?P%s)|(?P%s)|{(?P%s)}|(?P))", @@ -75,7 +75,9 @@ func Substitute(template string, mapping Mapping) (string, error) { name, errorMessage := partition(substitution, ":?") value, ok := mapping(name) if !ok || value == "" { - err = &InvalidTemplateError{Template: errorMessage} + err = &InvalidTemplateError{ + Template: fmt.Sprintf("required variable %s is missing a value: %s", name, errorMessage), + } return "" } return value @@ -84,7 +86,9 @@ func Substitute(template string, mapping Mapping) (string, error) { name, errorMessage := partition(substitution, "?") value, ok := mapping(name) if !ok { - err = &InvalidTemplateError{Template: errorMessage} + err = &InvalidTemplateError{ + Template: fmt.Sprintf("required variable %s is missing a value: %s", name, errorMessage), + } return "" } return value diff --git a/cli/compose/template/template_test.go b/cli/compose/template/template_test.go index 66e50124edfd..4fec57e8e8de 100644 --- a/cli/compose/template/template_test.go +++ b/cli/compose/template/template_test.go @@ -97,23 +97,23 @@ func TestMandatoryVariableErrors(t *testing.T) { }{ { template: "not ok ${UNSET_VAR:?Mandatory Variable Unset}", - expectedError: "Mandatory Variable Unset", + expectedError: "required variable UNSET_VAR is missing a value: Mandatory Variable Unset", }, { template: "not ok ${BAR:?Mandatory Variable Empty}", - expectedError: "Mandatory Variable Empty", + expectedError: "required variable BAR is missing a value: Mandatory Variable Empty", }, { template: "not ok ${UNSET_VAR:?}", - expectedError: "", + expectedError: "required variable UNSET_VAR is missing a value", }, { - template: "not ok ${UNSET_VAR?Mandatory Variable Unset", - expectedError: "Mandatory Variable Unset", + template: "not ok ${UNSET_VAR?Mandatory Variable Unset}", + expectedError: "required variable UNSET_VAR is missing a value: Mandatory Variable Unset", }, { template: "not ok ${UNSET_VAR?}", - expectedError: "", + expectedError: "required variable UNSET_VAR is missing a value", }, }