From be034408a29369d3e432f1c57681be77874b56fa Mon Sep 17 00:00:00 2001 From: Sujeito Operator Date: Thu, 20 Aug 2026 15:06:01 +0000 Subject: [PATCH 1/2] 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 { From 9ae8562672a01b575c43d21f8c4e4df3e23b565e Mon Sep 17 00:00:00 2001 From: Sujeito Operator Date: Mon, 24 Aug 2026 14:10:06 +0000 Subject: [PATCH 2/2] fix(json): initialise the secret-store list accumulator Review preferred initialising the accumulator over normalising in WriteJSON, on the grounds that WriteJSON affects everything. Done: the change to pkg/argparser is reverted and secretstore/list.go declares data := make([]fastly.SecretStore, 0) so an account with no secret stores encodes as [] rather than null, matching config-store list --json. Closes #1389. The end-to-end test is kept and is a real guard: it fails with "null\n" doesn't contain "[]\n" against the nil declaration and passes with the initialised one, by both routes that reach WriteJSON. Signed-off-by: Sujeito Operator --- CHANGELOG.md | 2 +- pkg/argparser/flags.go | 32 +-------- pkg/argparser/flags_test.go | 114 ------------------------------- pkg/commands/secretstore/list.go | 5 +- 4 files changed, 6 insertions(+), 147 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a0ec341d..212a510a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ ### Bug Fixes: -- fix(json): emit `[]` rather than `null` when a `--json` command has nothing to list +- fix(json): `secret-store list --json` emits `[]` rather than `null` on an account with no secret stores ### Enhancements: diff --git a/pkg/argparser/flags.go b/pkg/argparser/flags.go index 451bbdaed..3e037fea0 100644 --- a/pkg/argparser/flags.go +++ b/pkg/argparser/flags.go @@ -8,7 +8,6 @@ import ( "io" "os" "path/filepath" - "reflect" "regexp" "sort" "strconv" @@ -386,36 +385,7 @@ func (j *JSONOutput) WriteJSON(out io.Writer, value any) (bool, error) { enc := json.NewEncoder(out) enc.SetIndent("", " ") - 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 + return true, enc.Encode(value) } func ConvertBoolFromStringFlag(value string, argName string) (*bool, error) { diff --git a/pkg/argparser/flags_test.go b/pkg/argparser/flags_test.go index 324c9c091..55c67bead 100644 --- a/pkg/argparser/flags_test.go +++ b/pkg/argparser/flags_test.go @@ -419,117 +419,3 @@ 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/list.go b/pkg/commands/secretstore/list.go index 769676469..be35ae6bd 100644 --- a/pkg/commands/secretstore/list.go +++ b/pkg/commands/secretstore/list.go @@ -45,7 +45,10 @@ func (c *ListCommand) Exec(in io.Reader, out io.Writer) error { return fsterr.ErrInvalidVerboseJSONCombo } - var data []fastly.SecretStore + // Initialised rather than declared nil so that an account with no secret + // stores encodes as `[]` instead of `null`: encoding/json writes a nil + // slice as null, and `--json` output should be a list either way. + data := make([]fastly.SecretStore, 0) for { o, err := c.Globals.APIClient.ListSecretStores(context.TODO(), &c.Input)