diff --git a/cmd/tailscale/cli/jsonoutput/jsonoutput.go b/cmd/tailscale/cli/jsonoutput/jsonoutput.go index e89a66f13..41b605eea 100644 --- a/cmd/tailscale/cli/jsonoutput/jsonoutput.go +++ b/cmd/tailscale/cli/jsonoutput/jsonoutput.go @@ -23,8 +23,9 @@ package jsonoutput import ( "errors" "flag" - "fmt" + "io" "strconv" + "strings" ) var _ flag.Value = &SchemaVersion{} @@ -46,32 +47,42 @@ type SchemaVersion struct { func (v *SchemaVersion) String() string { if v.IsSet { return strconv.Itoa(v.Version) - } else { - return "(not set)" } + return strconv.FormatBool(false) } // Set is called when the user passes the flag as a command-line argument. func (v *SchemaVersion) Set(s string) error { - if v.IsSet { - return errors.New("received multiple instances of --json; only pass it once") + // Delegate to a FlagSet to parse this as both a BoolVar and an IntVar. + // This is less efficient than copying the implementation from the standard library + // but this design makes it likelier that Set will inherit any upstream fixes. + fs := flag.NewFlagSet("", flag.ContinueOnError) + fs.BoolVar(&v.IsSet, "bool", false, "") + fs.IntVar(&v.Version, "int", 0, "") + fs.SetOutput(io.Discard) // silence + + // First, try to parse as an IntVar to handle -flag=INT. + // This order is important because -bool=0 will parse as false. + if err := fs.Parse([]string{"-int=" + s}); err == nil { + v.IsSet = true + return nil + } + // If that fails, parse as a BoolVar to handle -flag and -flag=false. + // This is checked last for compatibility with the boolean -json flag. + if err := fs.Parse([]string{"-bool=" + s}); err != nil { + // Unwrap the header added by FlagSet.failf: + // `invalid boolean value "invalid" for -bool: ` + bits := strings.SplitN(err.Error(), ": ", 2) + return errors.New(bits[len(bits)-1]) } - - v.IsSet = true - // If the user doesn't supply a schema version, default to 1. // This ensures that any existing scripts will continue to get their // current output. - if s == "true" { + if v.IsSet { v.Version = 1 - return nil + } else { + v.Version = 0 // if unset, zero out the Version } - - version, err := strconv.Atoi(s) - if err != nil { - return fmt.Errorf("invalid integer value passed to --json: %q", s) - } - v.Version = version return nil } diff --git a/cmd/tailscale/cli/jsonoutput/jsonoutput_test.go b/cmd/tailscale/cli/jsonoutput/jsonoutput_test.go new file mode 100644 index 000000000..595fe2a12 --- /dev/null +++ b/cmd/tailscale/cli/jsonoutput/jsonoutput_test.go @@ -0,0 +1,164 @@ +// Copyright (c) Tailscale Inc & contributors +// SPDX-License-Identifier: BSD-3-Clause + +package jsonoutput_test + +import ( + "flag" + "math" + "testing" + + gcmp "github.com/google/go-cmp/cmp" + "github.com/kballard/go-shellquote" + + "tailscale.com/cmd/tailscale/cli/jsonoutput" +) + +func TestSchemaVersion(t *testing.T) { + for _, tc := range []struct { + name string + args string + want jsonoutput.SchemaVersion + wantErr string + wantStr string + }{ + { + name: "none", + want: jsonoutput.SchemaVersion{IsSet: false, Version: 0}, + wantStr: "false", + }, + { + name: "default", + args: "-got", + want: jsonoutput.SchemaVersion{IsSet: true, Version: 1}, + wantStr: "1", + }, + { + name: "true", + args: "-got=true", + want: jsonoutput.SchemaVersion{IsSet: true, Version: 1}, + wantStr: "1", + }, + { + name: "false", + args: "-got=false", + want: jsonoutput.SchemaVersion{IsSet: false, Version: 0}, + wantStr: "false", + }, + { + // Test that -got=0 isn’t interpreted as -bool=0, i.e. false. + name: "zero_not_false", + args: "-got=0", + want: jsonoutput.SchemaVersion{IsSet: true, Version: 0}, + wantStr: "0", + }, + { + name: "one", + args: "-got=1", + want: jsonoutput.SchemaVersion{IsSet: true, Version: 1}, + wantStr: "1", + }, + { + name: "two", + args: "-got=2", + want: jsonoutput.SchemaVersion{IsSet: true, Version: 2}, + wantStr: "2", + }, + { + name: "max", + args: "-got=2147483647", + want: jsonoutput.SchemaVersion{IsSet: true, Version: math.MaxInt32}, + wantStr: "2147483647", + }, + { + name: "min", + args: "-got=-2147483648", + want: jsonoutput.SchemaVersion{IsSet: true, Version: math.MinInt32}, + wantStr: "-2147483648", + }, + { + name: "invalid", + args: "-got=invalid", + wantErr: `invalid boolean value "invalid" for -got: parse error`, + }, + { + name: "float", + args: "-got=1.3", + wantErr: `invalid boolean value "1.3" for -got: parse error`, + }, + { + name: "space", + args: "-got=' '", + wantErr: `invalid boolean value " " for -got: parse error`, + }, + { + name: "trailing_space", + args: "-got='1 '", + wantErr: `invalid boolean value "1 " for -got: parse error`, + }, + } { + args, err := shellquote.Split(tc.args) + if err != nil { + t.Fatalf("broken args %q: %v", tc.args, err) + } + + // Test both Set and String methods. + t.Run(tc.name, func(t *testing.T) { + var got jsonoutput.SchemaVersion + fs := flag.NewFlagSet("name", flag.ContinueOnError) + fs.Var(&got, "got", "usage") + + err = fs.Parse(args) + if err != nil && tc.wantErr == "" { + t.Errorf("parse error: %v", err) + } else if err != nil && err.Error() != tc.wantErr { + t.Errorf("parse error mismatch: %q, want %q", err, tc.wantErr) + } else if err == nil && tc.wantErr != "" { + t.Errorf("parse error: %v, want %q", err, tc.wantErr) + } + + if len(fs.Args()) != 0 { + t.Errorf("unexpected positional arguments: %q", fs.Args()) + } + + if diff := gcmp.Diff(tc.want, got); diff != "" { + t.Errorf("parse mismatch: -want +got\n%s", diff) + } + + if s := got.String(); s != tc.wantStr && tc.wantStr != "" { + t.Errorf("string %q, want %q", s, tc.wantStr) + } + }) + + if tc.args == "" { + continue // nothing to clobber + } + if tc.wantErr != "" { + continue // clobbering will just trigger another error + } + + // The last -got flag will clobber all previous -got flags. + t.Run(tc.name+"/clobber", func(t *testing.T) { + var got jsonoutput.SchemaVersion + fs := flag.NewFlagSet("name", flag.ContinueOnError) + fs.Var(&got, "got", "usage") + + sentinel := []string{"-got=-1"} + if err := fs.Parse(append(sentinel, args...)); err != nil { + t.Errorf("parse error: %v", err) + } + + if got.Version == -1 { + t.Errorf("sentinel detected: flag didn’t clobber") + } + + if len(fs.Args()) != 0 { + t.Errorf("unexpected positional arguments: %q", fs.Args()) + } + + if diff := gcmp.Diff(tc.want, got); diff != "" { + t.Errorf("parse mismatch: -want +got\n%s", diff) + } + }) + } +}