fix(json): emit an empty list rather than null when there is nothing to list (#1389)

CHANGELOG.md

@@ -6,6 +6,8 @@
 
 ### Bug Fixes:
 
+- fix(json): emit `[]` rather than `null` when a `--json` command has nothing to list
+
 ### Enhancements:
 
 ### Dependencies:

pkg/argparser/flags.go

@@ -8,6 +8,7 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"reflect"
 	"regexp"
 	"sort"
 	"strconv"
@@ -385,7 +386,36 @@ func (j *JSONOutput) WriteJSON(out io.Writer, value any) (bool, error) {
 
 	enc := json.NewEncoder(out)
 	enc.SetIndent("", "  ")
-	return true, enc.Encode(value)
+	return true, enc.Encode(emptyNotNull(value))
+}
+
+// emptyNotNull substitutes an empty collection for a nil one so that a command
+// with nothing to report emits `[]` (or `{}`) rather than `null`.
+//
+// Commands accumulate their results into a `var data []T` and hand that to
+// WriteJSON. When the account holds no resources, `data` is still nil and
+// encoding/json writes `null`, so `--json` output is not consistently a JSON
+// list and a caller has to special-case the empty account.
+//
+// Only the value's own nil-ness is considered. A nil pointer, a nil interface
+// and a struct holding nil fields are all left exactly as they were: those
+// encode as `null` because they *are* absent, which is a different statement
+// from "an empty list". A nil []byte is left alone for the same reason -- it
+// encodes as a base64 string rather than a list, so emptying it would turn
+// `null` into `""`, which is not what an empty list looks like either.
+func emptyNotNull(value any) any {
+	v := reflect.ValueOf(value)
+	if !v.IsValid() {
+		return value // an untyped nil; there is no collection here to empty
+	}
+	if v.Kind() == reflect.Slice && v.IsNil() &&
+		v.Type().Elem().Kind() != reflect.Uint8 {
+		return reflect.MakeSlice(v.Type(), 0, 0).Interface()
+	}
+	if v.Kind() == reflect.Map && v.IsNil() {
+		return reflect.MakeMap(v.Type()).Interface()
+	}
+	return value
 }
 
 func ConvertBoolFromStringFlag(value string, argName string) (*bool, error) {

pkg/argparser/flags_test.go

@@ -419,3 +419,117 @@ func cloneVersionResult(version int) func(_ context.Context, i *fastly.CloneVers
 func errMatches(version int, err error) bool {
 	return err.Error() == fmt.Sprintf("service version %d is not editable", version)
 }
+
+func TestJSONOutputWriteJSON(t *testing.T) {
+	type payload struct {
+		Items []string `json:"items"`
+	}
+
+	for _, testcase := range []struct {
+		name    string
+		enabled bool
+		value   any
+		wantOK  bool
+		want    string
+	}{
+		{
+			name:    "disabled writes nothing",
+			enabled: false,
+			value:   []string{"a"},
+			wantOK:  false,
+			want:    "",
+		},
+		// The reported defect: a command that accumulated its results into a
+		// `var data []T` and found none hands a nil slice to WriteJSON.
+		{
+			name:    "nil slice is an empty list, not null",
+			enabled: true,
+			value:   []string(nil),
+			wantOK:  true,
+			want:    "[]\n",
+		},
+		{
+			name:    "nil slice of structs is an empty list, not null",
+			enabled: true,
+			value:   []payload(nil),
+			wantOK:  true,
+			want:    "[]\n",
+		},
+		{
+			name:    "an already empty slice is unchanged",
+			enabled: true,
+			value:   []string{},
+			wantOK:  true,
+			want:    "[]\n",
+		},
+		{
+			name:    "a populated slice is unchanged",
+			enabled: true,
+			value:   []string{"a", "b"},
+			wantOK:  true,
+			want:    "[\n  \"a\",\n  \"b\"\n]\n",
+		},
+		{
+			name:    "nil map is an empty object, not null",
+			enabled: true,
+			value:   map[string]int(nil),
+			wantOK:  true,
+			want:    "{}\n",
+		},
+		// The boundary. Everything below is genuinely absent rather than
+		// empty, and must keep encoding as null.
+		{
+			name:    "a nil pointer is absent and stays null",
+			enabled: true,
+			value:   (*payload)(nil),
+			wantOK:  true,
+			want:    "null\n",
+		},
+		{
+			name:    "a nil interface is absent and stays null",
+			enabled: true,
+			value:   nil,
+			wantOK:  true,
+			want:    "null\n",
+		},
+		{
+			name:    "a nil error is absent and stays null",
+			enabled: true,
+			value:   error(nil),
+			wantOK:  true,
+			want:    "null\n",
+		},
+		// A []byte encodes as a base64 string, not as a list, so emptying it
+		// would trade null for "" -- neither of which is an empty list.
+		{
+			name:    "a nil byte slice is not a list and stays null",
+			enabled: true,
+			value:   []byte(nil),
+			wantOK:  true,
+			want:    "null\n",
+		},
+		{
+			name:    "nil slices nested inside a struct are untouched",
+			enabled: true,
+			value:   payload{},
+			wantOK:  true,
+			want:    "{\n  \"items\": null\n}\n",
+		},
+	} {
+		t.Run(testcase.name, func(t *testing.T) {
+			var buf bytes.Buffer
+			j := argparser.JSONOutput{Enabled: testcase.enabled}
+
+			ok, err := j.WriteJSON(&buf, testcase.value)
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if ok != testcase.wantOK {
+				t.Errorf("wanted ok %v, got %v", testcase.wantOK, ok)
+			}
+			if got := buf.String(); got != testcase.want {
+				t.Errorf("wanted %q, got %q", testcase.want, got)
+			}
+		})
+	}
+}

pkg/commands/secretstore/secretstore_test.go

@@ -341,6 +341,30 @@ func TestListStoresCommand(t *testing.T) {
 			wantAPIInvoked: true,
 			wantOutput:     fstfmt.EncodeJSON([]fastly.SecretStore{stores.Data[0]}),
 		},
+		// An account with no secret stores used to print `null` here while
+		// `config-store list --json` printed `[]` for the same situation.
+		{
+			args: "list --json",
+			api: mock.API{
+				ListSecretStoresFn: func(_ context.Context, _ *fastly.ListSecretStoresInput) (*fastly.SecretStores, error) {
+					return &fastly.SecretStores{Data: []fastly.SecretStore{}}, nil
+				},
+			},
+			wantAPIInvoked: true,
+			wantOutput:     "[]\n",
+		},
+		// The same, reached by the other route: the API returned no response
+		// body at all, so nothing was ever appended.
+		{
+			args: "list --json",
+			api: mock.API{
+				ListSecretStoresFn: func(_ context.Context, _ *fastly.ListSecretStoresInput) (*fastly.SecretStores, error) {
+					return nil, nil
+				},
+			},
+			wantAPIInvoked: true,
+			wantOutput:     "[]\n",
+		},
 	}
 
 	for _, testcase := range scenarios {