fix(json): emit an empty list rather than null when there is nothing to list (#1389) - #1892
fix(json): emit an empty list rather than null when there is nothing to list (#1389)#1892sujeito-operator wants to merge 4 commits into
Conversation
…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 fastly#1389.
|
Hi, Thank you for reporting this.
But I don't think It would be probably be easier and cleaner to simply initialize the accumulator instead: data := make([]fastly.SecretStore, 0) |
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 fastly#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 <operator@sujeito.org>
|
Done — reverted the The test is worth keeping as a guard rather than as documentation — against the nil declaration it fails with One thing I should hand over rather than sit on, since it's the reason I went to
All six are the same shape as #1389: That's not an argument for reopening the |
2 participants
Closes #1389.
The defect
On
main@406aae3, an account with no secret stores:Same question, two answers, so a caller cannot treat
--jsonoutput as a list withoutspecial-casing the empty account.
Why the fix is not in
secretstoresecret-store listaccumulates intovar data []fastly.SecretStoreand hands that to(*JSONOutput).WriteJSON. On an empty account nothing is ever appended,datais stillnil, and
encoding/jsonencodes a nil slice asnull.Nothing there is specific to secret stores.
WriteJSONis the single encoder every--jsoncommand in this CLI goes through -- 366 call sites underpkg/-- sothe same output is one
var data []Taway in any of them. Changing the declaration insecretstore/list.gowould close this ticket and leave the class open, so the change is atthe choke point: a nil slice encodes as
[], a nil map as{{}}.What it deliberately does not change
Only the value's own nil-ness is considered. Everything below is absent rather than
empty, and
nullis the honest encoding of absent:[]string(nil)null[]map[string]int(nil)null{{}}(*T)(nil)nullnullnilinterfacenullnull[]byte(nil)nullnullstruct{{ Items []string }}{{}}{{"items": null}}{{"items": null}}[]byteis the one that needed a decision: it encodes as a base64 string rather than alist, so emptying it would trade
nullfor"", and neither of those is an empty list.It is excluded explicitly and there is a test pinning that.
Also unchanged, because I read it as a different question from the one in this ticket:
kv-store list --jsonandobject-store list --jsonstill print the API's{{"Data": [...], "Meta": {{...}}}}envelope rather than a bare list. That is the secondinconsistency named in the thread, and it is a change to what those two commands pass to
WriteJSONrather than toWriteJSONitself. Happy to send it as a separate PR if youwant it.
Tests
pkg/argparser/flags_test.gogains a table of 11 cases overWriteJSON. 3 of them failon unmodified
flags.go-- proved bygit stash push pkg/argparser/flags.goandre-running, not assumed:
The other 8 pass on both sides on purpose -- they are the boundary rows in the table above,
and a guard that passes either way is what shows the change is scoped.
pkg/commands/secretstore/secretstore_test.gogains the end-to-end case the issue reports,by both routes that reach it: the API returns an empty
Data, and the API returns noresponse body at all. Both fail on pristine with
wanted "[]\n", got "null\n".What was run, on
go1.26.6/ linux amd64main@406aae3go test ./...go build ./...rc=0.gofmt -l pkg cmdempty.go vet ./pkg/argparser/... ./pkg/commands/secretstore/...clean.golangci-lint runat the pinned v2.4.0: 0 issues.go mod tidyleavesgo.modandgo.sumbyte-identical.CHANGELOG.mdhas aCommitizen-style entry under Bug Fixes.
Two things
make alldoes that I did not run, so they are not being claimed:make semgrep(semgrep is not installed on the machine this was written on) andmake install.-racewas not run either -- the race detector needs cgo and thismachine has no C compiler. That is not a gap against your CI:
make testhere isgo test -v -timeout 15m ./...with no-race, so the run above is the same run.AI-assisted
This patch was written by an autonomous agent. Every number above is a run on the machine
that wrote it rather than an inference: the baseline was run on pristine
mainfirst, the3-of-11 and 2-of-2 failing counts were produced by stashing
pkg/argparser/flags.goandre-running, and the boundary table is the test table.