From be034408a29369d3e432f1c57681be77874b56fa Mon Sep 17 00:00:00 2001 From: Sujeito Operator Date: Thu, 20 Aug 2026 15:06:01 +0000 Subject: [PATCH] fix(json): emit an empty list rather than null when there is nothing to list `secret-store list --json` printed `null` on an account with no secret stores while `config-store list --json` printed `[]` for the same situation, so `--json` output could not be treated as a list without special-casing the empty account. The cause is not in secretstore: commands accumulate into `var data []T` and hand that to (*JSONOutput).WriteJSON, and encoding/json writes a nil slice as null. WriteJSON is the one encoder every --json command goes through, so the same output is one `var data []T` away in any of them. Fixing the declaration in secretstore/list.go would close the ticket and leave the class open; this normalises at the choke point instead. Only the value's own nil-ness is considered. A nil pointer, a nil interface and nil fields inside a struct still encode as null, because they are absent rather than empty. A nil []byte is excluded too: it encodes as a base64 string, so emptying it would trade null for "", and neither is an empty list. 11 table cases over WriteJSON, 3 of which fail on the unmodified file, plus the end-to-end secret-store list --json case by both routes that reach it. Closes #1389. --- CHANGELOG.md | 2 + pkg/argparser/flags.go | 32 +++++- pkg/argparser/flags_test.go | 114 +++++++++++++++++++ pkg/commands/secretstore/secretstore_test.go | 24 ++++ 4 files changed, 171 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f6cbbda09..1a0ec341d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ ### Bug Fixes: +- fix(json): emit `[]` rather than `null` when a `--json` command has nothing to list + ### Enhancements: ### Dependencies: diff --git a/pkg/argparser/flags.go b/pkg/argparser/flags.go index 3e037fea0..451bbdaed 100644 --- a/pkg/argparser/flags.go +++ b/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) { diff --git a/pkg/argparser/flags_test.go b/pkg/argparser/flags_test.go index 55c67bead..324c9c091 100644 --- a/pkg/argparser/flags_test.go +++ b/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) + } + }) + } +} diff --git a/pkg/commands/secretstore/secretstore_test.go b/pkg/commands/secretstore/secretstore_test.go index 634b7ed3f..d46f6349f 100644 --- a/pkg/commands/secretstore/secretstore_test.go +++ b/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 {