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.
3 years ago · b286060ea8
parent 9135cc7668
commit b286060ea8
3 changed files with 48 additions and 18 deletions
--- 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)
--- 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:
--- 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{