cmd/tailscale/cli/jsonoutput: fix flag parsing for boolean values (#19996)

SchemaVersion didn’t actually parse boolean values properly, so calling `tailscale lock status --json=false` would fail with: invalid boolean value "false" for -json: invalid integer value passed to --json: "false" This patch makes SchemaVersion.Set delegate to flag.FlagSet for its argument parsing, with accompanying tests that ensure that both boolean and integer values are parsed properly. It also removes the restriction that prevented the flag from appearing multiple times in the arguments list. Now, the final flag clobbers all previous ones, aligning this behaviour with the standard flag package. We also change the SchemaVersion.String output for the zero value to "false", so that the default help message doesn’t change when we switch other commands over from their boolean representations: user@host:~$ tailscale whoami --help FLAGS --json, --json=false output in JSON format (default false) Updates #17613 Signed-off-by: Simon Law <sfllaw@tailscale.com>
2026-06-04 10:18:47 -07:00
parent 0bbaed6af4
commit 6ff761c5f8
2 changed files with 191 additions and 16 deletions
@@ -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
 }

@@ -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)
+			}
+		})
+	}
+}