fix(api): error on bracketed field names, send every query array value

Description

Bracketed field names now error where fields become a JSON request body, the shape @phikai proposed in the first round in place of nesting them. parseField returns the left side of the first = as a literal string, so -f 'position[base_sha]=abc' became the top-level JSON key position[base_sha]. The API ignores it: a discussion POST returns 201 with a plain note instead of the diff note it was asked for.

The message names what triggers it, a field name containing a bracket, and builds its advice from the name typed:

field name "position[base_sha]": a field name containing a bracket is not supported in a JSON request body; pass the value as JSON, for example -F 'position={"base_sha":"..."}', or use --input

On the query path that name keeps its literal meaning: Rack decodes it into a nested hash and the REST API documents that syntax, so the error fires only where fields become a JSON body.

The second half of this MR is the adjacent query path, which the first-round review named as a defect: -X GET -f 'ids[]=1' -f 'ids[]=2' sent only ids[]=2 and now sends both, in the order typed. One principle covers the three behaviours here: the last value wins within one flag and --field wins over --raw-field across flags, values accumulate where the wire format expresses repetition, and the request is rejected where the wire format cannot express it or two flags leave the intent ambiguous. Correcting the precedence rule across the two flags is scope beyond #8474 (closed), and I flagged it in the last round: the help text had said any repeated name sends only its last value, and -X GET -F 'a=2' -f 'a=1' has always sent ?a=2. All three consequences are in the help text as well as here:

  • A name ending in [] collects values, so --field does not override --raw-field of the same name, as it does for other names.
  • On single-value names the --field value wins in either flag order: -X GET -F 'a=2' -f 'a=1' sends ?a=2.
  • Mixing the two spellings of one wire key is an error: -f 'ids[]=1' with -F 'ids=[2,3]' says nothing about the order of the three values.

!3785 (merged) adds isQueryMethod in the same part of http.go this MR inserts queryList into, so whichever lands second resolves a small conflict there. That MR owns the predicate: fieldsBecomeJSONBody here inlines the same GET-or-DELETE test, and the second to land collapses it to !isQueryMethod(method).

Fixes #8474 (closed)

The --paginate re-append seen while testing the repeated key[] case is #8537 (closed); its fix is open as !3785 (merged).

How has this been tested?

  • go test ./internal/commands/api/... passes 267 tests including subtests, 0 failures; make lint 0 issues; make gen-docs clean.
  • The bracketed-name cases are rows against parseFields and parseQuery in Test_parseFields_queryString and Test_parseFields_bracketedFieldNameError, 331 lines where the seven end-to-end functions had been 523. Twelve stay end to end in Test_apiRun_bracketNameWiring for what only a whole run reaches.
  • gremlins unleash ./internal/commands/api/ reports 147 killed, 14 lived and 13 not covered over the package, and 27 killed, 1 lived, 0 not covered on the lines this MR changes. The one survivor there is an equivalent mutant: checkQueryKeyCollision orders the two names with second < first, and the two are always distinct keys, so <= cannot diverge from it.

Screenshots (if appropriate):

Not applicable.

Edited by Andrew Dunn

Merge request reports

Loading
Loading