From b286060ea8283be9aa55827327a3b5e3003e12b8 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Thu, 5 Jan 2023 10:25:59 -0800 Subject: [PATCH] Fix string regression in attribute parsing (#2759) In #2721, the ability for key-only values was added via using a wrapperspb.StringValue that could be null. This introduced a bug because originally the default case for `-attr` was to get the string value but instead it used the incoming value as-is to allow for nulls; the null check was moved up later but the original behavior was not restored. As a result, non-explicitly-quoted strings carried through the wrapperspb.StringValue, resulting in a map for the value in JSON instead of a string. This also fixes the ability to set explicitly quoted strings. --- internal/cmd/base/flags.go | 5 ++- internal/cmd/common/flags.go | 7 ++-- internal/cmd/common/flags_test.go | 54 +++++++++++++++++++++++++------ 3 files changed, 48 insertions(+), 18 deletions(-) diff --git a/internal/cmd/base/flags.go b/internal/cmd/base/flags.go index 783beb8748..3a09552023 100644 --- a/internal/cmd/base/flags.go +++ b/internal/cmd/base/flags.go @@ -1020,9 +1020,8 @@ func (c *combinedSliceValue) Set(val string) error { if err != nil && !errors.Is(err, parseutil.ErrNotAUrl) { return fmt.Errorf("error checking if value is a path: %w", err) } - if pathParsedValue != "" { - ret.Value = wrapperspb.String(pathParsedValue) - } + // This will either be the round-tripped value or the substituted value + ret.Value = wrapperspb.String(pathParsedValue) } *c.target = append(*c.target, ret) diff --git a/internal/cmd/common/flags.go b/internal/cmd/common/flags.go index 7935cbb1cb..f907545edb 100644 --- a/internal/cmd/common/flags.go +++ b/internal/cmd/common/flags.go @@ -288,7 +288,7 @@ func HandleAttributeFlags(c *base.Command, suffix, fullField string, sepFields [ if field.Value == nil { return fmt.Errorf("string-%s flag requires a value", suffix) } - val = strings.Trim(field.Value.GetValue(), `"`) + val = field.Value.GetValue() case "bool-" + suffix: if field.Value == nil { @@ -318,9 +318,6 @@ func HandleAttributeFlags(c *base.Command, suffix, fullField string, sepFields [ case field.Value.GetValue() == "false": // bool false val = false - case strings.HasPrefix(field.Value.GetValue(), `"`): // explicitly quoted string - val = strings.Trim(field.Value.GetValue(), `"`) - case jsonNumberRegex.MatchString(strings.Trim(field.Value.GetValue(), `"`)): // number // Same logic as above if strings.Contains(field.Value.GetValue(), ".") { @@ -355,7 +352,7 @@ func HandleAttributeFlags(c *base.Command, suffix, fullField string, sepFields [ default: // Default is to treat as a string value - val = field.Value + val = field.Value.GetValue() } default: diff --git a/internal/cmd/common/flags_test.go b/internal/cmd/common/flags_test.go index 91e12b7086..0f36b97a9c 100644 --- a/internal/cmd/common/flags_test.go +++ b/internal/cmd/common/flags_test.go @@ -3,6 +3,7 @@ package common import ( "encoding/json" "fmt" + "os" "strings" "testing" @@ -20,12 +21,14 @@ func TestPopulateAttrFlags(t *testing.T) { tests := []struct { name string args []string + envs [][]string expected []base.CombinedSliceFlagValue expectedErr string }{ { name: "strings-only", - args: []string{"-string-attr", "foo=bar", "-string-attr", `bar="baz"`}, + args: []string{"-string-attr", "foo=bar", "-string-attr", `bar="baz"`, "-string-attr", "zip=env://zip"}, + envs: [][]string{{"zip", "zap"}}, expected: []base.CombinedSliceFlagValue{ { Name: "string-attr", @@ -37,11 +40,17 @@ func TestPopulateAttrFlags(t *testing.T) { Keys: []string{"bar"}, Value: wrapperspb.String(`"baz"`), }, + { + Name: "string-attr", + Keys: []string{"zip"}, + Value: wrapperspb.String("zap"), + }, }, }, { name: "nums-only", - args: []string{"-num-attr", "foo=-1.2", "-num-attr", "bar=5"}, + args: []string{"-num-attr", "foo=-1.2", "-num-attr", "bar=5", "-num-attr", "zip=env://zip"}, + envs: [][]string{{"zip", "5"}}, expected: []base.CombinedSliceFlagValue{ { Name: "num-attr", @@ -53,11 +62,17 @@ func TestPopulateAttrFlags(t *testing.T) { Keys: []string{"bar"}, Value: wrapperspb.String("5"), }, + { + Name: "num-attr", + Keys: []string{"zip"}, + Value: wrapperspb.String("5"), + }, }, }, { name: "bools-only", - args: []string{"-bool-attr", "foo=true", "-bool-attr", "bar=false"}, + args: []string{"-bool-attr", "foo=true", "-bool-attr", "bar=false", "-bool-attr", "zip=env://zip"}, + envs: [][]string{{"zip", "true"}}, expected: []base.CombinedSliceFlagValue{ { Name: "bool-attr", @@ -69,6 +84,11 @@ func TestPopulateAttrFlags(t *testing.T) { Keys: []string{"bar"}, Value: wrapperspb.String("false"), }, + { + Name: "bool-attr", + Keys: []string{"zip"}, + Value: wrapperspb.String("true"), + }, }, }, { @@ -98,7 +118,7 @@ func TestPopulateAttrFlags(t *testing.T) { }, { name: "mixed", - args: []string{"-num-attr", "foo=9820", "-string-attr", "bar=9820", "-attr", "baz=9820"}, + args: []string{"-num-attr", "foo=9820", "-string-attr", "bar=9820", "-attr", "baz=9820", "-attr", `zoom="flubber"`}, expected: []base.CombinedSliceFlagValue{ { Name: "num-attr", @@ -115,11 +135,17 @@ func TestPopulateAttrFlags(t *testing.T) { Keys: []string{"baz"}, Value: wrapperspb.String("9820"), }, + { + Name: "attr", + Keys: []string{"zoom"}, + Value: wrapperspb.String("\"flubber\""), + }, }, }, { name: "mixed-segments", - args: []string{"-num-attr", "foo.bar.baz=9820", "-string-attr", "bar.baz.foo=9820", "-attr", "baz.foo.bar=9820"}, + args: []string{"-num-attr", "foo.bar.baz=9820", "-string-attr", "bar.baz.foo=9820", "-attr", "baz.foo.bar=9820", "-attr", "zip=env://zip"}, + envs: [][]string{{"zip", "zap"}}, expected: []base.CombinedSliceFlagValue{ { Name: "num-attr", @@ -136,6 +162,11 @@ func TestPopulateAttrFlags(t *testing.T) { Keys: []string{"baz", "foo", "bar"}, Value: wrapperspb.String("9820"), }, + { + Name: "attr", + Keys: []string{"zip"}, + Value: wrapperspb.String("zap"), + }, }, }, { @@ -192,6 +223,9 @@ func TestPopulateAttrFlags(t *testing.T) { } PopulateCombinedSliceFlagValue(attrsInput) + for _, env := range tt.envs { + require.NoError(os.Setenv(env[0], env[1])) + } err := flagSet.Parse(tt.args) if tt.expectedErr != "" { require.Error(err) @@ -231,7 +265,7 @@ func TestHandleAttributeFlags(t *testing.T) { }, expectedMap: map[string]any{ "foo": "bar", - "bar": "baz", + "bar": "\"baz\"", }, }, { @@ -368,7 +402,7 @@ func TestHandleAttributeFlags(t *testing.T) { { Name: "%s", Keys: []string{"s2"}, - Value: wrapperspb.String(`"woop"`), + Value: wrapperspb.String("\"woo\"p"), }, { Name: "%s", @@ -399,8 +433,8 @@ func TestHandleAttributeFlags(t *testing.T) { expectedMap: map[string]any{ "b1": true, "b2": false, - "s1": wrapperspb.String("scoopde"), - "s2": "woop", + "s1": "scoopde", + "s2": "\"woo\"p", "n1": float64(-1.2), "n2": int64(5), "a": []any{ @@ -462,7 +496,7 @@ func TestHandleAttributeFlags(t *testing.T) { expectedMap: map[string]any{ "bools": []any{true, false}, "strings": map[string]any{ - "s1": wrapperspb.String("scoopde"), + "s1": "scoopde", "s2": nil, }, "numbers": map[string]any{