Accept bytes as base64-encoded string by default (#631)

* Decode bytes as base64 by default

Fall back to bytes-as-quoted-literals mode if base64 decoding fails, and
log a warning.

* e2e: extend testcases

Test various cases, entering base64 or quoted literals without explicitly specifying --bytes-as-base64 or --bytes-as-quoted-literals.

* fill/proto/interactive_filler_test: fix

This test actually didn't succeed properly, but we didn't notice, as the
error wasn't propagated properly.

README.md

@@ -263,27 +263,41 @@ When <kbd>CTRL-D</kbd> is entered, inputting will be aborted.
 ```
 
 ### Bytes type fields
-You can use byte literal and Unicode literal.
+You can pass bytes as a base64-encoded string.
 
 ```
 > call UnaryBytes
+data (TYPE_BYTES) => SGVsbG8gV29ybGQh
+{
+  "message": "received: (bytes) 48 65 6c 6c 6f 20 57 6f 72 6c 64 21, (string) Hello World!"
+}
+```
+
+⚠️  Warning: Previously, bytes were passed as a (quoted) byte literal or Unicode
+literal string.
+
+While evans currenty still attempts to fall back to that encoding if decoding
+as base64 fails, it's discouraged, and might be dropped.
+
+Please either encode bytes as base64, or pass `--bytes-as-quoted-literals`
+explicitly:
+
+```
+> call UnaryBytes --bytes-as-quoted-literals
 data (TYPE_BYTES) => \x46\x6f\x6f
 {
   "message": "received: (bytes) 46 6f 6f, (string) Foo"
 }
 
-> call UnaryBytes
+> call UnaryBytes --bytes-as-quoted-literals
 data (TYPE_BYTES) => \u65e5\u672c\u8a9e
 {
   "message": "received: (bytes) e6 97 a5 e6 9c ac e8 aa 9e, (string) 日本語"
 }
 ```
 
-Or add the flag `--bytes-as-base64` to pass bytes as a base64-encoded string
-```
-> call UnaryBytes --bytes-as-base64
-data (TYPE_BYTES) => SGVsbG8gV29ybGQh
-```
+You can also add the flag `--bytes-as-base64` to explicitly disable the
+fallback behaviour.
 
 Or add the flag `--bytes-from-file` to read bytes from the provided relative path
 ```

e2e/old_repl_test.go

@@ -121,9 +121,21 @@ func TestE2E_OldREPL(t *testing.T) {
 			input: []interface{}{"call UnaryEnum", 0},
 		},
 		"call UnaryBytes": {
+			args:  "testdata/test.proto",
+			input: []interface{}{"call UnaryBytes", "44KE44Gv44KK5L+644Gu6Z2S5pil44Op44OW44Kz44Oh44Gv44G+44Gh44GM44Gj44Gm44GE44KL44CC"},
+		},
+		"call UnaryBytes (fallback)": {
 			args:  "testdata/test.proto",
 			input: []interface{}{"call UnaryBytes", "\\u3084\\u306f\\u308a\\u4ffa\\u306e\\u9752\\u6625\\u30e9\\u30d6\\u30b3\\u30e1\\u306f\\u307e\\u3061\\u304c\\u3063\\u3066\\u3044\\u308b\\u3002"},
 		},
+		"call UnaryBytes --bytes-as-base64": {
+			args:  "testdata/test.proto",
+			input: []interface{}{"call UnaryBytes --bytes-as-base64", "44KE44Gv44KK5L+644Gu6Z2S5pil44Op44OW44Kz44Oh44Gv44G+44Gh44GM44Gj44Gm44GE44KL44CC"},
+		},
+		"call UnaryBytes --bytes-as-quoted-literals": {
+			args:  "testdata/test.proto",
+			input: []interface{}{"call UnaryBytes --bytes-as-quoted-literals", "\\u3084\\u306f\\u308a\\u4ffa\\u306e\\u9752\\u6625\\u30e9\\u30d6\\u30b3\\u30e1\\u306f\\u307e\\u3061\\u304c\\u3063\\u3066\\u3044\\u308b\\u3002"},
+		},
 		"call UnaryRepeatedEnum": {
 			args:  "testdata/test.proto",
 			input: []interface{}{"call UnaryRepeatedEnum", 0, 0, 1, io.EOF},

e2e/repl_test.go

@@ -128,9 +128,21 @@ func TestE2E_REPL(t *testing.T) {
 			input:       []interface{}{"call UnaryEnum", 0},
 		},
 		"call UnaryBytes": {
+			commonFlags: "--proto testdata/test.proto",
+			input:       []interface{}{"call UnaryBytes", "44KE44Gv44KK5L+644Gu6Z2S5pil44Op44OW44Kz44Oh44Gv44G+44Gh44GM44Gj44Gm44GE44KL44CC"},
+		},
+		"call UnaryBytes (fallback)": {
 			commonFlags: "--proto testdata/test.proto",
 			input:       []interface{}{"call UnaryBytes", "\\u3084\\u306f\\u308a\\u4ffa\\u306e\\u9752\\u6625\\u30e9\\u30d6\\u30b3\\u30e1\\u306f\\u307e\\u3061\\u304c\\u3063\\u3066\\u3044\\u308b\\u3002"},
 		},
+		"call UnaryBytes --bytes-as-base64": {
+			commonFlags: "--proto testdata/test.proto",
+			input:       []interface{}{"call UnaryBytes --bytes-as-base64", "44KE44Gv44KK5L+644Gu6Z2S5pil44Op44OW44Kz44Oh44Gv44G+44Gh44GM44Gj44Gm44GE44KL44CC"},
+		},
+		"call UnaryBytes --bytes-as-quoted-literals": {
+			commonFlags: "--proto testdata/test.proto",
+			input:       []interface{}{"call UnaryBytes --bytes-as-quoted-literals", "\\u3084\\u306f\\u308a\\u4ffa\\u306e\\u9752\\u6625\\u30e9\\u30d6\\u30b3\\u30e1\\u306f\\u307e\\u3061\\u304c\\u3063\\u3066\\u3044\\u308b\\u3002"},
+		},
 		"call UnaryRepeatedEnum": {
 			commonFlags: "--proto testdata/test.proto",
 			input:       []interface{}{"call UnaryRepeatedEnum", 0, 0, 1, io.EOF},

e2e/testdata/fixtures/teste2e_oldrepl-call_unarybytes_(fallback).golden

@@ -0,0 +1,4 @@
+{
+  "message": "received: (bytes) e3 82 84 e3 81 af e3 82 8a e4 bf ba e3 81 ae e9 9d 92 e6 98 a5 e3 83 a9 e3 83 96 e3 82 b3 e3 83 a1 e3 81 af e3 81 be e3 81 a1 e3 81 8c e3 81 a3 e3 81 a6 e3 81 84 e3 82 8b e3 80 82, (string) やはり俺の青春ラブコメはまちがっている。"
+}
+

e2e/testdata/fixtures/teste2e_oldrepl-call_unarybytes_--bytes-as-base64.golden

@@ -0,0 +1,4 @@
+{
+  "message": "received: (bytes) e3 82 84 e3 81 af e3 82 8a e4 bf ba e3 81 ae e9 9d 92 e6 98 a5 e3 83 a9 e3 83 96 e3 82 b3 e3 83 a1 e3 81 af e3 81 be e3 81 a1 e3 81 8c e3 81 a3 e3 81 a6 e3 81 84 e3 82 8b e3 80 82, (string) やはり俺の青春ラブコメはまちがっている。"
+}
+

e2e/testdata/fixtures/teste2e_oldrepl-call_unarybytes_--bytes-as-quoted-literals.golden

@@ -0,0 +1,4 @@
+{
+  "message": "received: (bytes) e3 82 84 e3 81 af e3 82 8a e4 bf ba e3 81 ae e9 9d 92 e6 98 a5 e3 83 a9 e3 83 96 e3 82 b3 e3 83 a1 e3 81 af e3 81 be e3 81 a1 e3 81 8c e3 81 a3 e3 81 a6 e3 81 84 e3 82 8b e3 80 82, (string) やはり俺の青春ラブコメはまちがっている。"
+}
+

e2e/testdata/fixtures/teste2e_repl-call_--help.golden

@@ -1,11 +1,12 @@
 usage: call <method name>
 
 Options:
-      --add-repeated-manually   prompt asks whether to add a value if it encountered to a repeated field
-      --bytes-as-base64         interpret TYPE_BYTES input as base64-encoded string (mutually exclusive with --bytes-from-file)
-      --bytes-from-file         interpret TYPE_BYTES input as a relative path to a file (mutually exclusive with --bytes-as-base64)
-      --dig-manually            prompt asks whether to dig down if it encountered to a message field
-      --emit-defaults           render fields with default values
-      --enrich                  enrich response output includes header, message, trailer and status
-  -r, --repeat                  repeat previous unary or server streaming request (if exists)
+      --add-repeated-manually      prompt asks whether to add a value if it encountered to a repeated field
+      --bytes-as-base64            explicitly interpret TYPE_BYTES input as base64-encoded string (mutually exclusive with --bytes-from-file and --bytes-as-quoted-literals)
+      --bytes-as-quoted-literals   interpret TYPE_BYTES input as a string of (quoted) byte literal or Unicode (mutually exclusive with --bytes-from-file and --bytes-as-base64)
+      --bytes-from-file            interpret TYPE_BYTES input as a relative path to a file (mutually exclusive with --bytes-as-base64)
+      --dig-manually               prompt asks whether to dig down if it encountered to a message field
+      --emit-defaults              render fields with default values
+      --enrich                     enrich response output includes header, message, trailer and status
+  -r, --repeat                     repeat previous unary or server streaming request (if exists)
 

e2e/testdata/fixtures/teste2e_repl-call_unarybytes_(fallback).golden

@@ -0,0 +1,4 @@
+{
+  "message": "received: (bytes) e3 82 84 e3 81 af e3 82 8a e4 bf ba e3 81 ae e9 9d 92 e6 98 a5 e3 83 a9 e3 83 96 e3 82 b3 e3 83 a1 e3 81 af e3 81 be e3 81 a1 e3 81 8c e3 81 a3 e3 81 a6 e3 81 84 e3 82 8b e3 80 82, (string) やはり俺の青春ラブコメはまちがっている。"
+}
+

e2e/testdata/fixtures/teste2e_repl-call_unarybytes_--bytes-as-base64.golden

@@ -0,0 +1,4 @@
+{
+  "message": "received: (bytes) e3 82 84 e3 81 af e3 82 8a e4 bf ba e3 81 ae e9 9d 92 e6 98 a5 e3 83 a9 e3 83 96 e3 82 b3 e3 83 a1 e3 81 af e3 81 be e3 81 a1 e3 81 8c e3 81 a3 e3 81 a6 e3 81 84 e3 82 8b e3 80 82, (string) やはり俺の青春ラブコメはまちがっている。"
+}
+

e2e/testdata/fixtures/teste2e_repl-call_unarybytes_--bytes-as-quoted-literals.golden

@@ -0,0 +1,4 @@
+{
+  "message": "received: (bytes) e3 82 84 e3 81 af e3 82 8a e4 bf ba e3 81 ae e9 9d 92 e6 98 a5 e3 83 a9 e3 83 96 e3 82 b3 e3 83 a1 e3 81 af e3 81 be e3 81 a1 e3 81 8c e3 81 a3 e3 81 a6 e3 81 84 e3 82 8b e3 80 82, (string) やはり俺の青春ラブコメはまちがっている。"
+}
+

fill/filler.go

@@ -24,6 +24,8 @@ type InteractiveFillerOpts struct {
 	DigManually,
 	// BytesAsBase64 is true, Fill will interpret input as base64-encoded string
 	BytesAsBase64,
+	// BytesAsQuotedLiterals is true, Fill will interpret input as a string of (quoted) byte literal or Unicode
+	BytesAsQuotedLiterals,
 	// BytesFromFile is true, Fill will read the contents of the file from the provided relative path.
 	BytesFromFile,
 	// AddRepeatedManually is true, Fill asks whether to add a repeated field value

fill/proto/interactive_filler.go

@@ -12,6 +12,7 @@ import (
 	"github.com/jhump/protoreflect/desc/builder"
 	"github.com/jhump/protoreflect/dynamic"
 	"github.com/ktr0731/evans/fill"
+	"github.com/ktr0731/evans/logger"
 	"github.com/ktr0731/evans/prompt"
 	"github.com/pkg/errors"
 	"google.golang.org/protobuf/types/descriptorpb"
@@ -196,26 +197,60 @@ func (r *resolver) resolveField(f *desc.FieldDescriptor) error {
 		case descriptorpb.FieldDescriptorProto_TYPE_STRING:
 			converter = func(v string) (interface{}, error) { return v, nil }
 
+		// For bytes, if neither BytesAsBase64 nor BytesAsQuotedLiterals is explicitly set,
+		// try to decode as base64 first, and if that fails, fall back trying to parse
+		// as quoted literals (logging a warning).
+		//
+		// This is to preserve backwards compatibility, as we used to be accept quoted
+		// literals, but want to switch to base64.
+		//
+		// If either BytesAsBase64 or BytesAsQuotedLiterals is set, only parse it in that format,
+		// and if BytesFromFile is set, read it from file.
+		//
 		// Use strconv.Unquote to interpret byte literals and Unicode literals.
 		// For example, a user inputs `\x6f\x67\x69\x73\x6f`,
 		// His expects "ogiso" in string, but backslashes in the input are not interpreted as an escape sequence.
 		// So, we need to call strconv.Unquote to interpret backslashes as an escape sequence.
 		case descriptorpb.FieldDescriptorProto_TYPE_BYTES:
 			converter = func(v string) (interface{}, error) {
-				if r.opts.BytesAsBase64 {
+				if r.opts.BytesFromFile {
+					b, err := os.ReadFile(v)
+					if err != nil {
+						return nil, err
+					}
+					return b, nil
+
+				} else if r.opts.BytesAsBase64 {
 					b, err := base64.StdEncoding.DecodeString(v)
-					if err == nil {
-						return b, nil
+					if err != nil {
+						return nil, err
 					}
-				} else if r.opts.BytesFromFile {
-					b, err := os.ReadFile(v)
-					if err == nil {
-						return b, nil
+					return b, nil
+				} else if r.opts.BytesAsQuotedLiterals {
+					v, err := strconv.Unquote(`"` + v + `"`)
+
+					if err != nil {
+						return nil, err
 					}
+					return []byte(v), nil
 				}
 
-				v, err := strconv.Unquote(`"` + v + `"`)
-				return []byte(v), err
+				// try to decode as base64
+				b, err := base64.StdEncoding.DecodeString(v)
+				if err != nil {
+					// failed, try to parse as quoted literal
+					v, err2 := strconv.Unquote(`"` + v + `"`)
+					if err2 != nil {
+						// failed to parse as this too, assume user intended to input base64, propagate
+						// that error
+						return nil, err
+					}
+					// log a warning and return the decoded literal string
+					logger.Println(`warning: entering bytes as quoted literal is deprecated. Use --bytes-as-quoted-literals or base64 encoding"`)
+					return []byte(v), nil
+				}
+				// succeeded decoding as base64, return
+				return b, nil
 			}
 
 		default:

fill/proto/interactive_filler_test.go

@@ -76,8 +76,6 @@ func TestInteractiveFiller(t *testing.T) {
 	b.AddField(builder.NewField("o", builder.FieldTypeBool()))
 	b.AddField(builder.NewField("p", builder.FieldTypeString()))
 	b.AddField(builder.NewField("q", builder.FieldTypeBytes()))
-	b.AddField(builder.NewField("r", builder.FieldTypeBytes()))
-	b.AddField(builder.NewField("s", builder.FieldTypeBytes()))
 	m, err := b.Build()
 	if err != nil {
 		t.Fatalf("Build should not return an error, but got '%s'", err)
@@ -87,23 +85,21 @@ func TestInteractiveFiller(t *testing.T) {
 	p := &stubPrompt{
 		t: t,
 		input: []string{
-			"1.1",          // c
-			"1.2",          // d
-			"1",            // e
-			"2",            // f
-			"3",            // g
-			"4",            // h
-			"5",            // i
-			"6",            // j
-			"7",            // k
-			"8",            // l
-			"9",            // m
-			"10",           // n
-			"true",         // o
-			"foo",          // p
-			"bar",          // q
-			"\x62\x61\x7a", // r
-			"./proto.go",   // s
+			"1.1",        // c
+			"1.2",        // d
+			"1",          // e
+			"2",          // f
+			"3",          // g
+			"4",          // h
+			"5",          // i
+			"6",          // j
+			"7",          // k
+			"8",          // l
+			"9",          // m
+			"10",         // n
+			"true",       // o
+			"foo",        // p
+			"./proto.go", // q
 		},
 		selection: []int{
 			0, // a - yes
@@ -121,7 +117,7 @@ func TestInteractiveFiller(t *testing.T) {
 		return
 	}
 
-	const want = `{"a":[{}],"b":"enum2","c":1.1,"d":1.2,"e":"1","f":"2","g":"3","h":"4","i":"5","j":6,"k":7,"l":8,"m":9,"n":10,"o":true,"p":"foo","q":"YmFy","r":"YmF6","s":"Ly8gUGFja2FnZSBwcm90byBwcm92aWRlcyBhIGZpbGxlciBpbXBsZW1lbnRhdGlvbiBmb3IgUHJvdG9jb2wgQnVmZmVycy4KcGFja2FnZSBwcm90bwo="}`
+	const want = `{"a":[{}],"b":"enum2","c":1.1,"d":1.2,"e":"1","f":"2","g":"3","h":"4","i":"5","j":6,"k":7,"l":8,"m":9,"n":10,"o":true,"p":"foo","q":"Ly8gUGFja2FnZSBwcm90byBwcm92aWRlcyBhIGZpbGxlciBpbXBsZW1lbnRhdGlvbiBmb3IgUHJvdG9jb2wgQnVmZmVycy4KcGFja2FnZSBwcm90bwo="}`
 
 	marshaler := jsonpb.Marshaler{EmitDefaults: true}
 	got, err := marshaler.MarshalToString(msg)

repl/commands.go

@@ -155,15 +155,16 @@ func (c *showCommand) Run(w io.Writer, args []string) error {
 }
 
 type callCommand struct {
-	enrich, digManually, bytesAsBase64, bytesFromFile, emitDefaults, repeatCall, addRepeatedManually bool
+	enrich, digManually, bytesAsBase64, bytesAsQuotedLiterals, bytesFromFile, emitDefaults, repeatCall, addRepeatedManually bool
 }
 
 func (c *callCommand) FlagSet() (*pflag.FlagSet, bool) {
 	fs := pflag.NewFlagSet("call", pflag.ContinueOnError)
 	fs.Usage = func() {} // Disable help output when an error occurred.
 	fs.BoolVar(&c.enrich, "enrich", false, "enrich response output includes header, message, trailer and status")
 	fs.BoolVar(&c.digManually, "dig-manually", false, "prompt asks whether to dig down if it encountered to a message field")
-	fs.BoolVar(&c.bytesAsBase64, "bytes-as-base64", false, "interpret TYPE_BYTES input as base64-encoded string (mutually exclusive with --bytes-from-file)")
+	fs.BoolVar(&c.bytesAsBase64, "bytes-as-base64", false, "explicitly interpret TYPE_BYTES input as base64-encoded string (mutually exclusive with --bytes-from-file and --bytes-as-quoted-literals)")
+	fs.BoolVar(&c.bytesAsQuotedLiterals, "bytes-as-quoted-literals", false, "interpret TYPE_BYTES input as a string of (quoted) byte literal or Unicode (mutually exclusive with --bytes-from-file and --bytes-as-base64)")
 	fs.BoolVar(&c.bytesFromFile, "bytes-from-file", false, "interpret TYPE_BYTES input as a relative path to a file (mutually exclusive with --bytes-as-base64)")
 	fs.BoolVar(&c.emitDefaults, "emit-defaults", false, "render fields with default values")
 	fs.BoolVarP(&c.repeatCall, "repeat", "r", false, "repeat previous unary or server streaming request (if exists)")
@@ -200,15 +201,18 @@ func (c *callCommand) Run(w io.Writer, args []string) error {
 		},
 	)
 
-	// Ensure bytesAsBase64 and bytesFromFile are not both set
+	// Ensure only one of bytesAsBase64, bytesAsQuotedLiterals and bytesFromFile are not both set
 	// pflag doesn't suppport mutually exclusive flags (https://github.com/spf13/pflag/issues/270)
-	if c.bytesAsBase64 && c.bytesFromFile {
-		return errors.New("only one of --bytes-as-base64 or --bytes-from-file can be specified")
+	if c.bytesFromFile && (c.bytesAsBase64 || c.bytesAsQuotedLiterals) {
+		return errors.New("only one of --bytes-from-file or --bytes-as-* can be specified")
+	}
+	if c.bytesAsBase64 && c.bytesAsQuotedLiterals {
+		return errors.New("only one of --bytes-as-base64 or --bytes-as-quoted-literals can be specified")
 	}
 
 	// here we create the request context
 	// we also add the call command flags here
-	err := usecase.CallRPCInteractively(context.Background(), w, args[0], c.digManually, c.bytesAsBase64, c.bytesFromFile, c.repeatCall, c.addRepeatedManually)
+	err := usecase.CallRPCInteractively(context.Background(), w, args[0], c.digManually, c.bytesAsBase64, c.bytesAsQuotedLiterals, c.bytesFromFile, c.repeatCall, c.addRepeatedManually)
 	if errors.Is(err, io.EOF) {
 		return errors.New("inputting canceled")
 	}

usecase/call_rpc.go

@@ -478,18 +478,19 @@ func (f *interactiveFiller) Fill(v interface{}) error {
 	return f.fillFunc(v)
 }
 
-func CallRPCInteractively(ctx context.Context, w io.Writer, rpcName string, digManually, bytesAsBase64, bytesFromFile, rerunPrevious, addRepeatedManually bool) error {
-	return dm.CallRPCInteractively(ctx, w, rpcName, digManually, bytesAsBase64, bytesFromFile, rerunPrevious, addRepeatedManually)
+func CallRPCInteractively(ctx context.Context, w io.Writer, rpcName string, digManually, bytesAsBase64, bytesAsQuotedLiterals, bytesFromFile, rerunPrevious, addRepeatedManually bool) error {
+	return dm.CallRPCInteractively(ctx, w, rpcName, digManually, bytesAsBase64, bytesAsQuotedLiterals, bytesFromFile, rerunPrevious, addRepeatedManually)
 }
 
-func (m *dependencyManager) CallRPCInteractively(ctx context.Context, w io.Writer, rpcName string, digManually, bytesAsBase64, bytesFromFile, rerunPrevious, addRepeatedManually bool) error {
+func (m *dependencyManager) CallRPCInteractively(ctx context.Context, w io.Writer, rpcName string, digManually, bytesAsBase64, bytesAsQuotedLiterals, bytesFromFile, rerunPrevious, addRepeatedManually bool) error {
 	return m.CallRPC(ctx, w, rpcName, rerunPrevious, &interactiveFiller{
 		fillFunc: func(v interface{}) error {
 			return m.interactiveFiller.Fill(v, fill.InteractiveFillerOpts{
-				DigManually:         digManually,
-				BytesAsBase64:       bytesAsBase64,
-				BytesFromFile:       bytesFromFile,
-				AddRepeatedManually: addRepeatedManually,
+				DigManually:           digManually,
+				BytesAsBase64:         bytesAsBase64,
+				BytesAsQuotedLiterals: bytesAsQuotedLiterals,
+				BytesFromFile:         bytesFromFile,
+				AddRepeatedManually:   addRepeatedManually,
 			})
 		},
 	})